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

[Z.Q. Hou]

> -----Original Message-----
> From: Michael Walle [mailto:michael at walle.cc]
> Sent: 2021年10月13日 15:49
> To: Z.Q. Hou <zhiqiang.hou at nxp.com>
> Cc: u-boot at lists.denx.de; Jagan Teki <jagan at amarulasolutions.com>; Priyanka
> Jain <priyanka.jain at nxp.com>; Vladimir Oltean <vladimir.oltean at nxp.com>; Tom
> Rini <trini at konsulko.com>; Peter Griffin <peter.griffin at linaro.org>; Manivannan
> Sadhasivam <manivannan.sadhasivam at 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 at walle.cc>
> >> Sent: 2021年10月5日 16:38
> >> To: u-boot at lists.denx.de
> >> Cc: Jagan Teki <jagan at amarulasolutions.com>; Priyanka Jain
> >> <priyanka.jain at nxp.com>; Vladimir Oltean <vladimir.oltean at nxp.com>;
> >> Tom Rini <trini at konsulko.com>; Peter Griffin
> >> <peter.griffin at linaro.org>; Manivannan Sadhasivam
> >> <manivannan.sadhasivam at linaro.org>; Michael Walle <michael at walle.cc>;
> >> Z.Q. Hou <zhiqiang.hou at 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 at nxp.com>
> >> Signed-off-by: Michael Walle <michael at 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 at 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 at 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