[PATCH] of: treat PCI config space as IORESOURCE_MEM type

[PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Kumar Gala]

If we have a PCI config space specified in something like a ranges
property we should treat it as memory type resource.

Signed-off-by: Kumar Gala <galak@codeaurora.org>
---
 drivers/of/address.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index cb4242a..4e7ee59 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
 	u32 w = be32_to_cpup(addr);
 
 	switch((w >> 24) & 0x03) {
+	case 0x00: /* cfg space */
+		flags |= IORESOURCE_MEM;
+		break;
 	case 0x01:
 		flags |= IORESOURCE_IO;
 		break;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Rob Herring]

On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
> If we have a PCI config space specified in something like a ranges
> property we should treat it as memory type resource.

Config space should not be in ranges[1]. We have some cases that are,
but we don't want new ones.

>
> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> ---
>  drivers/of/address.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index cb4242a..4e7ee59 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
>         u32 w = be32_to_cpup(addr);
>
>         switch((w >> 24) & 0x03) {
> +       case 0x00: /* cfg space */
> +               flags |= IORESOURCE_MEM;
> +               break;

How would you then distinguish actual memory ranges?

Rob

[1] http://www.spinics.net/lists/linux-pci/msg30585.html

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Kumar Gala]

On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:

> On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
>> If we have a PCI config space specified in something like a ranges
>> property we should treat it as memory type resource.
> 
> Config space should not be in ranges[1]. We have some cases that are,
> but we don't want new ones.

For the cases we have I agree, however an ECAM based cfg seems completely legit.

>> Signed-off-by: Kumar Gala <galak@codeaurora.org>
>> ---
>> drivers/of/address.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index cb4242a..4e7ee59 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
>>        u32 w = be32_to_cpup(addr);
>> 
>>        switch((w >> 24) & 0x03) {
>> +       case 0x00: /* cfg space */
>> +               flags |= IORESOURCE_MEM;
>> +               break;
> 
> How would you then distinguish actual memory ranges?

One assumes you are still looking at pci_space as part of of_pci_range

> 
> Rob
> 
> [1] http://www.spinics.net/lists/linux-pci/msg30585.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Rob Herring]

Adding Jason Gunthorpe...

On Thu, May 29, 2014 at 3:51 PM, Kumar Gala <galak@codeaurora.org> wrote:
>
> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
>
>> On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
>>> If we have a PCI config space specified in something like a ranges
>>> property we should treat it as memory type resource.
>>
>> Config space should not be in ranges[1]. We have some cases that are,
>> but we don't want new ones.
>
> For the cases we have I agree, however an ECAM based cfg seems completely legit.

The previous discussion concluded otherwise. Debate it with Jason. I
don't really have an opinion other than it be done one way.

Rob

>>> Signed-off-by: Kumar Gala <galak@codeaurora.org>
>>> ---
>>> drivers/of/address.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>> index cb4242a..4e7ee59 100644
>>> --- a/drivers/of/address.c
>>> +++ b/drivers/of/address.c
>>> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
>>>        u32 w = be32_to_cpup(addr);
>>>
>>>        switch((w >> 24) & 0x03) {
>>> +       case 0x00: /* cfg space */
>>> +               flags |= IORESOURCE_MEM;
>>> +               break;
>>
>> How would you then distinguish actual memory ranges?
>
> One assumes you are still looking at pci_space as part of of_pci_range
>
>>
>> Rob
>>
>> [1] http://www.spinics.net/lists/linux-pci/msg30585.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Liviu Dudau]

On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
> 
> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
> 
> > On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
> >> If we have a PCI config space specified in something like a ranges
> >> property we should treat it as memory type resource.
> > 
> > Config space should not be in ranges[1]. We have some cases that are,
> > but we don't want new ones.
> 
> For the cases we have I agree, however an ECAM based cfg seems completely legit.
> 
> >> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> >> ---
> >> drivers/of/address.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/of/address.c b/drivers/of/address.c
> >> index cb4242a..4e7ee59 100644
> >> --- a/drivers/of/address.c
> >> +++ b/drivers/of/address.c
> >> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
> >>        u32 w = be32_to_cpup(addr);
> >> 
> >>        switch((w >> 24) & 0x03) {
> >> +       case 0x00: /* cfg space */
> >> +               flags |= IORESOURCE_MEM;
> >> +               break;
> > 
> > How would you then distinguish actual memory ranges?
> 
> One assumes you are still looking at pci_space as part of of_pci_range

That doesn't happen when you start scanning the bus. The existing code will
use the IORESOURCE_MEM for allocating memory space for devices, which is
not what you want. Did you test your patch on any PCI system? I'm pretty
sure that with my patch series that tries to make a generic framework for
host controllers this will fail.

We really need a IORESOURCE_CFG flag for this space.

Best regards,
Liviu

> 
> > 
> > Rob
> > 
> > [1] http://www.spinics.net/lists/linux-pci/msg30585.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Bjorn Helgaas]

On Thu, May 29, 2014 at 6:56 PM, Liviu Dudau <liviu-I3yL/QOVVjH10XsdtD+oqA@public.gmane.org> wrote:
> On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
>>
>> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> >> If we have a PCI config space specified in something like a ranges
>> >> property we should treat it as memory type resource.
>> >
>> > Config space should not be in ranges[1]. We have some cases that are,
>> > but we don't want new ones.
>>
>> For the cases we have I agree, however an ECAM based cfg seems completely legit.
>>
>> >> Signed-off-by: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> >> ---
>> >> drivers/of/address.c | 3 +++
>> >> 1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> >> index cb4242a..4e7ee59 100644
>> >> --- a/drivers/of/address.c
>> >> +++ b/drivers/of/address.c
>> >> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
>> >>        u32 w = be32_to_cpup(addr);
>> >>
>> >>        switch((w >> 24) & 0x03) {
>> >> +       case 0x00: /* cfg space */
>> >> +               flags |= IORESOURCE_MEM;
>> >> +               break;
>> >
>> > How would you then distinguish actual memory ranges?
>>
>> One assumes you are still looking at pci_space as part of of_pci_range
>
> That doesn't happen when you start scanning the bus. The existing code will
> use the IORESOURCE_MEM for allocating memory space for devices, which is
> not what you want. Did you test your patch on any PCI system? I'm pretty
> sure that with my patch series that tries to make a generic framework for
> host controllers this will fail.
>
> We really need a IORESOURCE_CFG flag for this space.

Maybe, but I'm not convinced yet.  The existing IORESOURCE_TYPE_BITS
types are for things that are mutually exclusive address spaces.  I
think this discussion is about ECAM, where the CPU side is definitely
in the same address space (IORESOURCE_MEM) as RAM, APICs, host bridge
apertures, device MMIO, etc.  The ECAM area must appear in the
iomem_resource tree so we avoid it when allocating other areas.

>> > [1] http://www.spinics.net/lists/linux-pci/msg30585.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Liviu Dudau]

On Thu, May 29, 2014 at 07:29:31PM -0600, Bjorn Helgaas wrote:
> On Thu, May 29, 2014 at 6:56 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> > On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
> >>
> >> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
> >>
> >> > On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
> >> >> If we have a PCI config space specified in something like a ranges
> >> >> property we should treat it as memory type resource.
> >> >
> >> > Config space should not be in ranges[1]. We have some cases that are,
> >> > but we don't want new ones.
> >>
> >> For the cases we have I agree, however an ECAM based cfg seems completely legit.
> >>
> >> >> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> >> >> ---
> >> >> drivers/of/address.c | 3 +++
> >> >> 1 file changed, 3 insertions(+)
> >> >>
> >> >> diff --git a/drivers/of/address.c b/drivers/of/address.c
> >> >> index cb4242a..4e7ee59 100644
> >> >> --- a/drivers/of/address.c
> >> >> +++ b/drivers/of/address.c
> >> >> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
> >> >>        u32 w = be32_to_cpup(addr);
> >> >>
> >> >>        switch((w >> 24) & 0x03) {
> >> >> +       case 0x00: /* cfg space */
> >> >> +               flags |= IORESOURCE_MEM;
> >> >> +               break;
> >> >
> >> > How would you then distinguish actual memory ranges?
> >>
> >> One assumes you are still looking at pci_space as part of of_pci_range
> >
> > That doesn't happen when you start scanning the bus. The existing code will
> > use the IORESOURCE_MEM for allocating memory space for devices, which is
> > not what you want. Did you test your patch on any PCI system? I'm pretty
> > sure that with my patch series that tries to make a generic framework for
> > host controllers this will fail.
> >
> > We really need a IORESOURCE_CFG flag for this space.
> 
> Maybe, but I'm not convinced yet.  The existing IORESOURCE_TYPE_BITS
> types are for things that are mutually exclusive address spaces.  I
> think this discussion is about ECAM, where the CPU side is definitely
> in the same address space (IORESOURCE_MEM) as RAM, APICs, host bridge
> apertures, device MMIO, etc.  The ECAM area must appear in the
> iomem_resource tree so we avoid it when allocating other areas.

Agree, I'm only concerned that if this ECAM config space gets added to
the list of pci_host_bridge windows it will be indistinguishable from
IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
bus and allow devices present on that bus to be assigned addresses from
that range. Which might not be what one wants for certain BARs.

I've had an aborted attempt to parse ECAM ranges in one version of my
series (granted, I was trying to hack the IORESOURCE_TYPE_BITS as well)
and things got horribly wrong quickly. I could give this patch a go with
my series tomorrow when I'm in the office and report back.

Best regards,
Liviu

> 
> >> > [1] http://www.spinics.net/lists/linux-pci/msg30585.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Jason Gunthorpe]

On Fri, May 30, 2014 at 02:41:17AM +0100, Liviu Dudau wrote:

> Agree, I'm only concerned that if this ECAM config space gets added to
> the list of pci_host_bridge windows it will be indistinguishable from
> IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
> bus and allow devices present on that bus to be assigned addresses from
> that range. Which might not be what one wants for certain BARs.

I wouldn't worry about supporting config in ranges. ECAM is the
logical use for config ranges, but it isn't specified and probably
will never be.

Will's driver the is the only driver I've seen to support ECAM and it
didn't use ranges.

Jason

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Kumar Gala]

On May 30, 2014, at 3:37 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Fri, May 30, 2014 at 02:41:17AM +0100, Liviu Dudau wrote:
> 
>> Agree, I'm only concerned that if this ECAM config space gets added to
>> the list of pci_host_bridge windows it will be indistinguishable from
>> IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
>> bus and allow devices present on that bus to be assigned addresses from
>> that range. Which might not be what one wants for certain BARs.
> 
> I wouldn't worry about supporting config in ranges. ECAM is the
> logical use for config ranges, but it isn't specified and probably
> will never be.
> 
> Will's driver the is the only driver I've seen to support ECAM and it
> didn't use ranges.

I expect with 64-bit parts we will see more use of ECAM, I think the reason its not used much is because of the address space it chews up, but that becomes less of an issue with LPAE or 64-bit parts with larger physical address spaces.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Kumar Gala]

On May 29, 2014, at 8:41 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:

> On Thu, May 29, 2014 at 07:29:31PM -0600, Bjorn Helgaas wrote:
>> On Thu, May 29, 2014 at 6:56 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
>>> On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
>>>> 
>>>> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>> 
>>>>> On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
>>>>>> If we have a PCI config space specified in something like a ranges
>>>>>> property we should treat it as memory type resource.
>>>>> 
>>>>> Config space should not be in ranges[1]. We have some cases that are,
>>>>> but we don't want new ones.
>>>> 
>>>> For the cases we have I agree, however an ECAM based cfg seems completely legit.
>>>> 
>>>>>> Signed-off-by: Kumar Gala <galak@codeaurora.org>
>>>>>> ---
>>>>>> drivers/of/address.c | 3 +++
>>>>>> 1 file changed, 3 insertions(+)
>>>>>> 
>>>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>>>>> index cb4242a..4e7ee59 100644
>>>>>> --- a/drivers/of/address.c
>>>>>> +++ b/drivers/of/address.c
>>>>>> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
>>>>>>       u32 w = be32_to_cpup(addr);
>>>>>> 
>>>>>>       switch((w >> 24) & 0x03) {
>>>>>> +       case 0x00: /* cfg space */
>>>>>> +               flags |= IORESOURCE_MEM;
>>>>>> +               break;
>>>>> 
>>>>> How would you then distinguish actual memory ranges?
>>>> 
>>>> One assumes you are still looking at pci_space as part of of_pci_range
>>> 
>>> That doesn't happen when you start scanning the bus. The existing code will
>>> use the IORESOURCE_MEM for allocating memory space for devices, which is
>>> not what you want. Did you test your patch on any PCI system? I'm pretty
>>> sure that with my patch series that tries to make a generic framework for
>>> host controllers this will fail.
>>> 
>>> We really need a IORESOURCE_CFG flag for this space.
>> 
>> Maybe, but I'm not convinced yet.  The existing IORESOURCE_TYPE_BITS
>> types are for things that are mutually exclusive address spaces.  I
>> think this discussion is about ECAM, where the CPU side is definitely
>> in the same address space (IORESOURCE_MEM) as RAM, APICs, host bridge
>> apertures, device MMIO, etc.  The ECAM area must appear in the
>> iomem_resource tree so we avoid it when allocating other areas.
> 
> Agree, I'm only concerned that if this ECAM config space gets added to
> the list of pci_host_bridge windows it will be indistinguishable from
> IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
> bus and allow devices present on that bus to be assigned addresses from
> that range. Which might not be what one wants for certain BARs.
> 
> I've had an aborted attempt to parse ECAM ranges in one version of my
> series (granted, I was trying to hack the IORESOURCE_TYPE_BITS as well)
> and things got horribly wrong quickly. I could give this patch a go with
> my series tomorrow when I'm in the office and report back.

We need to fix the parsing code to be smarter about this case.

- k
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Liviu Dudau]

On Fri, May 30, 2014 at 03:45:05PM -0500, Kumar Gala wrote:
> 
> On May 29, 2014, at 8:41 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> 
> > On Thu, May 29, 2014 at 07:29:31PM -0600, Bjorn Helgaas wrote:
> >> On Thu, May 29, 2014 at 6:56 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> >>> On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
> >>>> 
> >>>> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
> >>>> 
> >>>>> On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
> >>>>>> If we have a PCI config space specified in something like a ranges
> >>>>>> property we should treat it as memory type resource.
> >>>>> 
> >>>>> Config space should not be in ranges[1]. We have some cases that are,
> >>>>> but we don't want new ones.
> >>>> 
> >>>> For the cases we have I agree, however an ECAM based cfg seems completely legit.
> >>>> 
> >>>>>> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> >>>>>> ---
> >>>>>> drivers/of/address.c | 3 +++
> >>>>>> 1 file changed, 3 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
> >>>>>> index cb4242a..4e7ee59 100644
> >>>>>> --- a/drivers/of/address.c
> >>>>>> +++ b/drivers/of/address.c
> >>>>>> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
> >>>>>>       u32 w = be32_to_cpup(addr);
> >>>>>> 
> >>>>>>       switch((w >> 24) & 0x03) {
> >>>>>> +       case 0x00: /* cfg space */
> >>>>>> +               flags |= IORESOURCE_MEM;
> >>>>>> +               break;
> >>>>> 
> >>>>> How would you then distinguish actual memory ranges?
> >>>> 
> >>>> One assumes you are still looking at pci_space as part of of_pci_range
> >>> 
> >>> That doesn't happen when you start scanning the bus. The existing code will
> >>> use the IORESOURCE_MEM for allocating memory space for devices, which is
> >>> not what you want. Did you test your patch on any PCI system? I'm pretty
> >>> sure that with my patch series that tries to make a generic framework for
> >>> host controllers this will fail.
> >>> 
> >>> We really need a IORESOURCE_CFG flag for this space.
> >> 
> >> Maybe, but I'm not convinced yet.  The existing IORESOURCE_TYPE_BITS
> >> types are for things that are mutually exclusive address spaces.  I
> >> think this discussion is about ECAM, where the CPU side is definitely
> >> in the same address space (IORESOURCE_MEM) as RAM, APICs, host bridge
> >> apertures, device MMIO, etc.  The ECAM area must appear in the
> >> iomem_resource tree so we avoid it when allocating other areas.
> > 
> > Agree, I'm only concerned that if this ECAM config space gets added to
> > the list of pci_host_bridge windows it will be indistinguishable from
> > IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
> > bus and allow devices present on that bus to be assigned addresses from
> > that range. Which might not be what one wants for certain BARs.
> > 
> > I've had an aborted attempt to parse ECAM ranges in one version of my
> > series (granted, I was trying to hack the IORESOURCE_TYPE_BITS as well)
> > and things got horribly wrong quickly. I could give this patch a go with
> > my series tomorrow when I'm in the office and report back.
> 
> We need to fix the parsing code to be smarter about this case.

Wow, what a sweeping statement! Did you not understand that the issue is not
the parsing code but the way the rest of the core code uses an IORESOURCE_MEM
once you have parsed it into a resource structure and added it to the list
of pci_host_bridge_windows?

Best regards,
Liviu

> 
> - k
> -- 
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Bjorn Helgaas]

On Fri, May 30, 2014 at 5:11 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> On Fri, May 30, 2014 at 03:45:05PM -0500, Kumar Gala wrote:
>>
>> On May 29, 2014, at 8:41 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
>>
>> > On Thu, May 29, 2014 at 07:29:31PM -0600, Bjorn Helgaas wrote:
>> >> On Thu, May 29, 2014 at 6:56 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
>> >>> On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
>> >>>>
>> >>>> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
>> >>>>
>> >>>>> On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
>> >>>>>> If we have a PCI config space specified in something like a ranges
>> >>>>>> property we should treat it as memory type resource.
>> >>>>>
>> >>>>> Config space should not be in ranges[1]. We have some cases that are,
>> >>>>> but we don't want new ones.
>> >>>>
>> >>>> For the cases we have I agree, however an ECAM based cfg seems completely legit.
>> >>>>
>> >>>>>> Signed-off-by: Kumar Gala <galak@codeaurora.org>
>> >>>>>> ---
>> >>>>>> drivers/of/address.c | 3 +++
>> >>>>>> 1 file changed, 3 insertions(+)
>> >>>>>>
>> >>>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> >>>>>> index cb4242a..4e7ee59 100644
>> >>>>>> --- a/drivers/of/address.c
>> >>>>>> +++ b/drivers/of/address.c
>> >>>>>> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
>> >>>>>>       u32 w = be32_to_cpup(addr);
>> >>>>>>
>> >>>>>>       switch((w >> 24) & 0x03) {
>> >>>>>> +       case 0x00: /* cfg space */
>> >>>>>> +               flags |= IORESOURCE_MEM;
>> >>>>>> +               break;
>> >>>>>
>> >>>>> How would you then distinguish actual memory ranges?
>> >>>>
>> >>>> One assumes you are still looking at pci_space as part of of_pci_range
>> >>>
>> >>> That doesn't happen when you start scanning the bus. The existing code will
>> >>> use the IORESOURCE_MEM for allocating memory space for devices, which is
>> >>> not what you want. Did you test your patch on any PCI system? I'm pretty
>> >>> sure that with my patch series that tries to make a generic framework for
>> >>> host controllers this will fail.
>> >>>
>> >>> We really need a IORESOURCE_CFG flag for this space.
>> >>
>> >> Maybe, but I'm not convinced yet.  The existing IORESOURCE_TYPE_BITS
>> >> types are for things that are mutually exclusive address spaces.  I
>> >> think this discussion is about ECAM, where the CPU side is definitely
>> >> in the same address space (IORESOURCE_MEM) as RAM, APICs, host bridge
>> >> apertures, device MMIO, etc.  The ECAM area must appear in the
>> >> iomem_resource tree so we avoid it when allocating other areas.
>> >
>> > Agree, I'm only concerned that if this ECAM config space gets added to
>> > the list of pci_host_bridge windows it will be indistinguishable from
>> > IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
>> > bus and allow devices present on that bus to be assigned addresses from
>> > that range. Which might not be what one wants for certain BARs.
>> >
>> > I've had an aborted attempt to parse ECAM ranges in one version of my
>> > series (granted, I was trying to hack the IORESOURCE_TYPE_BITS as well)
>> > and things got horribly wrong quickly. I could give this patch a go with
>> > my series tomorrow when I'm in the office and report back.
>>
>> We need to fix the parsing code to be smarter about this case.
>
> Wow, what a sweeping statement! Did you not understand that the issue is not
> the parsing code but the way the rest of the core code uses an IORESOURCE_MEM
> once you have parsed it into a resource structure and added it to the list
> of pci_host_bridge_windows?

Why do you want to add the ECAM area to the list of host bridge
windows?  My intent was that the windows tell the core what resources
are available for devices behind the bridge.

Bjorn

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Liviu Dudau]

On Fri, May 30, 2014 at 05:16:52PM -0600, Bjorn Helgaas wrote:
> On Fri, May 30, 2014 at 5:11 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> > On Fri, May 30, 2014 at 03:45:05PM -0500, Kumar Gala wrote:
> >>
> >> On May 29, 2014, at 8:41 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> >>
> >> > On Thu, May 29, 2014 at 07:29:31PM -0600, Bjorn Helgaas wrote:
> >> >> On Thu, May 29, 2014 at 6:56 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> >> >>> On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
> >> >>>>
> >> >>>> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
> >> >>>>
> >> >>>>> On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
> >> >>>>>> If we have a PCI config space specified in something like a ranges
> >> >>>>>> property we should treat it as memory type resource.
> >> >>>>>
> >> >>>>> Config space should not be in ranges[1]. We have some cases that are,
> >> >>>>> but we don't want new ones.
> >> >>>>
> >> >>>> For the cases we have I agree, however an ECAM based cfg seems completely legit.
> >> >>>>
> >> >>>>>> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> >> >>>>>> ---
> >> >>>>>> drivers/of/address.c | 3 +++
> >> >>>>>> 1 file changed, 3 insertions(+)
> >> >>>>>>
> >> >>>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
> >> >>>>>> index cb4242a..4e7ee59 100644
> >> >>>>>> --- a/drivers/of/address.c
> >> >>>>>> +++ b/drivers/of/address.c
> >> >>>>>> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
> >> >>>>>>       u32 w = be32_to_cpup(addr);
> >> >>>>>>
> >> >>>>>>       switch((w >> 24) & 0x03) {
> >> >>>>>> +       case 0x00: /* cfg space */
> >> >>>>>> +               flags |= IORESOURCE_MEM;
> >> >>>>>> +               break;
> >> >>>>>
> >> >>>>> How would you then distinguish actual memory ranges?
> >> >>>>
> >> >>>> One assumes you are still looking at pci_space as part of of_pci_range
> >> >>>
> >> >>> That doesn't happen when you start scanning the bus. The existing code will
> >> >>> use the IORESOURCE_MEM for allocating memory space for devices, which is
> >> >>> not what you want. Did you test your patch on any PCI system? I'm pretty
> >> >>> sure that with my patch series that tries to make a generic framework for
> >> >>> host controllers this will fail.
> >> >>>
> >> >>> We really need a IORESOURCE_CFG flag for this space.
> >> >>
> >> >> Maybe, but I'm not convinced yet.  The existing IORESOURCE_TYPE_BITS
> >> >> types are for things that are mutually exclusive address spaces.  I
> >> >> think this discussion is about ECAM, where the CPU side is definitely
> >> >> in the same address space (IORESOURCE_MEM) as RAM, APICs, host bridge
> >> >> apertures, device MMIO, etc.  The ECAM area must appear in the
> >> >> iomem_resource tree so we avoid it when allocating other areas.
> >> >
> >> > Agree, I'm only concerned that if this ECAM config space gets added to
> >> > the list of pci_host_bridge windows it will be indistinguishable from
> >> > IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
> >> > bus and allow devices present on that bus to be assigned addresses from
> >> > that range. Which might not be what one wants for certain BARs.
> >> >
> >> > I've had an aborted attempt to parse ECAM ranges in one version of my
> >> > series (granted, I was trying to hack the IORESOURCE_TYPE_BITS as well)
> >> > and things got horribly wrong quickly. I could give this patch a go with
> >> > my series tomorrow when I'm in the office and report back.
> >>
> >> We need to fix the parsing code to be smarter about this case.
> >
> > Wow, what a sweeping statement! Did you not understand that the issue is not
> > the parsing code but the way the rest of the core code uses an IORESOURCE_MEM
> > once you have parsed it into a resource structure and added it to the list
> > of pci_host_bridge_windows?
> 
> Why do you want to add the ECAM area to the list of host bridge
> windows?  My intent was that the windows tell the core what resources
> are available for devices behind the bridge.

I don't *want*, it is just that with my series that enhances Andrew's
parses of the ranges they all come together as host bridge windows. And it is
quite natural to put them all together when creating the root bus as the
space needs to be added to the iomem_resource tree anyway and that will happen
without special casing.

But maybe I'm wrong with that idea. What I know for sure is that wherever you
are going to pass the ECAM range converted to an IORESOURCE_MEM, the existing
code will not be able to distinguish it from a normal IORESOURCE_MEM and it
will treat it as a non-prefetcheable memory area. Is that how we want to treat
the ECFG space or do we want to special case it?

Best regards,
Liviu

> 
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [PATCH] of: treat PCI config space as IORESOURCE_MEM type

[Liviu Dudau]

On Sat, May 31, 2014 at 12:30:34AM +0100, Liviu Dudau wrote:
> On Fri, May 30, 2014 at 05:16:52PM -0600, Bjorn Helgaas wrote:
> > On Fri, May 30, 2014 at 5:11 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> > > On Fri, May 30, 2014 at 03:45:05PM -0500, Kumar Gala wrote:
> > >>
> > >> On May 29, 2014, at 8:41 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> > >>
> > >> > On Thu, May 29, 2014 at 07:29:31PM -0600, Bjorn Helgaas wrote:
> > >> >> On Thu, May 29, 2014 at 6:56 PM, Liviu Dudau <liviu@dudau.co.uk> wrote:
> > >> >>> On Thu, May 29, 2014 at 03:51:28PM -0500, Kumar Gala wrote:
> > >> >>>>
> > >> >>>> On May 29, 2014, at 3:44 PM, Rob Herring <robherring2@gmail.com> wrote:
> > >> >>>>
> > >> >>>>> On Thu, May 29, 2014 at 11:03 AM, Kumar Gala <galak@codeaurora.org> wrote:
> > >> >>>>>> If we have a PCI config space specified in something like a ranges
> > >> >>>>>> property we should treat it as memory type resource.
> > >> >>>>>
> > >> >>>>> Config space should not be in ranges[1]. We have some cases that are,
> > >> >>>>> but we don't want new ones.
> > >> >>>>
> > >> >>>> For the cases we have I agree, however an ECAM based cfg seems completely legit.
> > >> >>>>
> > >> >>>>>> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> > >> >>>>>> ---
> > >> >>>>>> drivers/of/address.c | 3 +++
> > >> >>>>>> 1 file changed, 3 insertions(+)
> > >> >>>>>>
> > >> >>>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
> > >> >>>>>> index cb4242a..4e7ee59 100644
> > >> >>>>>> --- a/drivers/of/address.c
> > >> >>>>>> +++ b/drivers/of/address.c
> > >> >>>>>> @@ -122,6 +122,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
> > >> >>>>>>       u32 w = be32_to_cpup(addr);
> > >> >>>>>>
> > >> >>>>>>       switch((w >> 24) & 0x03) {
> > >> >>>>>> +       case 0x00: /* cfg space */
> > >> >>>>>> +               flags |= IORESOURCE_MEM;
> > >> >>>>>> +               break;
> > >> >>>>>
> > >> >>>>> How would you then distinguish actual memory ranges?
> > >> >>>>
> > >> >>>> One assumes you are still looking at pci_space as part of of_pci_range
> > >> >>>
> > >> >>> That doesn't happen when you start scanning the bus. The existing code will
> > >> >>> use the IORESOURCE_MEM for allocating memory space for devices, which is
> > >> >>> not what you want. Did you test your patch on any PCI system? I'm pretty
> > >> >>> sure that with my patch series that tries to make a generic framework for
> > >> >>> host controllers this will fail.
> > >> >>>
> > >> >>> We really need a IORESOURCE_CFG flag for this space.
> > >> >>
> > >> >> Maybe, but I'm not convinced yet.  The existing IORESOURCE_TYPE_BITS
> > >> >> types are for things that are mutually exclusive address spaces.  I
> > >> >> think this discussion is about ECAM, where the CPU side is definitely
> > >> >> in the same address space (IORESOURCE_MEM) as RAM, APICs, host bridge
> > >> >> apertures, device MMIO, etc.  The ECAM area must appear in the
> > >> >> iomem_resource tree so we avoid it when allocating other areas.
> > >> >
> > >> > Agree, I'm only concerned that if this ECAM config space gets added to
> > >> > the list of pci_host_bridge windows it will be indistinguishable from
> > >> > IORESOURCE_MEM resources and pci_create_root_bus() will add it to the
> > >> > bus and allow devices present on that bus to be assigned addresses from
> > >> > that range. Which might not be what one wants for certain BARs.
> > >> >
> > >> > I've had an aborted attempt to parse ECAM ranges in one version of my
> > >> > series (granted, I was trying to hack the IORESOURCE_TYPE_BITS as well)
> > >> > and things got horribly wrong quickly. I could give this patch a go with
> > >> > my series tomorrow when I'm in the office and report back.
> > >>
> > >> We need to fix the parsing code to be smarter about this case.
> > >
> > > Wow, what a sweeping statement! Did you not understand that the issue is not
> > > the parsing code but the way the rest of the core code uses an IORESOURCE_MEM
> > > once you have parsed it into a resource structure and added it to the list
> > > of pci_host_bridge_windows?
> >
> > Why do you want to add the ECAM area to the list of host bridge
> > windows?  My intent was that the windows tell the core what resources
> > are available for devices behind the bridge.
>
> I don't *want*, it is just that with my series that enhances Andrew's
> parses of the ranges they all come together as host bridge windows. And it is
> quite natural to put them all together when creating the root bus as the
> space needs to be added to the iomem_resource tree anyway and that will happen
> without special casing.
>
> But maybe I'm wrong with that idea. What I know for sure is that wherever you
> are going to pass the ECAM range converted to an IORESOURCE_MEM, the existing
> code will not be able to distinguish it from a normal IORESOURCE_MEM and it
> will treat it as a non-prefetcheable memory area. Is that how we want to treat
> the ECFG space or do we want to special case it?

OK, how about these patches? They should allow one to be able to distinguish
between standard IORESOURCE_MEM and ECFG one.

Best regards,
Liviu

[PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Liviu Dudau]

We would like to be able to describe PCIe ECAM resources as
IORESOURCE_MEM blocks while distinguish them from standard
memory resources. Add an IORESOURCE_BIT entry for this case.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 include/linux/ioport.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 5e3a906..9672533 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -103,6 +103,7 @@ struct resource {
 
 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */
+#define IORESOURCE_PCI_ECFG		(1<<5)  /* ECAM config resource */
 
 
 /* helpers to define resources */
-- 
1.9.2

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Arnd Bergmann]

On Saturday 31 May 2014 01:36:40 Liviu Dudau wrote:
> We would like to be able to describe PCIe ECAM resources as
> IORESOURCE_MEM blocks while distinguish them from standard
> memory resources. Add an IORESOURCE_BIT entry for this case.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>

I still don't see any value in this at all. What is the advantage
of doing this opposed to just having a standardized 'reg' property
for a particular compatible string?

	Arnd

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Liviu Dudau]

On Sat, May 31, 2014 at 08:41:04PM +0200, Arnd Bergmann wrote:
> On Saturday 31 May 2014 01:36:40 Liviu Dudau wrote:
> > We would like to be able to describe PCIe ECAM resources as
> > IORESOURCE_MEM blocks while distinguish them from standard
> > memory resources. Add an IORESOURCE_BIT entry for this case.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> I still don't see any value in this at all. What is the advantage
> of doing this opposed to just having a standardized 'reg' property
> for a particular compatible string?

Oh, I agree. This patch was trying to provide an improved solution
to the original patch just in case someone considers this a worthwhile
exercise.

Best regards,
Liviu

> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Grant Likely]

On Sat, 31 May 2014 20:41:04 +0200, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 31 May 2014 01:36:40 Liviu Dudau wrote:
> > We would like to be able to describe PCIe ECAM resources as
> > IORESOURCE_MEM blocks while distinguish them from standard
> > memory resources. Add an IORESOURCE_BIT entry for this case.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> 
> I still don't see any value in this at all. What is the advantage
> of doing this opposed to just having a standardized 'reg' property
> for a particular compatible string?

I'm inclined to agree. It doesn't seem appropriate to put config space
in ranges, and the host controller binding is responsible for
identifying how config space is memory mapped.

g.

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Kumar Gala]

On Jun 2, 2014, at 10:09 AM, Grant Likely <grant.likely@linaro.org> wrote:

> On Sat, 31 May 2014 20:41:04 +0200, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Saturday 31 May 2014 01:36:40 Liviu Dudau wrote:
>>> We would like to be able to describe PCIe ECAM resources as
>>> IORESOURCE_MEM blocks while distinguish them from standard
>>> memory resources. Add an IORESOURCE_BIT entry for this case.
>>> 
>>> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>> 
>> I still don't see any value in this at all. What is the advantage
>> of doing this opposed to just having a standardized 'reg' property
>> for a particular compatible string?
> 
> I'm inclined to agree. It doesn't seem appropriate to put config space
> in ranges, and the host controller binding is responsible for
> identifying how config space is memory mapped.
> 
> g.

I don’t agree when it comes to ECAM, but we can drop this for now until someone really does that.

However, what do we do with the 2 cases that exist in upstream that are using ranges for cfg space?

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Grant Likely]

On Mon, 2 Jun 2014 10:40:30 -0500, Kumar Gala <galak@codeaurora.org> wrote:
> 
> On Jun 2, 2014, at 10:09 AM, Grant Likely <grant.likely@linaro.org> wrote:
> 
> > On Sat, 31 May 2014 20:41:04 +0200, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Saturday 31 May 2014 01:36:40 Liviu Dudau wrote:
> >>> We would like to be able to describe PCIe ECAM resources as
> >>> IORESOURCE_MEM blocks while distinguish them from standard
> >>> memory resources. Add an IORESOURCE_BIT entry for this case.
> >>> 
> >>> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >> 
> >> I still don't see any value in this at all. What is the advantage
> >> of doing this opposed to just having a standardized 'reg' property
> >> for a particular compatible string?
> > 
> > I'm inclined to agree. It doesn't seem appropriate to put config space
> > in ranges, and the host controller binding is responsible for
> > identifying how config space is memory mapped.
> > 
> > g.
> 
> I don’t agree when it comes to ECAM, but we can drop this for now
> until someone really does that.

Okay, humor me then. What would a ranges property look like for ECAM? Do
you have an example? I believe there would need to be a separate entry
for each and every PCI device on the bus to get the config spaces to be
contiguous.

> However, what do we do with the 2 cases that exist in upstream that
> are using ranges for cfg space?

Ignore them in the core code? Make the specific host controller handle
them I would think.

g.

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Kumar Gala]

On Jun 2, 2014, at 11:23 AM, Grant Likely <grant.likely@linaro.org> wrote:

> On Mon, 2 Jun 2014 10:40:30 -0500, Kumar Gala <galak@codeaurora.org> wrote:
>> 
>> On Jun 2, 2014, at 10:09 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> 
>>> On Sat, 31 May 2014 20:41:04 +0200, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Saturday 31 May 2014 01:36:40 Liviu Dudau wrote:
>>>>> We would like to be able to describe PCIe ECAM resources as
>>>>> IORESOURCE_MEM blocks while distinguish them from standard
>>>>> memory resources. Add an IORESOURCE_BIT entry for this case.
>>>>> 
>>>>> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
>>>> 
>>>> I still don't see any value in this at all. What is the advantage
>>>> of doing this opposed to just having a standardized 'reg' property
>>>> for a particular compatible string?
>>> 
>>> I'm inclined to agree. It doesn't seem appropriate to put config space
>>> in ranges, and the host controller binding is responsible for
>>> identifying how config space is memory mapped.
>>> 
>>> g.
>> 
>> I don’t agree when it comes to ECAM, but we can drop this for now
>> until someone really does that.
> 
> Okay, humor me then. What would a ranges property look like for ECAM? Do
> you have an example? I believe there would need to be a separate entry
> for each and every PCI device on the bus to get the config spaces to be
> contiguous.

The definition of ECAM is a 256M linear region with each 4k being a different bus/dev/func.

So the ranges would look something like:

   ranges = <0x00000000 0 0x00000000 0x0ff00000 0 0x10000000>   /* configuration space */

The reason I think allow an ECAM makes sense in ranges is because it allows for a direct IO read/write to CFG space (w/o any mapping) similar to what one would do for MEM space or IO.

> However, what do we do with the 2 cases that exist in upstream that
>> are using ranges for cfg space?
> 
> Ignore them in the core code? Make the specific host controller handle
> them I would think.

I just meant, should we ‘break’ their DTs and move them from using ranges to reg?

- k

> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Arnd Bergmann]

On Monday 02 June 2014 13:09:08 Kumar Gala wrote:
> > However, what do we do with the 2 cases that exist in upstream that
> >> are using ranges for cfg space?
> > 
> > Ignore them in the core code? Make the specific host controller handle
> > them I would think.
> 
> I just meant, should we ‘break’ their DTs and move them from using ranges to reg?

dw-pcie is used on a lot of systems, I think we should make the common
part of that driver always handle config space in a common way, and
move out the part that parses the ranges property into the individual
soc-specific glue drivers that want to keep optional backwards compatibility
with existing dtbs.

Which one is the other driver?

	Arnd

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Kumar Gala]

On Jun 2, 2014, at 2:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 02 June 2014 13:09:08 Kumar Gala wrote:
>>> However, what do we do with the 2 cases that exist in upstream that
>>>> are using ranges for cfg space?
>>> 
>>> Ignore them in the core code? Make the specific host controller handle
>>> them I would think.
>> 
>> I just meant, should we ‘break’ their DTs and move them from using ranges to reg?
> 
> dw-pcie is used on a lot of systems, I think we should make the common
> part of that driver always handle config space in a common way, and
> move out the part that parses the ranges property into the individual
> soc-specific glue drivers that want to keep optional backwards compatibility
> with existing dtbs.
> 
> Which one is the other driver?
> 
> 	Arnd

Its imx6 and exynos, havent looked to see if dw-pcie is handling the parsing or not for them.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Arnd Bergmann]

On Monday 02 June 2014 15:43:02 Kumar Gala wrote:
> Its imx6 and exynos, havent looked to see if dw-pcie is handling
> the parsing or not for them.

Ok, so they are both dw-pcie variants.

	Arnd 

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Grant Likely]

On Mon, 2 Jun 2014 13:09:08 -0500, Kumar Gala <galak@codeaurora.org> wrote:
> 
> On Jun 2, 2014, at 11:23 AM, Grant Likely <grant.likely@linaro.org> wrote:
> 
> > On Mon, 2 Jun 2014 10:40:30 -0500, Kumar Gala <galak@codeaurora.org> wrote:
> >> 
> >> On Jun 2, 2014, at 10:09 AM, Grant Likely <grant.likely@linaro.org> wrote:
> >> 
> >>> On Sat, 31 May 2014 20:41:04 +0200, Arnd Bergmann <arnd@arndb.de> wrote:
> >>>> On Saturday 31 May 2014 01:36:40 Liviu Dudau wrote:
> >>>>> We would like to be able to describe PCIe ECAM resources as
> >>>>> IORESOURCE_MEM blocks while distinguish them from standard
> >>>>> memory resources. Add an IORESOURCE_BIT entry for this case.
> >>>>> 
> >>>>> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >>>> 
> >>>> I still don't see any value in this at all. What is the advantage
> >>>> of doing this opposed to just having a standardized 'reg' property
> >>>> for a particular compatible string?
> >>> 
> >>> I'm inclined to agree. It doesn't seem appropriate to put config space
> >>> in ranges, and the host controller binding is responsible for
> >>> identifying how config space is memory mapped.
> >>> 
> >>> g.
> >> 
> >> I don’t agree when it comes to ECAM, but we can drop this for now
> >> until someone really does that.
> > 
> > Okay, humor me then. What would a ranges property look like for ECAM? Do
> > you have an example? I believe there would need to be a separate entry
> > for each and every PCI device on the bus to get the config spaces to be
> > contiguous.
> 
> The definition of ECAM is a 256M linear region with each 4k being a different bus/dev/func.
> 
> So the ranges would look something like:
> 
>    ranges = <0x00000000 0 0x00000000 0x0ff00000 0 0x10000000>   /* configuration space */
> 
> The reason I think allow an ECAM makes sense in ranges is because it allows for a direct IO read/write to CFG space (w/o any mapping) similar to what one would do for MEM space or IO.

I don't think that's right. PCI addresses are defined as follows:
phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
phys.low cell: llllllll llllllll llllllll llllllll

where 'ddddd' is the device number (0-31) and 'fff' is the function number (0-7)

Going up by one device number or even function number does not result in
contiguious address values:

device 0: 0x00000000 00000000 00000000
device 1: 0x00000800 00000000 00000000
device 2: 0x00001000 00000000 00000000
device 3: 0x00001800 00000000 00000000
...
device 30:0x0000f000 00000000 00000000
device 31:0x0000f800 00000000 00000000

a simple ranges doesn't work transparently because each of those config
ranges needs to be mapped to a 4k block. I think ranges would need to
look like this:

ranges = <0x00000000 0 0  0x0ff00000  0x1000>,
         <0x00000800 0 0  0x0ff01000  0x1000>,
	 <0x00001800 0 0  0x0ff02000  0x1000>,
	 ...
	 <0x0000f000 0 0  0x0ff1e000  0x1000>,
	 <0x0000f800 0 0  0x0ff1f000  0x1000>;

(I just hacked the above up; I make no claims to it's accuracy for
actual address values)

But I don't even thing the semantics work there because the address is
encoded in the phys.hi cell, not the phys.low cell. Incrementing by one
does not behaves as most bus addresses work. To actually work properly
we would have needed a way to define a stride of 64bits when
incrementing config space addresses in a ranges mapping.

> > However, what do we do with the 2 cases that exist in upstream that
> >> are using ranges for cfg space?
> > 
> > Ignore them in the core code? Make the specific host controller handle
> > them I would think.
> 
> I just meant, should we ‘break’ their DTs and move them from using ranges to reg?

If ranges is the only place to get the config space address for those
platforms, and if the support is in mainline, then we can't break it.
Fix the in-tree dts files, but leave backwards compatible code in the
host controller driver (perhaps with a warning sent to the kernel log).
The best we can do is discourage new boards from using the broken
pattern.

If it /won't/ affect deployed systems then it is okay to remove the
broken behaviour.

g.

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Arnd Bergmann]

On Tuesday 03 June 2014 09:44:59 Grant Likely wrote:
> > The reason I think allow an ECAM makes sense in ranges is because it allows for a direct IO read/write to CFG space (w/o any mapping) similar to what one would do for MEM space or IO.
> 
> I don't think that's right. PCI addresses are defined as follows:
> phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
> phys.low cell: llllllll llllllll llllllll llllllll
> 
> where 'ddddd' is the device number (0-31) and 'fff' is the function number (0-7)
> 
> Going up by one device number or even function number does not result in
> contiguious address values:
> 
> device 0: 0x00000000 00000000 00000000
> device 1: 0x00000800 00000000 00000000
> device 2: 0x00001000 00000000 00000000
> device 3: 0x00001800 00000000 00000000
> ...
> device 30:0x0000f000 00000000 00000000
> device 31:0x0000f800 00000000 00000000
> 
> a simple ranges doesn't work transparently because each of those config
> ranges needs to be mapped to a 4k block. I think ranges would need to
> look like this:
> 
> ranges = <0x00000000 0 0  0x0ff00000  0x1000>,
>          <0x00000800 0 0  0x0ff01000  0x1000>,
>          <0x00001800 0 0  0x0ff02000  0x1000>,
>          ...
>          <0x0000f000 0 0  0x0ff1e000  0x1000>,
>          <0x0000f800 0 0  0x0ff1f000  0x1000>;
> 
> (I just hacked the above up; I make no claims to it's accuracy for
> actual address values)
> 
> But I don't even thing the semantics work there because the address is
> encoded in the phys.hi cell, not the phys.low cell. Incrementing by one
> does not behaves as most bus addresses work. To actually work properly
> we would have needed a way to define a stride of 64bits when
> incrementing config space addresses in a ranges mapping.

Thanks for clearing that up. I always suspected it was roughly this
way, but never managed to think it through completely before getting
distracted by something else.

I wonder if the OF definition matches CAM though, if not ECAM, as
CAM is also limited to 256 byte config space per function.

	Arnd

Re: [PATCH 1/2] pci: Add IORESOURCE_BIT entry for PCIe ECAM resources.

[Grant Likely]

On Tue, 03 Jun 2014 11:21:10 +0200, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 03 June 2014 09:44:59 Grant Likely wrote:
> > > The reason I think allow an ECAM makes sense in ranges is because it allows for a direct IO read/write to CFG space (w/o any mapping) similar to what one would do for MEM space or IO.
> > 
> > I don't think that's right. PCI addresses are defined as follows:
> > phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> > phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
> > phys.low cell: llllllll llllllll llllllll llllllll
> > 
> > where 'ddddd' is the device number (0-31) and 'fff' is the function number (0-7)
> > 
> > Going up by one device number or even function number does not result in
> > contiguious address values:
> > 
> > device 0: 0x00000000 00000000 00000000
> > device 1: 0x00000800 00000000 00000000
> > device 2: 0x00001000 00000000 00000000
> > device 3: 0x00001800 00000000 00000000
> > ...
> > device 30:0x0000f000 00000000 00000000
> > device 31:0x0000f800 00000000 00000000
> > 
> > a simple ranges doesn't work transparently because each of those config
> > ranges needs to be mapped to a 4k block. I think ranges would need to
> > look like this:
> > 
> > ranges = <0x00000000 0 0  0x0ff00000  0x1000>,
> >          <0x00000800 0 0  0x0ff01000  0x1000>,
> >          <0x00001800 0 0  0x0ff02000  0x1000>,
> >          ...
> >          <0x0000f000 0 0  0x0ff1e000  0x1000>,
> >          <0x0000f800 0 0  0x0ff1f000  0x1000>;
> > 
> > (I just hacked the above up; I make no claims to it's accuracy for
> > actual address values)
> > 
> > But I don't even thing the semantics work there because the address is
> > encoded in the phys.hi cell, not the phys.low cell. Incrementing by one
> > does not behaves as most bus addresses work. To actually work properly
> > we would have needed a way to define a stride of 64bits when
> > incrementing config space addresses in a ranges mapping.
> 
> Thanks for clearing that up. I always suspected it was roughly this
> way, but never managed to think it through completely before getting
> distracted by something else.
> 
> I wonder if the OF definition matches CAM though, if not ECAM, as
> CAM is also limited to 256 byte config space per function.

It's the same problem for 256 byte entries. The address values don't
increment nicely and there is a big block of remapping needed.

g.

[PATCH 2/2] of: treat PCI config space as IORESOURCE_MEM type with special flags.

[Liviu Dudau]

If we have a PCI config space specified in something like a ranges
property we should treat it as memory type resource. Use the
IORESOURCE_ECFG bit to distinguish the config space from standard
IORESOURCE_MEM resources.

Signed-off-by: Kumar Gama <galak@codeaurora.org>
[Updated commit log and added IORESOURCE_ECFG bit]
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/of/address.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 2fcfbae..877e9e5 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -123,6 +123,9 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
 	u32 w = be32_to_cpup(addr);
 
 	switch((w >> 24) & 0x03) {
+	case 0x00: /* cfg space */
+		flags |= IORESOURCE_MEM | IORESOURCE_PCI_ECFG;
+		break;
 	case 0x01:
 		flags |= IORESOURCE_IO;
 		break;
-- 
1.9.2