| Message ID | 1704727654-13999-2-git-send-email-quic_mojha@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc SCM driver changes | expand |
On Mon, Jan 08, 2024 at 08:57:31PM +0530, Mukesh Ojha wrote: > It was realized 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_rmw() which masks out the bits > and write the passed value to that bit-offset. > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332 > --- > drivers/firmware/qcom/qcom_scm.c | 26 ++++++++++++++++++++++++++ > include/linux/firmware/qcom/qcom_scm.h | 1 + > 2 files changed, 27 insertions(+) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 520de9b5633a..25549178a30f 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -19,6 +19,7 @@ > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/spinlock.h> > #include <linux/reset-controller.h> > #include <linux/types.h> > > @@ -41,6 +42,8 @@ struct qcom_scm { > int scm_vote_count; > > u64 dload_mode_addr; > + /* Atomic context only */ > + spinlock_t lock; IMHO, this comment can be confusing later. one might think that qcom_scm_call_atomic() needs to be called with this lock, but that does not seems to be the intention here. > }; > > struct qcom_scm_current_perm_info { > @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void) > return ret ? : res.result[0]; > } > > +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) > +{ > + unsigned int old, new; > + int ret; > + > + if (!__scm) > + return -EINVAL; > + > + spin_lock(&__scm->lock); So, this function can't be called from hardirq context. If that ever happens, with this new spinlock (without disabling interrupts), can result in deadlock. > + ret = qcom_scm_io_readl(addr, &old); > + if (ret) > + goto unlock; > + > + new = (old & ~mask) | (val & mask); > + > + ret = qcom_scm_io_writel(addr, new); > +unlock: > + spin_unlock(&__scm->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); Thanks, Pavan
On 1/9/2024 10:34 AM, Pavan Kondeti wrote: > On Mon, Jan 08, 2024 at 08:57:31PM +0530, Mukesh Ojha wrote: >> It was realized 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_rmw() which masks out the bits >> and write the passed value to that bit-offset. >> >> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332 >> --- >> drivers/firmware/qcom/qcom_scm.c | 26 ++++++++++++++++++++++++++ >> include/linux/firmware/qcom/qcom_scm.h | 1 + >> 2 files changed, 27 insertions(+) >> >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c >> index 520de9b5633a..25549178a30f 100644 >> --- a/drivers/firmware/qcom/qcom_scm.c >> +++ b/drivers/firmware/qcom/qcom_scm.c >> @@ -19,6 +19,7 @@ >> #include <linux/of_irq.h> >> #include <linux/of_platform.h> >> #include <linux/platform_device.h> >> +#include <linux/spinlock.h> >> #include <linux/reset-controller.h> >> #include <linux/types.h> >> >> @@ -41,6 +42,8 @@ struct qcom_scm { >> int scm_vote_count; >> >> u64 dload_mode_addr; >> + /* Atomic context only */ >> + spinlock_t lock; > > IMHO, this comment can be confusing later. one might think that > qcom_scm_call_atomic() needs to be called with this lock, but that does > not seems to be the intention here. > >> }; >> >> struct qcom_scm_current_perm_info { >> @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void) >> return ret ? : res.result[0]; >> } >> >> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) >> +{ >> + unsigned int old, new; >> + int ret; >> + >> + if (!__scm) >> + return -EINVAL; >> + >> + spin_lock(&__scm->lock); > > So, this function can't be called from hardirq context. If that ever > happens, with this new spinlock (without disabling interrupts), can > result in deadlock. Ok, let's make it fully atomic with spin_lock_irqsave(); -Mukesh > >> + ret = qcom_scm_io_readl(addr, &old); >> + if (ret) >> + goto unlock; >> + >> + new = (old & ~mask) | (val & mask); >> + >> + ret = qcom_scm_io_writel(addr, new); >> +unlock: >> + spin_unlock(&__scm->lock); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); > > Thanks, > Pavan
On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > It was realized 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_rmw() which masks out the bits > and write the passed value to that bit-offset. (...) > +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) > +{ > + unsigned int old, new; > + int ret; > + > + if (!__scm) > + return -EINVAL; > + > + spin_lock(&__scm->lock); > + ret = qcom_scm_io_readl(addr, &old); > + if (ret) > + goto unlock; > + > + new = (old & ~mask) | (val & mask); > + > + ret = qcom_scm_io_writel(addr, new); > +unlock: > + spin_unlock(&__scm->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); This looks a lot like you are starting to re-invent regmaps regmap_update_bits(). If you are starting to realize you need more and more of regmap, why not use regmap and its functions? Yours, Linus Walleij
On 1/9/2024 6:44 PM, Linus Walleij wrote: > On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > >> It was realized 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_rmw() which masks out the bits >> and write the passed value to that bit-offset. > (...) >> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) >> +{ >> + unsigned int old, new; >> + int ret; >> + >> + if (!__scm) >> + return -EINVAL; >> + >> + spin_lock(&__scm->lock); >> + ret = qcom_scm_io_readl(addr, &old); >> + if (ret) >> + goto unlock; >> + >> + new = (old & ~mask) | (val & mask); >> + >> + ret = qcom_scm_io_writel(addr, new); >> +unlock: >> + spin_unlock(&__scm->lock); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); > > This looks a lot like you are starting to re-invent regmaps > regmap_update_bits(). > > If you are starting to realize you need more and more of > regmap, why not use regmap and its functions? I think, this discussion has happened already .. https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/ -Mukesh > > Yours, > Linus Walleij
On Tue, Jan 9, 2024 at 2:24 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > On 1/9/2024 6:44 PM, Linus Walleij wrote: > > On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > > > >> It was realized 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_rmw() which masks out the bits > >> and write the passed value to that bit-offset. > > (...) > >> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) > >> +{ > >> + unsigned int old, new; > >> + int ret; > >> + > >> + if (!__scm) > >> + return -EINVAL; > >> + > >> + spin_lock(&__scm->lock); > >> + ret = qcom_scm_io_readl(addr, &old); > >> + if (ret) > >> + goto unlock; > >> + > >> + new = (old & ~mask) | (val & mask); > >> + > >> + ret = qcom_scm_io_writel(addr, new); > >> +unlock: > >> + spin_unlock(&__scm->lock); > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); > > > > This looks a lot like you are starting to re-invent regmaps > > regmap_update_bits(). > > > > If you are starting to realize you need more and more of > > regmap, why not use regmap and its functions? > > I think, this discussion has happened already .. > > https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/ That discussion ended with: [Bjorn] > 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. When you add more and more accessors the premise starts to change, and it becomes more and more of a reimplementation. It may be time to actually fix this. Yours, Linus Walleij
On Tue, Jan 09, 2024 at 02:34:10PM +0100, Linus Walleij wrote: > On Tue, Jan 9, 2024 at 2:24 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > > On 1/9/2024 6:44 PM, Linus Walleij wrote: > > > On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > > > > > >> It was realized 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_rmw() which masks out the bits > > >> and write the passed value to that bit-offset. > > > (...) > > >> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) > > >> +{ > > >> + unsigned int old, new; > > >> + int ret; > > >> + > > >> + if (!__scm) > > >> + return -EINVAL; > > >> + > > >> + spin_lock(&__scm->lock); > > >> + ret = qcom_scm_io_readl(addr, &old); > > >> + if (ret) > > >> + goto unlock; > > >> + > > >> + new = (old & ~mask) | (val & mask); > > >> + > > >> + ret = qcom_scm_io_writel(addr, new); > > >> +unlock: > > >> + spin_unlock(&__scm->lock); > > >> + return ret; > > >> +} > > >> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); > > > > > > This looks a lot like you are starting to re-invent regmaps > > > regmap_update_bits(). > > > > > > If you are starting to realize you need more and more of > > > regmap, why not use regmap and its functions? > > > > I think, this discussion has happened already .. > > > > https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/ > > That discussion ended with: > > [Bjorn] > > 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. > > When you add more and more accessors the premise starts to > change, and it becomes more and more of a reimplementation. > > It may be time to actually fix this. > Thought I had replied to this already, did we discuss this previously as well? My concern with expressing this as a regmap is that from the provider's point of view, the regmap would span the entire 32-bit address space. I'm guessing that there's something on the other side limiting what subregions are actually accessible for each platform/firmware configuration, but I'm not convinced that regmap a good abstraction... Regards, Bjorn > Yours, > Linus Walleij
On Mon, Jan 08, 2024 at 08:57:31PM +0530, Mukesh Ojha wrote: > It was realized by Srinivas K. that there is a need of "need" is a strong word for this functionality, unless there's some use case that I'm missing. > read-modify-write scm exported function so that it can > be used by multiple clients. > > Let's introduce qcom_scm_io_rmw() which masks out the bits > and write the passed value to that bit-offset. > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com> # IPQ9574 and IPQ5332 > --- > drivers/firmware/qcom/qcom_scm.c | 26 ++++++++++++++++++++++++++ > include/linux/firmware/qcom/qcom_scm.h | 1 + > 2 files changed, 27 insertions(+) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 520de9b5633a..25549178a30f 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -19,6 +19,7 @@ > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/spinlock.h> > #include <linux/reset-controller.h> > #include <linux/types.h> > > @@ -41,6 +42,8 @@ struct qcom_scm { > int scm_vote_count; > > u64 dload_mode_addr; > + /* Atomic context only */ > + spinlock_t lock; > }; > > struct qcom_scm_current_perm_info { > @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void) > return ret ? : res.result[0]; > } > > +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) > +{ > + unsigned int old, new; > + int ret; > + > + if (!__scm) > + return -EINVAL; > + > + spin_lock(&__scm->lock); Please express that this lock is just for create mutual exclusion between rmw operations, nothing else. Also please make a statement why this is desirable and/or needed. Regards, Bjorn > + ret = qcom_scm_io_readl(addr, &old); > + if (ret) > + goto unlock; > + > + new = (old & ~mask) | (val & mask); > + > + ret = qcom_scm_io_writel(addr, new); > +unlock: > + spin_unlock(&__scm->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); > + > static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) > { > struct qcom_scm_desc desc = { > @@ -1824,6 +1849,7 @@ static int qcom_scm_probe(struct platform_device *pdev) > return ret; > > mutex_init(&scm->scm_bw_lock); > + spin_lock_init(&scm->lock); > > scm->path = devm_of_icc_get(&pdev->dev, NULL); > if (IS_ERR(scm->path)) > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h > index ccaf28846054..3a8bb2e603b3 100644 > --- a/include/linux/firmware/qcom/qcom_scm.h > +++ b/include/linux/firmware/qcom/qcom_scm.h > @@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral); > > int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val); > int qcom_scm_io_writel(phys_addr_t addr, unsigned int val); > +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val); > > bool qcom_scm_restore_sec_cfg_available(void); > int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare); > -- > 2.7.4 >
On 2/17/2024 12:01 AM, Bjorn Andersson wrote: > On Tue, Jan 09, 2024 at 02:34:10PM +0100, Linus Walleij wrote: >> On Tue, Jan 9, 2024 at 2:24 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: >>> On 1/9/2024 6:44 PM, Linus Walleij wrote: >>>> On Mon, Jan 8, 2024 at 4:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: >>>> >>>>> It was realized 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_rmw() which masks out the bits >>>>> and write the passed value to that bit-offset. >>>> (...) >>>>> +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) >>>>> +{ >>>>> + unsigned int old, new; >>>>> + int ret; >>>>> + >>>>> + if (!__scm) >>>>> + return -EINVAL; >>>>> + >>>>> + spin_lock(&__scm->lock); >>>>> + ret = qcom_scm_io_readl(addr, &old); >>>>> + if (ret) >>>>> + goto unlock; >>>>> + >>>>> + new = (old & ~mask) | (val & mask); >>>>> + >>>>> + ret = qcom_scm_io_writel(addr, new); >>>>> +unlock: >>>>> + spin_unlock(&__scm->lock); >>>>> + return ret; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); >>>> >>>> This looks a lot like you are starting to re-invent regmaps >>>> regmap_update_bits(). >>>> >>>> If you are starting to realize you need more and more of >>>> regmap, why not use regmap and its functions? >>> >>> I think, this discussion has happened already .. >>> >>> https://lore.kernel.org/lkml/CACRpkdb95V5GC81w8fiuLfx_V1DtWYpO33FOfMnArpJeC9SDQA@mail.gmail.com/ >> >> That discussion ended with: >> >> [Bjorn] >>> 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. >> >> When you add more and more accessors the premise starts to >> change, and it becomes more and more of a reimplementation. >> >> It may be time to actually fix this. >> > > Thought I had replied to this already, did we discuss this previously as > well? > > My concern with expressing this as a regmap is that from the provider's > point of view, the regmap would span the entire 32-bit address space. > I'm guessing that there's something on the other side limiting what > subregions are actually accessible for each platform/firmware > configuration, but I'm not convinced that regmap a good abstraction... To add more to it, in current series, we are just accessing single register for read/write and using regmap for this looks overkill to me. -Mukesh > > Regards, > Bjorn > >> Yours, >> Linus Walleij
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 520de9b5633a..25549178a30f 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -19,6 +19,7 @@ #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/spinlock.h> #include <linux/reset-controller.h> #include <linux/types.h> @@ -41,6 +42,8 @@ struct qcom_scm { int scm_vote_count; u64 dload_mode_addr; + /* Atomic context only */ + spinlock_t lock; }; struct qcom_scm_current_perm_info { @@ -481,6 +484,28 @@ static int qcom_scm_disable_sdi(void) return ret ? : res.result[0]; } +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val) +{ + unsigned int old, new; + int ret; + + if (!__scm) + return -EINVAL; + + spin_lock(&__scm->lock); + ret = qcom_scm_io_readl(addr, &old); + if (ret) + goto unlock; + + new = (old & ~mask) | (val & mask); + + ret = qcom_scm_io_writel(addr, new); +unlock: + spin_unlock(&__scm->lock); + return ret; +} +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw); + static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) { struct qcom_scm_desc desc = { @@ -1824,6 +1849,7 @@ static int qcom_scm_probe(struct platform_device *pdev) return ret; mutex_init(&scm->scm_bw_lock); + spin_lock_init(&scm->lock); scm->path = devm_of_icc_get(&pdev->dev, NULL); if (IS_ERR(scm->path)) diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h index ccaf28846054..3a8bb2e603b3 100644 --- a/include/linux/firmware/qcom/qcom_scm.h +++ b/include/linux/firmware/qcom/qcom_scm.h @@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral); int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val); int qcom_scm_io_writel(phys_addr_t addr, unsigned int val); +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val); bool qcom_scm_restore_sec_cfg_available(void); int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);