| Message ID | 8a2629bb98cabb1be72b32c120bb5ed0114b21bc.1724453781.git.nicolinc@nvidia.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add Tegra241 (Grace) CMDQV Support (part 1/2) | expand |
On Fri, Aug 23, 2024 at 05:10:40PM -0700, Nicolin Chen wrote: > For model-specific implementation, repurpose the acpi_smmu_get_options() > to a wider acpi_smmu_acpi_probe_model(). A new model can add to the list > in this new function. > > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 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 afdb8a76a72a..ceb31d63f79b 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -4341,18 +4341,28 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu) > } > > #ifdef CONFIG_ACPI > -static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu) > +static int acpi_smmu_iort_probe_model(struct acpi_iort_node *node, > + struct arm_smmu_device *smmu) > { > - switch (model) { > + struct acpi_iort_smmu_v3 *iort_smmu = > + (struct acpi_iort_smmu_v3 *)node->node_data; > + > + switch (iort_smmu->model) { > case ACPI_IORT_SMMU_V3_CAVIUM_CN99XX: > smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY; > break; > case ACPI_IORT_SMMU_V3_HISILICON_HI161X: > smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH; > break; > + case ACPI_IORT_SMMU_V3_GENERIC: > + break; > + default: > + dev_err(smmu->dev, "Unknown/unsupported IORT model!\n"); > + return -ENXIO; We probably don't want this 'default' case, otherwise the driver will need to be updated every time there's a new model. If you agree, then I can just drop this bit when applying. Will
On 27/08/2024 2:02 pm, Will Deacon wrote: > On Fri, Aug 23, 2024 at 05:10:40PM -0700, Nicolin Chen wrote: >> For model-specific implementation, repurpose the acpi_smmu_get_options() >> to a wider acpi_smmu_acpi_probe_model(). A new model can add to the list >> in this new function. >> >> Suggested-by: Will Deacon <will@kernel.org> >> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> >> --- >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 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 afdb8a76a72a..ceb31d63f79b 100644 >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> @@ -4341,18 +4341,28 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu) >> } >> >> #ifdef CONFIG_ACPI >> -static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu) >> +static int acpi_smmu_iort_probe_model(struct acpi_iort_node *node, >> + struct arm_smmu_device *smmu) >> { >> - switch (model) { >> + struct acpi_iort_smmu_v3 *iort_smmu = >> + (struct acpi_iort_smmu_v3 *)node->node_data; >> + >> + switch (iort_smmu->model) { >> case ACPI_IORT_SMMU_V3_CAVIUM_CN99XX: >> smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY; >> break; >> case ACPI_IORT_SMMU_V3_HISILICON_HI161X: >> smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH; >> break; >> + case ACPI_IORT_SMMU_V3_GENERIC: >> + break; >> + default: >> + dev_err(smmu->dev, "Unknown/unsupported IORT model!\n"); >> + return -ENXIO; > > We probably don't want this 'default' case, otherwise the driver will > need to be updated every time there's a new model. ...although the intent is very strongly that there should never be any new models, because by now hardware should really not be failing to implement SMMU_IIDR correctly. In some ways, having this might help further discourage people from abusing the mechanism and making random stuff up in their firmware :/ Cheers, Robin.
On Tue, Aug 27, 2024 at 04:57:48PM +0100, Robin Murphy wrote: > > > @@ -4341,18 +4341,28 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu) > > > } > > > > > > #ifdef CONFIG_ACPI > > > -static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu) > > > +static int acpi_smmu_iort_probe_model(struct acpi_iort_node *node, > > > + struct arm_smmu_device *smmu) > > > { > > > - switch (model) { > > > + struct acpi_iort_smmu_v3 *iort_smmu = > > > + (struct acpi_iort_smmu_v3 *)node->node_data; > > > + > > > + switch (iort_smmu->model) { > > > case ACPI_IORT_SMMU_V3_CAVIUM_CN99XX: > > > smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY; > > > break; > > > case ACPI_IORT_SMMU_V3_HISILICON_HI161X: > > > smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH; > > > break; > > > + case ACPI_IORT_SMMU_V3_GENERIC: > > > + break; > > > + default: > > > + dev_err(smmu->dev, "Unknown/unsupported IORT model!\n"); > > > + return -ENXIO; > > > > We probably don't want this 'default' case, otherwise the driver will > > need to be updated every time there's a new model. > > ...although the intent is very strongly that there should never be any > new models, because by now hardware should really not be failing to > implement SMMU_IIDR correctly. In some ways, having this might help > further discourage people from abusing the mechanism and making random > stuff up in their firmware :/ I don't have a strong feeling about this, though Robin's point was my intention here. Apart from this "default case", I typo-ed the function name in the patch subject and commit message. Also, there's a missed kdoc line in struct tegra241_cmdqv (PATCH-8). So, I prepared a v14: https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v14 v14 changelog (attaching git-diff at the EOM): * Rebased on Will's for-joerg/arm-smmu/updates * Added a missed kdoc for "dev" in struct tegra241_cmdqv * Dropped the default case in acpi_smmu_iort_probe_model() (did this before seeing Robin's mail here.) Let's see what makes the best for you, Will. Thanks Nicolin 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 8b1437240ce5..f23245012681 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -4356,9 +4356,6 @@ static int acpi_smmu_iort_probe_model(struct acpi_iort_node *node, break; case ACPI_IORT_SMMU_V3_GENERIC: break; - default: - dev_err(smmu->dev, "Unknown/unsupported IORT model!\n"); - return -ENXIO; } dev_notice(smmu->dev, "option mask 0x%x\n", smmu->options); diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c index b794a4dcbce1..5ac3032ee6dd 100644 --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c @@ -160,6 +160,7 @@ struct tegra241_vintf { /** * struct tegra241_cmdqv - CMDQ-V for SMMUv3 * @smmu: SMMUv3 device + * @dev: CMDQV device * @base: MMIO base address * @irq: IRQ number * @num_vintfs: Total number of VINTFs
Hi Will, On Tue, Aug 27, 2024 at 09:18:47AM -0700, Nicolin Chen wrote: > On Tue, Aug 27, 2024 at 04:57:48PM +0100, Robin Murphy wrote: > > > > -static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu) > > > > +static int acpi_smmu_iort_probe_model(struct acpi_iort_node *node, > > > > + struct arm_smmu_device *smmu) > > > > { > > > > - switch (model) { > > > > + struct acpi_iort_smmu_v3 *iort_smmu = > > > > + (struct acpi_iort_smmu_v3 *)node->node_data; > > > > + > > > > + switch (iort_smmu->model) { > > > > case ACPI_IORT_SMMU_V3_CAVIUM_CN99XX: > > > > smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY; > > > > break; > > > > case ACPI_IORT_SMMU_V3_HISILICON_HI161X: > > > > smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH; > > > > break; > > > > + case ACPI_IORT_SMMU_V3_GENERIC: > > > > + break; > > > > + default: > > > > + dev_err(smmu->dev, "Unknown/unsupported IORT model!\n"); > > > > + return -ENXIO; > > > > > > We probably don't want this 'default' case, otherwise the driver will > > > need to be updated every time there's a new model. > > > > ...although the intent is very strongly that there should never be any > > new models, because by now hardware should really not be failing to > > implement SMMU_IIDR correctly. In some ways, having this might help > > further discourage people from abusing the mechanism and making random > > stuff up in their firmware :/ > > I don't have a strong feeling about this, though Robin's point was > my intention here. > > Apart from this "default case", I typo-ed the function name in the > patch subject and commit message. Also, there's a missed kdoc line > in struct tegra241_cmdqv (PATCH-8). > > So, I prepared a v14: > https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v14 > v14 changelog (attaching git-diff at the EOM): > * Rebased on Will's for-joerg/arm-smmu/updates > * Added a missed kdoc for "dev" in struct tegra241_cmdqv > * Dropped the default case in acpi_smmu_iort_probe_model() > (did this before seeing Robin's mail here.) > > Let's see what makes the best for you, Will. I just sent v14 by keeping the default case, given Robin's remarks here. If you'd like to remove the default case, please feel free! Thank you Nicolin
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 afdb8a76a72a..ceb31d63f79b 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -4341,18 +4341,28 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu) } #ifdef CONFIG_ACPI -static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu) +static int acpi_smmu_iort_probe_model(struct acpi_iort_node *node, + struct arm_smmu_device *smmu) { - switch (model) { + struct acpi_iort_smmu_v3 *iort_smmu = + (struct acpi_iort_smmu_v3 *)node->node_data; + + switch (iort_smmu->model) { case ACPI_IORT_SMMU_V3_CAVIUM_CN99XX: smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY; break; case ACPI_IORT_SMMU_V3_HISILICON_HI161X: smmu->options |= ARM_SMMU_OPT_SKIP_PREFETCH; break; + case ACPI_IORT_SMMU_V3_GENERIC: + break; + default: + dev_err(smmu->dev, "Unknown/unsupported IORT model!\n"); + return -ENXIO; } dev_notice(smmu->dev, "option mask 0x%x\n", smmu->options); + return 0; } static int arm_smmu_device_acpi_probe(struct platform_device *pdev, @@ -4367,8 +4377,6 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev, /* Retrieve SMMUv3 specific data */ iort_smmu = (struct acpi_iort_smmu_v3 *)node->node_data; - acpi_smmu_get_options(iort_smmu->model, smmu); - if (iort_smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE) smmu->features |= ARM_SMMU_FEAT_COHERENCY; @@ -4380,7 +4388,7 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev, smmu->features |= ARM_SMMU_FEAT_HA; } - return 0; + return acpi_smmu_iort_probe_model(node, smmu); } #else static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
For model-specific implementation, repurpose the acpi_smmu_get_options() to a wider acpi_smmu_acpi_probe_model(). A new model can add to the list in this new function. Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)