Re: [PATCH v4 3/5] iommu/arm-smmu: add ACTLR data and support for SM8550
From: Bibek Kumar Patro
Date: Mon Dec 18 2023 - 06:23:46 EST
On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
On 16/12/2023 02:03, Konrad Dybcio wrote:
On 15.12.2023 13:54, Robin Murphy wrote:
On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro
<quic_bibekkum@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89
++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index cb49291f5233..d2006f610243 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -20,6 +20,85 @@ struct actlr_config {
u32 actlr;
};
+/*
+ * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching
in the
+ * macro TLB) and BIT(1) as CPRE (Enable context caching in the
prefetch
+ * buffer). The remaining bits are implementation defined and
vary across
+ * SoCs.
+ */
+
+#define PREFETCH_DEFAULT 0
+#define PREFETCH_SHALLOW BIT(8)
+#define PREFETCH_MODERATE BIT(9)
+#define PREFETCH_DEEP (BIT(9) | BIT(8))
I thin the following might be more correct:
#include <linux/bitfield.h>
#define PREFETCH_MASK GENMASK(9, 8)
#define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
#define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
#define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
#define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
Ack, thanks for this suggestion. Let me try this out using
GENMASK. Once tested, will take care of this in next version.
FWIW the more typical usage would be to just define the named macros
for the raw field values, then put the FIELD_PREP() at the point of
use. However in this case that's liable to get pretty verbose, so
although I'm usually a fan of bitfield.h, the most readable option
here might actually be to stick with simpler definitions of "(0 <<
8)", "(1 << 8)", etc. However it's not really a big deal either way,
and I defer to whatever Dmitry and Konrad prefer, since they're the
ones looking after arm-smmu-qcom the most :)
My 5 cents would be to just use the "common" style of doing this, so:
#define ACTRL_PREFETCH GENMASK(9, 8)
#define PREFETCH_DEFAULT 0
#define PREFETCH_SHALLOW 1
#define PREFETCH_MODERATE 2
#define PREFETCH_DEEP 3
and then use
| FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)
it can get verbose, but.. arguably that's good, since you really want
to make sure the right bits are set here
Sounds good to me.
Konrad, Dimitry, just checked FIELD_PREP() implementation
#define FIELD_FIT(_mask, _val)
({ \
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
since it is defined as a block, it won't be possible to use FIELD_PREP
in macro or as a structure value, and can only be used inside a
block/function. Orelse would show compilation errors as following
kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in
expansion of macro 'PREFETCH_SHALLOW'
{ 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
^
kernel/include/linux/bitfield.h:113:2: error: braced-group within
expression allowed only inside a function
({ \
^
So as per my understanding I think, we might need to go ahead with the
generic implementation only. Let me know if I missed something.
Thanks,
Bibek