[2/3] nvmem: sunxi_sid: Add support for H616 SID

  Add support for the H616 SID controller.

The config can be reused from A64.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 drivers/nvmem/sunxi_sid.c | 1 +
 1 file changed, 1 insertion(+)
  

On Fri, 11 Aug 2023 13:02:35 +0200
Martin Botka <martin.botka@somainline.org> wrote:

Hi Martin,

> Add support for the H616 SID controller.

thanks for upstreaming this!

> 
> The config can be reused from A64.
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/nvmem/sunxi_sid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> index a970f1741cc6..df6fb5e0b724 100644
> --- a/drivers/nvmem/sunxi_sid.c
> +++ b/drivers/nvmem/sunxi_sid.c
> @@ -216,6 +216,7 @@ static const struct of_device_id sunxi_sid_of_match[] = {
>  	{ .compatible = "allwinner,sun50i-a64-sid", .data = &sun50i_a64_cfg },
>  	{ .compatible = "allwinner,sun50i-h5-sid", .data = &sun50i_a64_cfg },
>  	{ .compatible = "allwinner,sun50i-h6-sid", .data = &sun50i_h6_cfg },
> +	{ .compatible = "allwinner,sun50i-h616-sid", .data = &sun50i_a64_cfg },

You don't need a new compatible string, then.
Just use 
  compatible = "allwinner,sun50i-h616-sid", "allwinner,sun50i-a64-sid";
in the DT, and add that combo to the binding, then you don't need any
driver patches at all - with the added benefit of that already working
with existing kernels.

Though I wonder if that has really only 256 bytes of fuses? Do we have any
evidence of that?

Cheers,
Andre

>  	{/* sentinel */},
>  };
>  MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
>
  

On Fri, Aug 11 2023 at 12:17:54 PM +01:00:00, Andre Przywara 
<andre.przywara@arm.com> wrote:
> On Fri, 11 Aug 2023 13:02:35 +0200
> Martin Botka <martin.botka@somainline.org> wrote:
> 
> Hi Martin,
> 
>>  Add support for the H616 SID controller.
> 
> thanks for upstreaming this!
> 
>> 
>>  The config can be reused from A64.
>> 
>>  Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>  ---
>>   drivers/nvmem/sunxi_sid.c | 1 +
>>   1 file changed, 1 insertion(+)
>> 
>>  diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
>>  index a970f1741cc6..df6fb5e0b724 100644
>>  --- a/drivers/nvmem/sunxi_sid.c
>>  +++ b/drivers/nvmem/sunxi_sid.c
>>  @@ -216,6 +216,7 @@ static const struct of_device_id 
>> sunxi_sid_of_match[] = {
>>   	{ .compatible = "allwinner,sun50i-a64-sid", .data = 
>> &sun50i_a64_cfg },
>>   	{ .compatible = "allwinner,sun50i-h5-sid", .data = 
>> &sun50i_a64_cfg },
>>   	{ .compatible = "allwinner,sun50i-h6-sid", .data = &sun50i_h6_cfg 
>> },
>>  +	{ .compatible = "allwinner,sun50i-h616-sid", .data = 
>> &sun50i_a64_cfg },
> 
> You don't need a new compatible string, then.
> Just use
>   compatible = "allwinner,sun50i-h616-sid", 
> "allwinner,sun50i-a64-sid";
> in the DT, and add that combo to the binding, then you don't need any
> driver patches at all - with the added benefit of that already working
> with existing kernels.
I added it cause H5 and A83T were added after their DT binding was in 
and they also use A64 config.
But I will drop this patch and use a64 compatible if that is the 
prefered way :)
> 
> Though I wonder if that has really only 256 bytes of fuses? Do we 
> have any
> evidence of that?
Datasheet has 0 info regarding this. The way i got the size and offset 
was to check the vendor code.
The values matched A64.
> 
> Cheers,
> Andre
> 
>>   	{/* sentinel */},
>>   };
>>   MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
>> 
>
  

On Fri, 11 Aug 2023 15:12:34 +0200
Martin Botka <martin.botka@somainline.org> wrote:

Hi,

Maxime, Srinivas, can you possibly shed some light on the exact scope of
this nvmem DT binding? See below for more details ...

> On Fri, Aug 11 2023 at 12:17:54 PM +01:00:00, Andre Przywara 
> <andre.przywara@arm.com> wrote:
> > On Fri, 11 Aug 2023 13:02:35 +0200
> > Martin Botka <martin.botka@somainline.org> wrote:
> > 
> > Hi Martin,
> >   
> >>  Add support for the H616 SID controller.  
> > 
> > thanks for upstreaming this!
> >   
> >> 
> >>  The config can be reused from A64.
> >> 
> >>  Signed-off-by: Martin Botka <martin.botka@somainline.org>
> >>  ---
> >>   drivers/nvmem/sunxi_sid.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >> 
> >>  diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> >>  index a970f1741cc6..df6fb5e0b724 100644
> >>  --- a/drivers/nvmem/sunxi_sid.c
> >>  +++ b/drivers/nvmem/sunxi_sid.c
> >>  @@ -216,6 +216,7 @@ static const struct of_device_id 
> >> sunxi_sid_of_match[] = {
> >>   	{ .compatible = "allwinner,sun50i-a64-sid", .data = 
> >> &sun50i_a64_cfg },
> >>   	{ .compatible = "allwinner,sun50i-h5-sid", .data = 
> >> &sun50i_a64_cfg },
> >>   	{ .compatible = "allwinner,sun50i-h6-sid", .data = &sun50i_h6_cfg 
> >> },
> >>  +	{ .compatible = "allwinner,sun50i-h616-sid", .data = 
> >> &sun50i_a64_cfg },  
> > 
> > You don't need a new compatible string, then.
> > Just use
> >   compatible = "allwinner,sun50i-h616-sid", 
> > "allwinner,sun50i-a64-sid";
> > in the DT, and add that combo to the binding, then you don't need any
> > driver patches at all - with the added benefit of that already working
> > with existing kernels.  
> I added it cause H5 and A83T were added after their DT binding was in 
> and they also use A64 config.

Yeah, I saw this, and going back in the commit messages I found this:
"The H5 SoC have a SID controller that looks like the one in A64, the
cells are in the same offset but doesn't contain the same data (thermal
sensor calibration for example)."

So does the compatible string for the NVMEM *controller* really
encompass the location, encoding and meaning of all its NVMEM cells?
Isn't that described independently by the child nodes, that put names
to addresses, and that are referenced by other DT nodes?
Or was it originally not the case, and the scope changed over time?

It's certainly no biggie to use a single compatible string, and add a
single line to the driver (like in this patch), but it would be good to
know if this is really the intention.

Cheers,
Andre

> But I will drop this patch and use a64 compatible if that is the 
> prefered way :)
> > 
> > Though I wonder if that has really only 256 bytes of fuses? Do we 
> > have any
> > evidence of that?  
> Datasheet has 0 info regarding this. The way i got the size and offset 
> was to check the vendor code.
> The values matched A64.
> > 
> > Cheers,
> > Andre
> >   
> >>   	{/* sentinel */},
> >>   };
> >>   MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
> >>   
> >   
> 
> 
>