| Message ID | 20230306090014.128732-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add pinctrl sysfs and RZ/G2L POEG support | expand |
Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti: > Add pinctrl_get_device() to find a device handle associated with > a pincontrol group(i.e. by matching function name and group name for a > device). This device handle can then be used for finding match for the > pin output disable device that protects device against short circuits > on the pins. Not sure I understand the use case. Please, create a better commit message. Also it is missing the explanation why there will be no collisions when looking by the same pair of function and group name from different device. (Always imagine that you have 2+ same IP blocks on the platform before doing any pin control core work. This will help you to design it properly. )
Mon, Mar 06, 2023 at 09:00:04AM +0000, Biju Das kirjoitti: > Add a simple sysfs interface to the generic pinctrl framework > for configuring pins for output disable operation. > > /sys/class/pinctrl/ > `-- output-disable/ > |-- configure (w/o) ask the kernel to configure a pin group > for output disable operation. > > echo "<group-name function-name 0 1>" > configure > > The existing "pinmux-functions" debugfs file lists the pin functions > registered for the pin controller. For example: > > function 0: usb0, groups = [ usb0 ] > function 1: usb1, groups = [ usb1 ] > function 2: gpt4-pins, groups = [ gpt4-pins ] > function 3: scif0, groups = [ scif0 ] > function 4: scif2, groups = [ scif2 ] > function 5: spi1, groups = [ spi1 ] > > To configure gpt4-pins for output disable activation by user: > > echo "gpt4-pins gpt4-pins 0 1" > configure ... > +static struct attribute *pinctrl_attrs[] = { > + &dev_attr_configure.attr, > + NULL, No comma for a terminator entry. > +};
Hi Andy Shevchenko, Thanks for the feedback. > -----Original Message----- > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com> > Sent: Monday, March 6, 2023 11:36 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Linus Walleij <linus.walleij@linaro.org>; Philipp Zabel > <p.zabel@pengutronix.de>; Geert Uytterhoeven <geert+renesas@glider.be>; > Thierry Reding <thierry.reding@gmail.com>; Uwe Kleine-König <u.kleine- > koenig@pengutronix.de>; linux-pwm@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; linux-gpio@vger.kernel.org; Chris Paterson > <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com> > Subject: Re: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG > driver support > > Mon, Mar 06, 2023 at 09:00:07AM +0000, Biju Das kirjoitti: > > The output pins of the RZ/G2L general PWM timer (GPT) can be disabled > > by using the port output enabling function for the GPT (POEG). > > > > Add basic support using s/w control through generic pincontrol sysfs > > to enable/disable output from GPT by registering with RZ/G2L > > pincontrol driver. > > You have wrong Subject prefix. Oops. Will fix. > > ... > > > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 data) > > +{ > > + iowrite32(data, chip->mmio); > > +} > > + > > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) { > > + return ioread32(chip->mmio); > > +} > > Why not regmap MMIO? Some drivers used iowrite32, some uses writel, some uses regmap. will use regmap for read/write,If the preference is regmap MMIO as it comes with spinlock for MMIO access. > > ... > > > +static struct platform_driver rzg2l_poeg_driver = { > > + .driver = { > > + .name = "rzg2l-poeg", > > + .of_match_table = of_match_ptr(rzg2l_poeg_of_table), > > Why do you need of_match_ptr()? Not needed. Will remove it. Cheers, Biju > > > + }, > > + .probe = rzg2l_poeg_probe, > > + .remove = rzg2l_poeg_remove, > > +}; > > -- > With Best Regards, > Andy Shevchenko >
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [DO NOT APPLY PATCH v6 10/13] pinctrl: renesas: rzg2l-poeg: > output-disable request from GPT when both outputs are low. > > Mon, Mar 06, 2023 at 09:00:11AM +0000, Biju Das kirjoitti: > > This patch adds support fpr output-disable requests from GPT, when > > both outputs are low. > > > > Added sysfs to enable/disable for configuring GPT output disable > > request when both outputs are low. > > ... > > > +static int rzg2l_poeg_output_disable_both_low(struct rzg2l_poeg_chip > *chip, > > + bool enable) > > +{ > > + if (enable) > > + set_bit(RZG2L_GPT_OABLF, chip->gpt_irq); > > + else > > + clear_bit(RZG2L_GPT_OABLF, chip->gpt_irq); > > JFYI: assign_bit() OK, will use assign_bit() and remove the above code. Cheers, Biju > > > > + rzg2l_gpt_poeg_disable_req_both_low(chip->gpt_dev, chip->index, > > + test_bit(RZG2L_GPT_OABLF, chip- > >gpt_irq)); > > + > > + return 0; > > +} > > -- > With Best Regards, > Andy Shevchenko >
On Tue, Mar 7, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com> > > Sent: Monday, March 6, 2023 11:36 PM > > Mon, Mar 06, 2023 at 09:00:07AM +0000, Biju Das kirjoitti: ... > > > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 data) > > > +{ > > > + iowrite32(data, chip->mmio); > > > +} > > > + > > > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) { > > > + return ioread32(chip->mmio); > > > +} > > > > Why not regmap MMIO? > > Some drivers used iowrite32, some uses writel, some uses regmap. > > will use regmap for read/write,If the preference is regmap MMIO > as it comes with spinlock for MMIO access. Lock can be disabled. It's up to the user of regmap.
On Tue, Mar 7, 2023 at 12:10 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > On Tue, Mar 7, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com> > > > > Sent: Monday, March 6, 2023 11:36 PM Mon, Mar 06, 2023 at 09:00:07AM > > > > +0000, Biju Das kirjoitti: ... > > > > > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 > > > > > +data) { > > > > > + iowrite32(data, chip->mmio); > > > > > +} > > > > > + > > > > > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) { > > > > > + return ioread32(chip->mmio); > > > > > +} > > > > > > > > Why not regmap MMIO? > > > > > > Some drivers used iowrite32, some uses writel, some uses regmap. > > > > > > will use regmap for read/write,If the preference is regmap MMIO as it > > > comes with spinlock for MMIO access. > > > > Lock can be disabled. It's up to the user of regmap. > > Ok, Just want to double check, > POEG has a single 32 bit register. So it worth to use regmap? > A simple readl/write is sufficient no?? It can be. But can you explain why you used iowriteXX() / ioreadXX() instead of writeX()/readX()?
On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Add a simple sysfs interface to the generic pinctrl framework > for configuring pins for output disable operation. > > /sys/class/pinctrl/ > `-- output-disable/ > |-- configure (w/o) ask the kernel to configure a pin group > for output disable operation. > > echo "<group-name function-name 0 1>" > configure > > The existing "pinmux-functions" debugfs file lists the pin functions > registered for the pin controller. For example: > > function 0: usb0, groups = [ usb0 ] > function 1: usb1, groups = [ usb1 ] > function 2: gpt4-pins, groups = [ gpt4-pins ] > function 3: scif0, groups = [ scif0 ] > function 4: scif2, groups = [ scif2 ] > function 5: spi1, groups = [ spi1 ] > > To configure gpt4-pins for output disable activation by user: > > echo "gpt4-pins gpt4-pins 0 1" > configure > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> You have to run things like this by Greg K-H (adde on To) > +static struct class pinctrl_class = { > + .name = "pinctrl", > + .owner = THIS_MODULE, > + .dev_groups = pinctrl_groups, > +}; Why are you adding a new device class? IIRC Greg don't like them, we should probably just deal with the devices directly on the bus where they are, which also implies some topology search etc which is a feature. Yours, Linus Walleij
Hi Linus Walleij, Thanks for the feedback. > Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support > > On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Add a simple sysfs interface to the generic pinctrl framework for > > configuring pins for output disable operation. > > > > /sys/class/pinctrl/ > > `-- output-disable/ > > |-- configure (w/o) ask the kernel to configure a pin group > > for output disable operation. > > > > echo "<group-name function-name 0 1>" > configure > > > > The existing "pinmux-functions" debugfs file lists the pin functions > > registered for the pin controller. For example: > > > > function 0: usb0, groups = [ usb0 ] > > function 1: usb1, groups = [ usb1 ] > > function 2: gpt4-pins, groups = [ gpt4-pins ] > > function 3: scif0, groups = [ scif0 ] > > function 4: scif2, groups = [ scif2 ] > > function 5: spi1, groups = [ spi1 ] > > > > To configure gpt4-pins for output disable activation by user: > > > > echo "gpt4-pins gpt4-pins 0 1" > configure > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > You have to run things like this by Greg K-H (adde on To) > > > +static struct class pinctrl_class = { > > + .name = "pinctrl", > > + .owner = THIS_MODULE, > > + .dev_groups = pinctrl_groups, > > +}; > > Why are you adding a new device class? > > IIRC Greg don't like them, we should probably just deal with the devices > directly on the bus where they are, which also implies some topology search > etc which is a feature. I am ok for both, I thought adding new device class will be more generic and people can use simple sysfs[1] like pwm for output disable operation rather than the SoC specific operation[2]. [1] /sys/class/pinctrl/output-disable/configure vs [2] /sys/devices/platform/soc/11030000.pinctrl/output_disable Cheers, Biju
On Tue, Mar 7, 2023 at 10:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti: ... > > > Add pinctrl_get_device() to find a device handle associated with a > > > pincontrol group(i.e. by matching function name and group name for a > > > device). This device handle can then be used for finding match for the > > > pin output disable device that protects device against short circuits > > > on the pins. > > > > Not sure I understand the use case. Please, create a better commit message. > > OK, Basically pinmux_enable_setting allows exclusive access of pin to a device. > It won't allow multiple devices to claim a pin. Which is correct. No? Show me the schematics of the real use case for that. The owner of the pin is the host side. I can't imagine how the same pin is shared inside the SoC. Is it broken hardware design? ... > > Also it is missing the explanation why there will be no collisions when > > looking by the same pair of function and group name from different device. > > setting->data.mux will be unique for a pin. So there won't be a collision when > looking by the same pair of function and group name from different device. > > > (Always imagine that you have 2+ same IP blocks on the platform before doing > > any pin control core work. This will help you to design it properly. ) Not sure how the pair function_name group_name makes the device unique.
On Tue, Mar 7, 2023 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti: > > > Add pinctrl_get_device() to find a device handle associated with a > > > pincontrol group(i.e. by matching function name and group name for a > > > device). This device handle can then be used for finding match for the > > > pin output disable device that protects device against short circuits > > > on the pins. > > > > Not sure I understand the use case. Please, create a better commit message. > > OK, Basically pinmux_enable_setting allows exclusive access of pin to a device. > It won't allow multiple devices to claim a pin. So what is the use case? Which two devices need to use the same pin at the same time? You can already: 1) Use the same pin with different devices at different times, because pin configs can be changed arbitrarily at runtime, see for example: drivers/i2c/muxes/i2c-demux-pinctrl.c 2) Mux a pin to a certain device *and* use it for GPIO at the same time, all that is needed is to set .strict to false in struct pinmux_ops. This should be false if e.g. the GPIO can be used to "sample" the output of a I2C block connected to the same pins, so the two functions (I2C and GPIO) are not electrically decoupled. So do you really have a use case where two devices need to use the same pin at the same time? I've seen much but I haven't seen this before! Which two devices are that? Yours, Linus Walleij
Hi Linus and Andy, > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Tue, Mar 7, 2023 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti: > > > > > Add pinctrl_get_device() to find a device handle associated with a > > > > pincontrol group(i.e. by matching function name and group name for > > > > a device). This device handle can then be used for finding match > > > > for the pin output disable device that protects device against > > > > short circuits on the pins. > > > > > > Not sure I understand the use case. Please, create a better commit > message. > > > > OK, Basically pinmux_enable_setting allows exclusive access of pin to a > device. > > It won't allow multiple devices to claim a pin. > > So what is the use case? Which two devices need to use the same pin at the > same time? Andy asked a question Can a pin be used by multiple devices at same time My answer is no. The reason is, There will be a single owner claiming a pin at given time. setting->data.mux will be unique for a pin. So there won't be a collision when looking by the same pair of function and group name from different device. > > You can already: > > 1) Use the same pin with different devices at different times, because > pin configs can be changed arbitrarily at runtime, see for example: > drivers/i2c/muxes/i2c-demux-pinctrl.c Agreed. > > 2) Mux a pin to a certain device *and* use it for GPIO at the same time, > all that is needed is to set .strict to false in struct pinmux_ops. > This should be false if e.g. the GPIO can be used to "sample" the > output of a I2C block connected to the same pins, so the two > functions (I2C and GPIO) are not electrically decoupled. > > So do you really have a use case where two devices need to use the same pin > at the same time? I've seen much but I haven't seen this before! > Which two devices are that? I don't have a use case like that. Cheers, Biju
Hi Andy, Thanks for the feedback. > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Tue, Mar 7, 2023 at 10:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das > kirjoitti: > > ... > > > > > Add pinctrl_get_device() to find a device handle associated with a > > > > pincontrol group(i.e. by matching function name and group name for > > > > a device). This device handle can then be used for finding match > > > > for the pin output disable device that protects device against > > > > short circuits on the pins. > > > > > > Not sure I understand the use case. Please, create a better commit > message. > > > > OK, Basically pinmux_enable_setting allows exclusive access of pin to a > device. > > It won't allow multiple devices to claim a pin. > > Which is correct. No? Show me the schematics of the real use case for that. > > The owner of the pin is the host side. I can't imagine how the same pin is > shared inside the SoC. Is it broken hardware design? We are discussing usage of echo "fname gname" and you asked a question whether multiple devices can claim a pin at the same time and my answer is no. as setting->data.mux will be unique for a pin and will be claimed by device during commit state. Am I missing anything here?? Cheers, Biju > > ... > > > > Also it is missing the explanation why there will be no collisions > > > when looking by the same pair of function and group name from different > device. > > > > setting->data.mux will be unique for a pin. So there won't be a > > setting->collision when > > looking by the same pair of function and group name from different device. > > > > > (Always imagine that you have 2+ same IP blocks on the platform > > > before doing any pin control core work. This will help you to design > > > it properly. ) > > Not sure how the pair function_name group_name makes the device unique. Do you agree Device handle + function_name + group_name make it unique. For pin outdisable we are making use of this 3 combination. See the details. This patch series adds support for controlling output disable function using sysfs in a generic way as described below. | A | | B | | C | | D | | E | |user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM| | | | | | | | | | | A executes command to configure a pin group for pin output disable operation echo "fname gname conf conf_val" > configure B parses the command and identifies the binding device associated with that pin group C matches the binding device against the device registered by D for configuration operation D matches the pin group and configure the pins for Output disable operation Both D and E are linked together by dt(i.e. PWM channel linked with Output disable Port) Cheers, Biju
On Thu, Mar 9, 2023 at 3:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Tue, Mar 7, 2023 at 10:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das > > kirjoitti: ... > > > > > Add pinctrl_get_device() to find a device handle associated with a > > > > > pincontrol group(i.e. by matching function name and group name for > > > > > a device). This device handle can then be used for finding match > > > > > for the pin output disable device that protects device against > > > > > short circuits on the pins. > > > > > > > > Not sure I understand the use case. Please, create a better commit > > message. > > > > > > OK, Basically pinmux_enable_setting allows exclusive access of pin to a > > device. > > > It won't allow multiple devices to claim a pin. This is a confusion you brought. You got us completely lost. Please, try again from the clean sheet. > > Which is correct. No? Show me the schematics of the real use case for that. > > > > The owner of the pin is the host side. I can't imagine how the same pin is > > shared inside the SoC. Is it broken hardware design? > > We are discussing usage of > > echo "fname gname" and you asked a question whether multiple devices can > claim a pin at the same time > > and my answer is no. > as setting->data.mux will be unique for a pin and will be claimed by > device during commit state. > > Am I missing anything here?? Yes. The same fname/gname can be in many *pin control* (provider) devices. So, it does not uniquely define the pin control device. ... > > > > Also it is missing the explanation why there will be no collisions > > > > when looking by the same pair of function and group name from different > > device. > > > > > > setting->data.mux will be unique for a pin. So there won't be a > > > setting->collision when > > > looking by the same pair of function and group name from different device. > > > > > > > (Always imagine that you have 2+ same IP blocks on the platform > > > > before doing any pin control core work. This will help you to design > > > > it properly. ) > > > > Not sure how the pair function_name group_name makes the device unique. > > Do you agree Device handle + function_name + group_name make it unique. Yes. > For pin outdisable we are making use of this 3 combination. > See the details. > > > This patch series adds support for controlling output disable function using > sysfs in a generic way as described below. > > | A | | B | | C | | D | | E | > |user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM| > | | | | | | | | | | > > A executes command to configure a pin group for pin output disable operation > echo "fname gname conf conf_val" > configure > > B parses the command and identifies the binding device associated with that > pin group > > C matches the binding device against the device registered by D for > configuration operation > > D matches the pin group and configure the pins for Output disable operation > > Both D and E are linked together by dt(i.e. PWM channel linked with Output > disable Port) Sounds like an overengineered hack to achieve something that I can't read between the lines. Why is this so complicated interface and flow are needed to begin with?
Hi Andy, Thanks for the explanation. > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Thu, Mar 9, 2023 at 3:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > pinctrl_get_device() On Tue, Mar 7, 2023 at 10:13 AM Biju Das > <biju.das.jz@bp.renesas.com> wrote: > > > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju > > > > > Das > > > kirjoitti: > > ... > > > > > > > Add pinctrl_get_device() to find a device handle associated > > > > > > with a pincontrol group(i.e. by matching function name and > > > > > > group name for a device). This device handle can then be used > > > > > > for finding match for the pin output disable device that > > > > > > protects device against short circuits on the pins. > > > > > > > > > > Not sure I understand the use case. Please, create a better > > > > > commit > > > message. > > > > > > > > OK, Basically pinmux_enable_setting allows exclusive access of pin > > > > to a > > > device. > > > > It won't allow multiple devices to claim a pin. > > This is a confusion you brought. You got us completely lost. Please, try > again from the clean sheet. > > > > Which is correct. No? Show me the schematics of the real use case for > that. > > > > > > The owner of the pin is the host side. I can't imagine how the same > > > pin is shared inside the SoC. Is it broken hardware design? > > > > We are discussing usage of > > > > echo "fname gname" and you asked a question whether multiple devices > > can claim a pin at the same time > > > > and my answer is no. > > > as setting->data.mux will be unique for a pin and will be claimed by > > device during commit state. > > > > Am I missing anything here?? > > Yes. The same fname/gname can be in many *pin control* (provider) devices. I agree. I have used the code used for [1] getting *pin control* devices associated with function name and group name. [1] cat /sys/kernel/debug/pinctrl/pinctrl-handles if output of [1], can return multiple devices for a given fname/gname, then I am wrong. Please correct me if that is the case. So I was under the impression that [2], it will fail if multiple devices try to acquire a pin. [2] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinmux.c#L132 > > So, it does not uniquely define the pin control device. I agree. > > ... > > > > > > Also it is missing the explanation why there will be no > > > > > collisions when looking by the same pair of function and group > > > > > name from different > > > device. > > > > > > > > setting->data.mux will be unique for a pin. So there won't be a > > > > setting->collision when > > > > looking by the same pair of function and group name from different > device. > > > > > > > > > (Always imagine that you have 2+ same IP blocks on the platform > > > > > before doing any pin control core work. This will help you to > > > > > design it properly. ) > > > > > > Not sure how the pair function_name group_name makes the device unique. > > > > Do you agree Device handle + function_name + group_name make it unique. > > Yes. > > > For pin outdisable we are making use of this 3 combination. > > See the details. > > > > > > This patch series adds support for controlling output disable function > > using sysfs in a generic way as described below. > > > > | A | | B | | C | | D | | E | > > |user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM| > > | | | | | | | | | | > > > > A executes command to configure a pin group for pin output disable > operation > > echo "fname gname conf conf_val" > configure > > > > B parses the command and identifies the binding device associated with > that > > pin group > > > > C matches the binding device against the device registered by D for > > configuration operation > > > > D matches the pin group and configure the pins for Output disable > > operation > > > > Both D and E are linked together by dt(i.e. PWM channel linked with > > Output disable Port) > > Sounds like an overengineered hack to achieve something that I can't read > between the lines. Why is this so complicated interface and flow are needed > to begin with? It is very simple. I am trying to give details like how a pwm pins used for pin output disable is configured from user space in a generic way. My use case is, I have an IP which detects short circuit between the output terminals and disable the output from pwm pins ,when it detects short circuit to protect from system failure. pwm-pins are involved in this operation. From user space we need to configure the type of protection for this pins (eg: disable PWM output, when both pwm outputs are high at same time). For that, we need to find a provider device (which provides gpt-pins). pinctrl_get_device() returns "current provider device" associated with fname/gname. If " current provider device" == "pwm device" do the configuration. Cheers, Biju
Hi Andy, > Subject: RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > Hi Andy, > > Thanks for the explanation. > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > > > On Thu, Mar 9, 2023 at 3:26 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > > pinctrl_get_device() On Tue, Mar 7, 2023 at 10:13 AM Biju Das > > <biju.das.jz@bp.renesas.com> wrote: > > > > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > > > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000, > > > > > > Biju Das > > > > kirjoitti: > > > > ... > > > > > > > > > Add pinctrl_get_device() to find a device handle associated > > > > > > > with a pincontrol group(i.e. by matching function name and > > > > > > > group name for a device). This device handle can then be > > > > > > > used for finding match for the pin output disable device > > > > > > > that protects device against short circuits on the pins. > > > > > > > > > > > > Not sure I understand the use case. Please, create a better > > > > > > commit > > > > message. > > > > > > > > > > OK, Basically pinmux_enable_setting allows exclusive access of > > > > > pin to a > > > > device. > > > > > It won't allow multiple devices to claim a pin. > > > > This is a confusion you brought. You got us completely lost. Please, > > try again from the clean sheet. > > > > > > Which is correct. No? Show me the schematics of the real use case > > > > for > > that. > > > > > > > > The owner of the pin is the host side. I can't imagine how the > > > > same pin is shared inside the SoC. Is it broken hardware design? > > > > > > We are discussing usage of > > > > > > echo "fname gname" and you asked a question whether multiple devices > > > can claim a pin at the same time > > > > > > and my answer is no. > > > > > as setting->data.mux will be unique for a pin and will be claimed by > > > device during commit state. > > > > > > Am I missing anything here?? > > > > Yes. The same fname/gname can be in many *pin control* (provider) devices. > > I agree. > > I have used the code used for [1] getting *pin control* devices associated > with function name and group name. > > [1] > cat /sys/kernel/debug/pinctrl/pinctrl-handles > > if output of [1], can return multiple devices for a given fname/gname, then > I am wrong. > Please correct me if that is the case. > > So I was under the impression that [2], it will fail if multiple devices try > to acquire a pin. > > [2] > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinmux.c#L132 I have done the below modifications in my dt and confirm that, pinctrl framework doesn't allow 2 devices to claim pins with a given function name and group name at same time. dt-changes: &gpt { + gpt6-pins { + pinmux = <RZG2L_PORT_PINMUX(44, 0, 5)>, /* GTIOC6A */ + <RZG2L_PORT_PINMUX(44, 1, 5)>; /* GTIOC6B */ + }; + + gpt7-pins { + pinmux = <RZG2L_PORT_PINMUX(44, 2, 5)>, /* GTIOC7A */ + <RZG2L_PORT_PINMUX(44, 3, 5)>; /* GTIOC7B */ + }; }; &mtu { + gpt6-pins { + pinmux = <RZG2L_PORT_PINMUX(44, 0, 4)>, /* MTIOC3A */ + <RZG2L_PORT_PINMUX(44, 1, 4)>, /* MTIOC3B */ + <RZG2L_PORT_PINMUX(44, 2, 4)>, /* MTIOC3C */ + <RZG2L_PORT_PINMUX(44, 3, 4)>; /* MTIOC3D */ }; }; Initially MTU device is claiming the pins P44_0 and P44_1. root@smarc-rzg2l:~# cat /sys/kernel/debug/pinctrl/pinctrl-handles device: 10001200.timer current state: default state: default type: MUX_GROUP controller pinctrl-rzg2l group: mtu3_zphase_clk (3) function: mtu3_zphase_clk (3) type: MUX_GROUP controller pinctrl-rzg2l group: mtu3_clk (4) function: mtu3_clk (4) type: MUX_GROUP controller pinctrl-rzg2l group: gpt6-pins (5) function: gpt6-pins (5) When It tried to load gpt, I get the below erro, [ 15.024714] pinctrl-rzg2l 11030000.pinctrl: pin P44_0 already requested by 10001200.timer; cannot claim for 10048000.pwm Starting Update UTMP about System Runlevel Changes... [ 15.044828] pinctrl-rzg2l 11030000.pinctrl: pin-352 (10048000.pwm) status -22 [ 15.056515] pinctrl-rzg2l 11030000.pinctrl: could not request pin 352 (P44_0) from group gpt6-pins on device pinctrl-rzg2l [ 15.070224] pwm-rzg2l-gpt 10048000.pwm: Error applying setting, reverse things back After unloading mtu modules and loading gpt modules, gpt module is claims the pin. root@smarc-rzg2l:~# rmmod rz_mtu3_cnt root@smarc-rzg2l:~# rmmod pwm_rz_mtu3 root@smarc-rzg2l:~# cd /sys/bus/platform/drivers/rz-mtu3/ root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# echo 10001200.timer > unbind root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# rmmod rzg2l_poeg root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# rmmod pwm_rzg2l_gpt root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# cat /sys/kernel/debug/pinctrl/pinctrl-handles | grep gpt root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# modprobe rzg2l_poeg root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# modprobe pwm_rzg2l_gpt root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# cat /sys/kernel/debug/pinctrl/pinctrl-handles | grep gpt type: MUX_GROUP controller pinctrl-rzg2l group: gpt6-pins (5) function: gpt6-pins (5) type: MUX_GROUP controller pinctrl-rzg2l group: gpt7-pins (11) function: gpt7-pins (11) root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# cat /sys/kernel/debug/pinctrl/pinctrl-handles device: 10048000.pwm current state: default state: default type: MUX_GROUP controller pinctrl-rzg2l group: gpt6-pins (5) function: gpt6-pins (5) type: MUX_GROUP controller pinctrl-rzg2l group: gpt7-pins (11) function: gpt7-pins (11) Since I have used same function name/ group name it gets a unique selector(5) and the functionality for gpt is not working as the selector meant for MTU. But if I replace "gpt6-pins" -> "mtu-pins" in MTU node, then the functionality works as expected as the selector is different for mtu-pins and gpt-pins eventhough both uses same pins. So I believe current implementation is based on /sys/kernel/debug/pinctrl/pinctrl-handles is good unless I miss something here. Please correct me,if you think my observation is wrong. Cheers, Biju
On Thu, Mar 9, 2023 at 3:19 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > I have an IP which detects short circuit between the output terminals and > disable the output from pwm pins ,when it detects short circuit to protect from > system failure. > > pwm-pins are involved in this operation. > > From user space we need to configure the type of protection for this pins (eg: disable PWM output, > when both pwm outputs are high at same time). Why do you want to do this from user space? It sounds like something the kernel should be doing. The kernel has a PWM subsystem, and a pin control subsystem, and we don't even have a userspace ABI for pin control. Pin control is designed to avoid electrical disasters and a driver can add further policy for sure. If you want to add policy of different types to avoid electrical disaster into the pin control driver, go ahead, just run it by Geert so he's on board with the ideas. > For that, we need to find a provider device (which provides gpt-pins). > > pinctrl_get_device() returns "current provider device" associated with fname/gname. > If " current provider device" == "pwm device" do the configuration. I don't understand this, sorry. Yours, Linus Walleij
Hi Linus W, Thanks for feedback. > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Thu, Mar 9, 2023 at 3:19 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > I have an IP which detects short circuit between the output terminals > > and disable the output from pwm pins ,when it detects short circuit to > > protect from system failure. > > > > pwm-pins are involved in this operation. > > > > From user space we need to configure the type of protection for this > > pins (eg: disable PWM output, when both pwm outputs are high at same > time). > > Why do you want to do this from user space? To take care of the below features provided by Port Output Enable for GPT(a.k.a PWM) (POEG) IP. 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[1]. [1] Use case 1) ● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect short circuit in switching circuit externally and use an external pin(GTETRGA to GTETRGD) to disable the output pins of PWM) Use case 2) ● Output-disable request from the GPT (GPT detects short circuit in switching circuit internally and disable the output pins of PWM) Use case 3) ● Register settings(Detect short circuit in switching circuit externally and use internal register to disable the output pins of PWM) The advantage of providing these options from user space is, it is flexible. Runtime user can configure the use case he wants to use for his product. +Rob, + Krzysztof Kozlowski If we cannot do it in user space, then we need to make it as part of Dt bindings and users will define the use case they need in DT. Some of the failure conditions in switching circuits are like below when you use PWM push-pull configuration you SHOULD have a dead time. When + mosfet is turned off - mosfet can't be immediately turned on because you will create a direct path (short circuit) between + and - as parasitic capacitance will leave + mosfet turned on for a while . This will destroy your mosfets. Dead time is the delay measured from turning off the driver switch connected to one rail of the power supply to the time the switch connected to the other rail of the power supply is turned on. Switching devices like MOSFETs and IGBTs turn off after a delay when the gate drive is turned off. If the other switch on the half bridge is turned on immediately, both upper and lower switches may be in a conductive region for a brief moment, shorting the power rail. In order to avoid this, a dead time is maintained between turning off of one switch and turning on the other in a half bridge. POEG IP provides the below protections, 1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an active level simultaneously, the PWM generates an output-disable request to the POEG. Through reception of these requests, the POEG can control whether the GTIOCA and GTIOCB pins are output-disabled 2) PWM output pins can be set to be disabled when the PWM output pins detect a dead time error or short circuit detection between the output terminals > > It sounds like something the kernel should be doing. If we cannot do the above use cases[1] in user space, then we need to make it as part of Dt bindings and it will be fully taken care in kernel. > > The kernel has a PWM subsystem, and a pin control subsystem, and we don't > even have a userspace ABI for pin control. > Pin control is designed to avoid electrical disasters and a driver can add > further policy for sure. > > If you want to add policy of different types to avoid electrical disaster > into the pin control driver, go ahead, just run it by Geert so he's on board > with the ideas. OK. Geert Can you please provide valuable suggestion how to address this use cases[1] As mentioned above? Currently Pin control driver is used for identifying the pwm device by using pwm gname/fname and configuring the various use cases as mentioned above[1] for a system. > > > For that, we need to find a provider device (which provides gpt-pins). > > > > pinctrl_get_device() returns "current provider device" associated with > fname/gname. > > If " current provider device" == "pwm device" do the configuration. > > I don't understand this, sorry. Andy mentioned, the same fname/gname can be used in many *pin control* (provider) devices. But when checking the code(/sys/kernel/debug/pinctrl/pinctrl-handles), at a given time, there will be only one device claims Pins associated with a given fname/gname. Currently pinctrl_get_device() returns the current *pin control* (provider) device that claimed pins associated with a given fname/gname. Cheers, Biju
On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > If we cannot do it in user space, then we need to make it as part of > Dt bindings and users will define the use case they need in DT. That sounds like a much better idea :) The kernel is for protecting hardware from users after all, and it seems you want to select one of these use cases and DT is excellent for system config like this. So I would say work ahead on this path. Yours, Linus Walleij
Hi Biju, On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Thu, Mar 9, 2023 at 3:19 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > I have an IP which detects short circuit between the output terminals > > > and disable the output from pwm pins ,when it detects short circuit to > > > protect from system failure. > > > > > > pwm-pins are involved in this operation. > > > > > > From user space we need to configure the type of protection for this > > > pins (eg: disable PWM output, when both pwm outputs are high at same > > time). > > > > Why do you want to do this from user space? > > To take care of the below features provided by Port Output Enable for GPT(a.k.a PWM) > (POEG) IP. > > 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[1]. > > [1] > > Use case 1) > ● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect short circuit in switching circuit > externally and use an external pin(GTETRGA to GTETRGD) to disable the output pins of PWM) > > Use case 2) > ● Output-disable request from the GPT (GPT detects short circuit in switching circuit internally and > disable the output pins of PWM) > > Use case 3) > ● Register settings(Detect short circuit in switching circuit > externally and use internal register to disable the output pins of PWM) > > The advantage of providing these options from user space is, it is flexible. > Runtime user can configure the use case he wants to use for his product. > > +Rob, + Krzysztof Kozlowski > > If we cannot do it in user space, then we need to make it as part of > Dt bindings and users will define the use case they need in DT. > > Some of the failure conditions in switching circuits are like below > > when you use PWM push-pull configuration you SHOULD have a dead time. > When + mosfet is turned off - mosfet can't be immediately turned on > because you will create a direct path (short circuit) between + and - > as parasitic capacitance will leave + mosfet turned on for a while . > This will destroy your mosfets. > > Dead time is the delay measured from turning off the driver switch > connected to one rail of the power supply to the time the switch > connected to the other rail of the power supply is turned on. > Switching devices like MOSFETs and IGBTs turn off after a delay > when the gate drive is turned off. If the other switch on the half > bridge is turned on immediately, both upper and lower switches may be > in a conductive region for a brief moment, shorting the power rail. > In order to avoid this, a dead time is maintained between turning off > of one switch and turning on the other in a half bridge. > > POEG IP provides the below protections, > > 1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an active level simultaneously, the > PWM generates an output-disable request to the POEG. Through reception of these requests, > the POEG can control whether the GTIOCA and GTIOCB pins are output-disabled > > 2) PWM output pins can be set to be disabled when the PWM output pins detect a dead time error > or short circuit detection between the output terminals > > > > > It sounds like something the kernel should be doing. > > If we cannot do the above use cases[1] in user space, then we need to make it as part of > Dt bindings and it will be fully taken care in kernel. > > > > > The kernel has a PWM subsystem, and a pin control subsystem, and we don't > > even have a userspace ABI for pin control. > > Pin control is designed to avoid electrical disasters and a driver can add > > further policy for sure. > > > > If you want to add policy of different types to avoid electrical disaster > > into the pin control driver, go ahead, just run it by Geert so he's on board > > with the ideas. > > OK. Geert Can you please provide valuable suggestion how to address this use cases[1] > As mentioned above? Is this configuration you need to do once, based on the hardware, or do you need to change it at run-time? Before, I was under the impression you needed to change the configuration at run-time, hence the need for a sysfs API. If configuration is static, DT is the way to go. Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > Hi Biju, > > On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > pinctrl_get_device() On Thu, Mar 9, 2023 at 3:19 PM Biju Das > <biju.das.jz@bp.renesas.com> wrote: > > > > I have an IP which detects short circuit between the output > > > > terminals and disable the output from pwm pins ,when it detects > > > > short circuit to protect from system failure. > > > > > > > > pwm-pins are involved in this operation. > > > > > > > > From user space we need to configure the type of protection for > > > > this pins (eg: disable PWM output, when both pwm outputs are high > > > > at same > > > time). > > > > > > Why do you want to do this from user space? > > > > To take care of the below features provided by Port Output Enable for > > GPT(a.k.a PWM) > > (POEG) IP. > > > > 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[1]. > > > > [1] > > > > Use case 1) > > ● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect > > short circuit in switching circuit externally and use an external > > pin(GTETRGA to GTETRGD) to disable the output pins of PWM) > > > > Use case 2) > > ● Output-disable request from the GPT (GPT detects short circuit in > > switching circuit internally and disable the output pins of PWM) > > > > Use case 3) > > ● Register settings(Detect short circuit in switching circuit > > externally and use internal register to disable the output pins of > > PWM) > > > > The advantage of providing these options from user space is, it is > flexible. > > Runtime user can configure the use case he wants to use for his product. > > > > +Rob, + Krzysztof Kozlowski > > > > If we cannot do it in user space, then we need to make it as part of > > Dt bindings and users will define the use case they need in DT. > > > > Some of the failure conditions in switching circuits are like below > > > > when you use PWM push-pull configuration you SHOULD have a dead time. > > When + mosfet is turned off - mosfet can't be immediately turned on > > because you will create a direct path (short circuit) between + and - > > as parasitic capacitance will leave + mosfet turned on for a while . > > This will destroy your mosfets. > > > > Dead time is the delay measured from turning off the driver switch > > connected to one rail of the power supply to the time the switch > > connected to the other rail of the power supply is turned on. > > Switching devices like MOSFETs and IGBTs turn off after a delay when > > the gate drive is turned off. If the other switch on the half bridge > > is turned on immediately, both upper and lower switches may be in a > > conductive region for a brief moment, shorting the power rail. > > In order to avoid this, a dead time is maintained between turning off > > of one switch and turning on the other in a half bridge. > > > > POEG IP provides the below protections, > > > > 1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an > > active level simultaneously, the PWM generates an output-disable > > request to the POEG. Through reception of these requests, the POEG can > > control whether the GTIOCA and GTIOCB pins are output-disabled > > > > 2) PWM output pins can be set to be disabled when the PWM output pins > > detect a dead time error or short circuit detection between the output > > terminals > > > > > > > > It sounds like something the kernel should be doing. > > > > If we cannot do the above use cases[1] in user space, then we need to > > make it as part of Dt bindings and it will be fully taken care in kernel. > > > > > > > > The kernel has a PWM subsystem, and a pin control subsystem, and we > > > don't even have a userspace ABI for pin control. > > > Pin control is designed to avoid electrical disasters and a driver > > > can add further policy for sure. > > > > > > If you want to add policy of different types to avoid electrical > > > disaster into the pin control driver, go ahead, just run it by Geert > > > so he's on board with the ideas. > > > > OK. Geert Can you please provide valuable suggestion how to address > > this use cases[1] As mentioned above? > > Is this configuration you need to do once, based on the hardware, or do you > need to change it at run-time? I think this configuration needed only once based on the hardware. > > Before, I was under the impression you needed to change the configuration at > run-time, hence the need for a sysfs API. > If configuration is static, DT is the way to go. At that time, I was not explored the possibility of creating poeg char device. For eg: After the initial setting in DT, I guess with poeg char device we should be able to achieve below use cases. Use case 1) We can provide user space event indicating, Output-disable request from the GTETRGn pin occurred. and provide some options (rd/wr file ops) to user space to clear the fault. Use case 2) We can provide user space event indicating, Output-disable request from GPT disable request occurred. and provide some options(rd/wr file ops) to user space to clear the fault. Use case 3) User space to control Output-disable through rd/wr file ops. Please let me know is it ok or am I missing something here?? Cheers, Biju
Hi Biju, On Tue, Mar 14, 2023 at 12:33 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add > > > > pinctrl_get_device() On Thu, Mar 9, 2023 at 3:19 PM Biju Das > > <biju.das.jz@bp.renesas.com> wrote: > > > > > I have an IP which detects short circuit between the output > > > > > terminals and disable the output from pwm pins ,when it detects > > > > > short circuit to protect from system failure. > > > > > > > > > > pwm-pins are involved in this operation. > > > > > > > > > > From user space we need to configure the type of protection for > > > > > this pins (eg: disable PWM output, when both pwm outputs are high > > > > > at same > > > > time). > > > > > > > > Why do you want to do this from user space? > > > > > > To take care of the below features provided by Port Output Enable for > > > GPT(a.k.a PWM) > > > (POEG) IP. > > > > > > 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[1]. > > > > > > [1] > > > > > > Use case 1) > > > ● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect > > > short circuit in switching circuit externally and use an external > > > pin(GTETRGA to GTETRGD) to disable the output pins of PWM) > > > > > > Use case 2) > > > ● Output-disable request from the GPT (GPT detects short circuit in > > > switching circuit internally and disable the output pins of PWM) > > > > > > Use case 3) > > > ● Register settings(Detect short circuit in switching circuit > > > externally and use internal register to disable the output pins of > > > PWM) > > > > > > The advantage of providing these options from user space is, it is > > flexible. > > > Runtime user can configure the use case he wants to use for his product. > > > > > > +Rob, + Krzysztof Kozlowski > > > > > > If we cannot do it in user space, then we need to make it as part of > > > Dt bindings and users will define the use case they need in DT. > > > > > > Some of the failure conditions in switching circuits are like below > > > > > > when you use PWM push-pull configuration you SHOULD have a dead time. > > > When + mosfet is turned off - mosfet can't be immediately turned on > > > because you will create a direct path (short circuit) between + and - > > > as parasitic capacitance will leave + mosfet turned on for a while . > > > This will destroy your mosfets. > > > > > > Dead time is the delay measured from turning off the driver switch > > > connected to one rail of the power supply to the time the switch > > > connected to the other rail of the power supply is turned on. > > > Switching devices like MOSFETs and IGBTs turn off after a delay when > > > the gate drive is turned off. If the other switch on the half bridge > > > is turned on immediately, both upper and lower switches may be in a > > > conductive region for a brief moment, shorting the power rail. > > > In order to avoid this, a dead time is maintained between turning off > > > of one switch and turning on the other in a half bridge. > > > > > > POEG IP provides the below protections, > > > > > > 1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an > > > active level simultaneously, the PWM generates an output-disable > > > request to the POEG. Through reception of these requests, the POEG can > > > control whether the GTIOCA and GTIOCB pins are output-disabled > > > > > > 2) PWM output pins can be set to be disabled when the PWM output pins > > > detect a dead time error or short circuit detection between the output > > > terminals > > > > > > > > > > > It sounds like something the kernel should be doing. > > > > > > If we cannot do the above use cases[1] in user space, then we need to > > > make it as part of Dt bindings and it will be fully taken care in kernel. > > > > > > > > > > > The kernel has a PWM subsystem, and a pin control subsystem, and we > > > > don't even have a userspace ABI for pin control. > > > > Pin control is designed to avoid electrical disasters and a driver > > > > can add further policy for sure. > > > > > > > > If you want to add policy of different types to avoid electrical > > > > disaster into the pin control driver, go ahead, just run it by Geert > > > > so he's on board with the ideas. > > > > > > OK. Geert Can you please provide valuable suggestion how to address > > > this use cases[1] As mentioned above? > > > > Is this configuration you need to do once, based on the hardware, or do you > > need to change it at run-time? > > I think this configuration needed only once based on the hardware. OK, so using DT for that would be fine. > > Before, I was under the impression you needed to change the configuration at > > run-time, hence the need for a sysfs API. > > If configuration is static, DT is the way to go. > > At that time, I was not explored the possibility of creating poeg char device. > > For eg: After the initial setting in DT, I guess with poeg char device we should be able to > achieve below use cases. > > Use case 1) > We can provide user space event indicating, Output-disable request from the GTETRGn pin occurred. > and provide some options (rd/wr file ops) to user space to clear the fault. > > Use case 2) > We can provide user space event indicating, Output-disable request from GPT disable request occurred. > and provide some options(rd/wr file ops) to user space to clear the fault. > > Use case 3) > User space to control Output-disable through rd/wr file ops. > > Please let me know is it ok or am I missing something here?? Using a char device for that sounds fine to me. If you just needed to clear the fault, you could use a device property in sysfs. But that would still leave us without a way to provide events to userspace. Gr{oetje,eeting}s, Geert
Hi Greg, > Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support > > On Tue, Mar 07, 2023 at 02:43:54PM +0000, Biju Das wrote: > > Hi Linus Walleij, > > > > Thanks for the feedback. > > > > > Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support > > > > > > On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > > > > Add a simple sysfs interface to the generic pinctrl framework for > > > > configuring pins for output disable operation. > > > > > > > > /sys/class/pinctrl/ > > > > `-- output-disable/ > > > > |-- configure (w/o) ask the kernel to configure a pin group > > > > for output disable operation. > > > > > > > > echo "<group-name function-name 0 1>" > configure > > > > > > > > The existing "pinmux-functions" debugfs file lists the pin > > > > functions registered for the pin controller. For example: > > > > > > > > function 0: usb0, groups = [ usb0 ] > > > > function 1: usb1, groups = [ usb1 ] > > > > function 2: gpt4-pins, groups = [ gpt4-pins ] > > > > function 3: scif0, groups = [ scif0 ] > > > > function 4: scif2, groups = [ scif2 ] > > > > function 5: spi1, groups = [ spi1 ] > > > > > > > > To configure gpt4-pins for output disable activation by user: > > > > > > > > echo "gpt4-pins gpt4-pins 0 1" > configure > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > > > > > You have to run things like this by Greg K-H (adde on To) > > > > > > > +static struct class pinctrl_class = { > > > > + .name = "pinctrl", > > > > + .owner = THIS_MODULE, > > > > + .dev_groups = pinctrl_groups, }; > > > > > > Why are you adding a new device class? > > > > > > IIRC Greg don't like them, we should probably just deal with the > > > devices directly on the bus where they are, which also implies some > > > topology search etc which is a feature. > > > > I am ok for both, > > > > I thought adding new device class will be more generic and people can > > use simple sysfs[1] like pwm for output disable operation rather than > > the SoC specific operation[2]. > > > > [1] > > /sys/class/pinctrl/output-disable/configure > > That's fine, but you don't need a class for it, use configfs for configuring > things like this, that is what it is there for. As per discussion with Linus W and Geert, it is agreed to use configure this settings in device tree as DT is the best place for system config like these use cases. Cheers, Biju
Hi Linus Walleij, Thanks for the feedback. > <krzysztof.kozlowski+dt@linaro.org> > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device() > > On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > If we cannot do it in user space, then we need to make it as part of > > Dt bindings and users will define the use case they need in DT. > > That sounds like a much better idea :) > > The kernel is for protecting hardware from users after all, and it seems you > want to select one of these use cases and DT is excellent for system config > like this. > > So I would say work ahead on this path. Agreed. Will send next version based on this approach. 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 adds support for controlling output disable function using sysfs in a generic way as described below. | A | | B | | C | | D | | E | |user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM| | | | | | | | | | | A executes command to configure a pin group for pin output disable operation echo "fname gname conf conf_val" > configure B parses the command and identifies the binding device associated with that pin group C matches the binding device against the device registered by D for configuration operation D matches the pin group and configure the pins for Output disable operation Both D and E are linked together by dt(i.e. PWM channel linked with Output disable Port) eg: echo "gpt4-pins gpt4-pins 0 1" > configure ,will activate output disable by the user. As part of this added a simple sysfs interface to the generic pinctrl framework. /sys/class/pinctrl/ `-- output-disable/ |-- configure (w/o) ask the kernel to configure a pin group for output disable operation. Patch #1 to #5 crate a framework for registering pin output disable device to pincontroller. Patch #7 to patch #13 are 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-generic.sh [POEG]open 16 pwmchip0 Test case 1 user POEG control 74: 0 0 GICv3 357 Level 10049400.poeg Read at address 0x10048434 (0xffffb00b0434): 0x0200031B Read at address 0x10048438 (0xffff911ef438): 0x03000000 Read at address 0x10049400 (0xffffa8dff400): 0x00000030 Test case 2 user GPT control both high Read at address 0x10048434 (0xffff8537e434): 0x021B031B Read at address 0x10048438 (0xffffbb73c438): 0x23000000 Read at address 0x10049400 (0xffffa5742400): 0x00000030 gpt ch:3, irq=2 gpt ch:3, irq=2 Read at address 0x10048434 (0xffffb0193434): 0x021B031B Read at address 0x10048438 (0xffffae0b4438): 0x03000000 Read at address 0x10049400 (0xffff873f5400): 0x00000030 gpt ch:3, irq=2 74: 6 0 GICv3 357 Level 10049400.poeg Test case 3 user GPT control both low Read at address 0x10048434 (0xffffbadb1434): 0x021B031B Read at address 0x10048438 (0xffff88720438): 0x43000000 Read at address 0x10049400 (0xffff9d7a9400): 0x00000030 gpt ch:3, irq=4 gpt ch:3, irq=4 Read at address 0x10048434 (0xffff95d68434): 0x021B031B Read at address 0x10048438 (0xffff82428438): 0x03000000 Read at address 0x10049400 (0xffff92716400): 0x00000030 gpt ch:3, irq=4 74: 12 0 GICv3 357 Level 10049400.poeg root@smarc-rzg2l:~# v5->v6: * Dropped binding patch as it is accepted for 6.4. * Added sysfs support for configuring pin output disable function in a generic way. 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 (13): pinctrl: core: Add pinctrl_get_device() pinctrl: Add poutdisops variable to struct pinctrl_desc pinctrl: Add sysfs support pinctrl: renesas: rzg2l: Add pin output disable support soc: renesas: Kconfig: Enable pin output disable for RZ/G2L SoC 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 Documentation/ABI/testing/sysfs-class-pinctrl | 32 ++ drivers/pinctrl/Kconfig | 4 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/core.c | 52 ++ drivers/pinctrl/output-disable.c | 148 ++++++ drivers/pinctrl/output-disable.h | 32 ++ drivers/pinctrl/renesas/Kconfig | 2 + drivers/pinctrl/renesas/Makefile | 2 + drivers/pinctrl/renesas/pinctrl-rzg2l.c | 44 ++ drivers/pinctrl/renesas/poeg/Kconfig | 11 + drivers/pinctrl/renesas/poeg/Makefile | 2 + drivers/pinctrl/renesas/poeg/rzg2l-poeg.c | 486 ++++++++++++++++++ drivers/pwm/pwm-rzg2l-gpt.c | 129 +++++ drivers/soc/renesas/Kconfig | 1 + include/linux/pinctrl/consumer.h | 9 + include/linux/pinctrl/output-disable.h | 42 ++ include/linux/pinctrl/pinctrl-rzg2l.h | 35 ++ include/linux/pinctrl/pinctrl.h | 4 + include/linux/pwm/rzg2l-gpt.h | 44 ++ tools/poeg/Build | 1 + tools/poeg/Makefile | 53 ++ tools/poeg/poeg_app.c | 60 +++ 22 files changed, 1194 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-pinctrl create mode 100644 drivers/pinctrl/output-disable.c create mode 100644 drivers/pinctrl/output-disable.h 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/pinctrl/output-disable.h create mode 100644 include/linux/pinctrl/pinctrl-rzg2l.h create mode 100644 include/linux/pwm/rzg2l-gpt.h create mode 100644 tools/poeg/Build create mode 100644 tools/poeg/Makefile create mode 100644 tools/poeg/poeg_app.c