1. Patchew
  2. linux
  3. [PATCH v1 00/16] Add driver nodes for MT8195 SoC
  4. View patch

[PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node

Tinghan Shen posted 16 patches 2 years, 2 months ago
Only 14 patches received!
There is a newer version of this series
[PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by Tinghan Shen 2 years, 2 months ago 
The max clock items for the dts node with compatible
'mediatek,mt8195-smi-sub-common' should be 3.

However, the dtbs_check of such node will get following message,
arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0'] is too long
         From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml

Remove the last 'else' checking to fix this error.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 .../memory-controllers/mediatek,smi-common.yaml        | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
index a98b359bf909..e5f553e2e12a 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
@@ -143,7 +143,15 @@ allOf:
             - const: gals0
             - const: gals1
 
-    else:  # for gen2 HW that don't have gals
+  - if:  # for gen2 HW that don't have gals
+      properties:
+        compatible:
+          enum:
+            - mediatek,mt2712-smi-common
+            - mediatek,mt8167-smi-common
+            - mediatek,mt8173-smi-common
+
+    then:
       properties:
         clocks:
           minItems: 2
-- 
2.18.0
Re: [PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by Krzysztof Kozlowski 2 years, 2 months ago 
On 04/07/2022 12:00, Tinghan Shen wrote:
> The max clock items for the dts node with compatible
> 'mediatek,mt8195-smi-sub-common' should be 3.
> 
> However, the dtbs_check of such node will get following message,
> arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0'] is too long
>          From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> 
> Remove the last 'else' checking to fix this error.

Missing fixes tag.

> 
> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> ---
>  .../memory-controllers/mediatek,smi-common.yaml        | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> index a98b359bf909..e5f553e2e12a 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> @@ -143,7 +143,15 @@ allOf:
>              - const: gals0
>              - const: gals1
>  
> -    else:  # for gen2 HW that don't have gals
> +  - if:  # for gen2 HW that don't have gals
> +      properties:
> +        compatible:
> +          enum:
> +            - mediatek,mt2712-smi-common
> +            - mediatek,mt8167-smi-common
> +            - mediatek,mt8173-smi-common
> +

Without looking at the code, it's impossible to understand what you are
doing here. The commit msg says one, but you are doing something else.

Write commit msg explaining what you want to achieve and what you are doing.


Best regards,
Krzysztof
Re: [PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by Matthias Brugger 2 years, 2 months ago 

On 04/07/2022 14:36, Krzysztof Kozlowski wrote:
> On 04/07/2022 12:00, Tinghan Shen wrote:
>> The max clock items for the dts node with compatible
>> 'mediatek,mt8195-smi-sub-common' should be 3.
>>
>> However, the dtbs_check of such node will get following message,
>> arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0'] is too long
>>           From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>>
>> Remove the last 'else' checking to fix this error.
> 
> Missing fixes tag.
> 

 From my understanding, fixes tags are for patches that fix bugs (hw is not 
working etc) and not a warning message from dtbs_check. So my point of view 
would be to not add a fixes tag here.

Regards,
Matthias

>>
>> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
>> ---
>>   .../memory-controllers/mediatek,smi-common.yaml        | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>> index a98b359bf909..e5f553e2e12a 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>> @@ -143,7 +143,15 @@ allOf:
>>               - const: gals0
>>               - const: gals1
>>   
>> -    else:  # for gen2 HW that don't have gals
>> +  - if:  # for gen2 HW that don't have gals
>> +      properties:
>> +        compatible:
>> +          enum:
>> +            - mediatek,mt2712-smi-common
>> +            - mediatek,mt8167-smi-common
>> +            - mediatek,mt8173-smi-common
>> +
> 
> Without looking at the code, it's impossible to understand what you are
> doing here. The commit msg says one, but you are doing something else.
> 
> Write commit msg explaining what you want to achieve and what you are doing.
> 
> 
> Best regards,
> Krzysztof
Re: [PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by Krzysztof Kozlowski 2 years, 2 months ago 
On 06/07/2022 15:48, Matthias Brugger wrote:
> 
> 
> On 04/07/2022 14:36, Krzysztof Kozlowski wrote:
>> On 04/07/2022 12:00, Tinghan Shen wrote:
>>> The max clock items for the dts node with compatible
>>> 'mediatek,mt8195-smi-sub-common' should be 3.
>>>
>>> However, the dtbs_check of such node will get following message,
>>> arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0'] is too long
>>>           From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>>>
>>> Remove the last 'else' checking to fix this error.
>>
>> Missing fixes tag.
>>
> 
>  From my understanding, fixes tags are for patches that fix bugs (hw is not 
> working etc) and not a warning message from dtbs_check. So my point of view 
> would be to not add a fixes tag here.

Not conforming to bindings is also a bug. Missing properties or wrong
properties, even if hardware is working, is still a bug. If such bug is
not visible now in Linux, might be visible later in the future or
visible in different OS (DTS are used by other systems and pieces of
software like bootloaders). Limiting this only to Linux and to current
version (hardware still works) is OK for Linux drivers, but not for DTS.

Therefore Fixes tag in general is applicable. Of course maybe to this
one not really, maybe this is too trivial, or whatever, so I do not
insist. But I insist on the principle - reasonable dtbs_check warnings
are like compiler warnings - bugs which have to be fixed.


Best regards,
Krzysztof
Re: [PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by Matthias Brugger 2 years, 2 months ago 

On 06/07/2022 16:38, Krzysztof Kozlowski wrote:
> On 06/07/2022 15:48, Matthias Brugger wrote:
>>
>>
>> On 04/07/2022 14:36, Krzysztof Kozlowski wrote:
>>> On 04/07/2022 12:00, Tinghan Shen wrote:
>>>> The max clock items for the dts node with compatible
>>>> 'mediatek,mt8195-smi-sub-common' should be 3.
>>>>
>>>> However, the dtbs_check of such node will get following message,
>>>> arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0'] is too long
>>>>            From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>>>>
>>>> Remove the last 'else' checking to fix this error.
>>>
>>> Missing fixes tag.
>>>
>>
>>   From my understanding, fixes tags are for patches that fix bugs (hw is not
>> working etc) and not a warning message from dtbs_check. So my point of view
>> would be to not add a fixes tag here.
> 
> Not conforming to bindings is also a bug. Missing properties or wrong
> properties, even if hardware is working, is still a bug. If such bug is
> not visible now in Linux, might be visible later in the future or
> visible in different OS (DTS are used by other systems and pieces of
> software like bootloaders). Limiting this only to Linux and to current
> version (hardware still works) is OK for Linux drivers, but not for DTS.
> 

If a wrong DTS breaks software, then it's worth a fixes tag, especially for the 
DTS part, we could argue about the bindings part, but in that case it would be 
probably OK.

> Therefore Fixes tag in general is applicable. Of course maybe to this
> one not really, maybe this is too trivial, or whatever, so I do not
> insist. But I insist on the principle - reasonable dtbs_check warnings
> are like compiler warnings - bugs which have to be fixed.
> 

I'm not arguing that these things shouldn't be fixed, but that they are worth 
being back-ported to the stable branches.

Regards,
Matthias
Re: [PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by Tinghan Shen 2 years, 2 months ago 
On Mon, 2022-07-04 at 14:36 +0200, Krzysztof Kozlowski wrote:
> On 04/07/2022 12:00, Tinghan Shen wrote:
> > The max clock items for the dts node with compatible
> > 'mediatek,mt8195-smi-sub-common' should be 3.
> > 
> > However, the dtbs_check of such node will get following message,
> > arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0']
> > is too long
> >          From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
> > common.yaml
> > 
> > Remove the last 'else' checking to fix this error.
> 
> Missing fixes tag.
> 
> > 
> > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > ---
> >  .../memory-controllers/mediatek,smi-common.yaml        | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > index a98b359bf909..e5f553e2e12a 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > @@ -143,7 +143,15 @@ allOf:
> >              - const: gals0
> >              - const: gals1
> >  
> > -    else:  # for gen2 HW that don't have gals
> > +  - if:  # for gen2 HW that don't have gals
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - mediatek,mt2712-smi-common
> > +            - mediatek,mt8167-smi-common
> > +            - mediatek,mt8173-smi-common
> > +
> 
> Without looking at the code, it's impossible to understand what you are
> doing here. The commit msg says one, but you are doing something else.
> 
> Write commit msg explaining what you want to achieve and what you are doing.
> 
> 
> Best regards,
> Krzysztof

Ok, I'll update in next version.

Thanks,
TingHan
Re: [PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by AngeloGioacchino Del Regno 2 years, 2 months ago 
Il 04/07/22 12:00, Tinghan Shen ha scritto:
> The max clock items for the dts node with compatible
> 'mediatek,mt8195-smi-sub-common' should be 3.
> 
> However, the dtbs_check of such node will get following message,
> arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0'] is too long
>           From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> 
> Remove the last 'else' checking to fix this error.
> 
> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> ---
>   .../memory-controllers/mediatek,smi-common.yaml        | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> index a98b359bf909..e5f553e2e12a 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> @@ -143,7 +143,15 @@ allOf:
>               - const: gals0
>               - const: gals1
>   
> -    else:  # for gen2 HW that don't have gals
> +  - if:  # for gen2 HW that don't have gals
> +      properties:
> +        compatible:
> +          enum:
> +            - mediatek,mt2712-smi-common

MT6795 also doesn't have any GALS, please add it in here.

Regards,
Angelo

> +            - mediatek,mt8167-smi-common
> +            - mediatek,mt8173-smi-common
> +
> +    then:
>         properties:
>           clocks:
>             minItems: 2
Re: [PATCH v1 02/16] dt-bindings: memory: mediatek: Update condition for mt8195 smi node
Posted by Tinghan Shen 2 years, 2 months ago 
On Mon, 2022-07-04 at 12:25 +0200, AngeloGioacchino Del Regno wrote:
> Il 04/07/22 12:00, Tinghan Shen ha scritto:
> > The max clock items for the dts node with compatible
> > 'mediatek,mt8195-smi-sub-common' should be 3.
> > 
> > However, the dtbs_check of such node will get following message,
> > arch/arm64/boot/dts/mediatek/mt8195-evb.dtb: smi@14010000: clock-names: ['apb', 'smi', 'gals0']
> > is too long
> >           From schema: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-
> > common.yaml
> > 
> > Remove the last 'else' checking to fix this error.
> > 
> > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > ---
> >   .../memory-controllers/mediatek,smi-common.yaml        | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > index a98b359bf909..e5f553e2e12a 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > @@ -143,7 +143,15 @@ allOf:
> >               - const: gals0
> >               - const: gals1
> >   
> > -    else:  # for gen2 HW that don't have gals
> > +  - if:  # for gen2 HW that don't have gals
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - mediatek,mt2712-smi-common
> 
> MT6795 also doesn't have any GALS, please add it in here.

Ok, I'll update it in next version.

Thanks,
TingHan

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.