Hi Sanchayan,

The implementation looks good from my perspective. While the output
differs a bit from what i.MX6 provides, I think its closer to what is
specified. Also I like that we have the ROM revision available, since
this information is relevant to identify early versions of the chip
which have had issues...

Some minor things below.

On 2015-05-11 07:11, Sanchayan Maity wrote:
> Implements SoC bus support to export SoC specific information. Read
> the unique SoC ID from the Vybrid On Chip One Time Programmable (OCOTP)
> controller, SoC specific information from the Miscellaneous System
> Control Module (MSCM), revision from the ROM revision register and
> expose it via the SoC bus infrastructure.
> 
> Signed-off-by: Sanchayan Maity <[email protected]>
> ---
>  arch/arm/mach-imx/mach-vf610.c | 76 
> +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/mach-vf610.c b/arch/arm/mach-imx/mach-vf610.c
> index 1ba7738..74681f1 100644
> --- a/arch/arm/mach-imx/mach-vf610.c
> +++ b/arch/arm/mach-imx/mach-vf610.c
> @@ -9,13 +9,87 @@
>  
>  #include <linux/of_platform.h>
>  #include <linux/irqchip.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/random.h>
> +#include <linux/io.h>
>  #include <asm/mach/arch.h>
>  #include <asm/hardware/cache-l2x0.h>
>  #include "common.h"
>  
> +#define OCOTP_CFG0_OFFSET      0x00000410
> +#define OCOTP_CFG1_OFFSET      0x00000420
> +#define MSCM_CPxCOUNT_OFFSET   0x0000002C
> +#define MSCM_CPxCFG1_OFFSET    0x00000014
> +#define ROM_REVISION_REGISTER  0x00000080
> +
>  static void __init vf610_init_machine(void)
>  {
> -     of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +     struct regmap *ocotp_regmap, *mscm_regmap;
> +     struct soc_device_attribute *soc_dev_attr;
> +     struct device *parent = NULL;
> +     struct soc_device *soc_dev;
> +     char soc_type[] = "xx0";
> +     void __iomem *rom_rev;
> +     u32 cpxcount, cpxcfg1;
> +     u32 soc_id1, soc_id2;
> +     u64 soc_id;
> +
> +     ocotp_regmap = syscon_regmap_lookup_by_compatible("fsl,vf610-ocotp");
> +     if (IS_ERR(ocotp_regmap)) {
> +             pr_err("regmap lookup for octop failed\n");
> +             goto out;
> +     }
> +
> +     mscm_regmap = 
> syscon_regmap_lookup_by_compatible("fsl,vf610-mscm-cpucfg");
> +     if (IS_ERR(mscm_regmap)) {
> +             pr_err("regmap lookup for mscm failed");
> +             goto out;
> +     }
> +
> +     regmap_read(ocotp_regmap, OCOTP_CFG0_OFFSET, &soc_id1);
> +     regmap_read(ocotp_regmap, OCOTP_CFG1_OFFSET, &soc_id2);
> +
> +     soc_id = (u64) soc_id1 << 32 | soc_id2;
> +     add_device_randomness(&soc_id, sizeof(soc_id));
> +
> +     regmap_read(mscm_regmap, MSCM_CPxCOUNT_OFFSET, &cpxcount);
> +     regmap_read(mscm_regmap, MSCM_CPxCFG1_OFFSET, &cpxcfg1);
> +
> +     soc_type[0] = cpxcount ? '6' : '5'; /* Dual Core => VF6x0 */
> +     soc_type[1] = cpxcfg1 ? '1' : '0'; /* L2 Cache => VFx10 */
> +
> +     soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +     if (!soc_dev_attr)
> +             goto out;

This out seems not to take care of the memory allocated just above.

> +
> +     soc_dev_attr->machine = kasprintf(GFP_KERNEL, "Freescale Vybrid");
> +     soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%llx", soc_id);

I would prefer %016llx as format, that shows that we have 64 bit
identifier even when the highest bit is 0.

> +     soc_dev_attr->family = kasprintf(GFP_KERNEL, "Freescale Vybrid VF%s",
> +                                                              soc_type);
> +
> +     rom_rev = ioremap(ROM_REVISION_REGISTER, SZ_1);
> +     if (rom_rev)
> +             soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%08x",
> +                                             readl(rom_rev));

We should add the ROM to the device tree too. The memory map documented
in the RM states that the ROM is accessable at 0x0000_0000-0x007fffff,
that would be 8MiB. However, according to the RM, the on-chip ROM is
only 96KiB. I quickly checked, U-Boot crashed when reading after
0x00018000, which is the 96KiB boundary, hence I would add a DT node
with compatible fsl,vf610-ocrom or something which has a register range
from 0x0-0x00017fff.

> +
> +     soc_dev = soc_device_register(soc_dev_attr);
> +     if (IS_ERR(soc_dev)) {
> +             if (!rom_rev)
> +                     kfree(soc_dev_attr->revision);
> +             kfree(soc_dev_attr->family);
> +             kfree(soc_dev_attr->soc_id);
> +             kfree(soc_dev_attr->machine);
> +             kfree(soc_dev_attr);
> +             goto out;
> +     }
> +
> +     parent = soc_device_to_device(soc_dev);
> +
> +out:
> +     of_platform_populate(NULL, of_default_bus_match_table, NULL, parent);
>       vf610_pm_init();
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to