| Message ID | cover.1621809029.git.sander@svanheule.net |
|---|---|
| Headers | show |
| Series | RTL8231 GPIO expander support | expand |
> 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 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. -- With Best Regards, Andy Shevchenko
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
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 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?
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.
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. >
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
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).
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 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
Am 2021-05-31 10:36, schrieb Sander Vanheule: > 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. 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) I'm not sure if regmap can cache the value for us or if we have to do it ourselves. > 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; Ha I've thought of the same thing, but there might be hardware which actually mux the data in and data out register. Thus I think we have to distiguish between: (1) write only data registers (2) muxed data in/data out according to the direction for (1) we'd have to cache the value (ourselves (?)) for (2) we'd only need to drop a cached value if we switch directions > 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); mh, I don't see how this will work with a write only register. I seems that you might accidentially change the values of the other GPIOs in this registers (that is depending on the input of them, because you are still doing a RMW). > 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, -michael
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. -- With Best Regards, Andy Shevchenko
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
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.)
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. -- With Best Regards, Andy Shevchenko
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) 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