[PATCH v2] PCI: designware: Add support 4 ATUs assignment

[Lian Minghuan-B31939]

Hi Lucas,

On 2014年11月14日 19:42, Lucas Stach wrote:
> Am Freitag, den 14.11.2014, 11:30 +0000 schrieb
> Mingkai.Hu at freescale.com:
>>
>>> -----Original Message-----
>>> From: Lucas Stach [mailto:l.stach at pengutronix.de]
>>> Sent: Friday, November 14, 2014 6:02 PM
>>> To: Lian Minghuan-B31939
>>> Cc: Srikanth Thokala; Lian Minghuan-B31939; linux-pci at vger.kernel.org;
>>> linux-arm-kernel at lists.infradead.org; Zang Roy-R61911; Hu Mingkai-B21284;
>>> Bjorn Helgaas
>>> Subject: Re: [PATCH v2] PCI: designware: Add support 4 ATUs assignment
>>>
>>> Am Freitag, den 14.11.2014, 16:47 +0800 schrieb Lian Minghuan-B31939:
>>>> Hi Lucas and all,
>>>>
>>>> On 2014年11月13日 19:09, Lucas Stach wrote:
>>>>> Am Donnerstag, den 13.11.2014, 18:52 +0800 schrieb Lian Minghuan-
>>> B31939:
>>>>>> Hi Lucas,
>>>>>>
>>>>>> On 2014年11月13日 18:20, Lucas Stach wrote:
>>>>>>> Am Donnerstag, den 13.11.2014, 18:02 +0800 schrieb Lian Minghuan-
>>> B31939:
>>>>>>>> Hi Lucas,
>>>>>>>>
>>>>>>>> Please see my comments inline.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Minghuan
>>>>>>>>
>>>>>>>> On 2014年11月13日 00:32, Lucas Stach wrote:
>>>>>>>>> Am Mittwoch, den 12.11.2014, 21:53 +0530 schrieb Srikanth Thokala:
>>>>>>>>>> Hi Minghuan,
>>>>>>> [...]
>>>>>>>
>>>>>>>>> Using a smaller type complicates the DT for little to no
>>>>>>>>> benefit. I think it's ok to use u32 here, which is a common
>>>>>>>>> standard for integer values in DT.
>>>>>>>>>
>>>>>>>>> Though this discussion lead me to the question if we even need
>>>>>>>>> to have this property in the DT at all. Isn't this a property
>>>>>>>>> that is fixed for a specific silicon implementation of the DW
>>>>>>>>> core? In that case we could just infer the number of ATUs from
>>>>>>>>> the DT compatible, so this should probably just be added to
>>>>>>>>> struct pcie_port and properly initialized by the SoC glue drivers.
>>>>>>>> [Minghuan]  As far as I know, exynos implements only 2 ATUs, this
>>>>>>>> is why pcie-designware only supports 2 ATU. iMX implements 4 ATUs
>>>>>>>> and LS1021A implements 6 ATUs.
>>>>>>>>
>>>>>>> Right so we don't need an additional property in the DT at all.
>>>>>>> The number of ATUs is fixed for a specific core compatible and can
>>>>>>> be passed in by the respective exynos, imx and ls1021 glue drivers.
>>>>>>>
>>>>>>> You may ask the Keystone and Spear maintainers to get the correct
>>>>>>> number of ATUs for those implementations.
>>   > >>>
>>>>>>> Regards,
>>>>>>> Lucas
>>>>>> [Minghuan] Yes. This a way that specific core driver passes the ATU
>>>>>> number to pci-designware. But I perfer to adding dts node for the
>>>>>> following reasons:
>>>>>> 1. ATU number is hardware attribute, so it can be added to DTS.
>>>>> But it is a duplication of information that can be inferred from the
>>>>> DT compatible alone, which is usually frowned upon.
>>>>>
>>>>> Also in contrast to the num-lanes property I don't see a use-case to
>>>>> reduce the number of used ATUs in a specific system, so num-atus is
>>>>> basically fixed for a specific implementation.
>>>> [Minghuan] 4ATUs provides better support for multiple-function and
>>>> SR-IOV PCIe devices.
>>>> Can anyone tell me if there is the company will increase ATUs number?
>>>> If yes, we should avoid the following code:
>>>>
>>>> if (of_device_is_compatible(pcie->dev->of_node, "xxx,r1-pcie"))
>>>>       atu_num = 2;
>>>>
>>>> if (of_device_is_compatible(pcie->dev->of_node, "xxx,r2-pcie"))
>>>>       atu_num = 4;
>>>>
>>> The number of ATUs is fixed for a specific implementation. If the number
>>> of ATUs changes in a newer implementation of a SoC then the silicon
>>> implementation of the DW PCIe core changed, so it should get a new
>>> compatible anyway.
>>>
>> If there are multiple platforms to use the same SoC driver, there will be
>> multiple cases for different platforms as Minghuan has pointed out.
>>
>> For the platform that only has 2 ATUs, ATU0 is used for CFG0 and MEM and
>> there will be conflict when MEM and CFG0 are accessed simultaneously which
>> has cause the SATA link issue. I think more ATUs will be added in the
>> future design, then one driver maybe need to handle multiple platforms.
>>
>> DTS is designed for to describe the hardware property, it's a general way
>> to put hardware related property into DTS.
>>
> If another platform uses the same driver you should still have a
> specific compatible for that platform. Exactly to differentiate those
> silicon implementation differences.
>
> If you add 2 more ATUs to the layerscape pcie in a future design it is
> not fsl,ls1021a-pcie anymore, but for example fsl,ls1100-pcie. Also you
> don't need the code construct as seen above. The number of ATU can be
> added to implementation specific data.
> Please look at the Tegra PCI driver and especially struct
> tegra_pcie_soc_data on how to properly abstract properties that are
> defined by a specific silicon implementation of a core.
[Minghuan] Yes, we can added specific data for different implementation.
But we need to add hard coded information and assignment statement to
the layerscape PCIe driver even to every PCIe driver based on 
pcie-designware.c.

Why not use dts?  Nothing needs to be added to specific implementation 
driver code.

The Device Tree is a data structure for describing hardware. Rather than 
hard coding every detail of a device into an operating system, many 
aspect of the hardware can be described in a data structure that is 
passed to the operating system at boot time.

Thanks,
Minghuan

> Regards,
> Lucas
>