Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
On Wed, 3 Jul 2019 11:29:49 +
Naga Sureshkumar Relli wrote:
> Hi Boris,
>
> > -Original Message-
> > From: Boris Brezillon
> > Sent: Wednesday, July 3, 2019 4:37 PM
> > To: Naga Sureshkumar Relli
> > Cc: miquel.ray...@bootlin.com; helmut.gro...@intenta.de; rich...@nod.at;
> > dw...@infradead.org; computersforpe...@gmail.com; marek.va...@gmail.com;
> > bbrezil...@kernel.org; yamada.masah...@socionext.com;
> > linux-...@lists.infradead.org; linux-
> > ker...@vger.kernel.org; Michal Simek
> > Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver
> > for arm pl353
> > smc nand interface
> >
> > On Wed, 3 Jul 2019 08:57:57 +
> > Naga Sureshkumar Relli wrote:
> >
> > > Hi Boris,
> > >
> > > Thanks for the review.
> > >
> > > > -Original Message-
> > > > From: Boris Brezillon
> > > > Sent: Wednesday, July 3, 2019 11:56 AM
> > > > To: Naga Sureshkumar Relli
> > > > Cc: miquel.ray...@bootlin.com; helmut.gro...@intenta.de;
> > > > rich...@nod.at; dw...@infradead.org; computersforpe...@gmail.com;
> > > > marek.va...@gmail.com; vigne...@ti.com; bbrezil...@kernel.org;
> > > > yamada.masah...@socionext.com; linux- m...@lists.infradead.org;
> > > > linux-kernel@vger.kernel.org
> > > > Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic
> > > > driver for arm pl353 smc nand interface
> > > >
> > > > On Mon, 24 Jun 2019 22:46:30 -0600
> > > > Naga Sureshkumar Relli wrote:
> > > >
> > > >
> > > > > +
> > > > > +/**
> > > > > + * pl353_nand_exec_op_cmd - Send command to NAND device
> > > > > + * @chip:Pointer to the NAND chip info structure
> > > > > + * @subop: Pointer to array of instructions
> > > > > + * Return: Always return zero
> > > > > + */
> > > > > +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> > > > > + const struct nand_subop *subop) {
> > > > > + struct mtd_info *mtd = nand_to_mtd(chip);
> > > > > + const struct nand_op_instr *instr;
> > > > > + struct pl353_nfc_op nfc_op = {};
> > > > > + struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > > > > + unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> > > > > + unsigned long end_cmd;
> > > > > + unsigned int op_id, len;
> > > > > + bool reading;
> > > > > + u32 cmdphase_addrflags;
> > > > > +
> > > > > + pl353_nfc_parse_instructions(chip, subop, _op);
> > > > > + instr = nfc_op.data_instr;
> > > > > + op_id = nfc_op.data_instr_idx;
> > > > > + pl353_smc_clr_nand_int();
> > > > > +
> > > > > + /* Get the command phase address */
> > > > > + if (nfc_op.cmnds[1] != 0) {
> > > > > + if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > > > > + end_cmd_valid = 0;
> > > > > + else
> > > > > + end_cmd_valid = 1;
> > > >
> > > > You're testing the opcode, again. As I said several times, the
> > > > ->exec_op() implementation should be opcode agnostic, it should just
> > > > ->try
> > > > to match sequences of -- cycles.
> > > >
> > > This driver uses common function for all patterns.
> > > There was some discussion happened on v8 series
> > > https://lore.kernel.org/patchwork/patch/933639/
> > > There the comments from Miquel was to use an optional property In the
> > > pattern Matching, so with this approach, based on the command need to
> > > update the end_cmd_valid bit in command phase cycle.
> > > So in order to follow that approach, we defined a common pattern
> > > matching function And there we are checking the commands.
> > > It significantly reduces the code repetition.
> >
> > That's not what I'm talking about. I'm talking about the explicit
> > 'nfc_op.cmnds[0] ==
> > NAND_CMD_SEQIN' check, which AFAICT, is wrong, or at the very least, not
> > future-proof
> > at all.
> Ok.
> >
> > Let me see if I understand what end_cmd_valid means: it's supposed to be
> > set when the ADDR
> > cycles are fol
RE: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Hi Boris,
> -Original Message-
> From: Boris Brezillon
> Sent: Wednesday, July 3, 2019 4:37 PM
> To: Naga Sureshkumar Relli
> Cc: miquel.ray...@bootlin.com; helmut.gro...@intenta.de; rich...@nod.at;
> dw...@infradead.org; computersforpe...@gmail.com; marek.va...@gmail.com;
> bbrezil...@kernel.org; yamada.masah...@socionext.com;
> linux-...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Michal Simek
> Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for
> arm pl353
> smc nand interface
>
> On Wed, 3 Jul 2019 08:57:57 +
> Naga Sureshkumar Relli wrote:
>
> > Hi Boris,
> >
> > Thanks for the review.
> >
> > > -Original Message-
> > > From: Boris Brezillon
> > > Sent: Wednesday, July 3, 2019 11:56 AM
> > > To: Naga Sureshkumar Relli
> > > Cc: miquel.ray...@bootlin.com; helmut.gro...@intenta.de;
> > > rich...@nod.at; dw...@infradead.org; computersforpe...@gmail.com;
> > > marek.va...@gmail.com; vigne...@ti.com; bbrezil...@kernel.org;
> > > yamada.masah...@socionext.com; linux- m...@lists.infradead.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic
> > > driver for arm pl353 smc nand interface
> > >
> > > On Mon, 24 Jun 2019 22:46:30 -0600
> > > Naga Sureshkumar Relli wrote:
> > >
> > >
> > > > +
> > > > +/**
> > > > + * pl353_nand_exec_op_cmd - Send command to NAND device
> > > > + * @chip: Pointer to the NAND chip info structure
> > > > + * @subop: Pointer to array of instructions
> > > > + * Return: Always return zero
> > > > + */
> > > > +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> > > > + const struct nand_subop *subop) {
> > > > + struct mtd_info *mtd = nand_to_mtd(chip);
> > > > + const struct nand_op_instr *instr;
> > > > + struct pl353_nfc_op nfc_op = {};
> > > > + struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > > > + unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> > > > + unsigned long end_cmd;
> > > > + unsigned int op_id, len;
> > > > + bool reading;
> > > > + u32 cmdphase_addrflags;
> > > > +
> > > > + pl353_nfc_parse_instructions(chip, subop, _op);
> > > > + instr = nfc_op.data_instr;
> > > > + op_id = nfc_op.data_instr_idx;
> > > > + pl353_smc_clr_nand_int();
> > > > +
> > > > + /* Get the command phase address */
> > > > + if (nfc_op.cmnds[1] != 0) {
> > > > + if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > > > + end_cmd_valid = 0;
> > > > + else
> > > > + end_cmd_valid = 1;
> > >
> > > You're testing the opcode, again. As I said several times, the
> > > ->exec_op() implementation should be opcode agnostic, it should just
> > > ->try
> > > to match sequences of -- cycles.
> > >
> > This driver uses common function for all patterns.
> > There was some discussion happened on v8 series
> > https://lore.kernel.org/patchwork/patch/933639/
> > There the comments from Miquel was to use an optional property In the
> > pattern Matching, so with this approach, based on the command need to
> > update the end_cmd_valid bit in command phase cycle.
> > So in order to follow that approach, we defined a common pattern
> > matching function And there we are checking the commands.
> > It significantly reduces the code repetition.
>
> That's not what I'm talking about. I'm talking about the explicit
> 'nfc_op.cmnds[0] ==
> NAND_CMD_SEQIN' check, which AFAICT, is wrong, or at the very least, not
> future-proof
> at all.
Ok.
>
> Let me see if I understand what end_cmd_valid means: it's supposed to be set
> when the ADDR
> cycles are followed by a CMD cycle. You don't need to check if the first CMD
> cycle is !SEQIN
> (AKA start programming a page) to know that: just go through the flow of
> instructions in the
> subop, and check what's coming just after the ADDR instruction.
Ok. then let me update as per the flow of instructions.
>
> >
> > I understand your concern about not to check any NAND command in the
> > drivers under ->exec_op() implementation.
> > But do you see any issues/impact with
Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
On Wed, 3 Jul 2019 08:57:57 +
Naga Sureshkumar Relli wrote:
> Hi Boris,
>
> Thanks for the review.
>
> > -Original Message-
> > From: Boris Brezillon
> > Sent: Wednesday, July 3, 2019 11:56 AM
> > To: Naga Sureshkumar Relli
> > Cc: miquel.ray...@bootlin.com; helmut.gro...@intenta.de; rich...@nod.at;
> > dw...@infradead.org; computersforpe...@gmail.com; marek.va...@gmail.com;
> > vigne...@ti.com; bbrezil...@kernel.org; yamada.masah...@socionext.com;
> > linux-
> > m...@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver
> > for arm pl353
> > smc nand interface
> >
> > On Mon, 24 Jun 2019 22:46:30 -0600
> > Naga Sureshkumar Relli wrote:
> >
> >
> > > +
> > > +/**
> > > + * pl353_nand_exec_op_cmd - Send command to NAND device
> > > + * @chip:Pointer to the NAND chip info structure
> > > + * @subop: Pointer to array of instructions
> > > + * Return: Always return zero
> > > + */
> > > +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> > > + const struct nand_subop *subop) {
> > > + struct mtd_info *mtd = nand_to_mtd(chip);
> > > + const struct nand_op_instr *instr;
> > > + struct pl353_nfc_op nfc_op = {};
> > > + struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > > + unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> > > + unsigned long end_cmd;
> > > + unsigned int op_id, len;
> > > + bool reading;
> > > + u32 cmdphase_addrflags;
> > > +
> > > + pl353_nfc_parse_instructions(chip, subop, _op);
> > > + instr = nfc_op.data_instr;
> > > + op_id = nfc_op.data_instr_idx;
> > > + pl353_smc_clr_nand_int();
> > > +
> > > + /* Get the command phase address */
> > > + if (nfc_op.cmnds[1] != 0) {
> > > + if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > > + end_cmd_valid = 0;
> > > + else
> > > + end_cmd_valid = 1;
> >
> > You're testing the opcode, again. As I said several times, the
> > ->exec_op() implementation should be opcode agnostic, it should just try
> > to match sequences of -- cycles.
> >
> This driver uses common function for all patterns.
> There was some discussion happened on v8 series
> https://lore.kernel.org/patchwork/patch/933639/
> There the comments from Miquel was to use an optional property In the pattern
> Matching, so with this approach, based on the command need to update the
> end_cmd_valid bit in command phase cycle.
> So in order to follow that approach, we defined a common pattern matching
> function
> And there we are checking the commands.
> It significantly reduces the code repetition.
That's not what I'm talking about. I'm talking about the explicit
'nfc_op.cmnds[0] == NAND_CMD_SEQIN' check, which AFAICT, is wrong, or at
the very least, not future-proof at all.
Let me see if I understand what end_cmd_valid means: it's supposed to
be set when the ADDR cycles are followed by a CMD cycle. You don't need
to check if the first CMD cycle is !SEQIN (AKA start programming a page)
to know that: just go through the flow of instructions in the subop,
and check what's coming just after the ADDR instruction.
>
> I understand your concern about not to check any NAND command in the drivers
> under ->exec_op() implementation.
> But do you see any issues/impact with this?
Yes, I do. Sorry to say that, but the whole driver is coded with
specific use-cases (read/write page, read param page, etc) in mind,
which is exactly what we were trying to avoid when designing
exec_op(). The goal was to have something that's easily maintainable and
does not break every time one tests a previously untested chip <->
controller combination.
> Functionality wise Helmut tested each series and we addressed all the
> comments in v17 series.
Just because it's been tested does not mean it's ready to be merged,
sorry.
>
> Could you please let me know what do you say?
>
> > > + }
> > > +
> > > + end_cmd = nfc_op.cmnds[1];
> > > +
> > > + /*
> > > + * The SMC defines two phases of commands when transferring data to or
> > > + * from NAND flash.
> > > + * Command phase: Commands and optional address information are written
> > > + * to the NAND flash.The command and address can be associated with
> > > + * either a data phase operation to write to or read from the array,
> > > + * or a status/ID register transfer
RE: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Hi Boris,
Thanks for the review.
> -Original Message-
> From: Boris Brezillon
> Sent: Wednesday, July 3, 2019 11:56 AM
> To: Naga Sureshkumar Relli
> Cc: miquel.ray...@bootlin.com; helmut.gro...@intenta.de; rich...@nod.at;
> dw...@infradead.org; computersforpe...@gmail.com; marek.va...@gmail.com;
> vigne...@ti.com; bbrezil...@kernel.org; yamada.masah...@socionext.com; linux-
> m...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for
> arm pl353
> smc nand interface
>
> On Mon, 24 Jun 2019 22:46:30 -0600
> Naga Sureshkumar Relli wrote:
>
>
> > +
> > +/**
> > + * pl353_nand_exec_op_cmd - Send command to NAND device
> > + * @chip: Pointer to the NAND chip info structure
> > + * @subop: Pointer to array of instructions
> > + * Return: Always return zero
> > + */
> > +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> > + const struct nand_subop *subop) {
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > + const struct nand_op_instr *instr;
> > + struct pl353_nfc_op nfc_op = {};
> > + struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > + unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> > + unsigned long end_cmd;
> > + unsigned int op_id, len;
> > + bool reading;
> > + u32 cmdphase_addrflags;
> > +
> > + pl353_nfc_parse_instructions(chip, subop, _op);
> > + instr = nfc_op.data_instr;
> > + op_id = nfc_op.data_instr_idx;
> > + pl353_smc_clr_nand_int();
> > +
> > + /* Get the command phase address */
> > + if (nfc_op.cmnds[1] != 0) {
> > + if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > + end_cmd_valid = 0;
> > + else
> > + end_cmd_valid = 1;
>
> You're testing the opcode, again. As I said several times, the
> ->exec_op() implementation should be opcode agnostic, it should just try
> to match sequences of -- cycles.
>
This driver uses common function for all patterns.
There was some discussion happened on v8 series
https://lore.kernel.org/patchwork/patch/933639/
There the comments from Miquel was to use an optional property In the pattern
Matching, so with this approach, based on the command need to update the
end_cmd_valid bit in command phase cycle.
So in order to follow that approach, we defined a common pattern matching
function
And there we are checking the commands.
It significantly reduces the code repetition.
I understand your concern about not to check any NAND command in the drivers
under ->exec_op() implementation.
But do you see any issues/impact with this?
Functionality wise Helmut tested each series and we addressed all the comments
in v17 series.
Could you please let me know what do you say?
> > + }
> > +
> > + end_cmd = nfc_op.cmnds[1];
> > +
> > + /*
> > +* The SMC defines two phases of commands when transferring data to or
> > +* from NAND flash.
> > +* Command phase: Commands and optional address information are written
> > +* to the NAND flash.The command and address can be associated with
> > +* either a data phase operation to write to or read from the array,
> > +* or a status/ID register transfer.
> > +* Data phase: Data is either written to or read from the NAND flash.
> > +* This data can be either data transferred to or from the array,
> > +* or status/ID register information.
> > +*/
> > + cmdphase_addrflags = ((nfc_op.naddrs << ADDR_CYCLES_SHIFT) |
> > +(end_cmd_valid << END_CMD_VALID_SHIFT) |
> > +(COMMAND_PHASE) |
> > +(end_cmd << END_CMD_SHIFT) |
> > +(nfc_op.cmnds[0] << START_CMD_SHIFT));
> > +
> > + /* Get the data phase address */
> > + end_cmd_valid = 0;
> > +
> > + xnfc->dataphase_addrflags = ((0x0 << CLEAR_CS_SHIFT) |
> > + (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > + (DATA_PHASE) |
> > + (end_cmd << END_CMD_SHIFT) |
> > + (0x0 << ECC_LAST_SHIFT));
> > +
> > + /* Command phase AXI Read & Write */
> > + if (nfc_op.naddrs >= 5) {
> > + if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > + cmd_phase_data = nfc_op.addrs;
> > +
> > + /* Another address cycle for devices > 128MiB */
> > + if (chip->options & NAND_ROW_ADDR_3) {
>
Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
On Mon, 24 Jun 2019 22:46:30 -0600
Naga Sureshkumar Relli wrote:
> +
> +/**
> + * pl353_nand_exec_op_cmd - Send command to NAND device
> + * @chip:Pointer to the NAND chip info structure
> + * @subop: Pointer to array of instructions
> + * Return: Always return zero
> + */
> +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> + const struct nand_subop *subop)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + const struct nand_op_instr *instr;
> + struct pl353_nfc_op nfc_op = {};
> + struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> + unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> + unsigned long end_cmd;
> + unsigned int op_id, len;
> + bool reading;
> + u32 cmdphase_addrflags;
> +
> + pl353_nfc_parse_instructions(chip, subop, _op);
> + instr = nfc_op.data_instr;
> + op_id = nfc_op.data_instr_idx;
> + pl353_smc_clr_nand_int();
> +
> + /* Get the command phase address */
> + if (nfc_op.cmnds[1] != 0) {
> + if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> + end_cmd_valid = 0;
> + else
> + end_cmd_valid = 1;
You're testing the opcode, again. As I said several times, the
->exec_op() implementation should be opcode agnostic, it should just try
to match sequences of -- cycles.
> + }
> +
> + end_cmd = nfc_op.cmnds[1];
> +
> + /*
> + * The SMC defines two phases of commands when transferring data to or
> + * from NAND flash.
> + * Command phase: Commands and optional address information are written
> + * to the NAND flash.The command and address can be associated with
> + * either a data phase operation to write to or read from the array,
> + * or a status/ID register transfer.
> + * Data phase: Data is either written to or read from the NAND flash.
> + * This data can be either data transferred to or from the array,
> + * or status/ID register information.
> + */
> + cmdphase_addrflags = ((nfc_op.naddrs << ADDR_CYCLES_SHIFT) |
> + (end_cmd_valid << END_CMD_VALID_SHIFT) |
> + (COMMAND_PHASE) |
> + (end_cmd << END_CMD_SHIFT) |
> + (nfc_op.cmnds[0] << START_CMD_SHIFT));
> +
> + /* Get the data phase address */
> + end_cmd_valid = 0;
> +
> + xnfc->dataphase_addrflags = ((0x0 << CLEAR_CS_SHIFT) |
> + (end_cmd_valid << END_CMD_VALID_SHIFT) |
> + (DATA_PHASE) |
> + (end_cmd << END_CMD_SHIFT) |
> + (0x0 << ECC_LAST_SHIFT));
> +
> + /* Command phase AXI Read & Write */
> + if (nfc_op.naddrs >= 5) {
> + if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> + cmd_phase_data = nfc_op.addrs;
> +
> + /* Another address cycle for devices > 128MiB */
> + if (chip->options & NAND_ROW_ADDR_3) {
Clearly, none of this belongs in the ->exec_op() implementation. Looks
like something related to page read...
> + writel_relaxed(cmd_phase_data,
> +xnfc->regs + cmdphase_addrflags);
> + cmd_phase_data = nfc_op.addrs_56;
> + }
> + }
> + } else {
> + if (nfc_op.addrs != -1) {
> + int column = nfc_op.addrs;
> +
> + /*
> + * Change read/write column, read id etc
> + * Adjust columns for 16 bit bus width
> + */
> + if ((chip->options & NAND_BUSWIDTH_16) &&
> + (nfc_op.cmnds[0] == NAND_CMD_READ0 ||
> + nfc_op.cmnds[0] == NAND_CMD_SEQIN ||
> + nfc_op.cmnds[0] == NAND_CMD_RNDOUT ||
> + nfc_op.cmnds[0] == NAND_CMD_RNDIN)) {
> + column >>= 1;
And you keep testing opcodes here. Note that the address cycles should
have been adjusted by core already when we have 16-bit accesses.
> + }
> + cmd_phase_data = column;
> + }
> + }
> +
> + writel_relaxed(cmd_phase_data, xnfc->regs + cmdphase_addrflags);
> + if (!nfc_op.data_instr) {
> + if (nfc_op.rdy_timeout_ms) {
> + if (pl353_wait_for_dev_ready(chip))
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> + }
> +
> + reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
> + if (!reading) {
> + len = nand_subop_get_data_len(subop, op_id);
> + pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
> + len, instr->ctx.data.force_8bit);
> + if (nfc_op.rdy_timeout_ms) {
>
RE: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Hi Miquel, > -Original Message- > From: linux-mtd On Behalf Of Helmut > Grohne > Sent: Tuesday, June 25, 2019 7:41 PM > To: Naga Sureshkumar Relli > Cc: vigne...@ti.com; bbrezil...@kernel.org; yamada.masah...@socionext.com; > rich...@nod.at; linux-kernel@vger.kernel.org; marek.va...@gmail.com; linux- > m...@lists.infradead.org; miquel.ray...@bootlin.com; > computersforpe...@gmail.com; > dw...@infradead.org > Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for > arm pl353 > smc nand interface > > Thank you for the quick update. > > On Tue, Jun 25, 2019 at 06:46:30AM +0200, Naga Sureshkumar Relli wrote: > > -> Tested Micron MT29F2G08ABAEAWP (On-die capable) and AMD/Spansion > S34ML01G1. > > I tested the v17 series with the MT29F2G08ABAEAWP. I can now mount existing > jffs2 > volumes without issues. > > When running nandtest on a 64MB area, I no longer see lots of consecutive > errors. However I > see few (4-8) single byte errors for random locations on about half the runs. > In comparison, I > couldn't reproduce these on the older driver on v4.14. > > When writing random 1MB files on a fresh jffs2 filesystem and reading them > back after > umounting and mounting the filesystem, I got one faulty file in 50 attempts. > > So this driver mostly works for me, but I suspect that something (possibly > the tested > hardware) doesn't fully work yet. To say more, I'll need long term testing > results. In the mean > time, I'm in favour of merging the driver. What is your take on this? Can we consider to merge this driver? Could you please let me know? Thanks, Naga Sureshkumar Relli > > Helmut > > __ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Thank you for the quick update. On Tue, Jun 25, 2019 at 06:46:30AM +0200, Naga Sureshkumar Relli wrote: > -> Tested Micron MT29F2G08ABAEAWP (On-die capable) and AMD/Spansion S34ML01G1. I tested the v17 series with the MT29F2G08ABAEAWP. I can now mount existing jffs2 volumes without issues. When running nandtest on a 64MB area, I no longer see lots of consecutive errors. However I see few (4-8) single byte errors for random locations on about half the runs. In comparison, I couldn't reproduce these on the older driver on v4.14. When writing random 1MB files on a fresh jffs2 filesystem and reading them back after umounting and mounting the filesystem, I got one faulty file in 50 attempts. So this driver mostly works for me, but I suspect that something (possibly the tested hardware) doesn't fully work yet. To say more, I'll need long term testing results. In the mean time, I'm in favour of merging the driver. Helmut
[LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface
Add driver for arm pl353 static memory controller nand interface. This controller is used in Xilinx Zynq SoC for interfacing the NAND flash memory. Reviewed-by: Helmut Grohne Signed-off-by: Naga Sureshkumar Relli --- xilinx zynq TRM link: https://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf ARM pl353 smc TRM link: http://infocenter.arm.com/help/topic/com.arm.doc.ddi0380g/DDI0380G_smc_pl350_series_r2p1_trm.pdf -> Tested Micron MT29F2G08ABAEAWP (On-die capable) and AMD/Spansion S34ML01G1. -> Tested both x8 and x16 bus-widths. -> Tested ubifs, mtd_debug tools and mtd-tests which exists in kernel as modules. -> Tested jffs2 SMC memory controller driver is at drivers/memory/pl353-smc.c Changes in v17: - This patch fixes the comments given by Helmut Grohne - Removed struct clk *mclk from struct pl353_nand_controller, as it is required only once during probe, so declare it inside the probe - Merged NAND_OP_DATA_IN_INSTR and NAND_OP_DATA_OUT_INSTR as single case in switch - Added updated Makefile and Kconfig file - Added pl353_nand_wait_ready(), in pl353_nand_write_page_raw() to wait for write to complete Changes in v16: - Removed unnecessary comments Changes in v15: All the comments given by Helmut Grohne to v14 are addressed in this series as mentioned below. - Removed below unused macros PL353_NAND_CMD_PHASE, PL353_NAND_DATA_PHASE and PL353_NAND_ECC_CONFIG - Used cond_resched() instead of cpu_relax() to eleminate the CPU spin for a full second - changed the size of cmnds[4] to cmnds[2] - Removed the unused variable end_cmd in struct pl353_nfc_op - Added new variable u16 addrs_56, instead of u32 addr5 and u32 addr6 - Removed the unused variable cle_ale_delay_ns in struct pl353_nfc_op - Completely changed the nand_offset calculation, taken new varibale called dataphase_addrflags and eleminated the casting with __force just used offset + flags - in pl353_ecc_ooblayout64_free(), removed checking of section with ecc.steps, as section is 0 here - simplified the pl353_wait_for_dev_ready() and pl353_wait_for_ecc_done() - Updated the nfc_op->addrs calculation in pl353_nfc_parse_instructions() - Removed cond_delay(), instead used ndelay(), as it is sufficient - in pl353_nand_exec_op(), instead of assigning end_cmd twice, just assign it once by nfc_op.cmnds[1] - changed if (reading) to else in pl353_nand_exec_op() - Removed int err variable in pl353_nand_ecc_init(), instead just used single variable ret - Changed reading clock value by name rather than index in pl353_nand_probe() - Instead of always calling clk_get_rate(), stored it in the probe to a varaible and use it later Changes in v14: - Removed legacy hooks as per Miquel comments Changes in v13: - Rebased the driver to mtd/next Changes in v12: - Rebased the driver on top of v4.19 nand tree - Removed nand_scan_ident() and nand_scan_tail(), and added nand_controller_ops with ->attach_chip() and used nand_scan() instead. - Renamed pl353_nand_info structure to pl353_nand_controller - Renamed nand_base and nandaddr in pl353_nand_controller to 'regs' and 'buf_addr' - Added new API pl353_wait_for_ecc_done() to wait for ecc done and call it from pl353_nand_write_page_hwecc() and pl353_nand_read_page_hwecc() - Defined new macro for max ECC blocks - Added return value check for ecc.calculate() - Renamed pl353_nand_cmd_function() to pl353_nand_exec_op_cmd() - Added x16 bus-width support - The dependent driver pl353-smc is already reviewed and hence dropped the smc driver Changes in v11: - Removed Documentation patch and added the required info in driver as per Boris comments. - Removed unwanted variables from pl353_nand_info as per Miquel comments - Removed IO_ADDR_R/W. - Replaced onhot() with hweight32() - Defined macros for static values in function pl353_nand_correct_data() - Removed all unnecessary delays - Used nand_wait_ready() where ever is required - Modifed the pl353_setup_data_interface() logic as per Miquel comments. - Taken array instead of 7 values in pl353_setup_data_interface() and pass it to smc driver. - Added check to collect the return value of mtd_device_register(). Changes in 10: - Typos correction like nand to NAND and soc to SOC etc.. - Defined macros for the values in pl353_nand_calculate_hwecc() - Modifed ecc_status from int to char in pl353_nand_calculate_hwecc() - Changed the return type form int to bool to the function onehot() - Removed udelay(1000) in pl353_cmd_function, as it is not required - Dropped ecc->hwctl = NULL in pl353_ecc_init() - Added an error message in pl353_ecc_init(), when there is no matching oobsize - Changed the variable from xnand to xnfc - Added logic to get mtd->name from DT, if it is specified in DT Changes in v9: - Addressed the below comments given by Miquel - instead of using pl353_nand_write32, use directly writel_relaxed - Fixed check patch warnings - Renamed
