[U-Boot] [PATCH] 85xx: Add is_serdes_configured() support for P2020 SERDES

[U-Boot] [PATCH] 85xx: Add is_serdes_configured() support for P2020 SERDES

[Kumar Gala]

Add the ability to determine if a given IP block connected on SERDES is
configured.  This is useful for things like PCIe and SRIO since they are
only ever connected on SERDES.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 arch/powerpc/cpu/mpc85xx/Makefile       |    6 ++-
 arch/powerpc/cpu/mpc85xx/p2020_serdes.c |   73 +++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletions(-)
 create mode 100644 arch/powerpc/cpu/mpc85xx/p2020_serdes.c

diff --git a/arch/powerpc/cpu/mpc85xx/Makefile b/arch/powerpc/cpu/mpc85xx/Makefile
index f064fee..f1abf0e 100644
--- a/arch/powerpc/cpu/mpc85xx/Makefile
+++ b/arch/powerpc/cpu/mpc85xx/Makefile
@@ -62,11 +62,15 @@ COBJS-$(CONFIG_PPC_P4080)	+= ddr-gen3.o
 COBJS-$(CONFIG_CPM2)	+= ether_fcc.o
 COBJS-$(CONFIG_OF_LIBFDT) += fdt.o
 COBJS-$(CONFIG_MP)	+= mp.o
-COBJS-$(CONFIG_MPC8536) += mpc8536_serdes.o
 COBJS-$(CONFIG_PCI)	+= pci.o
 COBJS-$(CONFIG_QE)	+= qe_io.o
 COBJS-$(CONFIG_CPM2)	+= serial_scc.o
 
+# SERDES support
+COBJS-$(CONFIG_MPC8536) += mpc8536_serdes.o
+COBJS-$(CONFIG_P2010)	+= p2020_serdes.o
+COBJS-$(CONFIG_P2020)	+= p2020_serdes.o
+
 COBJS	= $(COBJS-y)
 COBJS	+= cpu.o
 COBJS	+= cpu_init.o
diff --git a/arch/powerpc/cpu/mpc85xx/p2020_serdes.c b/arch/powerpc/cpu/mpc85xx/p2020_serdes.c
new file mode 100644
index 0000000..79fb9bb
--- /dev/null
+++ b/arch/powerpc/cpu/mpc85xx/p2020_serdes.c
@@ -0,0 +1,73 @@
+/*
+ * Copyright 2010 Freescale Semiconductor, Inc.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <config.h>
+#include <common.h>
+#include <asm/io.h>
+#include <asm/immap_85xx.h>
+#include <asm/fsl_serdes.h>
+
+#define SRDS1_MAX_LANES		4
+
+static u8 serdes1_cfg_tbl[][SRDS1_MAX_LANES] = {
+	[0x0] = {PCIE1, NONE, NONE, NONE},
+	[0x2] = {PCIE1, PCIE2, PCIE3, PCIE3},
+	[0x4] = {PCIE1, PCIE1, PCIE3, PCIE3},
+	[0x6] = {PCIE1, PCIE1, PCIE1, PCIE1},
+	[0x7] = {SRIO2, SRIO1, NONE, NONE},
+	[0x8] = {SRIO2, SRIO2, SRIO2, SRIO2},
+	[0x9] = {SRIO2, SRIO2, SRIO2, SRIO2},
+	[0xa] = {SRIO2, SRIO2, SRIO2, SRIO2},
+	[0xb] = {SRIO2, SRIO1, SGMII_TSEC2, SGMII_TSEC3},
+	[0xc] = {SRIO2, SRIO1, SGMII_TSEC2, SGMII_TSEC3},
+	[0xd] = {PCIE1, SRIO1, SGMII_TSEC2, SGMII_TSEC3},
+	[0xe] = {PCIE1, PCIE2, SGMII_TSEC2, SGMII_TSEC3},
+	[0xf] = {PCIE1, PCIE1, SGMII_TSEC2, SGMII_TSEC3},
+};
+
+int is_serdes_configured(enum srds_prtcl device)
+{
+	int i;
+	ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
+	u32 pordevsr = in_be32(&gur->pordevsr);
+	u32 srds1_cfg = (pordevsr & MPC85xx_PORDEVSR_IO_SEL) >>
+				MPC85xx_PORDEVSR_IO_SEL_SHIFT;
+
+	debug("%s: dev = %d\n", __FUNCTION__, device);
+	debug("PORDEVSR[IO_SEL] = %x\n", srds1_cfg);
+
+	if (srds1_cfg > ARRAY_SIZE(serdes1_cfg_tbl)) {
+		printf("Invalid PORDEVSR[IO_SEL] = %d\n", srds1_cfg);
+		return 0;
+	}
+
+	for (i = 0; i < SRDS1_MAX_LANES; i++) {
+		if (serdes1_cfg_tbl[srds1_cfg][i] == device)
+			return 1;
+	}
+
+	return 0;
+}
+
+void fsl_serdes_init(void)
+{
+}
-- 
1.6.0.6

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Kumar Gala]

The new is_serdes_configured covers a broader range of devices than the
PCI specific code.  Use it instead as we convert away from the
is_fsl_pci_cfg() code.

Additionally move to setting LAWs for PCI based on if its configured.
Also updated PCI FDT fixup code to remove PCI controllers from dtb if
they are not configured.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 board/freescale/p2020ds/law.c     |    6 ------
 board/freescale/p2020ds/p2020ds.c |   27 +++++++++++++++++++++++----
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/board/freescale/p2020ds/law.c b/board/freescale/p2020ds/law.c
index 28ed2ed..c90c9ae 100644
--- a/board/freescale/p2020ds/law.c
+++ b/board/freescale/p2020ds/law.c
@@ -29,12 +29,6 @@
 
 struct law_entry law_table[] = {
 	SET_LAW(CONFIG_SYS_FLASH_BASE_PHYS, LAW_SIZE_256M, LAW_TRGT_IF_LBC),
-	SET_LAW(CONFIG_SYS_PCIE1_MEM_PHYS, LAW_SIZE_512M, LAW_TRGT_IF_PCIE_1),
-	SET_LAW(CONFIG_SYS_PCIE1_IO_PHYS, LAW_SIZE_64K, LAW_TRGT_IF_PCIE_1),
-	SET_LAW(CONFIG_SYS_PCIE2_MEM_PHYS, LAW_SIZE_512M, LAW_TRGT_IF_PCIE_2),
-	SET_LAW(CONFIG_SYS_PCIE2_IO_PHYS, LAW_SIZE_64K, LAW_TRGT_IF_PCIE_2),
-	SET_LAW(CONFIG_SYS_PCIE3_MEM_PHYS, LAW_SIZE_512M, LAW_TRGT_IF_PCIE_3),
-	SET_LAW(CONFIG_SYS_PCIE3_IO_PHYS, LAW_SIZE_64K, LAW_TRGT_IF_PCIE_3),
 	SET_LAW(PIXIS_BASE_PHYS, LAW_SIZE_4K, LAW_TRGT_IF_LBC),
 	SET_LAW(CONFIG_SYS_NAND_BASE_PHYS, LAW_SIZE_1M, LAW_TRGT_IF_LBC),
 };
diff --git a/board/freescale/p2020ds/p2020ds.c b/board/freescale/p2020ds/p2020ds.c
index f0ff209..7b76be8 100644
--- a/board/freescale/p2020ds/p2020ds.c
+++ b/board/freescale/p2020ds/p2020ds.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 2007-2009 Freescale Semiconductor, Inc.
+ * Copyright 2007-2010 Freescale Semiconductor, Inc.
  *
  * See file CREDITS for list of people who contributed to this
  * project.
@@ -30,6 +30,7 @@
 #include <asm/fsl_pci.h>
 #include <asm/fsl_ddr_sdram.h>
 #include <asm/io.h>
+#include <asm/fsl_serdes.h>
 #include <miiphy.h>
 #include <libfdt.h>
 #include <fdt_support.h>
@@ -210,9 +211,13 @@ void pci_init_board(void)
 
 	puts("\n");
 #ifdef CONFIG_PCIE2
-	pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_2, io_sel);
+	pcie_configured = is_serdes_configured(PCIE2);
 
 	if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE2)) {
+		set_next_law(CONFIG_SYS_PCIE2_MEM_PHYS, LAW_SIZE_512M,
+				LAW_TRGT_IF_PCIE_2);
+		set_next_law(CONFIG_SYS_PCIE2_IO_PHYS, LAW_SIZE_64K,
+				LAW_TRGT_IF_PCIE_2);
 		SET_STD_PCIE_INFO(pci_info[num], 2);
 		pcie_ep = fsl_setup_hose(&pcie2_hose, pci_info[num].regs);
 		printf("    PCIE2 connected to ULI as %s (base addr %lx)\n",
@@ -250,9 +255,13 @@ void pci_init_board(void)
 #endif
 
 #ifdef CONFIG_PCIE3
-	pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_3, io_sel);
+	pcie_configured = is_serdes_configured(PCIE3);
 
 	if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE3)) {
+		set_next_law(CONFIG_SYS_PCIE3_MEM_PHYS, LAW_SIZE_512M,
+				LAW_TRGT_IF_PCIE_3);
+		set_next_law(CONFIG_SYS_PCIE3_IO_PHYS, LAW_SIZE_64K,
+				LAW_TRGT_IF_PCIE_3);
 		SET_STD_PCIE_INFO(pci_info[num], 3);
 		pcie_ep = fsl_setup_hose(&pcie3_hose, pci_info[num].regs);
 		printf("    PCIE3 connected to Slot 1 as %s (base addr %lx)\n",
@@ -269,9 +278,13 @@ void pci_init_board(void)
 #endif
 
 #ifdef CONFIG_PCIE1
-	pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_1, io_sel);
+	pcie_configured = is_serdes_configured(PCIE1);
 
 	if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE)) {
+		set_next_law(CONFIG_SYS_PCIE1_MEM_PHYS, LAW_SIZE_512M,
+				LAW_TRGT_IF_PCIE_1);
+		set_next_law(CONFIG_SYS_PCIE1_IO_PHYS, LAW_SIZE_64K,
+				LAW_TRGT_IF_PCIE_1);
 		SET_STD_PCIE_INFO(pci_info[num], 1);
 		pcie_ep = fsl_setup_hose(&pcie1_hose, pci_info[num].regs);
 		printf("    PCIE1 connected to Slot 2 as %s (base addr %lx)\n",
@@ -517,12 +530,18 @@ void ft_board_setup(void *blob, bd_t *bd)
 
 #ifdef CONFIG_PCIE3
 	ft_fsl_pci_setup(blob, "pci0", &pcie3_hose);
+#else
+	ft_fsl_pci_setup(blob, "pci0", NULL);
 #endif
 #ifdef CONFIG_PCIE2
 	ft_fsl_pci_setup(blob, "pci1", &pcie2_hose);
+#else
+	ft_fsl_pci_setup(blob, "pci1", NULL);
 #endif
 #ifdef CONFIG_PCIE1
 	ft_fsl_pci_setup(blob, "pci2", &pcie1_hose);
+#else
+	ft_fsl_pci_setup(blob, "pci2", NULL);
 #endif
 #ifdef CONFIG_FSL_SGMII_RISER
 	fsl_sgmii_riser_fdt_fixup(blob);
-- 
1.6.0.6

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Timur Tabi]

On Fri, May 21, 2010 at 4:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote:

> ?#ifdef CONFIG_PCIE3
> ? ? ? ?ft_fsl_pci_setup(blob, "pci0", &pcie3_hose);
> +#else
> + ? ? ? ft_fsl_pci_setup(blob, "pci0", NULL);
> ?#endif

What is the reason why CONFIG_PCIE3 setups up the pci node called
"pci0"?  Is it because CONFIG_SYS_PCIE3_ADDR is the address of the
pci0 node in the p2020ds device tree?

If so, why can't we do this is P2020DS.h:

#define CONFIG_SYS_PCIE1_ADDR		(CONFIG_SYS_CCSRBAR+0x8000) /* pci0 */
#define CONFIG_SYS_PCIE2_ADDR		(CONFIG_SYS_CCSRBAR+0x9000) /* pci1 */
#define CONFIG_SYS_PCIE3_ADDR		(CONFIG_SYS_CCSRBAR+0xa000) /* pci2 */

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Timur Tabi]

On Fri, May 21, 2010 at 4:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote:

> ?#ifdef CONFIG_PCIE2
> - ? ? ? pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_2, io_sel);
> + ? ? ? pcie_configured = is_serdes_configured(PCIE2);
>
> ? ? ? ?if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE2)) {
> + ? ? ? ? ? ? ? set_next_law(CONFIG_SYS_PCIE2_MEM_PHYS, LAW_SIZE_512M,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? LAW_TRGT_IF_PCIE_2);
> + ? ? ? ? ? ? ? set_next_law(CONFIG_SYS_PCIE2_IO_PHYS, LAW_SIZE_64K,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? LAW_TRGT_IF_PCIE_2);
> ? ? ? ? ? ? ? ?SET_STD_PCIE_INFO(pci_info[num], 2);

Are you sure that pci_info needs to be an array?  It looks as if
pci_info[num] is only used inside the #ifdef block.  That is, I think
we can do this:

struct fsl_pci_info pci_info;

...

#ifdef CONFIG_PCIE3
	pcie_configured = is_serdes_configured(PCIE3);

	if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE3)) {
		set_next_law(CONFIG_SYS_PCIE3_MEM_PHYS, LAW_SIZE_512M,
				LAW_TRGT_IF_PCIE_3);
		set_next_law(CONFIG_SYS_PCIE3_IO_PHYS, LAW_SIZE_64K,
				LAW_TRGT_IF_PCIE_3);
		SET_STD_PCIE_INFO(&pci_info, 3);
		pcie_ep = fsl_setup_hose(&pcie3_hose, pci_info.regs);
		printf("    PCIE3 connected to Slot 1 as %s (base addr %lx)\n",
				pcie_ep ? "Endpoint" : "Root Complex",
				pci_info.regs);
		first_free_busno = fsl_pci_init_port(&pci_info,
					&pcie3_hose, first_free_busno);
	} else {
		printf("    PCIE3: disabled\n");
	}
	puts("\n");
#else

Each #ifdef CONFIG_PCIEx block is self-contained, with respect to
pci_info.  It appears that there's no need to remember pci_info[0] and
use pci_info[1] for the next PCIE device.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Wolfgang Denk]

Dear Kumar Gala,

In message <1274433478-31849-2-git-send-email-galak@kernel.crashing.org> you wrote:
> The new is_serdes_configured covers a broader range of devices than the
> PCI specific code.  Use it instead as we convert away from the
> is_fsl_pci_cfg() code.
> 
> Additionally move to setting LAWs for PCI based on if its configured.
> Also updated PCI FDT fixup code to remove PCI controllers from dtb if
> they are not configured.
...
>  #ifdef CONFIG_PCIE3
>  	ft_fsl_pci_setup(blob, "pci0", &pcie3_hose);
> +#else
> +	ft_fsl_pci_setup(blob, "pci0", NULL);
>  #endif
>  #ifdef CONFIG_PCIE2
>  	ft_fsl_pci_setup(blob, "pci1", &pcie2_hose);
> +#else
> +	ft_fsl_pci_setup(blob, "pci1", NULL);
>  #endif
>  #ifdef CONFIG_PCIE1
>  	ft_fsl_pci_setup(blob, "pci2", &pcie1_hose);
> +#else
> +	ft_fsl_pci_setup(blob, "pci2", NULL);
>  #endif

As Timur already pointed out: can we please clean up this mess of
3 = 0 = 3, 2 = 1 = 2, 1 = 2 = 1?

This is extremely confusing.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He had been eight years upon a project for extracting sunbeams out of
cucumbers, which were to be put in vials hermetically sealed, and let
out to warm the air in raw inclement summers.        - Jonathan Swift
              _Gulliver's Travels_ ``A Voyage to Laputa, etc.'' ch. 5

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Kumar Gala]

On May 27, 2010, at 2:08 AM, Wolfgang Denk wrote:

> Dear Kumar Gala,
> 
> In message <1274433478-31849-2-git-send-email-galak@kernel.crashing.org> you wrote:
>> The new is_serdes_configured covers a broader range of devices than the
>> PCI specific code.  Use it instead as we convert away from the
>> is_fsl_pci_cfg() code.
>> 
>> Additionally move to setting LAWs for PCI based on if its configured.
>> Also updated PCI FDT fixup code to remove PCI controllers from dtb if
>> they are not configured.
> ...
>> #ifdef CONFIG_PCIE3
>> 	ft_fsl_pci_setup(blob, "pci0", &pcie3_hose);
>> +#else
>> +	ft_fsl_pci_setup(blob, "pci0", NULL);
>> #endif
>> #ifdef CONFIG_PCIE2
>> 	ft_fsl_pci_setup(blob, "pci1", &pcie2_hose);
>> +#else
>> +	ft_fsl_pci_setup(blob, "pci1", NULL);
>> #endif
>> #ifdef CONFIG_PCIE1
>> 	ft_fsl_pci_setup(blob, "pci2", &pcie1_hose);
>> +#else
>> +	ft_fsl_pci_setup(blob, "pci2", NULL);
>> #endif
> 
> As Timur already pointed out: can we please clean up this mess of
> 3 = 0 = 3, 2 = 1 = 2, 1 = 2 = 1?
> 
> This is extremely confusing.

This is my fault.  However not sure what to do about it since we'd break compatibility with kernel .dts to clean this up.

99% of the u-boot code should match the HW docs.  In this one place I tried to "rename" things such that it made sense.  The pci aliases in the .dts are in order of address (so whatever HW controller is @ 0x8000 would be "pci0", 0x9000 - "pci1", etc.)

- k

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Wolfgang Denk]

Dear Kumar Gala,

In message <5F58DE0B-6EF8-4ED6-A1A8-C0E37C8539BE@kernel.crashing.org> you wrote:
> 
> This is my fault.  However not sure what to do about it since we'd break
> compatibility with kernel .dts to clean this up.
> 
> 99% of the u-boot code should match the HW docs.  In this one place I
> tried to "rename" things such that it made sense.  The pci aliases in
> the .dts are in order of address (so whatever HW controller is @ 0x8000
> would be "pci0", 0x9000 - "pci1", etc.)

This doesn't seem to be the case in U-Boot; here we see:

#define CONFIG_SYS_PCIE3_ADDR           (CONFIG_SYS_CCSRBAR+0x8000)
#define CONFIG_SYS_PCIE2_ADDR           (CONFIG_SYS_CCSRBAR+0x9000)
#define CONFIG_SYS_PCIE1_ADDR           (CONFIG_SYS_CCSRBAR+0xa000)

i. e. the highest number is at the lowest address??



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The easiest way to figure the cost of living is to take  your  income
and add ten percent.

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Kumar Gala]

On May 27, 2010, at 6:20 AM, Wolfgang Denk wrote:

> Dear Kumar Gala,
> 
> In message <5F58DE0B-6EF8-4ED6-A1A8-C0E37C8539BE@kernel.crashing.org> you wrote:
>> 
>> This is my fault.  However not sure what to do about it since we'd break
>> compatibility with kernel .dts to clean this up.
>> 
>> 99% of the u-boot code should match the HW docs.  In this one place I
>> tried to "rename" things such that it made sense.  The pci aliases in
>> the .dts are in order of address (so whatever HW controller is @ 0x8000
>> would be "pci0", 0x9000 - "pci1", etc.)
> 
> This doesn't seem to be the case in U-Boot; here we see:
> 
> #define CONFIG_SYS_PCIE3_ADDR           (CONFIG_SYS_CCSRBAR+0x8000)
> #define CONFIG_SYS_PCIE2_ADDR           (CONFIG_SYS_CCSRBAR+0x9000)
> #define CONFIG_SYS_PCIE1_ADDR           (CONFIG_SYS_CCSRBAR+0xa000)
> 
> i. e. the highest number is at the lowest address??

Correct, that is matching FSL HW docs numbering/naming.

in the .dts the alias:
* "pci0" is @ 0x8000	- FSL HW calls it PCIE3
* "pci1" is @ 0x9000	- FSL HW calls it PCIE2
* "pci2" is @ 0xa000	- FSL HW calls it PCIE1

- k

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Wolfgang Denk]

Dear Kumar Gala,

In message <7D2BA6A1-2EEF-4BF5-8B76-07AD55537BEE@kernel.crashing.org> you wrote:
> 
> > i. e. the highest number is at the lowest address??
>
> Correct, that is matching FSL HW docs numbering/naming.
>
> in the .dts the alias:
> * "pci0" is @ 0x8000	- FSL HW calls it PCIE3
> * "pci1" is @ 0x9000	- FSL HW calls it PCIE2
> * "pci2" is @ 0xa000	- FSL HW calls it PCIE1

IMHO there were several decisions that I would not exactly call
intelligent - the first to assign HW names in descending order, and
the second to chose a different order for the bus names.

Now we sit here and have to suffer from this :-(

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There are two ways of constructing a software design. One way  is  to
make  it  so  simple that there are obviously no deficiencies and the
other is to make it so complicated that there are  no  obvious  defi-
ciencies.                             - Charles Anthony Richard Hoare

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Kumar Gala]

On May 27, 2010, at 8:20 AM, Wolfgang Denk wrote:

> Dear Kumar Gala,
> 
> In message <7D2BA6A1-2EEF-4BF5-8B76-07AD55537BEE@kernel.crashing.org> you wrote:
>> 
>>> i. e. the highest number is at the lowest address??
>> 
>> Correct, that is matching FSL HW docs numbering/naming.
>> 
>> in the .dts the alias:
>> * "pci0" is @ 0x8000	- FSL HW calls it PCIE3
>> * "pci1" is @ 0x9000	- FSL HW calls it PCIE2
>> * "pci2" is @ 0xa000	- FSL HW calls it PCIE1
> 
> IMHO there were several decisions that I would not exactly call
> intelligent - the first to assign HW names in descending order, and
> the second to chose a different order for the bus names.
> 
> Now we sit here and have to suffer from this :-(

Agree on both fronts.

- k

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Scott Wood]

On Thu, May 27, 2010 at 07:45:16AM -0500, Kumar Gala wrote:
> 
> On May 27, 2010, at 6:20 AM, Wolfgang Denk wrote:
> 
> > Dear Kumar Gala,
> > 
> > In message <5F58DE0B-6EF8-4ED6-A1A8-C0E37C8539BE@kernel.crashing.org> you wrote:
> >> 
> >> This is my fault.  However not sure what to do about it since we'd break
> >> compatibility with kernel .dts to clean this up.
> >> 
> >> 99% of the u-boot code should match the HW docs.  In this one place I
> >> tried to "rename" things such that it made sense.  The pci aliases in
> >> the .dts are in order of address (so whatever HW controller is @ 0x8000
> >> would be "pci0", 0x9000 - "pci1", etc.)
> > 
> > This doesn't seem to be the case in U-Boot; here we see:
> > 
> > #define CONFIG_SYS_PCIE3_ADDR           (CONFIG_SYS_CCSRBAR+0x8000)
> > #define CONFIG_SYS_PCIE2_ADDR           (CONFIG_SYS_CCSRBAR+0x9000)
> > #define CONFIG_SYS_PCIE1_ADDR           (CONFIG_SYS_CCSRBAR+0xa000)
> > 
> > i. e. the highest number is at the lowest address??
> 
> Correct, that is matching FSL HW docs numbering/naming.
> 
> in the .dts the alias:
> * "pci0" is @ 0x8000	- FSL HW calls it PCIE3
> * "pci1" is @ 0x9000	- FSL HW calls it PCIE2
> * "pci2" is @ 0xa000	- FSL HW calls it PCIE1

This is why the dts files should live with u-boot, not Linux...

but if we can't change the dts file, we could ignore the aliases and look up
PCI controllers by address.

-Scott

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Kumar Gala]

On May 27, 2010, at 2:38 PM, Scott Wood wrote:

> On Thu, May 27, 2010 at 07:45:16AM -0500, Kumar Gala wrote:
>> 
>> On May 27, 2010, at 6:20 AM, Wolfgang Denk wrote:
>> 
>>> Dear Kumar Gala,
>>> 
>>> In message <5F58DE0B-6EF8-4ED6-A1A8-C0E37C8539BE@kernel.crashing.org> you wrote:
>>>> 
>>>> This is my fault.  However not sure what to do about it since we'd break
>>>> compatibility with kernel .dts to clean this up.
>>>> 
>>>> 99% of the u-boot code should match the HW docs.  In this one place I
>>>> tried to "rename" things such that it made sense.  The pci aliases in
>>>> the .dts are in order of address (so whatever HW controller is @ 0x8000
>>>> would be "pci0", 0x9000 - "pci1", etc.)
>>> 
>>> This doesn't seem to be the case in U-Boot; here we see:
>>> 
>>> #define CONFIG_SYS_PCIE3_ADDR           (CONFIG_SYS_CCSRBAR+0x8000)
>>> #define CONFIG_SYS_PCIE2_ADDR           (CONFIG_SYS_CCSRBAR+0x9000)
>>> #define CONFIG_SYS_PCIE1_ADDR           (CONFIG_SYS_CCSRBAR+0xa000)
>>> 
>>> i. e. the highest number is at the lowest address??
>> 
>> Correct, that is matching FSL HW docs numbering/naming.
>> 
>> in the .dts the alias:
>> * "pci0" is @ 0x8000	- FSL HW calls it PCIE3
>> * "pci1" is @ 0x9000	- FSL HW calls it PCIE2
>> * "pci2" is @ 0xa000	- FSL HW calls it PCIE1
> 
> This is why the dts files should live with u-boot, not Linux...
> 
> but if we can't change the dts file, we could ignore the aliases and look up
> PCI controllers by address.

Actually, I think we can just kill the aliases completely.

We can do something like:

        for (hose = hose_head; hose; hose = hose->next) {
		off = fdt_node_offset_by_compatible(blob, -1, pci_compat);
		while (off != -FDT_ERR_NOTFOUND) {
			const int reg * = fdt_getprop(blob, off, "reg", NULL);

			match hose->cfg_addr against reg

			off = fdt_node_offset_by_compatible(blob, off, pci_compat);
		}
	}

- k

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Timur Tabi]

Kumar Gala wrote:

> We can do something like:
> 
>         for (hose = hose_head; hose; hose = hose->next) {
> 		off = fdt_node_offset_by_compatible(blob, -1, pci_compat);
> 		while (off != -FDT_ERR_NOTFOUND) {
> 			const int reg * = fdt_getprop(blob, off, "reg", NULL);
> 
> 			match hose->cfg_addr against reg
> 
> 			off = fdt_node_offset_by_compatible(blob, off, pci_compat);
> 		}
> 	}

Ok, I'll start working on this after I'm done with the P1022DS.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Timur Tabi]

On Fri, May 21, 2010 at 4:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote:

> ? ? ? ?if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE2)) {
> + ? ? ? ? ? ? ? set_next_law(CONFIG_SYS_PCIE2_MEM_PHYS, LAW_SIZE_512M,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? LAW_TRGT_IF_PCIE_2);
> + ? ? ? ? ? ? ? set_next_law(CONFIG_SYS_PCIE2_IO_PHYS, LAW_SIZE_64K,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? LAW_TRGT_IF_PCIE_2);
> ? ? ? ? ? ? ? ?SET_STD_PCIE_INFO(pci_info[num], 2);

How about this instead:

                SET_STD_PCIE_INFO(pci_info[num], 2);
+               set_next_law(pci_info[num].mem_phys,
+			     law_size_bits(pci_info[num].mem_size),
+			     LAW_TRGT_IF_PCIE_2);
+               set_next_law(pci_info[num].io_phys,
+			     law_size_bits(pci_info[num].io_size),
+                               LAW_TRGT_IF_PCIE_2);

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Kumar Gala]

On May 27, 2010, at 4:38 PM, Timur Tabi wrote:

> On Fri, May 21, 2010 at 4:17 AM, Kumar Gala <galak@kernel.crashing.org> wrote:
> 
>>        if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE2)) {
>> +               set_next_law(CONFIG_SYS_PCIE2_MEM_PHYS, LAW_SIZE_512M,
>> +                               LAW_TRGT_IF_PCIE_2);
>> +               set_next_law(CONFIG_SYS_PCIE2_IO_PHYS, LAW_SIZE_64K,
>> +                               LAW_TRGT_IF_PCIE_2);
>>                SET_STD_PCIE_INFO(pci_info[num], 2);
> 
> How about this instead:
> 
>                SET_STD_PCIE_INFO(pci_info[num], 2);
> +               set_next_law(pci_info[num].mem_phys,
> +			     law_size_bits(pci_info[num].mem_size),
> +			     LAW_TRGT_IF_PCIE_2);
> +               set_next_law(pci_info[num].io_phys,
> +			     law_size_bits(pci_info[num].io_size),
> +                               LAW_TRGT_IF_PCIE_2);

that's fine.  We could fold the law setup into fsl_pci_init_port()

* tweak SET_STD_PCIE_INFO to track LAW_TRGT_IF_PCIE_n
* add something like the following to fsl_pci_init_port():

        struct law_entry law;

	law = find_law(pci_info->mem_phys);
        if (law.index == -1) {
                law.index = set_next_law(pci_info->mem_phys,
					law_size_bits(pci_info->mem_size),
					pci_info->law_trgt_id);
        }

	/* duplicate for IO */

- k

[U-Boot] [PATCH] 85xx/p2020ds: Use is_serdes_configured() to determine of PCIe enabled

[Timur Tabi]

Kumar Gala wrote:

> * tweak SET_STD_PCIE_INFO to track LAW_TRGT_IF_PCIE_n
> * add something like the following to fsl_pci_init_port():
> 
>         struct law_entry law;
> 
> 	law = find_law(pci_info->mem_phys);
>         if (law.index == -1) {
>                 law.index = set_next_law(pci_info->mem_phys,
> 					law_size_bits(pci_info->mem_size),
> 					pci_info->law_trgt_id);
>         }
> 
> 	/* duplicate for IO */

Ok, I'll add it to my list for the PCI revamp.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] 85xx: Add is_serdes_configured() support for P2020SERDES

[Li Yang]

On 5/21/2010 5:17 PM, Kumar Gala wrote:

 >

 >Add the ability to determine if a given IP block connected on SERDES is

 >configured. This is useful for things like PCIe and SRIO since they are

 >only ever connected on SERDES.

 >

 >Signed-off-by: Kumar Gala <galak at kernel.crashing.org 
<mailto:galak@kernel.crashing.org>>

I'm not sure why this code has to be re-done rather than using the patch 
I submitted?

{snip}

 >+int is_serdes_configured(enum srds_prtcl device) {

 >+ int i;

 >+ ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);

 >+ u32 pordevsr = in_be32(&gur->pordevsr);

Wouldn't it be better to only read it once on fsl_serdes_init(), after 
all it will not be changed after reset.

 >+ u32 srds1_cfg = (pordevsr & MPC85xx_PORDEVSR_IO_SEL) >>

 >+ MPC85xx_PORDEVSR_IO_SEL_SHIFT;

 >+

 >+ debug("%s: dev = %d\n", __FUNCTION__, device);

 >+ debug("PORDEVSR[IO_SEL] = %x\n", srds1_cfg);

 >+

 >+ if (srds1_cfg > ARRAY_SIZE(serdes1_cfg_tbl)) {

 >+ printf("Invalid PORDEVSR[IO_SEL] = %d\n", srds1_cfg);

 >+ return 0;

 >+ }

 >+

 >+ for (i = 0; i < SRDS1_MAX_LANES; i++) {

 >+ if (serdes1_cfg_tbl[srds1_cfg][i] == device)

 >+ return 1;

Given the fact that the is_serdes_configured() will be called multiple 
times, it will be better to process the table in fsl_serdes_init() and 
save time for every is_serdes_configured() judgment like what I did.

- Leo

 >+ }

 >+

 >+ return 0;

 >+}

 >+

 >+void fsl_serdes_init(void)

 >+{

 >+}

 >--

 >1.6.0.6

 >

 >_______________________________________________

 >U-Boot mailing list

 >U-Boot at lists.denx.de <mailto:U-Boot@lists.denx.de>

 >http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] 85xx: Add is_serdes_configured() support for P2020SERDES

[Kumar Gala]

On May 21, 2010, at 6:26 AM, Li Yang-R58472 wrote:

>> Subject: [U-Boot] [PATCH] 85xx: Add is_serdes_configured() support for
>> P2020SERDES
>> 
>> Add the ability to determine if a given IP block connected on SERDES is
>> configured.  This is useful for things like PCIe and SRIO since they are
>> only ever connected on SERDES.
>> 
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> 
> I'm not sure why this code has to be re-done rather than using the patch I submitted?

There was some minor unification with the P4080 SERDES code.

> {snip}
> 
>> +int is_serdes_configured(enum srds_prtcl device) {
>> +	int i;
>> +	ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
>> +	u32 pordevsr = in_be32(&gur->pordevsr);
> 
> Wouldn't it be better to only read it once on fsl_serdes_init(), after all it will not be changed after reset.  

This depend on the use of global static variables in u-boot.  Not sure the current state of that being acceptable.

> 
>> +	u32 srds1_cfg = (pordevsr & MPC85xx_PORDEVSR_IO_SEL) >>
>> +				MPC85xx_PORDEVSR_IO_SEL_SHIFT;
>> +
>> +	debug("%s: dev = %d\n", __FUNCTION__, device);
>> +	debug("PORDEVSR[IO_SEL] = %x\n", srds1_cfg);
>> +
>> +	if (srds1_cfg > ARRAY_SIZE(serdes1_cfg_tbl)) {
>> +		printf("Invalid PORDEVSR[IO_SEL] = %d\n", srds1_cfg);
>> +		return 0;
>> +	}
>> +
>> +	for (i = 0; i < SRDS1_MAX_LANES; i++) {
>> +		if (serdes1_cfg_tbl[srds1_cfg][i] == device)
>> +			return 1;
> 
> Given the fact that the is_serdes_configured() will be called multiple times, it will be better to process the table in fsl_serdes_init() and save time for every is_serdes_configured() judgment like what I did.
> 
> - Leo
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +void fsl_serdes_init(void)
>> +{
>> +}
>> --
>> 1.6.0.6
>> 
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>