RE: [PATCH v4 23/29] pci: layerscape: add official ls1028a binding support

-----Original Message----- From: Michael Walle [mailto:michael@walle.cc] Sent: 2021年10月13日 15:49 To: Z.Q. Hou zhiqiang.hou@nxp.com Cc: u-boot@lists.denx.de; Jagan Teki jagan@amarulasolutions.com; Priyanka Jain priyanka.jain@nxp.com; Vladimir Oltean vladimir.oltean@nxp.com; Tom Rini trini@konsulko.com; Peter Griffin peter.griffin@linaro.org; Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Subject: Re: [PATCH v4 23/29] pci: layerscape: add official ls1028a binding support

Hi Zhiqiang,

thanks for looking at this patch.

Am 2021-10-13 03:46, schrieb Z.Q. Hou:

...

...

-----Original Message----- From: Michael Walle michael@walle.cc Sent: 2021年10月5日 16:38 To: u-boot@lists.denx.de Cc: Jagan Teki jagan@amarulasolutions.com; Priyanka Jain priyanka.jain@nxp.com; Vladimir Oltean vladimir.oltean@nxp.com; Tom Rini trini@konsulko.com; Peter Griffin peter.griffin@linaro.org; Manivannan Sadhasivam manivannan.sadhasivam@linaro.org; Michael Walle michael@walle.cc; Z.Q. Hou zhiqiang.hou@nxp.com Subject: [PATCH v4 23/29] pci: layerscape: add official ls1028a binding support

The official bindind of the PCIe controller of the ls1028a has the following compatible string: compatible = "fsl,ls1028a-pcie";

Additionally, the resource names and count are different. Update the driver to support this binding and change the entry in the ls1028a device tree.

Cc: Hou Zhiqiang Zhiqiang.Hou@nxp.com Signed-off-by: Michael Walle michael@walle.cc

---

arch/arm/dts/fsl-ls1028a.dtsi | 20 +++++------ drivers/pci/pcie_layerscape_rc.c | 61 +++++++++++++++++++++++--------- 2 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/arch/arm/dts/fsl-ls1028a.dtsi b/arch/arm/dts/fsl-ls1028a.dtsi index cc055e65e5..435b965d00 100644 --- a/arch/arm/dts/fsl-ls1028a.dtsi +++ b/arch/arm/dts/fsl-ls1028a.dtsi @@ -344,12 +344,10 @@ };

pcie1: pcie@3400000 {

• 	compatible = "fsl,ls-pcie", "fsl,ls1028-pcie", "snps,dw-pcie";
  

• 	reg = <0x00 0x03400000 0x0 0x80000
  

• 	       0x00 0x03480000 0x0 0x40000   /* lut registers */
  

• 	       0x00 0x034c0000 0x0 0x40000   /* pf controls
  

registers */

• 	       0x80 0x00000000 0x0 0x20000>; /* configuration
  

space */

• 	reg-names = "dbi", "lut", "ctrl", "config";
  

• 	compatible = "fsl,ls1028a-pcie";
  

• 	reg = <0x00 0x03400000 0x0 0x00100000>, /* controller
  

registers */

• 	      <0x80 0x00000000 0x0 0x00002000>; /* configuration
  

space */

• 	reg-names = "regs", "config";
  #address-cells = <3>;
  #size-cells = <2>;
  device_type = "pci";
  

@@ -360,12 +358,10 @@ };

pcie2: pcie@3500000 {

• 	compatible = "fsl,ls-pcie", "fsl,ls1028-pcie", "snps,dw-pcie";
  

• 	reg = <0x00 0x03500000 0x0 0x80000
  

• 	       0x00 0x03580000 0x0 0x40000   /* lut registers */
  

• 	       0x00 0x035c0000 0x0 0x40000   /* pf controls
  

registers */

• 	       0x88 0x00000000 0x0 0x20000>; /* configuration
  

space */

• 	reg-names = "dbi", "lut", "ctrl", "config";
  

• 	compatible = "fsl,ls1028a-pcie";
  

• 	reg = <0x00 0x03500000 0x0 0x00100000>, /* controller
  

registers */

• 	      <0x88 0x00000000 0x0 0x00002000>; /* configuration
  

space */

• 	reg-names = "regs", "config";
  #address-cells = <3>;
  #size-cells = <2>;
  device_type = "pci";
  

diff --git a/drivers/pci/pcie_layerscape_rc.c b/drivers/pci/pcie_layerscape_rc.c index f50d6ef653..217b420076 100644 --- a/drivers/pci/pcie_layerscape_rc.c +++ b/drivers/pci/pcie_layerscape_rc.c @@ -21,6 +21,12 @@

DECLARE_GLOBAL_DATA_PTR;

+struct ls_pcie_drvdata {

• u32 lut_offset;
• u32 ctrl_offset;
• bool big_endian;

The endianness property is better only put in the DT nodes.

The whole point of this series is to align the u-boot binding with the (official) found in linux. There is no endianness property in the binding:

Got it.

https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2FDocumentation%2Fdevicetree%2 Fbindings%2Fpci%2Fsnps%252Cdw-pcie.yaml&data=04%7C01%7Czhiqiang. hou%40nxp.com%7C521b06e63e814dc69a5308d98e1dfbc4%7C686ea1d3bc2b 4c6fa92cd99c5c301635%7C0%7C0%7C637697081711351235%7CUnknown%7 CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ XVCI6Mn0%3D%7C1000&sdata=SZwT%2B6ErhG%2BIeMZ5FbfTFQZvbXJUj dSrsR5YOLn2IaA%3D&reserved=0

https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2FDocumentation%2Fdevicetree%2 Fbindings%2Fpci%2Flayerscape-pci.txt&data=04%7C01%7Czhiqiang.hou%4 0nxp.com%7C521b06e63e814dc69a5308d98e1dfbc4%7C686ea1d3bc2b4c6fa9 2cd99c5c301635%7C0%7C0%7C637697081711351235%7CUnknown%7CTWF pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6 Mn0%3D%7C1000&sdata=wx4vYCeXI0yFnip%2Bfj%2FMoLNi7PlEv41icJysiu kZyMA%3D&reserved=0

Also from a technical point of view, this property seems redundant with the compatible string which names a specific CPU.

The register's endianness is a hardware feature and should be put in the DT node, although the driver can determine it from the compatible string. The current Linux driver of Layerscape PCIe handles the endianness issue for various platforms, while I'm changing this. According to the current Linux driver and binding, this patch is good.

Thanks, Zhiqiang

-michael

...

The others looks good for me.

Thanks, Zhiqiang