| 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
| ||
|
Date: Thu, 20 Aug 2015 10:52:47 +0200
From: Marek Vasut <marex@...x.de>
To: Nga Chi <ngachi86@...il.com>
Cc: "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
Viet Nga Dao <vndao@...era.com>,
Brian Norris <computersforpeace@...il.com>,
David Woodhouse <dwmw2@...radead.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [PATCH v5] mtd:spi-nor: Add Altera Quad SPI Driver
On Thursday, August 20, 2015 at 10:13:30 AM, Nga Chi wrote:
> On Thu, Aug 20, 2015 at 4:03 PM, Marek Vasut <marex@...x.de> wrote:
> > On Thursday, August 20, 2015 at 08:55:05 AM, vndao@...era.com wrote:
> >> From: VIET NGA DAO <vndao@...era.com>
> >>
> >> Altera Quad SPI Controller is a soft IP which enables access to
> >> Altera EPCS and EPCQ flash chips. This patch adds driver
> >> for these devices.
> >>
> >> Signed-off-by: VIET NGA DAO <vndao@...era.com>
> >>
> >> ---
> >> v5:
> >> - Remove Micron support
> >> - Add multiple flashes probe failure handle
> >>
> >> v4:
> >> - Add more flash devices support ( EPCQL and Micron)
> >> - Remove redundant messages
> >> - Change EPCQ_OPCODE_ID to NON_EPCS_OPCODE_ID
> >> - Replace get_flash_name to altera_quadspi_scan
> >> - Remove clk related parts
> >> - Remove altera_quadspi_plat
> >> - Change device tree reg name and remove opcode-id
> >>
> >> v3:
> >> - Change altera_epcq driver name to altera_quadspi for more generic name
> >> - Implement flash name searching in altera_quadspi.c instead of spi-nor
> >> - Edit the altra quadspi info table in spi-nor
> >> - Remove wait_til_ready in all read,write, erase, lock, unlock functions
> >> - Merge .h and .c into 1 file
> >>
> >> v2:
> >> - Change to spi_nor structure
> >> - Add lock and unlock functions for spi_nor
> >> - Simplify the altera_epcq_lock function
> >> - Replace reg by compatible in device tree
> >> ---
> >>
> >> .../devicetree/bindings/mtd/altera-quadspi.txt | 45 ++
> >> drivers/mtd/spi-nor/Kconfig | 8 +
> >> drivers/mtd/spi-nor/Makefile | 1 +
> >> drivers/mtd/spi-nor/altera-quadspi.c | 557
> >>
> >> ++++++++++++++++++++ drivers/mtd/spi-nor/spi-nor.c
> >> |
> >>
> >> 18 +
> >> 5 files changed, 629 insertions(+), 0 deletions(-)
> >> create mode 100644
> >>
> >> Documentation/devicetree/bindings/mtd/altera-quadspi.txt create mode
> >> 100644 drivers/mtd/spi-nor/altera-quadspi.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/altera-quadspi.txt
> >> b/Documentation/devicetree/bindings/mtd/altera-quadspi.txt new file mode
> >> 100644
> >> index 0000000..e1bcf18
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mtd/altera-quadspi.txt
> >> @@ -0,0 +1,45 @@
> >> +* MTD Altera QUADSPI driver
> >> +
> >> +Required properties:
> >> +- compatible: Should be "altr,quadspi-1.0"
> >> +- reg: Address and length of the register set for the device. It
> >> contains + the information of registers in the same order as described
> >> by reg-names +- reg-names: Should contain the reg names
> >> + "avl_csr": Should contain the register configuration base address
> >> + "avl_mem": Should contain the data base address
> >> +- #address-cells: Must be <1>.
> >> +- #size-cells: Must be <0>.
> >> +- flash device tree subnode, there must be a node with the following
> >> fields: + - compatible: Should contain the flash name:
> >> + 1. EPCS: epcs16, epcs64, epcs128
> >> + 2. EPCQ: epcq16, epcq32, epcq64, epcq128, epcq256, epcq512,
> >> epcq1024 + 3. EPCQ-L: epcql256, epcql512, epcql1024
> >> + - #address-cells: please refer to /mtd/partition.txt
> >> + - #size-cells: please refer to /mtd/partition.txt
> >> + For partitions inside each flash, please refer to
> >> /mtd/partition.txt +
> >> +Example:
> >> +
> >> + quadspi_controller_0: quadspi@...80014a0 {
> >> + compatible = "altr,quadspi-1.0";
> >> + reg = <0x180014a0 0x00000020>,
> >> + <0x14000000 0x04000000>;
> >> + reg-names = "avl_csr", "avl_mem";
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + flash0: epcq256@0 {
> >> + compatible = "altr,epcq256";
> >> + #address-cells = <1>;
> >> + #size-cells = <1>;
> >> + partition@0 {
> >> + /* 16 MB for raw data. */
> >> + label = "EPCQ Flash 0 raw
> >> data"; + reg = <0x0
> >> 0x1000000>; + };
> >> + partition@...0000 {
> >> + /* 16 MB for jffs2 data.
> >> */ + label = "EPCQ Flash 0
> >> JFFS 2"; + reg = <0x1000000
> >> 0x1000000>; + };
> >
> > IIRC, encoding partitions into OF is deprecated (and it shouldn't be
> > part of the example anyway, so please remove this bit).
> >
> >> + };
> >> + }; //end quadspi@...80014a0 (quadspi_controller_0)
> >
> > Remove this incorrect comment.
> >
> >
> > [...]
>
> Do you mean the partition part below should not be in example?
> partition@0 {
> /* 16 MB for raw data. */
> label = "EPCQ Flash 0 raw
> data"; reg = <0x0 0x1000000>; };
> partition@...0000 {
> /* 16 MB for jffs2 data. */
> label = "EPCQ Flash 0 JFFS 2";
> reg = <0x1000000 0x1000000>;
> };
Yes, it's not really relevant.
> >> +static struct flash_device flash_devices[] = {
> >> + FLASH_ID("epcs16", EPCS_OPCODE_ID, 0x14),
> >> + FLASH_ID("epcs64", EPCS_OPCODE_ID, 0x16),
> >> + FLASH_ID("epcs128", EPCS_OPCODE_ID, 0x18),
> >> +
> >> + FLASH_ID("epcq16", NON_EPCS_OPCODE_ID, 0x15),
> >> + FLASH_ID("epcq32", NON_EPCS_OPCODE_ID, 0x16),
> >> + FLASH_ID("epcq64", NON_EPCS_OPCODE_ID, 0x17),
> >> + FLASH_ID("epcq128", NON_EPCS_OPCODE_ID, 0x18),
> >> + FLASH_ID("epcq256", NON_EPCS_OPCODE_ID, 0x19),
> >> + FLASH_ID("epcq512", NON_EPCS_OPCODE_ID, 0x20),
> >> + FLASH_ID("epcq1024", NON_EPCS_OPCODE_ID, 0x21),
> >> +
> >> + FLASH_ID("epcql256", NON_EPCS_OPCODE_ID, 0x19),
> >> + FLASH_ID("epcql512", NON_EPCS_OPCODE_ID, 0x20),
> >> + FLASH_ID("epcql1024", NON_EPCS_OPCODE_ID, 0x21),
> >> +
> >> +};
> >
> > I think it'd be better to wait until the IP block is fixed and can
> > issue READID correctly.
>
> The hardware IP fix will take time while there are lot of customer are
> waiting for this driver.
Does that justify pushing serious crap into mainline Linux in any way ?
> That is why we decided to upstream the
> driver. If the hardware fix, there might not need to have any changes
> in driver to support or if yes, it will be just minor.
If the hardware can do proper READID opcode, this entire nonsense table
will go away and a proper integration into the SPI NOR framework will
take place.
You might consider submitting this driver for staging, but I definitely
am not a big fan of that.
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >> b/drivers/mtd/spi-nor/spi-nor.c index 14a5d23..2ab7279 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -687,6 +687,24 @@ static const struct spi_device_id spi_nor_ids[] = {
> >>
> >> { "cat25c09", CAT25_INFO( 128, 8, 32, 2, SPI_NOR_NO_ERASE |
> >>
> >> SPI_NOR_NO_FR) }, { "cat25c17", CAT25_INFO( 256, 8, 32, 2,
> >> SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, { "cat25128", CAT25_INFO(2048, 8,
> >> 64, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) }, +
> >> + /* Altera EPCQ/EPCS Flashes are non-JEDEC */
> >
> > Are they really ? Last time I checked on CV SoC, I was able to read their
> > JEDEC ID just fine ; though it's true I used that EPCS core.
> >
> >> + { "epcs16", INFO(0, 0, 0x10000, 32, 0) },
> >> + { "epcs64", INFO(0, 0, 0x10000, 128, 0) },
> >> + { "epcs128", INFO(0, 0, 0x40000, 256, 0) },
> >> +
> >> + { "epcq16", INFO(0, 0, 0x10000, 32, 0) },
> >> + { "epcq32", INFO(0, 0, 0x10000, 64, 0) },
> >> + { "epcq64", INFO(0, 0, 0x10000, 128, 0) },
> >> + { "epcq128", INFO(0, 0, 0x10000, 256, 0) },
> >> + { "epcq256", INFO(0, 0, 0x10000, 512, 0) },
> >> + { "epcq512", INFO(0, 0, 0x10000, 1024, 0) },
> >> + { "epcq1024", INFO(0, 0, 0x10000, 2048, 0) },
> >> +
> >> + { "epcql256", INFO(0, 0, 0x10000, 512, 0) },
> >> + { "epcql512", INFO(0, 0, 0x10000, 1024, 0) },
> >> + { "epcql1024", INFO(0, 0, 0x10000, 2048, 0) },
> >> +
> >>
> >> { },
> >>
> >> };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists