[2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to yaml

  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
  

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 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 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.

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 <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 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