| Message ID | 20171003220311.GU457@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/03/2017 05:03 PM, Stephen Boyd wrote: > I've run into this now on our mobile SoCs after I pull in commit > 8e51533780ba ("pinctrl: qcom: add get_direction function"). > Before that commit we never read each pin of the device. On our > mobile SoCs we have devicetree and it feels like having that > describe which pins are available and not available is > half-duplicating information we would already have via consumers > indicating which pins they care about. I don't see any value > beyond system wide debug in figuring out the default pin > configuration of a pin that doesn't have a consumer in Linux. At the time I wrote that patch, the ACPI tables exposed all of the GPIOs, even the ones it didn't care about. The new ACPI tables list only specific GPIOs, and so we no longer need to blindly read the direction of all GPIOs. > Could we remove the pin direction finding part here in > gpiochip_add_pin_range() and lazily resolve the pin direction > when a pin is requested? That makes a lot more sense. > We would need a similar check in the msm > specific debugfs code where we skip pins that aren't requested. I have that in patch #1. > This is basically a revert of commit 72d320006177 ("gpio: set up > initial state from .get_direction()"). I would be in favor of either reverting that patch, or moving the code into gpiochip_add_pin_range(). > ACPI can still describe only the pin ranges that they care about > exposing, but from the devicetree side it's been working well > enough to not touch pins that aren't used by anything in Linux. I do hate having to hack up the driver to support crappy ACPI definitions, but I'm stuck between a rock and a hard place.
On 10/03, Timur Tabi wrote: > On 10/03/2017 05:03 PM, Stephen Boyd wrote: > >I've run into this now on our mobile SoCs after I pull in commit > >8e51533780ba ("pinctrl: qcom: add get_direction function"). > >Before that commit we never read each pin of the device. On our > >mobile SoCs we have devicetree and it feels like having that > >describe which pins are available and not available is > >half-duplicating information we would already have via consumers > >indicating which pins they care about. I don't see any value > >beyond system wide debug in figuring out the default pin > >configuration of a pin that doesn't have a consumer in Linux. > > At the time I wrote that patch, the ACPI tables exposed all of the > GPIOs, even the ones it didn't care about. The new ACPI tables list > only specific GPIOs, and so we no longer need to blindly read the > direction of all GPIOs. > Do you avoid this problem on new ACPI tables because only pins that are able to be read are exposed? > > >This is basically a revert of commit 72d320006177 ("gpio: set up > >initial state from .get_direction()"). > > I would be in favor of either reverting that patch, or moving the > code into gpiochip_add_pin_range(). If it's in gpiochip_add_pin_range() would we still read the hardware when creating the pin ranges? I don't want to have to describe pin ranges of "valid" pins that won't cause the system to blow up if we touch them, because those pins are never used by Linux so reading them is not useful.
On 10/04/2017 04:50 PM, Stephen Boyd wrote: >> At the time I wrote that patch, the ACPI tables exposed all of the >> GPIOs, even the ones it didn't care about. The new ACPI tables list >> only specific GPIOs, and so we no longer need to blindly read the >> direction of all GPIOs. >> > > Do you avoid this problem on new ACPI tables because only pins > that are able to be read are exposed? Yes. A recent firmware update enabled the "XPU" block which is being programmed with a select subset of individual GPIOs. On our silicon, each TLMM GPIO is in a separate 64k page, and so the XPU can block any individual GPIO. Any attempt to touch those registers causes an XPU violation which takes the whole system down. >>> This is basically a revert of commit 72d320006177 ("gpio: set up >>> initial state from .get_direction()"). >> >> I would be in favor of either reverting that patch, or moving the >> code into gpiochip_add_pin_range(). > > If it's in gpiochip_add_pin_range() would we still read the > hardware when creating the pin ranges? I presume so. The idea is that pinctrl-qdf2xxx/pinctrl-msm only submit pin ranges that are present in the ACPI tables. > I don't want to have to> describe pin ranges of "valid" pins that won't cause the system > to blow up if we touch them, because those pins are never used by > Linux so reading them is not useful. Well, that's exactly what I'm trying to do with current patch set :-) It seems the most logical approach to me. I don't understand the dislike for it. What else are pin ranges for, other than to specify ranges of pins that can be accessed? Another alternative was to enumerate all of the GPIOs starting from 0. So the first GPIO in ACPI would be gpio0, regardless of what gpio number it actually was. E.g. GPIO 37 would appear as gpio0, GPIO 38 would appear as gpio1, and so on. That also worked, but it meant that customers would need to figure out which GPIO that "gpio0" actually pointed to. That was not acceptable, so I dropped it. I'm at a loss on how else to do it. I think a gpio_chip.available callback is far less elegant than define pin ranges. There is no chance that unavailable GPIOs can be accessed because the physical addresses are not in the msm_pingroup array. That is, groups[0].ctrl_reg == 0, not 0xFF02010000.
On 10/04, Timur Tabi wrote: > On 10/04/2017 04:50 PM, Stephen Boyd wrote: > > Yes. A recent firmware update enabled the "XPU" block which is > being programmed with a select subset of individual GPIOs. On our > silicon, each TLMM GPIO is in a separate 64k page, and so the XPU > can block any individual GPIO. Any attempt to touch those registers > causes an XPU violation which takes the whole system down. Yes it's the same sort of design with the hardware I have too. > > > > >If it's in gpiochip_add_pin_range() would we still read the > >hardware when creating the pin ranges? > > I presume so. The idea is that pinctrl-qdf2xxx/pinctrl-msm only > submit pin ranges that are present in the ACPI tables. Ok. > > > I don't want to have to describe pin ranges of "valid" pins that > > won't cause the system > > to blow up if we touch them, because those pins are never used by > > Linux so reading them is not useful. > > Well, that's exactly what I'm trying to do with current patch set > :-) It seems the most logical approach to me. I don't understand > the dislike for it. What else are pin ranges for, other than to > specify ranges of pins that can be accessed? I have no idea. To describe non-contiguous pin ranges? Linus? > > Another alternative was to enumerate all of the GPIOs starting from > 0. So the first GPIO in ACPI would be gpio0, regardless of what gpio > number it actually was. E.g. GPIO 37 would appear as gpio0, GPIO 38 > would appear as gpio1, and so on. That also worked, but it meant > that customers would need to figure out which GPIO that "gpio0" > actually pointed to. That was not acceptable, so I dropped it. Agreed. > > I'm at a loss on how else to do it. I think a gpio_chip.available > callback is far less elegant than define pin ranges. There is no > chance that unavailable GPIOs can be accessed because the physical > addresses are not in the msm_pingroup array. That is, > groups[0].ctrl_reg == 0, not 0xFF02010000. > Yes, thinking more about it I don't want an available callback either. It will add burden on DT platforms where we have to describe per-firmware pin ranges just because gpiolib is reading the direction of gpios we don't use. Instead, I'd prefer we delay reading the direction until a consumer requests the gpio, this way we don't touch the hardware unless a consumer wants to. That seems simpler and doesn't require anything special from the driver. Don't get me wrong, I'm willing to describe with DT/ACPI which pins are available if we have a need for it, but so far I don't see the requirement and I'm a lazy person so I like avoiding more work. Does my patch fail on your platform for some reason? I can only guess that somewhere we don't request the gpio before using it and then you don't see the proper direction.
On Wed, Oct 4, 2017 at 12:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > Could we remove the pin direction finding part here in > gpiochip_add_pin_range() and lazily resolve the pin direction > when a pin is requested? We would need a similar check in the msm > specific debugfs code where we skip pins that aren't requested. > This is basically a revert of commit 72d320006177 ("gpio: set up > initial state from .get_direction()"). It seems reasonable for the gpiolib to be able to call this function immediately after registering the new GPIO chip with its vtable. I think it is more up to the driver to numb the reply with some dummy return value (i.e. input mode) or refactor the callback so that it is acceptable for gpiolib to get an -EINVAL or so from the driver (again it will assume input mode) if the driver can't return the direction at this time. Yours, Linus Walleij
On 10/11, Linus Walleij wrote: > On Wed, Oct 4, 2017 at 12:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > Could we remove the pin direction finding part here in > > gpiochip_add_pin_range() and lazily resolve the pin direction > > when a pin is requested? We would need a similar check in the msm > > specific debugfs code where we skip pins that aren't requested. > > This is basically a revert of commit 72d320006177 ("gpio: set up > > initial state from .get_direction()"). > > It seems reasonable for the gpiolib to be able to call this > function immediately after registering the new GPIO chip > with its vtable. I agree. I don't see the benefit though. Reading the direction later would achieve the same effect and also work for ACPI qcom platforms. > > I think it is more up to the driver to numb the reply with > some dummy return value (i.e. input mode) or refactor the > callback so that it is acceptable for gpiolib to get an -EINVAL > or so from the driver (again it will assume input mode) if the > driver can't return the direction at this time. > For qcom platforms the driver will never be able to return the direction for these certain pins because reading the register is not allowed by the firmware. Doing so will cause the device to crash with a security violation. If you don't want to delay reading the direction until request time, we should have the DT msm pinctrl drivers leave the get_direction() pointer as NULL. We don't need to read the direction on DT platforms to make anything work.
On 10/03/2017 05:03 PM, Stephen Boyd wrote: > I don't see any value > beyond system wide debug in figuring out the default pin > configuration of a pin that doesn't have a consumer in Linux. I can agree with that. > Could we remove the pin direction finding part here in > gpiochip_add_pin_range() and lazily resolve the pin direction > when a pin is requested? We would need a similar check in the msm > specific debugfs code where we skip pins that aren't requested. > This is basically a revert of commit 72d320006177 ("gpio: set up > initial state from .get_direction()"). > > ACPI can still describe only the pin ranges that they care about > exposing, but from the devicetree side it's been working well > enough to not touch pins that aren't used by anything in Linux. > > ---8<---- > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index cd003b74512f..673028823bc5 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data) > * wrong direction in sysfs. > */ > > - if (chip->get_direction) { > - /* > - * If we have .get_direction, set up the initial > - * direction flag from the hardware. > - */ > - int dir = chip->get_direction(chip, i); > - > - if (!dir) > - set_bit(FLAG_IS_OUT, &desc->flags); > - } else if (!chip->direction_input) { > + if (!chip->direction_input) { > /* > * If the chip lacks the .direction_input callback > * we logically assume all lines are outputs. > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 273badd92561..4a0aeceb42f1 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -24,7 +24,7 @@ > #include <linux/pinctrl/pinconf.h> > #include <linux/pinctrl/pinconf-generic.h> > #include <linux/slab.h> > -#include <linux/gpio.h> > +#include <linux/gpio/driver.h> > #include <linux/interrupt.h> > #include <linux/spinlock.h> > #include <linux/reboot.h> > @@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, > }; > > g = &pctrl->soc->groups[offset]; > + > + if (!gpiochip_is_requested(chip, gpio)) { > + seq_printf(s, " %-8s:", g->name); > + return; > + } > + > ctl_reg = readl(pctrl->regs + g->ctl_reg); > > is_out = !!(ctl_reg & BIT(g->oe_bit)); In order for this to work, I had to add this function from patch #1: static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset) { struct msm_pinctrl *pctrl = gpiochip_get_data(chip); const struct msm_pingroup *g = &pctrl->soc->groups[offset]; if (!g->npins) return -ENODEV; return gpiochip_generic_request(chip, offset); } The problem with this is that none of the GPIOs are "requested", so it displays an output like this: # cat /sys/kernel/debug/gpio gpiochip0: GPIOs 0-149, parent: platform/QCOM8002:00, QCOM8002:00: (null) : (null) : (null) : [... truncated ]] (null) : (null) : (null) : (null) : gpio36 : gpio37 : gpio38 : gpio39 : (null) : (null) : It can't differentiate between GPIOs that don't exist and GPIOs that haven't been requested. Plus, the "(null)" entries are what I've been trying to avoid in the first place. So overall, this patch seems okay, although it needs a little work. However, it doesn't address Bjorn's complaint that he doesn't want me to use pin ranges at all.
On Thu, Oct 12, 2017 at 9:39 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > For qcom platforms the driver will never be able to return the > direction for these certain pins because reading the register is > not allowed by the firmware. Doing so will cause the device to > crash with a security violation. So I guess the driver needs to know what pin registers it can't access so the user does not get a gun to shoot in the foot with. If we augment gpiolib to just handle -EACCES or something (-EIO?) from the driver .get_direction() callback for these lines, things should be smooth? Yours, Linus Walleij
On Fri, Oct 13, 2017 at 06:26:23PM -0500, Timur Tabi wrote: > On 10/03/2017 05:03 PM, Stephen Boyd wrote: > > I don't see any value > > beyond system wide debug in figuring out the default pin > > configuration of a pin that doesn't have a consumer in Linux. > > I can agree with that. > > > Could we remove the pin direction finding part here in > > gpiochip_add_pin_range() and lazily resolve the pin direction > > when a pin is requested? We would need a similar check in the msm > > specific debugfs code where we skip pins that aren't requested. > > This is basically a revert of commit 72d320006177 ("gpio: set up > > initial state from .get_direction()"). > > > > ACPI can still describe only the pin ranges that they care about > > exposing, but from the devicetree side it's been working well > > enough to not touch pins that aren't used by anything in Linux. > > > > ---8<---- > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index cd003b74512f..673028823bc5 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data) > > * wrong direction in sysfs. > > */ > > - if (chip->get_direction) { > > - /* > > - * If we have .get_direction, set up the initial > > - * direction flag from the hardware. > > - */ > > - int dir = chip->get_direction(chip, i); > > - > > - if (!dir) > > - set_bit(FLAG_IS_OUT, &desc->flags); > > - } else if (!chip->direction_input) { > > + if (!chip->direction_input) { > > /* > > * If the chip lacks the .direction_input callback > > * we logically assume all lines are outputs. > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > index 273badd92561..4a0aeceb42f1 100644 > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > @@ -24,7 +24,7 @@ > > #include <linux/pinctrl/pinconf.h> > > #include <linux/pinctrl/pinconf-generic.h> > > #include <linux/slab.h> > > -#include <linux/gpio.h> > > +#include <linux/gpio/driver.h> > > #include <linux/interrupt.h> > > #include <linux/spinlock.h> > > #include <linux/reboot.h> > > @@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, > > }; > > g = &pctrl->soc->groups[offset]; > > + > > + if (!gpiochip_is_requested(chip, gpio)) { > > + seq_printf(s, " %-8s:", g->name); > > + return; > > + } > > + > > ctl_reg = readl(pctrl->regs + g->ctl_reg); > > is_out = !!(ctl_reg & BIT(g->oe_bit)); > > In order for this to work, I had to add this function from patch #1: > > static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset) > { > struct msm_pinctrl *pctrl = gpiochip_get_data(chip); > const struct msm_pingroup *g = &pctrl->soc->groups[offset]; > > if (!g->npins) > return -ENODEV; > > return gpiochip_generic_request(chip, offset); > } > > The problem with this is that none of the GPIOs are "requested", so it > displays an output like this: > > # cat /sys/kernel/debug/gpio > gpiochip0: GPIOs 0-149, parent: platform/QCOM8002:00, QCOM8002:00: > (null) : > (null) : > (null) : > [... truncated ]] > (null) : > (null) : > (null) : > (null) : > gpio36 : > gpio37 : > gpio38 : > gpio39 : > (null) : > (null) : > > It can't differentiate between GPIOs that don't exist and GPIOs that haven't > been requested. This confuses me. Why would you even want to register pins that don't exist? It sounds to me like you're lying to gpiolib and then try to work around it trying to access the GPIOs that don't exist but which you told it were there. Why not just tell gpiolib about only the GPIOs that exist? Thierry
On 10/15/17 3:18 PM, Thierry Reding wrote: > This confuses me. Why would you even want to register pins that don't > exist? It sounds to me like you're lying to gpiolib and then try to work > around it trying to access the GPIOs that don't exist but which you told > it were there. > > Why not just tell gpiolib about only the GPIOs that exist? Please look at my patches. That's exactly what they do, but no one else likes that approach.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index cd003b74512f..673028823bc5 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data) * wrong direction in sysfs. */ - if (chip->get_direction) { - /* - * If we have .get_direction, set up the initial - * direction flag from the hardware. - */ - int dir = chip->get_direction(chip, i); - - if (!dir) - set_bit(FLAG_IS_OUT, &desc->flags); - } else if (!chip->direction_input) { + if (!chip->direction_input) { /* * If the chip lacks the .direction_input callback * we logically assume all lines are outputs. diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 273badd92561..4a0aeceb42f1 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -24,7 +24,7 @@ #include <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinconf-generic.h> #include <linux/slab.h> -#include <linux/gpio.h> +#include <linux/gpio/driver.h> #include <linux/interrupt.h> #include <linux/spinlock.h> #include <linux/reboot.h> @@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, }; g = &pctrl->soc->groups[offset]; + + if (!gpiochip_is_requested(chip, gpio)) { + seq_printf(s, " %-8s:", g->name); + return; + } + ctl_reg = readl(pctrl->regs + g->ctl_reg); is_out = !!(ctl_reg & BIT(g->oe_bit));