Messages in this thread | | | Date | Tue, 24 Jan 2023 15:58:53 +0800 | From | Li Chen <> | Subject | Re: [PATCH 06/15] soc: add Ambarella driver |
| |
Hi Arnd,
On Mon, 23 Jan 2023 16:29:06 +0800, Arnd Bergmann wrote: > > On Mon, Jan 23, 2023, at 08:32, Li Chen wrote: > > This driver add soc_id support for Ambarella, > > which is stored inside "cpuid" AXI address mapping. > > > > Also provide sys_config(POC, aka power on configuration) > > for other drivers. > > > > Signed-off-by: Li Chen <lchen@ambarella.com> > > The soc_id support looks ok > > > Change-Id: I4869a3497366ac7779e792835f8e0309239036a8 > > Please drop these lines in the submission, the IDs are > not reachable outside of your own git, so we don't want > these to show up in the public history.
Sorry, I forgot to remove this.
> > +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?
> > +static int __init ambarella_socinfo_init(void) > > +{ > > + struct soc_device_attribute *soc_dev_attr; > > + struct soc_device *soc_dev; > > + struct device_node *np; > > + struct regmap *cpuid_regmap; > > + unsigned int soc_id; > > + > > + cpuid_regmap = syscon_regmap_lookup_by_compatible("ambarella,cpuid"); > > + if (IS_ERR(cpuid_regmap)) > > + return PTR_ERR(cpuid_regmap); > > Is there anything else in this syscon node? If the block > of registers only contains the identification bits, you > could just make this file a platform_driver that binds to > the node instead of using a syscon. > > 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".
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.
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.
> > +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.
> > +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, and rct_regmap is need to be updated here, like sys_config and soft reboot. So I think this rct regmap is still needed.
> > +#ifndef __SOC_AMBARELLA_MISC_H__ > > +#define __SOC_AMBARELLA_MISC_H__ > > + > > +extern unsigned int ambarella_sys_config(void); > > +extern struct proc_dir_entry *ambarella_proc_dir(void); > > + > > The ambarella_proc_dir looks like a stale entry that should be > removed. Ideally you should not need a private header at all.
Oops, my bad. I will remove proc dir in v2.
Regards, Li
| |