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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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)
#define PREFETCH_DEEP          (3 << 8)

This should be okay I think ?

Thanks,
Bibek





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux