On Tue, Aug 06, 2024 at 07:11:52PM -0700, Nicolin Chen wrote:
>
> -static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> - struct arm_smmu_device *smmu)
> +static struct arm_smmu_device *
> +arm_smmu_impl_acpi_probe(struct arm_smmu_device *smmu,
> + struct acpi_iort_node *node)
> +{
> + /*
> + * DSDT might hold some SMMU extension, so we have no option but to go
> + * through the ACPI tables unconditionally. On success, this returns a
> + * copy of smmu struct holding an impl pointer. Otherwise, an impl may
> + * choose to return an ERR_PTR as an error out, or to return the pass-
> + * in smmu pointer as a fallback to the standard SMMU.
> + */
> + return arm_smmu_impl_acpi_dsdt_probe(smmu, node);
> +}
Lets generalize this a bit more and have the impl mechanism work for
DT too.. Keep the main probe the same and add a new function after the
dt/acpi steps:
smmu = arm_smmu_probe_impl(smmu);
if (IS_ERR(smmu))
return PTR_ERR(smmu);
Which is more like:
/*
* Probe all the compiled in implementations. Each one checks to see if it
* matches this HW and if so returns a devm_krealloc'd arm_smmu_device which
* replaces the callers. Otherwise the original is returned or ERR_PTR.
*
*/
static struct arm_smmu_device *arm_smmu_probe_impl(struct arm_smmu_device *orig_smmu)
{
struct arm_smmu_device *new_smmu;
int ret;
new_smmu = tegra241_cmdqv_acpi_dsdt_probe(orig_smmu);
if (new_smmu != ERR_PTR(-ENODEV))
goto out_new_impl;
return orig_smmu;
out_new_impl:
if (IS_ERR(new_smmu))
return new_smmu;
/* FIXME: check is this ordering OK during remove? */
ret = devm_add_action_or_reset(new_smmu->dev, arm_smmu_impl_remove,
new_smmu);
if (ret)
return ERR_PTR(ret);
return new_smmu;
}
Easy to add new sub implementations. Provide an inline ENODEV sub in
the header file for tegra241_cmdqv_acpi_dsdt_probe
Add something like this to the header to get the ACPI node:
static inline struct acpi_iort_node *
arm_smmu_get_iort_node(struct arm_smmu_device *smmu)
{
return *(struct acpi_iort_node **)dev_get_platdata(smmu->dev);
}
Since it isn't passed down
> @@ -4560,6 +4602,7 @@ static void arm_smmu_device_remove(struct platform_device *pdev)
> {
> struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
>
> + arm_smmu_impl_remove(smmu);
Can't call this if devm has been used to set it up, and this would be
in the wrong order anyhow. Just remove it.. I guess the devm was put
for this to avoid adding goto error unwind to probe?
> +struct arm_smmu_impl {
> + int (*device_reset)(struct arm_smmu_device *smmu);
> + void (*device_remove)(struct arm_smmu_device *smmu);
> + struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu);
> +};
Can we put the word "ops" into this struct somehow? That would be a
more typically kernely name.
arm_smmu_impl_ops perhaps?
> struct arm_smmu_device {
> struct device *dev;
> + /* An SMMUv3 implementation */
The comment is self explanatory
Jason
