| Message ID | 20240108135421.684263-7-tmaimon77@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
On 08/01/2024 14:54, Tomer Maimon wrote: > Replace reg with syscon property since the clock registers handle the > reset registers as well. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > .../bindings/clock/nuvoton,npcm845-clk.yaml | 22 +++++++++---------- > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > index 0b642bfce292..c6bf05c163b4 100644 > --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > @@ -18,8 +18,9 @@ properties: > enum: > - nuvoton,npcm845-clk > > - reg: > - maxItems: 1 > + nuvoton,sysclk: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: phandle to access clock registers. NAK. Not explained, not justified, not reasonable, breaking ABI. Best regards, Krzysztof
Hi Krzysztof, As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers mail. In the NPCM8XX SoC, the reset and the clock register modules are scrambled in the same memory register region. The NPCM8XX Clock driver is still in the upstream process (for a long time) but the NPCM8XX reset driver is already upstreamed. On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 08/01/2024 14:54, Tomer Maimon wrote: > > Replace reg with syscon property since the clock registers handle the > > reset registers as well. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > .../bindings/clock/nuvoton,npcm845-clk.yaml | 22 +++++++++---------- > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > > index 0b642bfce292..c6bf05c163b4 100644 > > --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > > +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > > @@ -18,8 +18,9 @@ properties: > > enum: > > - nuvoton,npcm845-clk > > > > - reg: > > - maxItems: 1 > > + nuvoton,sysclk: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: phandle to access clock registers. > > NAK. Not explained, not justified, not reasonable, breaking ABI. Should I explain more in the commit message or/and the nuvoton,sysclk property? > > Best regards, > Krzysztof > Best regards, Tomer
On 16/01/2024 20:37, Tomer Maimon wrote: > Hi Krzysztof, > > As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add > binding for clock and reset registers mail. > > In the NPCM8XX SoC, the reset and the clock register modules are > scrambled in the same memory register region. > The NPCM8XX Clock driver is still in the upstream process (for a long > time) but the NPCM8XX reset driver is already upstreamed. First of all, the drivers itself don't matter here, we talk about bindings. I assume though they were going together, so that's why you mentioned driver... but just to clarify: we talk here only about drivers. If reset bindings were accepted, then why they aren't referenced? If clock bindings were not accepted, then what is this patch and this file about? > > On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 08/01/2024 14:54, Tomer Maimon wrote: >>> Replace reg with syscon property since the clock registers handle the >>> reset registers as well. >>> >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> >>> --- >>> .../bindings/clock/nuvoton,npcm845-clk.yaml | 22 +++++++++---------- >>> 1 file changed, 10 insertions(+), 12 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml >>> index 0b642bfce292..c6bf05c163b4 100644 >>> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml >>> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml >>> @@ -18,8 +18,9 @@ properties: >>> enum: >>> - nuvoton,npcm845-clk >>> >>> - reg: >>> - maxItems: 1 >>> + nuvoton,sysclk: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: phandle to access clock registers. >> >> NAK. Not explained, not justified, not reasonable, breaking ABI. > Should I explain more in the commit message or/and the nuvoton,sysclk property? Let's try to explain here first. I really do not understand why do you change this binding. Your device did not change, so your binding should not. Now, if you say "but I changed drivers", then it does not matter. Bindings do not change because you did something in the drivers, in general. At least they should not. Best regards, Krzysztof
Hi Krzysztof, Thanks for your comments. On Tue, 16 Jan 2024 at 22:40, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 16/01/2024 20:37, Tomer Maimon wrote: > > Hi Krzysztof, > > > > As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add > > binding for clock and reset registers mail. > > > > In the NPCM8XX SoC, the reset and the clock register modules are > > scrambled in the same memory register region. > > The NPCM8XX Clock driver is still in the upstream process (for a long > > time) but the NPCM8XX reset driver is already upstreamed. > > First of all, the drivers itself don't matter here, we talk about > bindings. I assume though they were going together, so that's why you > mentioned driver... but just to clarify: we talk here only about drivers. > > If reset bindings were accepted, then why they aren't referenced? > > If clock bindings were not accepted, then what is this patch and this > file about? > > > > > On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 08/01/2024 14:54, Tomer Maimon wrote: > >>> Replace reg with syscon property since the clock registers handle the > >>> reset registers as well. > >>> > >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > >>> --- > >>> .../bindings/clock/nuvoton,npcm845-clk.yaml | 22 +++++++++---------- > >>> 1 file changed, 10 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > >>> index 0b642bfce292..c6bf05c163b4 100644 > >>> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > >>> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml > >>> @@ -18,8 +18,9 @@ properties: > >>> enum: > >>> - nuvoton,npcm845-clk > >>> > >>> - reg: > >>> - maxItems: 1 > >>> + nuvoton,sysclk: > >>> + $ref: /schemas/types.yaml#/definitions/phandle > >>> + description: phandle to access clock registers. > >> > >> NAK. Not explained, not justified, not reasonable, breaking ABI. > > Should I explain more in the commit message or/and the nuvoton,sysclk property? > > Let's try to explain here first. I really do not understand why do you > change this binding. Your device did not change, so your binding should > not. Now, if you say "but I changed drivers", then it does not matter. > Bindings do not change because you did something in the drivers, in > general. At least they should not. The confusion here is because the clock binding was upstreamed but the clock driver has not upstreamed yet. The clock driver will use regmap and ioremap so reg property is not needed, should I remove it? or leave it? > > Best regards, > Krzysztof > Best regards, Tomer
On 22/01/2024 18:26, Tomer Maimon wrote: > Hi Krzysztof, > > Thanks for your comments. > > On Tue, 16 Jan 2024 at 22:40, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 16/01/2024 20:37, Tomer Maimon wrote: >>> Hi Krzysztof, >>> >>> As explained in my [PATCH v22 4/8] dt-bindings: soc: nuvoton: add >>> binding for clock and reset registers mail. >>> >>> In the NPCM8XX SoC, the reset and the clock register modules are >>> scrambled in the same memory register region. >>> The NPCM8XX Clock driver is still in the upstream process (for a long >>> time) but the NPCM8XX reset driver is already upstreamed. >> >> First of all, the drivers itself don't matter here, we talk about >> bindings. I assume though they were going together, so that's why you >> mentioned driver... but just to clarify: we talk here only about drivers. >> >> If reset bindings were accepted, then why they aren't referenced? >> >> If clock bindings were not accepted, then what is this patch and this >> file about? >> >>> >>> On Wed, 10 Jan 2024 at 22:59, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 08/01/2024 14:54, Tomer Maimon wrote: >>>>> Replace reg with syscon property since the clock registers handle the >>>>> reset registers as well. >>>>> >>>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> >>>>> --- >>>>> .../bindings/clock/nuvoton,npcm845-clk.yaml | 22 +++++++++---------- >>>>> 1 file changed, 10 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml >>>>> index 0b642bfce292..c6bf05c163b4 100644 >>>>> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml >>>>> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml >>>>> @@ -18,8 +18,9 @@ properties: >>>>> enum: >>>>> - nuvoton,npcm845-clk >>>>> >>>>> - reg: >>>>> - maxItems: 1 >>>>> + nuvoton,sysclk: >>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>>> + description: phandle to access clock registers. >>>> >>>> NAK. Not explained, not justified, not reasonable, breaking ABI. >>> Should I explain more in the commit message or/and the nuvoton,sysclk property? >> >> Let's try to explain here first. I really do not understand why do you >> change this binding. Your device did not change, so your binding should >> not. Now, if you say "but I changed drivers", then it does not matter. >> Bindings do not change because you did something in the drivers, in >> general. At least they should not. > The confusion here is because the clock binding was upstreamed but the > clock driver has not upstreamed yet. So what does it mean? Being upstreamed independently does not mean something is wrong, so I do not see any confusion. There is absolutely no confusion in upstreaming binding before upstreaming driver. > The clock driver will use regmap and ioremap so reg property is not > needed, should I remove it? or leave it? Just because you upstream drivers? You should not change bindings just because you figured out that you will implement something that or other way. Please describe the hardware in the binding, not the driver. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml index 0b642bfce292..c6bf05c163b4 100644 --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml @@ -18,8 +18,9 @@ properties: enum: - nuvoton,npcm845-clk - reg: - maxItems: 1 + nuvoton,sysclk: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle to access clock registers. clocks: items: @@ -37,7 +38,7 @@ properties: required: - compatible - - reg + - nuvoton,sysclk - clocks - clock-names - '#clock-cells' @@ -52,14 +53,11 @@ examples: clock-frequency = <25000000>; }; - ahb { - #address-cells = <2>; - #size-cells = <2>; - - clock-controller@f0801000 { - compatible = "nuvoton,npcm845-clk"; - reg = <0x0 0xf0801000 0x0 0x1000>; - #clock-cells = <1>; - }; + clk: clock-controller { + compatible = "nuvoton,npcm845-clk"; + nuvoton,sysclk = <&clk_rst>; + #clock-cells = <1>; + clocks = <&refclk>; + clock-names = "refclk"; }; ...
Replace reg with syscon property since the clock registers handle the reset registers as well. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- .../bindings/clock/nuvoton,npcm845-clk.yaml | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-)