The Raspberry Pi 5 includes Broadcom STB GPIO IP as well as
Broadcom 2712 pin controller IP.
The community has expressed interest in linking the two drivers
with the "gpio-ranges" property in device tree. This commit
stack implements the necessary changes.
Doug Berger (3):
dt-bindings: gpio: brcmstb: add gpio-ranges
gpio: of: support gpio-ranges for multiple gpiochip devices
gpio: brcmstb: add support for gpio-ranges
.../bindings/gpio/brcm,brcmstb-gpio.yaml | 3 +++
drivers/gpio/gpio-brcmstb.c | 2 ++
drivers/gpio/gpiolib-of.c | 23 +++++++++++++++++--
3 files changed, 26 insertions(+), 2 deletions(-)
--
2.34.1
Some drivers (e.g. gpio-mt7621 and gpio-brcmstb) have multiple
gpiochip banks within a single device. Unfortunately, the
gpio-ranges property of the device node was being applied to
every gpiochip of the device with device relative GPIO offset
values rather than gpiochip relative GPIO offset values.
This commit makes use of the gpio_chip offset value which can be
non-zero for such devices to split the device node gpio-ranges
property into GPIO offset ranges that can be applied to each
of the relevant gpiochips of the device.
Signed-off-by: Doug Berger <[email protected]>
---
drivers/gpio/gpiolib-of.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index cb0cefaec37e..d75f6ee37028 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1037,7 +1037,7 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
struct of_phandle_args pinspec;
struct pinctrl_dev *pctldev;
struct device_node *np;
- int index = 0, ret;
+ int index = 0, ret, trim;
const char *name;
static const char group_names_propname[] = "gpio-ranges-group-names";
struct property *group_names;
@@ -1059,7 +1059,14 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
if (!pctldev)
return -EPROBE_DEFER;
+ /* Ignore ranges outside of this GPIO chip */
+ if (pinspec.args[0] >= (chip->offset + chip->ngpio))
+ continue;
+ if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
+ continue;
+
if (pinspec.args[2]) {
+ /* npins != 0: linear range */
if (group_names) {
of_property_read_string_index(np,
group_names_propname,
@@ -1070,7 +1077,19 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
break;
}
}
- /* npins != 0: linear range */
+
+ /* Trim the range to fit this GPIO chip */
+ if (chip->offset > pinspec.args[0]) {
+ trim = chip->offset - pinspec.args[0];
+ pinspec.args[2] -= trim;
+ pinspec.args[1] += trim;
+ pinspec.args[0] = 0;
+ } else {
+ pinspec.args[0] -= chip->offset;
+ }
+ if ((pinspec.args[0] + pinspec.args[2]) > chip->ngpio)
+ pinspec.args[2] = chip->ngpio - pinspec.args[0];
+
ret = gpiochip_add_pin_range(chip,
pinctrl_dev_get_devname(pctldev),
pinspec.args[0],
--
2.34.1
Add optional gpio-ranges device-tree property to the Broadcom
Set-Top-Box GPIO controller.
Signed-off-by: Doug Berger <[email protected]>
---
Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml
index a1e71c974e79..f096f286da19 100644
--- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml
@@ -62,6 +62,8 @@ properties:
interrupt-controller: true
+ gpio-ranges: true
+
wakeup-source:
type: boolean
description: >
@@ -88,6 +90,7 @@ examples:
interrupt-parent = <&irq0_intc>;
interrupts = <0x6>;
brcm,gpio-bank-widths = <32 32 32 24>;
+ gpio-ranges = <&pinctrl 0 0 120>;
};
upg_gio_aon: gpio@f04172c0 {
--
2.34.1
A pin controller device mapped with the gpio-ranges property
will need implementations of the .request and .free members of
the gpiochip.
Signed-off-by: Doug Berger <[email protected]>
Tested-by: Phil Elwell <[email protected]>
---
drivers/gpio/gpio-brcmstb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index 790cb278b72a..8dce78ea7139 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -694,6 +694,8 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
/* not all ngpio lines are valid, will use bank width later */
gc->ngpio = MAX_GPIO_PER_BANK;
gc->offset = bank->id * MAX_GPIO_PER_BANK;
+ gc->request = gpiochip_generic_request;
+ gc->free = gpiochip_generic_free;
if (priv->parent_irq > 0)
gc->to_irq = brcmstb_gpio_to_irq;
--
2.34.1
On 4/24/24 11:50, Doug Berger wrote:
> Some drivers (e.g. gpio-mt7621 and gpio-brcmstb) have multiple
> gpiochip banks within a single device. Unfortunately, the
> gpio-ranges property of the device node was being applied to
> every gpiochip of the device with device relative GPIO offset
> values rather than gpiochip relative GPIO offset values.
>
> This commit makes use of the gpio_chip offset value which can be
> non-zero for such devices to split the device node gpio-ranges
> property into GPIO offset ranges that can be applied to each
> of the relevant gpiochips of the device.
>
> Signed-off-by: Doug Berger <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
On 4/24/24 11:50, Doug Berger wrote:
> Add optional gpio-ranges device-tree property to the Broadcom
> Set-Top-Box GPIO controller.
>
> Signed-off-by: Doug Berger <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
On 4/24/24 11:50, Doug Berger wrote:
> A pin controller device mapped with the gpio-ranges property
> will need implementations of the .request and .free members of
> the gpiochip.
>
> Signed-off-by: Doug Berger <[email protected]>
> Tested-by: Phil Elwell <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
On 24/04/2024 20:50, Doug Berger wrote:
> Add optional gpio-ranges device-tree property to the Broadcom
> Set-Top-Box GPIO controller.
>
> Signed-off-by: Doug Berger <[email protected]>
> ---
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <[email protected]> wrote:
>
> Some drivers (e.g. gpio-mt7621 and gpio-brcmstb) have multiple
> gpiochip banks within a single device. Unfortunately, the
> gpio-ranges property of the device node was being applied to
> every gpiochip of the device with device relative GPIO offset
> values rather than gpiochip relative GPIO offset values.
>
> This commit makes use of the gpio_chip offset value which can be
> non-zero for such devices to split the device node gpio-ranges
> property into GPIO offset ranges that can be applied to each
> of the relevant gpiochips of the device.
>
> Signed-off-by: Doug Berger <[email protected]>
> ---
This is a good improvement, thanks!
Bart
From: Bartosz Golaszewski <[email protected]>
On Wed, 24 Apr 2024 11:50:36 -0700, Doug Berger wrote:
> The Raspberry Pi 5 includes Broadcom STB GPIO IP as well as
> Broadcom 2712 pin controller IP.
>
> The community has expressed interest in linking the two drivers
> with the "gpio-ranges" property in device tree. This commit
> stack implements the necessary changes.
>
> [...]
Applied, thanks!
[1/3] dt-bindings: gpio: brcmstb: add gpio-ranges
commit: 7c66f8173360556ac0c3c38a91234af5a0a5a4a9
[2/3] gpio: of: support gpio-ranges for multiple gpiochip devices
commit: e818cd3c8a345c046edff00b5ad0be4d39f7e4d4
[3/3] gpio: brcmstb: add support for gpio-ranges
commit: 5539287ca65686f478e058a1e939294cb5682426
Best regards,
--
Bartosz Golaszewski <[email protected]>
On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <[email protected]> wrote:
> Add optional gpio-ranges device-tree property to the Broadcom
> Set-Top-Box GPIO controller.
>
> Signed-off-by: Doug Berger <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
Hi Dough,
thanks for your patch!
I'm a bit confused here:
On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <[email protected]> wrote:
> + /* Ignore ranges outside of this GPIO chip */
> + if (pinspec.args[0] >= (chip->offset + chip->ngpio))
> + continue;
> + if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
> + continue;
Here pinspec.args[0] and [2] comes directly from the device tree.
The documentation in Documentation/devicetree/bindings/gpio/gpio.txt
says:
> 2.2) Ordinary (numerical) GPIO ranges
> -------------------------------------
>
> It is useful to represent which GPIOs correspond to which pins on which pin
> controllers. The gpio-ranges property described below represents this with
> a discrete set of ranges mapping pins from the pin controller local number space
> to pins in the GPIO controller local number space.
>
> The format is: <[pin controller phandle], [GPIO controller offset],
> [pin controller offset], [number of pins]>;
>
> The GPIO controller offset pertains to the GPIO controller node containing the
> range definition.
So I do not understand how pinspec[0] and [2] can ever be compared
to something involving chip->offset which is a Linux-specific offset.
It rather looks like you are trying to accomodate the Linux numberspace
in the ranges, which it was explicitly designed to avoid.
I just don't get it.
So NACK until I understand what is going on here.
Yours,
Linus Walleij
On Fri, Apr 26, 2024 at 9:33 AM Bartosz Golaszewski <[email protected]> wrote:
> [2/3] gpio: of: support gpio-ranges for multiple gpiochip devices
> commit: e818cd3c8a345c046edff00b5ad0be4d39f7e4d4
I'm not sure this is a good idea. To me it looks like the commit violates
the device tree bindings, which says offsets are on the local GPIO chip
and turn them into offsets in the Linux GPIO numberspace.
Yours,
Linus Walleij
On 5/3/2024 1:25 AM, Linus Walleij wrote:
> Hi Dough,
>
> thanks for your patch!
Thanks for your review!
>
> I'm a bit confused here:
"Communication is hard" and I may be confused about your confusion, but
hopefully we can work it out.
>
> On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <[email protected]> wrote:
>
>
>> + /* Ignore ranges outside of this GPIO chip */
>> + if (pinspec.args[0] >= (chip->offset + chip->ngpio))
>> + continue;
>> + if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
>> + continue;
>
> Here pinspec.args[0] and [2] comes directly from the device tree.
>
> The documentation in Documentation/devicetree/bindings/gpio/gpio.txt
> says:
>
>> 2.2) Ordinary (numerical) GPIO ranges
>> -------------------------------------
>>
>> It is useful to represent which GPIOs correspond to which pins on which pin
>> controllers. The gpio-ranges property described below represents this with
>> a discrete set of ranges mapping pins from the pin controller local number space
>> to pins in the GPIO controller local number space.
>>
>> The format is: <[pin controller phandle], [GPIO controller offset],
>> [pin controller offset], [number of pins]>;
>>
>> The GPIO controller offset pertains to the GPIO controller node containing the
>> range definition.
I think we are in agreement here. For extra clarity, I will add that in
my understanding pinspec.args[0] corresponds to [GPIO controller offset]
and pinspec.args[2] corresponds to [number of pins].
>
> So I do not understand how pinspec[0] and [2] can ever be compared
> to something involving chip->offset which is a Linux-specific offset.
>
> It rather looks like you are trying to accomodate the Linux numberspace
> in the ranges, which it was explicitly designed to avoid.
The struct gpio_chip documentation in include/linux/gpio/driver.h says:
> * @offset: when multiple gpio chips belong to the same device this
> * can be used as offset within the device so friendly names can
> * be properly assigned.
It is my understanding that this value represents the offset of a
gpiochip relative to the GPIO controller device defined by the GPIO
controller node in device tree. This puts it in the same number space as
[GPIO controller offset]. I believe it was introduced for the specific
purpose of translating [GPIO controller offset] values into
Linux-specific offsets, which is why it is being reused for that purpose
in this patch.
For GPIO Controllers that contain a single gpiochip the 'offset' member
is 0 and the device tree node offsets can be applied directly to the
gpiochip. However, when a GPIO Controller contains multiple gpiochips,
the device tree node offsets must be translated to each individual gpiochip.
>
> I just don't get it.
>
> So NACK until I understand what is going on here.
>
> Yours,
> Linus Walleij
I hope it makes sense now, but if not please help me understand what I
may be missing.
Thanks,
Doug
On Fri, May 3, 2024 at 10:21 PM Doug Berger <[email protected]> wrote:
>
> On 5/3/2024 1:25 AM, Linus Walleij wrote:
> > Hi Dough,
> >
> > thanks for your patch!
> Thanks for your review!
>
> >
> > I'm a bit confused here:
> "Communication is hard" and I may be confused about your confusion, but
> hopefully we can work it out.
>
> >
> > On Wed, Apr 24, 2024 at 8:51 PM Doug Berger <[email protected]> wrote:
> >
> >
> >> + /* Ignore ranges outside of this GPIO chip */
> >> + if (pinspec.args[0] >= (chip->offset + chip->ngpio))
> >> + continue;
> >> + if (pinspec.args[0] + pinspec.args[2] <= chip->offset)
> >> + continue;
> >
> > Here pinspec.args[0] and [2] comes directly from the device tree.
> >
> > The documentation in Documentation/devicetree/bindings/gpio/gpio.txt
> > says:
> >
> >> 2.2) Ordinary (numerical) GPIO ranges
> >> -------------------------------------
> >>
> >> It is useful to represent which GPIOs correspond to which pins on which pin
> >> controllers. The gpio-ranges property described below represents this with
> >> a discrete set of ranges mapping pins from the pin controller local number space
> >> to pins in the GPIO controller local number space.
> >>
> >> The format is: <[pin controller phandle], [GPIO controller offset],
> >> [pin controller offset], [number of pins]>;
> >>
> >> The GPIO controller offset pertains to the GPIO controller node containing the
> >> range definition.
> I think we are in agreement here. For extra clarity, I will add that in
> my understanding pinspec.args[0] corresponds to [GPIO controller offset]
> and pinspec.args[2] corresponds to [number of pins].
>
> >
> > So I do not understand how pinspec[0] and [2] can ever be compared
> > to something involving chip->offset which is a Linux-specific offset.
> >
> > It rather looks like you are trying to accomodate the Linux numberspace
> > in the ranges, which it was explicitly designed to avoid.
> The struct gpio_chip documentation in include/linux/gpio/driver.h says:
>
> > * @offset: when multiple gpio chips belong to the same device this
> > * can be used as offset within the device so friendly names can
> > * be properly assigned.
>
> It is my understanding that this value represents the offset of a
> gpiochip relative to the GPIO controller device defined by the GPIO
> controller node in device tree. This puts it in the same number space as
> [GPIO controller offset]. I believe it was introduced for the specific
> purpose of translating [GPIO controller offset] values into
> Linux-specific offsets, which is why it is being reused for that purpose
> in this patch.
>
> For GPIO Controllers that contain a single gpiochip the 'offset' member
> is 0 and the device tree node offsets can be applied directly to the
> gpiochip. However, when a GPIO Controller contains multiple gpiochips,
> the device tree node offsets must be translated to each individual gpiochip.
>
> >
> > I just don't get it.
> >
> > So NACK until I understand what is going on here.
> >
> > Yours,
> > Linus Walleij
> I hope it makes sense now, but if not please help me understand what I
> may be missing.
>
> Thanks,
> Doug
>
Linus,
Please let me know if this is still a NAK, if so, I'll drop this
series from my tree at least for this release.
Bart
On Fri, May 3, 2024 at 10:21 PM Doug Berger <[email protected]> wrote:
> On 5/3/2024 1:25 AM, Linus Walleij wrote:
> > It rather looks like you are trying to accomodate the Linux numberspace
> > in the ranges, which it was explicitly designed to avoid.
> The struct gpio_chip documentation in include/linux/gpio/driver.h says:
>
> > * @offset: when multiple gpio chips belong to the same device this
> > * can be used as offset within the device so friendly names can
> > * be properly assigned.
>
> It is my understanding that this value represents the offset of a
> gpiochip relative to the GPIO controller device defined by the GPIO
> controller node in device tree. This puts it in the same number space as
> [GPIO controller offset]. I believe it was introduced for the specific
> purpose of translating [GPIO controller offset] values into
> Linux-specific offsets, which is why it is being reused for that purpose
> in this patch.
You're right, I had it confused with .base!
It's because I missed it completely when this new property was added
in 2021.
Sorry for the fuzz.
Yours,
Linus Walleij
On Sun, May 5, 2024 at 2:25 PM Bartosz Golaszewski <[email protected]> wrote:
> Please let me know if this is still a NAK, if so, I'll drop this
> series from my tree at least for this release.
No I was just wrong, I had missed Sergio's addition of local
offset for several-gpiochip-per-device handling completely
and confused .offset with .base...
Yours,
Linus Walleij