| Message ID | 20200313191513.11365-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: qcom: use scm_call to route GPIO irq to Apps | expand |
On Fri, Mar 13, 2020 at 8:15 PM Ansuel Smith <ansuelsmth@gmail.com> wrote: > For IPQ806x targets, TZ protects the registers that are used to > configure the routing of interrupts to a target processor. > To resolve this, this patch uses scm call to route GPIO interrupts > to application processor. Also the scm call interface is changed. > > Signed-off-by: Ajay Kishore <akisho@codeaurora.org> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> That's advanced. I'd really like Bjorn's review on this. > + ret = of_device_is_compatible(pctrl->dev->of_node, > + "qcom,ipq8064-pinctrl"); > /* Route interrupts to application cpu */ > - val = msm_readl_intr_target(pctrl, g); > - val &= ~(7 << g->intr_target_bit); > - val |= g->intr_target_kpss_val << g->intr_target_bit; > - msm_writel_intr_target(val, pctrl, g); > + if (!ret) { Can't you just: if (!of_device_is_compatible(pctrl->dev->of_node, "qcom,ipq8064-pinctrl")) { OK maybe nitpicky. > + val = msm_readl_intr_target(pctrl, g); > + val &= ~(7 << g->intr_target_bit); > + val |= g->intr_target_kpss_val << g->intr_target_bit; > + msm_writel_intr_target(val, pctrl, g); > + } else { > + const __be32 *reg = of_get_property(pctrl->dev->of_node, > + "reg", NULL); > + > + if (reg) { > + u32 addr = be32_to_cpup(reg) + g->intr_target_reg; > + > + qcom_scm_io_readl(addr, &val); > + __iormb(); > + > + val &= ~(7 << g->intr_target_bit); > + val |= g->intr_target_kpss_val << g->intr_target_bit; > + > + __iowmb(); > + ret = qcom_scm_io_writel(addr, val); > + if (ret) > + pr_err("\n Routing interrupts to Apps proc failed"); > + } Can we at least put a little comment on top here and explain what is going on? Yours, Linus Walleij
On Fri 13 Mar 12:15 PDT 2020, Ansuel Smith wrote: > For IPQ806x targets, TZ protects the registers that are used to > configure the routing of interrupts to a target processor. > To resolve this, this patch uses scm call to route GPIO interrupts > to application processor. Also the scm call interface is changed. > Oh no...but this does look pretty reasonable after all, some comments and suggestions below. > Signed-off-by: Ajay Kishore <akisho@codeaurora.org> With this --author should be Ajay. > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 36 ++++++++++++++++++++++++++---- > 1 file changed, 32 insertions(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 9a8daa256a32..a83cfd1da219 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -22,6 +22,8 @@ > #include <linux/reboot.h> > #include <linux/pm.h> > #include <linux/log2.h> > +#include <linux/qcom_scm.h> > +#include <linux/io.h> > > #include <linux/soc/qcom/irq.h> > > @@ -710,6 +712,9 @@ static void msm_gpio_irq_mask(struct irq_data *d) > const struct msm_pingroup *g; > unsigned long flags; > u32 val; > + u32 addr; > + int ret; > + const __be32 *reg; > > if (d->parent_data) > irq_chip_mask_parent(d); > @@ -863,6 +868,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > const struct msm_pingroup *g; > unsigned long flags; > u32 val; > + int ret; > > if (d->parent_data) > irq_chip_set_type_parent(d, type); > @@ -882,11 +888,33 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > else > clear_bit(d->hwirq, pctrl->dual_edge_irqs); > > + ret = of_device_is_compatible(pctrl->dev->of_node, > + "qcom,ipq8064-pinctrl"); This won't change in runtime, so I would like for you to query this during probe and then carry the result in msm_pinctrl. > /* Route interrupts to application cpu */ > - val = msm_readl_intr_target(pctrl, g); > - val &= ~(7 << g->intr_target_bit); > - val |= g->intr_target_kpss_val << g->intr_target_bit; > - msm_writel_intr_target(val, pctrl, g); > + if (!ret) { > + val = msm_readl_intr_target(pctrl, g); > + val &= ~(7 << g->intr_target_bit); > + val |= g->intr_target_kpss_val << g->intr_target_bit; > + msm_writel_intr_target(val, pctrl, g); > + } else { > + const __be32 *reg = of_get_property(pctrl->dev->of_node, > + "reg", NULL); This too will be static, and we already resolve this, properly, with address translation etc during probe. So if you do a partial rollback of '4b024225c4a8 ("pinctrl: use devm_platform_ioremap_resource() to simplify code")' you can stash the base address of the resource in msm_pinctrl, and then make your change conditional on this being NULL or not. > + > + if (reg) { > + u32 addr = be32_to_cpup(reg) + g->intr_target_reg; > + > + qcom_scm_io_readl(addr, &val); > + __iormb(); Why do you need this and below barriers? > + > + val &= ~(7 << g->intr_target_bit); > + val |= g->intr_target_kpss_val << g->intr_target_bit; > + > + __iowmb(); > + ret = qcom_scm_io_writel(addr, val); > + if (ret) > + pr_err("\n Routing interrupts to Apps proc failed"); You have msm_pinctrl->dev, so use dev_err(). And the \n goes at the end in Linux - and perhaps make it include d->hwirq to make it a little bit informative? Regards, Bjorn > + } > + } > > /* Update configuration for gpio. > * RAW_STATUS_EN is left on for all gpio irqs. Due to the > -- > 2.25.0 >
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 9a8daa256a32..a83cfd1da219 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -22,6 +22,8 @@ #include <linux/reboot.h> #include <linux/pm.h> #include <linux/log2.h> +#include <linux/qcom_scm.h> +#include <linux/io.h> #include <linux/soc/qcom/irq.h> @@ -710,6 +712,9 @@ static void msm_gpio_irq_mask(struct irq_data *d) const struct msm_pingroup *g; unsigned long flags; u32 val; + u32 addr; + int ret; + const __be32 *reg; if (d->parent_data) irq_chip_mask_parent(d); @@ -863,6 +868,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) const struct msm_pingroup *g; unsigned long flags; u32 val; + int ret; if (d->parent_data) irq_chip_set_type_parent(d, type); @@ -882,11 +888,33 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) else clear_bit(d->hwirq, pctrl->dual_edge_irqs); + ret = of_device_is_compatible(pctrl->dev->of_node, + "qcom,ipq8064-pinctrl"); /* Route interrupts to application cpu */ - val = msm_readl_intr_target(pctrl, g); - val &= ~(7 << g->intr_target_bit); - val |= g->intr_target_kpss_val << g->intr_target_bit; - msm_writel_intr_target(val, pctrl, g); + if (!ret) { + val = msm_readl_intr_target(pctrl, g); + val &= ~(7 << g->intr_target_bit); + val |= g->intr_target_kpss_val << g->intr_target_bit; + msm_writel_intr_target(val, pctrl, g); + } else { + const __be32 *reg = of_get_property(pctrl->dev->of_node, + "reg", NULL); + + if (reg) { + u32 addr = be32_to_cpup(reg) + g->intr_target_reg; + + qcom_scm_io_readl(addr, &val); + __iormb(); + + val &= ~(7 << g->intr_target_bit); + val |= g->intr_target_kpss_val << g->intr_target_bit; + + __iowmb(); + ret = qcom_scm_io_writel(addr, val); + if (ret) + pr_err("\n Routing interrupts to Apps proc failed"); + } + } /* Update configuration for gpio. * RAW_STATUS_EN is left on for all gpio irqs. Due to the