| Message ID | 20230306090014.128732-2-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| 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. )
Hi Andy Shevchenko, Thanks for the feedback. > 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. Maybe instead of find device handle, it should re worded as to find the current owner of the pins associated with a given function and group name?? Or please let me know have better suggestion. Then use this info for matching the device used for pin output disable. > > 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. ) OK. 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 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
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index d6e6c751255f..2ba222026db4 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -27,6 +27,7 @@ #include <linux/pinctrl/devinfo.h> #include <linux/pinctrl/machine.h> #include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinmux.h> #ifdef CONFIG_GPIOLIB #include "../gpio/gpiolib.h" @@ -1584,6 +1585,54 @@ int pinctrl_select_default_state(struct device *dev) } EXPORT_SYMBOL_GPL(pinctrl_select_default_state); +static bool pinctrl_get_device_match(struct pinctrl_setting *setting, + const char *fname, const char *gname) +{ + struct pinctrl_dev *pctldev = setting->pctldev; + const struct pinmux_ops *pmxops = pctldev->desc->pmxops; + const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; + const char *function = pmxops->get_function_name(pctldev, + setting->data.mux.func); + const char *group = pctlops->get_group_name(pctldev, + setting->data.mux.group); + + if ((!strcmp(function, fname)) && (!strcmp(group, gname))) + return true; + + return false; +} + +/** + * pinctrl_get_device() - returns device associated with a pincontrol group + * @fname: function name + * @gname: group name + */ +struct device *pinctrl_get_device(const char *fname, const char *gname) +{ + struct pinctrl *p; + struct pinctrl_state *state; + struct pinctrl_setting *setting; + + mutex_lock(&pinctrl_list_mutex); + + list_for_each_entry(p, &pinctrl_list, node) { + list_for_each_entry(state, &p->states, node) { + list_for_each_entry(setting, &state->settings, node) { + if (setting->type == PIN_MAP_TYPE_MUX_GROUP && + pinctrl_get_device_match(setting, fname, gname)) { + mutex_unlock(&pinctrl_list_mutex); + return p->dev; + } + } + } + } + + mutex_unlock(&pinctrl_list_mutex); + + return ERR_PTR(-EINVAL); +} +EXPORT_SYMBOL_GPL(pinctrl_get_device); + #ifdef CONFIG_PM /** diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h index 4729d54e8995..6ff8857c0a9c 100644 --- a/include/linux/pinctrl/consumer.h +++ b/include/linux/pinctrl/consumer.h @@ -42,6 +42,9 @@ extern struct pinctrl * __must_check devm_pinctrl_get(struct device *dev); extern void devm_pinctrl_put(struct pinctrl *p); extern int pinctrl_select_default_state(struct device *dev); +extern struct device * __must_check pinctrl_get_device(const char *fname, + const char *gname); + #ifdef CONFIG_PM extern int pinctrl_pm_select_default_state(struct device *dev); extern int pinctrl_pm_select_sleep_state(struct device *dev); @@ -142,6 +145,12 @@ static inline int pinctrl_pm_select_idle_state(struct device *dev) return 0; } +static inline struct device * __must_check pinctrl_get_device(const char *fname, + const char *gname) +{ + return NULL; +} + #endif /* CONFIG_PINCTRL */ static inline struct pinctrl * __must_check pinctrl_get_select(struct device *dev,
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. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v6: * New patch Ref: https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922F5494D3C0862E15F3F8486B39@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#t --- drivers/pinctrl/core.c | 49 ++++++++++++++++++++++++++++++++ include/linux/pinctrl/consumer.h | 9 ++++++ 2 files changed, 58 insertions(+)