| Message ID | 1395766604-30926-7-git-send-email-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 25 March 2014 16:56:41 Phil Edworthy wrote: > + /* Map all possible DDR as inbound ranges */ > + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x80000000 > + 0x43000000 1 0x80000000 1 0x80000000 0 0x80000000>; Typo: 0x43000000 should be 0x42000000 I guess. Since you control the mapping, I wonder if you could also do this as dma-ranges = <0x42000000 0 0x00000000 0 0x40000000 0 0x80000000 0x43000000 0 0x80000000 1 0x80000000 0 0x80000000>; i.e. map all the RAM into PCI bus addresses below the 32-bit boundary. This would be really nice from the perspective that now all PCI devices could access all of RAM without using an IOMMU or the relying on 64-bit master capability. The downside of this is that you'd probably need a custom dma_map_ops wrapper. Arnd
On Tue, Mar 25, 2014 at 04:56:41PM +0000, Phil Edworthy wrote: > This patch adds the device tree nodes for R8A7790 > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> Please split this patch into two patches. 1. A patch that updates arch/arm/boot/dts/r8a7790.dtsi I suggest calling that patch "ARM: shmobile: r8a7791: Add PCI device nodes" 2. A patch that updates arch/arm/boot/dts/r8a7790-lager.dts I suggest calling that patch "ARM: shmobile: lager: Add PCI device nodes" > --- > v5: > - Renesas SoCs compatible string has peripheral before device name > - Add PCIe bus clock reference > - Add additional interrupt bindings > - Use dma-ranges property to specify inbound memory regions > --- > arch/arm/boot/dts/r8a7790-lager.dts | 10 ++++++++++ > arch/arm/boot/dts/r8a7790.dtsi | 24 ++++++++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts > index a55c5f8..bbbcb63 100644 > --- a/arch/arm/boot/dts/r8a7790-lager.dts > +++ b/arch/arm/boot/dts/r8a7790-lager.dts > @@ -139,6 +139,16 @@ > states = <3300000 1 > 1800000 0>; > }; > + > + clocks { > + /* External PCIe bus clock - not used */ > + pcie_bus_clk: pcie_bus_clk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <0>; > + clock-output-names = "pcie_bus"; > + }; > + }; > }; > > &extal_clk { > diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi > index df9ec61..dbcea57 100644 > --- a/arch/arm/boot/dts/r8a7790.dtsi > +++ b/arch/arm/boot/dts/r8a7790.dtsi > @@ -821,4 +821,28 @@ > #size-cells = <0>; > status = "disabled"; > }; > + > + pcie: pcie@fe000000 { > + compatible = "renesas,pcie-r8a7790"; > + reg = <0 0xfe000000 0 0x80000>; > + #address-cells = <3>; > + #size-cells = <2>; > + device_type = "pci"; > + ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000 > + 0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000 > + 0x02000000 0 0x30000000 0 0x30000000 0 0x08000000 > + 0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>; > + /* Map all possible DDR as inbound ranges */ > + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x80000000 > + 0x43000000 1 0x80000000 1 0x80000000 0 0x80000000>; > + interrupts = <0 116 IRQ_TYPE_LEVEL_HIGH > + 0 117 IRQ_TYPE_LEVEL_HIGH > + 0 118 IRQ_TYPE_LEVEL_HIGH>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 0>; > + interrupt-map = <0 0 0 0 &gic 0 116 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp3_clks R8A7790_CLK_PCIE>, <&pcie_bus_clk>; > + clock-names = "pcie", "pcie_bus"; > + status = "disabled"; > + }; > }; > -- > 1.9.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sh" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi Simon, On: 25/03/2014 21:03, Simon wrote: > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes for R8A7790 > > On Tue, Mar 25, 2014 at 04:56:41PM +0000, Phil Edworthy wrote: > > This patch adds the device tree nodes for R8A7790 > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > Please split this patch into two patches. > > 1. A patch that updates arch/arm/boot/dts/r8a7790.dtsi > > I suggest calling that patch > "ARM: shmobile: r8a7791: Add PCI device nodes" > > 2. A patch that updates arch/arm/boot/dts/r8a7790-lager.dts > > I suggest calling that patch > "ARM: shmobile: lager: Add PCI device nodes" > For this, and your other comments, will do! Thanks Phil
Hi Arnd, On: 25/03/2014 18:42, Arnd wrote: > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes for R8A7790 > > On Tuesday 25 March 2014 16:56:41 Phil Edworthy wrote: > > + /* Map all possible DDR as inbound ranges */ > > + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x80000000 > > + 0x43000000 1 0x80000000 1 0x80000000 0 0x80000000>; > > Typo: 0x43000000 should be 0x42000000 I guess. I used 0x43000000 as this is a 64-bit type. The OF PCI range code currently treats both 32 and 64-bit types the same way, but I thought it would be good to set this in case we ever need to use it. > Since you control the mapping, I wonder if you could also do this as > > dma-ranges = <0x42000000 0 0x00000000 0 0x40000000 0 0x80000000 > 0x43000000 0 0x80000000 1 0x80000000 0 0x80000000>; > > i.e. map all the RAM into PCI bus addresses below the 32-bit boundary. > This would be really nice from the perspective that now all PCI > devices could access all of RAM without using an IOMMU or the > relying on 64-bit master capability. > > The downside of this is that you'd probably need a custom dma_map_ops > wrapper. Since the OF PCi range code treats both 32 and 64-bit types the same way, my PCIe driver only creates 64-bit mappings. In addition, the PCIe controller has to use a 64-bit mapping for anything over 2GiB. Based on this, I think it's sensible to leave the mappings as 1-to-1. Thanks Phil
On Wednesday 26 March 2014 09:55:04 Phil.Edworthy@renesas.com wrote: > Hi Arnd, > > On: 25/03/2014 18:42, Arnd wrote: > > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes > for R8A7790 > > > > On Tuesday 25 March 2014 16:56:41 Phil Edworthy wrote: > > > + /* Map all possible DDR as inbound ranges */ > > > + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 > 0x80000000 > > > + 0x43000000 1 0x80000000 1 0x80000000 0 > 0x80000000>; > > > > Typo: 0x43000000 should be 0x42000000 I guess. > I used 0x43000000 as this is a 64-bit type. The OF PCI range code > currently treats both 32 and 64-bit types the same way, but I thought it > would be good to set this in case we ever need to use it. Ah, I forgot about the space identifier. It looks correct then, but it seems a little strange to use a 32-bit identifier in one case and a 64-bit one in the other. > > Since you control the mapping, I wonder if you could also do this as > > > > dma-ranges = <0x42000000 0 0x00000000 0 0x40000000 0 > 0x80000000 > > 0x43000000 0 0x80000000 1 0x80000000 0 > 0x80000000>; > > > > i.e. map all the RAM into PCI bus addresses below the 32-bit boundary. > > This would be really nice from the perspective that now all PCI > > devices could access all of RAM without using an IOMMU or the > > relying on 64-bit master capability. > > > > The downside of this is that you'd probably need a custom dma_map_ops > > wrapper. > Since the OF PCi range code treats both 32 and 64-bit types the same way, > my PCIe driver only creates 64-bit mappings. In addition, the PCIe > controller has to use a 64-bit mapping for anything over 2GiB. Based on > this, I think it's sensible to leave the mappings as 1-to-1. I'm not following, sorry. What is the hardware requirement in the controller? Arnd
Hi Arnd, On: 26/03/2014 10:34, Arnd wrote: > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes for R8A7790 > > On Wednesday 26 March 2014 09:55:04 Phil.Edworthy@renesas.com wrote: > > Hi Arnd, > > > > On: 25/03/2014 18:42, Arnd wrote: > > > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes > > for R8A7790 > > > > > > On Tuesday 25 March 2014 16:56:41 Phil Edworthy wrote: > > > > + /* Map all possible DDR as inbound ranges */ > > > > + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 > > 0x80000000 > > > > + 0x43000000 1 0x80000000 1 0x80000000 0 > > 0x80000000>; > > > > > > Typo: 0x43000000 should be 0x42000000 I guess. > > I used 0x43000000 as this is a 64-bit type. The OF PCI range code > > currently treats both 32 and 64-bit types the same way, but I thought it > > would be good to set this in case we ever need to use it. > > Ah, I forgot about the space identifier. It looks correct then, but > it seems a little strange to use a 32-bit identifier in one case > and a 64-bit one in the other. If the OF PCI range code allowed the PCIe host driver to determine if it's a 32-bit mapping, we could use that and get a small performance improvement with PCIe throughput. > > > Since you control the mapping, I wonder if you could also do this as > > > > > > dma-ranges = <0x42000000 0 0x00000000 0 0x40000000 0 > > 0x80000000 > > > 0x43000000 0 0x80000000 1 0x80000000 0 > > 0x80000000>; > > > > > > i.e. map all the RAM into PCI bus addresses below the 32-bit boundary. > > > This would be really nice from the perspective that now all PCI > > > devices could access all of RAM without using an IOMMU or the > > > relying on 64-bit master capability. > > > > > > The downside of this is that you'd probably need a custom dma_map_ops > > > wrapper. > > Since the OF PCi range code treats both 32 and 64-bit types the same way, > > my PCIe driver only creates 64-bit mappings. In addition, the PCIe > > controller has to use a 64-bit mapping for anything over 2GiB. Based on > > this, I think it's sensible to leave the mappings as 1-to-1. > > I'm not following, sorry. What is the hardware requirement in the > controller? With this controller, you can only specify maps whose size are a power of two, and the size must be less than or equal to the cpu address alignment. Further, when the size is 4GiB, you have to use a 64-bit mapping. Thinking about it, the 4GiB case is not relevant to our discussion about 32-bit vs 64-bit mappings. Still, my comment about the OF PCI range code treating both 32 and 64-bit types the same way means that PCIe host driver has to assume it's a 64-bit mapping. Thanks Phil
On Wednesday 26 March 2014 11:01:46 Phil.Edworthy@renesas.com wrote: > On: 26/03/2014 10:34, Arnd wrote: > > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes > for R8A7790 > > > > On Wednesday 26 March 2014 09:55:04 Phil.Edworthy@renesas.com wrote: > > > Hi Arnd, > > > > > > On: 25/03/2014 18:42, Arnd wrote: > > > > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree > nodes > > > for R8A7790 > > > > > > > > On Tuesday 25 March 2014 16:56:41 Phil Edworthy wrote: > > > > > + /* Map all possible DDR as inbound ranges */ > > > > > + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 > 0 > > > 0x80000000 > > > > > + 0x43000000 1 0x80000000 1 0x80000000 > 0 > > > 0x80000000>; > > > > > > > > Typo: 0x43000000 should be 0x42000000 I guess. > > > I used 0x43000000 as this is a 64-bit type. The OF PCI range code > > > currently treats both 32 and 64-bit types the same way, but I thought > it > > > would be good to set this in case we ever need to use it. > > > > Ah, I forgot about the space identifier. It looks correct then, but > > it seems a little strange to use a 32-bit identifier in one case > > and a 64-bit one in the other. > > If the OF PCI range code allowed the PCIe host driver to determine if it's > a 32-bit mapping, we could use that and get a small performance > improvement with PCIe throughput. I don't think it's supposed to care. Soem of the upper bits of the ranges only really make sense of PCI device registers, not for the top-level ranges property. The driver can however still look at the address itself to get that information. > > > Since the OF PCi range code treats both 32 and 64-bit types the same > way, > > > my PCIe driver only creates 64-bit mappings. In addition, the PCIe > > > controller has to use a 64-bit mapping for anything over 2GiB. Based > on > > > this, I think it's sensible to leave the mappings as 1-to-1. > > > > I'm not following, sorry. What is the hardware requirement in the > > controller? > With this controller, you can only specify maps whose size are a power of > two, and the size must be less than or equal to the cpu address alignment. > Further, when the size is 4GiB, you have to use a 64-bit mapping. Thinking > about it, the 4GiB case is not relevant to our discussion about 32-bit vs > 64-bit mappings. But the ranges you specified in the property don't actually fit in those constraints: you have a range with size 0x8000000 and start 0x40000000, which you say can't be programmed into the hardware. > Still, my comment about the OF PCI range code treating both 32 and 64-bit > types the same way means that PCIe host driver has to assume it's a 64-bit > mapping. I was thinking more of PCI devices than the host itself. If the host driver can verify that all mappings are in the first 4GB and cover all of RAM, we won't have to use an swiotlb for devices that don't support 64-bit DMA, which is a very significant performance difference. Arnd
Hi Arnd, On: 26/03/2014 11:14, Arnd wrote: > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes for R8A7790 > > On Wednesday 26 March 2014 11:01:46 Phil.Edworthy@renesas.com wrote: > > On: 26/03/2014 10:34, Arnd wrote: > > > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes > > for R8A7790 > > > > > > On Wednesday 26 March 2014 09:55:04 Phil.Edworthy@renesas.com wrote: > > > > Hi Arnd, > > > > > > > > On: 25/03/2014 18:42, Arnd wrote: > > > > > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree > > nodes > > > > for R8A7790 > > > > > > > > > > On Tuesday 25 March 2014 16:56:41 Phil Edworthy wrote: > > > > > > + /* Map all possible DDR as inbound ranges */ > > > > > > + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 > > 0 > > > > 0x80000000 > > > > > > + 0x43000000 1 0x80000000 1 0x80000000 > > 0 > > > > 0x80000000>; > > > > > > > > > > Typo: 0x43000000 should be 0x42000000 I guess. > > > > I used 0x43000000 as this is a 64-bit type. The OF PCI range code > > > > currently treats both 32 and 64-bit types the same way, but I thought > > it > > > > would be good to set this in case we ever need to use it. > > > > > > Ah, I forgot about the space identifier. It looks correct then, but > > > it seems a little strange to use a 32-bit identifier in one case > > > and a 64-bit one in the other. > > > > If the OF PCI range code allowed the PCIe host driver to determine if it's > > a 32-bit mapping, we could use that and get a small performance > > improvement with PCIe throughput. > > I don't think it's supposed to care. Soem of the upper bits of the ranges > only really make sense of PCI device registers, not for the top-level > ranges property. The driver can however still look at the address itself > to get that information. Ah, yes that is a possibility. > > > > Since the OF PCi range code treats both 32 and 64-bit types the same > > way, > > > > my PCIe driver only creates 64-bit mappings. In addition, the PCIe > > > > controller has to use a 64-bit mapping for anything over 2GiB. Based > > on > > > > this, I think it's sensible to leave the mappings as 1-to-1. > > > > > > I'm not following, sorry. What is the hardware requirement in the > > > controller? > > With this controller, you can only specify maps whose size are a power of > > two, and the size must be less than or equal to the cpu address alignment. > > Further, when the size is 4GiB, you have to use a 64-bit mapping. Thinking > > about it, the 4GiB case is not relevant to our discussion about 32-bit vs > > 64-bit mappings. > > But the ranges you specified in the property don't actually fit in those > constraints: you have a range with size 0x8000000 and start 0x40000000, > which you say can't be programmed into the hardware. Actually, the driver checks the dma-ranges against these constraints, and if necessary will create multiple mappings to fulfil the requested dma-ranges. > > Still, my comment about the OF PCI range code treating both 32 and 64-bit > > types the same way means that PCIe host driver has to assume it's a 64-bit > > mapping. > > I was thinking more of PCI devices than the host itself. If the host > driver can verify that all mappings are in the first 4GB and cover all of > RAM, we won't have to use an swiotlb for devices that don't support 64-bit > DMA, which is a very significant performance difference. Ok, I think I understand. However, all the other PCI host drivers just do 1-to-1 mapping between PCI and CPU addresses, right? Whilst it might be nice be able to support mapping CPU addresses > 4GiB to PCI addresses under 4GiB, can that be something to consider later on? Thanks Phil
On Wednesday 26 March 2014 11:34:41 Phil.Edworthy@renesas.com wrote: > > But the ranges you specified in the property don't actually fit in those > > constraints: you have a range with size 0x8000000 and start 0x40000000, > > which you say can't be programmed into the hardware. > > Actually, the driver checks the dma-ranges against these constraints, and > if necessary will create multiple mappings to fulfil the requested > dma-ranges. Ok, I didn't notice. My initial suggestion was to not put that logic into the driver but instead specify in the host bridge binding that each entry in the dma-ranges property has to meet the hardware constraints. As long as you don't have too much complexity to detect this case, I'm fine with it either way. > > > Still, my comment about the OF PCI range code treating both 32 and > 64-bit > > > types the same way means that PCIe host driver has to assume it's a > 64-bit > > > mapping. > > > > I was thinking more of PCI devices than the host itself. If the host > > driver can verify that all mappings are in the first 4GB and cover all > of > > RAM, we won't have to use an swiotlb for devices that don't support > 64-bit > > DMA, which is a very significant performance difference. > Ok, I think I understand. However, all the other PCI host drivers just do > 1-to-1 mapping between PCI and CPU addresses, right? Whilst it might be > nice be able to support mapping CPU addresses > 4GiB to PCI addresses > under 4GiB, can that be something to consider later on? Yes, fair enough. The current version is much simpler, so that's ok. Just keep it in mind if you run into performance problems. Also, note that we don't actually support swiotlb on arm32 yet, so your current code is broken for an PCI DMA master that is not 64-bit capable. We need swiotlb on arm32 anyway, and that will fix this problem, but adding the hack I described would also fix it. Arnd
Hi Arnd, On: 26/03/2014 11:53, Arnd wrote: > Subject: Re: [PATCH v5 6/9] ARM: shmobile: Add PCIe device tree nodes for R8A7790 > > On Wednesday 26 March 2014 11:34:41 Phil.Edworthy@renesas.com wrote: > > > But the ranges you specified in the property don't actually fit in those > > > constraints: you have a range with size 0x8000000 and start 0x40000000, > > > which you say can't be programmed into the hardware. > > > > Actually, the driver checks the dma-ranges against these constraints, and > > if necessary will create multiple mappings to fulfil the requested > > dma-ranges. > > Ok, I didn't notice. My initial suggestion was to not put that logic > into the driver but instead specify in the host bridge binding that each > entry in the dma-ranges property has to meet the hardware constraints. > > As long as you don't have too much complexity to detect this case, I'm > fine with it either way. Ok, good! > > > > Still, my comment about the OF PCI range code treating both 32 and > > 64-bit > > > > types the same way means that PCIe host driver has to assume it's a > > 64-bit > > > > mapping. > > > > > > I was thinking more of PCI devices than the host itself. If the host > > > driver can verify that all mappings are in the first 4GB and cover all > > of > > > RAM, we won't have to use an swiotlb for devices that don't support > > 64-bit > > > DMA, which is a very significant performance difference. > > Ok, I think I understand. However, all the other PCI host drivers just do > > 1-to-1 mapping between PCI and CPU addresses, right? Whilst it might be > > nice be able to support mapping CPU addresses > 4GiB to PCI addresses > > under 4GiB, can that be something to consider later on? > > Yes, fair enough. The current version is much simpler, so that's ok. > Just keep it in mind if you run into performance problems. Also, note > that we don't actually support swiotlb on arm32 yet, so your current > code is broken for an PCI DMA master that is not 64-bit capable. > We need swiotlb on arm32 anyway, and that will fix this problem, but > adding the hack I described would also fix it. Ok, thanks for all your comments! Phil
diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts index a55c5f8..bbbcb63 100644 --- a/arch/arm/boot/dts/r8a7790-lager.dts +++ b/arch/arm/boot/dts/r8a7790-lager.dts @@ -139,6 +139,16 @@ states = <3300000 1 1800000 0>; }; + + clocks { + /* External PCIe bus clock - not used */ + pcie_bus_clk: pcie_bus_clk { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <0>; + clock-output-names = "pcie_bus"; + }; + }; }; &extal_clk { diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi index df9ec61..dbcea57 100644 --- a/arch/arm/boot/dts/r8a7790.dtsi +++ b/arch/arm/boot/dts/r8a7790.dtsi @@ -821,4 +821,28 @@ #size-cells = <0>; status = "disabled"; }; + + pcie: pcie@fe000000 { + compatible = "renesas,pcie-r8a7790"; + reg = <0 0xfe000000 0 0x80000>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000 + 0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000 + 0x02000000 0 0x30000000 0 0x30000000 0 0x08000000 + 0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>; + /* Map all possible DDR as inbound ranges */ + dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x80000000 + 0x43000000 1 0x80000000 1 0x80000000 0 0x80000000>; + interrupts = <0 116 IRQ_TYPE_LEVEL_HIGH + 0 117 IRQ_TYPE_LEVEL_HIGH + 0 118 IRQ_TYPE_LEVEL_HIGH>; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0>; + interrupt-map = <0 0 0 0 &gic 0 116 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp3_clks R8A7790_CLK_PCIE>, <&pcie_bus_clk>; + clock-names = "pcie", "pcie_bus"; + status = "disabled"; + }; };
This patch adds the device tree nodes for R8A7790 Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- v5: - Renesas SoCs compatible string has peripheral before device name - Add PCIe bus clock reference - Add additional interrupt bindings - Use dma-ranges property to specify inbound memory regions --- arch/arm/boot/dts/r8a7790-lager.dts | 10 ++++++++++ arch/arm/boot/dts/r8a7790.dtsi | 24 ++++++++++++++++++++++++ 2 files changed, 34 insertions(+)