Re: [PATCH 1/1][v2] IFC: Change IO accessor based on endianness

Re: [PATCH 1/1][v2] IFC: Change IO accessor based on endianness

[Scott Wood]

On Mon, 2014-10-20 at 11:41 +0530, Jaiprakash Singh wrote:
> From: Jaiprakash Singh <b44839@freescale.com>
> 
> IFC registers can be of type Little Endian
> or big Endian depending upon Freescale SoC.
> Here SoC defines the register type of
> IFC IP.So update accessors functions with
> common IFC accessors functions to take
> care both type of endianness.
> 
> IFC IO accressor are set at run time based
> on IFC IP registers endianness.IFC node in
> DTS file contains information about
> endianness.
> 
> Signed-off-by: Jaiprakash Singh <b44839@freescale.com>
> ---
> Changes for v2
> 	- Moved IFC accessor function to fsl_ifc.h
> 	from fsl_ifc.c and make them inline static
> 
> .../bindings/memory-controllers/fsl/ifc.txt        |    2 +
>  drivers/memory/fsl_ifc.c                           |   72 ++++++----
>  drivers/mtd/nand/fsl_ifc_nand.c                    |  151 +++++++++++---------
>  include/linux/fsl_ifc.h                            |   42 ++++++
>  4 files changed, 167 insertions(+), 100 deletions(-)

Given that this spans MTD and non-MTD files, whose tree should this go
through?

> diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> index d5e3704..ee6226b 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> @@ -18,6 +18,8 @@ Properties:
>                interrupt (NAND_EVTER_STAT).  If there is only one,
>                that interrupt reports both types of event.
>  
> +- little-endian : If this property is absent, the big-endian mode will
> +                  be in use as default for registers.
>  
>  - ranges : Each range corresponds to a single chipselect, and covers
>             the entire access window as configured.
> diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> index 3d5d792..5689e9b 100644
> --- a/drivers/memory/fsl_ifc.c
> +++ b/drivers/memory/fsl_ifc.c
> @@ -62,7 +62,8 @@ int fsl_ifc_find(phys_addr_t addr_base)
>  		return -ENODEV;
>  
>  	for (i = 0; i < ARRAY_SIZE(fsl_ifc_ctrl_dev->regs->cspr_cs); i++) {
> -		u32 cspr = in_be32(&fsl_ifc_ctrl_dev->regs->cspr_cs[i].cspr);
> +		u32 cspr = ifc_in32(
> +				&fsl_ifc_ctrl_dev->regs->cspr_cs[i].cspr);
>  		if (cspr & CSPR_V && (cspr & CSPR_BA) ==
>  				convert_ifc_address(addr_base))
>  			return i;
> @@ -79,16 +80,16 @@ static int fsl_ifc_ctrl_init(struct fsl_ifc_ctrl *ctrl)
>  	/*
>  	 * Clear all the common status and event registers
>  	 */
> -	if (in_be32(&ifc->cm_evter_stat) & IFC_CM_EVTER_STAT_CSER)
> -		out_be32(&ifc->cm_evter_stat, IFC_CM_EVTER_STAT_CSER);
> +	if (ifc_in32(&ifc->cm_evter_stat) & IFC_CM_EVTER_STAT_CSER)
> +		ifc_out32(IFC_CM_EVTER_STAT_CSER, &ifc->cm_evter_stat);
>  
>  	/* enable all error and events */
> -	out_be32(&ifc->cm_evter_en, IFC_CM_EVTER_EN_CSEREN);
> +	ifc_out32(IFC_CM_EVTER_EN_CSEREN, &ifc->cm_evter_en);
>  
>  	/* enable all error and event interrupts */
> -	out_be32(&ifc->cm_evter_intr_en, IFC_CM_EVTER_INTR_EN_CSERIREN);
> -	out_be32(&ifc->cm_erattr0, 0x0);
> -	out_be32(&ifc->cm_erattr1, 0x0);
> +	ifc_out32(IFC_CM_EVTER_INTR_EN_CSERIREN, &ifc->cm_evter_intr_en);
> +	ifc_out32(0x0, &ifc->cm_erattr0);
> +	ifc_out32(0x0, &ifc->cm_erattr1);
>  
>  	return 0;
>  }
> @@ -127,9 +128,9 @@ static u32 check_nand_stat(struct fsl_ifc_ctrl *ctrl)
>  
>  	spin_lock_irqsave(&nand_irq_lock, flags);
>  
> -	stat = in_be32(&ifc->ifc_nand.nand_evter_stat);
> +	stat = ifc_in32(&ifc->ifc_nand.nand_evter_stat);
>  	if (stat) {
> -		out_be32(&ifc->ifc_nand.nand_evter_stat, stat);
> +		ifc_out32(stat, &ifc->ifc_nand.nand_evter_stat);
>  		ctrl->nand_stat = stat;
>  		wake_up(&ctrl->nand_wait);
>  	}
> @@ -161,36 +162,37 @@ static irqreturn_t fsl_ifc_ctrl_irq(int irqno, void *data)
>  	irqreturn_t ret = IRQ_NONE;
>  
>  	/* read for chip select error */
> -	cs_err = in_be32(&ifc->cm_evter_stat);
> +	cs_err = ifc_in32(&ifc->cm_evter_stat);
>  	if (cs_err) {
> -		dev_err(ctrl->dev, "transaction sent to IFC is not mapped to"
> -				"any memory bank 0x%08X\n", cs_err);
> +		dev_err(ctrl->dev, "transaction sent to IFC is not mapped to");
> +		dev_err(ctrl->dev, " any memory bank 0x%08X\n", cs_err);

This is an unrelated and unexplained change.

>  		/* clear the chip select error */
> -		out_be32(&ifc->cm_evter_stat, IFC_CM_EVTER_STAT_CSER);
> +		ifc_out32(IFC_CM_EVTER_STAT_CSER, &ifc->cm_evter_stat);
>  
>  		/* read error attribute registers print the error information */
> -		status = in_be32(&ifc->cm_erattr0);
> -		err_addr = in_be32(&ifc->cm_erattr1);
> -
> -		if (status & IFC_CM_ERATTR0_ERTYP_READ)
> -			dev_err(ctrl->dev, "Read transaction error"
> -				"CM_ERATTR0 0x%08X\n", status);
> -		else
> -			dev_err(ctrl->dev, "Write transaction error"
> -				"CM_ERATTR0 0x%08X\n", status);
> -
> +		status = ifc_in32(&ifc->cm_erattr0);
> +		err_addr = ifc_in32(&ifc->cm_erattr1);
> +
> +		if (status & IFC_CM_ERATTR0_ERTYP_READ) {
> +			dev_err(ctrl->dev, "Read transaction error");
> +			dev_err(ctrl->dev, " CM_ERATTR0 0x%08X\n", status);
> +		} else {
> +			dev_err(ctrl->dev, "Write transaction error");
> +			dev_err(ctrl->dev, " CM_ERATTR0 0x%08X\n", status);
> +		}
>  		err_axiid = (status & IFC_CM_ERATTR0_ERAID) >>
>  					IFC_CM_ERATTR0_ERAID_SHIFT;
> -		dev_err(ctrl->dev, "AXI ID of the error"
> -					"transaction 0x%08X\n", err_axiid);
> +		dev_err(ctrl->dev, "AXI ID of the erro");
> +		dev_err(ctrl->dev, " transaction 0x%08X\n", err_axiid);

"erro"?

Why are you splitting all of these into two dev_err() calls?  And you
don't have a newline after the first of each pair, which means the
device ID prefix is going to go in the middle of the string, without
even a space after the last word of the previous line -- and then you'll
have an extra space between the device ID prefix and the second string.
Did you test this change?

> @@ -302,19 +303,20 @@ static void fsl_ifc_run_command(struct mtd_info *mtd)
>  	int i;
>  
>  	/* set the chip select for NAND Transaction */
> -	iowrite32be(priv->bank << IFC_NAND_CSEL_SHIFT,
> +	ifc_out32(priv->bank << IFC_NAND_CSEL_SHIFT,
>  		    &ifc->ifc_nand.nand_csel);

The continuation line was aligned with priv->bank -- please preserve
that after the rename.  Likewise elsewhere.

> @@ -592,15 +596,21 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>  		 * write-protected, even when it is not.
>  		 */
>  		if (chip->options & NAND_BUSWIDTH_16)
> -			setbits16(ifc_nand_ctrl->addr, NAND_STATUS_WP);
> +			ifc_out16(
> +			(ifc_in16(ifc_nand_ctrl->addr)
> +			| (NAND_STATUS_WP)),
> +			ifc_nand_ctrl->addr);
>  		else
> -			setbits8(ifc_nand_ctrl->addr, NAND_STATUS_WP);
> +			ifc_out8(
> +			(ifc_in8(ifc_nand_ctrl->addr)
> +			| (NAND_STATUS_WP)),
> +			ifc_nand_ctrl->addr);

Please do something better than this with the whitespace.  Probably
don't try to keep it all in one statement.

> -	if (ifc_nand_ctrl->index < ifc_nand_ctrl->read_bytes) {
> -		offset = ifc_nand_ctrl->index++;
> -		return in_8(ifc_nand_ctrl->addr + offset);
> -	}
> +	if (ifc_nand_ctrl->index < ifc_nand_ctrl->read_bytes)
> +		return ifc_in8(
> +				&ifc_nand_ctrl->addr[ifc_nand_ctrl->index++]);

Unnecessary newline.

>  	dev_err(priv->dev, "%s: beyond end of buffer\n", __func__);
>  	return ERR_BYTE;
> @@ -681,7 +689,8 @@ static uint8_t fsl_ifc_read_byte16(struct mtd_info *mtd)
>  	 * next byte.
>  	 */
>  	if (ifc_nand_ctrl->index < ifc_nand_ctrl->read_bytes) {
> -		data = in_be16(ifc_nand_ctrl->addr + ifc_nand_ctrl->index);
> +		data = ifc_in16((uint16_t __iomem *)&ifc_nand_ctrl->
> +			       addr[ifc_nand_ctrl->index]);

Why do you need the cast with ifc_in16() but not in_be16()?

> @@ -885,7 +896,8 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
>  
>  	/* fill in nand_chip structure */
>  	/* set up function call table */
> -	if ((ioread32be(&ifc->cspr_cs[priv->bank].cspr)) & CSPR_PORT_SIZE_16)
> +	if ((ifc_in32(&ifc->cspr_cs[priv->bank].cspr)) &
> +			CSPR_PORT_SIZE_16)
>  		chip->read_byte = fsl_ifc_read_byte16;

If this didn't need to be wrapped with the longer name, why are you
wrapping it now?  Likewise elsewhere.

-Scott

Re: [PATCH 1/1][v2] IFC: Change IO accessor based on endianness

[Brian Norris]

On Mon, Nov 03, 2014 at 05:22:46PM -0600, Scott Wood wrote:
> On Mon, 2014-10-20 at 11:41 +0530, Jaiprakash Singh wrote:
> > From: Jaiprakash Singh <b44839@freescale.com>
> > 
> > IFC registers can be of type Little Endian
> > or big Endian depending upon Freescale SoC.
> > Here SoC defines the register type of
> > IFC IP.So update accessors functions with
> > common IFC accessors functions to take
> > care both type of endianness.
> > 
> > IFC IO accressor are set at run time based
> > on IFC IP registers endianness.IFC node in
> > DTS file contains information about
> > endianness.
> > 
> > Signed-off-by: Jaiprakash Singh <b44839@freescale.com>
> > ---
> > Changes for v2
> > 	- Moved IFC accessor function to fsl_ifc.h
> > 	from fsl_ifc.c and make them inline static
> > 
> > .../bindings/memory-controllers/fsl/ifc.txt        |    2 +
> >  drivers/memory/fsl_ifc.c                           |   72 ++++++----
> >  drivers/mtd/nand/fsl_ifc_nand.c                    |  151 +++++++++++---------
> >  include/linux/fsl_ifc.h                            |   42 ++++++
> >  4 files changed, 167 insertions(+), 100 deletions(-)
> 
> Given that this spans MTD and non-MTD files, whose tree should this go
> through?

I already have one patch that touches these same files:

  http://git.infradead.org/l2-mtd.git/commitdiff/096916610f415e07cfe71d71a391011c617be5ed

I don't actually see a MAINTAINERS entry for most of drivers/memory/, so
I didn't think that was a problem. If the (fixed) full patch can be
based on l2-mtd.git and submitted with linux-mtd@infradead.org in CC, I
can consider taking it, pending proper review.

Brian

Re: [PATCH 1/1][v2] IFC: Change IO accessor based on endianness

[Scott Wood]

On Tue, 2014-11-11 at 23:07 -0800, Brian Norris wrote:
> On Mon, Nov 03, 2014 at 05:22:46PM -0600, Scott Wood wrote:
> > On Mon, 2014-10-20 at 11:41 +0530, Jaiprakash Singh wrote:
> > > From: Jaiprakash Singh <b44839@freescale.com>
> > > 
> > > IFC registers can be of type Little Endian
> > > or big Endian depending upon Freescale SoC.
> > > Here SoC defines the register type of
> > > IFC IP.So update accessors functions with
> > > common IFC accessors functions to take
> > > care both type of endianness.
> > > 
> > > IFC IO accressor are set at run time based
> > > on IFC IP registers endianness.IFC node in
> > > DTS file contains information about
> > > endianness.
> > > 
> > > Signed-off-by: Jaiprakash Singh <b44839@freescale.com>
> > > ---
> > > Changes for v2
> > > 	- Moved IFC accessor function to fsl_ifc.h
> > > 	from fsl_ifc.c and make them inline static
> > > 
> > > .../bindings/memory-controllers/fsl/ifc.txt        |    2 +
> > >  drivers/memory/fsl_ifc.c                           |   72 ++++++----
> > >  drivers/mtd/nand/fsl_ifc_nand.c                    |  151 +++++++++++---------
> > >  include/linux/fsl_ifc.h                            |   42 ++++++
> > >  4 files changed, 167 insertions(+), 100 deletions(-)
> > 
> > Given that this spans MTD and non-MTD files, whose tree should this go
> > through?
> 
> I already have one patch that touches these same files:
> 
>   http://git.infradead.org/l2-mtd.git/commitdiff/096916610f415e07cfe71d71a391011c617be5ed
> 
> I don't actually see a MAINTAINERS entry for most of drivers/memory/, so
> I didn't think that was a problem. If the (fixed) full patch can be
> based on l2-mtd.git and submitted with linux-mtd@infradead.org in CC, I
> can consider taking it, pending proper review.

OK.

-Scott