RE: [PATCH 2/3] iommu/ipmmu-vmsa: Calculate context registers' offset instead of a macro
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Friday, October 11, 2019 9:29 PM > > Hi Shimoda-san, > > On Wed, Oct 9, 2019 at 10:27 AM Yoshihiro Shimoda > wrote: > > Since we will have changed memory mapping of the IPMMU in the future, > > this patch uses ipmmu_features values instead of a macro to > > calculate context registers offset. No behavior change. > > > > Signed-off-by: Yoshihiro Shimoda > > Thanks for your patch! > > > --- a/drivers/iommu/ipmmu-vmsa.c > > +++ b/drivers/iommu/ipmmu-vmsa.c > > @@ -50,6 +50,8 @@ struct ipmmu_features { > > bool twobit_imttbcr_sl0; > > bool reserved_context; > > bool cache_snoop; > > + u32 ctx_offset_base; > > + u32 ctx_offset_stride; > > }; > > > > struct ipmmu_vmsa_device { > > @@ -99,8 +101,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device > > *dev) > > > > #define IM_NS_ALIAS_OFFSET 0x800 > > > > -#define IM_CTX_SIZE0x40 > > - > > #define IMCTR 0x > > #define IMCTR_TRE (1 << 17) > > #define IMCTR_AFE (1 << 16) > > @@ -253,18 +253,25 @@ static void ipmmu_write(struct ipmmu_vmsa_device > > *mmu, unsigned int offset, > > iowrite32(data, mmu->base + offset); > > } > > > > +static u32 ipmmu_ctx_reg(struct ipmmu_vmsa_device *mmu, unsigned int > > context_id, > > +unsigned int reg) > > +{ > > + return mmu->features->ctx_offset_base + > > + context_id * mmu->features->ctx_offset_stride + reg; > > +} > > + > > static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain, > >unsigned int reg) > > { > > return ipmmu_read(domain->mmu->root, > > - domain->context_id * IM_CTX_SIZE + reg); > > + ipmmu_ctx_reg(domain->mmu, domain->context_id, > > reg)); > > For consistency: > > ipmmu_ctx_reg(domain->mmu->root, ...) > > but in practice the features for domain->mmu and domain->mmu->root are > identical anyway. > > > } > > > > static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain, > > unsigned int reg, u32 data) > > { > > ipmmu_write(domain->mmu->root, > > - domain->context_id * IM_CTX_SIZE + reg, data); > > + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), > > data); > > Likewise: > > ipmmu_ctx_reg(domain->mmu->root, ...)? Thank you for the comments! Yes, we can use domain->mmu->root to ipmmu_ctx_reg() because ipmmu_ctx_reg() only use mmu->features. > I find these ipmmu_{read,write}() a bit hard too read, with passing the > mmu to both ipmmu_{read,write}() and ipmmu_ctx_reg(). I completely agree. > What do you think about providing two helpers ipmmu_ctx_{read,write}(), > so all users can just use e.g. > > ipmmu_ctx_write(mmu, context_id, reg, data); > > instead of > > ipmmu_write(mmu, ipmmu_ctx_reg(mmu, context_id, reg), data); > > ? I think so. I'll fix it. Perhaps, I'll make a patch which changes the function name at first. Best regards, Yoshihiro Shimoda > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > ge...@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds
Re: [PATCH 2/3] iommu/ipmmu-vmsa: Calculate context registers' offset instead of a macro
Hi Shimoda-san, On Wed, Oct 9, 2019 at 10:27 AM Yoshihiro Shimoda wrote: > Since we will have changed memory mapping of the IPMMU in the future, > this patch uses ipmmu_features values instead of a macro to > calculate context registers offset. No behavior change. > > Signed-off-by: Yoshihiro Shimoda Thanks for your patch! > --- a/drivers/iommu/ipmmu-vmsa.c > +++ b/drivers/iommu/ipmmu-vmsa.c > @@ -50,6 +50,8 @@ struct ipmmu_features { > bool twobit_imttbcr_sl0; > bool reserved_context; > bool cache_snoop; > + u32 ctx_offset_base; > + u32 ctx_offset_stride; > }; > > struct ipmmu_vmsa_device { > @@ -99,8 +101,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device > *dev) > > #define IM_NS_ALIAS_OFFSET 0x800 > > -#define IM_CTX_SIZE0x40 > - > #define IMCTR 0x > #define IMCTR_TRE (1 << 17) > #define IMCTR_AFE (1 << 16) > @@ -253,18 +253,25 @@ static void ipmmu_write(struct ipmmu_vmsa_device *mmu, > unsigned int offset, > iowrite32(data, mmu->base + offset); > } > > +static u32 ipmmu_ctx_reg(struct ipmmu_vmsa_device *mmu, unsigned int > context_id, > +unsigned int reg) > +{ > + return mmu->features->ctx_offset_base + > + context_id * mmu->features->ctx_offset_stride + reg; > +} > + > static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain, >unsigned int reg) > { > return ipmmu_read(domain->mmu->root, > - domain->context_id * IM_CTX_SIZE + reg); > + ipmmu_ctx_reg(domain->mmu, domain->context_id, > reg)); For consistency: ipmmu_ctx_reg(domain->mmu->root, ...) but in practice the features for domain->mmu and domain->mmu->root are identical anyway. > } > > static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain, > unsigned int reg, u32 data) > { > ipmmu_write(domain->mmu->root, > - domain->context_id * IM_CTX_SIZE + reg, data); > + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), > data); Likewise: ipmmu_ctx_reg(domain->mmu->root, ...)? I find these ipmmu_{read,write}() a bit hard too read, with passing the mmu to both ipmmu_{read,write}() and ipmmu_ctx_reg(). What do you think about providing two helpers ipmmu_ctx_{read,write}(), so all users can just use e.g. ipmmu_ctx_write(mmu, context_id, reg, data); instead of ipmmu_write(mmu, ipmmu_ctx_reg(mmu, context_id, reg), data); ? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH 2/3] iommu/ipmmu-vmsa: Calculate context registers' offset instead of a macro
Hi Shimoda-san, Thanks for your patch. On 2019-10-09 17:26:48 +0900, Yoshihiro Shimoda wrote: > Since we will have changed memory mapping of the IPMMU in the future, > this patch uses ipmmu_features values instead of a macro to > calculate context registers offset. No behavior change. > > Signed-off-by: Yoshihiro Shimoda Reviewed-by: Niklas Söderlund > --- > drivers/iommu/ipmmu-vmsa.c | 27 +++ > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c > index dd554c2..76fb250 100644 > --- a/drivers/iommu/ipmmu-vmsa.c > +++ b/drivers/iommu/ipmmu-vmsa.c > @@ -50,6 +50,8 @@ struct ipmmu_features { > bool twobit_imttbcr_sl0; > bool reserved_context; > bool cache_snoop; > + u32 ctx_offset_base; > + u32 ctx_offset_stride; > }; > > struct ipmmu_vmsa_device { > @@ -99,8 +101,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device > *dev) > > #define IM_NS_ALIAS_OFFSET 0x800 > > -#define IM_CTX_SIZE 0x40 > - > #define IMCTR0x > #define IMCTR_TRE(1 << 17) > #define IMCTR_AFE(1 << 16) > @@ -253,18 +253,25 @@ static void ipmmu_write(struct ipmmu_vmsa_device *mmu, > unsigned int offset, > iowrite32(data, mmu->base + offset); > } > > +static u32 ipmmu_ctx_reg(struct ipmmu_vmsa_device *mmu, unsigned int > context_id, > + unsigned int reg) > +{ > + return mmu->features->ctx_offset_base + > +context_id * mmu->features->ctx_offset_stride + reg; > +} > + > static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain, > unsigned int reg) > { > return ipmmu_read(domain->mmu->root, > - domain->context_id * IM_CTX_SIZE + reg); > + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg)); > } > > static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain, >unsigned int reg, u32 data) > { > ipmmu_write(domain->mmu->root, > - domain->context_id * IM_CTX_SIZE + reg, data); > + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), data); > } > > static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain, > @@ -272,10 +279,10 @@ static void ipmmu_ctx_write_all(struct > ipmmu_vmsa_domain *domain, > { > if (domain->mmu != domain->mmu->root) > ipmmu_write(domain->mmu, > - domain->context_id * IM_CTX_SIZE + reg, data); > + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), > + data); > > - ipmmu_write(domain->mmu->root, > - domain->context_id * IM_CTX_SIZE + reg, data); > + ipmmu_ctx_write_root(domain, reg, data); > } > > /* > - > @@ -974,7 +981,7 @@ static void ipmmu_device_reset(struct ipmmu_vmsa_device > *mmu) > > /* Disable all contexts. */ > for (i = 0; i < mmu->num_ctx; ++i) > - ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0); > + ipmmu_write(mmu, ipmmu_ctx_reg(mmu, i, IMCTR), 0); > } > > static const struct ipmmu_features ipmmu_features_default = { > @@ -986,6 +993,8 @@ static const struct ipmmu_features ipmmu_features_default > = { > .twobit_imttbcr_sl0 = false, > .reserved_context = false, > .cache_snoop = true, > + .ctx_offset_base = 0, > + .ctx_offset_stride = 0x40, > }; > > static const struct ipmmu_features ipmmu_features_rcar_gen3 = { > @@ -997,6 +1006,8 @@ static const struct ipmmu_features > ipmmu_features_rcar_gen3 = { > .twobit_imttbcr_sl0 = true, > .reserved_context = true, > .cache_snoop = false, > + .ctx_offset_base = 0, > + .ctx_offset_stride = 0x40, > }; > > static const struct of_device_id ipmmu_of_ids[] = { > -- > 2.7.4 > -- Regards, Niklas Söderlund
[PATCH 2/3] iommu/ipmmu-vmsa: Calculate context registers' offset instead of a macro
Since we will have changed memory mapping of the IPMMU in the future, this patch uses ipmmu_features values instead of a macro to calculate context registers offset. No behavior change. Signed-off-by: Yoshihiro Shimoda --- drivers/iommu/ipmmu-vmsa.c | 27 +++ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c index dd554c2..76fb250 100644 --- a/drivers/iommu/ipmmu-vmsa.c +++ b/drivers/iommu/ipmmu-vmsa.c @@ -50,6 +50,8 @@ struct ipmmu_features { bool twobit_imttbcr_sl0; bool reserved_context; bool cache_snoop; + u32 ctx_offset_base; + u32 ctx_offset_stride; }; struct ipmmu_vmsa_device { @@ -99,8 +101,6 @@ static struct ipmmu_vmsa_device *to_ipmmu(struct device *dev) #define IM_NS_ALIAS_OFFSET 0x800 -#define IM_CTX_SIZE0x40 - #define IMCTR 0x #define IMCTR_TRE (1 << 17) #define IMCTR_AFE (1 << 16) @@ -253,18 +253,25 @@ static void ipmmu_write(struct ipmmu_vmsa_device *mmu, unsigned int offset, iowrite32(data, mmu->base + offset); } +static u32 ipmmu_ctx_reg(struct ipmmu_vmsa_device *mmu, unsigned int context_id, +unsigned int reg) +{ + return mmu->features->ctx_offset_base + + context_id * mmu->features->ctx_offset_stride + reg; +} + static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain, unsigned int reg) { return ipmmu_read(domain->mmu->root, - domain->context_id * IM_CTX_SIZE + reg); + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg)); } static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain, unsigned int reg, u32 data) { ipmmu_write(domain->mmu->root, - domain->context_id * IM_CTX_SIZE + reg, data); + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), data); } static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain, @@ -272,10 +279,10 @@ static void ipmmu_ctx_write_all(struct ipmmu_vmsa_domain *domain, { if (domain->mmu != domain->mmu->root) ipmmu_write(domain->mmu, - domain->context_id * IM_CTX_SIZE + reg, data); + ipmmu_ctx_reg(domain->mmu, domain->context_id, reg), + data); - ipmmu_write(domain->mmu->root, - domain->context_id * IM_CTX_SIZE + reg, data); + ipmmu_ctx_write_root(domain, reg, data); } /* - @@ -974,7 +981,7 @@ static void ipmmu_device_reset(struct ipmmu_vmsa_device *mmu) /* Disable all contexts. */ for (i = 0; i < mmu->num_ctx; ++i) - ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0); + ipmmu_write(mmu, ipmmu_ctx_reg(mmu, i, IMCTR), 0); } static const struct ipmmu_features ipmmu_features_default = { @@ -986,6 +993,8 @@ static const struct ipmmu_features ipmmu_features_default = { .twobit_imttbcr_sl0 = false, .reserved_context = false, .cache_snoop = true, + .ctx_offset_base = 0, + .ctx_offset_stride = 0x40, }; static const struct ipmmu_features ipmmu_features_rcar_gen3 = { @@ -997,6 +1006,8 @@ static const struct ipmmu_features ipmmu_features_rcar_gen3 = { .twobit_imttbcr_sl0 = true, .reserved_context = true, .cache_snoop = false, + .ctx_offset_base = 0, + .ctx_offset_stride = 0x40, }; static const struct of_device_id ipmmu_of_ids[] = { -- 2.7.4