| Message ID | 20240215-b4-qcom-common-target-v4-10-ed06355c634a@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Qualcomm generic board support | expand |
On Fri, 16 Feb 2024 at 02:22, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Some platforms hard reset when attempting to configure PMIC GPIOs. Add > support for quirks specified in match data with a single quirk to skip > this configuration. We rely on the GPIO already be configured correctly, > which is always the case for volume up (the only current user of these > GPIOs). I can't find a similar quirk in the counterpart Linux driver (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c). Is there anything we are missing in the U-Boot driver? -Sumit > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > drivers/gpio/qcom_pmic_gpio.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c > index 2a4fef8d28cb..198cd84bc31e 100644 > --- a/drivers/gpio/qcom_pmic_gpio.c > +++ b/drivers/gpio/qcom_pmic_gpio.c > @@ -64,6 +64,15 @@ > #define REG_EN_CTL 0x46 > #define REG_EN_CTL_ENABLE (1 << 7) > > +/** > + * pmic_gpio_match_data - platform specific configuration > + * > + * @PMIC_MATCH_READONLY: treat all GPIOs as readonly, don't attempt to configure them > + */ > +enum pmic_gpio_quirks { > + QCOM_PMIC_QUIRK_READONLY = (1 << 0), > +}; > + > struct qcom_gpio_bank { > uint32_t pid; /* Peripheral ID on SPMI bus */ > bool lv_mv_type; /* If subtype is GPIO_LV(0x10) or GPIO_MV(0x11) */ > @@ -75,7 +84,12 @@ static int qcom_gpio_set_direction(struct udevice *dev, unsigned offset, > struct qcom_gpio_bank *priv = dev_get_priv(dev); > uint32_t gpio_base = priv->pid + REG_OFFSET(offset); > uint32_t reg_ctl_val; > - int ret; > + ulong quirks = dev_get_driver_data(dev); > + int ret = 0; > + > + /* Some PMICs don't like their GPIOs being configured */ > + if (quirks & QCOM_PMIC_QUIRK_READONLY) > + return 0; > > /* Disable the GPIO */ > ret = pmic_clrsetbits(dev->parent, gpio_base + REG_EN_CTL, > @@ -304,7 +318,7 @@ static int qcom_gpio_of_to_plat(struct udevice *dev) > static const struct udevice_id qcom_gpio_ids[] = { > { .compatible = "qcom,pm8916-gpio" }, > { .compatible = "qcom,pm8994-gpio" }, /* 22 GPIO's */ > - { .compatible = "qcom,pm8998-gpio" }, > + { .compatible = "qcom,pm8998-gpio", .data = QCOM_PMIC_QUIRK_READONLY }, > { .compatible = "qcom,pms405-gpio" }, > { } > }; > > -- > 2.43.1 >
On 20/02/2024 06:56, Sumit Garg wrote: > On Fri, 16 Feb 2024 at 02:22, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> Some platforms hard reset when attempting to configure PMIC GPIOs. Add >> support for quirks specified in match data with a single quirk to skip >> this configuration. We rely on the GPIO already be configured correctly, >> which is always the case for volume up (the only current user of these >> GPIOs). > > I can't find a similar quirk in the counterpart Linux driver > (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c). Is there anything we are > missing in the U-Boot driver? It's not ideal, it's fine to have it at first but at some point a proper solution should be found. Neil > > -Sumit > >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> drivers/gpio/qcom_pmic_gpio.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c >> index 2a4fef8d28cb..198cd84bc31e 100644 >> --- a/drivers/gpio/qcom_pmic_gpio.c >> +++ b/drivers/gpio/qcom_pmic_gpio.c >> @@ -64,6 +64,15 @@ >> #define REG_EN_CTL 0x46 >> #define REG_EN_CTL_ENABLE (1 << 7) >> >> +/** >> + * pmic_gpio_match_data - platform specific configuration >> + * >> + * @PMIC_MATCH_READONLY: treat all GPIOs as readonly, don't attempt to configure them >> + */ >> +enum pmic_gpio_quirks { >> + QCOM_PMIC_QUIRK_READONLY = (1 << 0), >> +}; >> + >> struct qcom_gpio_bank { >> uint32_t pid; /* Peripheral ID on SPMI bus */ >> bool lv_mv_type; /* If subtype is GPIO_LV(0x10) or GPIO_MV(0x11) */ >> @@ -75,7 +84,12 @@ static int qcom_gpio_set_direction(struct udevice *dev, unsigned offset, >> struct qcom_gpio_bank *priv = dev_get_priv(dev); >> uint32_t gpio_base = priv->pid + REG_OFFSET(offset); >> uint32_t reg_ctl_val; >> - int ret; >> + ulong quirks = dev_get_driver_data(dev); >> + int ret = 0; >> + >> + /* Some PMICs don't like their GPIOs being configured */ >> + if (quirks & QCOM_PMIC_QUIRK_READONLY) >> + return 0; >> >> /* Disable the GPIO */ >> ret = pmic_clrsetbits(dev->parent, gpio_base + REG_EN_CTL, >> @@ -304,7 +318,7 @@ static int qcom_gpio_of_to_plat(struct udevice *dev) >> static const struct udevice_id qcom_gpio_ids[] = { >> { .compatible = "qcom,pm8916-gpio" }, >> { .compatible = "qcom,pm8994-gpio" }, /* 22 GPIO's */ >> - { .compatible = "qcom,pm8998-gpio" }, >> + { .compatible = "qcom,pm8998-gpio", .data = QCOM_PMIC_QUIRK_READONLY }, >> { .compatible = "qcom,pms405-gpio" }, >> { } >> }; >> >> -- >> 2.43.1 >>
On Wed, 21 Feb 2024 at 14:19, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 20/02/2024 06:56, Sumit Garg wrote: > > On Fri, 16 Feb 2024 at 02:22, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> > >> Some platforms hard reset when attempting to configure PMIC GPIOs. Add > >> support for quirks specified in match data with a single quirk to skip > >> this configuration. We rely on the GPIO already be configured correctly, > >> which is always the case for volume up (the only current user of these > >> GPIOs). > > > > I can't find a similar quirk in the counterpart Linux driver > > (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c). Is there anything we are > > missing in the U-Boot driver? > > It's not ideal, it's fine to have it at first but at some point a proper > solution should be found. Then at least let's add code comments to say that it is just a workaround for the time being until a proper solution is found. -Sumit > > Neil > > > > > -Sumit > > > >> > >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > >> --- > >> drivers/gpio/qcom_pmic_gpio.c | 18 ++++++++++++++++-- > >> 1 file changed, 16 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c > >> index 2a4fef8d28cb..198cd84bc31e 100644 > >> --- a/drivers/gpio/qcom_pmic_gpio.c > >> +++ b/drivers/gpio/qcom_pmic_gpio.c > >> @@ -64,6 +64,15 @@ > >> #define REG_EN_CTL 0x46 > >> #define REG_EN_CTL_ENABLE (1 << 7) > >> > >> +/** > >> + * pmic_gpio_match_data - platform specific configuration > >> + * > >> + * @PMIC_MATCH_READONLY: treat all GPIOs as readonly, don't attempt to configure them > >> + */ > >> +enum pmic_gpio_quirks { > >> + QCOM_PMIC_QUIRK_READONLY = (1 << 0), > >> +}; > >> + > >> struct qcom_gpio_bank { > >> uint32_t pid; /* Peripheral ID on SPMI bus */ > >> bool lv_mv_type; /* If subtype is GPIO_LV(0x10) or GPIO_MV(0x11) */ > >> @@ -75,7 +84,12 @@ static int qcom_gpio_set_direction(struct udevice *dev, unsigned offset, > >> struct qcom_gpio_bank *priv = dev_get_priv(dev); > >> uint32_t gpio_base = priv->pid + REG_OFFSET(offset); > >> uint32_t reg_ctl_val; > >> - int ret; > >> + ulong quirks = dev_get_driver_data(dev); > >> + int ret = 0; > >> + > >> + /* Some PMICs don't like their GPIOs being configured */ > >> + if (quirks & QCOM_PMIC_QUIRK_READONLY) > >> + return 0; > >> > >> /* Disable the GPIO */ > >> ret = pmic_clrsetbits(dev->parent, gpio_base + REG_EN_CTL, > >> @@ -304,7 +318,7 @@ static int qcom_gpio_of_to_plat(struct udevice *dev) > >> static const struct udevice_id qcom_gpio_ids[] = { > >> { .compatible = "qcom,pm8916-gpio" }, > >> { .compatible = "qcom,pm8994-gpio" }, /* 22 GPIO's */ > >> - { .compatible = "qcom,pm8998-gpio" }, > >> + { .compatible = "qcom,pm8998-gpio", .data = QCOM_PMIC_QUIRK_READONLY }, > >> { .compatible = "qcom,pms405-gpio" }, > >> { } > >> }; > >> > >> -- > >> 2.43.1 > >>
diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c index 2a4fef8d28cb..198cd84bc31e 100644 --- a/drivers/gpio/qcom_pmic_gpio.c +++ b/drivers/gpio/qcom_pmic_gpio.c @@ -64,6 +64,15 @@ #define REG_EN_CTL 0x46 #define REG_EN_CTL_ENABLE (1 << 7) +/** + * pmic_gpio_match_data - platform specific configuration + * + * @PMIC_MATCH_READONLY: treat all GPIOs as readonly, don't attempt to configure them + */ +enum pmic_gpio_quirks { + QCOM_PMIC_QUIRK_READONLY = (1 << 0), +}; + struct qcom_gpio_bank { uint32_t pid; /* Peripheral ID on SPMI bus */ bool lv_mv_type; /* If subtype is GPIO_LV(0x10) or GPIO_MV(0x11) */ @@ -75,7 +84,12 @@ static int qcom_gpio_set_direction(struct udevice *dev, unsigned offset, struct qcom_gpio_bank *priv = dev_get_priv(dev); uint32_t gpio_base = priv->pid + REG_OFFSET(offset); uint32_t reg_ctl_val; - int ret; + ulong quirks = dev_get_driver_data(dev); + int ret = 0; + + /* Some PMICs don't like their GPIOs being configured */ + if (quirks & QCOM_PMIC_QUIRK_READONLY) + return 0; /* Disable the GPIO */ ret = pmic_clrsetbits(dev->parent, gpio_base + REG_EN_CTL, @@ -304,7 +318,7 @@ static int qcom_gpio_of_to_plat(struct udevice *dev) static const struct udevice_id qcom_gpio_ids[] = { { .compatible = "qcom,pm8916-gpio" }, { .compatible = "qcom,pm8994-gpio" }, /* 22 GPIO's */ - { .compatible = "qcom,pm8998-gpio" }, + { .compatible = "qcom,pm8998-gpio", .data = QCOM_PMIC_QUIRK_READONLY }, { .compatible = "qcom,pms405-gpio" }, { } };
Some platforms hard reset when attempting to configure PMIC GPIOs. Add support for quirks specified in match data with a single quirk to skip this configuration. We rely on the GPIO already be configured correctly, which is always the case for volume up (the only current user of these GPIOs). Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- drivers/gpio/qcom_pmic_gpio.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)