Re: [PATCH v2 2/4] mtd: parsers: Add Qcom SMEM parser

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Nov 17, 2020 at 10:20:33PM -0600, Bjorn Andersson wrote:
> On Tue 17 Nov 11:48 CST 2020, Manivannan Sadhasivam wrote:
> 
> > NAND based Qualcomm platforms have the partition table populated in the
> > Shared Memory (SMEM). Hence, add a parser for parsing the partitions
> > from it.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/mtd/parsers/Kconfig        |   8 ++
> >  drivers/mtd/parsers/Makefile       |   1 +
> >  drivers/mtd/parsers/qcomsmempart.c | 169 +++++++++++++++++++++++++++++
> >  3 files changed, 178 insertions(+)
> >  create mode 100644 drivers/mtd/parsers/qcomsmempart.c
> > 
> > diff --git a/drivers/mtd/parsers/Kconfig b/drivers/mtd/parsers/Kconfig
> > index e72354322f62..d90c30229052 100644
> > --- a/drivers/mtd/parsers/Kconfig
> > +++ b/drivers/mtd/parsers/Kconfig
> > @@ -160,3 +160,11 @@ config MTD_REDBOOT_PARTS_READONLY
> >  	  'FIS directory' images, enable this option.
> >  
> >  endif # MTD_REDBOOT_PARTS
> > +
> > +config MTD_QCOMSMEM_PARTS
> > +	tristate "Qualcomm SMEM NAND flash partition parser"
> > +	depends on MTD_NAND_QCOM || COMPILE_TEST
> > +	depends on QCOM_SMEM
> > +	help
> > +	  This provides support for parsing partitions from Shared Memory (SMEM)
> > +	  for NAND flash on Qualcomm platforms.
> > diff --git a/drivers/mtd/parsers/Makefile b/drivers/mtd/parsers/Makefile
> > index b0c5f62f9e85..50eb0b0a2210 100644
> > --- a/drivers/mtd/parsers/Makefile
> > +++ b/drivers/mtd/parsers/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_MTD_AFS_PARTS)		+= afs.o
> >  obj-$(CONFIG_MTD_PARSER_TRX)		+= parser_trx.o
> >  obj-$(CONFIG_MTD_SHARPSL_PARTS)		+= sharpslpart.o
> >  obj-$(CONFIG_MTD_REDBOOT_PARTS)		+= redboot.o
> > +obj-$(CONFIG_MTD_QCOMSMEM_PARTS)	+= qcomsmempart.o
> > diff --git a/drivers/mtd/parsers/qcomsmempart.c b/drivers/mtd/parsers/qcomsmempart.c
> > new file mode 100644
> > index 000000000000..d8c2a3fa4dfe
> > --- /dev/null
> > +++ b/drivers/mtd/parsers/qcomsmempart.c
> > @@ -0,0 +1,169 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Qualcomm SMEM NAND flash partition parser
> > + *
> > + * Copyright (C) 2020, Linaro Ltd.
> > + */
> > +
> > +#include <linux/ctype.h>
> > +#include <linux/module.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mtd/partitions.h>
> > +#include <linux/slab.h>
> > +#include <linux/soc/qcom/smem.h>
> > +
> > +#define SMEM_AARM_PARTITION_TABLE	9
> > +#define SMEM_APPS			0
> > +
> > +#define SMEM_FLASH_PART_MAGIC1		0x55ee73aa
> > +#define SMEM_FLASH_PART_MAGIC2		0xe35ebddb
> > +#define SMEM_FLASH_PTABLE_V3		3
> > +#define SMEM_FLASH_PTABLE_V4		4
> > +#define SMEM_FLASH_PTABLE_MAX_PARTS_V3	16
> > +#define SMEM_FLASH_PTABLE_MAX_PARTS_V4	48
> > +#define SMEM_FLASH_PTABLE_HDR_LEN	(4 * sizeof(u32))
> > +#define SMEM_FLASH_PTABLE_NAME_SIZE	16
> > +
> > +/**
> > + * struct smem_flash_pentry - SMEM Flash partition entry
> > + * @name: Name of the partition
> > + * @offset: Offset in blocks
> > + * @length: Length of the partition in blocks
> > + * @attr: Flags for this partition
> > + */
> > +struct smem_flash_pentry {
> > +	char name[SMEM_FLASH_PTABLE_NAME_SIZE];
> > +	u32 offset;
> 
> It would be nice if you noted that these are little endian (using
> __le32) and used le32_to_cpu() below.
> 
> 

Good catch! Will do.

> Apart from that I think this looks really good.
> 

[...]

> > +	}
> > +
> > +	pr_debug("SMEM partition table found: ver: %d len: %d\n",
> > +		 ptable->version, ptable->numparts);
> > +	*pparts = parts;
> > +
> > +	return i;
> 
> Had to check a few times, but afaict this is just ptable->numparts in
> disguise... So how about just writing that instead?
> 

Sure, will do.

Thanks,
Mani