| Message ID | cover.1620735871.git.sander@svanheule.net |
|---|---|
| Headers | show |
| Series | RTL8231 GPIO expander support | expand |
On Wed, 2021-05-12 at 18:29 +0300, Andy Shevchenko wrote: > > > On Tuesday, May 11, 2021, Sander Vanheule <sander@svanheule.net> wrote: > > The RTL8231 GPIO and LED expander can be configured for use as an MDIO or > > SMI > > bus device. Currently only the MDIO mode is supported, although SMI mode > > support should be fairly straightforward, once an SMI bus driver is > > available. > > > > Provided features by the RTL8231: > > - Up to 37 GPIOs > > - Configurable drive strength: 8mA or 4mA (currently unsupported) > > - Input debouncing on high GPIOs (currently unsupported) > > - Up to 88 LEDs in multiple scan matrix groups > > - On, off, or one of six toggling intervals > > - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs > > - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs > > - Up to one PWM output (currently unsupported) > > - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz) > > > > There remain some log warnings when probing the device, possibly due to the > > way > > I'm using the MFD subsystem. Would it be possible to avoid these? > > [ 2.602242] rtl8231-pinctrl: Failed to locate of_node [id: -2] > > [ 2.609380] rtl8231-pinctrl rtl8231-pinctrl.0.auto: no of_node; not > > parsing pinctrl DT > > > > When no 'leds' sub-node is specified: > > [ 2.922262] rtl8231-leds: Failed to locate of_node [id: -2] > > [ 2.967149] rtl8231-leds rtl8231-leds.1.auto: no of_node; not parsing > > pinctrl DT > > [ 2.975673] rtl8231-leds rtl8231-leds.1.auto: scan mode missing or > > invalid > > [ 2.983531] rtl8231-leds: probe of rtl8231-leds.1.auto failed with error > > -22 > > > > > > > I have several comments to the series, but I may give them next week. > > Just couple here: > 1. If subsystem provides a regmap API I would suggest to use it, I.o.w. try > again to understand what is wrong with MDIO case. Are you referring to the MDIO regmap interface, or the GPIO regmap interface? For the MDIO regmap interface, I have been able to resolve the Kconfig dependency issue. So I can reintroduce that, if that's preferred over the solution in this v1. With an extra patch, I was able to use the gpio-regmap interface, dropping most of the GPIO code. The current gpio-regmap implementation makes the assumption that an output value can be set while a pin is configured as an input. That assumption is invalid for this chip, so I had to provide an extra flag for gpio_regmap_config, similar to how this is handled in gpio-mmio. > 2. Please, switch to fwnode API in LED driver Since you had the same comment on my previous patch set, I had already tried to this this into account as much as possible. There's a few things I couldn't find the fwnode-equivalent for: * I use of_node_name_prefix to enforce the naming required by the binding. I could just walk over all (available) child nodes, which would be mostly equivalent. * To get the address of an LED child node, I use of_get_address, since this appeared to provide what I want to do: get the address of the node. I know next to nothing about ACPI. Does the equivalent exist there? Or am I taking the wrong approach? I have updated patches ready, if you would rather just review a v2. Best, Sander
On Mon, May 17, 2021 at 12:40 AM Sander Vanheule <sander@svanheule.net> wrote: > On Wed, 2021-05-12 at 18:29 +0300, Andy Shevchenko wrote: > > On Tuesday, May 11, 2021, Sander Vanheule <sander@svanheule.net> wrote: ... > > I have several comments to the series, but I may give them next week. > > > > Just couple here: > > 1. If subsystem provides a regmap API I would suggest to use it, I.o.w. try > > again to understand what is wrong with MDIO case. > > Are you referring to the MDIO regmap interface, or the GPIO regmap interface? MDIO > For the MDIO regmap interface, I have been able to resolve the Kconfig > dependency issue. So I can reintroduce that, if that's preferred over the > solution in this v1. > > With an extra patch, I was able to use the gpio-regmap interface, dropping most > of the GPIO code. The current gpio-regmap implementation makes the assumption > that an output value can be set while a pin is configured as an input. That > assumption is invalid for this chip, so I had to provide an extra flag for > gpio_regmap_config, similar to how this is handled in gpio-mmio. > > > > 2. Please, switch to fwnode API in LED driver > > Since you had the same comment on my previous patch set, I had already tried to > this this into account as much as possible. > > There's a few things I couldn't find the fwnode-equivalent for: > * I use of_node_name_prefix to enforce the naming required by the binding. I > could just walk over all (available) child nodes, which would be mostly > equivalent. AFAIU the LED traditional bindings is that you define LED compatible nodes and all child nodes of it are the one-per-LED ones, there shouldn't be others. > * To get the address of an LED child node, I use of_get_address, since this > appeared to provide what I want to do: get the address of the node. I know > next to nothing about ACPI. Does the equivalent exist there? Or am I taking > the wrong approach? What are the means of an address in this case? > I have updated patches ready, if you would rather just review a v2. -- With Best Regards, Andy Shevchenko
On Mon, 2021-05-17 at 11:13 +0300, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 12:40 AM Sander Vanheule <sander@svanheule.net> wrote: > > On Wed, 2021-05-12 at 18:29 +0300, Andy Shevchenko wrote: > > > On Tuesday, May 11, 2021, Sander Vanheule <sander@svanheule.net> wrote: > > ... > > > > * > > > I have several comments to the series, but I may give them next > > > week. > > > > > > Just couple here: > > > 1. If subsystem provides a regmap API I would suggest to use it, I.o.w. > > > try > > > again to understand what is wrong with MDIO case. > > > > Are you referring to the MDIO regmap interface, or the GPIO regmap > > interface? > > MDIO > > > For the MDIO regmap interface, I have been able to resolve the Kconfig > > dependency issue. So I can reintroduce that, if that's preferred over the > > solution in this v1. > > > > With an extra patch, I was able to use the gpio-regmap interface, dropping > > most > > of the GPIO code. The current gpio-regmap implementation makes the > > assumption > > that an output value can be set while a pin is configured as an input. That > > assumption is invalid for this chip, so I had to provide an extra flag for > > gpio_regmap_config, similar to how this is handled in gpio-mmio. > > > > > > > 2. Please, switch to fwnode API in LED driver > > > > Since you had the same comment on my previous patch set, I had already tried > > to > > this this into account as much as possible. > > > > There's a few things I couldn't find the fwnode-equivalent for: > > * I use of_node_name_prefix to enforce the naming required by the binding. > > I > > could just walk over all (available) child nodes, which would be mostly > > equivalent. > > AFAIU the LED traditional bindings is that you define LED compatible > nodes and all child nodes of it are the one-per-LED ones, there > shouldn't be others. OK, then I can just iterate over all child fwnodes. > > * To get the address of an LED child node, I use of_get_address, since this > > appeared to provide what I want to do: get the address of the node. I > > know > > next to nothing about ACPI. Does the equivalent exist there? Or am I > > taking > > the wrong approach? > > What are the means of an address in this case? The chip appears to be intended for use with ethernet switches. The registers are organised to into a few groups, to provide 2 or 3 status LEDs per switch port: * "LED0" group for 32 ports, * "LED1" group for 32 ports, * "LED2" group for 24 ports The number of LEDs that can be used depends on the output mode, so I use a two- part <#PORT #LED> address, resembling how this is defined by Realtek. A single linear LED address space would get awkward gaps in bi-color mode (where only the lower 24 ports can be used), but would still require addresses to be able to specify which LED is where. For example in case the user want to link them to a phy trigger for a specific switch port. Best, Sander
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI
bus device. Currently only the MDIO mode is supported, although SMI mode
support should be fairly straightforward, once an SMI bus driver is available.
Provided features by the RTL8231:
- Up to 37 GPIOs
- Configurable drive strength: 8mA or 4mA (currently unsupported)
- Input debouncing on high GPIOs (currently unsupported)
- Up to 88 LEDs in multiple scan matrix groups
- On, off, or one of six toggling intervals
- "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
- "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
- Up to one PWM output (currently unsupported)
- Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)
Register access is provided through a new MDIO regmap provider. The GPIO
controller uses gpio-regmap, although a patch is required to support a
limitation of the chip.
There remain some log warnings when probing the device, possibly due to the way
I'm using the MFD subsystem. Would it be possible to avoid these?
[ 2.602242] rtl8231-pinctrl: Failed to locate of_node [id: -2]
[ 2.609380] rtl8231-pinctrl rtl8231-pinctrl.0.auto: no of_node; not parsing pinctrl DT
When no 'leds' sub-node is specified:
[ 2.922262] rtl8231-leds: Failed to locate of_node [id: -2]
[ 2.967149] rtl8231-leds rtl8231-leds.1.auto: no of_node; not parsing pinctrl DT
[ 2.975673] rtl8231-leds rtl8231-leds.1.auto: scan mode missing or invalid
[ 2.983531] rtl8231-leds: probe of rtl8231-leds.1.auto failed with error -22
Changes since v1:
- Reintroduce MDIO regmap, with fixed Kconfig dependencies
- Add configurable dir/value order for gpio-regmap direction_out call
- Drop allocations for regmap fields that are used only on init
- Move some definitions to MFD header
- Add PM ops to replace driver remove for MFD
- Change pinctrl driver to (modified) gpio-regmap
- Change leds driver to use fwnode
Link: https://lore.kernel.org/lkml/cover.1620735871.git.sander@svanheule.net/
Changes since RFC:
- Dropped MDIO regmap interface. I was unable to resolve the Kconfig
dependency issue, so have reverted to using regmap_config.reg_read/write.
- Added pinctrl support
- Added LED support
- Changed root device to MFD, with pinctrl and leds child devices. Root
device is now an mdio_device driver.
Link: https://lore.kernel.org/linux-gpio/cover.1617914861.git.sander@svanheule.net/
Sander Vanheule (7):
regmap: Add MDIO bus support
gpio: regmap: Add configurable dir/value order
dt-bindings: leds: Binding for RTL8231 scan matrix
dt-bindings: mfd: Binding for RTL8231
mfd: Add RTL8231 core device
pinctrl: Add RTL8231 pin control and GPIO support
leds: Add support for RTL8231 LED scan matrix
.../bindings/leds/realtek,rtl8231-leds.yaml | 159 ++++++++
.../bindings/mfd/realtek,rtl8231.yaml | 202 ++++++++++
drivers/base/regmap/Kconfig | 6 +-
drivers/base/regmap/Makefile | 1 +
drivers/base/regmap/regmap-mdio.c | 57 +++
drivers/gpio/gpio-regmap.c | 20 +-
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-rtl8231.c | 293 ++++++++++++++
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 1 +
drivers/mfd/rtl8231.c | 153 +++++++
drivers/pinctrl/Kconfig | 11 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-rtl8231.c | 377 ++++++++++++++++++
include/linux/gpio/regmap.h | 3 +
include/linux/mfd/rtl8231.h | 57 +++
include/linux/regmap.h | 36 ++
18 files changed, 1393 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml
create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
create mode 100644 drivers/base/regmap/regmap-mdio.c
create mode 100644 drivers/leds/leds-rtl8231.c
create mode 100644 drivers/mfd/rtl8231.c
create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
create mode 100644 include/linux/mfd/rtl8231.h
On Sun, 2021-05-16 at 23:40 +0200, Sander Vanheule wrote: > > 2. Please, switch to fwnode API in LED driver > > Since you had the same comment on my previous patch set, I had already tried to > this this into account as much as possible. > > There's a few things I couldn't find the fwnode-equivalent for: > * I use of_node_name_prefix to enforce the naming required by the binding. I > could just walk over all (available) child nodes, which would be mostly > equivalent. > * To get the address of an LED child node, I use of_get_address, since this > appeared to provide what I want to do: get the address of the node. I know > next to nothing about ACPI. Does the equivalent exist there? Or am I taking > the wrong approach? Hi Andy, I found this: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/leds.html So instead of of_address, I now just read the "reg" property. The v2 I just sent should be fwnode-only. Best, Sander
On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > Both single and bi-color scanning modes are supported. The driver will > verify that the addresses are valid for the current mode, before > registering the LEDs. LEDs can be turned on, off, or toggled at one of > six predefined rates from 40ms to 1280ms. > > Implements a platform device for use as child device with RTL8231 MFD, as a child > and uses the parent regmap to access the required registers. ... > + When built as a module, this module will be named rtl8231_leds. Again, what it's written here is not what is in Makefile. > +obj-$(CONFIG_LEDS_RTL8231) += leds-rtl8231.o ... > +/** > + * struct led_toggle_rate - description of an LED blinking mode > + * @interval: LED toggle rate in ms > + * @mode: Register field value used to active this mode activate > + * > + * For LED hardware accelerated blinking, with equal on and off delay. > + * Both delays are given by @interval, so the interval at which the LED blinks > + * (i.e. turn on and off once) is double this value. > + */ ... > +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled) > +{ > + unsigned int mode; > + unsigned int i = 0; This... > + if (regmap_field_read(pled->reg_field, &mode)) > + return 0; > + > + while (i < pled->modes->num_toggle_rates && mode != pled->modes->toggle_rates[i].mode) > + i++; ...and this will be better as a for-loop. > + if (i < pled->modes->num_toggle_rates) > + return pled->modes->toggle_rates[i].interval; > + else Redundant. > + return 0; > +} ... > + unsigned int i = 0; As per above. ... > + interval = 500; interval_ms > + /* > + * If the current mode is blinking, choose the delay that (likely) changed. > + * Otherwise, choose the interval that would have the same total delay. > + */ > + interval = rtl8231_led_current_interval(pled); > + Redundant blank line. > + if (interval > 0 && interval == *delay_off) > + interval = *delay_on; > + else if (interval > 0 && interval == *delay_on) > + interval = *delay_off; > + else > + interval = (*delay_on + *delay_off) / 2; > + } ... > + u32 addr[2]; > + int err; > + > + if (!fwnode_property_count_u32(fwnode, "reg")) err = fwnode_property_count_u32(...); if (err < 0) return err; if (err == 0) return -ENODEV; > + return -ENODEV; > + > + err = fwnode_property_read_u32_array(fwnode, "reg", addr, ARRAY_SIZE(addr)); If count returns 1? What's the point of counting if you always want two? > + if (err) > + return err; > + > + *addr_port = addr[0]; > + *addr_led = addr[1]; > + > + return 0; > +} ... > + pled = devm_kzalloc(dev, sizeof(*pled), GFP_KERNEL); > + if (IS_ERR(pled)) Wrong. > + return PTR_ERR(pled); ... > + err = rtl8231_led_read_address(fwnode, &port_index, &led_index); > + Redundant blank line. > + if (err) { > + dev_err(dev, "LED address invalid\n"); > + return err; > + } else if (led_index >= RTL8231_NUM_LEDS || port_index >= port_counts[led_index]) { Redundant 'else' > + dev_err(dev, "LED address (%d.%d) invalid\n", port_index, led_index); > + return -ENODEV; > + } ... > + map = dev_get_regmap(dev->parent, NULL); > + if (IS_ERR_OR_NULL(map)) { Split it into two conditionals. > + dev_err(dev, "failed to retrieve regmap\n"); > + if (!map) > + return -ENODEV; > + else > + return PTR_ERR(map); > + } ... > + if (!device_property_match_string(dev, "realtek,led-scan-mode", "single-color")) { It seems that device_property_match_string() and accompanying functions have wrong description of returned codes, i.e. it returns the index of the matched string. It's possible that some APIs are broken (but I believe that the former is the case). That said, I think the proper comparison should be >= 0. > + port_counts = rtl8231_led_port_counts_single; > + regmap_update_bits(map, RTL8231_REG_FUNC0, > + RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_SINGLE); > + } else if (!device_property_match_string(dev, "realtek,led-scan-mode", "bi-color")) { Ditto. > + port_counts = rtl8231_led_port_counts_bicolor; > + regmap_update_bits(map, RTL8231_REG_FUNC0, > + RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_BICOLOR); > + } else { > + dev_err(dev, "scan mode missing or invalid\n"); > + return -EINVAL; > + }
On Tue, May 11, 2021 at 02:25:20PM +0200, Sander Vanheule wrote: > Add a binding description for the Realtek RTL8231, a GPIO and LED > expander chip commonly used in ethernet switches based on a Realtek > switch SoC. These chips can be addressed via an MDIO or SMI bus, or used > as a plain 36-bit shift register. > > This binding only describes the feature set provided by the MDIO/SMI > configuration, and covers the GPIO, PWM, and pin control properties. The > LED properties are defined in a separate binding. > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > --- > .../bindings/mfd/realtek,rtl8231.yaml | 202 ++++++++++++++++++ > 1 file changed, 202 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml > new file mode 100644 > index 000000000000..2023cfa887a3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml > @@ -0,0 +1,202 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/realtek,rtl8231.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Realtek RTL8231 GPIO and LED expander. > + > +maintainers: > + - Sander Vanheule <sander@svanheule.net> > + > +description: | > + The RTL8231 is a GPIO and LED expander chip, providing up to 37 GPIOs, up to > + 88 LEDs, and up to one PWM output. This device is frequently used alongside > + Realtek switch SoCs, to provide additional I/O capabilities. > + > + To manage the RTL8231's features, its strapping pins can be used to configure > + it in one of three modes: shift register, MDIO device, or SMI device. The > + shift register mode does not need special support. In MDIO or SMI mode, most > + pins can be configured as a GPIO output, LED matrix scan line/column, or as a > + PWM output. > + > + The GPIO and pin control are part of the main node. PWM and LED support are > + configured as sub-nodes. > + > +properties: > + compatible: > + const: realtek,rtl8231 > + > + reg: > + description: MDIO or SMI device address. > + maxItems: 1 > + > + # GPIO support > + gpio-controller: true > + > + "#gpio-cells": > + const: 2 > + description: | > + The first cell is the pin number and the second cell is used to specify > + the gpio active state. > + > + gpio-ranges: > + description: | > + Must reference itself, and provide a zero-based mapping for 37 pins. > + maxItems: 1 > + > + # Pin muxing and configuration > + realtek,drive-strength: > + $ref: /schemas/types.yaml#/definitions/uint32 Use the standard 'drive-strength' property. > + description: | > + Common drive strength used for all GPIO output pins, must be 4mA or 8mA. > + On reset, this value will default to 8mA. > + enum: [4, 8] > + > + # LED scanning matrix > + leds: > + $ref: ../leds/realtek,rtl8231-leds.yaml# > + > + # PWM output > + pwm: > + type: object > + description: | > + Subnode describing the PWM peripheral. To use the PWM output, gpio35 must > + be muxed to its 'pwm' function. Valid frequency values for consumers are > + 1200, 1600, 2000, 2400, 2800, 3200, 4000, and 4800. > + > + properties: > + "#pwm-cells": > + description: | > + Twos cells with PWM index (must be 0) and PWM frequency in Hz. > + const: 2 > + > + required: > + - "#pwm-cells" Just move this to the parent node. No reason for a child node or that 1 node can't be 2 providers. > + > +patternProperties: > + "-pins$": > + type: object > + $ref: ../pinctrl/pinmux-node.yaml# > + > + properties: > + pins: > + items: > + oneOf: No need for oneOf when there's only 1 entry. > + - enum: ["gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", > + "gpio7", "gpio8", "gpio9", "gpio10", "gpio11", "gpio12", "gpio13", > + "gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20", > + "gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27", > + "gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34", > + "gpio35", "gpio36"] > + minItems: 1 > + maxItems: 37 > + function: > + description: | > + Select which function to use. "gpio" is supported for all pins, "led" is supported > + for pins 0-34, "pwm" is supported for for pin 35. > + enum: ["gpio", "led", "pwm"] > + > + required: > + - pins > + - function > + > +required: > + - compatible > + - reg > + - gpio-controller > + - "#gpio-cells" > + - gpio-ranges > + > +additionalProperties: false > + > +examples: > + - | > + // Minimal example > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + expander0: expander@0 { > + compatible = "realtek,rtl8231"; > + reg = <0>; > + > + gpio-controller; > + #gpio-cells = <2>; > + gpio-ranges = <&expander0 0 0 37>; > + }; > + }; > + - | > + // All bells and whistles included > + #include <dt-bindings/leds/common.h> > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + expander1: expander@1 { > + compatible = "realtek,rtl8231"; > + reg = <1>; > + > + gpio-controller; > + #gpio-cells = <2>; > + gpio-ranges = <&expander1 0 0 37>; > + > + realtek,drive-strength = <4>; > + > + button-pins { > + pins = "gpio36"; > + function = "gpio"; > + input-debounce = "100000"; > + }; > + > + pwm-pins { > + pins = "gpio35"; > + function = "pwm"; > + }; > + > + led-pins { > + pins = "gpio0", "gpio1", "gpio3", "gpio4"; > + function = "led"; > + }; > + > + pwm { > + #pwm-cells = <2>; > + }; > + > + leds { > + compatible = "realtek,rtl8231-leds"; > + #address-cells = <2>; > + #size-cells = <0>; > + > + realtek,led-scan-mode = "single-color"; > + > + led@0,0 { > + reg = <0 0>; > + color = <LED_COLOR_ID_GREEN>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <0>; > + }; > + > + led@0,1 { > + reg = <0 1>; > + color = <LED_COLOR_ID_AMBER>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <0>; > + }; > + > + led@1,0 { > + reg = <1 0>; > + color = <LED_COLOR_ID_GREEN>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <1>; > + }; > + > + led@1,1 { > + reg = <1 1>; > + color = <LED_COLOR_ID_AMBER>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <1>; > + }; > + }; > + }; > + }; > -- > 2.31.1 >
On Mon, 17 May 2021 21:28:02 +0200, Sander Vanheule wrote: > The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI > bus device. Currently only the MDIO mode is supported, although SMI mode > support should be fairly straightforward, once an SMI bus driver is available. > > Provided features by the RTL8231: > - Up to 37 GPIOs > - Configurable drive strength: 8mA or 4mA (currently unsupported) > - Input debouncing on high GPIOs (currently unsupported) > - Up to 88 LEDs in multiple scan matrix groups > - On, off, or one of six toggling intervals > - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs > - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs > - Up to one PWM output (currently unsupported) > - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz) > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next Thanks! [1/7] regmap: Add MDIO bus support commit: 1f89d2fe16072a74b34bdb895160910091427891 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Andy, I've implemented the minor remarks (redundant assignments, if/else code structure...). Some extra details below. On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > > > The RTL8231 is implemented as an MDIO device, and provides a regmap > > interface for register access by the core and child devices. > > > > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek. > > Since kernel support for SMI is limited, and no real-world SMI > > implementations have been encountered for this device, this is currently > > unimplemented. The use of the regmap interface should make any future > > support relatively straightforward. > > > > After reset, all pins are muxed to GPIO inputs before the pin drivers > > are enabled. This is done to prevent accidental system resets, when a > > pin is connected to the parent SoC's reset line. > > > [missing MDIO_BUS dependency, provided via REGMAP_MDIO] > > Reported-by: kernel test robot <lkp@intel.com> > > What is the culprit? Shouldn't this have a Fixes tag? But it doesn't actually fix an issue created by an existing commit, just something that was wrong in the first version of the patch. This patch is not dedicated to fixing that single issue though, it's just a part of it. Hence the note above the Reported-by tag. > > > > + mdiodev->reset_gpio = gpiod_get_optional(dev, "reset", > > GPIOD_OUT_LOW); > > + device_property_read_u32(dev, "reset-assert-delay", &mdiodev- > > >reset_assert_delay); > > + device_property_read_u32(dev, "reset-deassert-delay", &mdiodev- > > >reset_deassert_delay); > > + > > + err = rtl8231_init(dev, map); > > + if (err) > > Resource leakage. Replaced gpiod_get_optional by devm_gpiod_get_optional. > > > + return err; > > + > > + /* LED_START enables power to output pins, and starts the LED engine > > */ > > + regmap_field_write(led_start, 1); > > > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, rtl8231_cells, > > + ARRAY_SIZE(rtl8231_cells), NULL, 0, NULL); > > Ditto. > > > +} > > ... > > > +#ifdef CONFIG_PM > > Replace this with __maybe_unused attribute. Done. I've also used a few extra macros from PM header to clean this part up a bit more. Best, Sander
Hi Andy, Also here I've tried to address your remarks for v3, some extra details below. On Tue, 2021-05-18 at 01:00 +0300, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled) > > +{ > > + unsigned int mode; > > > + unsigned int i = 0; > > This... > > > + if (regmap_field_read(pled->reg_field, &mode)) > > + return 0; > > + > > + while (i < pled->modes->num_toggle_rates && mode != pled->modes- > > >toggle_rates[i].mode) > > + i++; > > ...and this will be better as a for-loop. > > > + if (i < pled->modes->num_toggle_rates) > > + return pled->modes->toggle_rates[i].interval; > > > + else > > Redundant. > > > + return 0; > > +} Shrunk down to "for (...) if (...) return ...;" in v3. > > > + interval = 500; > > interval_ms Good suggestion, thanks. Don't need those comments in the code then. > > > + u32 addr[2]; > > + int err; > > + > > > + if (!fwnode_property_count_u32(fwnode, "reg")) > > err = fwnode_property_count_u32(...); > if (err < 0) > return err; > if (err == 0) > return -ENODEV; > > > + return -ENODEV; > > + > > + err = fwnode_property_read_u32_array(fwnode, "reg", addr, > > ARRAY_SIZE(addr)); > > If count returns 1? What's the point of counting if you always want two? If count returns 1, or more than 2, that's an error. But this check was missing in v2, so I added it in v3. > > > + if (!device_property_match_string(dev, "realtek,led-scan-mode", > > "single-color")) { > > It seems that device_property_match_string() and accompanying > functions have wrong description of returned codes, i.e. it returns > the index of the matched string. It's possible that some APIs are > broken (but I believe that the former is the case). > > That said, I think the proper comparison should be >= 0. Thanks, fixed. Best, Sander
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI
bus device. Currently only the MDIO mode is supported, although SMI mode
support should be fairly straightforward, once an SMI bus driver is available.
Provided features by the RTL8231:
- Up to 37 GPIOs
- Configurable drive strength: 8mA or 4mA (currently unsupported)
- Input debouncing on high GPIOs (currently unsupported)
- Up to 88 LEDs in multiple scan matrix groups
- On, off, or one of six toggling intervals
- "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
- "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
- Up to one PWM output (currently unsupported)
- Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)
Register access is provided through a new MDIO regmap provider. The GPIO
controller uses gpio-regmap, although a patch is required to support a
limitation of the chip.
Changes since v2:
- MDIO regmap support was merged, so patch is dropped here
- Implement feedback for DT bindings
- Use correct module names in Kconfigs
- Fix k*alloc return value checks
- Introduce GPIO regmap quirks to set output direction first
- pinctrl: Use static pin descriptions for pin controller
- pinctrl: Fix gpio consumer resource leak
- mfd: Replace CONFIG_PM-ifdef'ery
- leds: Rename interval to interval_ms
Changes since v1:
- Reintroduce MDIO regmap, with fixed Kconfig dependencies
- Add configurable dir/value order for gpio-regmap direction_out call
- Drop allocations for regmap fields that are used only on init
- Move some definitions to MFD header
- Add PM ops to replace driver remove for MFD
- Change pinctrl driver to (modified) gpio-regmap
- Change leds driver to use fwnode
Changes since RFC:
- Dropped MDIO regmap interface. I was unable to resolve the Kconfig
dependency issue, so have reverted to using regmap_config.reg_read/write.
- Added pinctrl support
- Added LED support
- Changed root device to MFD, with pinctrl and leds child devices. Root
device is now an mdio_device driver.
Sander Vanheule (6):
gpio: regmap: Add quirk for output data register
dt-bindings: leds: Binding for RTL8231 scan matrix
dt-bindings: mfd: Binding for RTL8231
mfd: Add RTL8231 core device
pinctrl: Add RTL8231 pin control and GPIO support
leds: Add support for RTL8231 LED scan matrix
.../bindings/leds/realtek,rtl8231-leds.yaml | 166 ++++++++
.../bindings/mfd/realtek,rtl8231.yaml | 190 +++++++++
drivers/gpio/gpio-regmap.c | 15 +-
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-rtl8231.c | 291 +++++++++++++
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 1 +
drivers/mfd/rtl8231.c | 143 +++++++
drivers/pinctrl/Kconfig | 11 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-rtl8231.c | 398 ++++++++++++++++++
include/linux/gpio/regmap.h | 13 +
include/linux/mfd/rtl8231.h | 57 +++
14 files changed, 1304 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml
create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
create mode 100644 drivers/leds/leds-rtl8231.c
create mode 100644 drivers/mfd/rtl8231.c
create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
create mode 100644 include/linux/mfd/rtl8231.h
> Changes since v2: > - MDIO regmap support was merged, so patch is dropped here Do you have any idea how this will get merged. It sounds like one of the Maintainers will need a stable branch of regmap. > - Introduce GPIO regmap quirks to set output direction first I thought you had determined it was possible to set output before direction? Andrew
On Mon, May 24, 2021 at 12:28 AM Sander Vanheule <sander@svanheule.net> wrote: > On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > > > > > The RTL8231 is implemented as an MDIO device, and provides a regmap > > > interface for register access by the core and child devices. > > > > > > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek. > > > Since kernel support for SMI is limited, and no real-world SMI > > > implementations have been encountered for this device, this is currently > > > unimplemented. The use of the regmap interface should make any future > > > support relatively straightforward. > > > > > > After reset, all pins are muxed to GPIO inputs before the pin drivers > > > are enabled. This is done to prevent accidental system resets, when a > > > pin is connected to the parent SoC's reset line. > > > > > [missing MDIO_BUS dependency, provided via REGMAP_MDIO] > > > Reported-by: kernel test robot <lkp@intel.com> > > > > What is the culprit? Shouldn't this have a Fixes tag? > > But it doesn't actually fix an issue created by an existing commit, just > something that was wrong in the first version of the patch. Then why is it in the tag block? If you want to give a credit to LKP, do it in the comments block (after '---' cutter line). > This patch is not > dedicated to fixing that single issue though, it's just a part of it. Hence the > note above the Reported-by tag. -- With Best Regards, Andy Shevchenko
Hi Andy, I forgot to address one of your questions, so here's a short follow up. On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > + err = regmap_read(map, RTL8231_REG_FUNC1, &v); > > > + ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v); > > If we got an error why we need a read_core, what for? The chip has a static 5-bit field in register 0x01, called READY_CODE according to the datasheet. If a device is present, and a read from register 0x01 succeeds, I still check that this field has the correct value. For the RTL8231, it should return 0x37. If this isn't the case, I assume this isn't an RTL8231, so the driver probe stops and returns an error value. Best, Sander
On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > Changes since v2: > > - MDIO regmap support was merged, so patch is dropped here > > Do you have any idea how this will get merged. It sounds like one of > the Maintainers will need a stable branch of regmap. This is not a problem if Mark provides an immutable branch to pull from. > > - Introduce GPIO regmap quirks to set output direction first > > I thought you had determined it was possible to set output before > direction? Same thoughts when I saw an updated version of that patch. My anticipation was to not see it at all.
On Mon, May 24, 2021 at 10:50 AM Sander Vanheule <sander@svanheule.net> wrote: > On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote: > > > + err = regmap_read(map, RTL8231_REG_FUNC1, &v); > > > > > + ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v); > > > > If we got an error why we need a read_core, what for? > > The chip has a static 5-bit field in register 0x01, called READY_CODE according > to the datasheet. If a device is present, and a read from register 0x01 > succeeds, I still check that this field has the correct value. For the RTL8231, > it should return 0x37. If this isn't the case, I assume this isn't an RTL8231, > so the driver probe stops and returns an error value. Right. And why do you get ready_code if you know that there is an error? -- With Best Regards, Andy Shevchenko
On Mon, 2021-05-24 at 10:55 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 10:50 AM Sander Vanheule <sander@svanheule.net> wrote: > > On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote: > > > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> > > > wrote: > > > > + err = regmap_read(map, RTL8231_REG_FUNC1, &v); > > > > > > > + ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v); > > > > > > If we got an error why we need a read_core, what for? > > > > The chip has a static 5-bit field in register 0x01, called READY_CODE > > according > > to the datasheet. If a device is present, and a read from register 0x01 > > succeeds, I still check that this field has the correct value. For the > > RTL8231, > > it should return 0x37. If this isn't the case, I assume this isn't an > > RTL8231, > > so the driver probe stops and returns an error value. > > Right. And why do you get ready_code if you know that there is an error? This has changed in v3. I now check if there was an error reading the register, and return if there was. Only if there wasn't an error, the code continues to extract and verify the READY_CODE value. Best, Sander
On Mon, 2021-05-24 at 14:32 +0300, Andy Shevchenko wrote: > Oops, I had sent this privately, Cc'ing to ML. I'll repeat my replies here then. > > On Mon, May 24, 2021 at 12:08 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, May 24, 2021 at 1:34 AM Sander Vanheule <sander@svanheule.net> wrote: > > > > > > This driver implements the GPIO and pin muxing features provided by the > > > RTL8231. The device should be instantiated as an MFD child, where the > > > parent device has already configured the regmap used for register > > > access. > > > > > > Although described in the bindings, pin debouncing and drive strength > > > selection are currently not implemented. Debouncing is only available > > > for the six highest GPIOs, and must be emulated when other pins are used > > > for (button) inputs anyway. > > > > ... > > > > > +struct rtl8231_function { > > > + const char *name; > > > + unsigned int ngroups; > > > + const char **groups; > > > > const char * const * groups? > > (Double check this, because I don't know if it's really const in your case) > > I had to rework rtl8231_pinctrl_init_functions a bit, but outside of that function this string array is indeed constant. > > > +}; > > > > ... > > > > > + const struct rtl8231_pin_desc *desc = > > > + (struct rtl8231_pin_desc *) > > > &rtl8231_pins[group_selector].drv_data; > > > > Casting from/to void * is redundant in C. > > > > ... > > > > > + struct rtl8231_pin_desc *desc = > > > + (struct rtl8231_pin_desc *) &rtl8231_pins[offset].drv_data; > > > > Ditto. Ok, changed. > > > > ... > > > > > + ctrl->nfunctions = ARRAY_SIZE(rtl8231_pin_function_names); > > > + ctrl->functions = devm_kcalloc(dev, ctrl->nfunctions, sizeof(*ctrl- > > > >functions), GFP_KERNEL); > > > + if (!ctrl->functions) { > > > > > + dev_err(dev, "failed to allocate pin function > > > descriptors\n"); > > > > Dtop this noisy message, user space will print the similar one. > > > > > + return -ENOMEM; > > > + } > > > > ... > > > > > + ctrl->map = dev_get_regmap(dev->parent, NULL); > > > + if (!ctrl->map) > > > + return -ENODEV; > > > + > > > + if (IS_ERR(ctrl->map)) > > > + return PTR_ERR(ctrl->map); > > > > Hmm... Is it really the case that you have to check for different values? > > What does NULL mean? Optional? > > Checked the documentation again, and this actually doesn't return error values. Only valid pointers or NULL. Will change accordingly here, and also in the LED driver. > > ... > > > > > + gr = devm_gpio_regmap_register(dev, &gpio_cfg); > > > + if (IS_ERR(gr)) { > > > > > + dev_err(dev, "failed to register gpio controller\n"); > > > + return PTR_ERR(gr); > > > > Is it possible to get a deferred probe here? If so, use dev_err_probe() > > gpiochip_add_data_with_key can indeed return EPROBE_DEFER (when gpiolib isn't loaded yet, if I understand correctly). I'll replace dev_err by dev_err_probe. Best, Sander
Hi Andy, Andrew, On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > Changes since v2: > > > - MDIO regmap support was merged, so patch is dropped here > > > > Do you have any idea how this will get merged. It sounds like one of > > the Maintainers will need a stable branch of regmap. > > This is not a problem if Mark provides an immutable branch to pull from. Mark has a tag (regmap-mdio) for this patch: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio > > > > - Introduce GPIO regmap quirks to set output direction first > > > > I thought you had determined it was possible to set output before > > direction? > > Same thoughts when I saw an updated version of that patch. My > anticipation was to not see it at all. The two devices I've been trying to test the behaviour on are: * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a pin configured as (active-low) GPIO. The LEDs are easy for a quick visual check. * Zyxel GS1900-8: RTL8231 used for the front panel button, and an active-low GPIO used to hard reset the main SoC (an RTL8380). I've modified this board to change some of the strapping pin values, but testing with the jumpers and pull-up/down resistors is a bit more tedious. On the Netgear, I tested the following with and without the quirk: # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on gpioset 1 32=0; gpioset 1 32=0 # Get value to change to input, turns the LED off (high impedance) # Will return 1 due to (weak) internal pull-up gpioget 1 32 # Set as OUT-HIGH, should result in LED off # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW value) # When the quirk is enabled, the LED remains off (i.e. correct OUT-HIGH value) gpioset 1 32=1 Now, what's confusing (to me) is that the inverse doesn't depend on the quirk: # Set as OUT-HIGH twice gpioset 1 32=1; gpioset 1 32=1 # Change to high-Z gpioget 1 32 # Set to OUT-LOW, always results in LED on, with or without quirk gpioset 1 32=0 Any idea why this would be (or appear) broken on the former case, but not on the latter? I was trying to reproduce this behaviour on the Zyxel, but using the strapping pins that are also used to configure the device's address. So perhaps the pull- ups/-downs were confusing the results. Using a separate pin on the Zyxel's RTL8231, I've now been able to confirm the same behaviour as on the Netgear, including capturing the resulting glitch (with my simple logic analyser) when enabling the quirk in the first test case. I hope this explains why I've still included the quirk in this revision. If not, please let me know what isn't clear. Best, Sander
On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote: > On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: > > On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: ... > > > > Changes since v2: > > > > - MDIO regmap support was merged, so patch is dropped here > > > > > > Do you have any idea how this will get merged. It sounds like one of > > > the Maintainers will need a stable branch of regmap. > > > > This is not a problem if Mark provides an immutable branch to pull from. > > Mark has a tag (regmap-mdio) for this patch: > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio Also works but you have to provide this information in the cover letter. ... > > > > - Introduce GPIO regmap quirks to set output direction first > > > > > > I thought you had determined it was possible to set output before > > > direction? > > > > Same thoughts when I saw an updated version of that patch. My > > anticipation was to not see it at all. > > The two devices I've been trying to test the behaviour on are: > * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a pin > configured as (active-low) GPIO. The LEDs are easy for a quick visual check. > * Zyxel GS1900-8: RTL8231 used for the front panel button, and an active-low > GPIO used to hard reset the main SoC (an RTL8380). I've modified this board > to change some of the strapping pin values, but testing with the jumpers and > pull-up/down resistors is a bit more tedious. > > On the Netgear, I tested the following with and without the quirk: > > # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on > gpioset 1 32=0; gpioset 1 32=0 > # Get value to change to input, turns the LED off (high impedance) > # Will return 1 due to (weak) internal pull-up > gpioget 1 32 > # Set as OUT-HIGH, should result in LED off > # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW value) > # When the quirk is enabled, the LED remains off (i.e. correct OUT-HIGH value) > gpioset 1 32=1 > > Now, what's confusing (to me) is that the inverse doesn't depend on the quirk: > > # Set as OUT-HIGH twice > gpioset 1 32=1; gpioset 1 32=1 > # Change to high-Z > gpioget 1 32 > # Set to OUT-LOW, always results in LED on, with or without quirk > gpioset 1 32=0 > > Any idea why this would be (or appear) broken on the former case, but not on the > latter? GPIO tools for the shell are context-less. Can you reproduce this with the legacy sysfs interface? > I was trying to reproduce this behaviour on the Zyxel, but using the strapping > pins that are also used to configure the device's address. So perhaps the pull- > ups/-downs were confusing the results. Using a separate pin on the Zyxel's > RTL8231, I've now been able to confirm the same behaviour as on the Netgear, > including capturing the resulting glitch (with my simple logic analyser) when > enabling the quirk in the first test case. > > I hope this explains why I've still included the quirk in this revision. If not, > please let me know what isn't clear. Do you possess a schematic of either of the devices and a link to the RTL datasheet (Btw, if it's publicly available, or you have a link that will ask for necessary sign-in it would be nice to include the link to it as a Datasheet: tag)? -- With Best Regards, Andy Shevchenko
On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote: > > On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: > > > On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: > > ... > > > > > > Changes since v2: > > > > > - MDIO regmap support was merged, so patch is dropped here > > > > > > > > Do you have any idea how this will get merged. It sounds like one of > > > > the Maintainers will need a stable branch of regmap. > > > > > > This is not a problem if Mark provides an immutable branch to pull from. > > > > Mark has a tag (regmap-mdio) for this patch: > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio > > Also works but you have to provide this information in the cover letter. > Ok, I will add the link to the cover letter for the next version. Does it need to be in a Link-tag, or can just be a reference? > ... > > > > > > - Introduce GPIO regmap quirks to set output direction first > > > > > > > > I thought you had determined it was possible to set output before > > > > direction? > > > > > > Same thoughts when I saw an updated version of that patch. My > > > anticipation was to not see it at all. > > > > The two devices I've been trying to test the behaviour on are: > > * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a pin > > configured as (active-low) GPIO. The LEDs are easy for a quick visual > > check. > > * Zyxel GS1900-8: RTL8231 used for the front panel button, and an active- > > low > > GPIO used to hard reset the main SoC (an RTL8380). I've modified this > > board > > to change some of the strapping pin values, but testing with the jumpers > > and > > pull-up/down resistors is a bit more tedious. > > > > On the Netgear, I tested the following with and without the quirk: > > > > # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on > > gpioset 1 32=0; gpioset 1 32=0 > > # Get value to change to input, turns the LED off (high impedance) > > # Will return 1 due to (weak) internal pull-up > > gpioget 1 32 > > # Set as OUT-HIGH, should result in LED off > > # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW value) > > # When the quirk is enabled, the LED remains off (i.e. correct OUT-HIGH > > value) > > gpioset 1 32=1 > > > > Now, what's confusing (to me) is that the inverse doesn't depend on the > > quirk: > > > > # Set as OUT-HIGH twice > > gpioset 1 32=1; gpioset 1 32=1 > > # Change to high-Z > > gpioget 1 32 > > # Set to OUT-LOW, always results in LED on, with or without quirk > > gpioset 1 32=0 > > > > Any idea why this would be (or appear) broken on the former case, but not on > > the > > latter? > > GPIO tools for the shell are context-less. Can you reproduce this with > the legacy sysfs interface? > > > I was trying to reproduce this behaviour on the Zyxel, but using the > > strapping > > pins that are also used to configure the device's address. So perhaps the > > pull- > > ups/-downs were confusing the results. Using a separate pin on the Zyxel's > > RTL8231, I've now been able to confirm the same behaviour as on the Netgear, > > including capturing the resulting glitch (with my simple logic analyser) > > when > > enabling the quirk in the first test case. > > > > I hope this explains why I've still included the quirk in this revision. If > > not, > > please let me know what isn't clear. > > Do you possess a schematic of either of the devices and a link to the > RTL datasheet (Btw, if it's publicly available, or you have a link > that will ask for necessary sign-in it would be nice to include the > link to it as a Datasheet: tag)? Sadly, I don't. Most of the info we have comes from code archives of switch vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the few leaked datasheets that can be found on the internet aren't exactly thick in information. The RTL8231 datasheet is actually quite useful, but makes no mention of the output value isse. Since this isn't an official resource, I don't think it would be appropriate to link it via a Datasheet: tag. https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_ 1.2.pdf Looking at the datasheet again, I came up with a... terrible hack to work around the output value issue. The chip also has GPIO_INVERT registers that I hadn't used until now, because the logical inversion is handled in the kernel. However, these inversion registers only apply to the output values. So, I could implement glitch-free output behaviour in the following way: * After chip reset, and before enabling the output driver (MFD initialisation): - Mux all pins as GPIO - Change all pins to outputs, so the data registers (0x1c-0x1e) become writable - Write value 0 to all pins - Change all pins to GPI to change them into high-Z * In the pinctrl/gpio driver: - Use data registers as input-only - Use inversion register to determine output value (can be written any time) The above gives glitch-free outputs, but the values that are read back (when configured as output), come from the data registers. They should now be coming from the inversion (reg_set_base) registers, but the code prefers to use the data registers (reg_dat_base). Best, Sander
Hi Andy, Forgot to reply to the sysfs suggestion. On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote: > > On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: > > > On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > - Introduce GPIO regmap quirks to set output direction first > > > > > > > > I thought you had determined it was possible to set output before > > > > direction? > > > > > > Same thoughts when I saw an updated version of that patch. My > > > anticipation was to not see it at all. > > > > The two devices I've been trying to test the behaviour on are: > > * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a pin > > configured as (active-low) GPIO. The LEDs are easy for a quick visual > > check. > > * Zyxel GS1900-8: RTL8231 used for the front panel button, and an active- > > low > > GPIO used to hard reset the main SoC (an RTL8380). I've modified this > > board > > to change some of the strapping pin values, but testing with the jumpers > > and > > pull-up/down resistors is a bit more tedious. > > > > On the Netgear, I tested the following with and without the quirk: > > > > # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on > > gpioset 1 32=0; gpioset 1 32=0 > > # Get value to change to input, turns the LED off (high impedance) > > # Will return 1 due to (weak) internal pull-up > > gpioget 1 32 > > # Set as OUT-HIGH, should result in LED off > > # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW value) > > # When the quirk is enabled, the LED remains off (i.e. correct OUT-HIGH > > value) > > gpioset 1 32=1 > > > > Now, what's confusing (to me) is that the inverse doesn't depend on the > > quirk: > > > > # Set as OUT-HIGH twice > > gpioset 1 32=1; gpioset 1 32=1 > > # Change to high-Z > > gpioget 1 32 > > # Set to OUT-LOW, always results in LED on, with or without quirk > > gpioset 1 32=0 > > > > Any idea why this would be (or appear) broken on the former case, but not on > > the > > latter? > > GPIO tools for the shell are context-less. Can you reproduce this with > the legacy sysfs interface? Using the sysfs interface produced the same behaviour for both test cases. E.g. case 1: # Set to output low echo out > direction; echo 0 > value # Change to input (with weak pull-up) echo in > direction # Try to set to output high # Fails to go high if the pin value is set before the direction echo high > direction Best, Sander
On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> wrote: > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: > > On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote: > > > On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: > > > > On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: ... > Ok, I will add the link to the cover letter for the next version. Does it need > to be in a Link-tag, or can just be a reference? Some kind of reference, no need to have a special tag in the cover letter. ... > > > > > > - Introduce GPIO regmap quirks to set output direction first > > > > > > > > > > I thought you had determined it was possible to set output before > > > > > direction? > > > > > > > > Same thoughts when I saw an updated version of that patch. My > > > > anticipation was to not see it at all. > > > > > > The two devices I've been trying to test the behaviour on are: > > > * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a pin > > > configured as (active-low) GPIO. The LEDs are easy for a quick visual > > > check. > > > * Zyxel GS1900-8: RTL8231 used for the front panel button, and an active- > > > low > > > GPIO used to hard reset the main SoC (an RTL8380). I've modified this > > > board > > > to change some of the strapping pin values, but testing with the jumpers > > > and > > > pull-up/down resistors is a bit more tedious. > > > > > > On the Netgear, I tested the following with and without the quirk: > > > > > > # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on > > > gpioset 1 32=0; gpioset 1 32=0 > > > # Get value to change to input, turns the LED off (high impedance) > > > # Will return 1 due to (weak) internal pull-up > > > gpioget 1 32 > > > # Set as OUT-HIGH, should result in LED off > > > # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW value) > > > # When the quirk is enabled, the LED remains off (i.e. correct OUT-HIGH > > > value) > > > gpioset 1 32=1 > > > > > > Now, what's confusing (to me) is that the inverse doesn't depend on the > > > quirk: > > > > > > # Set as OUT-HIGH twice > > > gpioset 1 32=1; gpioset 1 32=1 > > > # Change to high-Z > > > gpioget 1 32 > > > # Set to OUT-LOW, always results in LED on, with or without quirk > > > gpioset 1 32=0 > > > > > > Any idea why this would be (or appear) broken on the former case, but not on > > > the > > > latter? > > > > GPIO tools for the shell are context-less. Can you reproduce this with > > the legacy sysfs interface? > > > > > I was trying to reproduce this behaviour on the Zyxel, but using the > > > strapping > > > pins that are also used to configure the device's address. So perhaps the > > > pull- > > > ups/-downs were confusing the results. Using a separate pin on the Zyxel's > > > RTL8231, I've now been able to confirm the same behaviour as on the Netgear, > > > including capturing the resulting glitch (with my simple logic analyser) > > > when > > > enabling the quirk in the first test case. > > > > > > I hope this explains why I've still included the quirk in this revision. If > > > not, > > > please let me know what isn't clear. > > > > Do you possess a schematic of either of the devices and a link to the > > RTL datasheet (Btw, if it's publicly available, or you have a link > > that will ask for necessary sign-in it would be nice to include the > > link to it as a Datasheet: tag)? > > Sadly, I don't. Most of the info we have comes from code archives of switch > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the few > leaked datasheets that can be found on the internet aren't exactly thick in > information. > > The RTL8231 datasheet is actually quite useful, but makes no mention of the > output value isse. Since this isn't an official resource, I don't think it would > be appropriate to link it via a Datasheet: tag. > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_ > 1.2.pdf > > Looking at the datasheet again, I came up with a... terrible hack to work around > the output value issue. > > The chip also has GPIO_INVERT registers that I hadn't used until now, because > the logical inversion is handled in the kernel. However, these inversion > registers only apply to the output values. So, I could implement glitch-free > output behaviour in the following way: > * After chip reset, and before enabling the output driver (MFD initialisation): > - Mux all pins as GPIO > - Change all pins to outputs, No. no, no. This is much worse than the glitches. You never know what the hardware is connected there and it's potential breakage (on hw level) possible. > so the data registers (0x1c-0x1e) become writable > - Write value 0 to all pins > - Change all pins to GPI to change them into high-Z > * In the pinctrl/gpio driver: > - Use data registers as input-only > - Use inversion register to determine output value (can be written any time) > > The above gives glitch-free outputs, but the values that are read back (when > configured as output), come from the data registers. They should now be coming > from the inversion (reg_set_base) registers, but the code prefers to use the > data registers (reg_dat_base). Lemme read the datasheet and see if I find any clue for the hw behaviour. -- With Best Regards, Andy Shevchenko
On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> wrote: > > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: ... > > Sadly, I don't. Most of the info we have comes from code archives of switch > > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the few > > leaked datasheets that can be found on the internet aren't exactly thick in > > information. > > > > The RTL8231 datasheet is actually quite useful, but makes no mention of the > > output value isse. Since this isn't an official resource, I don't think it would > > be appropriate to link it via a Datasheet: tag. > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_ > > 1.2.pdf > > > > Looking at the datasheet again, I came up with a... terrible hack to work around > > the output value issue. > > > > The chip also has GPIO_INVERT registers that I hadn't used until now, because > > the logical inversion is handled in the kernel. However, these inversion > > registers only apply to the output values. So, I could implement glitch-free > > output behaviour in the following way: > > * After chip reset, and before enabling the output driver (MFD initialisation): > > - Mux all pins as GPIO > > - Change all pins to outputs, > > No. no, no. This is much worse than the glitches. You never know what > the hardware is connected there and it's potential breakage (on hw > level) possible. > > > so the data registers (0x1c-0x1e) become writable > > - Write value 0 to all pins > > - Change all pins to GPI to change them into high-Z > > * In the pinctrl/gpio driver: > > - Use data registers as input-only > > - Use inversion register to determine output value (can be written any time) > > > > The above gives glitch-free outputs, but the values that are read back (when > > configured as output), come from the data registers. They should now be coming > > from the inversion (reg_set_base) registers, but the code prefers to use the > > data registers (reg_dat_base). > > Lemme read the datasheet and see if I find any clue for the hw behaviour. Thank you for your patience! Have you explored the possibility of using En_Sync_GPIO? -- With Best Regards, Andy Shevchenko
On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> > > wrote: > > > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: > > ... > > > > Sadly, I don't. Most of the info we have comes from code archives of > > > switch > > > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the > > > few > > > leaked datasheets that can be found on the internet aren't exactly thick > > > in > > > information. > > > > > > The RTL8231 datasheet is actually quite useful, but makes no mention of > > > the > > > output value isse. Since this isn't an official resource, I don't think it > > > would > > > be appropriate to link it via a Datasheet: tag. > > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_ > > > 1.2.pdf > > > > > > Looking at the datasheet again, I came up with a... terrible hack to work > > > around > > > the output value issue. > > > > > > The chip also has GPIO_INVERT registers that I hadn't used until now, > > > because > > > the logical inversion is handled in the kernel. However, these inversion > > > registers only apply to the output values. So, I could implement glitch- > > > free > > > output behaviour in the following way: > > > * After chip reset, and before enabling the output driver (MFD > > > initialisation): > > > - Mux all pins as GPIO > > > - Change all pins to outputs, > > > > No. no, no. This is much worse than the glitches. You never know what > > the hardware is connected there and it's potential breakage (on hw > > level) possible. > > > > > so the data registers (0x1c-0x1e) become writable > > > - Write value 0 to all pins > > > - Change all pins to GPI to change them into high-Z > > > * In the pinctrl/gpio driver: > > > - Use data registers as input-only > > > - Use inversion register to determine output value (can be written any > > > time) > > > > > > The above gives glitch-free outputs, but the values that are read back > > > (when > > > configured as output), come from the data registers. They should now be > > > coming > > > from the inversion (reg_set_base) registers, but the code prefers to use > > > the > > > data registers (reg_dat_base). > > > > Lemme read the datasheet and see if I find any clue for the hw behaviour. > > Thank you for your patience! > > Have you explored the possibility of using En_Sync_GPIO? I haven't (output latching doesn't really appear to be a thing in the gpio framework?), but I did notice that the main SoC's RTL8231 integration uses it. Let me play around with it to see if it also latches the pin direction, or if that's always an immediate change. Best, Sander
On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> > > wrote: > > > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: > > ... > > > > Sadly, I don't. Most of the info we have comes from code archives of > > > switch > > > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the > > > few > > > leaked datasheets that can be found on the internet aren't exactly thick > > > in > > > information. > > > > > > The RTL8231 datasheet is actually quite useful, but makes no mention of > > > the > > > output value isse. Since this isn't an official resource, I don't think it > > > would > > > be appropriate to link it via a Datasheet: tag. > > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_ > > > 1.2.pdf > > > > > > Looking at the datasheet again, I came up with a... terrible hack to work > > > around > > > the output value issue. > > > > > > The chip also has GPIO_INVERT registers that I hadn't used until now, > > > because > > > the logical inversion is handled in the kernel. However, these inversion > > > registers only apply to the output values. So, I could implement glitch- > > > free > > > output behaviour in the following way: > > > * After chip reset, and before enabling the output driver (MFD > > > initialisation): > > > - Mux all pins as GPIO > > > - Change all pins to outputs, > > > > No. no, no. This is much worse than the glitches. You never know what > > the hardware is connected there and it's potential breakage (on hw > > level) possible. > > > > > so the data registers (0x1c-0x1e) become writable > > > - Write value 0 to all pins > > > - Change all pins to GPI to change them into high-Z > > > * In the pinctrl/gpio driver: > > > - Use data registers as input-only > > > - Use inversion register to determine output value (can be written any > > > time) > > > > > > The above gives glitch-free outputs, but the values that are read back > > > (when > > > configured as output), come from the data registers. They should now be > > > coming > > > from the inversion (reg_set_base) registers, but the code prefers to use > > > the > > > data registers (reg_dat_base). > > > > Lemme read the datasheet and see if I find any clue for the hw behaviour. > > Thank you for your patience! > > Have you explored the possibility of using En_Sync_GPIO? Got around to testing things. If En_Sync_GPIO is enabled, it's still possible to change the pin direction without also writing the Sync_GPIO bit. So even with the latching, glitches are still produced. As long as Sync_GPIO is not set to latch the new values, it also appears that reads of the data registers result in the current output value, not the new one. As a different test, I've added a pull-down, to make the input level low. Now I see the opposite behaviour as before (with set-value-before-direction): * OUT-HIGH > IN (low) > OUT-LOW: results in a high level (i.e. old value) * OUT-HIGH > IN (low) > OUT-HIGH: results in a high level (new/old value) * OUT-LOW > IN (low) > OUT-HIGH: results in a high level (new value, or toggled old value?) * OUT-LOW > IN (low) > OUT-LOW: results in a low level (new/old value) For reference, with a pull-up: * OUT-HIGH > IN (high) > OUT-HIGH: high result * OUT-HIGH > IN (high) > OUT-LOW: low result * OUT-LOW > IN (high) > OUT-HIGH: low result * OUT-LOW > IN (high) > OUT-LOW: low result I've only tested this with the sysfs interface, so I don't know what the result would be on multiple writes to the data register (during input, but probably not very relevant). Nor have I tested direction changes if the input has changed between two output values. I may have some time tomorrow for more testing, but otherwise it'll have to wait until the weekend. Any other ideas in the meantime? Best, Sander
+Cc: Hans Hans, sorry for disturbing you later too much. Here we have "nice" hardware which can't be used in a glitch-free mode (somehow it reminds me lynxpoint, baytrail, cherryview designs). If you have any ideas to share (no need to dive deep or look at it if you have no time), you're welcome. On Thu, May 27, 2021 at 12:02 AM Sander Vanheule <sander@svanheule.net> wrote: > > On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote: > > On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> > > > wrote: > > > > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: > > > > ... > > > > > > Sadly, I don't. Most of the info we have comes from code archives of > > > > switch > > > > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the > > > > few > > > > leaked datasheets that can be found on the internet aren't exactly thick > > > > in > > > > information. > > > > > > > > The RTL8231 datasheet is actually quite useful, but makes no mention of > > > > the > > > > output value isse. Since this isn't an official resource, I don't think it > > > > would > > > > be appropriate to link it via a Datasheet: tag. > > > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_ > > > > 1.2.pdf > > > > > > > > Looking at the datasheet again, I came up with a... terrible hack to work > > > > around > > > > the output value issue. > > > > > > > > The chip also has GPIO_INVERT registers that I hadn't used until now, > > > > because > > > > the logical inversion is handled in the kernel. However, these inversion > > > > registers only apply to the output values. So, I could implement glitch- > > > > free > > > > output behaviour in the following way: > > > > * After chip reset, and before enabling the output driver (MFD > > > > initialisation): > > > > - Mux all pins as GPIO > > > > - Change all pins to outputs, > > > > > > No. no, no. This is much worse than the glitches. You never know what > > > the hardware is connected there and it's potential breakage (on hw > > > level) possible. > > > > > > > so the data registers (0x1c-0x1e) become writable > > > > - Write value 0 to all pins > > > > - Change all pins to GPI to change them into high-Z > > > > * In the pinctrl/gpio driver: > > > > - Use data registers as input-only > > > > - Use inversion register to determine output value (can be written any > > > > time) > > > > > > > > The above gives glitch-free outputs, but the values that are read back > > > > (when > > > > configured as output), come from the data registers. They should now be > > > > coming > > > > from the inversion (reg_set_base) registers, but the code prefers to use > > > > the > > > > data registers (reg_dat_base). > > > > > > Lemme read the datasheet and see if I find any clue for the hw behaviour. > > > > Thank you for your patience! > > > > Have you explored the possibility of using En_Sync_GPIO? > > Got around to testing things. > > If En_Sync_GPIO is enabled, it's still possible to change the pin direction > without also writing the Sync_GPIO bit. So even with the latching, glitches are > still produced. > > As long as Sync_GPIO is not set to latch the new values, it also appears that > reads of the data registers result in the current output value, not the new one. > > As a different test, I've added a pull-down, to make the input level low. Now I > see the opposite behaviour as before (with set-value-before-direction): > * OUT-HIGH > IN (low) > OUT-LOW: results in a high level (i.e. old value) > * OUT-HIGH > IN (low) > OUT-HIGH: results in a high level (new/old value) > * OUT-LOW > IN (low) > OUT-HIGH: results in a high level (new value, or toggled > old value?) > * OUT-LOW > IN (low) > OUT-LOW: results in a low level (new/old value) > > For reference, with a pull-up: > * OUT-HIGH > IN (high) > OUT-HIGH: high result > * OUT-HIGH > IN (high) > OUT-LOW: low result > * OUT-LOW > IN (high) > OUT-HIGH: low result > * OUT-LOW > IN (high) > OUT-LOW: low result > > I've only tested this with the sysfs interface, so I don't know what the result > would be on multiple writes to the data register (during input, but probably not > very relevant). Nor have I tested direction changes if the input has changed > between two output values. > > I may have some time tomorrow for more testing, but otherwise it'll have to wait > until the weekend. Any other ideas in the meantime? No ideas so far. In x86 we used to have something similar (baytrail, cherryview, lynxpoint), but it's firmware assisted. I think that this hardware (realtek) is supposed either - to be firmware / bootloader assisted, so in a way that platform is preconfigured when Linux starts and any GPIO request won't be harmful as long as it doesn't change direction on the pins (which is usually guaranteed by DT and corresponding drivers to do the correct things) - be used for glitch-tolerant hardware (LEDs, for example, where nobody usually will noticed 1ms blink) That said, I have not been convinced we have to quirk gpio-regmap for this one. Just describe the issues with hardware in the accompanying documentation. But if maintainers or somebody comes with a better / different approach I am all ears. -- With Best Regards, Andy Shevchenko
Hi, On 5/27/21 12:38 PM, Andy Shevchenko wrote: > +Cc: Hans > > Hans, sorry for disturbing you later too much. Here we have "nice" > hardware which can't be used in a glitch-free mode (somehow it reminds > me lynxpoint, baytrail, cherryview designs). If you have any ideas to > share (no need to dive deep or look at it if you have no time), you're > welcome. I'm afraid I've no ideas how to solve this nicely. Documenting the issue might be the best we can do. Regards, Hans > > On Thu, May 27, 2021 at 12:02 AM Sander Vanheule <sander@svanheule.net> wrote: >> >> On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote: >>> On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko >>> <andy.shevchenko@gmail.com> wrote: >>>> On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> >>>> wrote: >>>>> On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote: >>> >>> ... >>> >>>>> Sadly, I don't. Most of the info we have comes from code archives of >>>>> switch >>>>> vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the >>>>> few >>>>> leaked datasheets that can be found on the internet aren't exactly thick >>>>> in >>>>> information. >>>>> >>>>> The RTL8231 datasheet is actually quite useful, but makes no mention of >>>>> the >>>>> output value isse. Since this isn't an official resource, I don't think it >>>>> would >>>>> be appropriate to link it via a Datasheet: tag. >>>>> https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_ >>>>> 1.2.pdf >>>>> >>>>> Looking at the datasheet again, I came up with a... terrible hack to work >>>>> around >>>>> the output value issue. >>>>> >>>>> The chip also has GPIO_INVERT registers that I hadn't used until now, >>>>> because >>>>> the logical inversion is handled in the kernel. However, these inversion >>>>> registers only apply to the output values. So, I could implement glitch- >>>>> free >>>>> output behaviour in the following way: >>>>> * After chip reset, and before enabling the output driver (MFD >>>>> initialisation): >>>>> - Mux all pins as GPIO >>>>> - Change all pins to outputs, >>>> >>>> No. no, no. This is much worse than the glitches. You never know what >>>> the hardware is connected there and it's potential breakage (on hw >>>> level) possible. >>>> >>>>> so the data registers (0x1c-0x1e) become writable >>>>> - Write value 0 to all pins >>>>> - Change all pins to GPI to change them into high-Z >>>>> * In the pinctrl/gpio driver: >>>>> - Use data registers as input-only >>>>> - Use inversion register to determine output value (can be written any >>>>> time) >>>>> >>>>> The above gives glitch-free outputs, but the values that are read back >>>>> (when >>>>> configured as output), come from the data registers. They should now be >>>>> coming >>>>> from the inversion (reg_set_base) registers, but the code prefers to use >>>>> the >>>>> data registers (reg_dat_base). >>>> >>>> Lemme read the datasheet and see if I find any clue for the hw behaviour. >>> >>> Thank you for your patience! >>> >>> Have you explored the possibility of using En_Sync_GPIO? >> >> Got around to testing things. >> >> If En_Sync_GPIO is enabled, it's still possible to change the pin direction >> without also writing the Sync_GPIO bit. So even with the latching, glitches are >> still produced. >> >> As long as Sync_GPIO is not set to latch the new values, it also appears that >> reads of the data registers result in the current output value, not the new one. >> >> As a different test, I've added a pull-down, to make the input level low. Now I >> see the opposite behaviour as before (with set-value-before-direction): >> * OUT-HIGH > IN (low) > OUT-LOW: results in a high level (i.e. old value) >> * OUT-HIGH > IN (low) > OUT-HIGH: results in a high level (new/old value) >> * OUT-LOW > IN (low) > OUT-HIGH: results in a high level (new value, or toggled >> old value?) >> * OUT-LOW > IN (low) > OUT-LOW: results in a low level (new/old value) >> >> For reference, with a pull-up: >> * OUT-HIGH > IN (high) > OUT-HIGH: high result >> * OUT-HIGH > IN (high) > OUT-LOW: low result >> * OUT-LOW > IN (high) > OUT-HIGH: low result >> * OUT-LOW > IN (high) > OUT-LOW: low result >> >> I've only tested this with the sysfs interface, so I don't know what the result >> would be on multiple writes to the data register (during input, but probably not >> very relevant). Nor have I tested direction changes if the input has changed >> between two output values. >> >> I may have some time tomorrow for more testing, but otherwise it'll have to wait >> until the weekend. Any other ideas in the meantime? > > No ideas so far. In x86 we used to have something similar (baytrail, > cherryview, lynxpoint), but it's firmware assisted. I think that this > hardware (realtek) is supposed either > - to be firmware / bootloader assisted, so in a way that platform is > preconfigured when Linux starts and any GPIO request won't be harmful > as long as it doesn't change direction on the pins (which is usually > guaranteed by DT and corresponding drivers to do the correct things) > - be used for glitch-tolerant hardware (LEDs, for example, where > nobody usually will noticed 1ms blink) > > That said, I have not been convinced we have to quirk gpio-regmap for > this one. Just describe the issues with hardware in the accompanying > documentation. > > But if maintainers or somebody comes with a better / different > approach I am all ears. >
On Mon, May 24, 2021 at 12:34 AM Sander Vanheule <sander@svanheule.net> wrote: > Add a binding description for the Realtek RTL8231, a GPIO and LED > expander chip commonly used in ethernet switches based on a Realtek > switch SoC. These chips can be addressed via an MDIO or SMI bus, or used > as a plain 36-bit shift register. > > This binding only describes the feature set provided by the MDIO/SMI > configuration, and covers the GPIO, PWM, and pin control properties. The > LED properties are defined in a separate binding. > > Signed-off-by: Sander Vanheule <sander@svanheule.net> This looks good to me from a GPIO and pin control PoV: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
> + gpio_cfg.reg_dat_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0); > + gpio_cfg.reg_set_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0); > + gpio_cfg.reg_dir_in_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DIR0); Btw. you'd only need GPIO_REGMAP_ADDR(x) if x might be 0. Because you have a constant != 0 there, you could save the GPIO_REGMAP_ADDR() call. You could drop this if you like, but no need to respin the series for this. -michael
Am 2021-05-24 13:41, schrieb Sander Vanheule: > Hi Andy, Andrew, > > On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: >> On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: >> > >> > > Changes since v2: >> > > - MDIO regmap support was merged, so patch is dropped here >> > >> > Do you have any idea how this will get merged. It sounds like one of >> > the Maintainers will need a stable branch of regmap. >> >> This is not a problem if Mark provides an immutable branch to pull >> from. > > Mark has a tag (regmap-mdio) for this patch: > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio > >> >> > > - Introduce GPIO regmap quirks to set output direction first >> > >> > I thought you had determined it was possible to set output before >> > direction? >> >> Same thoughts when I saw an updated version of that patch. My >> anticipation was to not see it at all. > > The two devices I've been trying to test the behaviour on are: > * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a > pin > configured as (active-low) GPIO. The LEDs are easy for a quick > visual check. > * Zyxel GS1900-8: RTL8231 used for the front panel button, and an > active-low > GPIO used to hard reset the main SoC (an RTL8380). I've modified > this board > to change some of the strapping pin values, but testing with the > jumpers and > pull-up/down resistors is a bit more tedious. > > On the Netgear, I tested the following with and without the quirk: > > # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on > gpioset 1 32=0; gpioset 1 32=0 > # Get value to change to input, turns the LED off (high impedance) > # Will return 1 due to (weak) internal pull-up > gpioget 1 32 > # Set as OUT-HIGH, should result in LED off > # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW > value) > # When the quirk is enabled, the LED remains off (i.e. correct > OUT-HIGH value) > gpioset 1 32=1 > > Now, what's confusing (to me) is that the inverse doesn't depend on the > quirk: > > # Set as OUT-HIGH twice > gpioset 1 32=1; gpioset 1 32=1 > # Change to high-Z > gpioget 1 32 > # Set to OUT-LOW, always results in LED on, with or without quirk > gpioset 1 32=0 > > Any idea why this would be (or appear) broken on the former case, but > not on the > latter? Before reading this, I'd have guessed that they switch the internal register depending on the GPIO direction; I mean there is only one register address for both the input and the output register. Hm. Did you try playing around with raw register accesses and see if the value of the GPIO data register is changing when you switch GPIOs to input/output. Eg. you could try https://github.com/kontron/miitool to access the registers from userspace (your ethernet controller has to have support for the ioctl's though, see commit a613bafec516 ("enetc: add ioctl() support for PHY-related ops") for an example). -michael
Am 2021-05-24 00:33, schrieb Sander Vanheule: > GPIO chips may not support setting the output value when a pin is > configured as an input, although the current implementation assumes > this > is always possible. > > Add support for setting pin direction before value. The order defaults > to setting the value first, but this can be reversed by setting the > GPIO_REGMAP_QUIRK_SET_DIRECTION_FIRST flag in regmap_config.quirks. Nice! If this is really needed: Reviewed-by: Michael Walle <michael@walle.cc> -michael
Hi Michael, On Fri, 2021-05-28 at 08:29 +0200, Michael Walle wrote: > > + gpio_cfg.reg_dat_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0); > > + gpio_cfg.reg_set_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0); > > + gpio_cfg.reg_dir_in_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DIR0); > > Btw. you'd only need GPIO_REGMAP_ADDR(x) if x might be 0. Because you > have > a constant != 0 there, you could save the GPIO_REGMAP_ADDR() call. You > could drop this if you like, but no need to respin the series for this. I will need to respin this series anyway, so I can drop the GPIO_REGMAP_ADDR() calls. I was aware they are no-ops in this case, as register address 0 is not used for the GPIO functions, so mainly included them as a form of documentation. Best, Sander
Am 2021-05-28 08:42, schrieb Sander Vanheule: > On Fri, 2021-05-28 at 08:29 +0200, Michael Walle wrote: >> > + gpio_cfg.reg_dat_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0); >> > + gpio_cfg.reg_set_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0); >> > + gpio_cfg.reg_dir_in_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DIR0); >> >> Btw. you'd only need GPIO_REGMAP_ADDR(x) if x might be 0. Because you >> have >> a constant != 0 there, you could save the GPIO_REGMAP_ADDR() call. You >> could drop this if you like, but no need to respin the series for >> this. > > I will need to respin this series anyway, so I can drop the > GPIO_REGMAP_ADDR() > calls. I was aware they are no-ops in this case, as register address 0 > is not > used for the GPIO functions, so mainly included them as a form of > documentation. It's up to you if you like to change it or keep it. -michael
Hi Michael, Andy, On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote: > Am 2021-05-24 13:41, schrieb Sander Vanheule: > > Hi Andy, Andrew, > > > > On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: > > > On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > > Changes since v2: > > > > > - MDIO regmap support was merged, so patch is dropped here > > > > > > > > Do you have any idea how this will get merged. It sounds like one of > > > > the Maintainers will need a stable branch of regmap. > > > > > > This is not a problem if Mark provides an immutable branch to pull > > > from. > > > > Mark has a tag (regmap-mdio) for this patch: > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio > > > > > > > > > > - Introduce GPIO regmap quirks to set output direction first > > > > > > > > I thought you had determined it was possible to set output before > > > > direction? > > > > > > Same thoughts when I saw an updated version of that patch. My > > > anticipation was to not see it at all. > > > > The two devices I've been trying to test the behaviour on are: > > * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a > > pin > > configured as (active-low) GPIO. The LEDs are easy for a quick > > visual check. > > * Zyxel GS1900-8: RTL8231 used for the front panel button, and an > > active-low > > GPIO used to hard reset the main SoC (an RTL8380). I've modified > > this board > > to change some of the strapping pin values, but testing with the > > jumpers and > > pull-up/down resistors is a bit more tedious. > > > > On the Netgear, I tested the following with and without the quirk: > > > > # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on > > gpioset 1 32=0; gpioset 1 32=0 > > # Get value to change to input, turns the LED off (high impedance) > > # Will return 1 due to (weak) internal pull-up > > gpioget 1 32 > > # Set as OUT-HIGH, should result in LED off > > # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW > > value) > > # When the quirk is enabled, the LED remains off (i.e. correct > > OUT-HIGH value) > > gpioset 1 32=1 > > > > Now, what's confusing (to me) is that the inverse doesn't depend on the > > quirk: > > > > # Set as OUT-HIGH twice > > gpioset 1 32=1; gpioset 1 32=1 > > # Change to high-Z > > gpioget 1 32 > > # Set to OUT-LOW, always results in LED on, with or without quirk > > gpioset 1 32=0 > > > > Any idea why this would be (or appear) broken on the former case, but > > not on the > > latter? > > Before reading this, I'd have guessed that they switch the internal > register > depending on the GPIO direction; I mean there is only one register > address > for both the input and the output register. Hm. > > Did you try playing around with raw register accesses and see if the > value > of the GPIO data register is changing when you switch GPIOs to > input/output. > > Eg. you could try https://github.com/kontron/miitool to access the > registers > from userspace (your ethernet controller has to have support for the > ioctl's > though, see commit a613bafec516 ("enetc: add ioctl() support for > PHY-related > ops") for an example). I think I found a solution! As Michael suggested, I tried raw register reads and writes, to eliminate any side effects of the intermediate code. I didn't use the ioctls (this isn't a netdev), but I found regmap's debugfs write functionality, which allowed me to do the same. I was trying to reproduce the behaviour I reported earlier, but couldn't. The output levels were always the intended ones. At some point I realised that the regmap_update_bits function does a read-modify-write, which might shadow the actual current output value. For example: * Set output low: current out is low * Change to input with pull-up: current out is still low, but DATAx reads high * Set output high: RMW reads a high value (the input), so assumes a write is not necessary, leaving the old output value (low). Currently, I see two options: * Use regmap_update_bits_base to avoid the lazy RMW behaviour * Add a cache for the output data values to the driver, and only use these values to write to the output registers. This would allow keeping lazy RMW behaviour, which may be a benefit on slow busses. With either of these implemented, if I set the output value before the direction, everything works! :-) Would you like this to be added to regmap-gpio, or should I revert back to a device-specific implementation? Best, Sander
Hi, On 5/30/21 6:19 PM, Sander Vanheule wrote: > Hi Michael, Andy, > > On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote: >> Am 2021-05-24 13:41, schrieb Sander Vanheule: >>> Hi Andy, Andrew, >>> >>> On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: >>>> On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote: >>>>> >>>>>> Changes since v2: >>>>>> - MDIO regmap support was merged, so patch is dropped here >>>>> >>>>> Do you have any idea how this will get merged. It sounds like one of >>>>> the Maintainers will need a stable branch of regmap. >>>> >>>> This is not a problem if Mark provides an immutable branch to pull >>>> from. >>> >>> Mark has a tag (regmap-mdio) for this patch: >>> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio >>> >>>> >>>>>> - Introduce GPIO regmap quirks to set output direction first >>>>> >>>>> I thought you had determined it was possible to set output before >>>>> direction? >>>> >>>> Same thoughts when I saw an updated version of that patch. My >>>> anticipation was to not see it at all. >>> >>> The two devices I've been trying to test the behaviour on are: >>> * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a >>> pin >>> configured as (active-low) GPIO. The LEDs are easy for a quick >>> visual check. >>> * Zyxel GS1900-8: RTL8231 used for the front panel button, and an >>> active-low >>> GPIO used to hard reset the main SoC (an RTL8380). I've modified >>> this board >>> to change some of the strapping pin values, but testing with the >>> jumpers and >>> pull-up/down resistors is a bit more tedious. >>> >>> On the Netgear, I tested the following with and without the quirk: >>> >>> # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on >>> gpioset 1 32=0; gpioset 1 32=0 >>> # Get value to change to input, turns the LED off (high impedance) >>> # Will return 1 due to (weak) internal pull-up >>> gpioget 1 32 >>> # Set as OUT-HIGH, should result in LED off >>> # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW >>> value) >>> # When the quirk is enabled, the LED remains off (i.e. correct >>> OUT-HIGH value) >>> gpioset 1 32=1 >>> >>> Now, what's confusing (to me) is that the inverse doesn't depend on the >>> quirk: >>> >>> # Set as OUT-HIGH twice >>> gpioset 1 32=1; gpioset 1 32=1 >>> # Change to high-Z >>> gpioget 1 32 >>> # Set to OUT-LOW, always results in LED on, with or without quirk >>> gpioset 1 32=0 >>> >>> Any idea why this would be (or appear) broken on the former case, but >>> not on the >>> latter? >> >> Before reading this, I'd have guessed that they switch the internal >> register >> depending on the GPIO direction; I mean there is only one register >> address >> for both the input and the output register. Hm. >> >> Did you try playing around with raw register accesses and see if the >> value >> of the GPIO data register is changing when you switch GPIOs to >> input/output. >> >> Eg. you could try https://github.com/kontron/miitool to access the >> registers >> from userspace (your ethernet controller has to have support for the >> ioctl's >> though, see commit a613bafec516 ("enetc: add ioctl() support for >> PHY-related >> ops") for an example). > > I think I found a solution! > > As Michael suggested, I tried raw register reads and writes, to eliminate any > side effects of the intermediate code. I didn't use the ioctls (this isn't a > netdev), but I found regmap's debugfs write functionality, which allowed me to > do the same. > > I was trying to reproduce the behaviour I reported earlier, but couldn't. The > output levels were always the intended ones. At some point I realised that the > regmap_update_bits function does a read-modify-write, which might shadow the > actual current output value. > For example: > * Set output low: current out is low > * Change to input with pull-up: current out is still low, but DATAx reads high > * Set output high: RMW reads a high value (the input), so assumes a write is > not necessary, leaving the old output value (low). > > Currently, I see two options: > * Use regmap_update_bits_base to avoid the lazy RMW behaviour > * Add a cache for the output data values to the driver, and only use these > values to write to the output registers. This would allow keeping lazy RMW > behaviour, which may be a benefit on slow busses. > > With either of these implemented, if I set the output value before the > direction, everything works! :-) > > Would you like this to be added to regmap-gpio, or should I revert back to a > device-specific implementation? Regmap allows you to mark certain ranges as volatile, so that they will not be cached, these GPIO registers containing the current pin value seems like a good candidate for this. This is also necessary to make reading the GPIO work without getting back a stale, cached value. Regards, Hans
On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/30/21 6:19 PM, Sander Vanheule wrote: > > On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote: ... > > I think I found a solution! > > > > As Michael suggested, I tried raw register reads and writes, to eliminate any > > side effects of the intermediate code. I didn't use the ioctls (this isn't a > > netdev), but I found regmap's debugfs write functionality, which allowed me to > > do the same. > > > > I was trying to reproduce the behaviour I reported earlier, but couldn't. The > > output levels were always the intended ones. At some point I realised that the > > regmap_update_bits function does a read-modify-write, which might shadow the > > actual current output value. > > For example: > > * Set output low: current out is low > > * Change to input with pull-up: current out is still low, but DATAx reads high > > * Set output high: RMW reads a high value (the input), so assumes a write is > > not necessary, leaving the old output value (low). > > > > Currently, I see two options: > > * Use regmap_update_bits_base to avoid the lazy RMW behaviour > > * Add a cache for the output data values to the driver, and only use these > > values to write to the output registers. This would allow keeping lazy RMW > > behaviour, which may be a benefit on slow busses. > > > > With either of these implemented, if I set the output value before the > > direction, everything works! :-) > > > > Would you like this to be added to regmap-gpio, or should I revert back to a > > device-specific implementation? > > Regmap allows you to mark certain ranges as volatile, so that they will not > be cached, these GPIO registers containing the current pin value seems like > a good candidate for this. This is also necessary to make reading the GPIO > work without getting back a stale, cached value. After all it seems a simple missed proper register configuration in the driver for regmap. Oh, as usual something easy-to-solve requires tons of time to find it. :-) Sander, I think you may look at gpio-pca953x.c to understand how it works (volatility of registers). -- With Best Regards, Andy Shevchenko
Am 2021-05-30 20:16, schrieb Andy Shevchenko: > On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> > wrote: >> On 5/30/21 6:19 PM, Sander Vanheule wrote: >> > On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote: > > ... > >> > I think I found a solution! nice! >> > As Michael suggested, I tried raw register reads and writes, to eliminate any >> > side effects of the intermediate code. I didn't use the ioctls (this isn't a >> > netdev), but I found regmap's debugfs write functionality, which allowed me to >> > do the same. >> > >> > I was trying to reproduce the behaviour I reported earlier, but couldn't. The >> > output levels were always the intended ones. At some point I realised that the >> > regmap_update_bits function does a read-modify-write, which might shadow the >> > actual current output value. >> > For example: >> > * Set output low: current out is low >> > * Change to input with pull-up: current out is still low, but DATAx reads high >> > * Set output high: RMW reads a high value (the input), so assumes a write is >> > not necessary, leaving the old output value (low). >> > >> > Currently, I see two options: >> > * Use regmap_update_bits_base to avoid the lazy RMW behaviour >> > * Add a cache for the output data values to the driver, and only use these >> > values to write to the output registers. This would allow keeping lazy RMW >> > behaviour, which may be a benefit on slow busses. >> > >> > With either of these implemented, if I set the output value before the >> > direction, everything works! :-) >> > >> > Would you like this to be added to regmap-gpio, or should I revert back to a >> > device-specific implementation? >> >> Regmap allows you to mark certain ranges as volatile, so that they >> will not >> be cached, these GPIO registers containing the current pin value seems >> like >> a good candidate for this. This is also necessary to make reading the >> GPIO >> work without getting back a stale, cached value. > > After all it seems a simple missed proper register configuration in > the driver for regmap. > Oh, as usual something easy-to-solve requires tons of time to find it. > :-) > > Sander, I think you may look at gpio-pca953x.c to understand how it > works (volatility of registers). But as far as I see is the regmap instantiated without a cache? -michael
On Mon, May 24, 2021 at 12:34 AM Sander Vanheule <sander@svanheule.net> wrote: > > GPIO chips may not support setting the output value when a pin is > configured as an input, although the current implementation assumes this > is always possible. > > Add support for setting pin direction before value. The order defaults > to setting the value first, but this can be reversed by setting the > GPIO_REGMAP_QUIRK_SET_DIRECTION_FIRST flag in regmap_config.quirks. > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > --- Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
On Sun, 2021-05-30 at 23:22 +0200, Michael Walle wrote: > Am 2021-05-30 20:16, schrieb Andy Shevchenko: > > On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> > > wrote: > > > On 5/30/21 6:19 PM, Sander Vanheule wrote: > > > > As Michael suggested, I tried raw register reads and writes, to eliminate > > > > any > > > > side effects of the intermediate code. I didn't use the ioctls (this isn't > > > > a > > > > netdev), but I found regmap's debugfs write functionality, which allowed > > > > me to > > > > do the same. > > > > > > > > I was trying to reproduce the behaviour I reported earlier, but couldn't. > > > > The > > > > output levels were always the intended ones. At some point I realised that > > > > the > > > > regmap_update_bits function does a read-modify-write, which might shadow > > > > the > > > > actual current output value. > > > > For example: > > > > * Set output low: current out is low > > > > * Change to input with pull-up: current out is still low, but DATAx reads > > > > high > > > > * Set output high: RMW reads a high value (the input), so assumes a write > > > > is > > > > not necessary, leaving the old output value (low). > > > > > > > > Currently, I see two options: > > > > * Use regmap_update_bits_base to avoid the lazy RMW behaviour > > > > * Add a cache for the output data values to the driver, and only use > > > > these > > > > values to write to the output registers. This would allow keeping lazy > > > > RMW > > > > behaviour, which may be a benefit on slow busses. > > > > > > > > With either of these implemented, if I set the output value before the > > > > direction, everything works! :-) > > > > > > > > Would you like this to be added to regmap-gpio, or should I revert back to > > > > a > > > > device-specific implementation? > > > > > > Regmap allows you to mark certain ranges as volatile, so that they > > > will not > > > be cached, these GPIO registers containing the current pin value seems > > > like > > > a good candidate for this. This is also necessary to make reading the > > > GPIO > > > work without getting back a stale, cached value. > > > > After all it seems a simple missed proper register configuration in > > the driver for regmap. > > Oh, as usual something easy-to-solve requires tons of time to find it. > > :-) > > > > Sander, I think you may look at gpio-pca953x.c to understand how it > > works (volatility of registers). > > But as far as I see is the regmap instantiated without a cache? That's correct, there currently is no cache, although I could add one. The data register rather appears to be implemented as a read-only (pin inputs) register and a write-only (pin outputs) register, aliased on the same register address. As I understand, marking the DATA registers as volatile wouldn't help. With a cache this would force reads to not use the cache, which is indeed required for the pin input values (DATA register reads). However, the output values (DATA register writes) can in fact be cached. Looking at _regmap_update_bits(), marking a register as volatile would only make a difference if regmap.reg_update_bits is implemented. On an MDIO bus, this would also be emulated with a lazy RMW (see mdiobus_modify()), which is why I chose not to implement it for regmap-mdio. So, I still think the issue lies with the lazy RMW behaviour. The patch below would force a register update when reg_set_base (the data output register) and reg_dat_base (the data input register) are identical. Otherwise the two registers are assumed to have conventional RW behaviour. I'm just not entirely sure gpio-regmap.c is the right place for this. ---8<--- diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c index 95553734e169..c2fccd19548a 100644 --- a/drivers/gpio/gpio-regmap.c +++ b/drivers/gpio/gpio-regmap.c @@ -81,13 +81,16 @@ static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset, { struct gpio_regmap *gpio = gpiochip_get_data(chip); unsigned int base = gpio_regmap_addr(gpio->reg_set_base); + bool force = gpio->reg_set_base == gpio->reg_dat_base; unsigned int reg, mask; gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); if (val) - regmap_update_bits(gpio->regmap, reg, mask, mask); + regmap_update_bits_base(gpio->regmap, reg, mask, mask, NULL, + false, force); else - regmap_update_bits(gpio->regmap, reg, mask, 0); + regmap_update_bits_base(gpio->regmap, reg, mask, 0, NULL, + false, force); } static void gpio_regmap_set_with_clear(struct gpio_chip *chip, -- Best, Sander
On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote: > > > On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote: > > Am 2021-05-31 10:36, schrieb Sander Vanheule: ... > > > The data register rather appears to be implemented as a read-only (pin > > > inputs) > > > register and a write-only (pin outputs) register, aliased on the same > > > register > > > address. > > > > > > > Ahh so this makes more sense. If the data register is really write only > > regardless of the direction mode, then RMW doesn't make any sense at all. > > Please note, that even if regmap caches values, it might be marked as dirty > > and it will re-read the values from hardware. So I don't know if that will > > help you. > > > > So a possible quirk could be > > GPIO_REGMAP_WRITE_ONLY_DATA_REG (or something like that) > > > > > > Isn’t regmap property to do a such? I don’t think any quirks are needed since hw > works as expected. The HW works as expected, but it is regmap-gpio's assumption that values read from reg_set_base reflect the current output value that fails. I looked a bit more at the provided interface, to see if this can be done with existing regmap functionality. The data registers must not be marked volatile, to ensure cached reads. The pin set function can wrap the RMW in regcache_cache_only + regcache_sync, but this causes visible glitching on my device. The pin input values can be read by wrapping the regmap_read in regcache_cache_bypass guards. If only the regmap's internal lock is used, the RMW cycle is no longer atomic. Inside the cache_only guards you can't read the input data, and inside the cache_bypass guards other register writes cannot be performed, or the cache may get out of sync. regcache_sync_region could be used, but maybe we would then miss other registers that were updated in the meantime. Am I missing something here? It seems to me like the regmap interface can't really accommodate what's required, unless maybe the rtl8231 regmap users perform some manual locking. This all seems terribly complicated compared to using an internal output-value cache inside regmap-gpio. Best, Sander
On Sun, May 30, 2021 at 8:16 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Regmap allows you to mark certain ranges as volatile, so that they will not > > be cached, these GPIO registers containing the current pin value seems like > > a good candidate for this. This is also necessary to make reading the GPIO > > work without getting back a stale, cached value. > > After all it seems a simple missed proper register configuration in > the driver for regmap. > Oh, as usual something easy-to-solve requires tons of time to find it. :-) This is actually quite interesting. In the discussion around adding Rust support for the Linux kernel what I came to realize was that the memory safety that Rust adds is similar in application and ambition to what e.g. regmap-mmio provides. One aspect of writing kernel drivers in Rust is to always have something like regmap between your code and the hardware to strictly control the memory access pattern. After all regmap is "memory safety implemented in C". What we see in cases like this is that not only does that make things more strict and controlled (after all we have regmap for a reason) but also makes it possible to generate a whole new set of bugs by doing an error in how you specify the memory semantics. As all other paradigms, memory safety thinking implies that never specify anything wrong. Just regarding all registers/memory cells in a register page as default volatile (which is what we do a lot of the time) has its upsides: bugs like this doesn't happen. (Just some sidetracking...) Yours, Linus Walleij
Am 2021-06-01 11:59, schrieb Linus Walleij: > Just regarding all registers/memory cells in a register page > as default volatile (which is what we do a lot of the time) > has its upsides: bugs like this doesn't happen. I don't think this is the bug here. If it is really a write-only register the problem is the read in RMW. Because reading the register will return the input value instead of the (previously written) output value. -michael
On Tue, Jun 1, 2021 at 12:18 PM Michael Walle <michael@walle.cc> wrote: > Am 2021-06-01 11:59, schrieb Linus Walleij: > > Just regarding all registers/memory cells in a register page > > as default volatile (which is what we do a lot of the time) > > has its upsides: bugs like this doesn't happen. > > I don't think this is the bug here. If it is really a write-only > register > the problem is the read in RMW. Because reading the register will return > the input value instead of the (previously written) output value. True that. Write and read semantics differ on the register. Volatile is used for this and some other things, like for example interrupts being cleared when a register is read so it is strictly read-once. So the regmap config is really important to get right. IIUC one of the ambitions around Rust is to encode this in how memory is specified in the language. (I am still thinking about whether that is really a good idea or not.) Yours, Linus Walleij
Am 2021-06-01 12:51, schrieb Linus Walleij: > On Tue, Jun 1, 2021 at 12:18 PM Michael Walle <michael@walle.cc> wrote: >> Am 2021-06-01 11:59, schrieb Linus Walleij: > >> > Just regarding all registers/memory cells in a register page >> > as default volatile (which is what we do a lot of the time) >> > has its upsides: bugs like this doesn't happen. >> >> I don't think this is the bug here. If it is really a write-only >> register >> the problem is the read in RMW. Because reading the register will >> return >> the input value instead of the (previously written) output value. > > True that. Write and read semantics differ on the register. > > Volatile is used for this and some other things, > like for example interrupts being cleared when a register > is read so it is strictly read-once. Isn't that what precious is for? > So the regmap config is really important to get right. > > IIUC one of the ambitions around Rust is to encode this > in how memory is specified in the language. (I am still > thinking about whether that is really a good idea or not.) -- -michael
On Tue, Jun 1, 2021 at 1:41 PM Michael Walle <michael@walle.cc> wrote: > Am 2021-06-01 12:51, schrieb Linus Walleij: > > On Tue, Jun 1, 2021 at 12:18 PM Michael Walle <michael@walle.cc> wrote: > >> Am 2021-06-01 11:59, schrieb Linus Walleij: > > > >> > Just regarding all registers/memory cells in a register page > >> > as default volatile (which is what we do a lot of the time) > >> > has its upsides: bugs like this doesn't happen. > >> > >> I don't think this is the bug here. If it is really a write-only > >> register > >> the problem is the read in RMW. Because reading the register will > >> return > >> the input value instead of the (previously written) output value. > > > > True that. Write and read semantics differ on the register. > > > > Volatile is used for this and some other things, > > like for example interrupts being cleared when a register > > is read so it is strictly read-once. > > Isn't that what precious is for? I never figured that one out. But I assume you are right. Proper regmap semantics documentation is forthcoming! ;) Yours, Linus Walleij
Am 2021-05-31 17:48, schrieb Andy Shevchenko: > On Mon, May 31, 2021 at 6:33 PM Sander Vanheule <sander@svanheule.net> > wrote: >> On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote: >> > On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote: >> > > Am 2021-05-31 10:36, schrieb Sander Vanheule: > >> Am I missing something here? It seems to me like the regmap interface >> can't >> really accommodate what's required, unless maybe the rtl8231 regmap >> users >> perform some manual locking. This all seems terribly complicated >> compared to >> using an internal output-value cache inside regmap-gpio. > > Have you had a chance to look into the PCA953x driver? > Sounds to me that you are missing the APIs that regmap provides. What would that be? The register cache? We need to cache the value somehow, because (still assuming it is write only) we cannot read it back. Thus the read of the RMW, would need get the value from the cache. Thus the user of gpio-regmap would need to make sure, to (a) use a cache for the regmap supplied to gpio-regmap and (b) populate its initial values correctly. Is that what you are suggesting? And hopefully, no other user of the regmap will call regcache_mark_dirty() or something like that. I had a quick look at the PCA953x driver but it all its registers are readable according to the comment on the top of the file. -michael
On Tue, Jun 1, 2021 at 2:49 PM Michael Walle <michael@walle.cc> wrote: > Am 2021-05-31 17:48, schrieb Andy Shevchenko: > > On Mon, May 31, 2021 at 6:33 PM Sander Vanheule <sander@svanheule.net> > > wrote: > >> On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote: > >> > On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote: > >> > > Am 2021-05-31 10:36, schrieb Sander Vanheule: > > > >> Am I missing something here? It seems to me like the regmap interface > >> can't > >> really accommodate what's required, unless maybe the rtl8231 regmap > >> users > >> perform some manual locking. This all seems terribly complicated > >> compared to > >> using an internal output-value cache inside regmap-gpio. > > > > Have you had a chance to look into the PCA953x driver? > > Sounds to me that you are missing the APIs that regmap provides. > > What would that be? The register cache? We need to cache the > value somehow, because (still assuming it is write only) we cannot > read it back. Thus the read of the RMW, would need get the > value from the cache. Thus the user of gpio-regmap would need > to make sure, to (a) use a cache for the regmap supplied to > gpio-regmap and (b) populate its initial values correctly. Is > that what you are suggesting? And hopefully, no other user > of the regmap will call regcache_mark_dirty() or something > like that. > > I had a quick look at the PCA953x driver but it all its > registers are readable according to the comment on the top > of the file. Since regmap doesn't have a facility to support the registers _at the same offset_ with different meaning (depending on data direction), the only way to handle this (without redesign regmap internals) is to add specific "pages" via additional bits in the address space. So, let's say 0 = RW, 1 = RO, 2 = WO. In this case see the following offset mapping of the hypothetical hardware registers: REG1 (RW) 0x00 -> 0x000 REG2 (RW) 0x04 -> 0x004 REG3 (RO) 0x08 -> 0x108 REG3 (RW) 0x08 -> 0x208 Then these bits should be masked out. Something similar is done in the PCA953x driver for extended addresses and autoincrement. This is what I propose to look at as the starter.
On Mon, 24 May 2021 00:34:01 +0200, Sander Vanheule wrote: > Add a binding description for the Realtek RTL8231, a GPIO and LED > expander chip commonly used in ethernet switches based on a Realtek > switch SoC. These chips can be addressed via an MDIO or SMI bus, or used > as a plain 36-bit shift register. > > This binding only describes the feature set provided by the MDIO/SMI > configuration, and covers the GPIO, PWM, and pin control properties. The > LED properties are defined in a separate binding. > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > --- > .../bindings/mfd/realtek,rtl8231.yaml | 190 ++++++++++++++++++ > 1 file changed, 190 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
Hi Andy, On Tue, 2021-06-01 at 18:24 +0300, Andy Shevchenko wrote: > On Tue, Jun 1, 2021 at 2:49 PM Michael Walle <michael@walle.cc> wrote: > > Am 2021-05-31 17:48, schrieb Andy Shevchenko: > > > On Mon, May 31, 2021 at 6:33 PM Sander Vanheule <sander@svanheule.net> > > > wrote: > > > > On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote: > > > > > On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote: > > > > > > Am 2021-05-31 10:36, schrieb Sander Vanheule: > > > > > > > Am I missing something here? It seems to me like the regmap interface > > > > can't > > > > really accommodate what's required, unless maybe the rtl8231 regmap > > > > users > > > > perform some manual locking. This all seems terribly complicated > > > > compared to > > > > using an internal output-value cache inside regmap-gpio. > > > > > > Have you had a chance to look into the PCA953x driver? > > > Sounds to me that you are missing the APIs that regmap provides. > > > > What would that be? The register cache? We need to cache the > > value somehow, because (still assuming it is write only) we cannot > > read it back. Thus the read of the RMW, would need get the > > value from the cache. Thus the user of gpio-regmap would need > > to make sure, to (a) use a cache for the regmap supplied to > > gpio-regmap and (b) populate its initial values correctly. Is > > that what you are suggesting? And hopefully, no other user > > of the regmap will call regcache_mark_dirty() or something > > like that. > > > > I had a quick look at the PCA953x driver but it all its > > registers are readable according to the comment on the top > > of the file. > > Since regmap doesn't have a facility to support the registers _at the > same offset_ with different meaning (depending on data direction), the > only way to handle this (without redesign regmap internals) is to add > specific "pages" via additional bits in the address space. So, let's > say 0 = RW, 1 = RO, 2 = WO. In this case see the following offset > mapping of the hypothetical hardware registers: > > REG1 (RW) 0x00 -> 0x000 > REG2 (RW) 0x04 -> 0x004 > REG3 (RO) 0x08 -> 0x108 > REG3 (RW) 0x08 -> 0x208 > > Then these bits should be masked out. Something similar is done in the > PCA953x driver for extended addresses and autoincrement. > > This is what I propose to look at as the starter. Thank you for clarifying. Essentialy this is then the same solution as an extra cache in gpio-regmap for the output values, except the cacheing is handled by the regmap layer. I think this was the last issue standing, so after I implement this, I'll spin a v4. Best, Sander
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI
bus device. Currently only the MDIO mode is supported, although SMI mode
support should be fairly straightforward, once an SMI bus driver is available.
Provided features by the RTL8231:
- Up to 37 GPIOs
- Configurable drive strength: 8mA or 4mA (currently unsupported)
- Input debouncing on high GPIOs (currently unsupported)
- Up to 88 LEDs in multiple scan matrix groups
- On, off, or one of six toggling intervals
- "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
- "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
- Up to one PWM output (currently unsupported)
- Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)
The GPIO controller uses gpio-regmap. The assumed read-modify-write behaviour
of the data output is provided by using a cached virtual address range for the
output values.
Register access is provided through a new MDIO regmap provider. The required
MDIO regmap support was merged in Mark Brown's regmap repository, and can be
found under the regmap-mdio tag:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio
Changes since v3:
- Drop gpio-regmap direction-before-value quirk
- Use secondary virtual register range to enable proper read-modify-write
behaviour on GPIO output values
- Add pin debounce support
- Switch to generic pinmux functions
Changes since v2:
- MDIO regmap support was merged, so patch is dropped here
- Implement feedback for DT bindings
- Use correct module names in Kconfigs
- Fix k*alloc return value checks
- Introduce GPIO regmap quirks to set output direction first
- pinctrl: Use static pin descriptions for pin controller
- pinctrl: Fix gpio consumer resource leak
- mfd: Replace CONFIG_PM-ifdef'ery
- leds: Rename interval to interval_ms
Changes since v1:
- Reintroduce MDIO regmap, with fixed Kconfig dependencies
- Add configurable dir/value order for gpio-regmap direction_out call
- Drop allocations for regmap fields that are used only on init
- Move some definitions to MFD header
- Add PM ops to replace driver remove for MFD
- Change pinctrl driver to (modified) gpio-regmap
- Change leds driver to use fwnode
Changes since RFC:
- Dropped MDIO regmap interface. I was unable to resolve the Kconfig
dependency issue, so have reverted to using regmap_config.reg_read/write.
- Added pinctrl support
- Added LED support
- Changed root device to MFD, with pinctrl and leds child devices. Root
device is now an mdio_device driver.
Sander Vanheule (5):
dt-bindings: leds: Binding for RTL8231 scan matrix
dt-bindings: mfd: Binding for RTL8231
mfd: Add RTL8231 core device
pinctrl: Add RTL8231 pin control and GPIO support
leds: Add support for RTL8231 LED scan matrix
.../bindings/leds/realtek,rtl8231-leds.yaml | 166 +++++++
.../bindings/mfd/realtek,rtl8231.yaml | 190 ++++++++
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-rtl8231.c | 291 ++++++++++++
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 1 +
drivers/mfd/rtl8231.c | 240 ++++++++++
drivers/pinctrl/Kconfig | 11 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-rtl8231.c | 438 ++++++++++++++++++
include/linux/mfd/rtl8231.h | 78 ++++
12 files changed, 1436 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml
create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
create mode 100644 drivers/leds/leds-rtl8231.c
create mode 100644 drivers/mfd/rtl8231.c
create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
create mode 100644 include/linux/mfd/rtl8231.h
Hi Michael, Bartosz, On Fri, 2021-05-28 at 08:40 +0200, Michael Walle wrote: > Am 2021-05-24 00:33, schrieb Sander Vanheule: > > GPIO chips may not support setting the output value when a pin is > > configured as an input, although the current implementation assumes > > this > > is always possible. > > > > Add support for setting pin direction before value. The order defaults > > to setting the value first, but this can be reversed by setting the > > GPIO_REGMAP_QUIRK_SET_DIRECTION_FIRST flag in regmap_config.quirks. > > Nice! If this is really needed: > > Reviewed-by: Michael Walle <michael@walle.cc> Looks like the quirk won't be needed for this series, but I can always resubmit it separately if needed. Best, Sander
On Thu, Jun 3, 2021 at 1:01 PM Sander Vanheule <sander@svanheule.net> wrote: > > The RTL8231 is implemented as an MDIO device, and provides a regmap > interface for register access by the core and child devices. > > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek. > Since kernel support for SMI is limited, and no real-world SMI > implementations have been encountered for this device, this is currently > unimplemented. The use of the regmap interface should make any future > support relatively straightforward. > > After reset, all pins are muxed to GPIO inputs before the pin drivers > are enabled. This is done to prevent accidental system resets, when a > pin is connected to the parent SoC's reset line. > > To provide different read and write semantics for the GPIO data > registers, a secondary virtual register range is used to enable separate > cacheing properties of pin input and output values. caching ... > +static int rtl8231_reg_read(void *context, unsigned int reg, unsigned int *val) > +{ > + struct mdio_device *mdio_dev = context; > + int ret; > + > + ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, RTL8231_REAL_REG(reg)); > + > + if (ret < 0) > + return ret; > + > + *val = ret & 0xffff; > + > + return 0; > +} > + > +static int rtl8231_reg_write(void *context, unsigned int reg, unsigned int val) > +{ > + struct mdio_device *mdio_dev = context; > + > + return mdiobus_write(mdio_dev->bus, mdio_dev->addr, RTL8231_REAL_REG(reg), val); > +} Hmm... Maybe we can amend regmap-mdio to avoid duplication of the above? Something like xlate in gpio-regmap or so? ... > + mdiodev->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); Missed if (IS_ERR(mdiodev->reset_gpio)) return PTR_ERR(mdiodev->reset_gpio);
On Thu, 2021-06-03 at 13:58 +0300, Andy Shevchenko wrote: > On Thu, Jun 3, 2021 at 1:01 PM Sander Vanheule <sander@svanheule.net> wrote: > > > > The RTL8231 is implemented as an MDIO device, and provides a regmap > > interface for register access by the core and child devices. > > > > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek. > > Since kernel support for SMI is limited, and no real-world SMI > > implementations have been encountered for this device, this is currently > > unimplemented. The use of the regmap interface should make any future > > support relatively straightforward. > > > > After reset, all pins are muxed to GPIO inputs before the pin drivers > > are enabled. This is done to prevent accidental system resets, when a > > pin is connected to the parent SoC's reset line. > > > > To provide different read and write semantics for the GPIO data > > registers, a secondary virtual register range is used to enable separate > > cacheing properties of pin input and output values. > > caching > > ... > > > > +static int rtl8231_reg_read(void *context, unsigned int reg, unsigned int > > *val) > > +{ > > + struct mdio_device *mdio_dev = context; > > + int ret; > > + > > + ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, > > RTL8231_REAL_REG(reg)); > > + > > + if (ret < 0) > > + return ret; > > + > > + *val = ret & 0xffff; > > + > > + return 0; > > +} > > + > > +static int rtl8231_reg_write(void *context, unsigned int reg, unsigned int > > val) > > +{ > > + struct mdio_device *mdio_dev = context; > > + > > + return mdiobus_write(mdio_dev->bus, mdio_dev->addr, > > RTL8231_REAL_REG(reg), val); > > +} > > Hmm... Maybe we can amend regmap-mdio to avoid duplication of the > above? Something like xlate in gpio-regmap or so? > I wanted to make the masking explicit, but since regmap-mdio currently requires a register address width of 5 bit, it could move there. Actually, can we safely assume that any MDIO driver implementing clause-22 access (5-bit register address width) will just ignore higher bits? In that case, I could just drop these functions and not even modify regmap-mdio. It appears to work for bitbanged MDIO. > > + mdiodev->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > GPIOD_OUT_LOW); > > Missed > > if (IS_ERR(mdiodev->reset_gpio)) > return PTR_ERR(mdiodev->reset_gpio); > Will fix. Best, Sander
> I wanted to make the masking explicit, but since regmap-mdio currently requires > a register address width of 5 bit, it could move there. > > Actually, can we safely assume that any MDIO driver implementing clause-22 > access (5-bit register address width) will just ignore higher bits? In that > case, I could just drop these functions and not even modify regmap-mdio. It > appears to work for bitbanged MDIO. How are C45 addresses handled? The API to the MDIO bus driver uses a register value which is 32 bits in width. Bit 30 indicates the address is a C45 address, and then you have 21 bits of actual address. regmap-mdio needs to be generic and support both C22 and C45. Andrew
On Thu, 2021-06-03 at 16:03 +0200, Andrew Lunn wrote: > > I wanted to make the masking explicit, but since regmap-mdio currently > > requires > > a register address width of 5 bit, it could move there. > > > > Actually, can we safely assume that any MDIO driver implementing clause-22 > > access (5-bit register address width) will just ignore higher bits? In that > > case, I could just drop these functions and not even modify regmap-mdio. It > > appears to work for bitbanged MDIO. > > How are C45 addresses handled? The API to the MDIO bus driver uses a > register value which is 32 bits in width. Bit 30 indicates the address > is a C45 address, and then you have 21 bits of actual address. > regmap-mdio needs to be generic and support both C22 and C45. Currently regmap-mdio will only accept regmap_config structs where the register width is 5 bit, but this is not enforced for the reg_read/reg_write functions and the regnum is passed verbatim to mdiobus_read/mdiobus_write. So, if I understand correctly: * for a regmap configured for C22 access, register addresses should be masked with 0x1f to create a C22 regnum * for a regmap configured for C45 access, register addresses should be masked and formatted with (MII_ADDR_C45 | (mdiodev->addr << MII_DEVADDR_C45_SHIFT) | (reg_addr 0xffff)) I would think that a device that supports both C22 and C45 access, can then just be set up to have two regmaps. If they can be considered to be independent register spaces, one regmap for each access type would make sense to me. I'll cook up a patch for this. Best, Sander
On Thu, 2021-06-03 at 13:58 +0300, Andy Shevchenko wrote: > On Thu, Jun 3, 2021 at 1:01 PM Sander Vanheule <sander@svanheule.net> wrote: > > +static int rtl8231_reg_read(void *context, unsigned int reg, unsigned int > > *val) > > +{ > > + struct mdio_device *mdio_dev = context; > > + int ret; > > + > > + ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, > > RTL8231_REAL_REG(reg)); > > + > > + if (ret < 0) > > + return ret; > > + > > + *val = ret & 0xffff; > > + > > + return 0; > > +} > > + > > +static int rtl8231_reg_write(void *context, unsigned int reg, unsigned int > > val) > > +{ > > + struct mdio_device *mdio_dev = context; > > + > > + return mdiobus_write(mdio_dev->bus, mdio_dev->addr, > > RTL8231_REAL_REG(reg), val); > > +} > > Hmm... Maybe we can amend regmap-mdio to avoid duplication of the > above? Something like xlate in gpio-regmap or so? (+Cc Mark for the regmap discussion) It turns out that I can't use both devm_regmap_init_mdio and the overrides regmap_config.reg_read/write. This appears to be in contrast to what the documentation for these two overrides suggest. devm_regmap_init_mdio provides a bus for the regmap, which causes the overrides to be ignored in regmap.c Then I tried to use the paging support provided by regmap, by adding the following config: static struct regmap_range_cfg rtl8231_reg_ranges[] = { { /* Specify an unused register with an empty mask */ .selector_reg = 0x1f, .selector_mask = 0x00, .selector_shift = 0, .range_min = 0x00, .range_max = RTL8231_VREG(RTL8231_REG_COUNT - 1), .window_start = 0x00, .window_len = 0x20, }, }; This also doesn't work, because the used _regmap_bus_reg_read/write don't resolve register pages. The patch below fixes this, but maybe this missing functionality is intentional, and I should actually implement regmap_bus.read/write? ----8<---- diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 0d185ec018a5..20b6a2e0d2e3 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1881,6 +1881,15 @@ static int _regmap_bus_reg_write(void *context, unsigned int reg, unsigned int val) { struct regmap *map = context; + struct regmap_range_node *range; + int ret; + + range = _regmap_range_lookup(map, reg); + if (range) { + ret = _regmap_select_page(map, ®, range, 1); + if (ret != 0) + return ret; + } return map->bus->reg_write(map->bus_context, reg, val); } @@ -2651,6 +2660,15 @@ static int _regmap_bus_reg_read(void *context, unsigned int reg, unsigned int *val) { struct regmap *map = context; + struct regmap_range_node *range; + int ret; + + range = _regmap_range_lookup(map, reg); + if (range) { + ret = _regmap_select_page(map, ®, range, 1); + if (ret != 0) + return ret; + } return map->bus->reg_read(map->bus_context, reg, val); } -- Best, Sander
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI bus device. Currently only the MDIO mode is supported, although SMI mode support should be fairly straightforward, once an SMI bus driver is available. Provided features by the RTL8231: - Up to 37 GPIOs - Configurable drive strength: 8mA or 4mA (currently unsupported) - Input debouncing on high GPIOs (currently unsupported) - Up to 88 LEDs in multiple scan matrix groups - On, off, or one of six toggling intervals - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs - Up to one PWM output (currently unsupported) - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz) There remain some log warnings when probing the device, possibly due to the way I'm using the MFD subsystem. Would it be possible to avoid these? [ 2.602242] rtl8231-pinctrl: Failed to locate of_node [id: -2] [ 2.609380] rtl8231-pinctrl rtl8231-pinctrl.0.auto: no of_node; not parsing pinctrl DT When no 'leds' sub-node is specified: [ 2.922262] rtl8231-leds: Failed to locate of_node [id: -2] [ 2.967149] rtl8231-leds rtl8231-leds.1.auto: no of_node; not parsing pinctrl DT [ 2.975673] rtl8231-leds rtl8231-leds.1.auto: scan mode missing or invalid [ 2.983531] rtl8231-leds: probe of rtl8231-leds.1.auto failed with error -22 Changes since RFC: - Dropped MDIO regmap interface. I was unable to resolve the Kconfig dependency issue, so have reverted to using regmap_config.reg_read/write. - Added pinctrl support - Added LED support - Changed root device to MFD, with pinctrl and leds child devices. Root device is now an mdio_device driver. Sander Vanheule (5): dt-bindings: leds: Binding for RTL8231 scan matrix dt-bindings: mfd: Binding for RTL8231 mfd: Add RTL8231 core device pinctrl: Add RTL8231 pin control and GPIO support leds: Add support for RTL8231 LED scan matrix .../bindings/leds/realtek,rtl8231-leds.yaml | 159 ++++++ .../bindings/mfd/realtek,rtl8231.yaml | 202 +++++++ drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/leds-rtl8231.c | 281 ++++++++++ drivers/mfd/Kconfig | 9 + drivers/mfd/Makefile | 1 + drivers/mfd/rtl8231.c | 163 ++++++ drivers/pinctrl/Kconfig | 10 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-rtl8231.c | 497 ++++++++++++++++++ include/linux/mfd/rtl8231.h | 49 ++ 12 files changed, 1383 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml create mode 100644 drivers/leds/leds-rtl8231.c create mode 100644 drivers/mfd/rtl8231.c create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c create mode 100644 include/linux/mfd/rtl8231.h