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

[Bibek Kumar Patro]

On 11/15/2023 10:12 PM, Konrad Dybcio wrote:
> On 11/15/23 13:49, Bibek Kumar Patro wrote:
> > On 11/15/2023 4:15 PM, Dmitry Baryshkov wrote:
> > > On Wed, 15 Nov 2023 at 11:51, Bibek Kumar Patro
> > > <quic_bibekkum@xxxxxxxxxxx> wrote:
> > > > On 11/15/2023 3:08 PM, Dmitry Baryshkov wrote:
> > > > > On Wed, 15 Nov 2023 at 11:22, Bibek Kumar Patro
> > > > > <quic_bibekkum@xxxxxxxxxxx> wrote:
> > > > > > On 11/14/2023 7:42 PM, Dmitry Baryshkov wrote:
> > > > > > > On Tue, 14 Nov 2023 at 15:57, 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 | 92 
> > > > > > > > +++++++++++++++++++++-
> > > > > > > >     1 file changed, 88 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
> > > > > > > > b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > > > > > index 578c662c7c30..0eaf6f2a2e49 100644
> > > > > > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > > > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > > > > > @@ -25,6 +25,70 @@ struct actlr_data {
> > > > > > > >            u32 actlr;
> > > > > > > >     };
> > > > > > > >
> > > > > > > > +#define PRE_FETCH_1    0
> > > > > > > > +#define PRE_FETCH_2    BIT(8)
> > > > > > > > +#define PRE_FETCH_3    (BIT(9) | BIT(8))
> > > > > > >
> > > > > > > What is the difference between PRE_FETCH_3 and PRE_FETCH_2? And
> > > > > > > PRE_FETCH_1? Are these real numbers that refer to some amount / 
> > > > > > > count
> > > > > > > or just dummy names?
> > > > > >
> > > > > > No,these are not real numbers, but prefetch settings for a particular
> > > > > > perfect configuration.
> > > > >
> > > > > Then I'd ask for some better names or descriptions.
> > > >
> > > > Noted, PREFETCH_SETTING_n / PREFETCH_OPTION_n sounds like a better name
> > > > in the following case. Would it be okay to use this name instead?
> > >
> > > Not really.
> >
> > Any suggestion you have in mind, if not this nomenclature?
> Dmitry's concern seems to be that you provide:
>
> PRE_FETCH_1 /* prefetcher with settings preset no. 1 */
> PRE_FETCH_2 /* prefetcher with settings preset no. 2 */
> PRE_FETCH_3 /* prefetcher with settings preset no. 3 */
>
> whereas it would be both useful and interesting to see what these
> settings mean, i.e. what differences there are between all of
> these presets.

Ah, okay got it now from Dimitry and yours' response.
But we exactly won't be able to reveal what each of these settings
mean, as this might risk of revealing IP as ACTLR bits are
implementation defined (except CPRE and CMTLB) which other SoC vendors
might be using it in different manner(or different purpose) in their
downstream implementation.
We can name it like (e.g PREFETCH_DISABLE, PREFETCH_SHALLOW,
PREFETCH_DEEP) to indicate the behaviour, but won't be exactly
name/describe it to explain what it does with a particular setting.

> Konrad