| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7e293d04-da1c-461c-b58a-18c1b942193b@kernel.org>
Date: Mon, 2 Jun 2025 10:13:39 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
Felix Fietkau <nbd@....name>, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] dt-bindings: clock: airoha: Document support for
AN7583 clock
On 30/05/2025 17:26, Christian Marangi wrote:
> On Thu, May 29, 2025 at 11:00:39AM +0200, Krzysztof Kozlowski wrote:
>> On 28/05/2025 14:57, Christian Marangi wrote:
>>>>> Again sorry if this question keeps coming around and I can totally
>>>>> understand if you are getting annoyed by this. The reason I always ask
>>>>> this is because it's a total PAIN to implement this with the driver
>>>>> structure due to the old "simple-mfd" model.
>>>>
>>>> ... and Rob was saying multiple times: be careful when adding
>>>> simple-mfd. If it bites back, then I am sorry, but everyone were warned,
>>>> weren't they?
>>>>
>>>> What is exactly the pain anyway? You cannot instantiate children from
>>>> SCU driver?
>>>>
>>>
>>> Answering below since they are related.
>>>
>>>>>
>>>>> (as again putting everything in a single node conflicts with the OF
>>>>> principle of autoprobing stuff with compatible property)
>>>>
>>>> I am not sure if I follow. What principle? Where is this principle
>>>> expressed?
>>>>
>>>> And you do not have in your second example additional compatibles, so
>>>> even if such principle exists it is not broken: everything autoprobes, I
>>>> think.
>>>>
>>>>>
>>>>
>>>>
>>>
>>> The principle I'm talking about is one driver for one compatible.
>>
>> There is no such principle. One compatible can map to many drivers and
>> many compatibles can map to one driver.
>>
>
> I might be wrong but there is currently a limitation on the OF system
> where if a driver gets probed for a node then it's ignored for any other
> driver. (sorry for the bad explaination, hope it's understandable)
Yes but this can be changed easily. See: depopulate. Whether you
populate or not-populate is not a reason to model hardware description
one way or another.
>
>>> (to be more precise excluding syscon compatible that is actually
>>> ignored, if a driver for the compatible is found, any other compatible
>>> is ignored.)
>>>
>>> This means that declaring multiple compatible as:
>>>
>>> compatible = "airoha,clock", "airoha,mdio"
>>>
>>> doesn't result in the clock driver and the mdio driver probed but only
>>> one of the 2 (probably only clock since it does have priority)
>>
>> I don't understand this example. It makes no sense - clock is not
>> compatible with mdio.
>>
>
> This was an example to put every properties in the oparent node.
So it was a bad example because it makes no sense. You move the
properties, not merge compatibles!
>
>>>
>>> The "simple-mfd" compatible is just a simple compatible that indicate to
>>> the OF system that every child (with a compatible) should be also probed.
>>> And then automagically the driver gets probed.
>>>
>>> Now the ""PAIN"" explaination. Not using the "simple-mfd" way with the
>>> child with compatible and putting everything in the node means having to
>>> create a dedicated MFD driver that just instruct to manually probe the
>>> clock and mdio driver. (cause the compatible system can't be used)
>>
>> You already have that driver - SCU. No need for new MFD driver...
>>
>
> The SCU driver is actually the clock driver (currently). This was done
> for simplicity and because in SCU there were only some bits.
>
> But now with AN7583 they put 2 MDIO controller in it.
>
>>
>>>
>>> So it's 3 driver instead of 2 with the extra effort of MFD driver
>>> maintainer saying "Why simple-mfd is not used?"
>>
>> Sorry, that's a wrong argument. You can use simple-mfd, iff it follows
>> standard practices. If it does not fit standard practices, you cannot
>> use an argument "now I need more complicated solution".
>>
>
> Then I think we are getting confused because I am following the usual
> pattern.
>
> This is what would be the ideal and easy solution. (ti what was done on
> EN7581 with pinctrl and pwm)
>
> scu: system-controller@...20000 {
> compatible = "syscon", "simple-mfd";
> reg = <0x0 0x1fb00000 0x0 0x970>;
>
> scuclk: scuclk {
> compatible = "airoha,an7583-scu";
> #clock-cells = <1>;
> #reset-cells = <1>;
> };
>
> mdio {
> compatible = "airoha,an7583-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
>
> mdio_0: bus@0 {
> reg = <0>;
> resets = <&scuclk AN7583_MDIO0>;
> };
>
> mdio_1: bus@1 {
> reg = <1>;
> resets = <&scuclk AN7583_MDIO1>;
> };
> };
> };
>
>
By repeating the same you will not get different answers but rather me
become annoyed.
>
>>>
>>>
>>> There is a solution for this but I always feel it's more of a workaround
>>> since it doesn't really describe the HW with the DT node.
>>
>> Really? All arguments you used here are driver arguments - that
>> something is a pain in drivers. Now you mention that hardware would not
>> match description.
>>
>> Then let's change entire talk towards hardware description and send
>> patches matching hardware, not matching your MFD driver structure.
>>
>
> Ok to describe the HW for this register block
>
> SCU register:
What is here?
> - clock
Here are clocks, but what is in "SCU register"?
> - mdio controller 1
> - mdio controller 2
>
> So this is why I think a good matching node block is:
>
> parent node (SCU register):
So what is here?
> - child 1 (clock)
> - child 2 (mdio controller)
> - child 1 (mdio bus 1)
> - child 2 (mdio bus 2)
>
> Is it wrong to model the DT node this way?
Yes and I explained you already why.
>
>>>
>>> The workaround is:
>>>
>>> system-controller@...20000 {
>>> /* The parent SCU node implement the clock driver */
>>> compatible = "airoha,an7583-scu", "syscon";
>>> reg = <0x0 0x1fb00000 0x0 0x970>;
>>>
>>> #clock-cells = <1>;
>>> #reset-cells = <1>;
>>>
>>> /* Clock driver is instructed to probe child */
>>> mdio {
>>> compatible = "airoha,an7583-mdio";
>>
>> Again, drop compatible.
>>
>
> To drop the compatible a dedicated MFD driver is needed (or adding code
> in the clock driver to register the MDIO controller and that is net
> clean code wise)
If you need to do some driver changes, do some driver changes...
Best regards,
Krzysztof
Powered by blists - more mailing lists