| Message ID | 20210520202730.4444-3-dariobin@libero.it |
|---|---|
| State | New |
| Headers | show |
| Series | am335x: set pinmux registers from pins debug file | expand |
Hello Dario, On 5/20/21 11:27 PM, Dario Binacchi wrote: > The MPUs of some architectures (e.g AM335x) must be in privileged > operating mode to write on the pinmux registers. In such cases, where > writes will not work from user space, now it can be done from the pins user space has no connection to the problem you're trying to solve. Please provide a reasonable rationale for adding a new interface, thank you in advance. > debug file if the platform driver exports the pin_dbg_set() helper among > the registered operations. > > Signed-off-by: Dario Binacchi <dariobin@libero.it> I strongly object against this new interface. As Andy've already mentioned you have to operate with defined pin groups and functions, and so far you create an interface with an option to disasterous misusage, it shall be avoided, because there are better options. What's the issue with a regular declaration of pin groups and functions on your SoC? When it's done, you can operate on this level of abstraction, there is absolutely no need to add the proposed low-level debug interface. -- Best wishes, Vladimir
Hi Vladimir, > Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > > > Hello Dario, > > On 5/20/21 11:27 PM, Dario Binacchi wrote: > > The MPUs of some architectures (e.g AM335x) must be in privileged > > operating mode to write on the pinmux registers. In such cases, where > > writes will not work from user space, now it can be done from the pins > > user space has no connection to the problem you're trying to solve. > > Please provide a reasonable rationale for adding a new interface, thank > you in advance. > > > debug file if the platform driver exports the pin_dbg_set() helper among > > the registered operations. > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > I strongly object against this new interface. > > As Andy've already mentioned you have to operate with defined pin groups > and functions, and so far you create an interface with an option to > disasterous misusage, it shall be avoided, because there are better > options. > > What's the issue with a regular declaration of pin groups and functions > on your SoC? When it's done, you can operate on this level of abstraction, > there is absolutely no need to add the proposed low-level debug interface. > I quote Drew's words: "I think it could be helpful to be able to set the conf_<module>_<pin> registers directly through debugfs as I can imagine situations where it would be useful for testing. It is a bit dangerous as the person using it has to be careful not to change the wrong bits, but they would need to have debugfs mounted and permissions to write to it." "Bits 6:3 are related to what this subsystem would refer to as pin conf such as slew, input enable and bias. Thus it might make sense to expose something like a select-pinconf file to activate pin conf settings from userspace." From the emails exchanged I seem to have understood that there is no way to reconfigure slew rate, pull up / down and other properties on the fly. In the kernel version 4.1.6 that I am using on my custom board, I have fixed the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). The patches I've submitted implement some sort of devmem for pinmux. It too can be used in a dangerous way, but it exists and it is used. Anyway, the implementation may be wrong but it does highlight a feature that can be useful in testing or prototyping boards but is not present in the kernel. Can we then find a solution that is right for everyone? Thanks and regards, Dario > -- > Best wishes, > Vladimir
Hi Dario, On 5/24/21 8:28 PM, Dario Binacchi wrote: > Hi Vladimir, > >> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto: >> >> >> Hello Dario, >> >> On 5/20/21 11:27 PM, Dario Binacchi wrote: >>> The MPUs of some architectures (e.g AM335x) must be in privileged >>> operating mode to write on the pinmux registers. In such cases, where >>> writes will not work from user space, now it can be done from the pins >> >> user space has no connection to the problem you're trying to solve. >> >> Please provide a reasonable rationale for adding a new interface, thank >> you in advance. >> >>> debug file if the platform driver exports the pin_dbg_set() helper among >>> the registered operations. >>> >>> Signed-off-by: Dario Binacchi <dariobin@libero.it> >> >> I strongly object against this new interface. >> >> As Andy've already mentioned you have to operate with defined pin groups >> and functions, and so far you create an interface with an option to >> disasterous misusage, it shall be avoided, because there are better >> options. >> >> What's the issue with a regular declaration of pin groups and functions >> on your SoC? When it's done, you can operate on this level of abstraction, >> there is absolutely no need to add the proposed low-level debug interface. >> > > I quote Drew's words: > > "I think it could be helpful to be able to set the conf_<module>_<pin> > registers directly through debugfs as I can imagine situations where it would > be useful for testing. It is a bit dangerous as the person using it has to be > careful not to change the wrong bits, but they would need to have debugfs mounted > and permissions to write to it." > > "Bits 6:3 are related to what this subsystem would refer to as pin conf > such as slew, input enable and bias. Thus it might make sense to expose > something like a select-pinconf file to activate pin conf settings from > userspace." This is already present, please define all wanted configurations of pin groups and pin group functions, then switch them in runtime. I see no need of a coarse grained interface here... > From the emails exchanged I seem to have understood that there is no way to > reconfigure slew rate, pull up / down and other properties on the fly. I think you still can do all the tasks mentioned above on the recent kernel, why not? I am not closely familiar with TI AM335x pinmux/pinconf controller, and if needed I can look at the datasheet, but I can imagine that there are pins, pin groups, and pin group functions controls. Board specific configuration of pinmux is given in DTS, you can modify it with DT overlays for instance, and selection of pin group functions in runtime is already possible for users in runtime. What is missing from the picture, and why do you insist on re-introduction of a much worse interface? > In the kernel version 4.1.6 that I am using on my custom board, I have fixed > the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, > this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). Exactly, the feature is not needed. > The patches I've submitted implement some sort of devmem for pinmux. It too can > be used in a dangerous way, but it exists and it is used. My objection is to giving a "red button" to users, even to users of debugfs. If it's possible to keep the "dangerous goods" on developers' side only, then this shall be preferable, I believe. And fortunately there is such a mechanism. > Anyway, the implementation may be wrong but it does highlight a feature that > can be useful in testing or prototyping boards but is not present in the kernel. > Can we then find a solution that is right for everyone? Please see the method above. In my understanding the problem you are trying to solve shall be defined much more precised. Can you please elaborate on this part thoroughly? I still can not grasp a too generic explanation from you, writing of totally arbitrary data to controller registers looks senseless to me... Assume you are giving a handle to users to write arbitrary data to arbitrary I/O mem region, will it solve the problem for you? Of course yes. But does it sound like a good and acceptable solution? Of course no. Why? You need a better and more fine grained interface, namely write only to pinmux I/O mem. Great, that's provided by your change, however another important condition is still missing, a user shall write only valid data. Thus a higher level of abstraction is wanted: * writing data to I/O mem -- not good enough, * writing data to pinmux/pinconf I/O mem -- better, but not good enough, * writing *valid* data to pinmux/pinconf I/O mem -- that's right. The validity of data is defined by a developer, the abstraction name has been mentioned multiple times, it's pin groups and pin group functions. -- Best wishes, Vladimir
Hi, * Vladimir Zapolskiy <vz@mleia.com> [210524 18:52]: > Thus a higher level of abstraction is wanted: > > * writing data to I/O mem -- not good enough, > * writing data to pinmux/pinconf I/O mem -- better, but not good enough, > * writing *valid* data to pinmux/pinconf I/O mem -- that's right. > > The validity of data is defined by a developer, the abstraction name > has been mentioned multiple times, it's pin groups and pin group functions. Sounds like the planned interface should only allow changing between already defined pingroups, and only if the pins are unclaimed. That would leave out the issues tinkering with raw register data. And it probably should also allow adding and removing new pingroups out of the already defined pin functions that are unclaimed. I also think such interface should be sysfs and not debugfs eventually. Regards, Tony
Hi Vladimir, > Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > > > Hi Dario, > > On 5/24/21 8:28 PM, Dario Binacchi wrote: > > Hi Vladimir, > > > >> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > >> > >> > >> Hello Dario, > >> > >> On 5/20/21 11:27 PM, Dario Binacchi wrote: > >>> The MPUs of some architectures (e.g AM335x) must be in privileged > >>> operating mode to write on the pinmux registers. In such cases, where > >>> writes will not work from user space, now it can be done from the pins > >> > >> user space has no connection to the problem you're trying to solve. > >> > >> Please provide a reasonable rationale for adding a new interface, thank > >> you in advance. > >> > >>> debug file if the platform driver exports the pin_dbg_set() helper among > >>> the registered operations. > >>> > >>> Signed-off-by: Dario Binacchi <dariobin@libero.it> > >> > >> I strongly object against this new interface. > >> > >> As Andy've already mentioned you have to operate with defined pin groups > >> and functions, and so far you create an interface with an option to > >> disasterous misusage, it shall be avoided, because there are better > >> options. > >> > >> What's the issue with a regular declaration of pin groups and functions > >> on your SoC? When it's done, you can operate on this level of abstraction, > >> there is absolutely no need to add the proposed low-level debug interface. > >> > > > > I quote Drew's words: > > > > "I think it could be helpful to be able to set the conf_<module>_<pin> > > registers directly through debugfs as I can imagine situations where it would > > be useful for testing. It is a bit dangerous as the person using it has to be > > careful not to change the wrong bits, but they would need to have debugfs mounted > > and permissions to write to it." > > > > "Bits 6:3 are related to what this subsystem would refer to as pin conf > > such as slew, input enable and bias. Thus it might make sense to expose > > something like a select-pinconf file to activate pin conf settings from > > userspace." > > This is already present, please define all wanted configurations of pin > groups and pin group functions, then switch them in runtime. I see no > need of a coarse grained interface here... > > > From the emails exchanged I seem to have understood that there is no way to > > reconfigure slew rate, pull up / down and other properties on the fly. > > I think you still can do all the tasks mentioned above on the recent kernel, > why not? > > I am not closely familiar with TI AM335x pinmux/pinconf controller, and if > needed I can look at the datasheet, but I can imagine that there are pins, > pin groups, and pin group functions controls. Board specific configuration > of pinmux is given in DTS, you can modify it with DT overlays for instance, > and selection of pin group functions in runtime is already possible for > users in runtime. What is missing from the picture, and why do you insist > on re-introduction of a much worse interface? > > > In the kernel version 4.1.6 that I am using on my custom board, I have fixed > > the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, > > this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). > > Exactly, the feature is not needed. > > > The patches I've submitted implement some sort of devmem for pinmux. It too can > > be used in a dangerous way, but it exists and it is used. > > My objection is to giving a "red button" to users, even to users of debugfs. > > If it's possible to keep the "dangerous goods" on developers' side only, > then this shall be preferable, I believe. And fortunately there is such > a mechanism. > > > Anyway, the implementation may be wrong but it does highlight a feature that > > can be useful in testing or prototyping boards but is not present in the kernel. > > Can we then find a solution that is right for everyone? > > Please see the method above. > > In my understanding the problem you are trying to solve shall be defined > much more precised. Can you please elaborate on this part thoroughly? > > I still can not grasp a too generic explanation from you, writing of > totally arbitrary data to controller registers looks senseless to me... > > Assume you are giving a handle to users to write arbitrary data to arbitrary > I/O mem region, will it solve the problem for you? Of course yes. > > But does it sound like a good and acceptable solution? Of course no. > Why? You need a better and more fine grained interface, namely write only > to pinmux I/O mem. Great, that's provided by your change, however another > important condition is still missing, a user shall write only valid data. > Thus a higher level of abstraction is wanted: > > * writing data to I/O mem -- not good enough, > * writing data to pinmux/pinconf I/O mem -- better, but not good enough, > * writing *valid* data to pinmux/pinconf I/O mem -- that's right. So, why not start from the feature you removed? It wrote valid data to pinconf I/O mem. Thanks and regards, Dario > > The validity of data is defined by a developer, the abstraction name > has been mentioned multiple times, it's pin groups and pin group functions. > > -- > Best wishes, > Vladimir
Hi Dario, On 5/27/21 10:23 PM, Dario Binacchi wrote: > Hi Vladimir, > >> Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto: >> >> >> Hi Dario, >> >> On 5/24/21 8:28 PM, Dario Binacchi wrote: >>> Hi Vladimir, >>> >>>> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto: >>>> >>>> >>>> Hello Dario, >>>> >>>> On 5/20/21 11:27 PM, Dario Binacchi wrote: >>>>> The MPUs of some architectures (e.g AM335x) must be in privileged >>>>> operating mode to write on the pinmux registers. In such cases, where >>>>> writes will not work from user space, now it can be done from the pins >>>> >>>> user space has no connection to the problem you're trying to solve. >>>> >>>> Please provide a reasonable rationale for adding a new interface, thank >>>> you in advance. >>>> >>>>> debug file if the platform driver exports the pin_dbg_set() helper among >>>>> the registered operations. >>>>> >>>>> Signed-off-by: Dario Binacchi <dariobin@libero.it> >>>> >>>> I strongly object against this new interface. >>>> >>>> As Andy've already mentioned you have to operate with defined pin groups >>>> and functions, and so far you create an interface with an option to >>>> disasterous misusage, it shall be avoided, because there are better >>>> options. >>>> >>>> What's the issue with a regular declaration of pin groups and functions >>>> on your SoC? When it's done, you can operate on this level of abstraction, >>>> there is absolutely no need to add the proposed low-level debug interface. >>>> >>> >>> I quote Drew's words: >>> >>> "I think it could be helpful to be able to set the conf_<module>_<pin> >>> registers directly through debugfs as I can imagine situations where it would >>> be useful for testing. It is a bit dangerous as the person using it has to be >>> careful not to change the wrong bits, but they would need to have debugfs mounted >>> and permissions to write to it." >>> >>> "Bits 6:3 are related to what this subsystem would refer to as pin conf >>> such as slew, input enable and bias. Thus it might make sense to expose >>> something like a select-pinconf file to activate pin conf settings from >>> userspace." >> >> This is already present, please define all wanted configurations of pin >> groups and pin group functions, then switch them in runtime. I see no >> need of a coarse grained interface here... >> >>> From the emails exchanged I seem to have understood that there is no way to >>> reconfigure slew rate, pull up / down and other properties on the fly. >> >> I think you still can do all the tasks mentioned above on the recent kernel, >> why not? >> >> I am not closely familiar with TI AM335x pinmux/pinconf controller, and if >> needed I can look at the datasheet, but I can imagine that there are pins, >> pin groups, and pin group functions controls. Board specific configuration >> of pinmux is given in DTS, you can modify it with DT overlays for instance, >> and selection of pin group functions in runtime is already possible for >> users in runtime. What is missing from the picture, and why do you insist >> on re-introduction of a much worse interface? >> >>> In the kernel version 4.1.6 that I am using on my custom board, I have fixed >>> the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, >>> this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). >> >> Exactly, the feature is not needed. >> >>> The patches I've submitted implement some sort of devmem for pinmux. It too can >>> be used in a dangerous way, but it exists and it is used. >> >> My objection is to giving a "red button" to users, even to users of debugfs. >> >> If it's possible to keep the "dangerous goods" on developers' side only, >> then this shall be preferable, I believe. And fortunately there is such >> a mechanism. >> >>> Anyway, the implementation may be wrong but it does highlight a feature that >>> can be useful in testing or prototyping boards but is not present in the kernel. >>> Can we then find a solution that is right for everyone? >> >> Please see the method above. >> >> In my understanding the problem you are trying to solve shall be defined >> much more precised. Can you please elaborate on this part thoroughly? >> >> I still can not grasp a too generic explanation from you, writing of >> totally arbitrary data to controller registers looks senseless to me... >> >> Assume you are giving a handle to users to write arbitrary data to arbitrary >> I/O mem region, will it solve the problem for you? Of course yes. >> >> But does it sound like a good and acceptable solution? Of course no. >> Why? You need a better and more fine grained interface, namely write only >> to pinmux I/O mem. Great, that's provided by your change, however another >> important condition is still missing, a user shall write only valid data. >> Thus a higher level of abstraction is wanted: >> >> * writing data to I/O mem -- not good enough, >> * writing data to pinmux/pinconf I/O mem -- better, but not good enough, >> * writing *valid* data to pinmux/pinconf I/O mem -- that's right. > > So, why not start from the feature you removed? > It wrote valid data to pinconf I/O mem. Nope. The interface you've introduced allows to write invalid data, and the choice between writing valid data and writing invalid data is given to a user. Please remove this choice completely, technically it's doable. > >> >> The validity of data is defined by a developer, the abstraction name >> has been mentioned multiple times, it's pin groups and pin group functions. Unfortunately you continue to cling to the broken interface, while I see no comments from you about asked to consider pin groups and pin group functions. -- Best wishes, Vladimir
Hi Vladimir, > Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > > > Hi Dario, > > On 5/27/21 10:23 PM, Dario Binacchi wrote: > > Hi Vladimir, > > > >> Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > >> > >> > >> Hi Dario, > >> > >> On 5/24/21 8:28 PM, Dario Binacchi wrote: > >>> Hi Vladimir, > >>> > >>>> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > >>>> > >>>> > >>>> Hello Dario, > >>>> > >>>> On 5/20/21 11:27 PM, Dario Binacchi wrote: > >>>>> The MPUs of some architectures (e.g AM335x) must be in privileged > >>>>> operating mode to write on the pinmux registers. In such cases, where > >>>>> writes will not work from user space, now it can be done from the pins > >>>> > >>>> user space has no connection to the problem you're trying to solve. > >>>> > >>>> Please provide a reasonable rationale for adding a new interface, thank > >>>> you in advance. > >>>> > >>>>> debug file if the platform driver exports the pin_dbg_set() helper among > >>>>> the registered operations. > >>>>> > >>>>> Signed-off-by: Dario Binacchi <dariobin@libero.it> > >>>> > >>>> I strongly object against this new interface. > >>>> > >>>> As Andy've already mentioned you have to operate with defined pin groups > >>>> and functions, and so far you create an interface with an option to > >>>> disasterous misusage, it shall be avoided, because there are better > >>>> options. > >>>> > >>>> What's the issue with a regular declaration of pin groups and functions > >>>> on your SoC? When it's done, you can operate on this level of abstraction, > >>>> there is absolutely no need to add the proposed low-level debug interface. > >>>> > >>> > >>> I quote Drew's words: > >>> > >>> "I think it could be helpful to be able to set the conf_<module>_<pin> > >>> registers directly through debugfs as I can imagine situations where it would > >>> be useful for testing. It is a bit dangerous as the person using it has to be > >>> careful not to change the wrong bits, but they would need to have debugfs mounted > >>> and permissions to write to it." > >>> > >>> "Bits 6:3 are related to what this subsystem would refer to as pin conf > >>> such as slew, input enable and bias. Thus it might make sense to expose > >>> something like a select-pinconf file to activate pin conf settings from > >>> userspace." > >> > >> This is already present, please define all wanted configurations of pin > >> groups and pin group functions, then switch them in runtime. I see no > >> need of a coarse grained interface here... > >> > >>> From the emails exchanged I seem to have understood that there is no way to > >>> reconfigure slew rate, pull up / down and other properties on the fly. > >> > >> I think you still can do all the tasks mentioned above on the recent kernel, > >> why not? > >> > >> I am not closely familiar with TI AM335x pinmux/pinconf controller, and if > >> needed I can look at the datasheet, but I can imagine that there are pins, > >> pin groups, and pin group functions controls. Board specific configuration > >> of pinmux is given in DTS, you can modify it with DT overlays for instance, > >> and selection of pin group functions in runtime is already possible for > >> users in runtime. What is missing from the picture, and why do you insist > >> on re-introduction of a much worse interface? > >> > >>> In the kernel version 4.1.6 that I am using on my custom board, I have fixed > >>> the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, > >>> this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). > >> > >> Exactly, the feature is not needed. > >> > >>> The patches I've submitted implement some sort of devmem for pinmux. It too can > >>> be used in a dangerous way, but it exists and it is used. > >> > >> My objection is to giving a "red button" to users, even to users of debugfs. > >> > >> If it's possible to keep the "dangerous goods" on developers' side only, > >> then this shall be preferable, I believe. And fortunately there is such > >> a mechanism. > >> > >>> Anyway, the implementation may be wrong but it does highlight a feature that > >>> can be useful in testing or prototyping boards but is not present in the kernel. > >>> Can we then find a solution that is right for everyone? > >> > >> Please see the method above. > >> > >> In my understanding the problem you are trying to solve shall be defined > >> much more precised. Can you please elaborate on this part thoroughly? > >> > >> I still can not grasp a too generic explanation from you, writing of > >> totally arbitrary data to controller registers looks senseless to me... > >> > >> Assume you are giving a handle to users to write arbitrary data to arbitrary > >> I/O mem region, will it solve the problem for you? Of course yes. > >> > >> But does it sound like a good and acceptable solution? Of course no. > >> Why? You need a better and more fine grained interface, namely write only > >> to pinmux I/O mem. Great, that's provided by your change, however another > >> important condition is still missing, a user shall write only valid data. > >> Thus a higher level of abstraction is wanted: > >> > >> * writing data to I/O mem -- not good enough, > >> * writing data to pinmux/pinconf I/O mem -- better, but not good enough, > >> * writing *valid* data to pinmux/pinconf I/O mem -- that's right. > > > > So, why not start from the feature you removed? > > It wrote valid data to pinconf I/O mem. > > Nope. The interface you've introduced allows to write invalid data, and > the choice between writing valid data and writing invalid data is given > to a user. Please remove this choice completely, technically it's doable. I was not pointing to my patch, but to patch f07512e615dd ("pinctrl / pinconfig: add debug interface" (which has been removed). If I am not missing something it wrote data to the pinconf I/O mem in a more controlled way. Could we use this patch as a starting point for a new implementation that uses pin groups and pin group functions ? > > > > >> > >> The validity of data is defined by a developer, the abstraction name > >> has been mentioned multiple times, it's pin groups and pin group functions. > > Unfortunately you continue to cling to the broken interface, while I see no > comments from you about asked to consider pin groups and pin group functions. Could you kindly explain to me, with some practical examples, what kind of interface would you implement ? Thanks and regards, Dario > > -- > Best wishes, > Vladimir
Hi Dario, On 5/27/21 11:33 PM, Dario Binacchi wrote: > Hi Vladimir, > >> Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto: >> >> >> Hi Dario, >> >> On 5/27/21 10:23 PM, Dario Binacchi wrote: >>> Hi Vladimir, >>> >>>> Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto: >>>> >>>> >>>> Hi Dario, >>>> >>>> On 5/24/21 8:28 PM, Dario Binacchi wrote: >>>>> Hi Vladimir, >>>>> >>>>>> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto: >>>>>> >>>>>> >>>>>> Hello Dario, >>>>>> >>>>>> On 5/20/21 11:27 PM, Dario Binacchi wrote: >>>>>>> The MPUs of some architectures (e.g AM335x) must be in privileged >>>>>>> operating mode to write on the pinmux registers. In such cases, where >>>>>>> writes will not work from user space, now it can be done from the pins >>>>>> >>>>>> user space has no connection to the problem you're trying to solve. >>>>>> >>>>>> Please provide a reasonable rationale for adding a new interface, thank >>>>>> you in advance. >>>>>> >>>>>>> debug file if the platform driver exports the pin_dbg_set() helper among >>>>>>> the registered operations. >>>>>>> >>>>>>> Signed-off-by: Dario Binacchi <dariobin@libero.it> >>>>>> >>>>>> I strongly object against this new interface. >>>>>> >>>>>> As Andy've already mentioned you have to operate with defined pin groups >>>>>> and functions, and so far you create an interface with an option to >>>>>> disasterous misusage, it shall be avoided, because there are better >>>>>> options. >>>>>> >>>>>> What's the issue with a regular declaration of pin groups and functions >>>>>> on your SoC? When it's done, you can operate on this level of abstraction, >>>>>> there is absolutely no need to add the proposed low-level debug interface. >>>>>> >>>>> >>>>> I quote Drew's words: >>>>> >>>>> "I think it could be helpful to be able to set the conf_<module>_<pin> >>>>> registers directly through debugfs as I can imagine situations where it would >>>>> be useful for testing. It is a bit dangerous as the person using it has to be >>>>> careful not to change the wrong bits, but they would need to have debugfs mounted >>>>> and permissions to write to it." >>>>> >>>>> "Bits 6:3 are related to what this subsystem would refer to as pin conf >>>>> such as slew, input enable and bias. Thus it might make sense to expose >>>>> something like a select-pinconf file to activate pin conf settings from >>>>> userspace." >>>> >>>> This is already present, please define all wanted configurations of pin >>>> groups and pin group functions, then switch them in runtime. I see no >>>> need of a coarse grained interface here... >>>> >>>>> From the emails exchanged I seem to have understood that there is no way to >>>>> reconfigure slew rate, pull up / down and other properties on the fly. >>>> >>>> I think you still can do all the tasks mentioned above on the recent kernel, >>>> why not? >>>> >>>> I am not closely familiar with TI AM335x pinmux/pinconf controller, and if >>>> needed I can look at the datasheet, but I can imagine that there are pins, >>>> pin groups, and pin group functions controls. Board specific configuration >>>> of pinmux is given in DTS, you can modify it with DT overlays for instance, >>>> and selection of pin group functions in runtime is already possible for >>>> users in runtime. What is missing from the picture, and why do you insist >>>> on re-introduction of a much worse interface? >>>> >>>>> In the kernel version 4.1.6 that I am using on my custom board, I have fixed >>>>> the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However, >>>>> this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/). >>>> >>>> Exactly, the feature is not needed. >>>> >>>>> The patches I've submitted implement some sort of devmem for pinmux. It too can >>>>> be used in a dangerous way, but it exists and it is used. >>>> >>>> My objection is to giving a "red button" to users, even to users of debugfs. >>>> >>>> If it's possible to keep the "dangerous goods" on developers' side only, >>>> then this shall be preferable, I believe. And fortunately there is such >>>> a mechanism. >>>> >>>>> Anyway, the implementation may be wrong but it does highlight a feature that >>>>> can be useful in testing or prototyping boards but is not present in the kernel. >>>>> Can we then find a solution that is right for everyone? >>>> >>>> Please see the method above. >>>> >>>> In my understanding the problem you are trying to solve shall be defined >>>> much more precised. Can you please elaborate on this part thoroughly? >>>> >>>> I still can not grasp a too generic explanation from you, writing of >>>> totally arbitrary data to controller registers looks senseless to me... >>>> >>>> Assume you are giving a handle to users to write arbitrary data to arbitrary >>>> I/O mem region, will it solve the problem for you? Of course yes. >>>> >>>> But does it sound like a good and acceptable solution? Of course no. >>>> Why? You need a better and more fine grained interface, namely write only >>>> to pinmux I/O mem. Great, that's provided by your change, however another >>>> important condition is still missing, a user shall write only valid data. >>>> Thus a higher level of abstraction is wanted: >>>> >>>> * writing data to I/O mem -- not good enough, >>>> * writing data to pinmux/pinconf I/O mem -- better, but not good enough, >>>> * writing *valid* data to pinmux/pinconf I/O mem -- that's right. >>> >>> So, why not start from the feature you removed? >>> It wrote valid data to pinconf I/O mem. >> >> Nope. The interface you've introduced allows to write invalid data, and >> the choice between writing valid data and writing invalid data is given >> to a user. Please remove this choice completely, technically it's doable. > > I was not pointing to my patch, but to patch f07512e615dd ("pinctrl / pinconfig: add debug interface" > (which has been removed). If I am not missing something it wrote data to the > pinconf I/O mem in a more controlled way. Right, but still it's not good enough, the reasons are given in the message of the commit, which reverts the change. And again and again, it is not based on the most reasonable user-visible abstractions as pin groups and functions. > Could we use this patch as a starting point for a new implementation that > uses pin groups and pin group functions ? Well, you can include this patch into your development process as one of the initial stages, but it makes no sense to resurrect it in the vanilla. >> >>> >>>> >>>> The validity of data is defined by a developer, the abstraction name >>>> has been mentioned multiple times, it's pin groups and pin group functions. >> >> Unfortunately you continue to cling to the broken interface, while I see no >> comments from you about asked to consider pin groups and pin group functions. > > Could you kindly explain to me, with some practical examples, what kind of interface > would you implement ? > So far I'm quite happy with device tree overlays, for me it gives a solution to a problem which you're trying to address, but that's my guess only, because I still don't quite understand what do you seek for... Changing pin slew rates in runtime or something else? Why can't your keep this strange function out of vanilla or just make it specific to the AM335x controller? Any reasonable pinmux/pinctrl "debug" interface anyway should be based on board device trees/firmware, that's the right place to describe SoC/board specific valid configurations of the controller. -- Best wishes, Vladimir
On Thu, May 27, 2021 at 10:33 PM Dario Binacchi <dariobin@libero.it> wrote: > > Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > > Unfortunately you continue to cling to the broken interface, while I see no > > comments from you about asked to consider pin groups and pin group functions. > > Could you kindly explain to me, with some practical examples, what kind of interface > would you implement ? I am not fully understanding this discussion. I get the feeling that this is caused by the early architectural decisions with pinctrl-single to put all configuration of pin groups and functions per pin into the device tree. Tony specifically wanted this because what he gets from TI are some raw ASIC data dumps from the designers, that he could make a script to process into device tree rather than into .h files, and get this out of the kernel. (As I remember it, Tony correct me if I'm wrong.) This makes it hard to align some concepts of the pin control subsystem such as functions and groups because pinctrl-single assume a 1-to-1 mapping between one pin and one group, which in turn has a 1-to-many mapping to functions. Is the patch trying to debugfs around this somehow? If this hack is only needed for pinctrl-single.c then it should be placed in that driver, so Tony can review it and maintain it as applicable in that driver's context only, not in the pinctrl core as it appears the general applicability for other drivers is not there. Would this really be useful for other drivers than pinctrl-single.c? Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [210528 09:08]: > On Thu, May 27, 2021 at 10:33 PM Dario Binacchi <dariobin@libero.it> wrote: > > > Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > > > > Unfortunately you continue to cling to the broken interface, while I see no > > > comments from you about asked to consider pin groups and pin group functions. > > > > Could you kindly explain to me, with some practical examples, what kind of interface > > would you implement ? > > I am not fully understanding this discussion. > > I get the feeling that this is caused by the early architectural decisions with > pinctrl-single to put all configuration of pin groups and functions per pin into > the device tree. > > Tony specifically wanted this because what he gets from TI are some raw > ASIC data dumps from the designers, that he could make a script to process > into device tree rather than into .h files, and get this out of the kernel. > (As I remember it, Tony correct me if I'm wrong.) Yeah the idea was to avoid stuffing even more SoC specific data into the kernel and rather use devicetree data only for the booted SoC. > This makes it hard to align some concepts of the pin control subsystem such > as functions and groups because pinctrl-single assume a 1-to-1 mapping > between one pin and one group, which in turn has a 1-to-many mapping > to functions. > > Is the patch trying to debugfs around this somehow? > > If this hack is only needed for pinctrl-single.c then it should be placed in > that driver, so Tony can review it and maintain it as applicable in that > driver's context only, not in the pinctrl core as it appears the general > applicability for other drivers is not there. > > Would this really be useful for other drivers than pinctrl-single.c? I'd rather go with a generic interface. I think it should work if we only allow enabling and disabling of unclaimed pingroups from sysfs. And then we can also allow creating new pingroups for unclaimed pins if needed. Regards, Tony
Hi, > Il 02/06/2021 07:03 Tony Lindgren <tony@atomide.com> ha scritto: > > > * Linus Walleij <linus.walleij@linaro.org> [210528 09:08]: > > On Thu, May 27, 2021 at 10:33 PM Dario Binacchi <dariobin@libero.it> wrote: > > > > Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto: > > > > > > Unfortunately you continue to cling to the broken interface, while I see no > > > > comments from you about asked to consider pin groups and pin group functions. > > > > > > Could you kindly explain to me, with some practical examples, what kind of interface > > > would you implement ? > > > > I am not fully understanding this discussion. > > > > I get the feeling that this is caused by the early architectural decisions with > > pinctrl-single to put all configuration of pin groups and functions per pin into > > the device tree. > > > > Tony specifically wanted this because what he gets from TI are some raw > > ASIC data dumps from the designers, that he could make a script to process > > into device tree rather than into .h files, and get this out of the kernel. > > (As I remember it, Tony correct me if I'm wrong.) > > Yeah the idea was to avoid stuffing even more SoC specific data into the > kernel and rather use devicetree data only for the booted SoC. > > > This makes it hard to align some concepts of the pin control subsystem such > > as functions and groups because pinctrl-single assume a 1-to-1 mapping > > between one pin and one group, which in turn has a 1-to-many mapping > > to functions. > > > > Is the patch trying to debugfs around this somehow? > > > > If this hack is only needed for pinctrl-single.c then it should be placed in > > that driver, so Tony can review it and maintain it as applicable in that > > driver's context only, not in the pinctrl core as it appears the general > > applicability for other drivers is not there. > > > > Would this really be useful for other drivers than pinctrl-single.c? > > I'd rather go with a generic interface. I think it should work if we only > allow enabling and disabling of unclaimed pingroups from sysfs. And then > we can also allow creating new pingroups for unclaimed pins if needed. > Could you kindly explain to me, with some practical examples, what kind of interface would you implement ? Or something similar to start from that is already in the Kernel? Thanks and regards, Dario > Regards, > > Tony
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index a4ac87c8b4f8..ab832044a0c3 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -1620,6 +1620,53 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state); #ifdef CONFIG_DEBUG_FS +static ssize_t pinctrl_pins_write(struct file *file, + const char __user *user_buf, size_t count, + loff_t *ppos) +{ + struct seq_file *s = file->private_data; + struct pinctrl_dev *pctldev = s->private; + const struct pinctrl_ops *ops = pctldev->desc->pctlops; + char buf[32]; + char *c = &buf[0]; + char *token; + int ret; + unsigned int i, pin, val; + + if (!ops->pin_dbg_set) + return -EFAULT; + + ret = strncpy_from_user(buf, user_buf, sizeof(buf)); + if (ret == 0 || ret == sizeof(buf)) + ret = -ERANGE; + + if (ret < 0) + return ret; + + token = strsep(&c, " "); + ret = kstrtouint(token, 0, &pin); + if (ret) + return ret; + + token = strsep(&c, " "); + ret = kstrtouint(token, 0, &val); + if (ret) + return ret; + + for (i = 0; i < pctldev->desc->npins; i++) { + if (pin != pctldev->desc->pins[i].number) + continue; + + ret = ops->pin_dbg_set(pctldev, pin, val); + if (ret) + return ret; + + return count; + } + + return -EINVAL; +} + static int pinctrl_pins_show(struct seq_file *s, void *what) { struct pinctrl_dev *pctldev = s->private; @@ -1677,7 +1724,11 @@ static int pinctrl_pins_show(struct seq_file *s, void *what) return 0; } -DEFINE_SHOW_ATTRIBUTE(pinctrl_pins); + +static int pinctrl_pins_open(struct inode *inode, struct file *file) +{ + return single_open(file, pinctrl_pins_show, inode->i_private); +} static int pinctrl_groups_show(struct seq_file *s, void *what) { @@ -1886,6 +1937,14 @@ static int pinctrl_show(struct seq_file *s, void *what) } DEFINE_SHOW_ATTRIBUTE(pinctrl); +static const struct file_operations pinctrl_pins_fops = { + .open = pinctrl_pins_open, + .read = seq_read, + .write = pinctrl_pins_write, + .llseek = seq_lseek, + .release = single_release, +}; + static struct dentry *debugfs_root; static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev) @@ -1915,7 +1974,7 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev) dev_name(pctldev->dev)); return; } - debugfs_create_file("pins", 0444, + debugfs_create_file("pins", 0644, device_root, pctldev, &pinctrl_pins_fops); debugfs_create_file("pingroups", 0444, device_root, pctldev, &pinctrl_groups_fops); diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h index 70b45d28e7a9..7ae8aca3dfa5 100644 --- a/include/linux/pinctrl/pinctrl.h +++ b/include/linux/pinctrl/pinctrl.h @@ -75,6 +75,8 @@ struct pinctrl_gpio_range { * group selector @pins, and the size of the array in @num_pins * @pin_dbg_show: optional debugfs display hook that will provide per-device * info for a certain pin in debugfs + * @pin_dbg_set: optional debugfs set hook that will write per-device pinmux + * register for a certain pin in debugfs * @dt_node_to_map: parse a device tree "pin configuration node", and create * mapping table entries for it. These are returned through the @map and * @num_maps output parameters. This function is optional, and may be @@ -95,6 +97,8 @@ struct pinctrl_ops { unsigned *num_pins); void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s, unsigned offset); + int (*pin_dbg_set) (struct pinctrl_dev *pctldev, unsigned int offset, + unsigned int val); int (*dt_node_to_map) (struct pinctrl_dev *pctldev, struct device_node *np_config, struct pinctrl_map **map, unsigned *num_maps);
The MPUs of some architectures (e.g AM335x) must be in privileged operating mode to write on the pinmux registers. In such cases, where writes will not work from user space, now it can be done from the pins debug file if the platform driver exports the pin_dbg_set() helper among the registered operations. Signed-off-by: Dario Binacchi <dariobin@libero.it> --- Changes in v3: - Use strncpy_from_user() instead of copy_from_user(). - Do not shadow the error code returned by kstrtouint(). - Change pin_dbg_set() interface (char *buf --> unsigned int val). - Describe pin_dbg_set(). drivers/pinctrl/core.c | 63 +++++++++++++++++++++++++++++++-- include/linux/pinctrl/pinctrl.h | 4 +++ 2 files changed, 65 insertions(+), 2 deletions(-)