Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 16 May 2023 13:59:38 -0500 | Subject | Re: [PATCH] mux: mmio: use reg property when parent device is not a syscon | From | Andrew Davis <> |
| |
On 5/16/23 1:33 PM, Krzysztof Kozlowski wrote: > On 16/05/2023 19:47, Andrew Davis wrote: >> On 5/16/23 11:49 AM, Krzysztof Kozlowski wrote: >>> On 16/05/2023 18:29, Andrew Davis wrote: >>>> On 5/16/23 11:19 AM, Krzysztof Kozlowski wrote: >>>>> On 16/05/2023 17:18, Andrew Davis wrote: >>>>>> On 5/15/23 4:14 PM, Peter Rosin wrote: >>>>>>> Hi! >>>>>>> >>>>>>> 2023-05-15 at 21:19, Andrew Davis wrote: >>>>>>>> The DT binding for the reg-mux compatible states it can be used when the >>>>>>>> "parent device of mux controller is not syscon device". It also allows >>>>>>>> for a reg property. When the parent device is indeed not a syscon device, >>>>>>>> nor is it a regmap provider, we should fallback to using that reg >>>>>>>> property to identify the address space to use for this mux. >>>>>>> >>>>>>> We should? Says who? >>>>>>> >>>>>>> Don't get me wrong, I'm not saying the change is bad or wrong, I would just >>>>>>> like to see an example where it matters. Or, at least some rationale for why >>>>>>> the code needs to change other than covering some case that looks like it >>>>>>> could/should be possible based on the binding. I.e., why is it not better to >>>>>>> "close the hole" in the binding instead? >>>>>>> >>>>>> >>>>>> Sure, so this all stated when I was building a checker to make sure that drivers >>>>>> are not mapping overlapping register spaces. I noticed syscon nodes are a source >>>>>> of that so I'm trying to look into their usage. >>>>>> >>>>>> To start, IHMO there is only one valid use for syscon and that is when more than >>>>>> one driver needs to access shared bits in a single register. DT has no way to >>>>> >>>>> It has... what about all existing efuse/nvmem devices? >>>>> >>>>>> describe down to the bit granular level, so one must give that register to >>>>>> a "syscon node", then have the device node use a phandle to the syscon node: >>>>>> >>>>>> common_reg: syscon@10000 { >>>>>> compatible = "syscon"; >>>>>> reg = <0x10000 0x4>; >>>>>> }; >>>>>> >>>>>> consumer@1 { >>>>>> syscon-efuse = <&common_reg 0x1>; >>>>>> }; >>>>>> >>>>>> consumer@2 { >>>>>> syscon-efuse = <&common_reg 0x2>; >>>>>> }; >>>>>> >>>>>> Something like that, then regmap will take care of synchronizing access. >>>>> >>>>> Syscon is not for this. >>>>> >>>> >>>> That is how it is used today, and in 5 other ways too and there is >>>> no guidance on it. Let me know what syscon is for then. >>> >>> Like described in its bindings (syscon.yaml). The main case is: some >>> part of address space (dedicated) for various purposes. >>> >> >> That is a "simple-bus", you could use the same reasoning and make the >> whole address space one big "syscon" node instead then just poke >> registers from drivers all over. > > Yes and both are discouraged. > >> >> It is not clear where to draw the line, and for that reason I would >> like to discourage "syscon" as much as possible and use the normal DT >> scheme of node per device/register space. > > We all keep discouraging using syscon, so I agree. What exactly do you mean? >
Great, then we are in alignment. This thread started as Peter asked for the "why" and this was my explanation, basically "syscon is discouraged".
>> >>> Secondary case is a device, with its address space, which has few >>> registers from other domain, so it needs to expose these to the other >>> devices. >>> >> >> That is not the case for "reg-mux"; neither case is. So you would >> agree that "reg-mux" nodes should not be syscon nodes > > I don't understand. reg-mux is not a syscon. No syscon compatible in: > Documentation/devicetree/bindings/mux/reg-mux.yaml >
This was more a segue into a patch that does fix an instance of that I just posted here:
https://lore.kernel.org/lkml/20230516184626.154892-2-afd@ti.com/
> >> nor should >> they force their parents to be one when they do not meet the above >> two cases? > > reg-mux does not force the parent to be syscon. You are now mistaking it > with mmio-mux, which apparently for our Linux implementation it expects > parent to be syscon. >
I might have been mistaking it with "ti,phy-gmii-sel" which does force that and I am fixing here[0].
https://www.spinics.net/lists/kernel/msg4789373.html
>>> efuse is not syscon, because it is not writeable. efuse has entirely >>> different purpose with its own defined purpose/type - efuse/OTP etc. >>> >> >> That was just one example I found, I have not found a standard way >> to describe down to the bit level in DT, only to the word/register. >> Anything more granular needs non-standard ways of describing which >> bits belong to which nodes/devices and using syscon to fetch the >> common registers. >> >>>> >>>>>> >>>>> >>>>> ... >>>>> >>>>>> >>>>>> Ideally DT nodes all describe their register space in a "reg" >>>>>> property and all the "large collection of devices" spaces become >>>>>> "simple-bus" nodes. "syscon" nodes can then be limited to only the >>>>>> rare case when multiple devices share bits in a single register. >>>>>> >>>>>> If Rob and Krzysztof agree I can send a patch with the above >>>>>> guidance to the Devicetree Specification repo also. >>>>> >>>>> Agree on what? >>>>> >>>> >>>> That we should provide the above guidance on when and how to use syscon >>>> nodes. Right now it is a free for all and it is causing issues. >>> >>> Sure, providing more guidance seems good. We already provide guidance >>> via review, but we can codify it more. Where? syscon.yaml? It's already >>> describing everything needed to know... >>> >>> What particular problems do you see which need to be solved? >>> >> >> My issue is the guidance is not clear, nor being followed. For instance >> this is listed as a requirement: >> >> "The registers are not cohesive enough to represent as any specific type of device." >> >> Take "ti,j721e-system-controller" for instance, today this region is modeled >> as a "syscon" node but it actually is a region of well defined register spaces >> and devices. Like PHYs, clock controllers, and our even our pinmux controller. > > Then it should not be syscon. The platform maintainer should tell > submitter: this is not syscon, please stop this nonsense. > > We do not have access to your datasheets and we do not have time to > investigate each one of device, so we, DT maintainers, cannot really > judge. Submitters want everything to be syscon because they can write > code much faster and shove into kernel poor quality drivers which do not > adhere to any design principles. >
And that is the point of the guidance I'd like to add, it should be: "here are the only correct uses for syscon", and every other use is just hacking around making proper device nodes.
>> Most of these devices use the normal "reg" property to claim their registers and >> so this space should be a "simple-bus" but we are forced to make it one big >> "syscon" node because a couple devices in this area have a Linux driver that >> requires their parent node to be a syscon node. > > I don't think it is requirement. You could have a device which has > children, gives them regmap, but is not really syscon. >
Sure, but at least currently making a node "syscon" is the easiest way to have a parent get a regmap that can be fetched, so most do that. I have some fixes for that too..
Andrew
>> >> That is the point of this patch, to relax that restriction in this driver. >> It doesn't even change the binding, it only makes the driver match what >> the binding allows. > > Hm, we might be talking about different topics, I don't know. I did not > look at the driver as it does not fall into category of bindings at all > and is fully ignored by my filters. > > Best regards, > Krzysztof >
| ![\](/images/icornerr.gif) |