Message-ID: <CAN8TOE_Vd9c2eYgomhu_ukofTeO9eK8Yhrtt-8BQckmJnGfj6w@mail.gmail.com>
Date: Fri, 26 Apr 2024 15:08:06 -0700
From: Brian Norris <computersforpeace@...il.com>
To: Johan Hovold <johan@...nel.org>
Cc: Bjorn Andersson <quic_bjorande@...cinc.com>, Bjorn Andersson <andersson@...nel.org>, 
	Linus Walleij <linus.walleij@...aro.org>, Jaiganesh Narayanan <njaigane@...eaurora.org>, 
	Doug Anderson <dianders@...omium.org>, linux-arm-msm@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

Hi Johan, Bjorn,

On Thu, Apr 25, 2024 at 5:02 AM Johan Hovold <johan@...nel.org> wrote:
>
> On Wed, Apr 24, 2024 at 08:45:31PM -0700, Bjorn Andersson wrote:
> > When a GPIO is configured as OPEN_DRAIN gpiolib will in
> > gpiod_direction_output() attempt to configure the open-drain property of
> > the hardware and if this fails fall back to software emulation of this
> > state.
> >
> > The TLMM block in most Qualcomm platform does not implement such
> > functionality, so this call would be expected to fail. But due to lack
> > of checks for this condition, the zero-initialized od_bit will cause
> > this request to silently corrupt the lowest bit in the config register
> > (which typically is part of the bias configuration) and happily continue
> > on.

Apologies if I broke something here. Both the pinctrl subsystem and
the wide world of diverse QCOM chips can be complicated beasts. I
definitely could have missed things along the way. (And on first
glance, it seems like you may have found one. I definitely did not
consider the gpiod_direction_output() "emulation" behavior here when
submitting this.)

But I can't tell based on subsequent conversation: are you observing a
real problem, or is this a theoretical one that only exists if the
gpiochip driver adds set_config() support?

> > Fix this by checking if the od_bit value is unspecified and if so fail
> > the request to avoid the unexpected state, and to make sure the software
> > fallback actually kicks in.
>
> Fortunately, this is currently not a problem as the gpiochip driver does
> not implement the set_config() callback, which means that the attempt to
> change the pin configuration currently always fails with -ENOTSUP (see
> gpio_do_set_config()).
>
> Specifically, this means that the software fallback kicks in, which I
> had already verified.
>
> Now, perhaps there is some other path which can allow you to end up
> here, but it's at least not via gpiod_direction_output().
>
> The msm pinctrl binding does not allow 'drive-open-drain' so that path
> should also be ok unless you have a non-conformant devicetree.

The ipq4019 binding does:
https://git.kernel.org/linus/99d19f5a48ee6fbc647935de458505e9308078e3

This is used in OpenWrt device trees.

> > It is assumed for now that no implementation will come into existence
> > with BIT(0) being the open-drain bit, simply for convenience sake.
> >
> > Fixes: 13355ca35cd1 ("pinctrl: qcom: ipq4019: add open drain support")
>
> I guess hardware open-drain mode has never been properly tested on
> ipq4019.

It was quite some time ago that I wrote and tested this, and per the
above, I easily could have missed things. (Plus, the open drain
configuration may not have much practical effect on the systems in
question, so certain errors may not even be observable.)

But I do recall seeing the code in question activate. And inspection
shows that the pinconf_apply_setting() -> ... msm_config_group_set()
path is non-dead code here, for appropriate device trees.

I can try to fire up my development devices again and see what's up if
that helps, but I won't have time to do that in the next few days.

Brian

Powered by blists - more mailing lists