Convert the xlnx,zynqmp-nvmem.txt to yaml.
Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
---
.../bindings/nvmem/xlnx,zynqmp-nvmem.txt | 46 ---------------
.../bindings/nvmem/xlnx,zynqmp-nvmem.yaml | 59 +++++++++++++++++++
2 files changed, 59 insertions(+), 46 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
deleted file mode 100644
index 4881561b3a02..000000000000
--- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
+++ /dev/null
@@ -1,46 +0,0 @@
---------------------------------------------------------------------------
-= Zynq UltraScale+ MPSoC nvmem firmware driver binding =
---------------------------------------------------------------------------
-The nvmem_firmware node provides access to the hardware related data
-like soc revision, IDCODE... etc, By using the firmware interface.
-
-Required properties:
-- compatible: should be "xlnx,zynqmp-nvmem-fw"
-
-= Data cells =
-Are child nodes of silicon id, bindings of which as described in
-bindings/nvmem/nvmem.txt
-
--------
- Example
--------
-firmware {
- zynqmp_firmware: zynqmp-firmware {
- compatible = "xlnx,zynqmp-firmware";
- method = "smc";
-
- nvmem_firmware {
- compatible = "xlnx,zynqmp-nvmem-fw";
- #address-cells = <1>;
- #size-cells = <1>;
-
- /* Data cells */
- soc_revision: soc_revision {
- reg = <0x0 0x4>;
- };
- };
- };
-};
-
-= Data consumers =
-Are device nodes which consume nvmem data cells.
-
-For example:
- pcap {
- ...
-
- nvmem-cells = <&soc_revision>;
- nvmem-cell-names = "soc_revision";
-
- ...
- };
diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
new file mode 100644
index 000000000000..e03ed8c32537
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
+
+description: |
+ The ZynqMP MPSoC provides access to the hardware related data
+ like SOC revision, IDCODE.
+
+maintainers:
+ - Kalyani Akula <kalyani.akula@amd.com>
+ - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
+
+allOf:
+ - $ref: "nvmem.yaml#"
+
+properties:
+ compatible:
+ const: xlnx,zynqmp-nvmem-fw
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 1
+
+required:
+ - compatible
+
+patternProperties:
+ "^soc_revision@0$":
+ type: object
+ description:
+ This node is used to read SOC version and IDCODE of ZynqMP. Read-only.
+
+ properties:
+ reg:
+ maxItems: 1
+
+ required:
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ nvmem_firmware {
+ compatible = "xlnx,zynqmp-nvmem-fw";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ /* Data cells */
+ soc_revision: soc_revision@0 {
+ reg = <0x0 0x4>;
+ };
+ };
--
2.36.1On Fri, Oct 13, 2023 at 03:44:47PM +0530, Praveen Teja Kundanala wrote:
> Convert the xlnx,zynqmp-nvmem.txt to yaml.
>
> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> ---
> .../bindings/nvmem/xlnx,zynqmp-nvmem.txt | 46 ---------------
> .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml | 59 +++++++++++++++++++
> 2 files changed, 59 insertions(+), 46 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> deleted file mode 100644
> index 4881561b3a02..000000000000
> --- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> +++ /dev/null
> @@ -1,46 +0,0 @@
> ---------------------------------------------------------------------------
> -= Zynq UltraScale+ MPSoC nvmem firmware driver binding =
> ---------------------------------------------------------------------------
> -The nvmem_firmware node provides access to the hardware related data
> -like soc revision, IDCODE... etc, By using the firmware interface.
> -
> -Required properties:
> -- compatible: should be "xlnx,zynqmp-nvmem-fw"
> -
> -= Data cells =
> -Are child nodes of silicon id, bindings of which as described in
> -bindings/nvmem/nvmem.txt
> -
> --------
> - Example
> --------
> -firmware {
> - zynqmp_firmware: zynqmp-firmware {
> - compatible = "xlnx,zynqmp-firmware";
> - method = "smc";
> -
> - nvmem_firmware {
> - compatible = "xlnx,zynqmp-nvmem-fw";
> - #address-cells = <1>;
> - #size-cells = <1>;
> -
> - /* Data cells */
> - soc_revision: soc_revision {
> - reg = <0x0 0x4>;
> - };
> - };
> - };
> -};
> -
> -= Data consumers =
> -Are device nodes which consume nvmem data cells.
> -
> -For example:
> - pcap {
> - ...
> -
> - nvmem-cells = <&soc_revision>;
> - nvmem-cell-names = "soc_revision";
> -
> - ...
> - };
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> new file mode 100644
> index 000000000000..e03ed8c32537
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
> +
> +description: |
> + The ZynqMP MPSoC provides access to the hardware related data
> + like SOC revision, IDCODE.
> +
> +maintainers:
> + - Kalyani Akula <kalyani.akula@amd.com>
> + - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> +
> +allOf:
> + - $ref: "nvmem.yaml#"
> +
> +properties:
> + compatible:
> + const: xlnx,zynqmp-nvmem-fw
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 1
> +
> +required:
> + - compatible
> +
> +patternProperties:
> + "^soc_revision@0$":
Fixed string, not a pattern. dtschema will now flag this. Thanks for the
test case. ;)
> + type: object
Needs 'additionalProperties' or...
> + description:
> + This node is used to read SOC version and IDCODE of ZynqMP. Read-only.
> +
> + properties:
> + reg:
> + maxItems: 1
> +
> + required:
> + - reg
fixed-cell.yaml (via nvmem.yaml) already says this, so this can all be
dropped. Except that this[1] just landed, so you will need to adapt to
it.
Though there is an issue that fixed-cell.yaml doesn't restrict the
allowed properties, so that needs to happen somewhere... Not sure what
the fix is. Hopefully fixed-cell.yaml can just be changed to
'additionalProperties: false', but need to see if other properties are
in use.
Rob
[1] https://lore.kernel.org/all/169667333484.74178.7121029453685069845.b4-ty@linaro.org/
On Fri, 13 Oct 2023 15:44:47 +0530, Praveen Teja Kundanala wrote: > Convert the xlnx,zynqmp-nvmem.txt to yaml. > > Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com> > --- > .../bindings/nvmem/xlnx,zynqmp-nvmem.txt | 46 --------------- > .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml | 59 +++++++++++++++++++ > 2 files changed, 59 insertions(+), 46 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt > create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.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: ./Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml:18:11: [error] string value is redundantly quoted with any quotes (quoted-strings) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231013101450.573-3-praveen.teja.kundanala@amd.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. 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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
> Convert the xlnx,zynqmp-nvmem.txt to yaml.
>
> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> ---
> .../bindings/nvmem/xlnx,zynqmp-nvmem.txt | 46 ---------------
> .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml | 59 +++++++++++++++++++
> 2 files changed, 59 insertions(+), 46 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> deleted file mode 100644
> index 4881561b3a02..000000000000
> --- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
> +++ /dev/null
> @@ -1,46 +0,0 @@
> ---------------------------------------------------------------------------
> -= Zynq UltraScale+ MPSoC nvmem firmware driver binding =
> ---------------------------------------------------------------------------
> -The nvmem_firmware node provides access to the hardware related data
> -like soc revision, IDCODE... etc, By using the firmware interface.
> -
> -Required properties:
> -- compatible: should be "xlnx,zynqmp-nvmem-fw"
> -
> -= Data cells =
> -Are child nodes of silicon id, bindings of which as described in
> -bindings/nvmem/nvmem.txt
> -
> --------
> - Example
> --------
> -firmware {
> - zynqmp_firmware: zynqmp-firmware {
> - compatible = "xlnx,zynqmp-firmware";
> - method = "smc";
> -
> - nvmem_firmware {
> - compatible = "xlnx,zynqmp-nvmem-fw";
> - #address-cells = <1>;
> - #size-cells = <1>;
> -
> - /* Data cells */
> - soc_revision: soc_revision {
> - reg = <0x0 0x4>;
> - };
> - };
> - };
> -};
> -
> -= Data consumers =
> -Are device nodes which consume nvmem data cells.
> -
> -For example:
> - pcap {
> - ...
> -
> - nvmem-cells = <&soc_revision>;
> - nvmem-cell-names = "soc_revision";
> -
> - ...
> - };
> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> new file mode 100644
> index 000000000000..e03ed8c32537
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
> +
> +description: |
> + The ZynqMP MPSoC provides access to the hardware related data
> + like SOC revision, IDCODE.
> +
> +maintainers:
> + - Kalyani Akula <kalyani.akula@amd.com>
> + - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
> +
> +allOf:
> + - $ref: "nvmem.yaml#"
Drop quotes.
> +
> +properties:
> + compatible:
> + const: xlnx,zynqmp-nvmem-fw
> +
> + '#address-cells':
> + const: 1
Drop
> +
> + '#size-cells':
> + const: 1
Drop
> +
> +required:
> + - compatible
required: block goes after patternProperties: block
> +
> +patternProperties:
> + "^soc_revision@0$":
Why do you define individual memory cells? Is this part of a binding?
IOW, OS/Linux requires this?
> + type: object
> + description:
> + This node is used to read SOC version and IDCODE of ZynqMP. Read-only.
> +
> + properties:
> + reg:
> + maxItems: 1
> +
> + required:
> + - reg
> +
> +additionalProperties: false
Instead: unevaluatedProperties: false
> +
> +examples:
> + - |
> + nvmem_firmware {
Underscores are not allowed in node names.
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Look at other files for examples.
> + compatible = "xlnx,zynqmp-nvmem-fw";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + /* Data cells */
> + soc_revision: soc_revision@0 {
Drop unused label. No underscores in node names.
Best regards,
Krzysztof
On 10/13/23 12:30, Krzysztof Kozlowski wrote:
> On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
>> Convert the xlnx,zynqmp-nvmem.txt to yaml.
>>
>> Signed-off-by: Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
>> ---
>> .../bindings/nvmem/xlnx,zynqmp-nvmem.txt | 46 ---------------
>> .../bindings/nvmem/xlnx,zynqmp-nvmem.yaml | 59 +++++++++++++++++++
>> 2 files changed, 59 insertions(+), 46 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>> create mode 100644 Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>> deleted file mode 100644
>> index 4881561b3a02..000000000000
>> --- a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.txt
>> +++ /dev/null
>> @@ -1,46 +0,0 @@
>> ---------------------------------------------------------------------------
>> -= Zynq UltraScale+ MPSoC nvmem firmware driver binding =
>> ---------------------------------------------------------------------------
>> -The nvmem_firmware node provides access to the hardware related data
>> -like soc revision, IDCODE... etc, By using the firmware interface.
>> -
>> -Required properties:
>> -- compatible: should be "xlnx,zynqmp-nvmem-fw"
>> -
>> -= Data cells =
>> -Are child nodes of silicon id, bindings of which as described in
>> -bindings/nvmem/nvmem.txt
>> -
>> --------
>> - Example
>> --------
>> -firmware {
>> - zynqmp_firmware: zynqmp-firmware {
>> - compatible = "xlnx,zynqmp-firmware";
>> - method = "smc";
>> -
>> - nvmem_firmware {
>> - compatible = "xlnx,zynqmp-nvmem-fw";
>> - #address-cells = <1>;
>> - #size-cells = <1>;
>> -
>> - /* Data cells */
>> - soc_revision: soc_revision {
>> - reg = <0x0 0x4>;
>> - };
>> - };
>> - };
>> -};
>> -
>> -= Data consumers =
>> -Are device nodes which consume nvmem data cells.
>> -
>> -For example:
>> - pcap {
>> - ...
>> -
>> - nvmem-cells = <&soc_revision>;
>> - nvmem-cell-names = "soc_revision";
>> -
>> - ...
>> - };
>> diff --git a/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>> new file mode 100644
>> index 000000000000..e03ed8c32537
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>> @@ -0,0 +1,59 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/nvmem/xlnx,zynqmp-nvmem.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Zynq UltraScale+ MPSoC Non Volatile Memory interface
>> +
>> +description: |
>> + The ZynqMP MPSoC provides access to the hardware related data
>> + like SOC revision, IDCODE.
>> +
>> +maintainers:
>> + - Kalyani Akula <kalyani.akula@amd.com>
>> + - Praveen Teja Kundanala <praveen.teja.kundanala@amd.com>
>> +
>> +allOf:
>> + - $ref: "nvmem.yaml#"
>
> Drop quotes.
>
>> +
>> +properties:
>> + compatible:
>> + const: xlnx,zynqmp-nvmem-fw
>> +
>> + '#address-cells':
>> + const: 1
>
> Drop
>
>> +
>> + '#size-cells':
>> + const: 1
>
> Drop
>
>> +
>> +required:
>> + - compatible
>
> required: block goes after patternProperties: block
>
>> +
>> +patternProperties:
>> + "^soc_revision@0$":
>
> Why do you define individual memory cells? Is this part of a binding?
> IOW, OS/Linux requires this?
nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
calls. It means you should be able to describe internal layout that's why names
are used. And address in name is there because of reg property is used to
describe base offset and size.
Thanks,
Michal
On 13/10/2023 13:22, Michal Simek wrote: >> >>> + >>> +required: >>> + - compatible >> >> required: block goes after patternProperties: block >> >>> + >>> +patternProperties: >>> + "^soc_revision@0$": >> >> Why do you define individual memory cells? Is this part of a binding? >> IOW, OS/Linux requires this? > > nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() > calls. It means you should be able to describe internal layout that's why names > are used. And address in name is there because of reg property is used to > describe base offset and size. That's not really what I am asking. Why internal layout of memory must be part of the bindings? Best regards, Krzysztof
On 10/13/23 13:46, Krzysztof Kozlowski wrote: > On 13/10/2023 13:22, Michal Simek wrote: >>> >>>> + >>>> +required: >>>> + - compatible >>> >>> required: block goes after patternProperties: block >>> >>>> + >>>> +patternProperties: >>>> + "^soc_revision@0$": >>> >>> Why do you define individual memory cells? Is this part of a binding? >>> IOW, OS/Linux requires this? >> >> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() >> calls. It means you should be able to describe internal layout that's why names >> are used. And address in name is there because of reg property is used to >> describe base offset and size. > > That's not really what I am asking. Why internal layout of memory must > be part of the bindings? It doesn't need to be but offsets are hardcoded inside the driver itself and they can't be different. On different nvmem locations like MAC location in eeprom this can vary across boards but in this case location has to be only like this. I am fine if they don't need to be actually check but there is no any other way how they can be composed. And also others are not valid that's why not to describe only valid one. Thanks, Michal
On 13/10/2023 13:51, Michal Simek wrote: > > > On 10/13/23 13:46, Krzysztof Kozlowski wrote: >> On 13/10/2023 13:22, Michal Simek wrote: >>>> >>>>> + >>>>> +required: >>>>> + - compatible >>>> >>>> required: block goes after patternProperties: block >>>> >>>>> + >>>>> +patternProperties: >>>>> + "^soc_revision@0$": >>>> >>>> Why do you define individual memory cells? Is this part of a binding? >>>> IOW, OS/Linux requires this? >>> >>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() >>> calls. It means you should be able to describe internal layout that's why names >>> are used. And address in name is there because of reg property is used to >>> describe base offset and size. >> >> That's not really what I am asking. Why internal layout of memory must >> be part of the bindings? > > It doesn't need to be but offsets are hardcoded inside the driver itself and > they can't be different. Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any hard-coded offsets. > On different nvmem locations like MAC location in > eeprom this can vary across boards but in this case location has to be only like > this. > I am fine if they don't need to be actually check but there is no any other way > how they can be composed. And also others are not valid that's why not to > describe only valid one. OK, that would be valid (if I find anywhere the offsets) and answers my questions but I wish it was documented somewhere. Because now you are making it a binding, so it cannot change (e.g. for different devices with same hardware but different firmware or manufacturing process, for future hardware sharing this binding). In any case the binding should have only items which are really fixed and OS depends on them. Neither this nor next commit answers this. Best regards, Krzysztof
On 10/13/23 13:58, Krzysztof Kozlowski wrote:
> On 13/10/2023 13:51, Michal Simek wrote:
>>
>>
>> On 10/13/23 13:46, Krzysztof Kozlowski wrote:
>>> On 13/10/2023 13:22, Michal Simek wrote:
>>>>>
>>>>>> +
>>>>>> +required:
>>>>>> + - compatible
>>>>>
>>>>> required: block goes after patternProperties: block
>>>>>
>>>>>> +
>>>>>> +patternProperties:
>>>>>> + "^soc_revision@0$":
>>>>>
>>>>> Why do you define individual memory cells? Is this part of a binding?
>>>>> IOW, OS/Linux requires this?
>>>>
>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get()
>>>> calls. It means you should be able to describe internal layout that's why names
>>>> are used. And address in name is there because of reg property is used to
>>>> describe base offset and size.
>>>
>>> That's not really what I am asking. Why internal layout of memory must
>>> be part of the bindings?
>>
>> It doesn't need to be but offsets are hardcoded inside the driver itself and
>> they can't be different.
>
> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any
> hard-coded offsets.
Current driver supports only soc revision from offset 0.
But if you look at 5/5 you need to define offsets where information is present.
+#define SOC_VERSION_OFFSET 0x0
+#define EFUSE_START_OFFSET 0xC
+#define EFUSE_END_OFFSET 0xFC
+#define EFUSE_PUF_START_OFFSET 0x100
+#define EFUSE_PUF_MID_OFFSET 0x140
+#define EFUSE_PUF_END_OFFSET 0x17F
>
>> On different nvmem locations like MAC location in
>> eeprom this can vary across boards but in this case location has to be only like
>> this.
>> I am fine if they don't need to be actually check but there is no any other way
>> how they can be composed. And also others are not valid that's why not to
>> describe only valid one.
>
> OK, that would be valid (if I find anywhere the offsets) and answers my
> questions but I wish it was documented somewhere. Because now you are
> making it a binding, so it cannot change (e.g. for different devices
> with same hardware but different firmware or manufacturing process, for
> future hardware sharing this binding).
ZynqMP firmware with xlnx,zynqmp-nvmem-fw compatible string is using this map.
If there is different firmware likely xlnx,zynqmp-nvmem-fw compatible string
can't be used.
> In any case the binding should have only items which are really fixed
> and OS depends on them. Neither this nor next commit answers this.
Actually 5/5 has this in commit message
0 - SOC version(read only)
0xC - 0xFC -ZynqMP specific purpose efuses
0x100 - 0x17F - Physical Unclonable Function(PUF)
efuses repurposed as user efuses
Thanks,
Michal
On 13/10/2023 14:08, Michal Simek wrote: > > > On 10/13/23 13:58, Krzysztof Kozlowski wrote: >> On 13/10/2023 13:51, Michal Simek wrote: >>> >>> >>> On 10/13/23 13:46, Krzysztof Kozlowski wrote: >>>> On 13/10/2023 13:22, Michal Simek wrote: >>>>>> >>>>>>> + >>>>>>> +required: >>>>>>> + - compatible >>>>>> >>>>>> required: block goes after patternProperties: block >>>>>> >>>>>>> + >>>>>>> +patternProperties: >>>>>>> + "^soc_revision@0$": >>>>>> >>>>>> Why do you define individual memory cells? Is this part of a binding? >>>>>> IOW, OS/Linux requires this? >>>>> >>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() >>>>> calls. It means you should be able to describe internal layout that's why names >>>>> are used. And address in name is there because of reg property is used to >>>>> describe base offset and size. >>>> >>>> That's not really what I am asking. Why internal layout of memory must >>>> be part of the bindings? >>> >>> It doesn't need to be but offsets are hardcoded inside the driver itself and >>> they can't be different. >> >> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any >> hard-coded offsets. > > Current driver supports only soc revision from offset 0. > But if you look at 5/5 you need to define offsets where information is present. > +#define SOC_VERSION_OFFSET 0x0 > +#define EFUSE_START_OFFSET 0xC > +#define EFUSE_END_OFFSET 0xFC > +#define EFUSE_PUF_START_OFFSET 0x100 > +#define EFUSE_PUF_MID_OFFSET 0x140 > +#define EFUSE_PUF_END_OFFSET 0x17F There is nothing like this in existing driver, so the argument that "I am adding this to the binding during conversion because driver needs it" is not true. Conversion is only a conversion. Now, if you want to add something new to the binding because of new driver changes, that's separate topic. And since it is new change in the driver I can comment: please don't. Your nvmem driver should not depend on it. nvmem is only the provider. Best regards, Krzysztof
On 10/13/23 14:54, Krzysztof Kozlowski wrote: > On 13/10/2023 14:08, Michal Simek wrote: >> >> >> On 10/13/23 13:58, Krzysztof Kozlowski wrote: >>> On 13/10/2023 13:51, Michal Simek wrote: >>>> >>>> >>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote: >>>>> On 13/10/2023 13:22, Michal Simek wrote: >>>>>>> >>>>>>>> + >>>>>>>> +required: >>>>>>>> + - compatible >>>>>>> >>>>>>> required: block goes after patternProperties: block >>>>>>> >>>>>>>> + >>>>>>>> +patternProperties: >>>>>>>> + "^soc_revision@0$": >>>>>>> >>>>>>> Why do you define individual memory cells? Is this part of a binding? >>>>>>> IOW, OS/Linux requires this? >>>>>> >>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() >>>>>> calls. It means you should be able to describe internal layout that's why names >>>>>> are used. And address in name is there because of reg property is used to >>>>>> describe base offset and size. >>>>> >>>>> That's not really what I am asking. Why internal layout of memory must >>>>> be part of the bindings? >>>> >>>> It doesn't need to be but offsets are hardcoded inside the driver itself and >>>> they can't be different. >>> >>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any >>> hard-coded offsets. >> >> Current driver supports only soc revision from offset 0. >> But if you look at 5/5 you need to define offsets where information is present. >> +#define SOC_VERSION_OFFSET 0x0 >> +#define EFUSE_START_OFFSET 0xC >> +#define EFUSE_END_OFFSET 0xFC >> +#define EFUSE_PUF_START_OFFSET 0x100 >> +#define EFUSE_PUF_MID_OFFSET 0x140 >> +#define EFUSE_PUF_END_OFFSET 0x17F > > There is nothing like this in existing driver, so the argument that "I > am adding this to the binding during conversion because driver needs it" > is not true. Conversion is only a conversion. Conversion in 2/5 is adding only soc revision which is already there. It is starting from 0 and world size is 1. And 0 is not listed because that's start all the time. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39 And soc revision was also listed in origin binding example. > Now, if you want to add something new to the binding because of new > driver changes, that's separate topic. Functionality in firmware is there for quite a long time but as I said I am fine if map is not going to be inside dt binding spec. > And since it is new change in the driver I can comment: please don't. > Your nvmem driver should not depend on it. nvmem is only the provider. Let's see what Srinivas says about implementation. If driver should be just provider then pretty much current driver should be completely rewritten to different style. I mean to have just transport via SMCs with offset/size and then providing functionality in firmware. Thanks, Michal
On 13/10/2023 15:06, Michal Simek wrote: > > > On 10/13/23 14:54, Krzysztof Kozlowski wrote: >> On 13/10/2023 14:08, Michal Simek wrote: >>> >>> >>> On 10/13/23 13:58, Krzysztof Kozlowski wrote: >>>> On 13/10/2023 13:51, Michal Simek wrote: >>>>> >>>>> >>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote: >>>>>> On 13/10/2023 13:22, Michal Simek wrote: >>>>>>>> >>>>>>>>> + >>>>>>>>> +required: >>>>>>>>> + - compatible >>>>>>>> >>>>>>>> required: block goes after patternProperties: block >>>>>>>> >>>>>>>>> + >>>>>>>>> +patternProperties: >>>>>>>>> + "^soc_revision@0$": >>>>>>>> >>>>>>>> Why do you define individual memory cells? Is this part of a binding? >>>>>>>> IOW, OS/Linux requires this? >>>>>>> >>>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() >>>>>>> calls. It means you should be able to describe internal layout that's why names >>>>>>> are used. And address in name is there because of reg property is used to >>>>>>> describe base offset and size. >>>>>> >>>>>> That's not really what I am asking. Why internal layout of memory must >>>>>> be part of the bindings? >>>>> >>>>> It doesn't need to be but offsets are hardcoded inside the driver itself and >>>>> they can't be different. >>>> >>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any >>>> hard-coded offsets. >>> >>> Current driver supports only soc revision from offset 0. >>> But if you look at 5/5 you need to define offsets where information is present. >>> +#define SOC_VERSION_OFFSET 0x0 >>> +#define EFUSE_START_OFFSET 0xC >>> +#define EFUSE_END_OFFSET 0xFC >>> +#define EFUSE_PUF_START_OFFSET 0x100 >>> +#define EFUSE_PUF_MID_OFFSET 0x140 >>> +#define EFUSE_PUF_END_OFFSET 0x17F >> >> There is nothing like this in existing driver, so the argument that "I >> am adding this to the binding during conversion because driver needs it" >> is not true. Conversion is only a conversion. > > Conversion in 2/5 is adding only soc revision which is already there. It is > starting from 0 and world size is 1. And 0 is not listed because that's start > all the time. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39 This defines the nvmem config, not what should be where. > > And soc revision was also listed in origin binding example. Example is not a binding. Please drop enforcement of some specific nodes from the binding. > >> Now, if you want to add something new to the binding because of new >> driver changes, that's separate topic. > > Functionality in firmware is there for quite a long time but as I said I am fine > if map is not going to be inside dt binding spec. > >> And since it is new change in the driver I can comment: please don't. >> Your nvmem driver should not depend on it. nvmem is only the provider. > > Let's see what Srinivas says about implementation. If driver should be just > provider then pretty much current driver should be completely rewritten to > different style. I mean to have just transport via SMCs with offset/size and > then providing functionality in firmware. Best regards, Krzysztof
On 10/13/23 15:10, Krzysztof Kozlowski wrote: > On 13/10/2023 15:06, Michal Simek wrote: >> >> >> On 10/13/23 14:54, Krzysztof Kozlowski wrote: >>> On 13/10/2023 14:08, Michal Simek wrote: >>>> >>>> >>>> On 10/13/23 13:58, Krzysztof Kozlowski wrote: >>>>> On 13/10/2023 13:51, Michal Simek wrote: >>>>>> >>>>>> >>>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote: >>>>>>> On 13/10/2023 13:22, Michal Simek wrote: >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> +required: >>>>>>>>>> + - compatible >>>>>>>>> >>>>>>>>> required: block goes after patternProperties: block >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> +patternProperties: >>>>>>>>>> + "^soc_revision@0$": >>>>>>>>> >>>>>>>>> Why do you define individual memory cells? Is this part of a binding? >>>>>>>>> IOW, OS/Linux requires this? >>>>>>>> >>>>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() >>>>>>>> calls. It means you should be able to describe internal layout that's why names >>>>>>>> are used. And address in name is there because of reg property is used to >>>>>>>> describe base offset and size. >>>>>>> >>>>>>> That's not really what I am asking. Why internal layout of memory must >>>>>>> be part of the bindings? >>>>>> >>>>>> It doesn't need to be but offsets are hardcoded inside the driver itself and >>>>>> they can't be different. >>>>> >>>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any >>>>> hard-coded offsets. >>>> >>>> Current driver supports only soc revision from offset 0. >>>> But if you look at 5/5 you need to define offsets where information is present. >>>> +#define SOC_VERSION_OFFSET 0x0 >>>> +#define EFUSE_START_OFFSET 0xC >>>> +#define EFUSE_END_OFFSET 0xFC >>>> +#define EFUSE_PUF_START_OFFSET 0x100 >>>> +#define EFUSE_PUF_MID_OFFSET 0x140 >>>> +#define EFUSE_PUF_END_OFFSET 0x17F >>> >>> There is nothing like this in existing driver, so the argument that "I >>> am adding this to the binding during conversion because driver needs it" >>> is not true. Conversion is only a conversion. >> >> Conversion in 2/5 is adding only soc revision which is already there. It is >> starting from 0 and world size is 1. And 0 is not listed because that's start >> all the time. >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39 > > This defines the nvmem config, not what should be where. > >> >> And soc revision was also listed in origin binding example. > > Example is not a binding. Please drop enforcement of some specific nodes > from the binding. ok. Fine. Praveen: Please drop that descriptions about sub nodes. Thanks, Michal
[AMD Official Use Only - General] > -----Original Message----- > From: Simek, Michal <michal.simek@amd.com> > Sent: Monday, October 16, 2023 1:32 PM > To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Kundanala, Praveen > Teja <praveen.teja.kundanala@amd.com>; srinivas.kandagatla@linaro.org; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Subject: Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt > to yaml > > > > On 10/13/23 15:10, Krzysztof Kozlowski wrote: > > On 13/10/2023 15:06, Michal Simek wrote: > >> > >> > >> On 10/13/23 14:54, Krzysztof Kozlowski wrote: > >>> On 13/10/2023 14:08, Michal Simek wrote: > >>>> > >>>> > >>>> On 10/13/23 13:58, Krzysztof Kozlowski wrote: > >>>>> On 13/10/2023 13:51, Michal Simek wrote: > >>>>>> > >>>>>> > >>>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote: > >>>>>>> On 13/10/2023 13:22, Michal Simek wrote: > >>>>>>>>> > >>>>>>>>>> + > >>>>>>>>>> +required: > >>>>>>>>>> + - compatible > >>>>>>>>> > >>>>>>>>> required: block goes after patternProperties: block > >>>>>>>>> > >>>>>>>>>> + > >>>>>>>>>> +patternProperties: > >>>>>>>>>> + "^soc_revision@0$": > >>>>>>>>> > >>>>>>>>> Why do you define individual memory cells? Is this part of a > binding? > >>>>>>>>> IOW, OS/Linux requires this? > >>>>>>>> > >>>>>>>> nvmem has in kernel interface where you can reference to nodes. > >>>>>>>> nvmem_cell_get() calls. It means you should be able to describe > >>>>>>>> internal layout that's why names are used. And address in name > >>>>>>>> is there because of reg property is used to describe base offset and > size. > >>>>>>> > >>>>>>> That's not really what I am asking. Why internal layout of > >>>>>>> memory must be part of the bindings? > >>>>>> > >>>>>> It doesn't need to be but offsets are hardcoded inside the driver > >>>>>> itself and they can't be different. > >>>>> > >>>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not > see > >>>>> any hard-coded offsets. > >>>> > >>>> Current driver supports only soc revision from offset 0. > >>>> But if you look at 5/5 you need to define offsets where information is > present. > >>>> +#define SOC_VERSION_OFFSET 0x0 > >>>> +#define EFUSE_START_OFFSET 0xC > >>>> +#define EFUSE_END_OFFSET 0xFC > >>>> +#define EFUSE_PUF_START_OFFSET 0x100 > >>>> +#define EFUSE_PUF_MID_OFFSET 0x140 > >>>> +#define EFUSE_PUF_END_OFFSET 0x17F > >>> > >>> There is nothing like this in existing driver, so the argument that > >>> "I am adding this to the binding during conversion because driver needs it" > >>> is not true. Conversion is only a conversion. > >> > >> Conversion in 2/5 is adding only soc revision which is already there. > >> It is starting from 0 and world size is 1. And 0 is not listed > >> because that's start all the time. > >> > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tr > >> ee/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39 > > > > This defines the nvmem config, not what should be where. > > > >> > >> And soc revision was also listed in origin binding example. > > > > Example is not a binding. Please drop enforcement of some specific > > nodes from the binding. > > ok. Fine. > Praveen: Please drop that descriptions about sub nodes. >[Kundanala, Praveen Teja] Okay. > > Thanks, > Michal
On 10/13/23 15:10, Krzysztof Kozlowski wrote: > On 13/10/2023 15:06, Michal Simek wrote: >> >> >> On 10/13/23 14:54, Krzysztof Kozlowski wrote: >>> On 13/10/2023 14:08, Michal Simek wrote: >>>> >>>> >>>> On 10/13/23 13:58, Krzysztof Kozlowski wrote: >>>>> On 13/10/2023 13:51, Michal Simek wrote: >>>>>> >>>>>> >>>>>> On 10/13/23 13:46, Krzysztof Kozlowski wrote: >>>>>>> On 13/10/2023 13:22, Michal Simek wrote: >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> +required: >>>>>>>>>> + - compatible >>>>>>>>> >>>>>>>>> required: block goes after patternProperties: block >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> +patternProperties: >>>>>>>>>> + "^soc_revision@0$": >>>>>>>>> >>>>>>>>> Why do you define individual memory cells? Is this part of a binding? >>>>>>>>> IOW, OS/Linux requires this? >>>>>>>> >>>>>>>> nvmem has in kernel interface where you can reference to nodes. nvmem_cell_get() >>>>>>>> calls. It means you should be able to describe internal layout that's why names >>>>>>>> are used. And address in name is there because of reg property is used to >>>>>>>> describe base offset and size. >>>>>>> >>>>>>> That's not really what I am asking. Why internal layout of memory must >>>>>>> be part of the bindings? >>>>>> >>>>>> It doesn't need to be but offsets are hardcoded inside the driver itself and >>>>>> they can't be different. >>>>> >>>>> Hm, where? I opened drivers/nvmem/zynqmp_nvmem.c and I do not see any >>>>> hard-coded offsets. >>>> >>>> Current driver supports only soc revision from offset 0. >>>> But if you look at 5/5 you need to define offsets where information is present. >>>> +#define SOC_VERSION_OFFSET 0x0 >>>> +#define EFUSE_START_OFFSET 0xC >>>> +#define EFUSE_END_OFFSET 0xFC >>>> +#define EFUSE_PUF_START_OFFSET 0x100 >>>> +#define EFUSE_PUF_MID_OFFSET 0x140 >>>> +#define EFUSE_PUF_END_OFFSET 0x17F >>> >>> There is nothing like this in existing driver, so the argument that "I >>> am adding this to the binding during conversion because driver needs it" >>> is not true. Conversion is only a conversion. >> >> Conversion in 2/5 is adding only soc revision which is already there. It is >> starting from 0 and world size is 1. And 0 is not listed because that's start >> all the time. >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/zynqmp_nvmem.c?h=v6.6-rc5#n39 > > This defines the nvmem config, not what should be where. If you have only one entry you are also saying where it is. Thanks, Michal
© 2016 - 2025 Red Hat, Inc.