Re: [PATCH v9 4/5] iommu/arm-smmu: add ACTLR data and support for SM8550

From: Will Deacon <will@kernel.org>
To: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
Cc: robin.murphy@arm.com, joro@8bytes.org,
	dmitry.baryshkov@linaro.org, konrad.dybcio@linaro.org,
	jsnitsel@redhat.com, quic_bjorande@quicinc.com, mani@kernel.org,
	quic_eberman@quicinc.com, robdclark@chromium.org,
	u.kleine-koenig@pengutronix.de, robh@kernel.org,
	vladimir.oltean@nxp.com, quic_pkondeti@quicinc.com,
	quic_molvera@quicinc.com, linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev,
	linux-kernel@vger.kernel.org, krzysztof.kozlowski+dt@linaro.org,
	saravanak@google.com
Subject: Re: [PATCH v9 4/5] iommu/arm-smmu: add ACTLR data and support for SM8550
Date: Tue, 13 Feb 2024 13:47:14 +0000	[thread overview]
Message-ID: <20240213134714.GC28926@willie-the-truck> (raw)
In-Reply-To: <20240123144543.9405-5-quic_bibekkum@quicinc.com>

On Tue, Jan 23, 2024 at 08:15:42PM +0530, Bibek Kumar Patro wrote:
> Add ACTLR data table for SM8550 along with support for
> same including SM8550 specific implementation operations.
> 
> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 90 ++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 6004c6d9a7b2..db15b1eade97 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -23,6 +23,86 @@
> 
>  #define CPRE			(1 << 1)
>  #define CMTLB			(1 << 0)
> +#define PREFETCH_SHIFT		8
> +#define PREFETCH_DEFAULT	0
> +#define PREFETCH_SHALLOW	(1 << PREFETCH_SHIFT)
> +#define PREFETCH_MODERATE	(2 << PREFETCH_SHIFT)
> +#define PREFETCH_DEEP		(3 << PREFETCH_SHIFT)
> +#define PREFETCH_SWITCH_GFX	(5 << 3)
> +
> +static const struct actlr_config sm8550_apps_actlr_cfg[] = {
> +	{ 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +	{ 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
> +	{ 0x0800, 0x0020, PREFETCH_DEFAULT | CMTLB },
> +	{ 0x1800, 0x00c0, PREFETCH_DEFAULT | CMTLB },
> +	{ 0x1820, 0x0000, PREFETCH_DEFAULT | CMTLB },
> +	{ 0x1860, 0x0000, PREFETCH_DEFAULT | CMTLB },
> +	{ 0x0c01, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +	{ 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +	{ 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +	{ 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +	{ 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },
> +	{ 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB },

[...]

Isn't this effectively hard-coding the topology of the SoC in the driver?
Wouldn't it better describing higher-level prefetch properties in the DT
nodes corresponding to the upstream devices?

Looking back at the prior revisions of this series, it seems like others
were in favour of this approach, so if that's the general consensus, then
so be it. But is this _really_ what we want in the SMMU driver? It would
be good to have an Ack from Robin and a DT maintainer on this mechanism.

It just all feels a bit like a step back into the bad old world of
platform data to me, where we end up trying to maintain a bunch of random
constants that supposedly make things faster for somebody :/

Will

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