2022-04-07 01:14:43

by Mikhail Zhilkin

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

Add YAML binding for Sercomm partition parser.

Signed-off-by: Mikhail Zhilkin <[email protected]>
---
.../mtd/partitions/sercomm,sc-partitions.yaml | 70 +++++++++++++++++++
1 file changed, 70 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml

diff --git a/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
new file mode 100644
index 000000000000..07ea5596200c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/partitions/sercomm,sc-partitions.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sercomm Partitions
+
+description: |
+ Sercomm is one of hardware manufacturers providing SoCs used in home routers.
+ The Sercomm partition map table contains information about non-standard
+ partition offsets and sizes (depending on the bad blocks presence and their
+ locations). Partition map is used by many Sercomm-based Ralink devices
+ (e.g. Beeline, Netgear).
+
+maintainers:
+ - Mikhail Zhilkin <[email protected]>
+
+properties:
+ compatible:
+ const: sercomm,sc-partitions
+
+ scpart-id:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Partition id in Sercomm partition map
+
+required:
+ - compatible
+ - scpart-id
+
+additionalProperties: false
+
+examples:
+ - |
+ partitions
+ compatible = "sercomm,sc-partitions", "fixed-partitions";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ partition@0 {
+ label = "u-boot";
+ reg = <0x0 0x100000>;
+ scpart-id = <0>;
+ read-only;
+ };
+
+ partition@100000 {
+ label = "dynamic partition map";
+ reg = <0x100000 0x100000>;
+ scpart-id = <1>;
+ };
+
+ factory: partition@200000 {
+ label = "Factory";
+ reg = <0x200000 0x100000>;
+ scpart-id = <2>;
+ read-only;
+
+ compatible = "nvmem-cells";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ macaddr_factory_21000: macaddr@21000 {
+ reg = <0x21000 0x6>;
+ };
+ };
+
+ /* ... */
+
+ };
--
2.25.1


2022-04-07 20:51:31

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On Wed, 06 Apr 2022 19:59:46 +0000, Mikhail Zhilkin wrote:
> Add YAML binding for Sercomm partition parser.
>
> Signed-off-by: Mikhail Zhilkin <[email protected]>
> ---
> .../mtd/partitions/sercomm,sc-partitions.yaml | 70 +++++++++++++++++++
> 1 file changed, 70 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.example.dts:21.13-14 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1401: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

2022-04-07 21:20:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 06/04/2022 21:59, Mikhail Zhilkin wrote:
> Add YAML binding for Sercomm partition parser.
>
> Signed-off-by: Mikhail Zhilkin <[email protected]>
> ---
> .../mtd/partitions/sercomm,sc-partitions.yaml | 70 +++++++++++++++++++
> 1 file changed, 70 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
>

(...)


> +
> +properties:
> + compatible:
> + const: sercomm,sc-partitions
> +
> + scpart-id:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Partition id in Sercomm partition map

Do you really need it? The reg should define the order, unless you
expect some incomplete partition list?

In any case this requires vendor prefix.

> +
> +required:
> + - compatible

Missing reg.

> + - scpart-id
> +
> +additionalProperties: false

Are you sure that you tested your bindings? You miss here address/size
cells and children, so you should have big fat warning.

Plus your DTS example has error and does not compile...

Best regards,
Krzysztof

2022-04-10 10:49:08

by Mikhail Zhilkin

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

Hello Rob,

On 4/7/2022 4:50 PM, Rob Herring wrote:
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit.

Thanks for your great explanation how to test! I found and fixed a mistake.
How I have only one WARNING:
"added, moved or deleted file(s), does MAINTAINERS need updating?"

I hope it doesn't require additional change. What do you think?


Best regards,
Mikhail

2022-04-11 04:37:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 09/04/2022 14:26, Mikhail Zhilkin wrote:
>>
>> In any case this requires vendor prefix.
>
> I'm not sure that "scpart-id" is necessary here. "sercomm,sc-partitions"
> is necessary. I'm going to add vendor prefix in a separate patch. Is this
> ok?

Yes.

>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 01430973ecec..65ff22364fb3 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1082,6 +1082,8 @@ patternProperties:
>      description: Sensirion AG
>    "^sensortek,.*":
>      description: Sensortek Technology Corporation
> +  "^sercomm,.*":
> +    description: Sercomm (Suzhou) Corporation
>    "^sff,.*":
>      description: Small Form Factor Committee
>    "^sgd,.*":
> --
>
>>> +
>>> +required:
>>> + - compatible
>> Missing reg.
>
> reg isn't required. Parser can read partition offsets and sizes from
> SC PART MAP table. Or do you mean something else?  All is ok
> without reg definition in "Example" (except the warns that reg property
> is missing).

reg might not be required for current implementation but it is required
by devicetree for every node with unit address. Do you expect here nodes
without unit addresses?

>> Are you sure that you tested your bindings? You miss here address/size
>> cells and children, so you should have big fat warning.
>>
>> Plus your DTS example has error and does not compile...
>
> Whole dts, for the real device (not for example), was tested many times.

Yeah, I did not speak about whole DTS, but bindings and example in the
bindings.

> Thank you for your feedback! I checked the another examples and there
> are no any warnings now. But I'm not yet sure that "properties" and
> "required" are correct.
> What do you think (or what else I have to read / check)?

There is no way you tested the bindings. There are for sure warnings
because it simply cannot be even compiled. The writing-schema.rst
describes how to test it.

Best regards,
Krzysztof

2022-04-11 20:07:59

by Mikhail Zhilkin

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

Hello Krzysztof,

On 4/7/2022 10:48 AM, Krzysztof Kozlowski wrote:

(...)

>> +properties:
>> + compatible:
>> + const: sercomm,sc-partitions
>> +
>> + scpart-id:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description: Partition id in Sercomm partition map
> Do you really need it? The reg should define the order, unless you
> expect some incomplete partition list?
>
> In any case this requires vendor prefix.

I'm not sure that "scpart-id" is necessary here. "sercomm,sc-partitions"
is necessary. I'm going to add vendor prefix in a separate patch. Is this
ok?

---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 01430973ecec..65ff22364fb3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1082,6 +1082,8 @@ patternProperties:
     description: Sensirion AG
   "^sensortek,.*":
     description: Sensortek Technology Corporation
+  "^sercomm,.*":
+    description: Sercomm (Suzhou) Corporation
   "^sff,.*":
     description: Small Form Factor Committee
   "^sgd,.*":
--

>> +
>> +required:
>> + - compatible
> Missing reg.

reg isn't required. Parser can read partition offsets and sizes from
SC PART MAP table. Or do you mean something else?  All is ok
without reg definition in "Example" (except the warns that reg property
is missing).

> Are you sure that you tested your bindings? You miss here address/size
> cells and children, so you should have big fat warning.
>
> Plus your DTS example has error and does not compile...

Whole dts, for the real device (not for example), was tested many times.
Thank you for your feedback! I checked the another examples and there
are no any warnings now. But I'm not yet sure that "properties" and
"required" are correct.
What do you think (or what else I have to read / check)?

---
 .../mtd/partitions/sercomm,sc-partitions.yaml | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml

diff --git
a/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
b/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
new file mode 100644
index 000000000000..cb171a0383aa
--- /dev/null
+++
b/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id:
http://devicetree.org/schemas/mtd/partitions/sercomm,sc-partitions.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sercomm Partitions
+
+description: |
+  Sercomm is one of hardware manufacturers providing SoCs used in home
routers.
+  The Sercomm partition map table contains information about non-standard
+  partition offsets and sizes (depending on the bad blocks presence and
their
+  locations). Partition map is used by many Sercomm-based Ralink devices
+  (e.g. Beeline, Netgear).
+
+maintainers:
+  - Mikhail Zhilkin <[email protected]>
+
+properties:
+  compatible:
+    const: sercomm,sc-partitions
+
+  "#address-cells": true
+
+  "#size-cells": true
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: true
+
+examples:
+  - |
+    partitions {
+        compatible = "sercomm,sc-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@0 {
+            label = "u-boot";
+            reg = <0x0 0x100000>;
+            scpart-id = <0>;
+            read-only;
+        };
+
+        partition@100000 {
+            label = "dynamic partition map";
+            reg = <0x100000 0x100000>;
+            scpart-id = <1>;
+        };
+
+        factory: partition@200000 {
+            label = "Factory";
+            reg = <0x200000 0x100000>;
+            scpart-id = <2>;
+            read-only;
+
+            compatible = "nvmem-cells";
+            #address-cells = <1>;
+            #size-cells = <1>;
+
+            macaddr_factory_21000: macaddr@21000 {
+                reg = <0x21000 0x6>;
+            };
+        };
+
+        /* ... */
+
+    };
--

> Best regards,
> Krzysztof

Best regards,
Mikhail

2022-04-11 22:38:29

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 09/04/2022 20:04, Mikhail Zhilkin wrote:
> On 4/9/2022 3:43 PM, Krzysztof Kozlowski wrote:
>
>>> I'm not sure that "scpart-id" is necessary here. "sercomm,sc-partitions"
>>> is necessary. I'm going to add vendor prefix in a separate patch. Is this
>>> ok?
>> Yes.
>
> Thanks!
>
>>>>> +required:
>>>>> + - compatible
>>>> Missing reg.
>>> reg isn't required. Parser can read partition offsets and sizes from
>>> SC PART MAP table. Or do you mean something else?  All is ok
>>> without reg definition in "Example" (except the warns that reg property
>>> is missing).
>> reg might not be required for current implementation but it is required
>> by devicetree for every node with unit address. Do you expect here nodes
>> without unit addresses?
> Only "partitions" node has no unit address.

This confused me. Do you have a child node named "partitions"?

> All subnodes  have unit
> addresses and therefore have to have reg property.

So all of them need reg.

> I've just realized
> that "fixed-partitions.yaml" is almost my case. It looks like I can
> copy'n'paste  "required" and "*properties".
> Do you mind if I don't reinvent the wheel and reuse this good
> practice?

Re-using proper stuff is good, but just don't blindly copy. You need
still compatible and reg in required properties.

Best regards,
Krzysztof

2022-04-12 01:04:26

by Mikhail Zhilkin

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 4/9/2022 3:43 PM, Krzysztof Kozlowski wrote:

>> I'm not sure that "scpart-id" is necessary here. "sercomm,sc-partitions"
>> is necessary. I'm going to add vendor prefix in a separate patch. Is this
>> ok?
> Yes.

Thanks!

>>>> +required:
>>>> + - compatible
>>> Missing reg.
>> reg isn't required. Parser can read partition offsets and sizes from
>> SC PART MAP table. Or do you mean something else?  All is ok
>> without reg definition in "Example" (except the warns that reg property
>> is missing).
> reg might not be required for current implementation but it is required
> by devicetree for every node with unit address. Do you expect here nodes
> without unit addresses?
Only "partitions" node has no unit address. All subnodes  have unit
addresses and therefore have to have reg property. I've just realized
that "fixed-partitions.yaml" is almost my case. It looks like I can
copy'n'paste  "required" and "*properties".
Do you mind if I don't reinvent the wheel and reuse this good
practice?

Here's what I got (no any warnings appears):

---
 .../mtd/partitions/sercomm,sc-partitions.yaml | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml

diff --git
a/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
b/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
new file mode 100644
index 000000000000..cb171a0383aa
--- /dev/null
+++
b/Documentation/devicetree/bindings/mtd/partitions/sercomm,sc-partitions.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id:
http://devicetree.org/schemas/mtd/partitions/sercomm,sc-partitions.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sercomm Partitions
+
+description: |
+  Sercomm is one of hardware manufacturers providing SoCs used in home
routers.
+  The Sercomm partition map table contains information about non-standard
+  partition offsets and sizes (depending on the bad blocks presence and
their
+  locations). Partition map is used by many Sercomm-based Ralink devices
+  (e.g. Beeline, Netgear).
+
+maintainers:
+  - Mikhail Zhilkin <[email protected]>
+
+properties:
+  compatible:
+    const: sercomm,sc-partitions
+
+  "#address-cells": true
+
+  "#size-cells": true
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: true
+
+examples:
+  - |
+    partitions {
+        compatible = "sercomm,sc-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@0 {
+            label = "u-boot";
+            reg = <0x0 0x100000>;
+            scpart-id = <0>;
+            read-only;
+        };
+
+        partition@100000 {
+            label = "dynamic partition map";
+            reg = <0x100000 0x100000>;
+            scpart-id = <1>;
+        };
+
+        factory: partition@200000 {
+            label = "Factory";
+            reg = <0x200000 0x100000>;
+            scpart-id = <2>;
+            read-only;
+
+            compatible = "nvmem-cells";
+            #address-cells = <1>;
+            #size-cells = <1>;
+
+            macaddr_factory_21000: macaddr@21000 {
+                reg = <0x21000 0x6>;
+            };
+        };
+
+        /* ... */
+
+    };
--

(...)

> Best regards,
> Krzysztof

--
Best regards,
Mikhail

2022-04-12 03:40:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 09/04/2022 14:35, Mikhail Zhilkin wrote:
> Hello Rob,
>
> On 4/7/2022 4:50 PM, Rob Herring wrote:
>> If you already ran 'make dt_binding_check' and didn't see the above
>> error(s), then make sure 'yamllint' is installed and dt-schema is up to
>> date:
>>
>> pip3 install dtschema --upgrade
>>
>> Please check and re-submit.
>
> Thanks for your great explanation how to test! I found and fixed a mistake.

One? Apart of broken compilation, there were other mistakes. When you
run the tests, you will see all of them.

> How I have only one WARNING:
> "added, moved or deleted file(s), does MAINTAINERS need updating?"
>
> I hope it doesn't require additional change. What do you think?

This is not related to dt_binding_check. if you ask about checkpatch,
then no, this does not require fixing.

Best regards,
Krzysztof

2022-04-12 20:07:28

by Mikhail Zhilkin

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 4/9/2022 3:49 PM, Krzysztof Kozlowski wrote:

> One? Apart of broken compilation, there were other mistakes. When you
> run the tests, you will see all of them.

I checked the first version again. It was:
- One "FATAL ERROR" (missing '{' in dts example)
- 5 warning / errors (severity not specified) "From schema"

Fixed version is here (not a single warning):
https://lore.kernel.org/linux-mtd/[email protected]/T/#ma43afb59fd1f0fab8899951005ae9ce011fbb0cc

Is it ok if I send it in PATCH v3?


> This is not related to dt_binding_check. if you ask about checkpatch,
> then no, this does not require fixing.


Yeah, it's about checkpatch. Thanks.

> Best regards,
> Krzysztof


Best regards,
Mikhail

2022-04-29 15:19:51

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 28/04/2022 17:24, Mikhail Zhilkin wrote:
>
> Hi, Krzysztof,
>
> On 4/10/2022 11:18 AM, Krzysztof Kozlowski wrote:
>> I am sorry, but you changed now a lot in the bindings and it looks
>> entirely different. Things previously being correct now are wrong, so
>> rather start from your old bindings...
>
>
> Looks like I'm a bit confused... I use dual "compatible" in my real dts
> and I realized that:
>
> 1. Therefore I have to use  dual "compatible" in example too:
>
> compatible = "sercomm,sc-partitions", "fixed-partitions";
>
> 2. When I'm trying to reuse "fixed-partitions" compatible from
> fixed-partitions.yaml in my new .yaml I get "too long" errors.

Yes, the fixed-partitions.yaml would have to be changed to allow extension.

>
> Real dts:
>
> Link:
> https://github.com/openwrt/openwrt/blob/edcc1a9a734bb3fcdc9242025290d3f173e71b78/target/linux/ramips/dts/mt7621_beeline_smartbox-giga.dts#L79
>
> So, I currently found another solution - to extend fixed-partitions.yaml
> with "sercomm,sc-partitions". Is It ok from your side? Can I use this
> code in v3?

Not really, I don't understand why do you need it and it does not
include our previous talks.

>
> diff --git
> a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> index ea4cace6a955..9eebe39a57fb 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> @@ -19,7 +19,11 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: fixed-partitions
> +    oneOf:
> +      - const: fixed-partitions
> +      - items:
> +          - const: sercomm,sc-partitions
> +          - const: fixed-partitions
>  
>    "#address-cells": true
>  
> @@ -27,7 +31,18 @@ properties:
>  
>  patternProperties:
>    "@[0-9a-f]+$":
> -    $ref: "partition.yaml#"
> +    allOf:
> +      - $ref: "partition.yaml#"
> +      - if:
> +          properties:
> +            compatible:
> +              contains:
> +                const: sercomm,sc-partitions
> +        then:
> +          properties:
> +            scpart-id:

It still misses vendor prefix and we agreed you don't need it, didn't we?


Best regards,
Krzysztof

2022-04-30 15:38:14

by Mikhail Zhilkin

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 4/29/2022 9:46 AM, Krzysztof Kozlowski wrote:

>>> I am sorry, but you changed now a lot in the bindings and it looks
>>> entirely different. Things previously being correct now are wrong, so
>>> rather start from your old bindings...
>>
>> Looks like I'm a bit confused... I use dual "compatible" in my real dts
>> and I realized that:
>>
>> 1. Therefore I have to use  dual "compatible" in example too:
>>
>> compatible = "sercomm,sc-partitions", "fixed-partitions";
>>
>> 2. When I'm trying to reuse "fixed-partitions" compatible from
>> fixed-partitions.yaml in my new .yaml I get "too long" errors.
> Yes, the fixed-partitions.yaml would have to be changed to allow extension.

Well.

>> Real dts:
>>
>> Link:
>> https://github.com/openwrt/openwrt/blob/edcc1a9a734bb3fcdc9242025290d3f173e71b78/target/linux/ramips/dts/mt7621_beeline_smartbox-giga.dts#L79
>>
>> So, I currently found another solution - to extend fixed-partitions.yaml
>> with "sercomm,sc-partitions". Is It ok from your side? Can I use this
>> code in v3?
> Not really, I don't understand why do you need it

The main idea is keeping original Sercomm firmware behavior:

1. If dynamic partition map found then use offsets and mtd sizes stored
in partition map. It's provided by "sercomm,sc-partitions" compatible.

2. If dynamic partition map doesn't exist or broken then default values
(from dts) are used. It's provided by "fixed-partitions" compatible.

> and it does not
> include our previous talks.

At the time, I didn't realize how important is it. Understanding began
to come after dozens of experiments and checking the similar Linux patches.

>> diff --git
>> a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>> b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>> index ea4cace6a955..9eebe39a57fb 100644
>> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>> @@ -19,7 +19,11 @@ maintainers:
>>  
>>  properties:
>>    compatible:
>> -    const: fixed-partitions
>> +    oneOf:
>> +      - const: fixed-partitions
>> +      - items:
>> +          - const: sercomm,sc-partitions
>> +          - const: fixed-partitions
>>  
>>    "#address-cells": true
>>  
>> @@ -27,7 +31,18 @@ properties:
>>  
>>  patternProperties:
>>    "@[0-9a-f]+$":
>> -    $ref: "partition.yaml#"
>> +    allOf:
>> +      - $ref: "partition.yaml#"
>> +      - if:
>> +          properties:
>> +            compatible:
>> +              contains:
>> +                const: sercomm,sc-partitions
>> +        then:
>> +          properties:
>> +            scpart-id:
> It still misses vendor prefix and we agreed you don't need it, didn't we?

Do you mean "sercomm" vendor prefix? If so then we agreed that I include
it in a separate patch:

Link:
https://lore.kernel.org/linux-mtd/[email protected]/

I'm going to send it in v3:

---
dt-bindings: Add Sercomm (Suzhou) Corporation vendor prefix

Update Documentation/devicetree/bindings/vendor-prefixes.yaml to include
"sercomm" as a vendor prefix for "Sercomm (Suzhou) Corporation".
Company website:
Link: https://www.sercomm.com/

Signed-off-by: Mikhail Zhilkin <[email protected]>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 01430973ecec..65ff22364fb3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1082,6 +1082,8 @@ patternProperties:
     description: Sensirion AG
   "^sensortek,.*":
     description: Sensortek Technology Corporation
+  "^sercomm,.*":
+    description: Sercomm (Suzhou) Corporation
   "^sff,.*":
     description: Small Form Factor Committee
   "^sgd,.*":
--
2.25.1

> Best regards,
> Krzysztof

--
Best regards,
Mikhail

2022-05-03 00:34:18

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] dt-bindings: mtd: partitions: Add binding for Sercomm parser

On 29/04/2022 17:26, Mikhail Zhilkin wrote:
> On 4/29/2022 9:46 AM, Krzysztof Kozlowski wrote:
>
>>>> I am sorry, but you changed now a lot in the bindings and it looks
>>>> entirely different. Things previously being correct now are wrong, so
>>>> rather start from your old bindings...
>>>
>>> Looks like I'm a bit confused... I use dual "compatible" in my real dts
>>> and I realized that:
>>>
>>> 1. Therefore I have to use  dual "compatible" in example too:
>>>
>>> compatible = "sercomm,sc-partitions", "fixed-partitions";
>>>
>>> 2. When I'm trying to reuse "fixed-partitions" compatible from
>>> fixed-partitions.yaml in my new .yaml I get "too long" errors.
>> Yes, the fixed-partitions.yaml would have to be changed to allow extension.
>
> Well.
>
>>> Real dts:
>>>
>>> Link:
>>> https://github.com/openwrt/openwrt/blob/edcc1a9a734bb3fcdc9242025290d3f173e71b78/target/linux/ramips/dts/mt7621_beeline_smartbox-giga.dts#L79
>>>
>>> So, I currently found another solution - to extend fixed-partitions.yaml
>>> with "sercomm,sc-partitions". Is It ok from your side? Can I use this
>>> code in v3?
>> Not really, I don't understand why do you need it
>
> The main idea is keeping original Sercomm firmware behavior:
>
> 1. If dynamic partition map found then use offsets and mtd sizes stored
> in partition map. It's provided by "sercomm,sc-partitions" compatible.
>
> 2. If dynamic partition map doesn't exist or broken then default values
> (from dts) are used. It's provided by "fixed-partitions" compatible.

Then you need to adjust fixed-partitions for such case. See syscon case
(all over the tree and Documentation/devicetree/bindings/mfd/syscon.yaml).

>
>> and it does not
>> include our previous talks.
>
> At the time, I didn't realize how important is it. Understanding began
> to come after dozens of experiments and checking the similar Linux patches.
>
>>> diff --git
>>> a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> index ea4cace6a955..9eebe39a57fb 100644
>>> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>>> @@ -19,7 +19,11 @@ maintainers:
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: fixed-partitions
>>> +    oneOf:
>>> +      - const: fixed-partitions
>>> +      - items:
>>> +          - const: sercomm,sc-partitions
>>> +          - const: fixed-partitions
>>>  
>>>    "#address-cells": true
>>>  
>>> @@ -27,7 +31,18 @@ properties:
>>>  
>>>  patternProperties:
>>>    "@[0-9a-f]+$":
>>> -    $ref: "partition.yaml#"
>>> +    allOf:
>>> +      - $ref: "partition.yaml#"
>>> +      - if:
>>> +          properties:
>>> +            compatible:
>>> +              contains:
>>> +                const: sercomm,sc-partitions
>>> +        then:
>>> +          properties:
>>> +            scpart-id:
>> It still misses vendor prefix and we agreed you don't need it, didn't we?
>
> Do you mean "sercomm" vendor prefix? If so then we agreed that I include
> it in a separate patch:

There was some misunderstanding then. We talk here about scpart-id name.
Adding vendor prefix cannot be a separate patch because it does not make
much sense. You add new property with wrong name and immediately
change/fix it in next patch.

No, it should have proper name since beginning. The property is not used
in the kernel.

>
> Link:
> https://lore.kernel.org/linux-mtd/[email protected]/
>
> I'm going to send it in v3:
>
> ---
> dt-bindings: Add Sercomm (Suzhou) Corporation vendor prefix
>
> Update Documentation/devicetree/bindings/vendor-prefixes.yaml to include
> "sercomm" as a vendor prefix for "Sercomm (Suzhou) Corporation".
> Company website:
> Link: https://www.sercomm.com/
>
> Signed-off-by: Mikhail Zhilkin <[email protected]>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 01430973ecec..65ff22364fb3 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1082,6 +1082,8 @@ patternProperties:
>      description: Sensirion AG
>    "^sensortek,.*":
>      description: Sensortek Technology Corporation
> +  "^sercomm,.*":
> +    description: Sercomm (Suzhou) Corporation

This can be separate patch, but it's separate issue...


Best regards,
Krzysztof