RE: [PATCH v2] arm64: dts: renesas: Add IOMMU related properties into PCIe host nodes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Manivannan,

> From: Manivannan Sadhasivam, Sent: Thursday, May 4, 2023 4:40 PM
> 
> On Wed, May 03, 2023 at 02:38:49PM +0200, Geert Uytterhoeven wrote:
> > Hi Shimoda-san,
> >
> > CC linux-pci
> >
> > On Wed, Apr 26, 2023 at 10:28 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > > Add iommu-map and iommu-map-mask properties into PCIe host nodes.
> > > Note that iommu-map-mask should be zero because the IPMMU assigns
> > > one micro TLB ID only to the PCIe host.
> > >
> 
> What do you mean by "only to the PCIe host"? Are you referring to the host
> bridge in the SoC?

Oops. I intended to mean "one micro TLB ID only". The SoC doesn't have
any host bridge. So, I should have mentioned that "the IPMMU assigns one micro
TLB ID only, to the PCIe host.".

> > > Also change dma-ranges arguments for IOMMU. Notes that the dma-ranges
> > > can be used if IOMMU is disabled.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> >
> > Thanks for your patch!
> >
> > This is not really my area of expertise, but you can still find some
> > questions and comments below...
> >
> > > ---
> > >  Changes from v1:
> > >
<snip URL>
> > >  - Drop iommus property.
> > >  - Add iommu-map-mask property.
> > >  - Revise the commit description.
> > >
> > >  arch/arm64/boot/dts/renesas/r8a77951.dtsi | 12 ++++++++----
> > >  arch/arm64/boot/dts/renesas/r8a77960.dtsi | 12 ++++++++----
> > >  arch/arm64/boot/dts/renesas/r8a77961.dtsi | 12 ++++++++----
> > >  arch/arm64/boot/dts/renesas/r8a77965.dtsi | 12 ++++++++----
> > >  arch/arm64/boot/dts/renesas/r8a77990.dtsi |  6 ++++--
> > >  5 files changed, 36 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a77951.dtsi b/arch/arm64/boot/dts/renesas/r8a77951.dtsi
> > > index 10b91e9733bf..2adec8b6c93f 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a77951.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r8a77951.dtsi
> > > @@ -2778,8 +2778,8 @@ pciec0: pcie@fe000000 {
> > >                                  <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 0x40000000>;
> > > +                       /* Map all possible DDR/IOMMU as inbound ranges */
> > > +                       dma-ranges = <0x42000000 0 0x00000000 0 0x00000000 1 0x00000000>;
> >
> > So this is limited to the first 4 GiB of DDR (DDR0), i.e. to 32-bit
> > address space? Shouldn't this include DDR1/2/3?
> >
> > >                         interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
> > >                                 <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>,
> > >                                 <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
> > > @@ -2790,6 +2790,8 @@ pciec0: pcie@fe000000 {
> > >                         clock-names = "pcie", "pcie_bus";
> > >                         power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> > >                         resets = <&cpg 319>;
> > > +                       iommu-map = <0 &ipmmu_hc 0 0x10000>;
> >
> > Reading
> >
<snip URL>
> > the above means you map 65536 RIDs n in the range 0..65535
> > to  <&ipmmu_hc n>, while only micro-TLBs 0 and 1 are assigned to PCIe?
> >
> > Hence I think this should be:
> >
> >     iommu-map = <0 &ipmmu_hc 0 1>;
> >
> 
> If there are no PCI-PCI bridges in the SoC, then the devices connected to the
> host bridge should share the same bus number i.e., 00:00.0 to 00.1f.8. In that
> case, you should have the iommu-map as per Geert's suggestion. Paired with
> iommu-map-mask of 0, this implies that all the devices and functions of bus 0
> will share the same RID.
> 
> This holds true for other instance as well.

Thank you for the comments. I'll fix this patch.

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> > > +                       iommu-map-mask = <0>;
> > >                         status = "disabled";
> > >                 };
> > >
> > > @@ -2805,8 +2807,8 @@ pciec1: pcie@ee800000 {
> > >                                  <0x02000000 0 0xeea00000 0 0xeea00000 0 0x00200000>,
> > >                                  <0x02000000 0 0xc0000000 0 0xc0000000 0 0x08000000>,
> > >                                  <0x42000000 0 0xc8000000 0 0xc8000000 0 0x08000000>;
> > > -                       /* Map all possible DDR as inbound ranges */
> > > -                       dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x40000000>;
> > > +                       /* Map all possible DDR/IOMMU as inbound ranges */
> > > +                       dma-ranges = <0x42000000 0 0x00000000 0 0x00000000 1 0x00000000>;
> >
> > Likewise.
> >
> > >                         interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
> > >                                 <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>,
> > >                                 <GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>;
> > > @@ -2817,6 +2819,8 @@ pciec1: pcie@ee800000 {
> > >                         clock-names = "pcie", "pcie_bus";
> > >                         power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> > >                         resets = <&cpg 318>;
> > > +                       iommu-map = <0 &ipmmu_hc 1 0x10000>;
> >
> > Likewise, the above means you map 65536 RIDs n in the range 0..65535
> > to  <&ipmmu_hc (1 + n)>?
> >
> > Hence I think this should be:
> >
> >     iommu-map = <0 &ipmmu_hc 1 1>;
> >
> > > +                       iommu-map-mask = <0>;
> > >                         status = "disabled";
> > >                 };
> >
> > Same comment for all other changes.
> >
> > In addition, we need similar changes to r8a774{a1,b1,c0,e1}.dtsi,
> > and slightly different changes (using ipmmu_vi0 uTLB 5) to r8a77980.dtsi.
> >
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                 -- Linus Torvalds
> 
> --
> மணிவண்ணன் சதாசிவம்




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux