Message-ID: <877cx59568.wl-me@linux.beauty>
Date:   Sun, 29 Jan 2023 15:21:51 +0800
From:   Li Chen <me@...ux.beauty>
To:     "Arnd Bergmann" <arnd@...db.de>
Cc:     Heiko Stübner <heiko@...ech.de>,
        "Lubomir Rintel" <lkundrak@...sk>,
        "Conor.Dooley" <conor.dooley@...rochip.com>,
        "Robert Jarzmik" <robert.jarzmik@...e.fr>,
        "Sven Peter" <sven@...npeter.dev>,
        "Yinbo Zhu" <zhuyinbo@...ngson.cn>,
        "Brian Norris" <briannorris@...omium.org>,
        "Hitomi Hasegawa" <hasegawa-hitomi@...itsu.com>,
        "open list" <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Ambarella SoC support" 
        <linux-arm-kernel@...ts.infradead.org>
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

Powered by blists - more mailing lists