| 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
| ||
|
Message-ID: <83a4e27a-b96c-0558-fbb5-10e64f9bf59b@adaptrum.com>
Date: Mon, 31 Jul 2017 10:17:17 -0700
From: Alexandru Gagniuc <alex.g@...ptrum.com>
To: Marek Vasut <marek.vasut@...il.com>,
linux-snps-arc@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc: David Woodhouse <dwmw2@...radead.org>,
Brian Norris <computersforpeace@...il.com>,
Boris Brezillon <boris.brezillon@...e-electrons.com>,
Richard Weinberger <richard@....at>,
Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI
controller
On 07/29/2017 02:34 AM, Marek Vasut wrote:
> On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
>> Add support for the QSPI controller found in Adaptrum Anarion SOCs.
>> This controller is designed specifically to handle SPI NOR chips, and
>> the driver is modeled as such.
>>
>> Because the system is emulated on an FPGA, we don't have a way to test
>> all the hardware adjustemts, so only basic features are implemented at
>> this time.
>>
Hi Marek,
Thank you very much for the comments. I am going to fix most issues you
pointed out in v2. I do have some follow-up question on some of your
comments, so please bear with me. I hope I have responded to all your
comments, but feel free to nudge me if I missed any.
[snip]
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/anarion-quadspi.txt
>> @@ -0,0 +1,22 @@
>> +* Adaptrum Anarion Quad SPI controller
>> +
>> +Required properties:
>> +- compatible : Should be "adaptrum,anarion-qspi".
>> +- reg : Contains two entries, each of which is a tuple consisting of a
>> + physical address and length. The first entry is the address and
>> + length of the controller register set. The second entry is the
>> + address and length of the memory-mapped flash. This second region is
>> + the region where the controller responds to XIP requests, and may be
>> + larger than the size of the attached flash.
>
> You want to split the bindings into separate patch and CC Rob to review
> them.
Yes, I do want that now!
[snip]
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 293c8a4..98dc012 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -48,6 +48,13 @@ config SPI_ATMEL_QUADSPI
>> This driver does not support generic SPI. The implementation only
>> supports SPI NOR.
>>
>> +config SPI_ANARION_QUADSPI
>> + tristate "Adaptrum Anarion Quad SPI Controller"
>> + depends on OF && HAS_IOMEM
>> + help
>> + Enable support for the Adaptrum Anarion Quad SPI controller.
>> + This driver does not support generic SPI. It only supports SPI NOR.
>
> Keep the list sorted.
Staged for [PATCH v2].
>> config SPI_CADENCE_QUADSPI
>> tristate "Cadence Quad SPI controller"
>> depends on OF && (ARM || COMPILE_TEST)
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index 285aab8..53635f6 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,6 +1,7 @@
>> obj-$(CONFIG_MTD_SPI_NOR) += spi-nor.o
>> obj-$(CONFIG_SPI_ASPEED_SMC) += aspeed-smc.o
>> obj-$(CONFIG_SPI_ATMEL_QUADSPI) += atmel-quadspi.o
>> +obj-$(CONFIG_SPI_ANARION_QUADSPI) += anarion-quadspi.o
>
> DTTO, N is before S and T .
Staged for [PATCH v2].
>> obj-$(CONFIG_SPI_CADENCE_QUADSPI) += cadence-quadspi.o
>> obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o
>> obj-$(CONFIG_SPI_HISI_SFC) += hisi-sfc.o
>> diff --git a/drivers/mtd/spi-nor/anarion-quadspi.c b/drivers/mtd/spi-nor/anarion-quadspi.c
>> new file mode 100644
>> index 0000000..d981356
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/anarion-quadspi.c
>> @@ -0,0 +1,490 @@
>> +/*
>> + * Adaptrum Anarion Quad SPI controller driver
>> + *
>> + * Copyright (C) 2017, Adaptrum, Inc.
>> + * (Written by Alexandru Gagniuc <alex.g at adaptrum.com> for Adaptrum, Inc.)
>> + * Licensed under the GPLv2 or (at your option) any later version.
>
> The GPL boilerplate should be here.
I chose this form of the boilerplate because it seems to be quite used
in other places. I am assuming the fatter boilerplate the requirement
for drivers/mtd, correct?
[snip]
>> +#define ASPI_CLK_SW_RESET (1 << 0)
>
> BIT(0) , fix globally
Staged for [PATCH v2].
>> +#define ASPI_CLK_RESET_BUF (1 << 1)
>> +#define ASPI_CLK_RESET_ALL (ASPI_CLK_SW_RESET | ASPI_CLK_RESET_BUF)
>> +#define ASPI_CLK_SPI_MODE3 (1 << 2)
>> +#define ASPI_CLOCK_DIV_MASK (0xff << 8)
>> +#define ASPI_CLOCK_DIV(d) (((d) << 8) & ASPI_CLOCK_DIV_MASK)
>> +
>> +#define ASPI_TIMEOUT_US 100000
>> +
>> +#define ASPI_DATA_LEN_MASK 0x3fff
>> +#define ASPI_MAX_XFER_LEN (size_t)(ASPI_DATA_LEN_MASK + 1)
>> +
>> +#define MODE_IO_X1 (0 << 16)
>> +#define MODE_IO_X2 (1 << 16)
>> +#define MODE_IO_X4 (2 << 16)
>> +#define MODE_IO_SDR_POS_SKEW (0 << 20)
>> +#define MODE_IO_SDR_NEG_SKEW (1 << 20)
>> +#define MODE_IO_DDR_34_SKEW (2 << 20)
>> +#define MODE_IO_DDR_PN_SKEW (3 << 20)
>> +#define MODE_IO_DDR_DQS (5 << 20)
>> +
>> +#define ASPI_STATUS_BUSY (1 << 2)
>> +
>> +/*
>> + * This mask does not match reality. Get over it:
>
> What is this about ?
Each stage of the QSPI chain has two registers. The second register has
a bitfield which takes in the length of the stage. For example, for
DATA2, we can set the length up to 0x4000, but for ADDR2, we can only
set a max of 4 bytes. I wrote this comment as a reminder to myself to be
careful about using this mask. I'll rephrase the comment for [v2]
>> + * DATA2: 0x3fff
>> + * CMD2: 0x0003
>> + * ADDR2: 0x0007
>> + * PERF2: 0x0000
>> + * HI_Z: 0x003f
>> + * BCNT: 0x0007
>> + */
>> +#define CHAIN_LEN(x) ((x - 1) & ASPI_DATA_LEN_MASK)
>> +
>> +struct anarion_qspi {
>> + struct spi_nor nor;
>> + struct device *dev;
>> + uintptr_t regbase;
>
> Should be void __iomem * I guess ?
I chose uintptr_t as opposed to void *, because arithmetic on void * is
not valid in C. What is the right answer hen, without risking undefined
behavior?
>
>> + uintptr_t xipbase;
>> + uint32_t xfer_mode_cmd;
>
> u32 etc, fix globally, this is not userspace.
From coding-style, section 5.(d), my understanding is that
"Linux-specific u8/u16/u32/u64 types [...] are not mandatory in new
code". Most of the code in this driver is shared between Linux, u-boot,
openocd, ASIC validation tests, and manufacturing tests. Unlike,
shortint types, stdint types are available in all cases.
Therefore, having to use a different set of primitive types makes code
sharing much more difficult, and increases the maintenance burden, hence
the strong preference for standard types. Is this reasonable?
[snip]
>> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf, size_t len)
>> +{
>> + uint32_t data;
>
> Is this stuff below something like ioread32_rep() ?
>
>> + aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>> + while (len >= 4) {
>> + data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>> + memcpy(buf, &data, sizeof(data));
>> + buf += 4;
>> + len -= 4;
>> + }
That is very similar to ioread32_rep, yes. I kept this as for the
reasons outlined above, but changing this to _rep() seems innocent enough.
>> + if (len) {
>> + aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, len);
>> + data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>> + memcpy(buf, &data, len);
>> + }
>> +}
>> +
>> +static void aspi_seed_fifo(struct anarion_qspi *spi,
>> + const uint8_t *buf, size_t len)
>> +{
>> + uint32_t data;
>> +
>> + aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>> + while (len >= 4) {
>> + memcpy(&data, buf, sizeof(data));
>> + aspi_write_reg(spi, ASPI_REG_DATA1, data);
>
> iowrite32_rep ?
>
[snip]
>> +/* While we could send read commands manually to the flash chip, we'd have to
>> + * get data back through the DATA2 register. That is on the AHB bus, whereas
>> + * XIP reads go over AXI. Hence, we use the memory-mapped flash space for read.
>> + * TODO: Look at using DMA instead of memcpy().
>> + */
>
> Multiline comment looks like this,
> /*
> * foo
> * bar
> */
>
Staged for [PATCH v2].
[snip]
>> +static ssize_t anarion_spi_nor_write(struct spi_nor *nor, loff_t to,
>> + size_t len, const uint8_t *src)
>> +{
>> + int ret;
>> + struct anarion_qspi *aspi = nor->priv;
>> +
>> + dev_err(aspi->dev, "%s, @0x%llx + %zu\n", __func__, to, len);
>
> Drop this.
>
OOPS! That wasn't supposed to be there.
Staged for [PATCH v2].
[snip]
>> + switch (nor->flash_read) {
>> + default: /* Fall through */
>
> This will break once we add OSPI support ...
Ooh, I see the API here has changed significantly from the 4.9 LTS
branch where we originally developed the driver. I will add and test
normal and FAST_READ support, but I won't have the bandwidth to test
other modes yet. Those will have to remain as a TODO.
>> + case SPI_NOR_NORMAL:
>> + aspi->num_hi_z_clocks = nor->read_dummy;
>> + aspi->xfer_mode_cmd = MODE_IO_X1;
>> + aspi->xfer_mode_addr = MODE_IO_X1;
>> + aspi->xfer_mode_data = MODE_IO_X1;
>> + break;
>> + case SPI_NOR_FAST:
>> + aspi->num_hi_z_clocks = nor->read_dummy;
>> + aspi->xfer_mode_cmd = MODE_IO_X1;
>> + aspi->xfer_mode_addr = MODE_IO_X1;
>> + aspi->xfer_mode_data = MODE_IO_X1;
>> + break;
>> + case SPI_NOR_DUAL:
>> + aspi->num_hi_z_clocks = nor->read_dummy;
>> + aspi->xfer_mode_cmd = MODE_IO_X1;
>> + aspi->xfer_mode_addr = MODE_IO_X1;
>> + aspi->xfer_mode_data = MODE_IO_X2;
>> + break;
>> + case SPI_NOR_QUAD:
>> + aspi->num_hi_z_clocks = nor->read_dummy;
>> + aspi->xfer_mode_cmd = MODE_IO_X1;
>> + aspi->xfer_mode_addr = MODE_IO_X1;
>> + aspi->xfer_mode_data = MODE_IO_X4;
>> + break;
>> + }
>> +
>> + aspi_setup_xip_read_chain(aspi, nor);
>> +
>> + mtd_device_register(&aspi->nor.mtd, NULL, 0);
>> +
>> + return 0;
>> +}
[snip]
Powered by blists - more mailing lists