Patchwork [RFC] sata PHY settings callback on SB700

login
register
about
Submitter Rudolf Marek
Date 2010-09-17 14:48:38
Message ID <4C937FC6.6060003@assembler.cz>
Download mbox | patch
Permalink /patch/1956/
State Accepted
Commit r5825
Headers show

Comments

Rudolf Marek - 2010-09-17 14:48:38
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 <r.marek@assembler.cz>

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

Thanks,
Rudolf
Peter Stuge - 2010-09-17 19:05:12
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 - 2010-09-21 02:28:24
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: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org]
> 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 <r.marek@assembler.cz>
> 
> My holiday ends soonish but I temped to do bit of stuff during El Paso
-
> Forth
> Worth ride ;)
> 
> Thanks,
> Rudolf
Peter Stuge - 2010-09-21 04:19:46
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 - 2010-09-21 17:03:43
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 - 2010-09-21 17:16:54
> 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 - 2010-09-22 21:16:50
Hi,

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

Thanks,
Rudolf
Peter Stuge - 2010-09-22 21:56:18
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 <r.marek@assembler.cz>

Acked-by: Peter Stuge <peter@stuge.se>

Let's try it out!

Patch

Index: src/southbridge/amd/sb700/sb700.h
===================================================================
--- src/southbridge/amd/sb700/sb700.h	(revision 5814)
+++ src/southbridge/amd/sb700/sb700.h	(working copy)
@@ -52,6 +52,11 @@ 
 #ifdef __PRE_RAM__
 void sb700_lpc_port80(void);
 void sb700_pci_port80(void);
+#else
+#include <device/pci.h>
+/* allow override in mainboard.c */
+void sb700_setup_sata_phys(struct device *dev);
+
 #endif
 
 #endif /* SB700_H */
Index: src/southbridge/amd/sb700/sb700_sata.c
===================================================================
--- src/southbridge/amd/sb700/sb700_sata.c	(revision 5814)
+++ src/southbridge/amd/sb700/sb700_sata.c	(working copy)
@@ -53,6 +53,29 @@ 
 	return 0;
 }
 
+	/* This function can be overloaded in mainboard.c */
+
+void __attribute__((weak)) sb700_setup_sata_phys(struct device *dev) {
+	/* RPR7.6.1 Program the PHY Global Control to 0x2C00 */
+	pci_write_config16(dev, 0x86, 0x2c00);
+
+	/* RPR7.6.2 SATA GENI PHY ports setting */
+	pci_write_config32(dev, 0x88, 0x01B48017);
+	pci_write_config32(dev, 0x8c, 0x01B48019);
+	pci_write_config32(dev, 0x90, 0x01B48016);
+	pci_write_config32(dev, 0x94, 0x01B48016);
+	pci_write_config32(dev, 0x98, 0x01B48016);
+	pci_write_config32(dev, 0x9C, 0x01B48016);
+
+	/* RPR7.6.3 SATA GEN II PHY port setting for port [0~5]. */
+	pci_write_config16(dev, 0xA0, 0xA09A);
+	pci_write_config16(dev, 0xA2, 0xA09F);
+	pci_write_config16(dev, 0xA4, 0xA07A);
+	pci_write_config16(dev, 0xA6, 0xA07A);
+	pci_write_config16(dev, 0xA8, 0xA07A);
+	pci_write_config16(dev, 0xAA, 0xA07A);
+}
+
 static void sata_init(struct device *dev)
 {
 	u8 byte;
@@ -161,27 +184,7 @@ 
 	/* Program the watchdog counter to 0x10 */
 	byte = 0x10;
 	pci_write_config8(dev, 0x46, byte);
-
-	/* RPR7.6.1 Program the PHY Global Control to 0x2C00 */
-	word = 0x2c00;
-	pci_write_config16(dev, 0x86, word);
-
-	/* RPR7.6.2 SATA GENI PHY ports setting */
-	pci_write_config32(dev, 0x88, 0x01B48017);
-	pci_write_config32(dev, 0x8c, 0x01B48019);
-	pci_write_config32(dev, 0x90, 0x01B48016);
-	pci_write_config32(dev, 0x94, 0x01B48016);
-	pci_write_config32(dev, 0x98, 0x01B48016);
-	pci_write_config32(dev, 0x9C, 0x01B48016);
-
-	/* RPR7.6.3 SATA GEN II PHY port setting for port [0~5]. */
-	pci_write_config16(dev, 0xA0, 0xA09A);
-	pci_write_config16(dev, 0xA2, 0xA09F);
-	pci_write_config16(dev, 0xA4, 0xA07A);
-	pci_write_config16(dev, 0xA6, 0xA07A);
-	pci_write_config16(dev, 0xA8, 0xA07A);
-	pci_write_config16(dev, 0xAA, 0xA07A);
-
+	sb700_setup_sata_phys(dev);
 	/* Enable the I/O, MM, BusMaster access for SATA */
 	byte = pci_read_config8(dev, 0x4);
 	byte |= 7 << 0;
Index: src/mainboard/asrock/939a785gmh/mainboard.c
===================================================================
--- src/mainboard/asrock/939a785gmh/mainboard.c	(revision 5814)
+++ src/mainboard/asrock/939a785gmh/mainboard.c	(working copy)
@@ -147,3 +147,25 @@ 
 	CHIP_NAME("Asrock 939A785GMH/128M Mainboard")
 	.enable_dev = mb_enable,
 };
+
+/* override the default SATA PHY setup */
+void sb700_setup_sata_phys(struct device *dev) {
+	/* RPR7.6.1 Program the PHY Global Control to 0x2C00 */
+	pci_write_config16(dev, 0x86, 0x2c00);
+
+	/* RPR7.6.2 SATA GENI PHY ports setting */
+	pci_write_config32(dev, 0x88, 0x01B48016);
+	pci_write_config32(dev, 0x8c, 0x01B48016);
+	pci_write_config32(dev, 0x90, 0x01B48016);
+	pci_write_config32(dev, 0x94, 0x01B48016);
+	pci_write_config32(dev, 0x98, 0x01B48016);
+	pci_write_config32(dev, 0x9C, 0x01B48016);
+
+	/* RPR7.6.3 SATA GEN II PHY port setting for port [0~5]. */
+	pci_write_config16(dev, 0xA0, 0xA07A);
+	pci_write_config16(dev, 0xA2, 0xA07A);
+	pci_write_config16(dev, 0xA4, 0xA07A);
+	pci_write_config16(dev, 0xA6, 0xA07A);
+	pci_write_config16(dev, 0xA8, 0xA07A);
+	pci_write_config16(dev, 0xAA, 0xA0FF);
+}
Index: src/mainboard/asus/p2b/romstage.c.orig
===================================================================
Index: src/mainboard/asus/p2b/acpi_tables.c
===================================================================
Index: src/mainboard/asus/p2b/dsdt.asl
===================================================================