[v3,2/5] pinctrl: qcom: Use qcom_scm_io_update_field()
Commit Message
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(-)
Comments
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
@@ -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);
}