[PATCH] mmc: tmio: enable odd number size transfer

[PATCH] mmc: tmio: enable odd number size transfer

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current tmio is using sd_ctrl_read16/write16_rep()
for data transfer.
It works if transfer size was even number,
but, last 1 byte will be ignored if
transfer size was odd number.
This patch adds new tmio_mmc_transfer_data()
and solve this issue.

Tested-by: Shinobu Uehara <shinobu.uehara.xc@renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/mmc/host/tmio_mmc_pio.c |   42 +++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index ba45413..817ec7d 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -376,6 +376,43 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command
 	return 0;
 }
 
+static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
+				   unsigned short *buf,
+				   unsigned int count)
+{
+	int is_read = host->data->flags & MMC_DATA_READ;
+	u16 extra;
+	u8  *extra8;
+	u8  *buf8;
+
+	/*
+	 * Transfer the data
+	 */
+	if (is_read)
+		sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
+	else
+		sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
+
+	/* count was even number */
+	if (!(count & 0x1))
+		return;
+
+	/* count was odd number */
+
+	extra8 = (u8 *)&extra;
+	buf8 = (u8 *)(buf + ((count - 1) >> 1));
+
+	if (is_read) {
+		extra = sd_ctrl_read16(host, CTL_SD_DATA_PORT);
+
+		buf8[1] = extra8[1];
+	} else {
+		extra = buf8[1] << 8;
+
+		sd_ctrl_write16(host, CTL_SD_DATA_PORT, extra);
+	}
+}
+
 /*
  * This chip always returns (at least?) as much data as you ask for.
  * I'm unsure what happens if you ask for less than a block. This should be
@@ -408,10 +445,7 @@ static void tmio_mmc_pio_irq(struct tmio_mmc_host *host)
 		 count, host->sg_off, data->flags);
 
 	/* Transfer the data */
-	if (data->flags & MMC_DATA_READ)
-		sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
-	else
-		sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
+	tmio_mmc_transfer_data(host, buf, count);
 
 	host->sg_off += count;
 
-- 
1.7.9.5

Re: [PATCH] mmc: tmio: enable odd number size transfer

[Geert Uytterhoeven]

Hi Morimoto-san,

On Mon, Sep 8, 2014 at 6:29 AM, Kuninori Morimoto
<kuninori.morimoto.gx@gmail.com> wrote:
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -376,6 +376,43 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command
>         return 0;
>  }
>
> +static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
> +                                  unsigned short *buf,
> +                                  unsigned int count)
> +{
> +       int is_read = host->data->flags & MMC_DATA_READ;
> +       u16 extra;
> +       u8  *extra8;
> +       u8  *buf8;
> +
> +       /*
> +        * Transfer the data
> +        */
> +       if (is_read)
> +               sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
> +       else
> +               sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
> +
> +       /* count was even number */
> +       if (!(count & 0x1))
> +               return;
> +
> +       /* count was odd number */
> +
> +       extra8 = (u8 *)&extra;

I would drop the extra8 variable, if possible... (see below)

> +       buf8 = (u8 *)(buf + ((count - 1) >> 1));

The "-1" is not really needed.

> +       if (is_read) {
> +               extra = sd_ctrl_read16(host, CTL_SD_DATA_PORT);
> +
> +               buf8[1] = extra8[1];
> +       } else {
> +               extra = buf8[1] << 8;
> +
> +               sd_ctrl_write16(host, CTL_SD_DATA_PORT, extra);
> +       }
> +}

Shouldn't this use buf8[0] instead of buf8[1]?
Originally, this is a byte buffer, right? Or is it a byte-swapped 16-bit
buffer?

Does this code need to care about endianness?
Due to using byte array accesses on read, and shifts on write, the result
differs depending on endianness.

On little endian, you have

Read: extra = [ buf8[1] | buf8[0] ]

Write: extra = [ buf8[1] | 0 ]

On big endian, you have

Read: extra = [ buf8[0] | buf8[1] ]
Write: extra = [ buf8[1] | 0 ]

I think it's better to use shifts for both read and write, and add an
le16_to_cpu()/
cpu_to_le16() if needed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] mmc: tmio: enable odd number size transfer

[Kuninori Morimoto]

Hi Geert

 > +       buf8 = (u8 *)(buf + ((count - 1) >> 1));
> 
> The "-1" is not really needed.

This -1 is needed.

We want to have...
count - offset
1       0
2       0
3       1
4       1
...

> I think it's better to use shifts for both read and write, and add an
> le16_to_cpu()/
> cpu_to_le16() if needed.

I see. will fix in v2


Best regards
---
Kuninori Morimoto

Re: [PATCH] mmc: tmio: enable odd number size transfer

[Geert Uytterhoeven]

Hi Morimoto-san,

On Mon, Sep 8, 2014 at 9:36 AM, Kuninori Morimoto
<kuninori.morimoto.gx@gmail.com> wrote:
>  > +       buf8 = (u8 *)(buf + ((count - 1) >> 1));
>>
>> The "-1" is not really needed.
>
> This -1 is needed.
>
> We want to have...
> count - offset
> 1       0
> 2       0
> 3       1
> 4       1
> ...

If count is even, we never get that far, so the even numbers should not
be present in your table above?

If count is odd, "(count - 1) >> 1 == count >> 1".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] mmc: tmio: enable odd number size transfer

[Kuninori Morimoto]

Hi Geert

> If count is even, we never get that far, so the even numbers should not
> be present in your table above?
> 
> If count is odd, "(count - 1) >> 1 == count >> 1".

Ahh, yes, indeed.

About endianness / byte access, it requires byte access
especially "read" case.
Because if count was odd number,
using shift with u16 will might be buffer overflow

I need to check cpu_to_le16() / le16_to_cpu()

Best regards
---
Kuninori Morimoto

Re: [PATCH] mmc: tmio: enable odd number size transfer

[Geert Uytterhoeven]

Hi Morimoto-san,

On Mon, Sep 8, 2014 at 10:06 AM, Kuninori Morimoto
<kuninori.morimoto.gx@gmail.com> wrote:
> About endianness / byte access, it requires byte access
> especially "read" case.
> Because if count was odd number,
> using shift with u16 will might be buffer overflow

Of course the buffer must be accessed using byte accesses.
But sd_ctrl_read16() returns u16, and sd_ctrl_write16() takes u16.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] mmc: tmio: enable odd number size transfer

[Sergei Shtylyov]

Hello.

On 9/8/2014 8:29 AM, Kuninori Morimoto wrote:

> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> Current tmio is using sd_ctrl_read16/write16_rep()
> for data transfer.
> It works if transfer size was even number,
> but, last 1 byte will be ignored if
> transfer size was odd number.
> This patch adds new tmio_mmc_transfer_data()
> and solve this issue.

> Tested-by: Shinobu Uehara <shinobu.uehara.xc@renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   drivers/mmc/host/tmio_mmc_pio.c |   42 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 38 insertions(+), 4 deletions(-)

> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index ba45413..817ec7d 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -376,6 +376,43 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command
>   	return 0;
>   }
>
> +static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
> +				   unsigned short *buf,
> +				   unsigned int count)
> +{
> +	int is_read = host->data->flags & MMC_DATA_READ;
> +	u16 extra;
> +	u8  *extra8;
> +	u8  *buf8;
> +
> +	/*
> +	 * Transfer the data
> +	 */
> +	if (is_read)
> +		sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
> +	else
> +		sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
> +
> +	/* count was even number */
> +	if (!(count & 0x1))
> +		return;
> +
> +	/* count was odd number */
> +
> +	extra8 = (u8 *)&extra;
> +	buf8 = (u8 *)(buf + ((count - 1) >> 1));

    You don't need to subtract 1.

> +
> +	if (is_read) {
> +		extra = sd_ctrl_read16(host, CTL_SD_DATA_PORT);
> +
> +		buf8[1] = extra8[1];
> +	} else {
> +		extra = buf8[1] << 8;

    Hm, why [1] here and above? Shouldn't it be [0] instead?

> +
> +		sd_ctrl_write16(host, CTL_SD_DATA_PORT, extra);
> +	}
> +}
> +
>   /*
>    * This chip always returns (at least?) as much data as you ask for.
>    * I'm unsure what happens if you ask for less than a block. This should be

WBR, Sergei