Re: [PATCH 10/22] mtd: physmap_of: add a hook for Gemini flash probing

From: Marek Vasut
Date: Sun Jan 22 2017 - 09:47:21 EST


On 01/22/2017 01:21 PM, Linus Walleij wrote:
> In order to support device tree probing of Gemini NOR flash
> chips, a certain register in the syscon needs to be poked
> to enable parallel flash mode.
>
> Such things used to happen in "necessarily different" board
> file code, and this indeed was also done for the Gemini, so
> the MTD driver could treat it as any memory-mapped NOR flash,
> but this is not the way in the future: board files need to
> go, and hardware concerns distributed down to the applicable
> drivers.
>
> This adds a hook in the same way that the Versatile did: if
> the Kconfig symbol is not selected the net total of supporting
> Gemini should be zero bytes of added code. To live up to this
> promise, also the return value error print from the Versatile
> extra probe call get to be removed in this patch, all printing
> need to happen in the add-ons.
>
> Cc: Janos Laube <janos.dev@xxxxxxxxx>
> Cc: Paulius Zaleckas <paulius.zaleckas@xxxxxxxxx>
> Cc: Hans Ulli Kroll <ulli.kroll@xxxxxxxxxxxxxx>
> Cc: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> MTD maintainers: please just apply this to the MTD tree when
> you are happy with it. It is functionally orthogonal to the rest
> of the series and is just in the series for context.
> ---
> drivers/mtd/maps/Kconfig | 10 +++
> drivers/mtd/maps/Makefile | 3 +
> drivers/mtd/maps/physmap_of.c | 9 ++-
> drivers/mtd/maps/physmap_of_gemini.c | 120 +++++++++++++++++++++++++++++++++++
> drivers/mtd/maps/physmap_of_gemini.h | 16 +++++
> 5 files changed, 155 insertions(+), 3 deletions(-)
> create mode 100644 drivers/mtd/maps/physmap_of_gemini.c
> create mode 100644 drivers/mtd/maps/physmap_of_gemini.h
>
> diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> index 5bcc896a48c3..e9206060c598 100644
> --- a/drivers/mtd/maps/Kconfig
> +++ b/drivers/mtd/maps/Kconfig
> @@ -84,6 +84,16 @@ config MTD_PHYSMAP_OF_VERSATILE
> platforms, basically to add a VPP (write protection) callback so
> the flash can be taken out of write protection.
>
> +config MTD_PHYSMAP_OF_GEMINI
> + bool "Support Gemini physmap OF"

I understand the description is basically copied from versatile, but
it's really cryptic. But maybe this can be fixed with subsequent patch
alongside versatile.

> + depends on MTD_PHYSMAP_OF
> + depends on MFD_SYSCON
> + default ARCH_GEMINI
> + help
> + This provides some extra DT physmap parsing for the Gemini
> + platforms, some detection and setting up parallel mode on the
> + external interface.
> +
> config MTD_PMC_MSP_EVM
> tristate "CFI Flash device mapped on PMC-Sierra MSP"
> depends on PMC_MSP && MTD_CFI
> diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
> index 644f7d36d35d..36fe05ec9fab 100644
> --- a/drivers/mtd/maps/Makefile
> +++ b/drivers/mtd/maps/Makefile
> @@ -21,6 +21,9 @@ obj-$(CONFIG_MTD_PHYSMAP_OF) += physmap_of.o
> ifdef CONFIG_MTD_PHYSMAP_OF_VERSATILE
> obj-$(CONFIG_MTD_PHYSMAP_OF) += physmap_of_versatile.o
> endif
> +ifdef CONFIG_MTD_PHYSMAP_OF_GEMINI
> +obj-$(CONFIG_MTD_PHYSMAP_OF) += physmap_of_gemini.o
> +endif

Can CONFIG_MTD_PHYSMAP_OF_GEMINI be selected without
CONFIG_MTD_PHYSMAP_OF being selected ? I don't think
so, since CONFIG_MTD_PHYSMAP_OF_GEMINI depends on
CONFIG_MTD_PHYSMAP_OF . So do we really need the ifdef?

> obj-$(CONFIG_MTD_PISMO) += pismo.o
> obj-$(CONFIG_MTD_PMC_MSP_EVM) += pmcmsp-flash.o
> obj-$(CONFIG_MTD_PCMCIA) += pcmciamtd.o

[...]

> +int of_flash_probe_gemini(struct platform_device *pdev,
> + struct device_node *np,
> + struct map_info *map)
> +{
> + static struct regmap *rmap;
> + struct device *dev = &pdev->dev;
> + u32 val;
> + int ret;
> +
> + /* Multiplatform guard */
> + if (!of_device_is_compatible(np, "cortina,gemini-flash"))
> + return 0;
> +
> + rmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> + if (IS_ERR(rmap)) {
> + dev_err(dev, "no syscon\n");
> + return PTR_ERR(rmap);
> + }
> +
> + ret = regmap_read(rmap, GLOBAL_STATUS, &val);
> + if (ret) {
> + dev_err(dev, "failed to read global status register\n");
> + return -ENODEV;
> + }
> + dev_info(dev, "global status reg: %08x\n", val);

dev_dbg() ?

> + /*
> + * It would be contradictory if a physmap flash was NOT parallel.
> + */
> + if ((val & FLASH_TYPE_MASK) != FLASH_TYPE_PARALLEL) {
> + dev_err(dev, "flash is not parallel\n");
> + return -ENODEV;
> + }
> +

[...]

--
Best regards,
Marek Vasut