| Message ID | 1679070482-8391-3-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refactor to support multiple download mode | expand |
On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > Use qcom_scm_io_update_field() exported function introduced > in last commit. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> Fine by me, but I want you to first consider switching the custom register accessors to regmap. Yours, Linus Walleij
On Fri, Mar 17, 2023 at 09:58:04PM +0100, Linus Walleij wrote: > On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > > > Use qcom_scm_io_update_field() exported function introduced > > in last commit. > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > Fine by me, but I want you to first consider switching the > custom register accessors to regmap. > I took a quick look at it and there seem to be two ways that it can be done. We can retain the MSM_ACCESSOR() macros that generates the custom register accessors, but plug in a regmap between these accessors and the mmio operations. But this just adds a few extra hops inbetween the driver and the volatile read/write, with a slight increase of memory, without any obvious benefits. The more alluring alternative is to replace the custom accessors with reg_fields. This would allow us to replace some (perhaps many) of the bit-manipulation with regmap_update_bits(). But at minimum we'd need one reg_field per register, per pin, so that's 5 reg_fields per pin which adds up to ~10-24kb extra space, depending on platform. Even more alluring would be to have reg_fields describing the actual fields in the registers, which would allow us to better utilize the regmap API directly. This would cost us 35-75kb of heap. IMHO this is quite a significant effort, and given that the driver seems to be doing its job I'd rather see such efforts being focused elsewhere. Regards, Bjorn
On Mon, Mar 20, 2023 at 5:07 AM Bjorn Andersson <andersson@kernel.org> wrote: > > Fine by me, but I want you to first consider switching the > > custom register accessors to regmap. (...) > IMHO this is quite a significant effort, and given that the driver seems > to be doing its job I'd rather see such efforts being focused elsewhere. I think you know it better than me, if regmap is just going to clutter the view the don't do it. Regmap does have the upside of looking the same on all platforms so it would potentially give less maintenance burden. Acked-by: Linus Walleij <linus.walleij@linaro.org> for these patches if you need to merge them elsewhere, I can also queue them if you ACK them. Yours, Linus Walleij
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index daeb79a..3d3d520 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -1016,6 +1016,8 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) unsigned long flags; bool was_enabled; u32 val; + u32 tmp_val; + u32 mask; if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) { set_bit(d->hwirq, pctrl->dual_edge_irqs); @@ -1049,24 +1051,21 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) * With intr_target_use_scm interrupts are routed to * application cpu using scm calls. */ + mask = (7 << g->intr_target_bit); + tmp_val = g->intr_target_kpss_val << g->intr_target_bit; if (pctrl->intr_target_use_scm) { u32 addr = pctrl->phys_base[0] + g->intr_target_reg; int ret; - qcom_scm_io_readl(addr, &val); - - val &= ~(7 << g->intr_target_bit); - val |= g->intr_target_kpss_val << g->intr_target_bit; - - ret = qcom_scm_io_writel(addr, val); + ret = qcom_scm_io_update_field(addr, mask, tmp_val); if (ret) dev_err(pctrl->dev, "Failed routing %lu interrupt to Apps proc", d->hwirq); } else { val = msm_readl_intr_target(pctrl, g); - val &= ~(7 << g->intr_target_bit); - val |= g->intr_target_kpss_val << g->intr_target_bit; + val &= ~mask; + val |= tmp_val; msm_writel_intr_target(val, pctrl, g); }
Use qcom_scm_io_update_field() exported function introduced in last commit. Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- drivers/pinctrl/qcom/pinctrl-msm.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)