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

[Michael Walle]

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 ;)

> 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.

> 	/*
> 	 * 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.

> 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.

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).

-michael