RE: changes today.

[YhLu]

Ron/Eric/Stephan/Ollie,

Please check the patch.

1. some tyan volatile messing because of ROMCC changes.
2. S2882 interrupt line assignment in mptable.c
3. exclude range for flash_rom.
4. better ht_setup_chainx() in incoherent_ht.c ---> change share bus 0 to
different bus for different incoherent HT link.
5. fix one bug about io and mem allocation for multi ht links. --- in
northbridge.c
    Mark it if used before assign final value.
6. enable amd/quartet to init multi incoherent HT link in auto.c --- not
test, no MB on hand.
7. enable S2885 to init multi incoherent HT link in auto.c

Regards

YH

changes today.

[Stefan Reinauer]

Hi Yinghai,
> S2882 interrupt line assignment in mptable.c
> exclude range for flash_rom.
> better ht_setup_chainx() in incoherent_ht.c ---> change share bus 0 to
>
> different bus for different incoherent HT link.
> 5. fix one bug about io and mem allocation for multi ht links. --- in
> northbridge.c
>    Mark it if used before assign final value.
> 6. enable amd/quartet to init multi incoherent HT link in auto.c --- not
> test, no MB on hand.
> 7. enable S2885 to init multi incoherent HT link in auto.c
> diff -uNr ./freebios2/src/mainboard/amd/quartet/auto.c ../freebios2/src/mainboard/amd/quartet/auto.c
> --- ./freebios2/src/mainboard/amd/quartet/auto.c	2004-04-28 01:08:06.000000000 -0700
> +++ ../freebios2/src/mainboard/amd/quartet/auto.c	2004-09-29 09:30:57.000000000 -0700
> @@ -100,11 +100,12 @@
>     */
>
> uint32_t ret = 0x00010101;	/* default row entry */
>
> +/*
>    static const unsigned int rows_2p[2][2] = {
>    	{0x00030101, 0x00010202},
>    	{0x00010202, 0x00030101}
>    };
> +*/
>
> static const unsigned int rows_4p[4][4] = {
>    	{0x00070101, 0x00010202, 0x00030404, 0x00010204},
> @@ -114,9 +115,11 @@
>    };
>
> if (!(node >= maxnodes || row >= maxnodes)) {
> +/*
>    	if (maxnodes == 2)
>    		ret = rows_2p[node][row];
>    	if (maxnodes == 4)
> +*/
>    		ret = rows_4p[node][row];
>    }

I am in doubt this is a good idea. It will likely break 4p systems with
only 2 cpus installed. The better idea might be to mask the according
links decently.

Which reminds me that we could do this a lot nicer dynamically with a
little more stack depth available (ie with working CAR)
> @@ -199,6 +203,20 @@
>    };
>    static const struct ht_chain ht_c[] = {
>            {  /* Link 2 of CPU0 */
>                    .udev = PCI_DEV(0, 0x18, 0),
>                    .upos = 0xc0,
>                    .devreg = 0xe0,  /* Preset bus num in resource map */
>            }, 
>            {  /* Link 1 of CPU1 */
>                    .udev = PCI_DEV(0, 0x19, 0),
>                    .upos = 0xa0,  
>                    .devreg = 0xe4,  /* Preset bus num in resource map */
>            },
>    };  
>
> int needs_reset;
>
> enable_lapic();

Can we generate above struct from the information we have in the config
tool? at least the cpu link should be there. What's the bus num again?
> asm("jmp __normal_image"
> asm volatile ("jmp __normal_image"

Can't this be fixed in romcc instead? If we have to put an asm volatile
statement instead of every asm, we could as well just do
#define asm asm volatile 
or the adequate change in the romcc code
> diff -uNr ./freebios2/src/mainboard/tyan/s2882/irq_tables.c ../freebios2/src/mainboard/tyan/s2882/irq_tables.c
> --- ./freebios2/src/mainboard/tyan/s2882/irq_tables.c	2004-03-12 07:13:31.000000000 -0800
> +++ ../freebios2/src/mainboard/tyan/s2882/irq_tables.c	2004-07-02 10:01:48.000000000 -0700
> @@ -18,7 +18,7 @@
>    0x746b,         /* Device */
>    0,         /* Crap (miniport) */
>    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, /* u8 rfu[11] */
> 0x8d,         /*  u8 checksum , this hase to set to some value that would give 0 after the sum of all bytes for this structure (including checksum) */
> 0xff,         /*  u8 checksum , this hase to set to some value that would give 0 after the sum of all bytes for this structure (including checksum) */
> {
> {1,(4<<3)|0, {{0x1, 0xdef8}, {0x2, 0xdef8}, {0x3, 0xdef8}, {0x4, 0xdef8}}, 0, 0},
> {0x4,0, {{0, 0}, {0, 0}, {0, 0}, {0x4, 0xdef8}}, 0, 0},

To get this right, does Linux expect the PIRQ table to be in ROM in any
condition? The code will correct the table's checksum in RAM, and the
goal should definitely be to create all the table in ram during bootup.
I saw phoenix bios almost writes a nice dynamic device tree into the 
pirq table, including CPUs and busses, instead of just putting the most 
necessary lines for the pci slots in. This sounds like a way to go.
> mptable.c
> +#if ASSIGN_IRQ 
>    smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, 0x1, (4<<2)|3, 0x2, 0x13);
> {
> device_t dev;
>    	dev = dev_find_device(PCI_VENDOR_ID_AMD, 0x746b, 0);
>    	if (dev) {
>            	/* initialize PCI interupts - these assignments depend
>               	on the PCB routing of PINTA-D 
>               	PINTA = IRQ5
>               	PINTB = IRQ9
>               	PINTC = IRQ11
>               	PINTD = IRQ10
>            	*/
>            	pci_write_config16(dev, 0x56, 0xab95);
> }
>    }
>
> +#endif
>
> +#if ASSIGN_IRQ
>    printk_info("setting Onboard AMD Southbridge \n");
>    static const unsigned char slotIrqs_1_4[4] = { 5, 9, 11, 10 };
>    pci_assign_irqs(1, 4, slotIrqs_1_4);
>
> +#endif

Does this belong into the mptable generation? I would assume it better
fits into the "driver" code pieces

Otherwise I see that we have to shift all motherboards to use the above
mechanism, ending up with dozens of half broken mp tables (worse than it
is now already)
> diff -uNr ./freebios2/src/mainboard/tyan/s2885/resourcemap.c ../freebios2/src/mainboard/tyan/s2885/resourcemap.c
> --- ./freebios2/src/mainboard/tyan/s2885/resourcemap.c	2004-03-26 13:34:04.000000000 -0800
> +++ ../freebios2/src/mainboard/tyan/s2885/resourcemap.c	2004-09-20 11:00:09.000000000 -0700
> @@ -252,8 +252,8 @@
>    	 * [31:24] Bus Number Limit i
>    	 *	   This field defines the highest bus number in configuration regin i
>    	 */
> PCI_ADDR(0, 0x18, 1, 0xE0), 0x0000FC88, 0x06010207,
> PCI_ADDR(0, 0x18, 1, 0xE4), 0x0000FC88, 0x00000007,
> PCI_ADDR(0, 0x18, 1, 0xE0), 0x0000FC88, 0x06000203, // AMD 8111 on link2 of CPU 0
> PCI_ADDR(0, 0x18, 1, 0xE4), 0x0000FC88, 0x08070003, // AMD 8151 on link0 of CPU 0
> PCI_ADDR(0, 0x18, 1, 0xE8), 0x0000FC88, 0x00000000,
> PCI_ADDR(0, 0x18, 1, 0xEC), 0x0000FC88, 0x00000000,
> };

I remember I commited some code a long time ago that detected the
southbridge link and set it correctly. Looks like it got dumped by
accident. We should do this again, and the same for the 8151.
> diff -uNr ./freebios2/src/mainboard/tyan/s4880/auto.c ../freebios2/src/mainboard/tyan/s4880/auto.c
> --- ./freebios2/src/mainboard/tyan/s4880/auto.c	2004-04-24 16:01:19.000000000 -0700
> +++ ../freebios2/src/mainboard/tyan/s4880/auto.c	2004-08-31 18:57:46.000000000 -0700
> @@ -28,7 +28,7 @@
>          set_bios_reset();
>      /* enable cf9 */
>    pci_write_config8(PCI_DEV(1, 0x04, 3), 0x41, 0xf1);
>    pci_write_config8(PCI_DEV(0, 0x04, 3), 0x41, 0xf1);

Is this not configurable in the config file, as well, by now, in some
places? We need to factorize this kind of code a lot more.. Every change
hurts, because the opteron board support code is drifting in many
directions
> @@ -214,7 +214,7 @@
>          console_init();
>          setup_s4880_resource_map();
>          needs_reset = setup_coherent_ht_domain();
>    needs_reset |= ht_setup_chain(PCI_DEV(0, 0x18, 0), 0x80);
>    needs_reset |= ht_setup_chain(PCI_DEV(0, 0x18, 0), 0xc0);

Why are you not using the same struct as above for the s27xx?
> diff -uNr ./freebios2/src/northbridge/amd/amdk8/incoherent_ht.c ../freebios2/src/northbridge/amd/amdk8/incoherent_ht.c
> --- ./freebios2/src/northbridge/amd/amdk8/incoherent_ht.c	2004-04-15 10:33:21.000000000 -0700
> +++ ../freebios2/src/northbridge/amd/amdk8/incoherent_ht.c	2004-08-24 12:03:10.000000000 -0700
> @@ -247,19 +263,21 @@
>    unsigned upos;
>    unsigned devreg; 
>  };
>
> -static int ht_setup_chainx(device_t udev, unsigned upos, unsigned next_unitid)
> +static int ht_setup_chainx(device_t udev, unsigned upos, unsigned bus)
>  {
> unsigned last_unitid;
>
> unsigned next_unitid, last_unitid;
> unsigned uoffs;
> int reset_needed=0;
>
> uoffs = PCI_HT_HOST_OFFS;
>
> next_unitid = 1;
>
> do {
>   uint32_t id;
>   uint8_t pos;
>   unsigned flags, count;
> device_t dev = PCI_DEV(0, 0, 0);
> device_t dev = PCI_DEV(bus, 0, 0);
>
> last_unitid = next_unitid;
>
> id = pci_read_config32(dev, PCI_VENDOR_ID);
>
> @@ -293,8 +328,7 @@
>    	next_unitid += count;
>
> } while((last_unitid != next_unitid) && (next_unitid <= 0x1f));
> if(reset_needed!=0) next_unitid |= 0xffff0000;
> return next_unitid;
> return reset_needed;
>
> }

Did we not support icht chains hanging off any bus other than 0 before?!
Can you add some comments to the changes you made to incoherent_ht.c ?

This is some very fundamental piece of code that should be well
understood.
Same for the northbridge.c code.

Stefan

[BULK] Re: changes today.

Right now I feel like taffy, stretched thin and pulled in all directions.
My apologies for not responding sooner.

Stefan Reinauer  writes:
> Hi Yinghai,
> > S2882 interrupt line assignment in mptable.c
> > exclude range for flash_rom.
> > better ht_setup_chainx() in incoherent_ht.c ---> change share bus 0 to
> >
> > different bus for different incoherent HT link.
> > 5. fix one bug about io and mem allocation for multi ht links. --- in
> > northbridge.c
> >    Mark it if used before assign final value.
> > 6. enable amd/quartet to init multi incoherent HT link in auto.c --- not
> > test, no MB on hand.
> > 7. enable S2885 to init multi incoherent HT link in auto.c
> > diff -uNr ./freebios2/src/mainboard/amd/quartet/auto.c
>
> ../freebios2/src/mainboard/amd/quartet/auto.c
> > --- ./freebios2/src/mainboard/amd/quartet/auto.c 2004-04-28 01:08:06.000000000
>
> -0700
> > +++ ../freebios2/src/mainboard/amd/quartet/auto.c 2004-09-29
>
> 09:30:57.000000000 -0700
> > @@ -100,11 +100,12 @@
> >     */
> >
> > uint32_t ret = 0x00010101;	/* default row entry */
> >
> > +/*
> >    static const unsigned int rows_2p[2][2] = {
> >    	{0x00030101, 0x00010202},
> >    	{0x00010202, 0x00030101}
> >    };
> > +*/
> >
> > static const unsigned int rows_4p[4][4] = {
> >    	{0x00070101, 0x00010202, 0x00030404, 0x00010204},
> > @@ -114,9 +115,11 @@
> >    };
> >
> > if (!(node >= maxnodes || row >= maxnodes)) {
> > +/*
> >    	if (maxnodes == 2)
> >    		ret = rows_2p[node][row];
> >    	if (maxnodes == 4)
> > +*/
> >    		ret = rows_4p[node][row];
> >    }
>
> I am in doubt this is a good idea. It will likely break 4p systems with
> only 2 cpus installed. The better idea might be to mask the according
> links decently.

Which reminds me I need to push my patch that properly handles the case
of not having all of your cpus plugged in.
> Which reminds me that we could do this a lot nicer dynamically with a
> little more stack depth available (ie with working CAR)

Quite possibly.  I'm not holding my breath on this one.
> > @@ -199,6 +203,20 @@
> >    };
> >    static const struct ht_chain ht_c[] = {
> >            {  /* Link 2 of CPU0 */
> >                    .udev = PCI_DEV(0, 0x18, 0),
> >                    .upos = 0xc0,
> > .devreg = 0xe0, /* Preset bus num in resource map */
> >            }, 
> >            {  /* Link 1 of CPU1 */
> >                    .udev = PCI_DEV(0, 0x19, 0),
> >                    .upos = 0xa0,  
> > .devreg = 0xe4, /* Preset bus num in resource map */
> >            },
> >    };  
> >
> > int needs_reset;
> >
> > enable_lapic();
>
> Can we generate above struct from the information we have in the config
> tool? at least the cpu link should be there. What's the bus num again?

Ideally we could just put a loop through the HT links on the cpus
and if the link is up and not coherent enumerate it.  I suspect that
would be simples.
> > asm("jmp __normal_image"
> > asm volatile ("jmp __normal_image"
>
> Can't this be fixed in romcc instead? If we have to put an asm volatile
> statement instead of every asm, we could as well just do
> #define asm asm volatile 
> or the adequate change in the romcc code

The rules with romcc are exactly the same as the rules in gcc.

The rdtsc case I'm not certain about.  But for the rest romcc
will optimize out a non-volatile asm that has neither inputs
or outputs.   If romcc knew the jumps affected control flow
there would not be a problem.  Since it does not those statements
need to be volatile.
> > diff -uNr ./freebios2/src/mainboard/tyan/s2882/irq_tables.c
>
> ../freebios2/src/mainboard/tyan/s2882/irq_tables.c
> > --- ./freebios2/src/mainboard/tyan/s2882/irq_tables.c 2004-03-12
>
> 07:13:31.000000000 -0800
> > +++ ../freebios2/src/mainboard/tyan/s2882/irq_tables.c 2004-07-02
>
> 10:01:48.000000000 -0700
> > @@ -18,7 +18,7 @@
> >    0x746b,         /* Device */
> >    0,         /* Crap (miniport) */
> >    { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, /* u8 rfu[11] */
> > 0x8d, /* u8 checksum , this hase to set to some value that would give 0
>
> after the sum of all bytes for this structure (including checksum) */
> > 0xff, /* u8 checksum , this hase to set to some value that would give 0
>
> after the sum of all bytes for this structure (including checksum) */
> > {
> > {1,(4<<3)|0, {{0x1, 0xdef8}, {0x2, 0xdef8}, {0x3, 0xdef8}, {0x4, 0xdef8}}, 0,
>
> 0},
> > {0x4,0, {{0, 0}, {0, 0}, {0, 0}, {0x4, 0xdef8}}, 0, 0},
>
> To get this right, does Linux expect the PIRQ table to be in ROM in any
> condition? The code will correct the table's checksum in RAM, and the
> goal should definitely be to create all the table in ram during bootup.
> I saw phoenix bios almost writes a nice dynamic device tree into the 
> pirq table, including CPUs and busses, instead of just putting the most 
> necessary lines for the pci slots in. This sounds like a way to go.

Linux looks at 0xf00000 which is either ROM or RAM.  I'm looking at finding
the time to do a complete rewrite based on our dynamic device tree.  What
exists currently is a major pain.
> > mptable.c
> > +#if ASSIGN_IRQ 
> > smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, 0x1,
>
> (4<<2)|3, 0x2, 0x13);
> > {
> > device_t dev;
> >    	dev = dev_find_device(PCI_VENDOR_ID_AMD, 0x746b, 0);
> >    	if (dev) {
> >            	/* initialize PCI interupts - these assignments depend
> >               	on the PCB routing of PINTA-D 
> >               	PINTA = IRQ5
> >               	PINTB = IRQ9
> >               	PINTC = IRQ11
> >               	PINTD = IRQ10
> >            	*/
> >            	pci_write_config16(dev, 0x56, 0xab95);
> > }
> >    }
> >
> > +#endif
> >
> > +#if ASSIGN_IRQ
> >    printk_info("setting Onboard AMD Southbridge \n");
> >    static const unsigned char slotIrqs_1_4[4] = { 5, 9, 11, 10 };
> >    pci_assign_irqs(1, 4, slotIrqs_1_4);
> >
> > +#endif
>
> Does this belong into the mptable generation? I would assume it better
> fits into the "driver" code pieces
>
> Otherwise I see that we have to shift all motherboards to use the above
> mechanism, ending up with dozens of half broken mp tables (worse than it
> is now already)

True but at least at the moment it is motherboard specific code.  So
it is ``only'' a bad example.  This is equally doable with just
the pirq table, but the result (setting the interrupt line register
is desirable.
> I remember I commited some code a long time ago that detected the
> southbridge link and set it correctly. Looks like it got dumped by
> accident. We should do this again, and the same for the 8151.

Hmm.  Something like that surely.
> > diff -uNr ./freebios2/src/mainboard/tyan/s4880/auto.c
>
> ../freebios2/src/mainboard/tyan/s4880/auto.c
> > --- ./freebios2/src/mainboard/tyan/s4880/auto.c 2004-04-24 16:01:19.000000000
>
> -0700
> > +++ ../freebios2/src/mainboard/tyan/s4880/auto.c 2004-08-31 18:57:46.000000000
>
> -0700
> > @@ -28,7 +28,7 @@
> >          set_bios_reset();
> >      /* enable cf9 */
> >    pci_write_config8(PCI_DEV(1, 0x04, 3), 0x41, 0xf1);
> >    pci_write_config8(PCI_DEV(0, 0x04, 3), 0x41, 0xf1);
>
> Is this not configurable in the config file, as well, by now, in some
> places? We need to factorize this kind of code a lot more.. Every change
> hurts, because the opteron board support code is drifting in many
> directions

Something things we do need to setup early, and resets look like one
of them.
> > +static int ht_setup_chainx(device_t udev, unsigned upos, unsigned bus)
> >  {
> > unsigned last_unitid;
> >
> > unsigned next_unitid, last_unitid;
> > unsigned uoffs;
> > int reset_needed=0;
> >
> > uoffs = PCI_HT_HOST_OFFS;
> >
> > next_unitid = 1;
> >
> > do {
> >   uint32_t id;
> >   uint8_t pos;
> >   unsigned flags, count;
> > device_t dev = PCI_DEV(0, 0, 0);
> > device_t dev = PCI_DEV(bus, 0, 0);
> >
> > last_unitid = next_unitid;
> >
> > id = pci_read_config32(dev, PCI_VENDOR_ID);
> >
> > @@ -293,8 +328,7 @@
> >    	next_unitid += count;
> >
> > } while((last_unitid != next_unitid) && (next_unitid <= 0x1f));
> > if(reset_needed!=0) next_unitid |= 0xffff0000;
> > return next_unitid;
> > return reset_needed;
> >
> > }
>
> Did we not support icht chains hanging off any bus other than 0 before?!
> Can you add some comments to the changes you made to incoherent_ht.c ?

Stefan it is not devices hanging of bus 0.  It is what bus number the chain
will be assigned.  For the same reason the generic code does not use
bus 0, this code is simplest if we use another bus number as well.
> This is some very fundamental piece of code that should be well
> understood. Same for the northbridge.c code.

What is happening in northbridge.c is a legitimate bug fix.  Because
we may not set the resource for a while.  I would prefer
not to use the pci configuration registers as scratch registers, but
that may be the simplest way to do things.

Eric

[BULK] Re: changes today.

[Richard Smith]

Eric W. Biederman wrote:
> Right now I feel like taffy, stretched thin and pulled in all directions.
> My apologies for not responding sooner.

Amen. I'm with you.

changes today.

[Ronald G. Minnich]

On Sat, 2 Oct 2004, Stefan Reinauer wrote:
> Hi Yinghai,

Yinghai, there seem to be a few questions on this patch, can you let Ollie 
and me go over it next week? He is back then. Please don't commit yet.

thanks

ron

changes today.

[Stefan Reinauer]

* YhLu  [040930 21:00]:
> Ron/Eric/Stephan/Ollie,
>
> Please check the patch.
> some tyan volatile messing because of ROMCC changes.
> S2882 interrupt line assignment in mptable.c
> exclude range for flash_rom.
> better ht_setup_chainx() in incoherent_ht.c ---> change share bus 0 to
>
> different bus for different incoherent HT link.
> 5. fix one bug about io and mem allocation for multi ht links. --- in
> northbridge.c
>    Mark it if used before assign final value.
> 6. enable amd/quartet to init multi incoherent HT link in auto.c --- not
> test, no MB on hand.
> 7. enable S2885 to init multi incoherent HT link in auto.c

Have all these been commited now?

Stefan