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

[Pali Rohár]

On Thursday 21 April 2022 11:40:47 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.

Now I sent patch to Linux:
https://lore.kernel.org/linux-crypto/20220421134457.5867-1-pali@kernel.org/T/#u

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