[RFC,3/8] mtd: rawnand: ams-delta: Set port direction once per transfer
diff mbox series

Message ID 20180718235710.18242-4-jmkrzyszt@gmail.com
State New
Headers show
Series
  • mtd: rawnand: ams-delta: Use gpio-omap accessors for data I/O
Related show

Commit Message

Janusz Krzysztofik July 18, 2018, 11:57 p.m. UTC
In its current shape, the driver sets data port direction before each
byte read/write operation, even during multi-byte transfers.  Optimize
that by setting the port direction only on first byte of each transfer.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 42 ++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

Comments

Boris Brezillon July 19, 2018, 6:23 a.m. UTC | #1
On Thu, 19 Jul 2018 01:57:05 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> In its current shape, the driver sets data port direction before each
> byte read/write operation, even during multi-byte transfers.  Optimize
> that by setting the port direction only on first byte of each transfer.

Sounds like premature optimization for something you'll rework when
fully switching to the GPIO consumer API to control the DATA bus.

> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 42 ++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 6ac38e9cfa1a..dfefcd79b420 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -72,50 +72,72 @@ static const struct mtd_partition partition_info[] = {
>  	  .size		=  3 * SZ_256K },
>  };
>  
> -static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
> +static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
>  {
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct ams_delta_nand *priv = nand_get_controller_data(this);
> -	void __iomem *io_base = priv->io_base;
>  
> -	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
>  	writew(byte, this->IO_ADDR_W);
>  	gpiod_set_value(priv->gpiod_nwe, 0);
>  	ndelay(40);
>  	gpiod_set_value(priv->gpiod_nwe, 1);
>  }
>  
> -static u_char ams_delta_read_byte(struct mtd_info *mtd)
> +static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  {
> -	u_char res;
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct ams_delta_nand *priv = nand_get_controller_data(this);
>  	void __iomem *io_base = priv->io_base;
>  
> +	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
> +
> +	ams_delta_write_next_byte(mtd, byte);
> +}
> +
> +static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +	u_char res;
> +
>  	gpiod_set_value(priv->gpiod_nre, 0);
>  	ndelay(40);
> -	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
>  	res = readw(this->IO_ADDR_R);
>  	gpiod_set_value(priv->gpiod_nre, 1);
>  
>  	return res;
>  }
>  
> +static u_char ams_delta_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct ams_delta_nand *priv = nand_get_controller_data(this);
> +	void __iomem *io_base = priv->io_base;
> +
> +	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
> +
> +	return ams_delta_read_next_byte(mtd);
> +}
> +
>  static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
>  				int len)
>  {
>  	int i;
>  
> -	for (i=0; i<len; i++)
> -		ams_delta_write_byte(mtd, buf[i]);
> +	if (len > 0)
> +		ams_delta_write_byte(mtd, buf[0]);
> +	for (i = 1; i < len; i++)
> +		ams_delta_write_next_byte(mtd, buf[i]);
>  }
>  
>  static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
>  {
>  	int i;
>  
> -	for (i=0; i<len; i++)
> -		buf[i] = ams_delta_read_byte(mtd);
> +	if (len > 0)
> +		buf[0] = ams_delta_read_byte(mtd);
> +	for (i = 1; i < len; i++)
> +		buf[i] = ams_delta_read_next_byte(mtd);
>  }
>  
>  /*

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Janusz Krzysztofik July 20, 2018, 6:12 p.m. UTC | #2
On Thursday, July 19, 2018 8:23:18 AM CEST Boris Brezillon wrote:
> On Thu, 19 Jul 2018 01:57:05 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > In its current shape, the driver sets data port direction before each
> > byte read/write operation, even during multi-byte transfers.  Optimize
> > that by setting the port direction only on first byte of each transfer.
> 
> Sounds like premature optimization for something you'll rework when
> fully switching to the GPIO consumer API to control the DATA bus.

Indeed, this optimization was crucial for getting acceptable performance of 
data transfers over GPIO.  I'm only not sure if there is any action in 
response to your comment expected on my side, e.g., did you want to say I 
should modify the patch description, or change the order of patches?

Thanks,
Janusz



--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon July 20, 2018, 7:29 p.m. UTC | #3
On Fri, 20 Jul 2018 20:12:05 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> On Thursday, July 19, 2018 8:23:18 AM CEST Boris Brezillon wrote:
> > On Thu, 19 Jul 2018 01:57:05 +0200
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> >   
> > > In its current shape, the driver sets data port direction before each
> > > byte read/write operation, even during multi-byte transfers.  Optimize
> > > that by setting the port direction only on first byte of each transfer.  
> > 
> > Sounds like premature optimization for something you'll rework when
> > fully switching to the GPIO consumer API to control the DATA bus.  
> 
> Indeed, this optimization was crucial for getting acceptable performance of 
> data transfers over GPIO.  I'm only not sure if there is any action in 
> response to your comment expected on my side, e.g., did you want to say I 
> should modify the patch description, or change the order of patches?

I'm just saying that, since you switch to a solution that goes through
the GPIO framework to control the data bus, making sure the the pin
direction change is done only once when reading/writing several bytes is
something you can do after/when transitioning to the new approach.

So yes, I suggest to re-order patches, except that this patch won't
look the same at all if you move it after the "use the GPIO consumer
API to control data bus" patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox series

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 6ac38e9cfa1a..dfefcd79b420 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -72,50 +72,72 @@  static const struct mtd_partition partition_info[] = {
 	  .size		=  3 * SZ_256K },
 };
 
-static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
+static void ams_delta_write_next_byte(struct mtd_info *mtd, u_char byte)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
-	void __iomem *io_base = priv->io_base;
 
-	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
 	writew(byte, this->IO_ADDR_W);
 	gpiod_set_value(priv->gpiod_nwe, 0);
 	ndelay(40);
 	gpiod_set_value(priv->gpiod_nwe, 1);
 }
 
-static u_char ams_delta_read_byte(struct mtd_info *mtd)
+static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 {
-	u_char res;
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct ams_delta_nand *priv = nand_get_controller_data(this);
 	void __iomem *io_base = priv->io_base;
 
+	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
+
+	ams_delta_write_next_byte(mtd, byte);
+}
+
+static u_char ams_delta_read_next_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+	u_char res;
+
 	gpiod_set_value(priv->gpiod_nre, 0);
 	ndelay(40);
-	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
 	res = readw(this->IO_ADDR_R);
 	gpiod_set_value(priv->gpiod_nre, 1);
 
 	return res;
 }
 
+static u_char ams_delta_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct ams_delta_nand *priv = nand_get_controller_data(this);
+	void __iomem *io_base = priv->io_base;
+
+	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
+
+	return ams_delta_read_next_byte(mtd);
+}
+
 static void ams_delta_write_buf(struct mtd_info *mtd, const u_char *buf,
 				int len)
 {
 	int i;
 
-	for (i=0; i<len; i++)
-		ams_delta_write_byte(mtd, buf[i]);
+	if (len > 0)
+		ams_delta_write_byte(mtd, buf[0]);
+	for (i = 1; i < len; i++)
+		ams_delta_write_next_byte(mtd, buf[i]);
 }
 
 static void ams_delta_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 {
 	int i;
 
-	for (i=0; i<len; i++)
-		buf[i] = ams_delta_read_byte(mtd);
+	if (len > 0)
+		buf[0] = ams_delta_read_byte(mtd);
+	for (i = 1; i < len; i++)
+		buf[i] = ams_delta_read_next_byte(mtd);
 }
 
 /*