On Wed, Sep 7, 2022 at 1:40 PM Robin Murphy <robin.murphy@xxxxxxx> wrote: > > On 2022-09-07 19:24, Saravana Kannan wrote: > > On Wed, Sep 7, 2022 at 1:27 AM Robin Murphy <robin.murphy@xxxxxxx> wrote: > >> > >> On 2022-09-07 04:17, Gupta, Nipun wrote: > >>> [AMD Official Use Only - General] > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: Saravana Kannan <saravanak@xxxxxxxxxx> > >>>> Sent: Wednesday, September 7, 2022 5:41 AM > >>>> To: Gupta, Nipun <Nipun.Gupta@xxxxxxx> > >>>> Cc: robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; > >>>> gregkh@xxxxxxxxxxxxxxxxxxx; rafael@xxxxxxxxxx; eric.auger@xxxxxxxxxx; > >>>> alex.williamson@xxxxxxxxxx; cohuck@xxxxxxxxxx; Gupta, Puneet (DCG-ENG) > >>>> <puneet.gupta@xxxxxxx>; song.bao.hua@xxxxxxxxxxxxx; > >>>> mchehab+huawei@xxxxxxxxxx; maz@xxxxxxxxxx; f.fainelli@xxxxxxxxx; > >>>> jeffrey.l.hugo@xxxxxxxxx; Michael.Srba@xxxxxxxxx; mani@xxxxxxxxxx; > >>>> yishaih@xxxxxxxxxx; jgg@xxxxxxxx; jgg@xxxxxxxxxx; robin.murphy@xxxxxxx; > >>>> will@xxxxxxxxxx; joro@xxxxxxxxxx; masahiroy@xxxxxxxxxx; > >>>> ndesaulniers@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > >>>> kbuild@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > >>>> devicetree@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; okaya@xxxxxxxxxx; Anand, > >>>> Harpreet <harpreet.anand@xxxxxxx>; Agarwal, Nikhil > >>>> <nikhil.agarwal@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>; > >>>> Radovanovic, Aleksandar <aleksandar.radovanovic@xxxxxxx>; git (AMD-Xilinx) > >>>> <git@xxxxxxx> > >>>> 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@xxxxxxx> 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@xxxxxxx> > >>>>> --- > >>>>> 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.