by Kishon Vijay Abraham I

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

Hi Rob,

On 5/8/2020 1:56 AM, Rob Herring wrote:
> On Wed, May 06, 2020 at 08:52:13AM +0530, Kishon Vijay Abraham I wrote:
>> Hi Robin,
>>
>> On 5/4/2020 6:23 PM, Kishon Vijay Abraham I wrote:
>>> Hi Robin,
>>>
>>> On 5/4/2020 4:24 PM, Robin Murphy wrote:
>>>> On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
>>>>> Hi Robin,
>>>>>
>>>>> On 5/1/2020 9:24 PM, Robin Murphy wrote:
>>>>>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>>>>>>> [+Robin - to check on dma-ranges intepretation]
>>>>>>>
>>>>>>> I would need RobH and Robin to review this.
>>>>>>>
>>>>>>> Also, An ACK from Tom is required - for the whole series.
>>>>>>>
>>>>>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>>>>>>> property to configure the number of bits passed through from PCIe
>>>>>>>> address to internal address in Inbound Address Translation register.
>>>>>>>>
>>>>>>>> However standard PCI dt-binding already defines "dma-ranges" to
>>>>>>>> describe the address range accessible by PCIe controller. Parse
>>>>>>>> "dma-ranges" property to configure the number of bits passed
>>>>>>>> through from PCIe address to internal address in Inbound Address
>>>>>>>> Translation register.
>>>>>>>>
>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
>>>>>>>> ---
>>>>>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> index 9b1c3966414b..60f912a657b9 100644
>>>>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>        struct device *dev = rc->pcie.dev;
>>>>>>>>        struct platform_device *pdev = to_platform_device(dev);
>>>>>>>>        struct device_node *np = dev->of_node;
>>>>>>>> +    struct of_pci_range_parser parser;
>>>>>>>>        struct pci_host_bridge *bridge;
>>>>>>>>        struct list_head resources;
>>>>>>>> +    struct of_pci_range range;
>>>>>>>>        struct cdns_pcie *pcie;
>>>>>>>>        struct resource *res;
>>>>>>>>        int ret;
>>>>>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>        rc->max_regions = 32;
>>>>>>>>        of_property_read_u32(np, "cdns,max-outbound-regions",
>>>>>>>> &rc->max_regions);
>>>>>>>>    -    rc->no_bar_nbits = 32;
>>>>>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>>>>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>>>>>>> +        if (of_pci_range_parser_one(&parser, &range))
>>>>>>>> +            rc->no_bar_nbits = ilog2(range.size);
>>>>>>
>>>>>> You probably want "range.pci_addr + range.size" here just in case the bottom of
>>>>>> the window is ever non-zero. Is there definitely only ever a single inbound
>>>>>> window to consider?
>>>>>
>>>>> Cadence IP has 3 inbound address translation registers, however we use only 1
>>>>> inbound address translation register to map the entire 32 bit or 64 bit address
>>>>> region.
>>>>
>>>> OK, if anything that further strengthens the argument for deprecating a single
>>>> "number of bits" property in favour of ranges that accurately describe the
>>>> window(s). However it also suggests that other users in future might have some
>>>> expectation that specifying "dma-ranges" with up to 3 entries should work to
>>>> allow a more restrictive inbound configuration. Thus it would be desirable to
>>>> make the code a little more robust here - even if we don't support multiple
>>>> windows straight off, it would still be better to implement it in a way that
>>>> can be cleanly extended later, and at least say something if more ranges are
>>>> specified rather than just silently ignoring them.
>>>
>>> I looked at this further in the Cadence user doc. The three inbound ATU entries
>>> are for BAR0, BAR1 in RC configuration space and the third one is for NO MATCH
>>> BAR when there is no matching found in RC BARs. Right now we always configure
>>> the NO MATCH BAR. Would it be possible describe at BAR granularity in dma-ranges?
>>
>> I was thinking if I could use something like
>> dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR0 IB mapping
>> <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR1 IB mapping
>> <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>; //NO MATCH BAR
>>
>> This way the driver can tell the 1st tuple is for BAR0, 2nd is for BAR1 and
>> last is for NO MATCH. In the above case both BAR0 and BAR1 is just empty and
>> doesn't have valid values as we use only the NO MATCH BAR.
>>
>> However I'm not able to use for_each_of_pci_range() in Cadence driver to get
>> the configuration for each BAR, since the for loop gets invoked only once since
>> of_pci_range_parser_one() merges contiguous addresses.
>
> NO_MATCH_BAR could just be the last entry no matter how many? Who cares
> if they get merged? Maybe each BAR has max size and dma-ranges could
> exceed that, but if so you have to handle that and split them again.

Each of RP_BAR0, RP_BAR1 and RP_NO_BAR has separate register to be configured.
If they get merged, we'll loose info on which of the registers to be
configured. Cadence IP specifies maximum size of BAR0 as 256GB, maximum size of
BAR1 as 2 GB. However when I specify dma-ranges like below and use
for_each_of_pci_range(&parser, &range), the first range itself is 258.

dma-ranges = <0x02000000 0x00 0x0 0x00 0x0 0x40 0x00000000>, /* BAR0 256 GB */
<0x02000000 0x40 0x0 0x40 0x0 0x00 0x80000000>; /* BAR1 2 GB */
>
>> Do you think I should extend the flags cell to differentiate between BAR0, BAR1
>> and NO MATCH BAR? Can you suggest any other alternatives?
>
> If you just have 1 region, then just 1 entry makes sense to me. Why
> can't you use BAR0 in that case?

Well, Cadence has specified a max size for each BAR. I think we could specify a
single region (48 bits in my case) in dma-ranges and let the driver decide how
to split it among BAR0, BAR1 and NO_MATCH_BAR?

Thanks
Kishon

2020-05-08 11:56:27

by Kishon Vijay Abraham I

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

Hi Rob, Robin,

On 5/8/2020 2:19 PM, Kishon Vijay Abraham I wrote:
> Hi Rob,
>
> On 5/8/2020 1:56 AM, Rob Herring wrote:
>> On Wed, May 06, 2020 at 08:52:13AM +0530, Kishon Vijay Abraham I wrote:
>>> Hi Robin,
>>>
>>> On 5/4/2020 6:23 PM, Kishon Vijay Abraham I wrote:
>>>> Hi Robin,
>>>>
>>>> On 5/4/2020 4:24 PM, Robin Murphy wrote:
>>>>> On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
>>>>>> Hi Robin,
>>>>>>
>>>>>> On 5/1/2020 9:24 PM, Robin Murphy wrote:
>>>>>>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>>>>>>>> [+Robin - to check on dma-ranges intepretation]
>>>>>>>>
>>>>>>>> I would need RobH and Robin to review this.
>>>>>>>>
>>>>>>>> Also, An ACK from Tom is required - for the whole series.
>>>>>>>>
>>>>>>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>>>>>>>> property to configure the number of bits passed through from PCIe
>>>>>>>>> address to internal address in Inbound Address Translation register.
>>>>>>>>>
>>>>>>>>> However standard PCI dt-binding already defines "dma-ranges" to
>>>>>>>>> describe the address range accessible by PCIe controller. Parse
>>>>>>>>> "dma-ranges" property to configure the number of bits passed
>>>>>>>>> through from PCIe address to internal address in Inbound Address
>>>>>>>>> Translation register.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
>>>>>>>>> ---
>>>>>>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>>>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> index 9b1c3966414b..60f912a657b9 100644
>>>>>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>>        struct device *dev = rc->pcie.dev;
>>>>>>>>>        struct platform_device *pdev = to_platform_device(dev);
>>>>>>>>>        struct device_node *np = dev->of_node;
>>>>>>>>> +    struct of_pci_range_parser parser;
>>>>>>>>>        struct pci_host_bridge *bridge;
>>>>>>>>>        struct list_head resources;
>>>>>>>>> +    struct of_pci_range range;
>>>>>>>>>        struct cdns_pcie *pcie;
>>>>>>>>>        struct resource *res;
>>>>>>>>>        int ret;
>>>>>>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>>        rc->max_regions = 32;
>>>>>>>>>        of_property_read_u32(np, "cdns,max-outbound-regions",
>>>>>>>>> &rc->max_regions);
>>>>>>>>>    -    rc->no_bar_nbits = 32;
>>>>>>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>>>>>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>>>>>>>> +        if (of_pci_range_parser_one(&parser, &range))
>>>>>>>>> +            rc->no_bar_nbits = ilog2(range.size);
>>>>>>>
>>>>>>> You probably want "range.pci_addr + range.size" here just in case the bottom of
>>>>>>> the window is ever non-zero. Is there definitely only ever a single inbound
>>>>>>> window to consider?
>>>>>>
>>>>>> Cadence IP has 3 inbound address translation registers, however we use only 1
>>>>>> inbound address translation register to map the entire 32 bit or 64 bit address
>>>>>> region.
>>>>>
>>>>> OK, if anything that further strengthens the argument for deprecating a single
>>>>> "number of bits" property in favour of ranges that accurately describe the
>>>>> window(s). However it also suggests that other users in future might have some
>>>>> expectation that specifying "dma-ranges" with up to 3 entries should work to
>>>>> allow a more restrictive inbound configuration. Thus it would be desirable to
>>>>> make the code a little more robust here - even if we don't support multiple
>>>>> windows straight off, it would still be better to implement it in a way that
>>>>> can be cleanly extended later, and at least say something if more ranges are
>>>>> specified rather than just silently ignoring them.
>>>>
>>>> I looked at this further in the Cadence user doc. The three inbound ATU entries
>>>> are for BAR0, BAR1 in RC configuration space and the third one is for NO MATCH
>>>> BAR when there is no matching found in RC BARs. Right now we always configure
>>>> the NO MATCH BAR. Would it be possible describe at BAR granularity in dma-ranges?
>>>
>>> I was thinking if I could use something like
>>> dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR0 IB mapping
>>> <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR1 IB mapping
>>> <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>; //NO MATCH BAR
>>>
>>> This way the driver can tell the 1st tuple is for BAR0, 2nd is for BAR1 and
>>> last is for NO MATCH. In the above case both BAR0 and BAR1 is just empty and
>>> doesn't have valid values as we use only the NO MATCH BAR.
>>>
>>> However I'm not able to use for_each_of_pci_range() in Cadence driver to get
>>> the configuration for each BAR, since the for loop gets invoked only once since
>>> of_pci_range_parser_one() merges contiguous addresses.
>>
>> NO_MATCH_BAR could just be the last entry no matter how many? Who cares
>> if they get merged? Maybe each BAR has max size and dma-ranges could
>> exceed that, but if so you have to handle that and split them again.
>
> Each of RP_BAR0, RP_BAR1 and RP_NO_BAR has separate register to be configured.
> If they get merged, we'll loose info on which of the registers to be
> configured. Cadence IP specifies maximum size of BAR0 as 256GB, maximum size of
> BAR1 as 2 GB. However when I specify dma-ranges like below and use
> for_each_of_pci_range(&parser, &range), the first range itself is 258.
>
> dma-ranges = <0x02000000 0x00 0x0 0x00 0x0 0x40 0x00000000>, /* BAR0 256 GB */
> <0x02000000 0x40 0x0 0x40 0x0 0x00 0x80000000>; /* BAR1 2 GB */
>>
>>> Do you think I should extend the flags cell to differentiate between BAR0, BAR1
>>> and NO MATCH BAR? Can you suggest any other alternatives?
>>
>> If you just have 1 region, then just 1 entry makes sense to me. Why
>> can't you use BAR0 in that case?
>
> Well, Cadence has specified a max size for each BAR. I think we could specify a
> single region (48 bits in my case) in dma-ranges and let the driver decide how
> to split it among BAR0, BAR1 and NO_MATCH_BAR?

Okay, I'll add support in driver for parsing multiple dma-ranges (non
consecutive regions) and driver splitting the regions based on maximum size
supported by each BAR.

This means, we will not directly use NO_MATCH_BAR, but wil first fill up BAR0,
BAR1 and then only the remaining space in NO_MATCH_BAR.

Thanks
Kishon