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 <p...@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.
> 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: h...@denx.de
