| Message ID | 20221215213206.56666-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add RZ/G2L POEG support | expand |
On Thu, Dec 15, 2022 at 10:32 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> This patch series add support for controlling output disable function using sysfs.
What's wrong with using the debugfs approach Drew implemented in
commit 6199f6becc869d30ca9394ca0f7a484bf9d598eb
"pinctrl: pinmux: Add pinmux-select debugfs file"
?
Something driver specific seems like a bit of a hack, does it not?
If this should go into sysfs we should probably create something
generic, such as a list of stuff to be exported as sysfs switches.
It generally also looks really dangerous, which is another reason
for keeping it in debugfs. It's the big hammer to hurt yourself with,
more or less.
Yours,
Linus Walleij
Hi Linus, On Thu, Dec 29, 2022 at 2:17 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Dec 15, 2022 at 10:32 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > This patch series add support for controlling output disable function using sysfs. > > What's wrong with using the debugfs approach Drew implemented in > commit 6199f6becc869d30ca9394ca0f7a484bf9d598eb > "pinctrl: pinmux: Add pinmux-select debugfs file" > ? I think the main difference is that debugfs is meant for debugging and development features, while this feature is to be configured on production systems. There's just no existing API for it. > Something driver specific seems like a bit of a hack, does it not? > > If this should go into sysfs we should probably create something > generic, such as a list of stuff to be exported as sysfs switches. > > It generally also looks really dangerous, which is another reason > for keeping it in debugfs. It's the big hammer to hurt yourself with, > more or less. Yes, generic would be nice. Anyone familiar with other hardware that could make use of this? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Linus, On Tue, Jan 3, 2023 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Dec 29, 2022 at 2:17 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Dec 15, 2022 at 10:32 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > This patch series add support for controlling output disable function using sysfs. > > > > What's wrong with using the debugfs approach Drew implemented in > > commit 6199f6becc869d30ca9394ca0f7a484bf9d598eb > > "pinctrl: pinmux: Add pinmux-select debugfs file" > > ? > > I think the main difference is that debugfs is meant for debugging > and development features, while this feature is to be configured on > production systems. There's just no existing API for it. > > > Something driver specific seems like a bit of a hack, does it not? > > > > If this should go into sysfs we should probably create something > > generic, such as a list of stuff to be exported as sysfs switches. > > > > It generally also looks really dangerous, which is another reason > > for keeping it in debugfs. It's the big hammer to hurt yourself with, > > more or less. > > Yes, generic would be nice. Anyone familiar with other hardware > that could make use of this? That's also the reason why I have been rather hesitant in accepting this driver and bindings (I just saw you applied the bindings): I wanted to hear your input first ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Jan 3, 2023 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > If this should go into sysfs we should probably create something > > generic, such as a list of stuff to be exported as sysfs switches. > > > > It generally also looks really dangerous, which is another reason > > for keeping it in debugfs. It's the big hammer to hurt yourself with, > > more or less. > > Yes, generic would be nice. Anyone familiar with other hardware > that could make use of this? Drew was using this for Beagle Bone IIRC, Drew? Yours, Linus Walleij
Hi Linus, On Mon, Jan 9, 2023 at 2:16 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jan 3, 2023 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > If this should go into sysfs we should probably create something > > > generic, such as a list of stuff to be exported as sysfs switches. > > > > > > It generally also looks really dangerous, which is another reason > > > for keeping it in debugfs. It's the big hammer to hurt yourself with, > > > more or less. > > > > Yes, generic would be nice. Anyone familiar with other hardware > > that could make use of this? > > Drew was using this for Beagle Bone IIRC, Drew? Yes, that's what I remember, too. And I tested it on Koelsch. But again, that's for debugging purposes. For non-debugging operation, we need something different. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Jan 09, 2023 at 02:41:24PM +0100, Geert Uytterhoeven wrote: > On Mon, Jan 9, 2023 at 2:16 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 3, 2023 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > If this should go into sysfs we should probably create something > > > > generic, such as a list of stuff to be exported as sysfs switches. > > > > > > > > It generally also looks really dangerous, which is another reason > > > > for keeping it in debugfs. It's the big hammer to hurt yourself with, > > > > more or less. > > > > > > Yes, generic would be nice. Anyone familiar with other hardware > > > that could make use of this? > > > > Drew was using this for Beagle Bone IIRC, Drew? > > Yes, that's what I remember, too. And I tested it on Koelsch. > > But again, that's for debugging purposes. For non-debugging > operation, we need something different. I really would like to know the use case when you need to mux pins at run time. And the guarantee it won't break users' devices into smoke or killing somebody playing with robots.
On Mon, Jan 09, 2023 at 04:00:35PM +0200, Andy Shevchenko wrote: > On Mon, Jan 09, 2023 at 02:41:24PM +0100, Geert Uytterhoeven wrote: > > On Mon, Jan 9, 2023 at 2:16 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > On Tue, Jan 3, 2023 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > If this should go into sysfs we should probably create something > > > > > generic, such as a list of stuff to be exported as sysfs switches. > > > > > > > > > > It generally also looks really dangerous, which is another reason > > > > > for keeping it in debugfs. It's the big hammer to hurt yourself with, > > > > > more or less. > > > > > > > > Yes, generic would be nice. Anyone familiar with other hardware > > > > that could make use of this? > > > > > > Drew was using this for Beagle Bone IIRC, Drew? > > > > Yes, that's what I remember, too. And I tested it on Koelsch. > > > > But again, that's for debugging purposes. For non-debugging > > operation, we need something different. > > I really would like to know the use case when you need to mux pins at run time. > And the guarantee it won't break users' devices into smoke or killing somebody > playing with robots. Btw, I might agree on having something like this in production, but enabling it should print a BIG warning that the functionality is DANGEROUS. Something like trace_printk() has.
Hi Linus Walleij, Thanks for the feedback. > Subject: Re: [PATCH v5 0/9] Add RZ/G2L POEG support > > On Thu, Dec 15, 2022 at 10:32 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > This patch series add support for controlling output disable function > using sysfs. > > What's wrong with using the debugfs approach Drew implemented in commit > 6199f6becc869d30ca9394ca0f7a484bf9d598eb > "pinctrl: pinmux: Add pinmux-select debugfs file" > ? I am not sure, we supposed to use debugfs for production environment?? Currently output pins of the general PWM timer (GPT) can be disabled by using the below methods. 1) Register setting(ie, by setting POEGGn.SSF to 1) --> by Software control 2) Input level detection of the GTETRGA to GTETRGD pins-> sending an active level signal to disable the output pins of PWM. 3) Output-disable request from the GPT--> Here GPT detects short circuits and request POEG to disable the output pins. In case, if any one doesn't want to use 2) and 3), we should be able to disable output pins of the general PWM timer (GPT) by register control. > > Something driver specific seems like a bit of a hack, does it not? > > If this should go into sysfs we should probably create something generic, Yes, generic sysfs entry will be good > such as a list of stuff to be exported as sysfs switches. Can you please elaborate? Or Point me to an example for this? Cheers, Biju
On Mon, Jan 9, 2023 at 2:41 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Jan 9, 2023 at 2:16 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 3, 2023 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > If this should go into sysfs we should probably create something > > > > generic, such as a list of stuff to be exported as sysfs switches. > > > > > > > > It generally also looks really dangerous, which is another reason > > > > for keeping it in debugfs. It's the big hammer to hurt yourself with, > > > > more or less. > > > > > > Yes, generic would be nice. Anyone familiar with other hardware > > > that could make use of this? > > > > Drew was using this for Beagle Bone IIRC, Drew? > > Yes, that's what I remember, too. And I tested it on Koelsch. > > But again, that's for debugging purposes. For non-debugging > operation, we need something different. Actually Drew's usecase wasn't for debugging. It was kind-of production, but it was for "one-offs" such as factory lines and other very specific-purpose embedded. The placement in debugfs was mostly because it is fragile and dangerous. Yours, Linus Walleij
On Mon, Jan 9, 2023 at 4:10 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > What's wrong with using the debugfs approach Drew implemented in commit > > 6199f6becc869d30ca9394ca0f7a484bf9d598eb > > "pinctrl: pinmux: Add pinmux-select debugfs file" > > ? > > I am not sure, we supposed to use debugfs for production environment?? It depends what is meant by "production environment". If you mean a controlled environment "one-off" such as a factory line, a specific installation for a specific purpose such as a water purifier, that is very custom and hacked together for that one usecase. It will have other hacks too, so then Beagle is using debugfs in "production" if that is what you mean by "production", i.e. used to produce something. This is the same "production" use cases as used by i.e. the GPIO character device. If you mean that you are producing 6 million laptops where userspace is going to hammer this constantly, then no. In that case a real sysfs knob and ABI contract is needed. Usually vendors know which usecase their hardware is intended for, there is in my experience no unknown target audience, so which one is it in your case? > > such as a list of stuff to be exported as sysfs switches. > > Can you please elaborate? Or Point me to an example for this? Not sure what to say about that, you will have to invent something I'm afraid, good examples are in Documentation/ABI. Yours, Linus Walleij
Hi Linus Walleij, Thanks for the feedback. > Subject: Re: [PATCH v5 0/9] Add RZ/G2L POEG support > > On Mon, Jan 9, 2023 at 4:10 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > What's wrong with using the debugfs approach Drew implemented in > > > commit 6199f6becc869d30ca9394ca0f7a484bf9d598eb > > > "pinctrl: pinmux: Add pinmux-select debugfs file" > > > ? > > > > I am not sure, we supposed to use debugfs for production environment?? > > It depends what is meant by "production environment". Sorry for the confusion. I meant the final product used by the customers. I was under the impression that debugfs is for hacks and debugging. (For eg: if the HW doesn't have a USB3 function port, but using debugfs, we can emulate the condition and test) > > If you mean a controlled environment "one-off" such as a factory line, a > specific installation for a specific purpose such as a water purifier, that > is very custom and hacked together for that one usecase. It will have other > hacks too, so then Beagle is using debugfs in "production" > if that is what you mean by "production", i.e. used to produce something. > > This is the same "production" use cases as used by i.e. the GPIO character > device. > > If you mean that you are producing 6 million laptops where userspace is > going to hammer this constantly, then no. In that case a real sysfs knob and > ABI contract is needed. > > Usually vendors know which usecase their hardware is intended for, there is > in my experience no unknown target audience, so which one is it in your > case? POEG use case is related to protection from system failure(disable output pins in case of short circuits) Either 1) we detect externally and use software control to disable output pins --> Here I am using sysfs variable (/sys/devices/platform/soc/10049400.poeg/output_disable) to control it. Or 2) we detect externally and send an active level signal to disable output pins Or 3) we detect internally using GPT(PWM) and disable output pins --> Here 3 options or combination is possible for configuring the short circuit detection like 1) Dead Time Error Output Disable Request Enable 2) Same Time Output Level High Disable Request Enable 3) Same Time Output Level Low Disable Request Enable I have exported 3 sysfs variables for configuring these 3 options. 1) /sys/devices/platform/soc/10049400.poeg/gpt_req_both_high 2) /sys/devices/platform/soc/10049400.poeg/gpt_req_both_low 3) /sys/devices/platform/soc/10049400.poeg/gpt_req_deadtime_err > > > > such as a list of stuff to be exported as sysfs switches. > > > > Can you please elaborate? Or Point me to an example for this? > > Not sure what to say about that, you will have to invent something I'm > afraid, good examples are in Documentation/ABI. If it is preferable to use debugfs compared to sysfs for the use cases I mentioned above, I could change it to debugfs like Drew's patch. Please let me know. Cheers, Biju
On Tue, Jan 10, 2023 at 09:09:21AM +0100, Linus Walleij wrote: > On Mon, Jan 9, 2023 at 2:41 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Jan 9, 2023 at 2:16 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > On Tue, Jan 3, 2023 at 10:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > If this should go into sysfs we should probably create something > > > > > generic, such as a list of stuff to be exported as sysfs switches. > > > > > > > > > > It generally also looks really dangerous, which is another reason > > > > > for keeping it in debugfs. It's the big hammer to hurt yourself with, > > > > > more or less. > > > > > > > > Yes, generic would be nice. Anyone familiar with other hardware > > > > that could make use of this? > > > > > > Drew was using this for Beagle Bone IIRC, Drew? > > > > Yes, that's what I remember, too. And I tested it on Koelsch. > > > > But again, that's for debugging purposes. For non-debugging > > operation, we need something different. > > Actually Drew's usecase wasn't for debugging. It was kind-of production, > but it was for "one-offs" such as factory lines and other very specific-purpose > embedded. > > The placement in debugfs was mostly because it is fragile and dangerous. > > Yours, > Linus Walleij For the BeagleBone, the use case for selecting pin fuctions from userspace with pinmux-select in debugfs is to allow for rapid prototyping situations such as breadboarding. Our Debian install on the boards has an utility named config-pin that allows the user to select between defined pinctrl states for each pin on the expansion header. Some users like this as it means they do not need to constantly be editing device tree files and rebooting while protoyping. I agree that this is not a fool-proof scheme but Beaglebones have been shipping with this functionality for many years without any significant problems that I'm aware of. I do admit that it is possible for someone to potentially damage circuits with this flexibility, so having a warning in the kernel log like Andy suggested elsewhere in this thread might be a good idea. Thanks, Drew
Hi Linus, Thanks for feedback. > Subject: Re: [PATCH v5 0/9] Add RZ/G2L POEG support > > On Mon, Jan 9, 2023 at 4:10 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > What's wrong with using the debugfs approach Drew implemented in > > > commit 6199f6becc869d30ca9394ca0f7a484bf9d598eb > > > "pinctrl: pinmux: Add pinmux-select debugfs file" > > > ? > > > > I am not sure, we supposed to use debugfs for production environment?? > > It depends what is meant by "production environment". > > If you mean a controlled environment "one-off" such as a factory line, a > specific installation for a specific purpose such as a water purifier, that > is very custom and hacked together for that one usecase. It will have other > hacks too, so then Beagle is using debugfs in "production" > if that is what you mean by "production", i.e. used to produce something. > > This is the same "production" use cases as used by i.e. the GPIO character > device. > > If you mean that you are producing 6 million laptops where userspace is > going to hammer this constantly, then no. In that case a real sysfs knob and > ABI contract is needed. > > Usually vendors know which usecase their hardware is intended for, there is > in my experience no unknown target audience, so which one is it in your > case? > > > > such as a list of stuff to be exported as sysfs switches. > > > > Can you please elaborate? Or Point me to an example for this? > > Not sure what to say about that, you will have to invent something I'm > afraid, good examples are in Documentation/ABI. For generic approach, here is my plan From userspace echo "fname gname config configvalue" > output_disable At core level, we identify the device associated with the above "fname gname" --> this will be a new api. Core calls respective pincontrol driver with output_disable_cb(dev, fname, gname, config configvalue)--> there will be a new cb in pinctrl. The pincontrol driver has a list of devices with device_output_disable callbacks which will be registered by different drivers (eg:poeg_driver, poe3-driver) The poeg_driver gets callback and it will match against "fname gname" and configures the value. If anything fails, it will report error and propagates to user space. I will prototype based on the above. Please let me know if you have different opinion. Cheers, Biju
The output pins of the general PWM timer (GPT) can be disabled by using the port output enabling function for the GPT (POEG). Specifically, either of the following ways can be used. * Input level detection of the GTETRGA to GTETRGD pins. * Output-disable request from the GPT. * Register setting(ie, by setting POEGGn.SSF to 1) This patch series add support for controlling output disable function using sysfs. For output disable operation, POEG group needs to be linked with GPT. Plan to send a follow up patch with renesas,poeg-group as numeric property in pwm bindings for linking both GPT and POEG devices. Patch #3 to patch #9 are new and just for testing only(GPT Output-disable request to POEG) When dead time error occurs or the GTIOCA pin output value is the same as the GTIOCB pin output value, output protection is required. GPT detects this condition and generates output disable requests to POEG based on the settings in the output disable request permission bits, such as GTINTAD.GRPDTE, GTINTAD.GRPABH, GTINTAD.GRPABL. After the POEG receives output disable requests from each channel and calculates external input using an OR operation, the POEG generates output disable requests to GPT. POEG handles output disable request and send an event to userspace. Userspace clears the fault condition and request poeg to cancel the output disable request. Logs: root@smarc-rzg2l:~# /poeg_app & [1] 182 root@smarc-rzg2l:~# [POEG]open root@smarc-rzg2l:~# /poeg.sh 16 pwmchip0 Test case 1 user POEG control 72: 0 0 GICv3 354 Level 10048800.poeg 73: 0 0 GICv3 355 Level 10048c00.poeg 74: 0 0 GICv3 356 Level 10049000.poeg 76: 0 0 GICv3 357 Level 10049400.poeg Read at address 0x10048434 (0xffff8d92b434): 0x0200031B Read at address 0x10048438 (0xffff9884e438): 0x03000000 Read at address 0x10049400 (0xffffbd7b9400): 0x00000030 Test case 2 user GPT control both high Read at address 0x10048434 (0xffff96dcf434): 0x021B031B Read at address 0x10048438 (0xffff9ddfc438): 0x23000000 Read at address 0x10049400 (0xffff9a206400): 0x00000030 gpt ch:3, irq=2 gpt ch:3, irq=2 Read at address 0x10048434 (0xffff97fb2434): 0x021B031B Read at address 0x10048438 (0xffff817a1438): 0x03000000 Read at address 0x10049400 (0xffffb8f5e400): 0x00000030 gpt ch:3, irq=2 72: 0 0 GICv3 354 Level 10048800.poeg 73: 0 0 GICv3 355 Level 10048c00.poeg 74: 0 0 GICv3 356 Level 10049000.poeg 76: 6 0 GICv3 357 Level 10049400.poeg Test case 3 user GPT control both low Read at address 0x10048434 (0xffff897d8434): 0x031B031B Read at address 0x10048438 (0xffffa981a438): 0x43000000 Read at address 0x10049400 (0xffff90eba400): 0x00000030 gpt ch:3, irq=4 gpt ch:3, irq=4 Read at address 0x10048434 (0xffff959d7434): 0x021B031B Read at address 0x10048438 (0xffff92b6d438): 0x03000000 Read at address 0x10049400 (0xffffb60b3400): 0x00000030 gpt ch:3, irq=4 72: 0 0 GICv3 354 Level 10048800.poeg 73: 0 0 GICv3 355 Level 10048c00.poeg 74: 0 0 GICv3 356 Level 10049000.poeg 76: 12 0 GICv3 357 Level 10049400.poeg root@smarc-rzg2l:~# v4->v5: * Added Rb tag from Rob. * Updated kernel version in sysfs doc. v3->v4: * Replaced companion->renesas,gpt for the phandle to gpt instance * Replaced renesas,id->renesas,poeg-id * Removed default from renesas,poeg-id as default for a required property doesn't make much sense. * Updated the example and required properties with above changes v2->v3: * Removed Rb tag from Rob as there are some changes introduced. * Added companion property, so that poeg can link with gpt device * Documented renesas,id, as identifier for POEGG{A,B,C,D}. * Updated the binding example. * Added sysfs documentation for output_disable * PWM_RZG2L_GPT implies ARCH_RZG2L. So removed ARCH_RZG2L dependency * Used dev_get_drvdata to get device data * Replaced sprintf->sysfs_emit in show(). v1->v2: * Updated binding description. * Renamed the file poeg-rzg2l->rzg2l-poeg * Removed the macro POEGG as there is only single register and updated rzg2l_poeg_write() and rzg2l_poeg_read() * Updated error handling in probe() REF->v1: * Modelled as pincontrol as most of its configuration is intended to be static and moved driver files from soc to pincontrol directory. * Updated reg size in dt binding example. * Updated Kconfig REF: https://lore.kernel.org/linux-renesas-soc/20220510151112.16249-1-biju.das.jz@bp.renesas.com/ Biju Das (9): dt-bindings: pinctrl: renesas: Add RZ/G2L POEG binding drivers: pinctrl: renesas: Add RZ/G2L POEG driver support pwm: rzg2l-gpt: Add support for output disable request from gpt pinctrl: renesas: rzg2l-poeg: Add support for GPT Output-Disable Request pwm: rzg2l-gpt: Add support for output disable when both output low pinctrl: renesas: rzg2l-poeg: output-disable request from GPT when both outputs are low. pwm: rzg2l-gpt: Add support for output disable on dead time error pinctrl: renesas: rzg2l-poeg: output-disable request from GPT on dead time error tools/poeg: Add test app for poeg .../ABI/testing/sysfs-platform-rzg2l-poeg | 87 ++++ .../bindings/pinctrl/renesas,rzg2l-poeg.yaml | 86 ++++ drivers/pinctrl/renesas/Kconfig | 2 + drivers/pinctrl/renesas/Makefile | 2 + drivers/pinctrl/renesas/poeg/Kconfig | 11 + drivers/pinctrl/renesas/poeg/Makefile | 2 + drivers/pinctrl/renesas/poeg/rzg2l-poeg.c | 476 ++++++++++++++++++ drivers/pwm/pwm-rzg2l-gpt.c | 129 +++++ include/linux/soc/renesas/rzg2l-gpt.h | 44 ++ include/linux/soc/renesas/rzg2l-poeg.h | 16 + tools/poeg/Build | 1 + tools/poeg/Makefile | 53 ++ tools/poeg/poeg_app.c | 60 +++ 13 files changed, 969 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-platform-rzg2l-poeg create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rzg2l-poeg.yaml create mode 100644 drivers/pinctrl/renesas/poeg/Kconfig create mode 100644 drivers/pinctrl/renesas/poeg/Makefile create mode 100644 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c create mode 100644 include/linux/soc/renesas/rzg2l-gpt.h create mode 100644 include/linux/soc/renesas/rzg2l-poeg.h create mode 100644 tools/poeg/Build create mode 100644 tools/poeg/Makefile create mode 100644 tools/poeg/poeg_app.c