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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <edbd1434-c05b-461f-96e5-f57775dcf915@amd.com>
Date:   Fri, 13 Oct 2023 15:06:34 +0200
From:   Michal Simek <michal.simek@....com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "Praveen Teja Kundanala" <praveen.teja.kundanala@....com>,
        <srinivas.kandagatla@...aro.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
CC:     <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] dt-bindings: nvmem: Convert xlnx,zynqmp-nvmem.txt to
 yaml



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