[LinuxBIOS] [PATCH] Reimplementation/fixing of CS5530/CS5530A southbridge code

[Uwe Hermann]

See patch.

This is also required to make the JUKI-511P code work AFAICT (won't
build otherwise).

Uwe.

[LinuxBIOS] [PATCH] Reimplementation/fixing of CS5530/CS5530A southbridge code

[Stefan Reinauer]

* Uwe Hermann  [070606 23:34]:
> code and had several other problems, e.g. it enabled write access to the 
> ROM (why?)

for flash updates so flashrom does not have to do it.
> /* TODO: Make all of this configurable? */

No, thats a worthless "option" imho. Enable as much as you can. There is
no reason not to.
> /* Decode 0x000E0000-0x000FFFFF (128 KB), not just 64 KB. */
> reg8 = pci_read_config8(dev, ROM_AT_LOGIC_CONTROL_REG);
> reg8 |= LOWER_ROM_ADDRESS_RANGE;
> pci_write_config8(dev, ROM_AT_LOGIC_CONTROL_REG, reg8);

I'd drop that and rather put ram there. LinuxBIOS does not use the <1M
space.
> /* Decode 0xFF000000-0xFFFFFFFF (16 MB), not just 256 KB. */
> reg8 = pci_read_config8(dev, ROM_AT_LOGIC_CONTROL_REG);
> reg8 |= UPPER_ROM_ADDRESS_RANGE;
> pci_write_config8(dev, ROM_AT_LOGIC_CONTROL_REG, reg8);
> /* Set positive decode on ROM. */
> reg8 = pci_read_config8(dev, DECODE_CONTROL_REG2);
> reg8 |= BIOS_ROM_POSITIVE_DECODE;
> pci_write_config8(dev, DECODE_CONTROL_REG2, reg8);
> /* TODO: Make ROM writable? As config option maybe? */

If support is in flashrom, we should not, i guess.

But.. Should the above be done in failover.c or somewhat early? If you
have fallback and normal the high space may be required a lot earlier
than this.

Good work.

Acked-by: Stefan Reinauer

please fix above issues before or after the commit.

[Uwe Hermann]

On Thu, Jun 14, 2007 at 05:38:13PM +0200, Stefan Reinauer wrote:
> Uwe Hermann  [070606 23:34]:
> > code and had several other problems, e.g. it enabled write access to the 
> > ROM (why?)
>
> for flash updates so flashrom does not have to do it.

OK, then I'll drop this. That's the job of flashrom.
> > /* Decode 0x000E0000-0x000FFFFF (128 KB), not just 64 KB. */
> > reg8 = pci_read_config8(dev, ROM_AT_LOGIC_CONTROL_REG);
> > reg8 |= LOWER_ROM_ADDRESS_RANGE;
> > pci_write_config8(dev, ROM_AT_LOGIC_CONTROL_REG, reg8);
>
> I'd drop that and rather put ram there. LinuxBIOS does not use the <1M
> space.

OK, I'll test on hardware whether this has any consequences, but I guess
not, so I'll drop it.
> > /* TODO: Make ROM writable? As config option maybe? */
>
> If support is in flashrom, we should not, i guess.

Yep.
> Acked-by: Stefan Reinauer
>
> please fix above issues before or after the commit.

One more issue / poll:

What looks better and is more readable/understandable/efficient/short?

a)
        if (conf->ide1_enable) {
                reg8 |= SECONDARY_IDE_ENABLE;
        } else {
                reg8 &= ~(SECONDARY_IDE_ENABLE);
        }

b)

reg8 = (conf->ide1_enable ? (reg8 | SECONDARY_IDE_ENABLE)
                                  : (reg8 & ~(SECONDARY_IDE_ENABLE)));

c)

Some even more elegant solution?

Well, a) is probably easier to understand, but b) is 3 lines shorter, and
the 'foo ? bar : baz' idiom should be pretty well-known, I guess.

Opinions?

Uwe.

[Peter Stuge]

On Fri, Jun 15, 2007 at 02:15:00PM +0200, Uwe Hermann wrote:
> On Thu, Jun 14, 2007 at 05:38:13PM +0200, Stefan Reinauer wrote:
> > Uwe Hermann  [070606 23:34]:
> > > code and had several other problems, e.g. it enabled write
> > > access to the ROM (why?)
> >
> > for flash updates so flashrom does not have to do it.
>
> OK, then I'll drop this. That's the job of flashrom.

Not neccessarily.

Initially flashrom didn't have to do the job at all because the BIOS
did enable the ROM access.
>     Some even more elegant solution?

reg8 &= ~(SECONDARY_IDE_ENABLE);
if (conf->ide1_enable)
    reg8 |= SECONDARY_IDE_ENABLE;

?

//Peter

[Uwe Hermann]

On Fri, Jun 15, 2007 at 02:37:56PM +0200, Peter Stuge wrote:
> >     Some even more elegant solution?
>
> reg8 &= ~(SECONDARY_IDE_ENABLE);
> if (conf->ide1_enable)
>    reg8 |= SECONDARY_IDE_ENABLE;
>
> ?

Yeah, that would be another option.

As we're probably doing this type of operation quite often, I made a
macro out of it (which hopefully works for all situations):

/* If 'cond' is true this macro sets the bit(s) specified by 'bits' in the
 * 'reg' variable, otherwise it clears those bits.
 *
 * Examples:
 * reg16 = ONOFF(conf->ide0_enable, reg16, (1 << 5));
 * reg16 = ONOFF(conf->ide0_enable, reg16, (FOO | BAR));
 */
/* TODO: Move into some global header file? */

#define ONOFF(cond,reg,bits) ((cond) ? ((reg) | (bits)) : ((reg) & ~(bits)))

Usage example (from my other patch):

reg8 = pci_read_config8(dev, UDMACTL);
reg8 = ONOFF(conf->ide1_drive0_udma33_enable, reg8, SSDE0);
reg8 = ONOFF(conf->ide1_drive1_udma33_enable, reg8, SSDE1);
pci_write_config8(dev, UDMACTL, reg8);

Shall we create an include/macros.h file with some generic, useful
macros? Stuff like the above, and maybe something like:

#define MIN(a,b) ...
#define MAX(a,b) ...
#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
...

This type of common macros (not too many of course) will quite likely
improve the code readability a lot.

Uwe.

[LinuxBIOS] [PATCH] Reimplementation/fixing of CS5530/CS5530A southbridge code

[Jonathan Sturges]

Thanks for the updated 5530 code!
However, I'm having some difficulty enabling the IDE controllers when 
using this patch.  As far as I can tell, configuration parsing doesn't 
appear to work for new ideX_enable flags.

The 'register "ide0_enable" = "1"' configuration option seemed to have 
no effect on my 5530 system.  Even though both IDE controllers were 
enabled in Config.lb, when I'd test the resulting image, IDE0 would be 
disabled and IDE1 enabled.  In the end, to make it work, I changed:
"if (conf->ide0_enable) {"
to:
"if ( 1 ) {"
...in cs5530_ide.c to get IDE0 enabled.  This forces it to work.

Any ideas what would cause this?  By the way, I'm using the Eaglelion 
5BCM target for the moment, as it's nearly identical to my system.

thanks,
Jonathan

Uwe Hermann wrote:
> See patch.
>
> This is also required to make the JUKI-511P code work AFAICT (won't
> build otherwise).
>
> Uwe.
>
> This is a full rewrite of all the CS5530/CS5530A code. The previous code was
> mostly undocumented, had a broken coding style, contained lots of dead 
> code and had several other problems, e.g. it enabled write access to the 
> ROM (why?), it unconditionally enabled primary/secondary IDE (which should
> have a config option) and that even _twice_ (which is um... wrong).
>
> The new code
>
> has 'ide0_enable' and 'ide1_enable' config options (which actually
> work) to enable/disable the primary/secondary IDE interface in
> Config.lb.
>
> Does _not_ enable write access to the ROM (or is there some good
> reason to do that? If so, it should at least have a config option).
>
> Contains a bit more documentation.
>
> Uses readable (and documented) #defines instead of hardcoded magic values.
>
> aaand... it actually compiles ;-) Yep, that's right. The previous code
> wouldn't even build, as it hadn't been fully ported from v1 (still used
> v1 functions which are simply not available in v2).

[LinuxBIOS] [PATCH] Reimplementation/fixing of CS5530/CS5530A southbridge code

[Uwe Hermann]

On Wed, Aug 15, 2007 at 11:02:29PM -0400, Jonathan Sturges wrote:
> Thanks for the updated 5530 code!
> However, I'm having some difficulty enabling the IDE controllers when using 
> this patch.  As far as I can tell, configuration parsing doesn't appear to 
> work for new ideX_enable flags.
>
> The 'register "ide0_enable" = "1"' configuration option seemed to have no 
> effect on my 5530 system.  Even though both IDE controllers were enabled in 
> Config.lb, when I'd test the resulting image, IDE0 would be disabled and 
> IDE1 enabled.  In the end, to make it work, I changed:
> "if (conf->ide0_enable) {"
> to:
> "if ( 1 ) {"
> ...in cs5530_ide.c to get IDE0 enabled.  This forces it to work.

The placement of the ide0_enable lines is important. I think they should
_not_ be within the IDE section. Check src/mainboard/asi/mb_5blmp/Config.lb
for an example which worked for me, and please report if that fixes the
problem.

Uwe.