linux-kernel - Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <OF38C348F3.6278EE64-ON48258565.0025C5C2-48258565.0025FD93@mxic.com.tw>
Date:   Mon, 11 May 2020 14:54:56 +0800
From:   masonccyang@...c.com.tw
To:     "Boris Brezillon" <boris.brezillon@...labora.com>
Cc:     broonie@...nel.org, juliensu@...c.com.tw,
        linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
        "Pratyush Yadav" <me@...avpratyush.com>, miquel.raynal@...tlin.com,
        "Pratyush Yadav" <p.yadav@...com>, richard@....at,
        tudor.ambarus@...rochip.com, vigneshr@...com
Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode


Hi Boris,


> > > > 
> > > > To clarify a bit more, the idea is that we transmit the opcode MSB 

> > > > first, just we do for the address. Assume we want to issue the 
command 
> > > > 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. 
Treat 
> > > 
> > > > is as a 1-byte value, so the MSB is the same as the LSB. We 
directly 
> > > > send 0x5 on the bus. 
> > > 
> > > There are many SPI controllers driver use "op->cmd.opcode" directly,
> > > so is spi-mxic.c.
> > > 
> > > i.e,.
> > > ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 
op->cmd.nbytes); 
> > 
> > Just because you do it doesn't mean it's right. And most controllers 
use
> > the opcode value, they don't dereference the pointer as you do here.
> > 
> > > 
> > > > 
> > > > If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming 
extension 
> > > > is invert of command). So we send the MSB (0x05) first, and LSB 
(0xFA) 
> > > > next. 
> > > 
> > > My platform is Xilinx Zynq platform which CPU is ARMv7 processor.
> > > 
> > > In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
> > > in 8D-8D-8D mode, I need to patch
> > > 
> > > i.e.,
> > > op->cmd.opcode = op->cmd.opcode | (ext << 8);
> > > 
> > > rather than your patch. 
> > 
> > Seriously, how hard is it to extract each byte from the u16 if your
> > controller needs to pass things in a different order? I mean, that's
> > already how it's done for the address cycle, so why is it a problem
> > here? This sounds like bikeshedding to me. If the order is properly
> > documented in the kernel doc, I see no problem having it grouped in 
one
> > u16, with the first cmd cycle placed in the MSB and the second one in
> > the LSB.
> 
> So, I gave it a try, and we're talking about something as simple as the
> below diff. And yes, the mxic controller needs to be patched before
> extending the cmd.opcode field, but we're talking about one driver here
> (all other drivers should be fine). Oh, and if you look a few lines 
below
> the changed lines, you'll notice that we do exactly the same for the
> address.

yup,
thanks again for your time & comments.

> 
> --->8---
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index 69491f3a515d..c3f4136a7c1d 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
>         int nio = 1, i, ret;
>         u32 ss_ctrl;
>         u8 addr[8];
> +       u8 cmd[2];
> 
>         ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
>         if (ret)
> @@ -393,7 +394,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem 
*mem,
>         writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
>                mxic->regs + HC_CFG);
> 
> -       ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
> +       for (i = 0; i < op->cmd.nbytes; i++)
> +               cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 
1));
> +
> +       ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
>         if (ret)
>                 goto out;
> 

best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================