Re: [PATCH v4 3/5] iommu/arm-smmu: add ACTLR data and support for SM8550
From: Bibek Kumar Patro
Date: Tue Dec 19 2023 - 06:40:07 EST
On 12/19/2023 4:14 PM, Dmitry Baryshkov wrote:
On Tue, 19 Dec 2023 at 12:37, Bibek Kumar Patro
<quic_bibekkum@xxxxxxxxxxx> wrote:
On 12/19/2023 3:51 PM, Dmitry Baryshkov wrote:
On Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro
<quic_bibekkum@xxxxxxxxxxx> wrote:
On 12/18/2023 7:51 PM, Dmitry Baryshkov wrote:
On 18/12/2023 13:23, Bibek Kumar Patro wrote:
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.
Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned
earlier in his reply.
I can implement the defines as:
#define PREFETCH_DEFAULT 0
#define PREFETCH_SHALLOW (1 << 8)
#define PREFETCH_MODERATE (1 << 9)
2 << 8. Isn't that hard.
Ah, right. This is nice! .
Will use 2 << 8 instead. Thanks for the suggestion.
It might still be useful to define the PREFETCH_SHIFT equal to 8.
Sure, looks okay to me as well to define PREFETCH_SHIFT to 8
as it's constant.
Thanks,
Bibek
Thanks,
Bibek
#define PREFETCH_DEEP (3 << 8)
This should be okay I think ?
Thanks,
Bibek