All of lore.kernel.org
 help / color / mirror / Atom feed
* AW: (EXT) Re: [PATCH 2/2] mtd: spi-nor: micron-st: Add support for output-driver-strength
@ 2021-10-04 12:10 ` Alexander Stein
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Stein @ 2021-10-04 12:10 UTC (permalink / raw)
  To: Tudor.Ambarus, Alexander.Stein, miquel.raynal, richard, vigneshr,
	robh+dt@kernel.org, p.yadav, michael
  Cc: linux-mtd, devicetree, linux-kernel

Hello,

> Does the micron flash define the SCCR SFDP map?

As far as I can tell, it only defines the Basis SPI protocol and the 4-byte Adress Instruction Table.
Dunno when the SCCR SFDP map was added, but this flash only announces JESD216B (SFDP 1.6)

Is there some tool for dumping/decoding the SFDP available?

Best regards,
Alexander


^ permalink raw reply	[flat|nested] 6+ messages in thread

* AW: (EXT) Re: [PATCH 2/2] mtd: spi-nor: micron-st: Add support for output-driver-strength
@ 2021-10-04 12:10 ` Alexander Stein
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Stein @ 2021-10-04 12:10 UTC (permalink / raw)
  To: Tudor.Ambarus, Alexander.Stein, miquel.raynal, richard, vigneshr,
	robh+dt@kernel.org, p.yadav, michael
  Cc: linux-mtd, devicetree, linux-kernel

Hello,

> Does the micron flash define the SCCR SFDP map?

As far as I can tell, it only defines the Basis SPI protocol and the 4-byte Adress Instruction Table.
Dunno when the SCCR SFDP map was added, but this flash only announces JESD216B (SFDP 1.6)

Is there some tool for dumping/decoding the SFDP available?

Best regards,
Alexander


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: AW: (EXT) Re: [PATCH 2/2] mtd: spi-nor: micron-st: Add support for output-driver-strength
  2021-10-04 12:10 ` Alexander Stein
@ 2021-10-04 16:28   ` Tudor.Ambarus
  -1 siblings, 0 replies; 6+ messages in thread
From: Tudor.Ambarus @ 2021-10-04 16:28 UTC (permalink / raw)
  To: Alexander.Stein, Alexander.Stein, miquel.raynal, richard,
	vigneshr, robh, p.yadav, michael
  Cc: linux-mtd, devicetree, linux-kernel

On 10/4/21 3:10 PM, Alexander Stein wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello,

Hi,

> 
>> Does the micron flash define the SCCR SFDP map?
> 
> As far as I can tell, it only defines the Basis SPI protocol and the 4-byte Adress Instruction Table.
> Dunno when the SCCR SFDP map was added, but this flash only announces JESD216B (SFDP 1.6)
> 
> Is there some tool for dumping/decoding the SFDP available?
> 

you can access dump the SFDP tables via sysfs. Here's an example:

root@sama5d2-xplained:~# find / -iname spi-nor
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
/sys/devices/platform/ahb/ahb:apb/f8000000.spi/spi_master/spi0/spi0.0/spi-nor
/sys/bus/spi/drivers/spi-nor
root@sama5d2-xplained:~# ls -al /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
total 0
drwxr-xr-x 2 root root    0 Mar  9 14:51 .
drwxr-xr-x 6 root root    0 Mar  9 14:50 ..
-r--r--r-- 1 root root 4096 Mar  9 14:51 jedec_id
-r--r--r-- 1 root root 4096 Mar  9 14:51 manufacturer
-r--r--r-- 1 root root 4096 Mar  9 14:51 partname
-r--r--r-- 1 root root    0 Mar  9 14:51 sfdp
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
c22016
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
macronix
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
mx25l3233f
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp > mx25l3233f-sfdp
root@sama5d2-xplained:~# hexdump mx25l3233f-sfdp
0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
0000010 00c2 0401 0060 ff00 ffff ffff ffff ffff
0000020 ffff ffff ffff ffff ffff ffff ffff ffff
0000030 20e5 fff1 ffff 01ff eb44 6b08 3b08 bb04
0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
0000060 3600 2650 f99c 6477 cffe ffff ffff ffff
0000070

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: AW: (EXT) Re: [PATCH 2/2] mtd: spi-nor: micron-st: Add support for output-driver-strength
@ 2021-10-04 16:28   ` Tudor.Ambarus
  0 siblings, 0 replies; 6+ messages in thread
From: Tudor.Ambarus @ 2021-10-04 16:28 UTC (permalink / raw)
  To: Alexander.Stein, Alexander.Stein, miquel.raynal, richard,
	vigneshr, robh, p.yadav, michael
  Cc: linux-mtd, devicetree, linux-kernel

On 10/4/21 3:10 PM, Alexander Stein wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello,

Hi,

> 
>> Does the micron flash define the SCCR SFDP map?
> 
> As far as I can tell, it only defines the Basis SPI protocol and the 4-byte Adress Instruction Table.
> Dunno when the SCCR SFDP map was added, but this flash only announces JESD216B (SFDP 1.6)
> 
> Is there some tool for dumping/decoding the SFDP available?
> 

you can access dump the SFDP tables via sysfs. Here's an example:

root@sama5d2-xplained:~# find / -iname spi-nor
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
/sys/devices/platform/ahb/ahb:apb/f8000000.spi/spi_master/spi0/spi0.0/spi-nor
/sys/bus/spi/drivers/spi-nor
root@sama5d2-xplained:~# ls -al /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
total 0
drwxr-xr-x 2 root root    0 Mar  9 14:51 .
drwxr-xr-x 6 root root    0 Mar  9 14:50 ..
-r--r--r-- 1 root root 4096 Mar  9 14:51 jedec_id
-r--r--r-- 1 root root 4096 Mar  9 14:51 manufacturer
-r--r--r-- 1 root root 4096 Mar  9 14:51 partname
-r--r--r-- 1 root root    0 Mar  9 14:51 sfdp
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
c22016
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
macronix
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
mx25l3233f
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp > mx25l3233f-sfdp
root@sama5d2-xplained:~# hexdump mx25l3233f-sfdp
0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
0000010 00c2 0401 0060 ff00 ffff ffff ffff ffff
0000020 ffff ffff ffff ffff ffff ffff ffff ffff
0000030 20e5 fff1 ffff 01ff eb44 6b08 3b08 bb04
0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
0000060 3600 2650 f99c 6477 cffe ffff ffff ffff
0000070
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* AW: (EXT) Re: [PATCH 2/2] mtd: spi-nor: micron-st: Add support for output-driver-strength
@ 2021-10-11 14:22 ` Alexander Stein
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Stein @ 2021-10-11 14:22 UTC (permalink / raw)
  To: Pratyush Yadav, Alexander Stein
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Rob Herring, Michael Walle, Tudor Ambarus, linux-mtd, devicetree,
	linux-kernel

Hello Pratyush,

> On 08/10/21 01:18PM, Pratyush Yadav wrote:
> From: Pratyush Yadav <p.yadav@ti.com>
> > 
> > Micron flashes support this by the Bits [2:0] in the Enhanced Volatile
> > Configuration Register.
> > Checked datasheets:
> > - n25q_128mb_3v_65nm.pdf
> > - mt25t-qljs-L512-xBA-xxT.pdf
> 
> What does changing the impedance do? Does it improve latency? If we use 
> suboptimal impedance, will the flash still keep working correctly?
> 
> In other words, you need to justify why this patch is needed.

Hardware guys told me this will affect the signal qualitiy when the flash is
sending data. This depends among others on line length. If set incorrectly
voltage overshoots upon 0->1 or 1-> switch might occur. To answer to your
question if the flash will work with incorrect settings: It depends. It might
working the short term, but might fail in the long term.
Also this is completly unrelated to latency.

> > +static int micron_read_evcr(struct spi_nor *nor)
> > +{
> > +	int ret;
> > +
> > +	if (nor->spimem) {
> > +		struct spi_mem_op op =
> > +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_EVCR, 1),
> > +				   SPI_MEM_OP_NO_ADDR,
> > +				   SPI_MEM_OP_NO_DUMMY,
> > +				   SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
> 
> Are you always guaranteed to be in 1S-1S-1S mode during register write?
> 
> I would suggest calling spi_nor_spimem_setup_op() before this so that it 
> sets up all the buswidths correctly based on nor->reg_proto.

Thanks, I wasn't aware of that. I'll do that.

> > +
> > +		ret = spi_mem_exec_op(nor->spimem, &op);
> > +	} else {
> > +		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RD_EVCR, nor->bouncebuf,
> 1);
> 
> Split into 2 lines?

Will do.

> > +	}
> > +
> > +	if (ret < 0) {
> > +		dev_err(nor->dev, "error %d reading EVCR\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return nor->bouncebuf[0];
> > +}
> > +
> > +/*
> > + * Write Micron enhanced volatile configuration register
> > + * Return negative if error occurred or configuration register value
> > + */
> > +static int micron_write_evcr(struct spi_nor *nor, u8 evcr)
> > +{
> > +	nor->bouncebuf[0] = evcr;
> > +
> > +	spi_nor_write_enable(nor);
> 
> Check return code.

Will do.

> > +
> > +	if (nor->spimem) {
> > +		struct spi_mem_op op =
> > +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WD_EVCR, 1),
> > +				   SPI_MEM_OP_NO_ADDR,
> > +				   SPI_MEM_OP_NO_DUMMY,
> > +				   SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1));
> 
> Same as above.

Will do.

> > +
> > +		return spi_mem_exec_op(nor->spimem, &op);
> > +	}
> > +
> > +	return nor->controller_ops->write_reg(nor, SPINOR_OP_WD_EVCR, nor->bouncebuf,
> 1);
> 
> Same as above. Split into 2 lines?

Will do.

> > +}
> > +
> > +/*
> > + * Supported values from Enahanced Volatile COnfiguration Register (Bits
> 2:0)
> > + */
> > +static const struct micron_drive_strength drive_strength_data[] = {
> > +	{ .ohms = 90, .val = 1 },
> > +	{ .ohms = 45, .val = 3 },
> > +	{ .ohms = 20, .val = 5 },
> > +	{ .ohms = 30, .val = 7 },
> > +};
> > +
> > +static struct micron_drive_strength const *micron_st_find_drive_strength_entry(u32
> ohms)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(drive_strength_data); i++) {
> > +		if (ohms == drive_strength_data[i].ohms)
> > +			return &drive_strength_data[i];
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static void micron_st_post_sfdp(struct spi_nor *nor)
> > +{
> > +	struct device_node *np = spi_nor_get_flash_node(nor);
> > +	u32 ohms;
> > +
> > +	if (!np)
> > +		return;
> > +
> > +	if (!of_property_read_u32(np, "output-driver-strength", &ohms)) {
> > +		struct micron_drive_strength const *entry =
> > +			micron_st_find_drive_strength_entry(ohms);
> > +
> > +		if (entry) {
> > +			int evcrr = micron_read_evcr(nor);
> > +
> > +			if (evcrr >= 0) {
> 
> This is a bit confusing. Can you instead do:
> 
> if (evcrr < 0)
> 	return evcrr;
> 
> ...

Will do.

> > +				u8 evcr = (u8)(evcrr & 0xf8) | entry->val;
> 
> Don't use magic numbers. Define a bitmask, preferably using GENMASK().

Will do.

> > +
> > +				micron_write_evcr(nor, evcr);
> 
> Check return code. Do we need to abort flash probe if this fails, or can 
> we live with it, despite the suboptimal impedance?

Well, it's hard to say. It might work, see above. At least this should raise a warning
if setting the driver strength for some reason.

> > +				dev_dbg(nor->dev, "%s: EVCR 0x%x\n", __func__,
> > +					(u32)micron_read_evcr(nor));
> > +			}
> > +		} else {
> > +			dev_warn(nor->dev,
> > +				"Invalid output-driver-strength property specified: %u",
> > +				ohms);
> > +		}
> > +	}
> > +}
> > +
> >  static const struct spi_nor_fixups micron_st_fixups = {
> >  	.default_init = micron_st_default_init,
> > +	.post_sfdp = micron_st_post_sfdp,
> 
> NACK. Not every Micron flash has the EVCR register. For example, the 
> Micron MT35 flash family does not have an EVCR and the output drive 
> strength is programmed in a separate register. Set this only for the 
> flashes that need this.

Thanks for that information. I do not have the MT35 datasheet, so I wasn't aware of it.
I'll stick to MT25Q for now, which is the part number my datasheet is for.

Thanks for your feedback.

Best regards,
Alexander


^ permalink raw reply	[flat|nested] 6+ messages in thread

* AW: (EXT) Re: [PATCH 2/2] mtd: spi-nor: micron-st: Add support for output-driver-strength
@ 2021-10-11 14:22 ` Alexander Stein
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Stein @ 2021-10-11 14:22 UTC (permalink / raw)
  To: Pratyush Yadav, Alexander Stein
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Rob Herring, Michael Walle, Tudor Ambarus, linux-mtd, devicetree,
	linux-kernel

Hello Pratyush,

> On 08/10/21 01:18PM, Pratyush Yadav wrote:
> From: Pratyush Yadav <p.yadav@ti.com>
> > 
> > Micron flashes support this by the Bits [2:0] in the Enhanced Volatile
> > Configuration Register.
> > Checked datasheets:
> > - n25q_128mb_3v_65nm.pdf
> > - mt25t-qljs-L512-xBA-xxT.pdf
> 
> What does changing the impedance do? Does it improve latency? If we use 
> suboptimal impedance, will the flash still keep working correctly?
> 
> In other words, you need to justify why this patch is needed.

Hardware guys told me this will affect the signal qualitiy when the flash is
sending data. This depends among others on line length. If set incorrectly
voltage overshoots upon 0->1 or 1-> switch might occur. To answer to your
question if the flash will work with incorrect settings: It depends. It might
working the short term, but might fail in the long term.
Also this is completly unrelated to latency.

> > +static int micron_read_evcr(struct spi_nor *nor)
> > +{
> > +	int ret;
> > +
> > +	if (nor->spimem) {
> > +		struct spi_mem_op op =
> > +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_EVCR, 1),
> > +				   SPI_MEM_OP_NO_ADDR,
> > +				   SPI_MEM_OP_NO_DUMMY,
> > +				   SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
> 
> Are you always guaranteed to be in 1S-1S-1S mode during register write?
> 
> I would suggest calling spi_nor_spimem_setup_op() before this so that it 
> sets up all the buswidths correctly based on nor->reg_proto.

Thanks, I wasn't aware of that. I'll do that.

> > +
> > +		ret = spi_mem_exec_op(nor->spimem, &op);
> > +	} else {
> > +		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RD_EVCR, nor->bouncebuf,
> 1);
> 
> Split into 2 lines?

Will do.

> > +	}
> > +
> > +	if (ret < 0) {
> > +		dev_err(nor->dev, "error %d reading EVCR\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return nor->bouncebuf[0];
> > +}
> > +
> > +/*
> > + * Write Micron enhanced volatile configuration register
> > + * Return negative if error occurred or configuration register value
> > + */
> > +static int micron_write_evcr(struct spi_nor *nor, u8 evcr)
> > +{
> > +	nor->bouncebuf[0] = evcr;
> > +
> > +	spi_nor_write_enable(nor);
> 
> Check return code.

Will do.

> > +
> > +	if (nor->spimem) {
> > +		struct spi_mem_op op =
> > +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WD_EVCR, 1),
> > +				   SPI_MEM_OP_NO_ADDR,
> > +				   SPI_MEM_OP_NO_DUMMY,
> > +				   SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1));
> 
> Same as above.

Will do.

> > +
> > +		return spi_mem_exec_op(nor->spimem, &op);
> > +	}
> > +
> > +	return nor->controller_ops->write_reg(nor, SPINOR_OP_WD_EVCR, nor->bouncebuf,
> 1);
> 
> Same as above. Split into 2 lines?

Will do.

> > +}
> > +
> > +/*
> > + * Supported values from Enahanced Volatile COnfiguration Register (Bits
> 2:0)
> > + */
> > +static const struct micron_drive_strength drive_strength_data[] = {
> > +	{ .ohms = 90, .val = 1 },
> > +	{ .ohms = 45, .val = 3 },
> > +	{ .ohms = 20, .val = 5 },
> > +	{ .ohms = 30, .val = 7 },
> > +};
> > +
> > +static struct micron_drive_strength const *micron_st_find_drive_strength_entry(u32
> ohms)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(drive_strength_data); i++) {
> > +		if (ohms == drive_strength_data[i].ohms)
> > +			return &drive_strength_data[i];
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static void micron_st_post_sfdp(struct spi_nor *nor)
> > +{
> > +	struct device_node *np = spi_nor_get_flash_node(nor);
> > +	u32 ohms;
> > +
> > +	if (!np)
> > +		return;
> > +
> > +	if (!of_property_read_u32(np, "output-driver-strength", &ohms)) {
> > +		struct micron_drive_strength const *entry =
> > +			micron_st_find_drive_strength_entry(ohms);
> > +
> > +		if (entry) {
> > +			int evcrr = micron_read_evcr(nor);
> > +
> > +			if (evcrr >= 0) {
> 
> This is a bit confusing. Can you instead do:
> 
> if (evcrr < 0)
> 	return evcrr;
> 
> ...

Will do.

> > +				u8 evcr = (u8)(evcrr & 0xf8) | entry->val;
> 
> Don't use magic numbers. Define a bitmask, preferably using GENMASK().

Will do.

> > +
> > +				micron_write_evcr(nor, evcr);
> 
> Check return code. Do we need to abort flash probe if this fails, or can 
> we live with it, despite the suboptimal impedance?

Well, it's hard to say. It might work, see above. At least this should raise a warning
if setting the driver strength for some reason.

> > +				dev_dbg(nor->dev, "%s: EVCR 0x%x\n", __func__,
> > +					(u32)micron_read_evcr(nor));
> > +			}
> > +		} else {
> > +			dev_warn(nor->dev,
> > +				"Invalid output-driver-strength property specified: %u",
> > +				ohms);
> > +		}
> > +	}
> > +}
> > +
> >  static const struct spi_nor_fixups micron_st_fixups = {
> >  	.default_init = micron_st_default_init,
> > +	.post_sfdp = micron_st_post_sfdp,
> 
> NACK. Not every Micron flash has the EVCR register. For example, the 
> Micron MT35 flash family does not have an EVCR and the output drive 
> strength is programmed in a separate register. Set this only for the 
> flashes that need this.

Thanks for that information. I do not have the MT35 datasheet, so I wasn't aware of it.
I'll stick to MT25Q for now, which is the part number my datasheet is for.

Thanks for your feedback.

Best regards,
Alexander


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-10-11 14:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-04 12:10 AW: (EXT) Re: [PATCH 2/2] mtd: spi-nor: micron-st: Add support for output-driver-strength Alexander Stein
2021-10-04 12:10 ` Alexander Stein
2021-10-04 16:28 ` Tudor.Ambarus
2021-10-04 16:28   ` Tudor.Ambarus
2021-10-11 14:22 Alexander Stein
2021-10-11 14:22 ` Alexander Stein

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.