| Message ID | 20210524131852.263883-2-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/vc4: hdmi: Fix error path of hpd-gpios | expand |
On Mon, May 24, 2021 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > The new gpiod interface takes care of parsing the GPIO flags and to > return the logical value when accessing an active-low GPIO, so switching > to it simplifies a lot the driver. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Thanks for fixing this! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Linus On Fri, May 28, 2021 at 01:57:56AM +0200, Linus Walleij wrote: > On Mon, May 24, 2021 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > The new gpiod interface takes care of parsing the GPIO flags and to > > return the logical value when accessing an active-low GPIO, so switching > > to it simplifies a lot the driver. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Thanks for fixing this! > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Is it for both patches or just this one? Thanks! Maxime
On Fri, Jun 4, 2021 at 10:01 AM Maxime Ripard <maxime@cerno.tech> wrote: > On Fri, May 28, 2021 at 01:57:56AM +0200, Linus Walleij wrote: > > On Mon, May 24, 2021 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > The new gpiod interface takes care of parsing the GPIO flags and to > > > return the logical value when accessing an active-low GPIO, so switching > > > to it simplifies a lot the driver. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > Thanks for fixing this! > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Is it for both patches or just this one? I went and added Reviewed-by: to 1/2 as well so you can merge the patches. Was simple enough even though I'm not a VC4 expert. Yours, Linus Walleij
On Fri, Jun 04, 2021 at 11:45:36PM +0200, Linus Walleij wrote: > On Fri, Jun 4, 2021 at 10:01 AM Maxime Ripard <maxime@cerno.tech> wrote: > > On Fri, May 28, 2021 at 01:57:56AM +0200, Linus Walleij wrote: > > > On Mon, May 24, 2021 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > The new gpiod interface takes care of parsing the GPIO flags and to > > > > return the logical value when accessing an active-low GPIO, so switching > > > > to it simplifies a lot the driver. > > > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > > > Thanks for fixing this! > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > Is it for both patches or just this one? > > I went and added Reviewed-by: to 1/2 as well so you can merge the > patches. Was simple enough even though I'm not a VC4 expert. Thanks :) Applied both Maxime
On Mon, May 24, 2021 at 03:18:52PM +0200, Maxime Ripard wrote: > The new gpiod interface takes care of parsing the GPIO flags and to > return the logical value when accessing an active-low GPIO, so switching > to it simplifies a lot the driver. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- > drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- > 2 files changed, 8 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index ccc6c8079dc6..34622c59f6a7 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) > struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > bool connected = false; > > - if (vc4_hdmi->hpd_gpio) { > - if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ > - vc4_hdmi->hpd_active_low) > - connected = true; > + if (vc4_hdmi->hpd_gpio && > + gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) { > + connected = true; > } else if (drm_probe_ddc(vc4_hdmi->ddc)) { > connected = true; > } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { > @@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > struct vc4_hdmi *vc4_hdmi; > struct drm_encoder *encoder; > struct device_node *ddc_node; > - u32 value; > int ret; > > vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); > @@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > /* Only use the GPIO HPD pin if present in the DT, otherwise > * we'll use the HDMI core's register. > */ > - if (of_find_property(dev->of_node, "hpd-gpios", &value)) { > - enum of_gpio_flags hpd_gpio_flags; > - > - vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node, > - "hpd-gpios", 0, > - &hpd_gpio_flags); > - if (vc4_hdmi->hpd_gpio < 0) { > - ret = vc4_hdmi->hpd_gpio; > - goto err_put_ddc; > - } > - > - vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW; > + vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); > + if (IS_ERR(vc4_hdmi->hpd_gpio)) { > + ret = PTR_ERR(vc4_hdmi->hpd_gpio); > + goto err_put_ddc; > } > > vc4_hdmi->disable_wifi_frequencies = > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h > index 060bcaefbeb5..2688a55461d6 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h > @@ -146,8 +146,7 @@ struct vc4_hdmi { > /* VC5 Only */ > void __iomem *rm_regs; > > - int hpd_gpio; > - bool hpd_active_low; > + struct gpio_desc *hpd_gpio; > > /* > * On some systems (like the RPi4), some modes are in the same > -- > 2.31.1 Hi Maxime, This patch as commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod") causes my Raspberry Pi 3 to lock up shortly after boot in combination with commit 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm"). The serial console and ssh are completely unresponsive and I do not see any messages in dmesg with "debug ignore_loglevel". The device is running with a 32-bit kernel (multi_v7_defconfig) with 32-bit userspace. If there is any further information that I can provide, please let me know. Cheers, Nathan
Hi Nathan, On Thu, Jul 01, 2021 at 08:29:34PM -0700, Nathan Chancellor wrote: > On Mon, May 24, 2021 at 03:18:52PM +0200, Maxime Ripard wrote: > > The new gpiod interface takes care of parsing the GPIO flags and to > > return the logical value when accessing an active-low GPIO, so switching > > to it simplifies a lot the driver. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- > > drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- > > 2 files changed, 8 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index ccc6c8079dc6..34622c59f6a7 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) > > struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > bool connected = false; > > > > - if (vc4_hdmi->hpd_gpio) { > > - if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ > > - vc4_hdmi->hpd_active_low) > > - connected = true; > > + if (vc4_hdmi->hpd_gpio && > > + gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) { > > + connected = true; > > } else if (drm_probe_ddc(vc4_hdmi->ddc)) { > > connected = true; > > } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { > > @@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > > struct vc4_hdmi *vc4_hdmi; > > struct drm_encoder *encoder; > > struct device_node *ddc_node; > > - u32 value; > > int ret; > > > > vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); > > @@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > > /* Only use the GPIO HPD pin if present in the DT, otherwise > > * we'll use the HDMI core's register. > > */ > > - if (of_find_property(dev->of_node, "hpd-gpios", &value)) { > > - enum of_gpio_flags hpd_gpio_flags; > > - > > - vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node, > > - "hpd-gpios", 0, > > - &hpd_gpio_flags); > > - if (vc4_hdmi->hpd_gpio < 0) { > > - ret = vc4_hdmi->hpd_gpio; > > - goto err_put_ddc; > > - } > > - > > - vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW; > > + vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); > > + if (IS_ERR(vc4_hdmi->hpd_gpio)) { > > + ret = PTR_ERR(vc4_hdmi->hpd_gpio); > > + goto err_put_ddc; > > } > > > > vc4_hdmi->disable_wifi_frequencies = > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h > > index 060bcaefbeb5..2688a55461d6 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h > > @@ -146,8 +146,7 @@ struct vc4_hdmi { > > /* VC5 Only */ > > void __iomem *rm_regs; > > > > - int hpd_gpio; > > - bool hpd_active_low; > > + struct gpio_desc *hpd_gpio; > > > > /* > > * On some systems (like the RPi4), some modes are in the same > > -- > > 2.31.1 > > This patch as commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod") > causes my Raspberry Pi 3 to lock up shortly after boot in combination > with commit 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to > runtime_pm"). The serial console and ssh are completely unresponsive and > I do not see any messages in dmesg with "debug ignore_loglevel". The > device is running with a 32-bit kernel (multi_v7_defconfig) with 32-bit > userspace. If there is any further information that I can provide, > please let me know. Thanks for reporting this. The same bug has been reported on wednesday on the RPi repo here: https://github.com/raspberrypi/linux/pull/4418 More specifically, this commit should fix it: https://github.com/raspberrypi/linux/pull/4418/commits/6d404373c20a794da3d6a7b4f1373903183bb5d0 Even though it's based on the 5.10 kernel, it should apply without any warning on a mainline tree. Let me know if it fixes your issue too Maxime
On Fri, Jul 02, 2021 at 03:16:46PM +0200, Maxime Ripard wrote: > Hi Nathan, > > On Thu, Jul 01, 2021 at 08:29:34PM -0700, Nathan Chancellor wrote: > > On Mon, May 24, 2021 at 03:18:52PM +0200, Maxime Ripard wrote: > > > The new gpiod interface takes care of parsing the GPIO flags and to > > > return the logical value when accessing an active-low GPIO, so switching > > > to it simplifies a lot the driver. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- > > > drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- > > > 2 files changed, 8 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index ccc6c8079dc6..34622c59f6a7 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) > > > struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > > bool connected = false; > > > > > > - if (vc4_hdmi->hpd_gpio) { > > > - if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ > > > - vc4_hdmi->hpd_active_low) > > > - connected = true; > > > + if (vc4_hdmi->hpd_gpio && > > > + gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) { > > > + connected = true; > > > } else if (drm_probe_ddc(vc4_hdmi->ddc)) { > > > connected = true; > > > } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { > > > @@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > > > struct vc4_hdmi *vc4_hdmi; > > > struct drm_encoder *encoder; > > > struct device_node *ddc_node; > > > - u32 value; > > > int ret; > > > > > > vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); > > > @@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > > > /* Only use the GPIO HPD pin if present in the DT, otherwise > > > * we'll use the HDMI core's register. > > > */ > > > - if (of_find_property(dev->of_node, "hpd-gpios", &value)) { > > > - enum of_gpio_flags hpd_gpio_flags; > > > - > > > - vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node, > > > - "hpd-gpios", 0, > > > - &hpd_gpio_flags); > > > - if (vc4_hdmi->hpd_gpio < 0) { > > > - ret = vc4_hdmi->hpd_gpio; > > > - goto err_put_ddc; > > > - } > > > - > > > - vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW; > > > + vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); > > > + if (IS_ERR(vc4_hdmi->hpd_gpio)) { > > > + ret = PTR_ERR(vc4_hdmi->hpd_gpio); > > > + goto err_put_ddc; > > > } > > > > > > vc4_hdmi->disable_wifi_frequencies = > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h > > > index 060bcaefbeb5..2688a55461d6 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h > > > @@ -146,8 +146,7 @@ struct vc4_hdmi { > > > /* VC5 Only */ > > > void __iomem *rm_regs; > > > > > > - int hpd_gpio; > > > - bool hpd_active_low; > > > + struct gpio_desc *hpd_gpio; > > > > > > /* > > > * On some systems (like the RPi4), some modes are in the same > > > -- > > > 2.31.1 > > > > This patch as commit 6800234ceee0 ("drm/vc4: hdmi: Convert to gpiod") > > causes my Raspberry Pi 3 to lock up shortly after boot in combination > > with commit 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to > > runtime_pm"). The serial console and ssh are completely unresponsive and > > I do not see any messages in dmesg with "debug ignore_loglevel". The > > device is running with a 32-bit kernel (multi_v7_defconfig) with 32-bit > > userspace. If there is any further information that I can provide, > > please let me know. > > Thanks for reporting this. The same bug has been reported on wednesday > on the RPi repo here: > https://github.com/raspberrypi/linux/pull/4418 > > More specifically, this commit should fix it: > https://github.com/raspberrypi/linux/pull/4418/commits/6d404373c20a794da3d6a7b4f1373903183bb5d0 > > Even though it's based on the 5.10 kernel, it should apply without any > warning on a mainline tree. Let me know if it fixes your issue too Thank you for the links and the quick reply. Unfortunately, I applied this patch on top of commit d6b63b5b7d7f ("Merge tag 'sound-5.14-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound") in mainline, which does reproduce this issue still and it did not fix the issue. In fact, it did not even get to the raspberrypi login prompt before it locked up, yet again without any real output in the serial console except for maybe this message? [ 7.582480] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4]) If there is anything further that I can provide or test, please let me know! Cheers, Nathan
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index ccc6c8079dc6..34622c59f6a7 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -159,10 +159,9 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); bool connected = false; - if (vc4_hdmi->hpd_gpio) { - if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ - vc4_hdmi->hpd_active_low) - connected = true; + if (vc4_hdmi->hpd_gpio && + gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio)) { + connected = true; } else if (drm_probe_ddc(vc4_hdmi->ddc)) { connected = true; } else if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) { @@ -1993,7 +1992,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) struct vc4_hdmi *vc4_hdmi; struct drm_encoder *encoder; struct device_node *ddc_node; - u32 value; int ret; vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); @@ -2031,18 +2029,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) /* Only use the GPIO HPD pin if present in the DT, otherwise * we'll use the HDMI core's register. */ - if (of_find_property(dev->of_node, "hpd-gpios", &value)) { - enum of_gpio_flags hpd_gpio_flags; - - vc4_hdmi->hpd_gpio = of_get_named_gpio_flags(dev->of_node, - "hpd-gpios", 0, - &hpd_gpio_flags); - if (vc4_hdmi->hpd_gpio < 0) { - ret = vc4_hdmi->hpd_gpio; - goto err_put_ddc; - } - - vc4_hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW; + vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); + if (IS_ERR(vc4_hdmi->hpd_gpio)) { + ret = PTR_ERR(vc4_hdmi->hpd_gpio); + goto err_put_ddc; } vc4_hdmi->disable_wifi_frequencies = diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 060bcaefbeb5..2688a55461d6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -146,8 +146,7 @@ struct vc4_hdmi { /* VC5 Only */ void __iomem *rm_regs; - int hpd_gpio; - bool hpd_active_low; + struct gpio_desc *hpd_gpio; /* * On some systems (like the RPi4), some modes are in the same
The new gpiod interface takes care of parsing the GPIO flags and to return the logical value when accessing an active-low GPIO, so switching to it simplifies a lot the driver. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++----------------- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +-- 2 files changed, 8 insertions(+), 19 deletions(-)