| Message ID | 1628830531-14648-2-git-send-email-skakit@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Vol+ support for sc7280-idp | expand |
Hi Linus, On 2021-08-13 14:27, Linus Walleij wrote: > Hi Satya/David, > > nice work on identifying this bug! > > On Fri, Aug 13, 2021 at 6:56 AM satya priya <skakit@codeaurora.org> > wrote: >> >> From: David Collins <collinsd@codeaurora.org> >> >> pmic_gpio_child_to_parent_hwirq() and >> gpiochip_populate_parent_fwspec_fourcell() translate a pinctrl- >> spmi-gpio irqspec to an SPMI controller irqspec. When they do >> this, they use a fixed SPMI slave ID of 0 and a fixed GPIO >> peripheral offset of 0xC0 (corresponding to SPMI address 0xC000). >> This translation results in an incorrect irqspec for secondary >> PMICs that don't have a slave ID of 0 as well as for PMIC chips >> which have GPIO peripherals located at a base address other than >> 0xC000. >> >> Correct this issue by passing the slave ID of the pinctrl-spmi- >> gpio device's parent in the SPMI controller irqspec and by >> calculating the peripheral ID base from the device tree 'reg' >> property of the pinctrl-spmi-gpio device. >> >> Signed-off-by: David Collins <collinsd@codeaurora.org> >> Signed-off-by: satya priya <skakit@codeaurora.org> > > Is this a regression or is it fine if I just apply it for v5.15? > I was thinking v5.15 since it isn't yet used in device trees. > Without this fix, [2/2] Vol+ support is failing. If possible please merge it on current branch. > Yours, > Linus Walleij
Quoting skakit@codeaurora.org (2021-08-15 23:50:37) > Hi Linus, > > On 2021-08-13 14:27, Linus Walleij wrote: > > Hi Satya/David, > > > > nice work on identifying this bug! > > > > On Fri, Aug 13, 2021 at 6:56 AM satya priya <skakit@codeaurora.org> > > wrote: > >> > >> From: David Collins <collinsd@codeaurora.org> > >> > >> pmic_gpio_child_to_parent_hwirq() and > >> gpiochip_populate_parent_fwspec_fourcell() translate a pinctrl- > >> spmi-gpio irqspec to an SPMI controller irqspec. When they do > >> this, they use a fixed SPMI slave ID of 0 and a fixed GPIO > >> peripheral offset of 0xC0 (corresponding to SPMI address 0xC000). > >> This translation results in an incorrect irqspec for secondary > >> PMICs that don't have a slave ID of 0 as well as for PMIC chips > >> which have GPIO peripherals located at a base address other than > >> 0xC000. > >> > >> Correct this issue by passing the slave ID of the pinctrl-spmi- > >> gpio device's parent in the SPMI controller irqspec and by > >> calculating the peripheral ID base from the device tree 'reg' > >> property of the pinctrl-spmi-gpio device. > >> > >> Signed-off-by: David Collins <collinsd@codeaurora.org> > >> Signed-off-by: satya priya <skakit@codeaurora.org> Can you please add an appropriate Fixes tag? > > > > Is this a regression or is it fine if I just apply it for v5.15? > > I was thinking v5.15 since it isn't yet used in device trees. > > > > Without this fix, [2/2] Vol+ support is failing. If possible please > merge it on current branch. > Are there any boards supported upstream that have a gpio block that isn't at 0xc000?
On 2021-08-17 02:38, Stephen Boyd wrote: > Quoting skakit@codeaurora.org (2021-08-15 23:50:37) >> Hi Linus, >> >> On 2021-08-13 14:27, Linus Walleij wrote: >> > Hi Satya/David, >> > >> > nice work on identifying this bug! >> > >> > On Fri, Aug 13, 2021 at 6:56 AM satya priya <skakit@codeaurora.org> >> > wrote: >> >> >> >> From: David Collins <collinsd@codeaurora.org> >> >> >> >> pmic_gpio_child_to_parent_hwirq() and >> >> gpiochip_populate_parent_fwspec_fourcell() translate a pinctrl- >> >> spmi-gpio irqspec to an SPMI controller irqspec. When they do >> >> this, they use a fixed SPMI slave ID of 0 and a fixed GPIO >> >> peripheral offset of 0xC0 (corresponding to SPMI address 0xC000). >> >> This translation results in an incorrect irqspec for secondary >> >> PMICs that don't have a slave ID of 0 as well as for PMIC chips >> >> which have GPIO peripherals located at a base address other than >> >> 0xC000. >> >> >> >> Correct this issue by passing the slave ID of the pinctrl-spmi- >> >> gpio device's parent in the SPMI controller irqspec and by >> >> calculating the peripheral ID base from the device tree 'reg' >> >> property of the pinctrl-spmi-gpio device. >> >> >> >> Signed-off-by: David Collins <collinsd@codeaurora.org> >> >> Signed-off-by: satya priya <skakit@codeaurora.org> > > Can you please add an appropriate Fixes tag? > Okay. >> > >> > Is this a regression or is it fine if I just apply it for v5.15? >> > I was thinking v5.15 since it isn't yet used in device trees. >> > >> >> Without this fix, [2/2] Vol+ support is failing. If possible please >> merge it on current branch. >> > > Are there any boards supported upstream that have a gpio block that > isn't at 0xc000? yes, all the pmics used in sm8350-mtp.dts board have gpio block at addresses different than 0xc000. Thanks, Satya Priya
Quoting skakit@codeaurora.org (2021-08-17 02:06:42) > On 2021-08-17 02:38, Stephen Boyd wrote: > > > > Are there any boards supported upstream that have a gpio block that > > isn't at 0xc000? > > yes, all the pmics used in sm8350-mtp.dts board have gpio block at > addresses different than 0xc000. > So maybe Fixes: f67cc6a91d88 ("arm64: dts: qcom: sm8350-mtp: Add PMICs") is appropriate then?
Quoting skakit@codeaurora.org (2021-08-17 22:26:18) > On 2021-08-18 00:45, Stephen Boyd wrote: > > Quoting skakit@codeaurora.org (2021-08-17 02:06:42) > >> On 2021-08-17 02:38, Stephen Boyd wrote: > >> > > >> > Are there any boards supported upstream that have a gpio block that > >> > isn't at 0xc000? > >> > >> yes, all the pmics used in sm8350-mtp.dts board have gpio block at > >> addresses different than 0xc000. > >> > > > > So maybe > > > > Fixes: f67cc6a91d88 ("arm64: dts: qcom: sm8350-mtp: Add PMICs") > > > > is appropriate then? > > This patch is actually fixing the pinctrl-spmi-gpio.c driver. > So, I think we should add > > Fixes: ca69e2d165eb ("qcom: spmi-gpio: add support for hierarchical IRQ > chip") OK. Were you going to resend this patch? I don't see it in linux-next so I worry that Linus dropped it while the Fixes tag was figured out.
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c index 98bf0e2..dbae168 100644 --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. + * Copyright (c) 2012-2014, 2016-2021 The Linux Foundation. All rights reserved. */ #include <linux/gpio/driver.h> @@ -14,6 +14,7 @@ #include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/slab.h> +#include <linux/spmi.h> #include <linux/types.h> #include <dt-bindings/pinctrl/qcom,pmic-gpio.h> @@ -171,6 +172,8 @@ struct pmic_gpio_state { struct pinctrl_dev *ctrl; struct gpio_chip chip; struct irq_chip irq; + u8 usid; + u8 pid_base; }; static const struct pinconf_generic_params pmic_gpio_bindings[] = { @@ -949,12 +952,36 @@ static int pmic_gpio_child_to_parent_hwirq(struct gpio_chip *chip, unsigned int *parent_hwirq, unsigned int *parent_type) { - *parent_hwirq = child_hwirq + 0xc0; + struct pmic_gpio_state *state = gpiochip_get_data(chip); + + *parent_hwirq = child_hwirq + state->pid_base; *parent_type = child_type; return 0; } +static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip, + unsigned int parent_hwirq, + unsigned int parent_type) +{ + struct pmic_gpio_state *state = gpiochip_get_data(chip); + struct irq_fwspec *fwspec; + + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL); + if (!fwspec) + return NULL; + + fwspec->fwnode = chip->irq.parent_domain->fwnode; + + fwspec->param_count = 4; + fwspec->param[0] = state->usid; + fwspec->param[1] = parent_hwirq; + fwspec->param[2] = 0; + fwspec->param[3] = parent_type; + + return fwspec; +} + static int pmic_gpio_probe(struct platform_device *pdev) { struct irq_domain *parent_domain; @@ -965,6 +992,7 @@ static int pmic_gpio_probe(struct platform_device *pdev) struct pmic_gpio_pad *pad, *pads; struct pmic_gpio_state *state; struct gpio_irq_chip *girq; + const struct spmi_device *parent_spmi_dev; int ret, npins, i; u32 reg; @@ -984,6 +1012,9 @@ static int pmic_gpio_probe(struct platform_device *pdev) state->dev = &pdev->dev; state->map = dev_get_regmap(dev->parent, NULL); + parent_spmi_dev = to_spmi_device(dev->parent); + state->usid = parent_spmi_dev->usid; + state->pid_base = reg >> 8; pindesc = devm_kcalloc(dev, npins, sizeof(*pindesc), GFP_KERNEL); if (!pindesc) @@ -1059,7 +1090,7 @@ static int pmic_gpio_probe(struct platform_device *pdev) girq->fwnode = of_node_to_fwnode(state->dev->of_node); girq->parent_domain = parent_domain; girq->child_to_parent_hwirq = pmic_gpio_child_to_parent_hwirq; - girq->populate_parent_alloc_arg = gpiochip_populate_parent_fwspec_fourcell; + girq->populate_parent_alloc_arg = pmic_gpio_populate_parent_fwspec; girq->child_offset_to_irq = pmic_gpio_child_offset_to_irq; girq->child_irq_domain_ops.translate = pmic_gpio_domain_translate;