| Message ID | 1401496601-31983-2-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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.
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
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.
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
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
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
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
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.
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
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.
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 */
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(+)