Patchwork mainboard.c: init() still being called?

Hi Jens,

On 8/16/10 3:57 PM, Jens Rottmann wrote:
> The old code defined a config byte (for some GPIOs) in struct
> mainboard_lippert_spacerunner_lx_config (chip.h), which used to be set in
> Config.lb with a simple register "..." = "..." statement and used in
> mainboard.c's init() by casting dev->chip_info to struct
> mainboard_lippert_spacerunner_lx_config.
>
> The current code still has the struct (renamed to mainboard_config), but while
> changing Config.lb to devicetree.cb the register statement got dropped so
> there's no longer any value assigned to the config byte. I tried to re-add the
> register line where it was before (before chip northbridge/...) but this seems
> no longer to be allowed. And more importantly, init() which uses the config byte
> is still there but does not seem to be executed any more. At least I couldn't
> see its debug output.
Does the following patch fix the problem for you?


Stefan

Hi,

Myles Watson wrote:
> Sorry about the breakage.  Things have changed quite a bit.

No need to apologize, 2000 commits is really impressive, no one can expect this
to be possible without occasional hiccups, especially with code that doesn't get
tested.  It's really great what has become of coreboot!! Most visible to me (so
far) is Kconfig which makes things a lot less scary than they used to be and
Geode-LX VGA support.

> The attached patch calls init() again.

Indeed, it does. Excellent, thanks a lot!!!  :)

Stefan Reinauer wrote:
> Does the following patch fix the problem for you?

Umm ... nope, neither with nor without Myles's patch:

Reading resources...
Unknown device path type: -1
 read_resources bus cd6 link: 15
Unexpected Exception: 13 @ 10:0000674e - Halting
Code: 0 eflags: 00010086
eax: ffffffff ebx: 0000000f ecx: 0000fedf edx: 00000003
edi: 00000000 esi: 4623fc0f ebp: 0000fbeb esp: 0001ff14

But ...

Myles Watson wrote:
> I think that the value that was left out of devicetree.cb should move to
> the mainboard init() or into the device that's being configured (cs5536.)

In fact I was already wondering whether the whole code in mainboard init() is
actually at the right place.  That's one of the things I meant when saying
"bring the 2 older board's code up to scratch first".  The init() code does 3
things:

- set up misc CS5536 GPIOs
- configure the EC, which is a device of the IT8712F SIO.  However config isn't
  done via the usual SIO ports 2E/2F but via the EC's own I/O adresses.
- set misc IT8712 GPIOs, via the Simple-I/O port (with the config value that was
  dropped from devicetree.cb)

Other parts of the SIO config however, e.g. assigning the EC and Simple-I/O base
addresses is done in devicetree.cb whereas setting up which SIO GPIOs are
outputs and which are mapped for Simple-I/O is done in romstage.c because parts
of it are better be done early.

This mess makes me think if there is a cleaner way and which parts belong where.

> It makes sense to use values in the device tree to
> parameterize device code for specific mainboards, but I don't see the
> point of parameterizing mainboard-specific code from the device tree.

I on the other hand was intending to separate the configuration _data_ from the
_code_ that writes it into the chip.  The customers will still have to customize
coreboot for their purpose.  Some might want to move the COM1 from 3F8 to 3E8,
because they have added an external RS232 on PC/104 (ISA).  And some might
disable LPT because they need the IRQ for something else.  With a standard BIOS
they can do it in the BIOS Setup.  Here they can edit a common config _data_
file devicetree.cb.  They don't have to mess with e.g. the
superio/ite/it8712f/superio.c _code_.  And switching COM1 from RS232 to RS485
mode can be done in the standard BIOS Setup, too.  That's why I placed this
mainboard config value in devicetree.cb, too, rather than having customers mess
with the code in mainboard.c.  I had guessed that this is the purpose why there
is a struct mainboard_config in the mainboard's chip.h, just as there is a
struct superio_..._config in all superio's chip.h.  In what way is configuring
hardware implemented inside a SIO chip so much different from configuring
hardware implemented as a couple of discrete logic gates on the mainboard?  Am I
making any sense??  :)

There is second mainboard-specific config value (SMC_CONFIG), which I'd have
prefered in devicetree.cb, too. But I couldn't get this to work 2 years ago,
because the code is run early from romstage.c ...

Thanks and regards,
Jens

> It's really great what has become of coreboot!! Most visible to
> me (so
> far) is Kconfig which makes things a lot less scary than they used to be
> and
> Geode-LX VGA support.
> 
> > The attached patch calls init() again.
> 
> Indeed, it does. Excellent, thanks a lot!!!  :)
OK.  Hopefully you're almost there.

> Myles Watson wrote:
> > I think that the value that was left out of devicetree.cb should move to
> > the mainboard init() or into the device that's being configured
> (cs5536.)
> 
> In fact I was already wondering whether the whole code in mainboard init()
> is
> actually at the right place.  That's one of the things I meant when saying
> "bring the 2 older board's code up to scratch first".  The init() code
> does 3
> things:
> 
> - set up misc CS5536 GPIOs
> - configure the EC, which is a device of the IT8712F SIO.  However config
> isn't
>   done via the usual SIO ports 2E/2F but via the EC's own I/O adresses.
> - set misc IT8712 GPIOs, via the Simple-I/O port (with the config value
> that was
>   dropped from devicetree.cb)
> 
> Other parts of the SIO config however, e.g. assigning the EC and Simple-
> I/O base
> addresses is done in devicetree.cb whereas setting up which SIO GPIOs are
> outputs and which are mapped for Simple-I/O is done in romstage.c because
> parts
> of it are better be done early.
Yes.  Since the device tree isn't available until RAM is initialized, early
things can't live there.

> This mess makes me think if there is a cleaner way and which parts belong
> where.
Great.

> > It makes sense to use values in the device tree to
> > parameterize device code for specific mainboards, but I don't see the
> > point of parameterizing mainboard-specific code from the device tree.
> 
> I on the other hand was intending to separate the configuration _data_
> from the
> _code_ that writes it into the chip.  The customers will still have to
> customize
> coreboot for their purpose.  Some might want to move the COM1 from 3F8 to
> 3E8,
> because they have added an external RS232 on PC/104 (ISA).  And some might
> disable LPT because they need the IRQ for something else.  With a standard
> BIOS
> they can do it in the BIOS Setup.  Here they can edit a common config
> _data_
> file devicetree.cb.  They don't have to mess with e.g. the
> superio/ite/it8712f/superio.c _code_.  And switching COM1 from RS232 to
> RS485
> mode can be done in the standard BIOS Setup, too.  That's why I placed
> this
> mainboard config value in devicetree.cb, too, rather than having customers
> mess
> with the code in mainboard.c.  I had guessed that this is the purpose why
> there
> is a struct mainboard_config in the mainboard's chip.h, just as there is a
> struct superio_..._config in all superio's chip.h.  In what way is
> configuring
> hardware implemented inside a SIO chip so much different from configuring
> hardware implemented as a couple of discrete logic gates on the mainboard?
> Am I
> making any sense??  :)
Yes.  You're right that the separation is important.

> There is second mainboard-specific config value (SMC_CONFIG), which I'd
> have
> prefered in devicetree.cb, too. But I couldn't get this to work 2 years
> ago,
> because the code is run early from romstage.c ...

I think you need Kconfig variables for both of those things.  The device
tree isn't for options.  It's for immutable properties of the mainboard.

I'd suggest that your options depend on the motherboard.

Thanks,
Myles