Re: [PATCH v7 3/5] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings
From: Bibek Kumar Patro
Date: Fri Jan 12 2024 - 08:08:15 EST
On 1/12/2024 3:31 PM, Konrad Dybcio wrote:
On 1/11/24 19:09, Bibek Kumar Patro wrote:
On 1/10/2024 11:26 PM, Konrad Dybcio wrote:
On 1/10/24 13:55, Bibek Kumar Patro wrote:
On 1/10/2024 4:46 PM, Bibek Kumar Patro wrote:
On 1/10/2024 9:36 AM, Pavan Kondeti wrote:
[...]
@@ -274,6 +321,21 @@ static const struct of_device_id
qcom_smmu_client_of_match[] __maybe_unused = {
static int qcom_smmu_init_context(struct arm_smmu_domain
*smmu_domain,
struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
{
+ struct arm_smmu_device *smmu = smmu_domain->smmu;
+ struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
+ const struct actlr_variant *actlrvar;
+ int cbndx = smmu_domain->cfg.cbndx;
+
+ if (qsmmu->data->actlrvar) {
+ actlrvar = qsmmu->data->actlrvar;
+ for (; actlrvar->io_start; actlrvar++) {
+ if (actlrvar->io_start == smmu->ioaddr) {
+ qcom_smmu_set_actlr(dev, smmu, cbndx,
actlrvar->actlrcfg);
+ break;
+ }
+ }
+ }
+
This block and the one in qcom_adreno_smmu_init_context() are exactly
the same. Possible to do some refactoring?
I will check if this repeated blocks can be accomodated this into
qcom_smmu_set_actlr function if that would be fine.
Also adding to this, this might increase the number of indentation
inside qcom_smmu_set_actlr as well, to around 5. So wouldn't this
be an issue?
By the way, we can refactor this:
if (qsmmu->data->actlrvar) {
actlrvar = qsmmu->data->actlrvar;
for (; actlrvar->io_start; actlrvar++) {
if (actlrvar->io_start == smmu->ioaddr) {
qcom_smmu_set_actlr(dev, smmu, cbndx, actlrvar->actlrcfg);
break;
}
}
}
into
// add const u8 num_actlrcfgs to struct actrl_variant to
// save on sentinel space:
// sizeof(u8) < sizeof(ptr) + sizeof(resource_size_t)
Git it, Would it be better to add this in struct qcom_smmu_match_data ?
Yes, right.
Actually, I noticed now, we can do both the actlr_config (num_actlrcfg
is used) and actlr_var (num_smmu is used) in the similar by storing the
number of elements in each of them.
something like this:
+static const struct actlr_config sc7280_apps_actlr_cfg[] = {
+ { 0x0800, 0x24e1, PREFETCH_DEFAULT | CMTLB },
+ { 0x2000, 0x0163, PREFETCH_DEFAULT | CMTLB },
+ { 0x2080, 0x0461, PREFETCH_DEFAULT | CMTLB },
+ { 0x2100, 0x0161, PREFETCH_DEFAULT | CMTLB },
+ { 0x0900, 0x0407, PREFETCH_SHALLOW | CPRE | CMTLB },
+ { 0x2180, 0x0027, PREFETCH_SHALLOW | CPRE | CMTLB },
+ { 0x1000, 0x07ff, PREFETCH_DEEP | CPRE | CMTLB },
+};
+
+static const struct actlr_config sc7280_gfx_actlr_cfg[] = {
+ { 0x0000, 0x07ff, PREFETCH_SWITCH_GFX | PREFETCH_DEEP | CPRE | CMTLB },
+};
+
+static const struct actlr_variant sc7280_actlr[] = {
+ { .io_start = 0x15000000, .actlrcfg = sc7280_apps_actlr_cfg,
num_actlrcfg = 7 },
+ { .io_start = 0x03da0000, .actlrcfg = sc7280_gfx_actlr_cfg,
num_actlrcfg = 1 },
+};
+
static const struct actlr_config sm8550_apps_actlr_cfg[] = {
{ 0x18a0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
{ 0x18e0, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
@@ -661,6 +680,13 @@ static const struct qcom_smmu_match_data
sdm845_smmu_500_data = {
/* Also no debug configuration. */
};
+static const struct qcom_smmu_match_data sc7280_smmu_500_impl0_data = {
+ .impl = &qcom_smmu_500_impl,
+ .adreno_impl = &qcom_adreno_smmu_500_impl,
+ .cfg = &qcom_smmu_impl0_cfg,
+ .actlrvar = sc7280_actlr,
+ .num_smmu = 2,
+};
Just for note , there's a small hiccup here as we have to manually
calculate and the number of elements in actlr_config size everytime we
add this info for a new target, won't be an issue though but just a
hindrance to automation (?)
Posted a sample below.
[declarations]
const struct actlr_variant *actlrvar = qsmmu->data->actlrvar;
int i;
[rest of the functions]
if (!actlrvar)
return 0;
> for (i = 0; i < actrlvar->num_actrlcfgs; i++) {
if (actlrvar[i].io_start == smmu->ioaddr) {
qcom_smmu_set_actlr(dev, smmu, cbndx, actlrvar->actlrcfg);
break;
}
}
> Saving both on .TEXT size and indentation levels :)
Thanks for this suggestion Konrad, will try to implement this, as it
would reduce the indent levels to good extent.
Would something like this be okay?
static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
const struct actlr_variant *actlrvar;
int cbndx = smmu_domain->cfg.cbndx;
+ int i;
+ actlrvar = qsmmu->data->actlrvar;
+
+ if (!actlrvar)
+ goto end;
+
+ for (i = 0; i < qsmmu->data->num_smmu ; i++) {
+ if (actlrvar[i].io_start == smmu->ioaddr) {
+ qcom_smmu_set_actlr(dev, smmu, cbndx,
+ actlrvar[i].actlrcfg);
+ break;
}
}
+end:
smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
If you move this assignment before the actlrvar checking (there's no
dependency between them), you will get rid of the goto.
Ack thanks, it's tlb flush operation so won't be an issue moving this
before the check.
I also noticed that qcom_smmu_match_data.actlrvar could likely be
const struct actlr_variant * const (const pointer to a const
resource), similarly for actlr_variant.actlrcfg
Sure, make sense, as we aren't aiming to modify these values later on.
Would address all these inputs in next version.
Thanks & regards,
Bibek
Konrad