Date:   Tue, 24 Jan 2023 15:58:53 +0800
Message-ID: <871qnkicsi.wl-me@linux.beauty>
From:   Li Chen <me@linux.beauty>
To:     "Arnd Bergmann" <arnd@arndb.de>
Cc:     "Li Chen" <lchen@ambarella.com>,
        Heiko =?ISO-8859-1?Q?St=FCbner?= <heiko@sntech.de>,
        "Lubomir Rintel" <lkundrak@v3.sk>,
        "Conor.Dooley" <conor.dooley@microchip.com>,
        "Robert Jarzmik" <robert.jarzmik@free.fr>,
        "Sven Peter" <sven@svenpeter.dev>,
        "Yinbo Zhu" <zhuyinbo@loongson.cn>,
        "Brian Norris" <briannorris@chromium.org>,
        "Hitomi Hasegawa" <hasegawa-hitomi@fujitsu.com>,
        "open list" <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/Ambarella SoC support" 
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 06/15] soc: add Ambarella driver
In-Reply-To: <85b86d06-c63c-4481-a3dd-16b72572a5ee@app.fastmail.com>
References: <20230123073305.149940-1-lchen@ambarella.com>
        <20230123073305.149940-7-lchen@ambarella.com>
        <85b86d06-c63c-4481-a3dd-16b72572a5ee@app.fastmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0
 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


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