[PATCH] misc: atsha204a: Add support for atsha204 chip

[Heiko Schocher]

Hello Pali,

On 21.04.22 11:40, Pali Rohár wrote:
> On Thursday 21 April 2022 06:11:11 Heiko Schocher wrote:
>> Hello Pali,
>>
>> On 05.04.22 16:10, Pali Rohár wrote:
>>> On Tuesday 05 April 2022 15:52:17 Stefan Roese wrote:
>>>> On 4/5/22 15:28, Pali Rohár wrote:
>>>>> On Tuesday 05 April 2022 15:14:52 Stefan Roese wrote:
>>>>>> On 4/5/22 14:49, Pali Rohár wrote:
>>>>>>> atsha204 chip is predecessor of atsha204a chip. Current U-Boot driver
>>>>>>> atsha204a-i2c.c can use both atsha204 and atsha204a chips because it does
>>>>>>> not call specific functions to just one of these chips.
>>>>>>>
>>>>>>> So just add compatible string for atsha204.
>>>>>>>
>>>>>>> Signed-off-by: Pali Rohár <pali at kernel.org>
>>>>>>> ---
>>>>>>>    drivers/misc/atsha204a-i2c.c | 1 +
>>>>>>>    1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/drivers/misc/atsha204a-i2c.c b/drivers/misc/atsha204a-i2c.c
>>>>>>> index 63fe541dade3..8b0055f99893 100644
>>>>>>> --- a/drivers/misc/atsha204a-i2c.c
>>>>>>> +++ b/drivers/misc/atsha204a-i2c.c
>>>>>>> @@ -399,6 +399,7 @@ static int atsha204a_of_to_plat(struct udevice *dev)
>>>>>>>    }
>>>>>>>    static const struct udevice_id atsha204a_ids[] = {
>>>>>>> +	{ .compatible = "atmel,atsha204" },
>>>>>>>    	{ .compatible = "atmel,atsha204a" },
>>>>>>>    	{ }
>>>>>>>    };
>>>>>>
>>>>>> Why do we need this new compatible here in the driver?
>>>>>
>>>>> They are different chips,
>>>>
>>>> Sure...
>>>>
>>>>> so should have different compatible strings.
>>>>
>>>> ... but is this really necessary and "best practice"? If the driver
>>>> can handle both chips without any changes, why do you need the new
>>>> compatible here?
>>>
>>> Well, currently it can handle both of them.
>>>
>>> But if driver is going to be extended to support e.g. SHA command
>>> (Calculate a SHA256 digest) then this command should be issued only for
>>> atsha204a. atsha204 does not support it.
>>>
>>> Similarly, if other DTS-based system is going to implement that SHA
>>> command, it would mean that U-Boot DTS file would not be compatible with
>>> that other system.
>>>
>>> Also it is a good idea to have DTS files and its compatible strings
>>> universal and not u-boot specific. So it could be used also by other
>>> projects (e.g. linux kernel).
>>>
>>> And if we mix now two chips which are similar (and supports lot of
>>> common operations) we would not be able in future to extend drivers in
>>> backward compatible manner.
>>>
>>> Just to note, I'm not going to implement atsha204a specific commands
>>> (which are not available in atsha204; like SHA command) because I do not
>>> need them (right now).
>>>
>>>> Don't get me wrong. I'm not blocking this change, just want to be sure
>>>> that it's really necessary.
>>>
>>> In case U-Boot driver has compatible string something like
>>> "atsha204-common" which could say that driver is using only functions
>>> which are available in all chip family then there would not be need for
>>> it. But if driver has chip specific name, I think the best is not to
>>> mask one chip by another which does not have 1:1 SW API compatibility.
>>
>> From my side this is full okay to add here a new compatibility string
>> to differ between the two chips, and to see in DTS immediately which
>> chip is on the board. Also later if the driver really supports features
>> the other chip does not have, you do not need to change DTS anymore.
>>
>> I would love to see this patch first in linux. Do you plan to sent
>> similiar change to linux?
> 
> Hello! We are not using Linux kernel driver for atsha cryptochips (I was
> told that it decrease lifetime) but I can send also similar change to
> Linux.

See it, thanks!

>> And not forget, please add a documentation for the compatible string
>> in u-boot:/doc/device-tree-bindings/
> 
> Currently I do not see any information about atsha in
> u-boot/doc/device-tree-bindings.

So please add one, thanks!

bye,
Heiko
> 
>> Thanks!
>>
>> bye,
>> Heiko
>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>>> A quick grep
>>>>>> doesn't show this in any of the dts files, not in U-Boot and not in the
>>>>>> Kernel.
>>>>>
>>>>> Not yet. I'm preparing patches for a board which has atsha204 and will
>>>>> use this u-boot driver.
>>>>>
>>>>>> Just checking...
>>>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de