linux-kernel - Re: [PATCH v2 linux-next 3/3] spi: imx: Add support for SPI Slave mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170607083334.uk6pmgzdpgzfkn2i@pengutronix.de>
Date:   Wed, 7 Jun 2017 10:33:34 +0200
From:   Sascha Hauer <s.hauer@...gutronix.de>
To:     Jiada Wang <jiada_wang@...tor.com>
Cc:     broonie@...nel.org, robh+dt@...nel.org, mark.rutland@....com,
        shawnguo@...nel.org, kernel@...gutronix.de, fabio.estevam@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org
Subject: Re: [PATCH v2 linux-next 3/3] spi: imx: Add support for SPI Slave
 mode

On Fri, Jun 02, 2017 at 06:13:07AM -0700, Jiada Wang wrote:
> Hi Sascha
> 
> On 05/31/2017 05:10 AM, Sascha Hauer wrote:
> >> +
> >> +static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
> >> +{
> >> +	u32 val = 0;
> >> +	int shift = spi_imx->count % sizeof(val);
> >> +
> >> +	if (spi_imx->tx_buf) {
> >> +		if (shift) {
> >> +			memcpy(((u8 *)&val) + sizeof(val) - shift,
> >> +			       spi_imx->tx_buf, shift);
> >> +		} else {
> >> +			val = *((u32 *)spi_imx->tx_buf);
> >> +			shift = sizeof(val);
> >> +		}
> >> +		val = cpu_to_be32(val);
> >> +		spi_imx->tx_buf += shift;
> >> +	}
> >> +
> >> +	if (!shift)
> >> +		shift = sizeof(val);
> > A better name for 'shift' is n_bytes. Its value should be calculated
> > before the if (spi_imx->tx_buf) block. Then you can use memcpy for the
> > four byte case aswell.
> >
> >
> > But anyway, have you tested this function for anything with
> > spi_imx->count % sizeof(val) != 0? In this case you have to put the fill
> > the FIFO only partly somewhere. I would assume you have to do this at
> > the end of the transfer, but you do this at the beginning. Can this
> > work?
> Yes, I have tested the case when "spi_imx->count % sizeof(val) != 0", it
> works.

For the record: Yes, this seems to be right. I misread the datasheet.
When the controller is programmed to transfer 5 bytes, then it seemed
fairly logical to me that it first expects four bytes in its 32bit FIFO
and then the remaining single byte. In fact it's the other way round,
how the example of the BURST_LENGTH register description says:

0x021 A SPI burst contains the 2 LSB in first word and all 32 bits in
second word.

So the part that doesn't fill a whole FIFO width goes out first, not
last.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |