| Message ID | 20201004162908.3216898-1-martin.blumenstingl@googlemail.com |
|---|---|
| Headers | show |
| Series | GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers | expand |
On Sun, Oct 04, 2020 at 06:29:07PM +0200, Martin Blumenstingl wrote: > Etron EJ168/EJ188/EJ198 are USB xHCI host controllers which embed a GPIO > controller. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > .../devicetree/bindings/gpio/etron,ej1x8.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml > > diff --git a/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml b/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml > new file mode 100644 > index 000000000000..fa554045bdb5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpio/etron,ej1x8.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: GPIO controller embedded into the EJ168/EJ188/EJ198 xHCI controllers > + > +maintainers: > + - Martin Blumenstingl <martin.blumenstingl@googlemail.com> > + > +properties: > + compatible: > + enum: > + - pci1b6f,7023 > + - pci1b6f,7052 > + > + reg: > + maxItems: 1 > + > + "#gpio-cells": > + const: 2 > + > + gpio-controller: true > + > +required: > + - compatible > + - reg > + - "#gpio-cells" > + - gpio-controller > + > +additionalProperties: false > + > +examples: > + - | > + pcie { > + #address-cells = <3>; > + #size-cells = <2>; > + > + gpio@0,0,0 { > + compatible = "pci1b6f,7023"; > + reg = <0x0 0x0 0x0 0x0 0x1000>; > + gpio-controller; > + #gpio-cells = <2>; > + }; How would this look if you needed to describe the XHCI controller? That's another PCI function? > + }; > + > +... > -- > 2.28.0 >
On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Add the vendor ID for Etron Technology, Inc. as well as the device IDs > for the two USB xHCI controllers EJ168 and EJ188. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> (...) > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > +#define PCI_VENDOR_ID_ETRON 0x1b6f > +#define PCI_DEVICE_ID_ETRON_EJ168 0x7023 > +#define PCI_DEVICE_ID_ETRON_EJ188 0x7052 If you're defining that here, I think it should also be removed in drivers/usb/host/xhci-pci.c by including this file instead? Yours, Linus Walleij
On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > +properties: > + compatible: > + enum: > + - pci1b6f,7023 > + - pci1b6f,7052 I think it is better to let the PCI driver for the whole hardware in drivers/usb/host/xhci-pci.c probe from the PCI configuration space numbers, and then add a gpio_chip to xhci-pci.c. Thanks! Linus Walleij
Hi Linus, On Wed, Oct 7, 2020 at 11:14 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > > Add the vendor ID for Etron Technology, Inc. as well as the device IDs > > for the two USB xHCI controllers EJ168 and EJ188. > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > (...) > > > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > > > +#define PCI_VENDOR_ID_ETRON 0x1b6f > > +#define PCI_DEVICE_ID_ETRON_EJ168 0x7023 > > +#define PCI_DEVICE_ID_ETRON_EJ188 0x7052 > > If you're defining that here, I think it should also be > removed in > drivers/usb/host/xhci-pci.c > by including this file instead? you are absolutely right - I missed that part I will change this in v2 - thanks for pointing it out! Best regards, Martin
Hi Rob, On Tue, Oct 6, 2020 at 11:25 PM Rob Herring <robh@kernel.org> wrote: [...] > > +examples: > > + - | > > + pcie { > > + #address-cells = <3>; > > + #size-cells = <2>; > > + > > + gpio@0,0,0 { > > + compatible = "pci1b6f,7023"; > > + reg = <0x0 0x0 0x0 0x0 0x1000>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > How would this look if you needed to describe the XHCI controller? > That's another PCI function? unfortunately I have no documentation about this the "reference driver" is poking the PCI config space registers of the xHCI controller - without any dedicated function for this Best regards, Martin
Hi Linus, On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > > +properties: > > + compatible: > > + enum: > > + - pci1b6f,7023 > > + - pci1b6f,7052 > > I think it is better to let the PCI driver for the whole hardware in > drivers/usb/host/xhci-pci.c probe from the PCI configuration space > numbers, and then add a gpio_chip to xhci-pci.c. to have everything consistent I will move the binding to Documentation/devicetree/bindings/usb Best regards, Martin
On Wed, Oct 7, 2020 at 9:58 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> wrote: > > > > > +properties: > > > + compatible: > > > + enum: > > > + - pci1b6f,7023 > > > + - pci1b6f,7052 > > > > I think it is better to let the PCI driver for the whole hardware in > > drivers/usb/host/xhci-pci.c probe from the PCI configuration space > > numbers, and then add a gpio_chip to xhci-pci.c. > > to have everything consistent I will move the binding to > Documentation/devicetree/bindings/usb I do not understand why a PCI device would need a DT binding at all. They just probe from the magic number in the PCI config space, they spawn struct pci_dev PCI devices, not the type of platform devices coming out of the DT parser code. No DT compatible needed. What am I not understanding about this? Yours, Linus Walleij
On Tue, Oct 13, 2020 at 8:27 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Oct 7, 2020 at 9:58 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl > > > <martin.blumenstingl@googlemail.com> wrote: > > > > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - pci1b6f,7023 > > > > + - pci1b6f,7052 > > > > > > I think it is better to let the PCI driver for the whole hardware in > > > drivers/usb/host/xhci-pci.c probe from the PCI configuration space > > > numbers, and then add a gpio_chip to xhci-pci.c. > > > > to have everything consistent I will move the binding to > > Documentation/devicetree/bindings/usb > > I do not understand why a PCI device would need a DT binding > at all. They just probe from the magic number in the PCI > config space, they spawn struct pci_dev PCI devices, not the > type of platform devices coming out of the DT parser code. > No DT compatible needed. Same reason for all the discoverable buses need bindings. There can be parts that are not discoverable or connections with non-discoverable nodes. There's also cases where the discoverable device has to be powered, reset deasserted, clocks enabled, etc. first to be discovered. If the GPIOs here had connections elsewhere in the DT, then we have to describe the provider in DT. Rob
Hi Linus, On Wed, Oct 14, 2020 at 2:43 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Tue, Oct 13, 2020 at 8:27 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Wed, Oct 7, 2020 at 9:58 PM Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> wrote: > > > On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl > > > > <martin.blumenstingl@googlemail.com> wrote: > > > > > > > > > +properties: > > > > > + compatible: > > > > > + enum: > > > > > + - pci1b6f,7023 > > > > > + - pci1b6f,7052 > > > > > > > > I think it is better to let the PCI driver for the whole hardware in > > > > drivers/usb/host/xhci-pci.c probe from the PCI configuration space > > > > numbers, and then add a gpio_chip to xhci-pci.c. > > > > > > to have everything consistent I will move the binding to > > > Documentation/devicetree/bindings/usb > > > > I do not understand why a PCI device would need a DT binding > > at all. They just probe from the magic number in the PCI > > config space, they spawn struct pci_dev PCI devices, not the > > type of platform devices coming out of the DT parser code. > > No DT compatible needed. > > Same reason for all the discoverable buses need bindings. There can be > parts that are not discoverable or connections with non-discoverable > nodes. There's also cases where the discoverable device has to be > powered, reset deasserted, clocks enabled, etc. first to be > discovered. > > If the GPIOs here had connections elsewhere in the DT, then we have to > describe the provider in DT. this is exactly what I need it for: that platform has hand-written .dts files and I need to wire up a GPIO LED Best regards, Martin
On Fri, Oct 16, 2020 at 10:52 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > On Wed, Oct 14, 2020 at 2:43 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Tue, Oct 13, 2020 at 8:27 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > I do not understand why a PCI device would need a DT binding > > > at all. They just probe from the magic number in the PCI > > > config space, they spawn struct pci_dev PCI devices, not the > > > type of platform devices coming out of the DT parser code. > > > No DT compatible needed. > > > > Same reason for all the discoverable buses need bindings. There can be > > parts that are not discoverable or connections with non-discoverable > > nodes. There's also cases where the discoverable device has to be > > powered, reset deasserted, clocks enabled, etc. first to be > > discovered. > > > > If the GPIOs here had connections elsewhere in the DT, then we have to > > describe the provider in DT. > > this is exactly what I need it for: that platform has hand-written > .dts files and I need to wire up a GPIO LED Aha I get it! OK then :) Yours, Linus Walleij
Hello, I have a "Belkin F9K115v2" (wifi router) [0]. It comes with an Etron EJ168 xHCI controllers soldered to the board. One of the LEDs on this device is connected to one of the four GPIO lines provided by the Etron xHCI controller. The goal of this series to add support for the GPIO controller on the Etron EJ168/EJ188/EJ198 controllers. Unfortunately there's no (public) datasheet available. I have Cc'ed Etron and I'm hoping that they can either provide a datasheet or at least some code-review feedback. Instead I used the GPL tarball [0] for this device. Inside this tarball the relevant "reference" code is in: linux/kernels/mips-linux-2.6.31/drivers/usb/host/etxhci-pci.c Unfortunately it uses magic numbers for the registers instead of human-readable names. The register names are what I came up with. For reference, I have tested this on a patched OpenWrt build with the following .dts changes (I am showing these here so it will be easier to review the whole series): &pcie1 { status = "okay"; xhci: usb-controller@0,0,0 { compatible = "pci1b6f,7023"; reg = <0x0 0x0 0x0 0x0 0x1000>; #address-cells = <1>; #size-cells = <0>; gpio-controller; #gpio-cells = <2>; xhci_port0: port@1 { reg = <1>; #trigger-source-cells = <0>; }; }; }; leds { compatible = "gpio-leds"; usb3 { label = "green:usb3"; gpios = <&xhci 2 GPIO_ACTIVE_LOW>; trigger-sources = <&xhci_port0>; linux,default-trigger = "usbport"; }; }; In general I followed [2] because it says: PCI drivers should have a really good reason for not using the pci_register_driver() [...] The main reason [...] is because one PCI device implements several different HW services. My understanding that my driver fits into this category. I am sending this as RFC because this is my first self-written GPIO driver as well as my first PCI device driver. Any feedback is welcome! Best regards, Martin [0] https://openwrt.org/toh/belkin/f9k1115v2 [1] https://www.belkin.com/support/dl/F9K1115v2.03.97-GPL-10.2.85.tar.gz [2] https://www.kernel.org/doc/html/latest/PCI/pci.html#how-to-find-pci-devices-manually Martin Blumenstingl (3): PCI: Add the IDs for Etron EJ168 and EJ188 dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 .../devicetree/bindings/gpio/etron,ej1x8.yaml | 48 +++ drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-ej1x8.c | 311 ++++++++++++++++++ include/linux/pci_ids.h | 4 + 5 files changed, 373 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml create mode 100644 drivers/gpio/gpio-ej1x8.c