Message-ID: <CAA8EJpqDFeCVEiuVM4RJoYPVjRn+PNRAYbe3RcFdDsDTj_Gkwg@mail.gmail.com>
Date: Thu, 4 Jul 2024 14:26:01 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Bibek Kumar Patro <quic_bibekkum@...cinc.com>
Cc: Will Deacon <will@...nel.org>, robdclark@...il.com, robin.murphy@....com, 
	joro@...tes.org, jgg@...pe.ca, jsnitsel@...hat.com, robh@...nel.org, 
	krzysztof.kozlowski@...aro.org, quic_c_gdjako@...cinc.com, 
	konrad.dybcio@...aro.org, iommu@...ts.linux.dev, 
	linux-arm-msm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v13 4/6] iommu/arm-smmu: add ACTLR data and support for SM8550

On Thu, 4 Jul 2024 at 12:12, Bibek Kumar Patro
<quic_bibekkum@...cinc.com> wrote:
>
>
>
> On 7/3/2024 6:32 PM, Will Deacon wrote:
> > On Wed, Jul 03, 2024 at 05:45:23PM +0530, Bibek Kumar Patro wrote:
> >>
> >>
> >> On 7/2/2024 12:04 AM, Dmitry Baryshkov wrote:
> >>> On Fri, Jun 28, 2024 at 07:34:33PM GMT, 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@...cinc.com>
> >>>> ---
> >>>>    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 77c9abffe07d..b4521471ffe9 100644
> >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> @@ -23,6 +23,85 @@
> >>>>
> >>>>    #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)
> >>>> +
> >>>> +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 },
> >>>
> >>> - Please keep the list sorted
> >>
> >> Sure Dmitry, will sort this list in reverse-christmas-tree order
> >> in next iteration. Thanks for this input.
> >>
> >>> - Please comment, which devices use these settings.
> >>
> >> As discussed in earlier versions of this patch, these table entries
> >> are kind of just blind values for SMMU device, where SMMU do not have
> >> idea on which SID belong to which client. During probe time when the
> >> clients' Stream-ID has corresponding ACTLR entry then the driver would
> >> set value in register.
> >
> > I'm still firmly of the opinion that this stuff needs a higher-level
> > description in the device-tree and should not be hard-coded in the driver
> > like this. It's not just a list of opaque values; it describes
> > SoC-specific topological information that should not be this rigid.
> >
>
> As per my understanding since ACTLR register is an implementation
> defined register,
> so I think the placement can also depend on factor of how these
> registers are used?
>
> For Qualcomm SoCs, it stores prefetch values for each client, improving
> performance without defining hardware design.
> Even without setting this value, clients on these Stream-IDs would still
> function, albeit with reduced performance.
>
> The SteamID/Mask pair in first two columns <which is a SoC topology> is
> only used as reference to find preferred prefetch setting for the
> corresponding client on this StreamID
>
> To refer initial discussion and Robin's thoughts on device-tree approach
> for this property which we proposed as a part of RFC:
> https://lore.kernel.org/all/a01e7e60-6ead-4a9e-ba90-22a8a6bbd03f@quicinc.com/
>
> " On 9/18/2023 4:49 PM, Robin Murphy wrote: "
>  >
>  > At the very least this would need to be in a implementation-specific
>  > backend, since everything about ACTLR is implementation-defined; there
>  > could be bits in there that the driver needs to manage itself and
>  > clients have absolutely no business overriding (e.g. the MMU-500 errata
>  > workarounds). The generic driver can't know what's valid, nor what the
>  > consequences are of not being able to satisfy a particular setting. Then
>  > there's still the question of what if two clients ask for different
>  > settings but want to attach to the same context?
>  >
>  > It's also questionable whether this would belong in DT at all, since it
>  > has a bit of a smell of software policv about it.
>  >
>  > If it could be
>  > sufficiently justified then it would need a proper binding proposal, and
>  > "write this opaque value into this register" type properties are
>  > generally not approved of.
>  >
>  > Thanks,
>  > Robin.
>  >
>
> So as per the initial discussions it felt right to have this data stored
> inside driver.
> One potential downside is that the driver file could become cluttered
> with this data, but this can be mitigated by storing the table in a
> separate file if necessary.
>
> For use cases or vendor that implement the ACTLR register differently,
> deeply involving SoC topology values or defining hardware design
> (something similar to Stream Matching Register),then it might be more
> appropriate to place it in the devicetree?
>
> This is just my understanding. I’d appreciate your further thoughts on
> this - Will, Robin, Dmitry, Rob.

My understanding was that DT should be a place for variable
information. In this case the mapping between Stream-IDs and the
corresponding register programming is more or less fixed for a
particular Soc.
Probably the only way this can be handled outside of the driver is by
increasing #iommu-cells and encoding these values in this extra IOMMU
cell.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists