Re: [PATCH 3/3] mtd/ifc: Segregate IFC fcm and runtime registers

[Brian Norris <computersforpeace@gmail.com>]

Hi Raghav,

On Wed, Dec 16, 2015 at 04:12:09PM +0530, Raghav Dogra wrote:
> IFC has two set of registers viz FCM (Flash control machine)
> aka global and run time registers. These set are defined in two
> memory map PAGES. Upto IFC 1.4 PAGE size is 4 KB and from IFC2.0
> PAGE size is 64KB

So...this patch is adding new hardware support? It's not immediately
clear from the patch description. Perhaps that could use some work?

> Signed-off-by: Jaiprakash Singh <b44839@freescale.com>
> Signed-off-by: Raghav Dogra <raghav@freescale.com>
> ---
>  drivers/memory/fsl_ifc.c        | 37 ++++++++++-----------
>  drivers/mtd/nand/fsl_ifc_nand.c | 72 ++++++++++++++++++++++-------------------
>  include/linux/fsl_ifc.h         | 45 +++++++++++++++++---------

Who merges changes for the drivers/memory/ portions anyway? Is there a
maintainer? Or should I be taking these, since it touches drivers/mtd/
enough? If so, can I get an ack from a Freescale maintainer?

Mostly, the changes look good. I think this does need to be rebased on
the latest l2-mtd.git or linux-next.git, though, as there are some
conflicts. One other comment below.

>  3 files changed, 88 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> index 903c0a5..df17ead 100644
> --- a/drivers/memory/fsl_ifc.c
> +++ b/drivers/memory/fsl_ifc.c

[...]

> @@ -216,6 +216,7 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
>  {
>  	int ret = 0;
>  	int version, banks;
> +	void __iomem *addr;
>  
>  	dev_info(&dev->dev, "Freescale Integrated Flash Controller\n");
>  

[...]

> @@ -259,6 +252,14 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
>  	fsl_ifc_ctrl_dev->version = version;
>  	fsl_ifc_ctrl_dev->banks = banks;
>  
> +	addr = fsl_ifc_ctrl_dev->gregs;
> +	if (version >= FSL_IFC_VERSION_2_0_0)
> +		fsl_ifc_ctrl_dev->rregs =
> +			(struct fsl_ifc_runtime *)(addr + PGOFFSET_64K);
> +	else
> +		fsl_ifc_ctrl_dev->rregs =
> +			(struct fsl_ifc_runtime *)(addr + PGOFFSET_4K);
> +

Sparse doesn't like your casting here:

drivers/memory/fsl_ifc.c:258:41: warning: incorrect type in assignment (different address spaces) [sparse]
drivers/memory/fsl_ifc.c:258:41:    expected struct fsl_ifc_runtime [noderef] <asn:2>*rregs [sparse]
drivers/memory/fsl_ifc.c:258:41:    got struct fsl_ifc_runtime *<noident> [sparse]
drivers/memory/fsl_ifc.c:259:26: warning: cast removes address space of expression [sparse]
drivers/memory/fsl_ifc.c:261:41: warning: incorrect type in assignment (different address spaces) [sparse]
drivers/memory/fsl_ifc.c:261:41:    expected struct fsl_ifc_runtime [noderef] <asn:2>*rregs [sparse]
drivers/memory/fsl_ifc.c:261:41:    got struct fsl_ifc_runtime *<noident> [sparse]
drivers/memory/fsl_ifc.c:262:26: warning: cast removes address space of expression [sparse]

It might be better to do this, since you're bothering to add a local variable
'addr' already. This helps the __iomem annotations, and it keeps the lines
shorter too:


	addr = fsl_ifc_ctrl_dev->gregs;
	if (version >= FSL_IFC_VERSION_2_0_0)
		addr += PGOFFSET_64K;
	else
		addr += PGOFFSET_4K;
	fsl_ifc_ctrl_dev->rregs = addr;

Or anything similar.

>  	/* get the Controller level irq */
>  	fsl_ifc_ctrl_dev->irq = irq_of_parse_and_map(dev->dev.of_node, 0);
>  	if (fsl_ifc_ctrl_dev->irq == 0) {

[...]

Brian