Messages in this thread |  | | Date | Mon, 11 Mar 2024 14:12:06 +0530 | | From | Bibek Kumar Patro <> | | Subject | Re: [PATCH v9 4/5] iommu/arm-smmu: add ACTLR data and support for SM8550 |
| |
On 2/21/2024 6:51 PM, Will Deacon wrote: > On Wed, Feb 21, 2024 at 02:25:26PM +0530, Bibek Kumar Patro wrote: >> On 2/13/2024 7:17 PM, Will Deacon wrote: >>> On Tue, Jan 23, 2024 at 08:15:42PM +0530, 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@quicinc.com> >>>> --- >>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 90 ++++++++++++++++++++++ >>>> 1 file changed, 90 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> index 6004c6d9a7b2..db15b1eade97 100644 >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> @@ -23,6 +23,86 @@ >>>> >>>> #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) >>>> +#define PREFETCH_SWITCH_GFX (5 << 3) >>>> + >>>> +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 }, >>>> + { 0x0c02, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c03, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c04, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c05, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>>> + { 0x0c06, 0x0020, PREFETCH_DEEP | CPRE | CMTLB }, >>> >>> [...] >>> >>> Isn't this effectively hard-coding the topology of the SoC in the driver? >>> Wouldn't it better describing higher-level prefetch properties in the DT >>> nodes corresponding to the upstream devices? >> >> Since prefetch data stored in this table represent settings for the >> ACTLR register, and doesn't exactly define the hardware (So in this >> manner prefetch data won't exactly be a part of soc topology ?). > > The first two columns of the table are StreamID/Mask pairs, no? How is that > _not_ the SoC topology? I really think it would be better to define some > high-level prefetch properties in the DT binding which can be put on the > master nodes. > >> So it seemed apt not to use the device tree for storing the prefetch >> property. Hence we reverted from the DT approach (initial proposal in >> RFC to piggyback on iommus property to store prefetch settings) back to use >> driver for storing this data. >> >> Some drivers use the same approach for storing their platform specific >> data. Examples being >> drivers/phy/qualcomm/phy-qcom-qmp-combo.c >> drivers/soc/qcom/llcc-qcom.c >> These drivers were taken as reference for storing platform specific ACTLR >> data. > > I don't know anything about those drivers, but on the SMMU side we already > have ways to describe the topology in the DT and the driver is using them, > so I'm struggling to see the need to add these tables as well. > > But as I said before, if Robin and the DT folks prefer this approach, > then I won't get in the way. >
With the driver approach at the current state of patches, it has been ACKed by DT folks and it seems there has been no concern/objection from Robin till now. So can this patch go ahead Will? Let us know Robin of your opinion as well please.
Thanks & regards, Bibek
> Will
|  |