[PATCH v2 8/9] arm: dts: ls1028a: sync the fsl-ls1028a.dtsi with linux

[Vladimir Oltean]

On Wed, Sep 01, 2021 at 11:34:50PM +0200, Michael Walle wrote:
> Am 2021-09-01 12:29, schrieb Vladimir Oltean:
> > On Wed, Sep 01, 2021 at 10:55:21AM +0200, Michael Walle wrote:
> > > -		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";
> > > -			   #address-cells = <3>;
> > > -			   #size-cells = <2>;
> > > -			   device_type = "pci";
> > > -			   num-lanes = <4>;
> > > -			   bus-range = <0x0 0xff>;
> > > -			   ranges = <0x81000000 0x0 0x00000000 0x80 0x00020000 0x0
> > > 0x00010000   /* downstream I/O */
> > > -				   0x82000000 0x0 0x40000000 0x80 0x40000000 0x0 0x40000000>;
> > > /* non-prefetchable memory */
> > > -		};
> > > +		pcie1: pcie at 3400000 {
> > > +			compatible = "fsl,ls1028a-pcie";
> > > +			reg = <0x00 0x03400000 0x0 0x00100000>, /* controller registers */
> > > +			      <0x80 0x00000000 0x0 0x00002000>; /* configuration space */
> > 
> > This doesn't look good, the conversion is lossy. The Linux driver
> > (drivers/pci/controller/dwc/pci-layerscape.c) knows the lut_offset by
> > compatible string, but the U-Boot driver
> > (drivers/pci/pcie_layerscape_rc.c)
> > calls fdt_get_named_resource("lut"). I.... don't think that the driver
> > works with a NULL pcie->lut pointer? The StreamID writes in
> > fdt_fixup_pcie_device_ls probably didn't explode because I don't have
> > any PCIe card plugged in.
> 
> I guess it didn't probe anyway, because there is no driver for that
> compatible string ;)

omg, "ls1028" vs "ls1028a" :-/

> > Similar thing for the PEX PF control memory region. In the LS1028A RM,
> > technically this is part of the larger PEX_LUT memory map, just starting
> > from the 4_0014h offset.
> > 
> > U-Boot has this piece of logic to deduce pcie->ctrl based on pcie->lut,
> > but it seems that it needs to be extended to cover LS1028A:
> 
> Only that one probably don't want to overwrite cfg_res.start and
> cfg_res.end.

My internal parser is broken by this phrase's syntax, but yes.

> > 	/*
> > 	 * Fix the pcie memory map address and PF control registers address
> > 	 * for LS2088A series SoCs
> > 	 */
> > 	svr = get_svr();
> > 	svr = (svr >> SVR_VAR_PER_SHIFT) & 0xFFFFFE;
> > 	if (svr == SVR_LS2088A || svr == SVR_LS2084A ||
> > 	    svr == SVR_LS2048A || svr == SVR_LS2044A ||
> > 	    svr == SVR_LS2081A || svr == SVR_LS2041A) {
> > 		pcie_rc->cfg_res.start = LS2088A_PCIE1_PHYS_ADDR +
> > 					 LS2088A_PCIE_PHYS_SIZE * pcie->idx;
> > 		pcie_rc->cfg_res.end = pcie_rc->cfg_res.start + cfg_size;
> > 		pcie->ctrl = pcie->lut + 0x40000;
> > 	}
> 
> Oh the horror! Why didn't one just change the config register value in
> the (u-boot) device tree instead of overwriting the value for some
> specific SoCs.

Code added in commit 3d8553f0a3ee ("pci: layerscape: add LS2088A series
SoC pcie support"). As to why the differentiation between LS2080A and
LS2088A/LS2084A and the 4-core variants is done through code rather than
through DT, Zhiqiang said:

| There isn't LS2088A DT file, actually it reuse LS2080A DT file
| under u-boot, while under kernel there are DT files for LS2080A
| and LS2088A respectively with different PCIe node compatible
| string.

https://u-boot.denx.narkive.com/XVDnIKG9/patchv3-1-2-pci-layerscape-add-ls2088a-series-soc-pcie-support

Again, as to why that is, no clue whatsoever.

> 
> > As for pcie->lut itself, simplest would be to just default to what is
> > now the "dbi" reg value, plus a .lut_offset determined by compatible
> > string, in the case of "fsl,ls1028a-pcie" 0x80000, just like Linux.
> > 
> > Ah, and not to mention that the reg-names are not even the same between
> > U-Boot and Linux. What is "dbi" in U-Boot is "regs" in Linux :)
> 
> Well and there is a big-endian property, which of course is set for any
> Layerscape but the LS1028A. linux has instead just one endianess, but
> use a shift offset.

You lost me here. How are registers accessed with the right endianness
in Linux and in U-Boot?

> Mh, I'm for sure not going to fix all these hacks in there. I'm thinking
> about bascially branching to a different probe for the ls1028a based on
> the compatible string (or rather if .data is set or not).

Makes sense to introduce a driver data structure for LS1028A.