On Thu, 4 Jul 2024 at 12:12, Bibek Kumar Patro
<quic_bibekkum@xxxxxxxxxxx> 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@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 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@xxxxxxxxxxx/
>
> " 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
