On Fri, Dec 12, 2014 at 1:45 AM, Pankaj Dubey [off-list ref] wrote:

Hi Rob,

On Thursday 11 December 2014 11:00 PM, Rob Herring wrote:

quoted

On Thu, Dec 11, 2014 at 2:07 AM, Pankaj Dubey [off-list ref]
wrote:

quoted

Exynos SoCs have Chipid, for identification of product IDs
and SoC revisions. This patch intendes to provide initialization
code for all these functionalites.

[...]

quoted

quoted

+static void __iomem *exynos_chipid_base;
+
+struct exynos_chipid_info exynos_soc_info;
+EXPORT_SYMBOL(exynos_soc_info);

The soc_device already has similar data.Why is this needed? Is it
temporary for compatibility?

struct soc_device_attribute can hold these two (product_id, and revision)
but they are defined as char * in soc_device_atttribute, and I feel it's
more specific for exposing via sysfs.

But what is exposed generically for sysfs should also be exposed
internally in the kernel generically.

Also existing code in mach-exynos compares them via product_id/revision
macros, so I can say to keep compatibility.

Perhaps you will have to change the users. Otherwise, I don't see the
point in moving this code as is. If there are a lot of users, then
having both old and new interface and moving them over one by one
would be okay. However, if this is widely used that is a problem in
itself.

quoted

For early use?

Yes, partially correct. These parameters will be required in during early
boot, from mach-exynos/platsmp.c, by that time probe of chipid would not
have happened. But usage of this is not limited to early users, even
mach-exynos/pm.c will use this later any point of time.
Since there are early users I added "exynos_chipid_early_init" which will be
called via mach-exynos.c at very early stage [1].

[1]: https://lkml.org/lkml/2014/12/11/47

quoted

If for early use, then it
should not be exported.

Other reason to make and expose this structure was we can see that other
fields of chipid bank (other than product_id and revision, which is not part
of this patch as of now) can be used by other driver such as ASV (which is
not yet part of mainline but is there for every exynos SoC).

I do not exported this in my PATCH v4 [2] of this, and instead provided
exposed functions to directly access product_id and revision, but sometime
in future when we will need other fields of chipid bank, we will end-up
adding new exported function for each new field, so decided to expose this
structure itself.

More on this below.

quoted

quoted

+void __init exynos_chipid_early_init(struct device *dev)
+{
+       struct device_node *np;
+       const struct of_device_id *match;
+
+       if (exynos_chipid_base)
+               return;
+
+       if (!dev)
+               np = of_find_matching_node_and_match(NULL,
+                       of_exynos_chipid_ids, &match);
+       else
+               np = dev->of_node;
+
+       if (!np)
+               panic("%s, failed to find chipid node\n", __func__);

Do you really want to halt booting here?

Since some critical configuration are done in platsmp.c based on product_id
and revision, I don't see any point moving ahead without it.
Even if we allow to continue here it will crash or lead to system
malfunction later during system boot for existing SoC support.

Your console may not be up to

quoted

see the panic anyway.

I feel this we can still see via earlyprintk.

You can, yes. So when you don't boot getting no messages, you then
have to recompile to enable earlyprintk for exynos, load a new kernel,
and change your command line. That's not very good from a support
point of view. It would be better to boot while failing to boot
secondary cpus than not boot printing nothing. It is much better to
provide the warnings rather than have to debug why you are not
booting.

quoted

quoted

+struct exynos_chipid_info {
+       u32 product_id;
+       u32 revision;
+};

Exposing this struct kernel wide in an SOC specific way concerns me. I
would not like to see this done on every SOC family. That would become
a mess.

As of today chip-id can live up by exposing two APIs for getting product_id
and revision, but in future when we need to access other fields we may end
up adding new exported functions/extern functions. We had a discussion about
it in Patch V4 [3].

Yes, but every SOC has an id and revision, so we should expose them in
a common way. For other fields, the mechanism to retrieve them should
probably be common, but the data could be custom.

Rob

[3]: https://lkml.org/lkml/2014/12/10/748