Re: [PATCH 06/15] soc: add Ambarella driver

[Arnd Bergmann]

On Tue, Jan 24, 2023, at 08:58, Li Chen wrote:
> On Mon, 23 Jan 2023 16:29:06 +0800,
> Arnd Bergmann wrote:
>> On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
>> > +static struct ambarella_soc_id {
>> > +	unsigned int id;
>> > +	const char *name;
>> > +	const char *family;
>> > +} soc_ids[] = {
>> > +	{ 0x00483245, "s6lm",  "10nm", },
>> > +};
>>
>> I would suggest something more descriptive in the "family"
>> field to let users know they are on an Ambarella SoC.
>>
>> Maybe just "Ambarella 10nm".
>
> There is a "pr_info("Ambarella SoC %s detected\n", 
> soc_dev_attr->soc_id);" in this file,
> I think this should be enough, right?

The pr_info() can probably be removed here, or reworded
based on the changed contents, those are just meant for
humans reading through the log rather than parsed by
software.

The soc_id fields on the other hand need to be parsable
by scripts looking at the sysfs files, in a way that lets
them identify the system. Usually the script would look
at the "family" as the primary key before looking up the
"name", so you have to make sure that the family uniquely
identifies this as one of yours rather than a 10nm chip
from some other company.

>> If there are other unrelated registers in there, the compatible
>> string should probably be changed to better describe the
>> entire area based on the name in the datasheet.
>
> Yeah, this block is only used for identification bits. In datasheet,
> it is also named "CPU ID".

ok.

> Other than cpuid_regmap, this driver also looks for "model" name as soc 
> machine name:
> of_property_read_string(np, "model", &soc_dev_attr->machine);
>
> So I think it is not a good idea to conver it to into a platform driver.

I don't understand what you mean. A lot of soc_id drivers put
the model string into soc_dev_attr->machine, this makes no
difference here.

> As for "syscon", I think it is still very helpful to get regmap easily. 
> Generally speaking,
> I prefer regmap over void*, because it has debugfs support, so I can 
> get its value more easily.

What value would you get through debugfs that is not already in
the soc_device?

>> > +static unsigned int ambsys_config;
>> > +
>> > +unsigned int ambarella_sys_config(void)
>> > +{
>> > +	return ambsys_config;
>> > +}
>> > +EXPORT_SYMBOL(ambarella_sys_config);
>>
>> Which drivers use this bit? Can they be changed to
>> use soc_device_match() instead to avoid the export?
>
> sys_config is used by our nand and sd drivers. I also don't want to export,
> but struct soc_device_attribute/soc_device don't have private data to store it,
> I think there is no better way.

The nand and sd drivers should not rely on any private data
from another driver. What information do they actually
need here that is not already in their own DT nodes or
in the soc_device_attributes?

>> > +static int __init ambarella_soc_init(void)
>> > +{
>> > +	struct regmap *rct_regmap;
>> > +	int ret;
>> > +
>> > +	rct_regmap = syscon_regmap_lookup_by_compatible("ambarella,rct");
>> > +	if (IS_ERR(rct_regmap)) {
>> > +		pr_err("failed to get ambarella rct regmap\n");
>> > +		return PTR_ERR(rct_regmap);
>> > +	}
>> ...
>> > +arch_initcall(ambarella_soc_init);
>>
>> It is not an error to use a chip from another manufacturer,
>> please drop the pr_err() and return success here.
>
> Ok, good to know, thanks. But we don't have other manufacturers at 
> least for now,

I care a lot about supporting multiple SoC vendors, it would seem
very rude to assume that we stop supporting everything else after
merging Ambarella support.

> and rct_regmap is need to be updated here, like sys_config and soft 
> reboot. So I think this rct regmap is still needed.

It is certainly only needed on Ambarella SoCs, no other one
has this device at the moment.

      Arnd