Re: [PATCH v7 3/5] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.

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.

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

Konrad




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux