| Message ID | 1436239822-14132-6-git-send-email-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Thanks for the respin. I still have a few comments on the series, starting here; On 07/07/15 04:30, Zhen Lei wrote: > Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command, > execute it will trigger GERROR interrupt. Although the gerror code manage > to turn the prefetch into a SYNC, and the system can continue to run > normally, but it's ugly to print error information. No mention of the DT binding change, and no corresponding documentation update either. > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- [...] > +static struct arm_smmu_option_prop arm_smmu_options[] = { > + { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" }, > + { 0, NULL}, > +}; [...] > +static void parse_driver_options(struct arm_smmu_device *smmu) > +{ > + int i = 0; > + > + do { > + if (of_property_read_bool(smmu->dev->of_node, > + arm_smmu_options[i].prop)) { > + smmu->options |= arm_smmu_options[i].opt; > + dev_notice(smmu->dev, "option %s\n", > + arm_smmu_options[i].prop); > + } > + } while (arm_smmu_options[++i].opt); > +} > + Nitpicking for sure, but I'm still waiting for a good excuse to rewrite this overcomplicated loop logic in the SMMUv2 driver - can't we just treat a static array as a static array and iterate over the thing in the obvious way? for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++) Robin.
Hi Robin, FWIW, I already queued this ;) On Wed, Jul 08, 2015 at 02:03:13PM +0100, Robin Murphy wrote: > On 07/07/15 04:30, Zhen Lei wrote: > > Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command, > > execute it will trigger GERROR interrupt. Although the gerror code manage > > to turn the prefetch into a SYNC, and the system can continue to run > > normally, but it's ugly to print error information. > > No mention of the DT binding change, and no corresponding documentation > update either. I've added that. > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > --- > [...] > > +static struct arm_smmu_option_prop arm_smmu_options[] = { > > + { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" }, > > + { 0, NULL}, > > +}; > [...] > > +static void parse_driver_options(struct arm_smmu_device *smmu) > > +{ > > + int i = 0; > > + > > + do { > > + if (of_property_read_bool(smmu->dev->of_node, > > + arm_smmu_options[i].prop)) { > > + smmu->options |= arm_smmu_options[i].opt; > > + dev_notice(smmu->dev, "option %s\n", > > + arm_smmu_options[i].prop); > > + } > > + } while (arm_smmu_options[++i].opt); > > +} > > + > > Nitpicking for sure, but I'm still waiting for a good excuse to rewrite > this overcomplicated loop logic in the SMMUv2 driver - can't we just > treat a static array as a static array and iterate over the thing in the > obvious way? > > for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++) I'd rather have consistency with the other driver and I really have no personal preference about how we iterate over an array. If you have a technical reason to change both the drivers, please send a patch. Will
On 2015/7/9 1:11, Will Deacon wrote: > Hi Robin, > > FWIW, I already queued this ;) > > On Wed, Jul 08, 2015 at 02:03:13PM +0100, Robin Murphy wrote: >> On 07/07/15 04:30, Zhen Lei wrote: >>> Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command, >>> execute it will trigger GERROR interrupt. Although the gerror code manage >>> to turn the prefetch into a SYNC, and the system can continue to run >>> normally, but it's ugly to print error information. >> >> No mention of the DT binding change, and no corresponding documentation >> update either. > > I've added that. Thank you very much. You are a real gentleman. > >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>> --- >> [...] >>> +static struct arm_smmu_option_prop arm_smmu_options[] = { >>> + { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" }, >>> + { 0, NULL}, >>> +}; >> [...] >>> +static void parse_driver_options(struct arm_smmu_device *smmu) >>> +{ >>> + int i = 0; >>> + >>> + do { >>> + if (of_property_read_bool(smmu->dev->of_node, >>> + arm_smmu_options[i].prop)) { >>> + smmu->options |= arm_smmu_options[i].opt; >>> + dev_notice(smmu->dev, "option %s\n", >>> + arm_smmu_options[i].prop); >>> + } >>> + } while (arm_smmu_options[++i].opt); >>> +} >>> + >> >> Nitpicking for sure, but I'm still waiting for a good excuse to rewrite >> this overcomplicated loop logic in the SMMUv2 driver - can't we just >> treat a static array as a static array and iterate over the thing in the >> obvious way? >> >> for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++) > > I'd rather have consistency with the other driver and I really have no > personal preference about how we iterate over an array. If you have a > technical reason to change both the drivers, please send a patch. > > Will > > . >
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 4e7fd4e..d6e3494 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -543,6 +543,9 @@ struct arm_smmu_device { #define ARM_SMMU_FEAT_HYP (1 << 12) u32 features; +#define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0) + u32 options; + struct arm_smmu_cmdq cmdq; struct arm_smmu_evtq evtq; struct arm_smmu_priq priq; @@ -603,11 +606,35 @@ struct arm_smmu_domain { static DEFINE_SPINLOCK(arm_smmu_devices_lock); static LIST_HEAD(arm_smmu_devices); +struct arm_smmu_option_prop { + u32 opt; + const char *prop; +}; + +static struct arm_smmu_option_prop arm_smmu_options[] = { + { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" }, + { 0, NULL}, +}; + static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) { return container_of(dom, struct arm_smmu_domain, domain); } +static void parse_driver_options(struct arm_smmu_device *smmu) +{ + int i = 0; + + do { + if (of_property_read_bool(smmu->dev->of_node, + arm_smmu_options[i].prop)) { + smmu->options |= arm_smmu_options[i].opt; + dev_notice(smmu->dev, "option %s\n", + arm_smmu_options[i].prop); + } + } while (arm_smmu_options[++i].opt); +} + /* Low-level queue manipulation functions */ static bool queue_full(struct arm_smmu_queue *q) { @@ -1037,7 +1064,8 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, arm_smmu_sync_ste_for_sid(smmu, sid); /* It's likely that we'll want to use the new STE soon */ - arm_smmu_cmdq_issue_cmd(smmu, &prefetch_cmd); + if (!(smmu->options & ARM_SMMU_OPT_SKIP_PREFETCH)) + arm_smmu_cmdq_issue_cmd(smmu, &prefetch_cmd); } static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent) @@ -2576,6 +2604,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) if (irq > 0) smmu->gerr_irq = irq; + parse_driver_options(smmu); + /* Probe the h/w */ ret = arm_smmu_device_probe(smmu); if (ret)
Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command, execute it will trigger GERROR interrupt. Although the gerror code manage to turn the prefetch into a SYNC, and the system can continue to run normally, but it's ugly to print error information. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- drivers/iommu/arm-smmu-v3.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) -- 1.8.0