Re: [PATCH v1 1/2] mtd: spi-nor: add Octal DTR support for Macronix flash

[Michael Walle <michael@walle.cc>]

Hi,

> From: JaimeLiao <jaimeliao.tw@gmail.com>

You write "We" in your next patch. "We" as in macronix? Then please
use your macronix email address for the patches. Please note, you
can still send them through your gmail account.

> Enable Octal DTR mode with 20 dummy cycles to allow running
> at the maximum supported frequency for adding Macronix flash
> in Octal DTR mode.

Please explain a bit more what you are doing here. The patch itself
looks dodgy. You are writing CR2 but maybe thats used for register
accesses?! I also can't really tell from the macro names.

> 
> Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
> Co-authored-by: Tudor Ambarus <tudor.ambarus@linaro.org>

There seems to be no written process documentation wether this has
to be followed by a SoB (unlike Co-developed-by). Dunno.

> ---
>  drivers/mtd/spi-nor/macronix.c | 77 ++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c 
> b/drivers/mtd/spi-nor/macronix.c
> index 04888258e891..9010b81e098f 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,12 @@
> 
>  #include "core.h"
> 
> +#define SPINOR_OP_RD_CR2		0x71		/* Read configuration register 2 */
> +#define SPINOR_OP_WR_CR2		0x72		/* Write configuration register 2 */

Copied from spansion.c? Why isn't there an _MXIC_ in the name?

> +#define SPINOR_REG_MXIC_CR2_MODE	0x00000000	/* For setting octal DTR 
> mode */
> +#define SPINOR_REG_MXIC_OPI_DTR_EN	0x2		/* Enable Octal DTR */
> +#define SPINOR_REG_MXIC_SPI_EN		0x0		/* Enable SPI */

The names don't make much sense to me. Are you accessing individual
registers? If so please use MXIC_REG_<regname> and
MXIC_REG_<regname>_<bit/mode/mask/..>

> +
>  static int
>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>  			    const struct sfdp_parameter_header *bfpt_header,
> @@ -32,6 +38,76 @@ static const struct spi_nor_fixups mx25l25635_fixups 
> = {
>  	.post_bfpt = mx25l25635_post_bfpt_fixups,
>  };
> 
> +/**
> + * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix 
> flashes.
> + * @nor:		pointer to a 'struct spi_nor'
> + * @enable:		whether to enable Octal DTR or switch back to SPI
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool 
> enable)
> +{
> +	struct spi_mem_op op;
> +	u8 *buf = nor->bouncebuf, i;
> +	int ret;
> +
> +	/* Set/unset the octal and DTR enable bits. */
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	if (enable) {
> +		buf[0] = SPINOR_REG_MXIC_OPI_DTR_EN;
> +	} else {
> +		/*
> +		 * The register is 1-byte wide, but 1-byte transactions are not
> +		 * allowed in 8D-8D-8D mode. Since there is no register at the
> +		 * next location, just initialize the value to 0 and let the
> +		 * transaction go on.
> +		 */
> +		buf[0] = SPINOR_REG_MXIC_SPI_EN;
> +		buf[1] = 0x0;
> +	}
> +
> +	op = (struct spi_mem_op)
> +		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
> +			   SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1),
> +			   SPI_MEM_OP_NO_DUMMY,
> +			   SPI_MEM_OP_DATA_OUT(enable ? 1 : 2, buf, 1));
> +
> +	if (!enable)
> +		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +
> +	ret = spi_mem_exec_op(nor->spimem, &op);
> +	if (ret)
> +		return ret;
> +
> +	/* Read flash ID to make sure the switch was successful. */

While cleaning up the flash_info db I come around this and it is
copied all over the place. Please work on factoring this (also the
other code in micron-st.c and spansion.c) out into a helper.

> +	op = (struct spi_mem_op)
> +		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> +			   SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1),
> +			   SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1),
> +			   SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1));
> +
> +	if (enable)
> +		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +
> +	ret = spi_mem_exec_op(nor->spimem, &op);
> +	if (ret)
> +		return ret;
> +
> +	if (enable) {
> +		for (i = 0; i < nor->info->id_len; i++)
> +			if (buf[i * 2] != nor->info->id[i])
> +				return -EINVAL;

Why is the ID now swapped? Doesn't look right.

-michael

> +	} else {
> +		if (memcmp(buf, nor->info->id, nor->info->id_len))
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct flash_info macronix_nor_parts[] = {
>  	/* Macronix */
>  	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1)
> @@ -108,6 +184,7 @@ static const struct flash_info macronix_nor_parts[] 
> = {
>  static void macronix_nor_default_init(struct spi_nor *nor)
>  {
>  	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
> +	nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable;
>  }
> 
>  static void macronix_nor_late_init(struct spi_nor *nor)

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/