| Message ID | 20220906134801.4079497-4-nipun.gupta@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v3,1/7] dt-bindings: bus: add CDX bus device tree bindings | expand |
On Tue, Sep 6, 2022 at 6:48 AM Nipun Gupta <nipun.gupta@amd.com> wrote: > > With new CDX bus supported for AMD FPGA devices on ARM > platform, the bus requires registration for the SMMU v3 > driver. > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index d32b02336411..8ec9f2baf12d 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -29,6 +29,7 @@ > #include <linux/platform_device.h> > > #include <linux/amba/bus.h> > +#include <linux/cdx/cdx_bus.h> > > #include "arm-smmu-v3.h" > #include "../../iommu-sva-lib.h" > @@ -3690,16 +3691,27 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops) > if (err) > goto err_reset_pci_ops; > } > +#endif > +#ifdef CONFIG_CDX_BUS > + if (cdx_bus_type.iommu_ops != ops) { > + err = bus_set_iommu(&cdx_bus_type, ops); > + if (err) > + goto err_reset_amba_ops; > + } I'm not an expert on IOMMUs, so apologies if the question is stupid. Why does the CDX bus need special treatment here (like PCI) when there are so many other busses (eg: I2C, SPI, etc) that don't need any changes here? -Saravana > #endif > if (platform_bus_type.iommu_ops != ops) { > err = bus_set_iommu(&platform_bus_type, ops); > if (err) > - goto err_reset_amba_ops; > + goto err_reset_cdx_ops; > } > > return 0; > > -err_reset_amba_ops: > +err_reset_cdx_ops: > +#ifdef CONFIG_CDX_BUS > + bus_set_iommu(&cdx_bus_type, NULL); > +#endif > +err_reset_amba_ops: __maybe_unused; > #ifdef CONFIG_ARM_AMBA > bus_set_iommu(&amba_bustype, NULL); > #endif > -- > 2.25.1 >
[AMD Official Use Only - General] > -----Original Message----- > From: Saravana Kannan <saravanak@google.com> > Sent: Wednesday, September 7, 2022 5:41 AM > To: Gupta, Nipun <Nipun.Gupta@amd.com> > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > gregkh@linuxfoundation.org; rafael@kernel.org; eric.auger@redhat.com; > alex.williamson@redhat.com; cohuck@redhat.com; Gupta, Puneet (DCG-ENG) > <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; > mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; > jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; mani@kernel.org; > yishaih@nvidia.com; jgg@ziepe.ca; jgg@nvidia.com; robin.murphy@arm.com; > will@kernel.org; joro@8bytes.org; masahiroy@kernel.org; > ndesaulniers@google.com; linux-arm-kernel@lists.infradead.org; linux- > kbuild@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; kvm@vger.kernel.org; okaya@kernel.org; Anand, > Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil > <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; > Radovanovic, Aleksandar <aleksandar.radovanovic@amd.com>; git (AMD-Xilinx) > <git@amd.com> > Subject: Re: [RFC PATCH v3 3/7] iommu/arm-smmu-v3: support ops registration > for CDX bus > > [CAUTION: External Email] > > On Tue, Sep 6, 2022 at 6:48 AM Nipun Gupta <nipun.gupta@amd.com> wrote: > > > > With new CDX bus supported for AMD FPGA devices on ARM > > platform, the bus requires registration for the SMMU v3 > > driver. > > > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > index d32b02336411..8ec9f2baf12d 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -29,6 +29,7 @@ > > #include <linux/platform_device.h> > > > > #include <linux/amba/bus.h> > > +#include <linux/cdx/cdx_bus.h> > > > > #include "arm-smmu-v3.h" > > #include "../../iommu-sva-lib.h" > > @@ -3690,16 +3691,27 @@ static int arm_smmu_set_bus_ops(struct > iommu_ops *ops) > > if (err) > > goto err_reset_pci_ops; > > } > > +#endif > > +#ifdef CONFIG_CDX_BUS > > + if (cdx_bus_type.iommu_ops != ops) { > > + err = bus_set_iommu(&cdx_bus_type, ops); > > + if (err) > > + goto err_reset_amba_ops; > > + } > > I'm not an expert on IOMMUs, so apologies if the question is stupid. > > Why does the CDX bus need special treatment here (like PCI) when there > are so many other busses (eg: I2C, SPI, etc) that don't need any > changes here? AFAIU, the devices on I2C/SPI does not use SMMU. Apart from PCI/AMBA, FSL-MC is another similar bus (on SMMUv2) which uses SMMU ops. The devices here are behind SMMU. Robin can kindly correct or add more here from SMMU perspective. Thanks, Nipun > > -Saravana > > > #endif > > if (platform_bus_type.iommu_ops != ops) { > > err = bus_set_iommu(&platform_bus_type, ops); > > if (err) > > - goto err_reset_amba_ops; > > + goto err_reset_cdx_ops; > > } > > > > return 0; > > > > -err_reset_amba_ops: > > +err_reset_cdx_ops: > > +#ifdef CONFIG_CDX_BUS > > + bus_set_iommu(&cdx_bus_type, NULL); > > +#endif > > +err_reset_amba_ops: __maybe_unused; > > #ifdef CONFIG_ARM_AMBA > > bus_set_iommu(&amba_bustype, NULL); > > #endif > > -- > > 2.25.1 > >
On 2022-09-07 04:17, Gupta, Nipun wrote: > [AMD Official Use Only - General] > > > >> -----Original Message----- >> From: Saravana Kannan <saravanak@google.com> >> Sent: Wednesday, September 7, 2022 5:41 AM >> To: Gupta, Nipun <Nipun.Gupta@amd.com> >> Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; >> gregkh@linuxfoundation.org; rafael@kernel.org; eric.auger@redhat.com; >> alex.williamson@redhat.com; cohuck@redhat.com; Gupta, Puneet (DCG-ENG) >> <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; >> mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; >> jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; mani@kernel.org; >> yishaih@nvidia.com; jgg@ziepe.ca; jgg@nvidia.com; robin.murphy@arm.com; >> will@kernel.org; joro@8bytes.org; masahiroy@kernel.org; >> ndesaulniers@google.com; linux-arm-kernel@lists.infradead.org; linux- >> kbuild@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org; kvm@vger.kernel.org; okaya@kernel.org; Anand, >> Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil >> <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; >> Radovanovic, Aleksandar <aleksandar.radovanovic@amd.com>; git (AMD-Xilinx) >> <git@amd.com> >> Subject: Re: [RFC PATCH v3 3/7] iommu/arm-smmu-v3: support ops registration >> for CDX bus >> >> [CAUTION: External Email] >> >> On Tue, Sep 6, 2022 at 6:48 AM Nipun Gupta <nipun.gupta@amd.com> wrote: >>> >>> With new CDX bus supported for AMD FPGA devices on ARM >>> platform, the bus requires registration for the SMMU v3 >>> driver. >>> >>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> >>> --- >>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++-- >>> 1 file changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>> index d32b02336411..8ec9f2baf12d 100644 >>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>> @@ -29,6 +29,7 @@ >>> #include <linux/platform_device.h> >>> >>> #include <linux/amba/bus.h> >>> +#include <linux/cdx/cdx_bus.h> >>> >>> #include "arm-smmu-v3.h" >>> #include "../../iommu-sva-lib.h" >>> @@ -3690,16 +3691,27 @@ static int arm_smmu_set_bus_ops(struct >> iommu_ops *ops) >>> if (err) >>> goto err_reset_pci_ops; >>> } >>> +#endif >>> +#ifdef CONFIG_CDX_BUS >>> + if (cdx_bus_type.iommu_ops != ops) { >>> + err = bus_set_iommu(&cdx_bus_type, ops); >>> + if (err) >>> + goto err_reset_amba_ops; >>> + } >> >> I'm not an expert on IOMMUs, so apologies if the question is stupid. >> >> Why does the CDX bus need special treatment here (like PCI) when there >> are so many other busses (eg: I2C, SPI, etc) that don't need any >> changes here? > > AFAIU, the devices on I2C/SPI does not use SMMU. Apart from PCI/AMBA, > FSL-MC is another similar bus (on SMMUv2) which uses SMMU ops. > > The devices here are behind SMMU. Robin can kindly correct or add > more here from SMMU perspective. Indeed, there is no need to describe and handle how DMA may or may not be translated for I2C/SPI/USB/etc. because they are not DMA-capable buses (in those cases the relevant bus *controller* often does DMA, but it does that for itself as the platform/PCI/etc. device it is). Note that I have a series pending[1] that will make this patch a whole lot simpler. Thanks, Robin. [1] https://lore.kernel.org/linux-iommu/cover.1660572783.git.robin.murphy@arm.com/T/#t > > Thanks, > Nipun > >> >> -Saravana >> >>> #endif >>> if (platform_bus_type.iommu_ops != ops) { >>> err = bus_set_iommu(&platform_bus_type, ops); >>> if (err) >>> - goto err_reset_amba_ops; >>> + goto err_reset_cdx_ops; >>> } >>> >>> return 0; >>> >>> -err_reset_amba_ops: >>> +err_reset_cdx_ops: >>> +#ifdef CONFIG_CDX_BUS >>> + bus_set_iommu(&cdx_bus_type, NULL); >>> +#endif >>> +err_reset_amba_ops: __maybe_unused; >>> #ifdef CONFIG_ARM_AMBA >>> bus_set_iommu(&amba_bustype, NULL); >>> #endif >>> -- >>> 2.25.1 >>>
On Wed, Sep 7, 2022 at 1:27 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2022-09-07 04:17, Gupta, Nipun wrote: > > [AMD Official Use Only - General] > > > > > > > >> -----Original Message----- > >> From: Saravana Kannan <saravanak@google.com> > >> Sent: Wednesday, September 7, 2022 5:41 AM > >> To: Gupta, Nipun <Nipun.Gupta@amd.com> > >> Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > >> gregkh@linuxfoundation.org; rafael@kernel.org; eric.auger@redhat.com; > >> alex.williamson@redhat.com; cohuck@redhat.com; Gupta, Puneet (DCG-ENG) > >> <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; > >> mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; > >> jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; mani@kernel.org; > >> yishaih@nvidia.com; jgg@ziepe.ca; jgg@nvidia.com; robin.murphy@arm.com; > >> will@kernel.org; joro@8bytes.org; masahiroy@kernel.org; > >> ndesaulniers@google.com; linux-arm-kernel@lists.infradead.org; linux- > >> kbuild@vger.kernel.org; linux-kernel@vger.kernel.org; > >> devicetree@vger.kernel.org; kvm@vger.kernel.org; okaya@kernel.org; Anand, > >> Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil > >> <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; > >> Radovanovic, Aleksandar <aleksandar.radovanovic@amd.com>; git (AMD-Xilinx) > >> <git@amd.com> > >> Subject: Re: [RFC PATCH v3 3/7] iommu/arm-smmu-v3: support ops registration > >> for CDX bus > >> > >> [CAUTION: External Email] > >> > >> On Tue, Sep 6, 2022 at 6:48 AM Nipun Gupta <nipun.gupta@amd.com> wrote: > >>> > >>> With new CDX bus supported for AMD FPGA devices on ARM > >>> platform, the bus requires registration for the SMMU v3 > >>> driver. > >>> > >>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > >>> --- > >>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++-- > >>> 1 file changed, 14 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>> index d32b02336411..8ec9f2baf12d 100644 > >>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>> @@ -29,6 +29,7 @@ > >>> #include <linux/platform_device.h> > >>> > >>> #include <linux/amba/bus.h> > >>> +#include <linux/cdx/cdx_bus.h> > >>> > >>> #include "arm-smmu-v3.h" > >>> #include "../../iommu-sva-lib.h" > >>> @@ -3690,16 +3691,27 @@ static int arm_smmu_set_bus_ops(struct > >> iommu_ops *ops) > >>> if (err) > >>> goto err_reset_pci_ops; > >>> } > >>> +#endif > >>> +#ifdef CONFIG_CDX_BUS > >>> + if (cdx_bus_type.iommu_ops != ops) { > >>> + err = bus_set_iommu(&cdx_bus_type, ops); > >>> + if (err) > >>> + goto err_reset_amba_ops; > >>> + } > >> > >> I'm not an expert on IOMMUs, so apologies if the question is stupid. > >> > >> Why does the CDX bus need special treatment here (like PCI) when there > >> are so many other busses (eg: I2C, SPI, etc) that don't need any > >> changes here? > > > > AFAIU, the devices on I2C/SPI does not use SMMU. Apart from PCI/AMBA, > > FSL-MC is another similar bus (on SMMUv2) which uses SMMU ops. > > > > The devices here are behind SMMU. Robin can kindly correct or add > > more here from SMMU perspective. > > Indeed, there is no need to describe and handle how DMA may or may not > be translated for I2C/SPI/USB/etc. because they are not DMA-capable > buses (in those cases the relevant bus *controller* often does DMA, but > it does that for itself as the platform/PCI/etc. device it is). Ok this is what I was guessing was the reason, but didn't want to make that assumption. So if there are other cases like AMBA, FSL-MC where the devices can do direct DMA, why do those buses not need a #ifdef section in this function like CDX? Or put another way, why does CDX need special treatment? > Note that I have a series pending[1] that will make this patch a whole > lot simpler. Thanks for the pointer. I'll make some comments in that series about bus notifiers. -Saravana > > Thanks, > Robin. > > [1] > https://lore.kernel.org/linux-iommu/cover.1660572783.git.robin.murphy@arm.com/T/#t > > > > > Thanks, > > Nipun > > > >> > >> -Saravana > >> > >>> #endif > >>> if (platform_bus_type.iommu_ops != ops) { > >>> err = bus_set_iommu(&platform_bus_type, ops); > >>> if (err) > >>> - goto err_reset_amba_ops; > >>> + goto err_reset_cdx_ops; > >>> } > >>> > >>> return 0; > >>> > >>> -err_reset_amba_ops: > >>> +err_reset_cdx_ops: > >>> +#ifdef CONFIG_CDX_BUS > >>> + bus_set_iommu(&cdx_bus_type, NULL); > >>> +#endif > >>> +err_reset_amba_ops: __maybe_unused; > >>> #ifdef CONFIG_ARM_AMBA > >>> bus_set_iommu(&amba_bustype, NULL); > >>> #endif > >>> -- > >>> 2.25.1 > >>>
On 2022-09-07 19:24, Saravana Kannan wrote: > On Wed, Sep 7, 2022 at 1:27 AM Robin Murphy <robin.murphy@arm.com> wrote: >> >> On 2022-09-07 04:17, Gupta, Nipun wrote: >>> [AMD Official Use Only - General] >>> >>> >>> >>>> -----Original Message----- >>>> From: Saravana Kannan <saravanak@google.com> >>>> Sent: Wednesday, September 7, 2022 5:41 AM >>>> To: Gupta, Nipun <Nipun.Gupta@amd.com> >>>> Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; >>>> gregkh@linuxfoundation.org; rafael@kernel.org; eric.auger@redhat.com; >>>> alex.williamson@redhat.com; cohuck@redhat.com; Gupta, Puneet (DCG-ENG) >>>> <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; >>>> mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; >>>> jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; mani@kernel.org; >>>> yishaih@nvidia.com; jgg@ziepe.ca; jgg@nvidia.com; robin.murphy@arm.com; >>>> will@kernel.org; joro@8bytes.org; masahiroy@kernel.org; >>>> ndesaulniers@google.com; linux-arm-kernel@lists.infradead.org; linux- >>>> kbuild@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> devicetree@vger.kernel.org; kvm@vger.kernel.org; okaya@kernel.org; Anand, >>>> Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil >>>> <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; >>>> Radovanovic, Aleksandar <aleksandar.radovanovic@amd.com>; git (AMD-Xilinx) >>>> <git@amd.com> >>>> Subject: Re: [RFC PATCH v3 3/7] iommu/arm-smmu-v3: support ops registration >>>> for CDX bus >>>> >>>> [CAUTION: External Email] >>>> >>>> On Tue, Sep 6, 2022 at 6:48 AM Nipun Gupta <nipun.gupta@amd.com> wrote: >>>>> >>>>> With new CDX bus supported for AMD FPGA devices on ARM >>>>> platform, the bus requires registration for the SMMU v3 >>>>> driver. >>>>> >>>>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> >>>>> --- >>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++-- >>>>> 1 file changed, 14 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>>> index d32b02336411..8ec9f2baf12d 100644 >>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>>>> @@ -29,6 +29,7 @@ >>>>> #include <linux/platform_device.h> >>>>> >>>>> #include <linux/amba/bus.h> >>>>> +#include <linux/cdx/cdx_bus.h> >>>>> >>>>> #include "arm-smmu-v3.h" >>>>> #include "../../iommu-sva-lib.h" >>>>> @@ -3690,16 +3691,27 @@ static int arm_smmu_set_bus_ops(struct >>>> iommu_ops *ops) >>>>> if (err) >>>>> goto err_reset_pci_ops; >>>>> } >>>>> +#endif >>>>> +#ifdef CONFIG_CDX_BUS >>>>> + if (cdx_bus_type.iommu_ops != ops) { >>>>> + err = bus_set_iommu(&cdx_bus_type, ops); >>>>> + if (err) >>>>> + goto err_reset_amba_ops; >>>>> + } >>>> >>>> I'm not an expert on IOMMUs, so apologies if the question is stupid. >>>> >>>> Why does the CDX bus need special treatment here (like PCI) when there >>>> are so many other busses (eg: I2C, SPI, etc) that don't need any >>>> changes here? >>> >>> AFAIU, the devices on I2C/SPI does not use SMMU. Apart from PCI/AMBA, >>> FSL-MC is another similar bus (on SMMUv2) which uses SMMU ops. >>> >>> The devices here are behind SMMU. Robin can kindly correct or add >>> more here from SMMU perspective. >> >> Indeed, there is no need to describe and handle how DMA may or may not >> be translated for I2C/SPI/USB/etc. because they are not DMA-capable >> buses (in those cases the relevant bus *controller* often does DMA, but >> it does that for itself as the platform/PCI/etc. device it is). > > Ok this is what I was guessing was the reason, but didn't want to make > that assumption. > > So if there are other cases like AMBA, FSL-MC where the devices can do > direct DMA, why do those buses not need a #ifdef section in this > function like CDX? Or put another way, why does CDX need special treatment? Er, it doesn't? The only non-optional bus here is platform, since the others *can* be configured out and *are* #ifdefed accordingly. This patch is fine for the kernel it was based on, it'll just want rewriting now that I've cleaned all this horrible driver boilerplate up. And according to the thread on patch #4 there might need to be additional changes for CDX to express a reserved MSI region for SMMU support to actually work properly. Robin.
On Wed, Sep 7, 2022 at 1:40 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2022-09-07 19:24, Saravana Kannan wrote: > > On Wed, Sep 7, 2022 at 1:27 AM Robin Murphy <robin.murphy@arm.com> wrote: > >> > >> On 2022-09-07 04:17, Gupta, Nipun wrote: > >>> [AMD Official Use Only - General] > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: Saravana Kannan <saravanak@google.com> > >>>> Sent: Wednesday, September 7, 2022 5:41 AM > >>>> To: Gupta, Nipun <Nipun.Gupta@amd.com> > >>>> Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > >>>> gregkh@linuxfoundation.org; rafael@kernel.org; eric.auger@redhat.com; > >>>> alex.williamson@redhat.com; cohuck@redhat.com; Gupta, Puneet (DCG-ENG) > >>>> <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; > >>>> mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; > >>>> jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; mani@kernel.org; > >>>> yishaih@nvidia.com; jgg@ziepe.ca; jgg@nvidia.com; robin.murphy@arm.com; > >>>> will@kernel.org; joro@8bytes.org; masahiroy@kernel.org; > >>>> ndesaulniers@google.com; linux-arm-kernel@lists.infradead.org; linux- > >>>> kbuild@vger.kernel.org; linux-kernel@vger.kernel.org; > >>>> devicetree@vger.kernel.org; kvm@vger.kernel.org; okaya@kernel.org; Anand, > >>>> Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil > >>>> <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; > >>>> Radovanovic, Aleksandar <aleksandar.radovanovic@amd.com>; git (AMD-Xilinx) > >>>> <git@amd.com> > >>>> Subject: Re: [RFC PATCH v3 3/7] iommu/arm-smmu-v3: support ops registration > >>>> for CDX bus > >>>> > >>>> [CAUTION: External Email] > >>>> > >>>> On Tue, Sep 6, 2022 at 6:48 AM Nipun Gupta <nipun.gupta@amd.com> wrote: > >>>>> > >>>>> With new CDX bus supported for AMD FPGA devices on ARM > >>>>> platform, the bus requires registration for the SMMU v3 > >>>>> driver. > >>>>> > >>>>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > >>>>> --- > >>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++-- > >>>>> 1 file changed, 14 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>>>> index d32b02336411..8ec9f2baf12d 100644 > >>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > >>>>> @@ -29,6 +29,7 @@ > >>>>> #include <linux/platform_device.h> > >>>>> > >>>>> #include <linux/amba/bus.h> > >>>>> +#include <linux/cdx/cdx_bus.h> > >>>>> > >>>>> #include "arm-smmu-v3.h" > >>>>> #include "../../iommu-sva-lib.h" > >>>>> @@ -3690,16 +3691,27 @@ static int arm_smmu_set_bus_ops(struct > >>>> iommu_ops *ops) > >>>>> if (err) > >>>>> goto err_reset_pci_ops; > >>>>> } > >>>>> +#endif > >>>>> +#ifdef CONFIG_CDX_BUS > >>>>> + if (cdx_bus_type.iommu_ops != ops) { > >>>>> + err = bus_set_iommu(&cdx_bus_type, ops); > >>>>> + if (err) > >>>>> + goto err_reset_amba_ops; > >>>>> + } > >>>> > >>>> I'm not an expert on IOMMUs, so apologies if the question is stupid. > >>>> > >>>> Why does the CDX bus need special treatment here (like PCI) when there > >>>> are so many other busses (eg: I2C, SPI, etc) that don't need any > >>>> changes here? > >>> > >>> AFAIU, the devices on I2C/SPI does not use SMMU. Apart from PCI/AMBA, > >>> FSL-MC is another similar bus (on SMMUv2) which uses SMMU ops. > >>> > >>> The devices here are behind SMMU. Robin can kindly correct or add > >>> more here from SMMU perspective. > >> > >> Indeed, there is no need to describe and handle how DMA may or may not > >> be translated for I2C/SPI/USB/etc. because they are not DMA-capable > >> buses (in those cases the relevant bus *controller* often does DMA, but > >> it does that for itself as the platform/PCI/etc. device it is). > > > > Ok this is what I was guessing was the reason, but didn't want to make > > that assumption. > > > > So if there are other cases like AMBA, FSL-MC where the devices can do > > direct DMA, why do those buses not need a #ifdef section in this > > function like CDX? Or put another way, why does CDX need special treatment? > > Er, it doesn't? The only non-optional bus here is platform, since the > others *can* be configured out and *are* #ifdefed accordingly. Ah ok. Also I somehow missed the #ifdef AMBA there and thought there was only #ifdef PCI and the rest of the buses somehow got it working without having to muck around arm-smmu-v3.c. Thanks for the explanation. I'm done here :) -Saravana > This > patch is fine for the kernel it was based on, it'll just want rewriting > now that I've cleaned all this horrible driver boilerplate up. And > according to the thread on patch #4 there might need to be additional > changes for CDX to express a reserved MSI region for SMMU support to > actually work properly. > > Robin.
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index d32b02336411..8ec9f2baf12d 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -29,6 +29,7 @@ #include <linux/platform_device.h> #include <linux/amba/bus.h> +#include <linux/cdx/cdx_bus.h> #include "arm-smmu-v3.h" #include "../../iommu-sva-lib.h" @@ -3690,16 +3691,27 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops) if (err) goto err_reset_pci_ops; } +#endif +#ifdef CONFIG_CDX_BUS + if (cdx_bus_type.iommu_ops != ops) { + err = bus_set_iommu(&cdx_bus_type, ops); + if (err) + goto err_reset_amba_ops; + } #endif if (platform_bus_type.iommu_ops != ops) { err = bus_set_iommu(&platform_bus_type, ops); if (err) - goto err_reset_amba_ops; + goto err_reset_cdx_ops; } return 0; -err_reset_amba_ops: +err_reset_cdx_ops: +#ifdef CONFIG_CDX_BUS + bus_set_iommu(&cdx_bus_type, NULL); +#endif +err_reset_amba_ops: __maybe_unused; #ifdef CONFIG_ARM_AMBA bus_set_iommu(&amba_bustype, NULL); #endif
With new CDX bus supported for AMD FPGA devices on ARM platform, the bus requires registration for the SMMU v3 driver. Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)