Message ID | 1679070482-8391-2-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Refactor to support multiple download mode | expand |
On 3/17/2023 9:57 PM, Mukesh Ojha wrote: > It was released by Srinivas K. that there is a need of Typo: s/released/realized -- Mukesh > read-modify-write scm exported function so that it can > be used by multiple clients. > > Let's introduce qcom_scm_io_update_field() which masks > out the bits and write the passed value to that > bit-offset. Subsequent patch will use this function. > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > drivers/firmware/qcom_scm.c | 15 +++++++++++++++ > include/linux/firmware/qcom/qcom_scm.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 3e020d1..aca2556 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id) > } > EXPORT_SYMBOL(qcom_scm_set_remote_state); > > +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val) > +{ > + unsigned int old, new; > + int ret; > + > + ret = qcom_scm_io_readl(addr, &old); > + if (ret) > + return ret; > + > + new = (old & ~mask) | val; > + > + return qcom_scm_io_writel(addr, new); > +} > +EXPORT_SYMBOL(qcom_scm_io_update_field); > + > static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) > { > struct qcom_scm_desc desc = { > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h > index 1e449a5..203a781 100644 > --- a/include/linux/firmware/qcom/qcom_scm.h > +++ b/include/linux/firmware/qcom/qcom_scm.h > @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral); > > extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val); > extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val); > +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, > + unsigned int val); > > extern bool qcom_scm_restore_sec_cfg_available(void); > extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > It was released by Srinivas K. that there is a need of > read-modify-write scm exported function so that it can > be used by multiple clients. > > Let's introduce qcom_scm_io_update_field() which masks > out the bits and write the passed value to that > bit-offset. Subsequent patch will use this function. > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> This is starting to reimplement regmap. In this case regmap_update_bits(). What about just using regmap as accessor for these registers instead? Yours, Linus Walleij
On Fri, Mar 17, 2023 at 09:56:59PM +0100, Linus Walleij wrote: > On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > > > It was released by Srinivas K. that there is a need of > > read-modify-write scm exported function so that it can > > be used by multiple clients. > > > > Let's introduce qcom_scm_io_update_field() which masks > > out the bits and write the passed value to that > > bit-offset. Subsequent patch will use this function. > > > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > This is starting to reimplement regmap. > In this case regmap_update_bits(). > > What about just using regmap as accessor for these > registers instead? > I'm not sure it would be beneficial... The regmap interface provides a standardized representation of a block of registers, with the suitable accessors backing it. But in both cases touched upon in this series, the addressed registers are part of regions already handled by the kernel. So it wouldn't be suitable to create a regmap-abstraction for "a block of secure registers", at best that would give us two kinds of regmaps abstracting the same register block. Instead I believe we'd need to extend the struct regmap_config to introduce a new table telling a new secure-or-unsecure-mmio-regmap which accessor (secure or unsecure read/write) shoudl be used, and then have e.g. pinctrl-msm register such regmap, passing the information about which registers in its memory region is secure. We'd still need qcom_scm_io_readl() and qcom_scm_io_writel() exported to implement the new custom regmap implementation - and the struct regmap_config needed in just pinctrl-msm alone would be larger than the one function it replaces. But please let me know if I'm missing something? Regards, Bjorn
On Mon, Mar 20, 2023 at 4:19 AM Bjorn Andersson <andersson@kernel.org> wrote: > > This is starting to reimplement regmap. > > In this case regmap_update_bits(). > > > > What about just using regmap as accessor for these > > registers instead? > > > > I'm not sure it would be beneficial... Me neither, I don't know the details, I just notice the similarity in the accessors. > The regmap interface provides a standardized representation of a block > of registers, with the suitable accessors backing it. But in both cases > touched upon in this series, the addressed registers are part of regions > already handled by the kernel. > > So it wouldn't be suitable to create a regmap-abstraction for "a block > of secure registers", at best that would give us two kinds of regmaps > abstracting the same register block. From my viewpoint regmap does three things: - Abstract one coherent region of registers under a shared lock, with nifty accessors (such as mask-and-set with regmap_update_bits()) - Maps access patterns/permissions and permissible access range - Optionally cache the contents The way I would use it if these secure registers are in the same range as a bunch of non-secure ones is for sharing a lock and the regmap accessors. I wouldn't worry about access patterns and such. That usecase (block access to certain registers or bits) is partly overdesign in some cases IMO. If regmap abstraction isn't helpful overall then we shouldn't do it. Yours, Linus Walleij
On 17/03/2023 16:27, Mukesh Ojha wrote: > It was released by Srinivas K. that there is a need of > read-modify-write scm exported function so that it can > be used by multiple clients. > > Let's introduce qcom_scm_io_update_field() which masks > out the bits and write the passed value to that > bit-offset. Subsequent patch will use this function. > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > drivers/firmware/qcom_scm.c | 15 +++++++++++++++ > include/linux/firmware/qcom/qcom_scm.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 3e020d1..aca2556 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id) > } > EXPORT_SYMBOL(qcom_scm_set_remote_state); > > +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val) > +{ > + unsigned int old, new; > + int ret; > + > + ret = qcom_scm_io_readl(addr, &old); > + if (ret) > + return ret; > + > + new = (old & ~mask) | val; thanks for doing this, With field semantics, val should be shifted within the function, so the caller only sets value for field rather than passing a shifted value. so this should be: new = (old & ~mask) | (val << ffs(mask) - 1); --srini > + > + return qcom_scm_io_writel(addr, new); > +} > +EXPORT_SYMBOL(qcom_scm_io_update_field); > + > static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) > { > struct qcom_scm_desc desc = { > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h > index 1e449a5..203a781 100644 > --- a/include/linux/firmware/qcom/qcom_scm.h > +++ b/include/linux/firmware/qcom/qcom_scm.h > @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral); > > extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val); > extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val); > +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, > + unsigned int val); > > extern bool qcom_scm_restore_sec_cfg_available(void); > extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c index 3e020d1..aca2556 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id) } EXPORT_SYMBOL(qcom_scm_set_remote_state); +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val) +{ + unsigned int old, new; + int ret; + + ret = qcom_scm_io_readl(addr, &old); + if (ret) + return ret; + + new = (old & ~mask) | val; + + return qcom_scm_io_writel(addr, new); +} +EXPORT_SYMBOL(qcom_scm_io_update_field); + static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) { struct qcom_scm_desc desc = { diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h index 1e449a5..203a781 100644 --- a/include/linux/firmware/qcom/qcom_scm.h +++ b/include/linux/firmware/qcom/qcom_scm.h @@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral); extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val); extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val); +extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, + unsigned int val); extern bool qcom_scm_restore_sec_cfg_available(void); extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
It was released by Srinivas K. that there is a need of read-modify-write scm exported function so that it can be used by multiple clients. Let's introduce qcom_scm_io_update_field() which masks out the bits and write the passed value to that bit-offset. Subsequent patch will use this function. Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- drivers/firmware/qcom_scm.c | 15 +++++++++++++++ include/linux/firmware/qcom/qcom_scm.h | 2 ++ 2 files changed, 17 insertions(+)