| Message ID | 20240503162217.1999467-1-sean.anderson@linux.dev |
|---|---|
| Headers | show |
| Series | pinctrl: zynqmp: Support muxing individual pins | expand |
On Fri, May 3, 2024 at 6:22 PM Sean Anderson <sean.anderson@linux.dev> wrote: > This series adds support for muxing individual pins, instead of > requiring groups to be muxed together. See [1] for additional > discussion. > > [1] https://lore.kernel.org/linux-arm-kernel/5bb0dc7e-4c89-4f3d-abc6-41ae9ded5ae9@linux.dev/ The way I usually would recommend to solve this would be to define new subgroups, so e.g. for a UARTS: uart0_grp = pin_rx, pin_tx, pin_cts, pin_dts, pin_dcd; And today this would be used like that: mux0: function = "uart0"; groups = "uart0_grp"; Then we realize that not everyone need all the modem control signals provided. What to do. Well this: uart0_rxtx_grp = pin_rx, pin_tx: uart0_modem_grp = pin_cts, pin_dts, pin_dcd; mux0: function = "uart0"; groups = "uart0_rxtx_grp"; Now the CTS, DTS, DCD pins can be reused for something else such as GPIO. I *know* that this breaks ABI: the driver group definitions change and the device tree needs to be changed too. This only matters if the users have a habit of distributing the kernel and DTB separately so a new kernel needs to support and old DTB. This varies in how much control we have but I think for Xilinx systems the kernel and DTB are always updated in lockstep, so it really does not matter? Yours, Linus Walleij
On 5/6/24 02:43, Linus Walleij wrote: > On Fri, May 3, 2024 at 6:22 PM Sean Anderson <sean.anderson@linux.dev> wrote: > >> This series adds support for muxing individual pins, instead of >> requiring groups to be muxed together. See [1] for additional >> discussion. >> >> [1] https://lore.kernel.org/linux-arm-kernel/5bb0dc7e-4c89-4f3d-abc6-41ae9ded5ae9@linux.dev/ > > The way I usually would recommend to solve this would be to > define new subgroups, so e.g. for a UARTS: > > uart0_grp = pin_rx, pin_tx, pin_cts, pin_dts, pin_dcd; > > And today this would be used like that: > > mux0: > function = "uart0"; > groups = "uart0_grp"; > > Then we realize that not everyone need all the modem > control signals provided. What to do. Well this: > > uart0_rxtx_grp = pin_rx, pin_tx: > uart0_modem_grp = pin_cts, pin_dts, pin_dcd; > > mux0: > function = "uart0"; > groups = "uart0_rxtx_grp"; > > Now the CTS, DTS, DCD pins can be reused for something > else such as GPIO. > > I *know* that this breaks ABI: the driver group definitions change > and the device tree needs to be changed too. > > This only matters if the users have a habit of distributing the > kernel and DTB separately so a new kernel needs to support > and old DTB. This varies in how much control we have but I > think for Xilinx systems the kernel and DTB are always updated > in lockstep, so it really does not matter? Well, the pin groups are actually defined in the PMU firmware. And frankly, I don't see the point of pin "groups" when there are not actual pin groups at the hardware level. The pins can all be muxed individually, so there's no point in adding artificial groups on top. Just mux the pins like the hardware allows and everything is easy. Cuts down on the absurd number of strings too. --Sean
On 5/6/24 16:45, Sean Anderson wrote: > On 5/6/24 02:43, Linus Walleij wrote: >> On Fri, May 3, 2024 at 6:22 PM Sean Anderson <sean.anderson@linux.dev> wrote: >> >>> This series adds support for muxing individual pins, instead of >>> requiring groups to be muxed together. See [1] for additional >>> discussion. >>> >>> [1] https://lore.kernel.org/linux-arm-kernel/5bb0dc7e-4c89-4f3d-abc6-41ae9ded5ae9@linux.dev/ >> >> The way I usually would recommend to solve this would be to >> define new subgroups, so e.g. for a UARTS: >> >> uart0_grp = pin_rx, pin_tx, pin_cts, pin_dts, pin_dcd; >> >> And today this would be used like that: >> >> mux0: >> function = "uart0"; >> groups = "uart0_grp"; >> >> Then we realize that not everyone need all the modem >> control signals provided. What to do. Well this: >> >> uart0_rxtx_grp = pin_rx, pin_tx: >> uart0_modem_grp = pin_cts, pin_dts, pin_dcd; >> >> mux0: >> function = "uart0"; >> groups = "uart0_rxtx_grp"; >> >> Now the CTS, DTS, DCD pins can be reused for something >> else such as GPIO. >> >> I *know* that this breaks ABI: the driver group definitions change >> and the device tree needs to be changed too. >> >> This only matters if the users have a habit of distributing the >> kernel and DTB separately so a new kernel needs to support >> and old DTB. This varies in how much control we have but I >> think for Xilinx systems the kernel and DTB are always updated >> in lockstep, so it really does not matter? > > Well, the pin groups are actually defined in the PMU firmware. And > frankly, I don't see the point of pin "groups" when there are not actual > pin groups at the hardware level. The pins can all be muxed > individually, so there's no point in adding artificial groups on top. > Just mux the pins like the hardware allows and everything is easy. Cuts > down on the absurd number of strings too. That Linus example is split which would make sense but as Sean said HW is not really working like this. Because you can actually take tx from group0 and rx from group5. You can't configure it in design tools but you can configure via registers and it will just work fine. Thanks, Michal
On Mon, May 6, 2024 at 4:45 PM Sean Anderson <sean.anderson@linux.dev> wrote: > > Then we realize that not everyone need all the modem > > control signals provided. What to do. Well this: > > > > uart0_rxtx_grp = pin_rx, pin_tx: > > uart0_modem_grp = pin_cts, pin_dts, pin_dcd; > > > > mux0: > > function = "uart0"; > > groups = "uart0_rxtx_grp"; > > > > Now the CTS, DTS, DCD pins can be reused for something > > else such as GPIO. > > > > I *know* that this breaks ABI: the driver group definitions change > > and the device tree needs to be changed too. Actually I didn't think that over, it is possible to add new groups and retain the old ones. I.e. retain uart0_grp, but additionally add and use uart0_rxtx and uart0_modem_grp and use one or the other approach. > Well, the pin groups are actually defined in the PMU firmware. Is that firmware written in such an helpful way that the groups can be extracted from the firmware then, as with SCMI? Or is it a matter of duplicating the info from the PMU in the software-defined groups. > And > frankly, I don't see the point of pin "groups" when there are not actual > pin groups at the hardware level. The pins can all be muxed > individually, so there's no point in adding artificial groups on top. > Just mux the pins like the hardware allows and everything is easy. Cuts > down on the absurd number of strings too. So are you going to switch all of Xilinx devicetrees over to using exclusively the new method (muxing individual pins)? I'm fine with one (string identified groups) which I encourage, but I let individual pin control pass as well on several occasions. What I don't want to see is a Franken-solution that mixes the two approaches, even less so on the same system. Someone is going to have to maintain the resulting mess. And this looks like exactly that. If you want to mux individual pins instead of groups and functions, by all means, but please do not mix the two approaches in the same driver, I'm just trying to save Xilinx from themselves here. Yours, Linus Walleij
On 5/27/24 09:15, Linus Walleij wrote: > On Mon, May 6, 2024 at 4:45 PM Sean Anderson <sean.anderson@linux.dev> wrote: > >> > Then we realize that not everyone need all the modem >> > control signals provided. What to do. Well this: >> > >> > uart0_rxtx_grp = pin_rx, pin_tx: >> > uart0_modem_grp = pin_cts, pin_dts, pin_dcd; >> > >> > mux0: >> > function = "uart0"; >> > groups = "uart0_rxtx_grp"; >> > >> > Now the CTS, DTS, DCD pins can be reused for something >> > else such as GPIO. >> > >> > I *know* that this breaks ABI: the driver group definitions change >> > and the device tree needs to be changed too. > > Actually I didn't think that over, it is possible to add new groups > and retain the old ones. > > I.e. retain uart0_grp, but additionally add and use > uart0_rxtx and uart0_modem_grp and use one or the > other approach. That is what this patch does. >> Well, the pin groups are actually defined in the PMU firmware. > > Is that firmware written in such an helpful way that the groups > can be extracted from the firmware then, as with SCMI? Or is it > a matter of duplicating the info from the PMU in the software-defined > groups. Fundamentally, the pin muxings are known a priori from the reference manual. Because pinmuxing itself has been delegated to the PMU firmware, we defer to it when determining what muxings are available. The PMU firmware describes muxings in terms of pins and functions; groups are a Linux-only concept. >> And >> frankly, I don't see the point of pin "groups" when there are not actual >> pin groups at the hardware level. The pins can all be muxed >> individually, so there's no point in adding artificial groups on top. >> Just mux the pins like the hardware allows and everything is easy. Cuts >> down on the absurd number of strings too. > > So are you going to switch all of Xilinx devicetrees over to using exclusively > the new method (muxing individual pins)? No. We have to support it anyway for compatibility, so there is no point in changing everything for no reason. > I'm fine with one (string identified groups) which I encourage, but I > let individual pin control pass as well on several occasions. > > What I don't want to see is a Franken-solution that mixes the two > approaches, even less so on the same system. Someone is going to > have to maintain the resulting mess. And this looks like exactly that. Well, perhaps you should have reviewed the original driver more closely. > If you want to mux individual pins instead of groups and functions, by > all means, but please do not mix the two approaches in the same > driver, I'm just trying to save Xilinx from themselves here. I see no point in creating thousands of groups for every combination of pin muxings when we could just switch to the solution in this (or v2 of) patch. For compatibility we cannot be rid of the old situation, but we can at least fix it. There is no technical problem with them coexisting. --Sean
On Tue, May 28, 2024 at 4:28 PM Sean Anderson <sean.anderson@linux.dev> wrote: > Well, perhaps you should have reviewed the original driver more > closely. Do you want to push me down and increase my work related stress? Because that is the effect of such statements. It looks like criticism of me as a person, so explain yourself. Writing this kind of things looks to me like some kind of abusive way to express your desire and that is what burns maintainers out, so if that is what you are doing, stop doing that, adjust your behaviour and focus on technical issues. > > If you want to mux individual pins instead of groups and functions, by > > all means, but please do not mix the two approaches in the same > > driver, I'm just trying to save Xilinx from themselves here. > > I see no point in creating thousands of groups Please share your calculations for figures like "thousands". In my experience, groups are usually in the tens, perhaps hundreds, physically restricted by the number of pins underneath a BGA. A Micro-FCBGA has 479 balls and many are GND and power, so that sets a ballpark figure. > for every combination of pin musings It is clear from the documentation that the point if the pinmux groups and pins are not to present all possible options (known as a "phone exchange" solution) but those that are used in practice, i.e. these representing real world use cases. See below. > when we could just switch to the solution in this (or v2 of) > patch. For compatibility we cannot be rid of the old situation, but we > can at least fix it. There is no technical problem with them coexisting. Historically there are ~2 camps: - One camp want to use groups and functions to combine pins in groups with functions to form usecases. In some cases (such as pinctrl-gemini.c or the very latest pinctrl-scmi.c merged for v6.10) this reflects how the hardware actually looks: it does not make individual pins available for muxing, but you poke bits or send messages to change entire groups-to-function mappings, so it is necessary for some hardware. So when you write that "groups are a Linux-only concept" this is because you probably haven't seen this part of the world. Groups exist in hardware, and in the SCMI specification. There are systems with individual control of the muxing of every pin, such that e.g. every pin has a muxing register. These are again not really phone exchanges: I am yet to see a system where any function can be mapped to any pin. These just do not exist. What exists in practice is that each pin can be mapped to 2-4 functions, in extreme cases some more. Often these functions are mapped to adjacent pins, and the "chessboard" picture in the documentation for the subsystem reflects this. For this reason, it is often helpful for driver writers to group adjacent pins into groups, so an iterator can walk over the pins and poke their registers in order, instead of treating each pin as a unique entity. - Then there is the camp that just by habit *want* to control each pin individually. The extreme example is pinctrl-single.c which is named like such because each pin is controlled by a single register. TI wanted this solution mainly because their hardware wasn't described in manuals, but in other HW description files, and they needed to process large volumes of data into DT-form. I didn't like this solution initially because it makes it hard for people without datasheets to understand what is going on. But I was convinced to let this coexist with the group and function mapping, which is fine: maybe one size doesn't fit all. i.MX and others also do this approach but with large sets of defines in the <dt-bindings/*> files. Combining these two approaches is not something I recommend. Yours, Linus Walleij
On 5/29/24 04:38, Linus Walleij wrote: > On Tue, May 28, 2024 at 4:28 PM Sean Anderson <sean.anderson@linux.dev> wrote: > >> Well, perhaps you should have reviewed the original driver more >> closely. > > Do you want to push me down and increase my work related > stress? Because that is the effect of such statements. > > It looks like criticism of me as a person, so explain yourself. > > Writing this kind of things looks to me like some kind of abusive way > to express your desire and that is what burns maintainers out, so > if that is what you are doing, stop doing that, adjust your behaviour > and focus on technical issues. The technical issue is that the driver does not match the hardware. We must maintain the existing set of groups for backwards-compatibility. But this should not prevent improvement. Saying that we cannot have both group styles means that the driver is permanently stuck with whatever was picked when it was submitted. Hence, if you want to have only one style you had better review new drivers very carefully. >> > If you want to mux individual pins instead of groups and functions, by >> > all means, but please do not mix the two approaches in the same >> > driver, I'm just trying to save Xilinx from themselves here. >> >> I see no point in creating thousands of groups > > Please share your calculations for figures like "thousands". > > In my experience, groups are usually in the tens, perhaps > hundreds, physically restricted by the number of pins > underneath a BGA. A Micro-FCBGA has 479 balls and many > are GND and power, so that sets a ballpark figure. There are 78 muxable pins on this hardware, and around 40 groups, each with signals that can be muxed to each pin. If we were to create groups for each combination of signals and pins, there would literally be thousands of groups. >> for every combination of pin musings > > It is clear from the documentation that the point if the pinmux > groups and pins are not to present all possible options (known as > a "phone exchange" solution) but those that are used in practice, > i.e. these representing real world use cases. See below. > >> when we could just switch to the solution in this (or v2 of) >> patch. For compatibility we cannot be rid of the old situation, but we >> can at least fix it. There is no technical problem with them coexisting. > > Historically there are ~2 camps: > > - One camp want to use groups and > functions to combine pins in groups with functions to form usecases. > > In some cases (such as pinctrl-gemini.c or the very latest > pinctrl-scmi.c merged for v6.10) this reflects how the hardware > actually looks: it does not make individual pins available for muxing, > but you poke bits or send messages to change entire > groups-to-function mappings, so it is necessary for some hardware. > > So when you write that "groups are a Linux-only concept" this > is because you probably haven't seen this part of the world. > Groups exist in hardware, and in the SCMI specification. What I mean is that, for this hardware, groups are a Linux only concept. Neither the firmware nor the hardware itself has a concept of groups. While other hardware may have this concept, it does not apply here. I do not object to groups where that is the hardware reality, but they are unnecessarily constraining for this part. > There are systems with individual control of the muxing > of every pin, such that e.g. every pin has a muxing register. > > These are again not really phone exchanges: I am yet to see > a system where any function can be mapped to any pin. These > just do not exist. Canaan K210. > What exists in practice is that each pin can be mapped to 2-4 > functions, in extreme cases some more. Often these functions are > mapped to adjacent pins, and the "chessboard" picture in the > documentation for the subsystem reflects this. > > For this reason, it is often helpful for driver writers to group > adjacent pins into groups, so an iterator can walk over the > pins and poke their registers in order, instead of treating each > pin as a unique entity. > > - Then there is the camp that just by habit *want* to control > each pin individually. The extreme example is pinctrl-single.c > which is named like such because each pin is controlled by > a single register. TI wanted this solution mainly because their > hardware wasn't described in manuals, but in other HW > description files, and they needed to process large volumes > of data into DT-form. > > I didn't like this solution initially because it makes it hard for > people without datasheets to understand what is going on. > But I was convinced to let this coexist with the group and function > mapping, which is fine: maybe one size doesn't fit all. > > i.MX and others also do this approach but with large sets of > defines in the <dt-bindings/*> files. > > Combining these two approaches is not something I recommend. Well, the former approach is wrong for this hardware, but we must support it for backwards-compatibility. A combination is the obvious solution. --Sean
On Thu, May 30, 2024 at 7:08 PM Sean Anderson <sean.anderson@linux.dev> wrote: > On 5/29/24 04:38, Linus Walleij wrote: > > On Tue, May 28, 2024 at 4:28 PM Sean Anderson <sean.anderson@linux.dev> wrote: > > > >> Well, perhaps you should have reviewed the original driver more > >> closely. > > > > Do you want to push me down and increase my work related > > stress? Because that is the effect of such statements. > > > > It looks like criticism of me as a person, so explain yourself. > > > > Writing this kind of things looks to me like some kind of abusive way > > to express your desire and that is what burns maintainers out, so > > if that is what you are doing, stop doing that, adjust your behaviour > > and focus on technical issues. > > The technical issue is that the driver does not match the hardware. We > must maintain the existing set of groups for backwards-compatibility. > But this should not prevent improvement. > > Saying that we cannot have both group styles means that the driver is > permanently stuck with whatever was picked when it was submitted. Hence, > if you want to have only one style you had better review new drivers > very carefully. Actually I did say you can rewrite it to the other style, it's just work. If the previous approach was wrong, just redo it as it should be, and rewrite the DT bindings and the existing device trees. If backward-compatibility is so important, add a new driver with a new unique Kconfig CONFIG_PINCTRL_ZYNQMP_V2 and new bindings on the side and select one from a new compatible such as "xlnx,zynqmp-pinctrl-v2", problem solved: new driver new bindings, can be used on a per-board basis, can be compiled into the same kernel image. It may be embarrassing to have to tell the device tree maintainers that the bindings got wrong three years ago and now we need to roll a v2, but worse things have happened. I don't like the approach "this was done so we cannot redo it", we can always redo things, it is even expected as proven by Fred Brooks timeless statement in "The Mythical Man-Month": any team *will* always design a throw-away system whether they intend it or not, there will be a second version. This approach will be more clean, I think? Also it will be possible to phase over more boards and perhaps eventually drop the old driver and the old bindings. I'd like to hear from Xilinx/AMD how they want to solve this going forward. Yours, Linus Walleij
On 6/3/24 11:02, Linus Walleij wrote: > On Thu, May 30, 2024 at 7:08 PM Sean Anderson <sean.anderson@linux.dev> wrote: >> On 5/29/24 04:38, Linus Walleij wrote: >>> On Tue, May 28, 2024 at 4:28 PM Sean Anderson <sean.anderson@linux.dev> wrote: >>> >>>> Well, perhaps you should have reviewed the original driver more >>>> closely. >>> >>> Do you want to push me down and increase my work related >>> stress? Because that is the effect of such statements. >>> >>> It looks like criticism of me as a person, so explain yourself. >>> >>> Writing this kind of things looks to me like some kind of abusive way >>> to express your desire and that is what burns maintainers out, so >>> if that is what you are doing, stop doing that, adjust your behaviour >>> and focus on technical issues. >> >> The technical issue is that the driver does not match the hardware. We >> must maintain the existing set of groups for backwards-compatibility. >> But this should not prevent improvement. >> >> Saying that we cannot have both group styles means that the driver is >> permanently stuck with whatever was picked when it was submitted. Hence, >> if you want to have only one style you had better review new drivers >> very carefully. > > Actually I did say you can rewrite it to the other style, it's just work. > > If the previous approach was wrong, just redo it as it should be, > and rewrite the DT bindings and the existing device trees. If > backward-compatibility is so important, add a new driver with a new > unique Kconfig CONFIG_PINCTRL_ZYNQMP_V2 and new bindings > on the side and select one from a new compatible such as > "xlnx,zynqmp-pinctrl-v2", problem solved: > new driver new bindings, can be used on a per-board basis, > can be compiled into the same kernel image. > > It may be embarrassing to have to tell the device tree maintainers > that the bindings got wrong three years ago and now we need to roll > a v2, but worse things have happened. > > I don't like the approach > "this was done so we cannot redo it", we can always redo things, > it is even expected as proven by Fred Brooks timeless statement > in "The Mythical Man-Month": any team *will* always design > a throw-away system whether they intend it or not, there will be > a second version. > > This approach will be more clean, I think? Also it will be > possible to phase over more boards and perhaps eventually > drop the old driver and the old bindings. > > I'd like to hear from Xilinx/AMD how they want to solve this > going forward. Sorry for delay jumping to this long discussion. Groups were chosen because that's how design tool (Vivado) allow you to configure it. That's why these groups are described in TF-A. That's something what fits the need for most of use cases. But then there is technical side of things what can be setup via registers. If you look at uart0. There are 19 groups in Vivado. uart0_grp0 MIO2-3, etc uart0_grp19 MIO74-75 and that's what it is described today. But it is completely valid if you take MIO 2 from grp0 and MIO75 from grp19. It will work without any issue but likely we can't described it today. Uart is interesting one because you can configure one RX channel and up to 19 (number of groups) TX and it will work properly from HW perspective. I don't think you can describe it via DT today. In Sean's use case with SD we don't have groups with less then 4 data pins but SD cards should be still working and that's what we can't really describe too. I don't think Xilinx recommends these descriptions and normally customers are not design it like this but from HW perspective it should be working properly. Regarding driver. I agree that describing all possible configurations via groups is not a good idea because describing all combinations is very hard. It is not unlimited number of them but that number is very very high and it won't serve the purpose. TF-A should be capable to provide all information about configurations already it is just up to OS to use it properly. If there is something missing we can take a look but we have already issue with TF-A running out of space. From my perspective allowing functionality per single MIO pins is not a bad way to go. Having it in separate driver is possible but don't think it is worth of effort. I have asked Sai (driver owner) to take a look at the patch more closely. Thanks, Michal
On Mon, Jun 3, 2024 at 2:37 PM Michal Simek <michal.simek@amd.com> wrote:
> I have asked Sai (driver owner) to take a look at the patch more closely.
OK if you and Sai both provide your Reviewed-by tags so I know this is what you
want to happen, then I'll apply the patch.
Thanks Michal!
Yours,
Linus Walleij