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 <[email protected]>
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.
> +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".
> +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.
> +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?
> +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.
> +#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.
Arnd
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 <[email protected]>
>
> 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
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
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