[PATCH] [RFC] sata PHY settings callback on SB700

[Rudolf Marek]

Hi from Fort Worth,

Here is a proposed way how to handle the SATA PHY settings on SB700. It consists
of weak function which always exists (with defaults) and a possibility to 
override this with normal function in main.c. This is the other way of doing 
that and not using the devictree.cb.

Please tell if it is ok this way. I suggest all people who ported SB700 should 
have a look on their SATA PHY settings too because it is most  likely PCB/MB 
dependent.

This patch is not tested but it could work.

Signed-off-by: Rudolf Marek

My holiday ends soonish but I temped to do bit of stuff during El Paso - Forth 
Worth ride ;)

Thanks,
Rudolf

[Peter Stuge]

Rudolf Marek wrote:
> Here is a proposed way how to handle the SATA PHY settings on SB700.
> It consists of weak function which always exists (with defaults) and
> a possibility to override this with normal function in main.c. This
> is the other way of doing that and not using the devictree.cb.
>
> Please tell if it is ok this way.

I feel that having these settings in devicetree.cb is the right way<tm>,
but weak functions could be an excellent step in that direction!

Weak functions would add a bit of structure currently missing when
stuff is done directly in mainboard_init().

It's also much smaller changes to create the overrides than it is to
evolve the internal devicetree API.

I like it.

//Peter

[Bao, Zheng]

The following words are got from SB700 rpr. It seems that the phy
settings have something to do with the eSATA.

SATA_PCI_config
0x01B48017
SATA_PCI_config
0x01B48019
SATA_PCI_config
0x01B48016
SATA_PCI_config
0x01B48016
SATA_PCI_config
0x01B48016
SATA_PCI_config
0x01B48016

SATA GENI PHY ports setting, Pre-emphasis setting, and
GENII PHY setting enable setup for port [0~5] This setting
is for the Travelly board. Since it's port0 and port1are
eSATA ports, the PCI_config 0x88 and 0x8C have different
settings than the rest of the ports. For non-esata port, the
setting should be 0x01B48016. For shinner board,
SATA_PCI_config 0x88/8C/90/94/98/9C [31:0] =
0x01B48016.

SATA_PCI_config 0xA0 [15:0] = 0xA09A
SATA_PCI_config 0xA2 [15:0] = 0xA09F
SATA_PCI_config 0xA4 [15:0] = 0xA07A
SATA_PCI_config 0xA6 [15:0] = 0xA07A
SATA_PCI_config 0xA8 [15:0] = 0xA07A
SATA_PCI_config 0xAA [15:0] = 0xA07A
SATA GEN II PHY port setting for port [0~5]. This setting is
for the Travelly board. Since it's port0 and port1 are
eSATA ports, the PCI_config 0xA0 and 0xA2 have different
settings than the rest of the ports. For non-esata port, the
setting should be 0xA07A. For shinner board,
SATA_PCI_config 0xA0/A2/A4/A6/A8/AA [15:0] = 0xA07A.

I am not quite familiar with the attribute weak. It seems to be a good
way.

Zheng
> -----Original Message-----
> From: [e-mail]

[mailto:[e-mail]]
> On Behalf Of Rudolf Marek
> Sent: Friday, September 17, 2010 10:49 PM
> To: Coreboot
> Subject: [coreboot] [PATCH] [RFC] sata PHY settings callback on SB700
>
> Hi from Fort Worth,
>
> Here is a proposed way how to handle the SATA PHY settings on SB700.

It
> consists
> of weak function which always exists (with defaults) and a possibility

to
> override this with normal function in main.c. This is the other way of
> doing
> that and not using the devictree.cb.
>
> Please tell if it is ok this way. I suggest all people who ported

SB700
> should
> have a look on their SATA PHY settings too because it is most  likely
> PCB/MB
> dependent.
>
> This patch is not tested but it could work.
>
> Signed-off-by: Rudolf Marek
>
> My holiday ends soonish but I temped to do bit of stuff during El Paso

-
> Forth
> Worth ride ;)
>
> Thanks,
> Rudolf

[Peter Stuge]

Bao, Zheng wrote:
> I am not quite familiar with the attribute weak. It seems to be a
> good way.

Normally if two functions have the same name the linker will abort
with an error. If one function is weak then it will be removed, and
the non-weak function will be used.

It is good and bad. It allows to override a general function easily,
but on the other hand it may be difficult to determine which code is
actually being used, when many functions have the same name.

//Peter

[Stefan Reinauer]

On 9/21/10 6:19 AM, Peter Stuge wrote:
> Bao, Zheng wrote:
> > I am not quite familiar with the attribute weak. It seems to be a
> > good way.
>
> Normally if two functions have the same name the linker will abort
> with an error. If one function is weak then it will be removed, and
> the non-weak function will be used.
>
> It is good and bad. It allows to override a general function easily,
> but on the other hand it may be difficult to determine which code is
> actually being used, when many functions have the same name.

If a weak function is not defined there will be no linker error... Thus,
in order to give a board an easy way to define a platform specific
(reboot) function in FILO, we used:

void filo_reset_handler(void)
{
void __attribute__((weak)) platform_reboot(void);

if (platform_reboot)
platform_reboot();
else
printf("Rebooting not supported.\n");

}

Stefan

[Rudolf Marek]

> If a weak function is not defined there will be no linker error...

Yes but I made a default function to be weak

1) it avoids this scenario
2) because mainboard.c function is "strong" multiple  accidental uses 
should produce link error
(as opposed of multiple weak functions)
3) same scheme is used by linux kernel ;)

All seems to be fine with this can I get an ack if I test this?

Thanks,
Rudolf
> Thus,
> in order to give a board an easy way to define a platform specific
> (reboot) function in FILO, we used:
>
> void filo_reset_handler(void)
> {
> void __attribute__((weak)) platform_reboot(void);
>
> if (platform_reboot)
> platform_reboot();
> else
> printf("Rebooting not supported.\n");
>
> }
>
> Stefan

[Rudolf Marek]

Hi,

It works for me (new settings applied). Maybe it can be acked now?

Thanks,
Rudolf

[Peter Stuge]

Rudolf Marek wrote:
> Here is a proposed way how to handle the SATA PHY settings on SB700. It 
> consists
> of weak function which always exists (with defaults) and a possibility to 
> override this with normal function in main.c. This is the other way of 
> doing that and not using the devictree.cb.
>
> Please tell if it is ok this way. I suggest all people who ported SB700 
> should have a look on their SATA PHY settings too because it is most  
> likely PCB/MB dependent.
>
> This patch is not tested but it could work.
>
> Signed-off-by: Rudolf Marek

Acked-by: Peter Stuge

Let's try it out!