Convert the xlnx,zynqmp-nvmem.txt to yaml.
Signed-off-by: Praveen Teja Kundanala <[email protected]>
---
.../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 <[email protected]>
+ - Praveen Teja Kundanala <[email protected]>
+
+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.1
On 13/10/2023 12:14, Praveen Teja Kundanala wrote:
> Convert the xlnx,zynqmp-nvmem.txt to yaml.
>
> Signed-off-by: Praveen Teja Kundanala <[email protected]>
> ---
> .../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 <[email protected]>
> + - Praveen Teja Kundanala <[email protected]>
> +
> +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 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 <[email protected]>
> ---
> .../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/[email protected]
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 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 <[email protected]>
>> ---
>> .../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 <[email protected]>
>> + - Praveen Teja Kundanala <[email protected]>
>> +
>> +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.
If you have only one entry you are also saying where it is.
Thanks,
Michal
On 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 <[email protected]>
> ---
> .../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 <[email protected]>
> + - Praveen Teja Kundanala <[email protected]>
> +
> +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/[email protected]/
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 <[email protected]>
> Sent: Monday, October 16, 2023 1:32 PM
> To: Krzysztof Kozlowski <[email protected]>; Kundanala, Praveen
> Teja <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]
> 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