RE: [PATCH v5 3/6] iommu/arm-smmu-v3: Add feature detection for HTTU

From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
To: Nicolin Chen <nicolinc@nvidia.com>
Cc: "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"will@kernel.org" <will@kernel.org>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"jgg@nvidia.com" <jgg@nvidia.com>,
	"ryan.roberts@arm.com" <ryan.roberts@arm.com>,
	"kevin.tian@intel.com" <kevin.tian@intel.com>,
	"mshavit@google.com" <mshavit@google.com>,
	"eric.auger@redhat.com" <eric.auger@redhat.com>,
	"joao.m.martins@oracle.com" <joao.m.martins@oracle.com>,
	jiangkunkun <jiangkunkun@huawei.com>,
	zhukeqian <zhukeqian1@huawei.com>, Linuxarm <linuxarm@huawei.com>
Subject: RE: [PATCH v5 3/6] iommu/arm-smmu-v3: Add feature detection for HTTU
Date: Fri, 7 Jun 2024 08:04:45 +0000	[thread overview]
Message-ID: <e4e6c50d181641d086cb3ef1d60b6732@huawei.com> (raw)
In-Reply-To: <ZmJB/IjP+RVTKp/5@Asurada-Nvidia>

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 7, 2024 12:11 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> jgg@nvidia.com; ryan.roberts@arm.com; kevin.tian@intel.com;
> mshavit@google.com; eric.auger@redhat.com; joao.m.martins@oracle.com;
> jiangkunkun <jiangkunkun@huawei.com>; zhukeqian
> <zhukeqian1@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v5 3/6] iommu/arm-smmu-v3: Add feature detection for
> HTTU
> 
> Hi Shameer,
> 
> Some nitpicking inline.

Thanks for taking a look Nicolin.

> 
> On Thu, Jun 06, 2024 at 02:32:59PM +0100, Shameer Kolothum wrote:
> 
> > 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 e30ce55ed1af..c05a74aa52a4 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3738,6 +3738,29 @@ static void arm_smmu_device_iidr_probe(struct
> arm_smmu_device *smmu)
> >         }
> >  }
> >
> > +static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32
> reg)
> 
> Following the other helper arm_smmu_device_iidr_probe, how about
> arm_smmu_device_httu_probe?

Hmm..I don't have any objection per se but "_get_" feels like more simple
and straightforward to me. Let me know if you feel strongly about this.

> And we could pass in FIELD_GET(IDR0_HTTU, reg) too?

Same here.

> 
> > +{
> > +       u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA |
> ARM_SMMU_FEAT_HD);
> > +       u32 httu = FIELD_GET(IDR0_HTTU, reg);
> > +       u32 features = 0;
> 
> How about "hw_feats" v.s. "fw_feats"?

Ok.

> 
> > +
> > +       switch (httu) {
> > +       case IDR0_HTTU_ACCESS_DIRTY:
> > +               features |= ARM_SMMU_FEAT_HD;
> > +               fallthrough;
> > +       case IDR0_HTTU_ACCESS:
> > +               features |= ARM_SMMU_FEAT_HA;
> > +       }
> > +
> > +       if (smmu->dev->of_node)
> > +               smmu->features |= features;
> > +       else if (features != fw_features)
> > +               /* ACPI IORT sets the HTTU bits */
> > +               dev_warn(smmu->dev,
> > +                        "IDR0.HTTU(0x%x) overridden by FW configuration
> (0x%x)\n",
> > +                         httu, fw_features);
> 
> httu and fw_features have different shifts -- could be odd to see:
> 	IDR0.HTTU(0x2) overridden by FW configuration (0x600000)
> 
> FIELD_GET(ACPI_IORT_SMMU_V3_HTTU_OVERRIDE, iort_smmu->flags)
> seems
> to be overcomplicated to reference? So, how about just features?
> +			"IDR0.HTTU features (0x%x) overridden by FW
> configuration (0x%x)\n",

Yes. I think that make sense. Will change.

Thanks,
Shameer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel