Messages in this thread |  | | Date | Sun, 29 Jan 2023 15:21:51 +0800 | From | Li Chen <> | Subject | Re: [PATCH 06/15] soc: add Ambarella driver |
| |
Hi Arnd,
Sorry for late reply.
On Tue, 24 Jan 2023 23:46:06 +0800, Arnd Bergmann wrote: > > 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.
Ok, I will add "Ambarella" prefix to ->family.
> >> 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.
Ok, I will switch to builtin platform driver. Which compatible do you prefer?
compatible = "ambarella,cpuid" or compatible = "ambarella,<SoC>-cpuid"
> > 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?
Agree with you.
> >> > +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.
Agree.
> What information do they actually > need here that is not already in their own DT nodes or > in the soc_device_attributes?
sys_config from rct_regmap is not available for sd/nand node neither soc_device_attribute.
But given that I will switch dts model to(see https://www.spinics.net/lists/arm-kernel/msg1043684.html):
rct | nand | sd | ...
Then I can easily and naturally get sys_config via syscon_node_to_regmap(of_get_parent(nand_np/sd_np)).
So this is not a problem anymore and I will switch to builtin platform driver in v2.
> >> > +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.
I'm really sorry that I forgot that this builtin arch_initcall code would run on SoCs other than Ambarella.
I will remove the err output in v2.
Regards, Li
|  |