[PATCH 0/2] mtd: spi-nor: Add flash device reset support

[PATCH 0/2] mtd: spi-nor: Add flash device reset support

[Sai Krishna Potthuri]

This patch series update spi-nor dt-binding and spi-nor core to support
flash device reset using reset-gpios property.

Sai Krishna Potthuri (2):
  dt-bindings: mtd: spi-nor: Add reset-gpios property
  mtd: spi-nor: Add support for flash reset

 .../bindings/mtd/jedec,spi-nor.yaml           |  6 +++
 drivers/mtd/spi-nor/core.c                    | 50 +++++++++++++++++--
 2 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.17.1


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

[PATCH 1/2] dt-bindings: mtd: spi-nor: Add reset-gpios property

[Sai Krishna Potthuri]

SPI-NOR flashes have RESET pin which can be toggled using GPIO
controller, for those platforms reset-gpios property can be used to
reset the flash device.

Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
---
 Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
index 7149784a36ac..d2fc8e9c787f 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
@@ -70,6 +70,12 @@ properties:
       be used on such systems, to denote the absence of a reliable reset
       mechanism.
 
+  reset-gpios:
+    description:
+      contains a GPIO specifier. The reset GPIO is asserted and then deasserted
+      to perform device reset. If "broken-flash-reset" is present then having
+      this property does not make any difference.
+
   partitions:
     type: object
 
-- 
2.17.1


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

[PATCH 2/2] mtd: spi-nor: Add support for flash reset

[Sai Krishna Potthuri]

Add support for spi-nor flash reset via GPIO controller by reading the
reset-gpio property. If there is a valid GPIO specifier then reset will
be performed by asserting and deasserting the GPIO using gpiod APIs
otherwise it will not perform any operation.

Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
---
 drivers/mtd/spi-nor/core.c | 50 +++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f2c64006f8d7..d4703ff69ad0 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2401,12 +2401,8 @@ static void spi_nor_no_sfdp_init_params(struct spi_nor *nor)
  */
 static void spi_nor_init_flags(struct spi_nor *nor)
 {
-	struct device_node *np = spi_nor_get_flash_node(nor);
 	const u16 flags = nor->info->flags;
 
-	if (of_property_read_bool(np, "broken-flash-reset"))
-		nor->flags |= SNOR_F_BROKEN_RESET;
-
 	if (flags & SPI_NOR_SWP_IS_VOLATILE)
 		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
 
@@ -2933,9 +2929,47 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
 	mtd->_put_device = spi_nor_put_device;
 }
 
+static int spi_nor_hw_reset(struct spi_nor *nor)
+{
+	struct gpio_desc *reset;
+	int ret;
+
+	reset = devm_gpiod_get_optional(nor->dev, "reset", GPIOD_ASIS);
+	if (IS_ERR_OR_NULL(reset))
+		return PTR_ERR_OR_ZERO(reset);
+
+	/* Set the direction as output and enable the output */
+	ret = gpiod_direction_output(reset, 1);
+	if (ret)
+		return ret;
+
+	/*
+	 * Experimental Minimum Chip select high to Reset delay value
+	 * based on the flash device spec.
+	 */
+	usleep_range(1, 5);
+	gpiod_set_value(reset, 0);
+
+	/*
+	 * Experimental Minimum Reset pulse width value based on the
+	 * flash device spec.
+	 */
+	usleep_range(10, 15);
+	gpiod_set_value(reset, 1);
+
+	/*
+	 * Experimental Minimum Reset recovery delay value based on the
+	 * flash device spec.
+	 */
+	usleep_range(35, 40);
+
+	return 0;
+}
+
 int spi_nor_scan(struct spi_nor *nor, const char *name,
 		 const struct spi_nor_hwcaps *hwcaps)
 {
+	struct device_node *np = spi_nor_get_flash_node(nor);
 	const struct flash_info *info;
 	struct device *dev = nor->dev;
 	struct mtd_info *mtd = &nor->mtd;
@@ -2965,6 +2999,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (!nor->bouncebuf)
 		return -ENOMEM;
 
+	if (of_property_read_bool(np, "broken-flash-reset")) {
+		nor->flags |= SNOR_F_BROKEN_RESET;
+	} else {
+		ret = spi_nor_hw_reset(nor);
+		if (ret)
+			return ret;
+	}
+
 	info = spi_nor_get_flash_info(nor, name);
 	if (IS_ERR(info))
 		return PTR_ERR(info);
-- 
2.17.1


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

Re: [PATCH 2/2] mtd: spi-nor: Add support for flash reset

[Michael Walle]

Hi,

Am 2022-08-29 11:05, schrieb Sai Krishna Potthuri:
> Add support for spi-nor flash reset via GPIO controller by reading the
> reset-gpio property. If there is a valid GPIO specifier then reset will
> be performed by asserting and deasserting the GPIO using gpiod APIs
> otherwise it will not perform any operation.
> 
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> ---
>  drivers/mtd/spi-nor/core.c | 50 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f2c64006f8d7..d4703ff69ad0 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2401,12 +2401,8 @@ static void spi_nor_no_sfdp_init_params(struct
> spi_nor *nor)
>   */
>  static void spi_nor_init_flags(struct spi_nor *nor)
>  {
> -	struct device_node *np = spi_nor_get_flash_node(nor);
>  	const u16 flags = nor->info->flags;
> 
> -	if (of_property_read_bool(np, "broken-flash-reset"))
> -		nor->flags |= SNOR_F_BROKEN_RESET;
> -
>  	if (flags & SPI_NOR_SWP_IS_VOLATILE)
>  		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
> 
> @@ -2933,9 +2929,47 @@ static void spi_nor_set_mtd_info(struct spi_nor 
> *nor)
>  	mtd->_put_device = spi_nor_put_device;
>  }
> 
> +static int spi_nor_hw_reset(struct spi_nor *nor)
> +{
> +	struct gpio_desc *reset;
> +	int ret;
> +
> +	reset = devm_gpiod_get_optional(nor->dev, "reset", GPIOD_ASIS);

devm_gpiod_get_optional(nor->dev, "reset", GPIOD_OUT_HIGH);

> +	if (IS_ERR_OR_NULL(reset))
> +		return PTR_ERR_OR_ZERO(reset);
> +
> +	/* Set the direction as output and enable the output */
> +	ret = gpiod_direction_output(reset, 1);

Not necessary then.

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Experimental Minimum Chip select high to Reset delay value
> +	 * based on the flash device spec.
> +	 */

Which flash device spec?

> +	usleep_range(1, 5);
> +	gpiod_set_value(reset, 0);

Mh, is your logic inverted here? If I read the code correctly,
you should use a value of 1 to take the device into reset. The
device tree should then have a flag "active low", which will
invert the value here. Also please use the cansleep() variant.

> +	/*
> +	 * Experimental Minimum Reset pulse width value based on the
> +	 * flash device spec.
> +	 */
> +	usleep_range(10, 15);
> +	gpiod_set_value(reset, 1);
> +
> +	/*
> +	 * Experimental Minimum Reset recovery delay value based on the
> +	 * flash device spec.
> +	 */
> +	usleep_range(35, 40);
> +
> +	return 0;
> +}
> +
>  int spi_nor_scan(struct spi_nor *nor, const char *name,
>  		 const struct spi_nor_hwcaps *hwcaps)
>  {
> +	struct device_node *np = spi_nor_get_flash_node(nor);
>  	const struct flash_info *info;
>  	struct device *dev = nor->dev;
>  	struct mtd_info *mtd = &nor->mtd;
> @@ -2965,6 +2999,14 @@ int spi_nor_scan(struct spi_nor *nor, const char 
> *name,
>  	if (!nor->bouncebuf)
>  		return -ENOMEM;
> 
> +	if (of_property_read_bool(np, "broken-flash-reset")) {
> +		nor->flags |= SNOR_F_BROKEN_RESET;
> +	} else {
> +		ret = spi_nor_hw_reset(nor);
> +		if (ret)
> +			return ret;
> +	}

This should be done unconditionally, no? Even if the reset
pin is broken, we know we have one (otherwise the device
tree would be broken) and we can do a reset in any case.

Also, which tree are you using? That was moved into
spi_nor_init_flags() some time ago. Please rebase to latest
spi-next.

-michael

> +
>  	info = spi_nor_get_flash_info(nor, name);
>  	if (IS_ERR(info))
>  		return PTR_ERR(info);

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

RE: [PATCH 2/2] mtd: spi-nor: Add support for flash reset

[Potthuri, Sai Krishna]

Hi Michael,

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Monday, August 29, 2022 3:35 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>
> Cc: Tudor Ambarus <tudor.ambarus@microchip.com>; Pratyush Yadav
> <pratyush@kernel.org>; Miquel Raynal <miquel.raynal@bootlin.com>;
> Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> <vigneshr@ti.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; devicetree@vger.kernel.org; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org;
> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH 2/2] mtd: spi-nor: Add support for flash reset
> 
> Hi,
> 
> Am 2022-08-29 11:05, schrieb Sai Krishna Potthuri:
> > Add support for spi-nor flash reset via GPIO controller by reading the
> > reset-gpio property. If there is a valid GPIO specifier then reset will
> > be performed by asserting and deasserting the GPIO using gpiod APIs
> > otherwise it will not perform any operation.
> >
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > ---
> >  drivers/mtd/spi-nor/core.c | 50
> +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 46 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index f2c64006f8d7..d4703ff69ad0 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2401,12 +2401,8 @@ static void spi_nor_no_sfdp_init_params(struct
> > spi_nor *nor)
> >   */
> >  static void spi_nor_init_flags(struct spi_nor *nor)
> >  {
> > -	struct device_node *np = spi_nor_get_flash_node(nor);
> >  	const u16 flags = nor->info->flags;
> >
> > -	if (of_property_read_bool(np, "broken-flash-reset"))
> > -		nor->flags |= SNOR_F_BROKEN_RESET;
> > -
> >  	if (flags & SPI_NOR_SWP_IS_VOLATILE)
> >  		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
> >
> > @@ -2933,9 +2929,47 @@ static void spi_nor_set_mtd_info(struct spi_nor
> > *nor)
> >  	mtd->_put_device = spi_nor_put_device;
> >  }
> >
> > +static int spi_nor_hw_reset(struct spi_nor *nor)
> > +{
> > +	struct gpio_desc *reset;
> > +	int ret;
> > +
> > +	reset = devm_gpiod_get_optional(nor->dev, "reset", GPIOD_ASIS);
> 
> devm_gpiod_get_optional(nor->dev, "reset", GPIOD_OUT_HIGH);
> 
> > +	if (IS_ERR_OR_NULL(reset))
> > +		return PTR_ERR_OR_ZERO(reset);
> > +
> > +	/* Set the direction as output and enable the output */
> > +	ret = gpiod_direction_output(reset, 1);
> 
> Not necessary then.
Agree, I will fix in v2.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Experimental Minimum Chip select high to Reset delay value
> > +	 * based on the flash device spec.
> > +	 */
> 
> Which flash device spec?
I referred some of the qspi, ospi flash vendors datasheets like Micron,
Macronix, ISSI, gigadevice, spansion.
> 
> > +	usleep_range(1, 5);
> > +	gpiod_set_value(reset, 0);
> 
> Mh, is your logic inverted here? If I read the code correctly,
> you should use a value of 1 to take the device into reset. The
> device tree should then have a flag "active low", which will
Reset Sequence which I implemented here is high(1)->low(0)->high(1).
By doing this sequence (active low), flash device is getting reset, this sequence
is tested using Octal SPI flash device.

> invert the value here. Also please use the cansleep() variant.
Ok, I will use gpiod_set_value_cansleep() in v2.
> 
> > +	/*
> > +	 * Experimental Minimum Reset pulse width value based on the
> > +	 * flash device spec.
> > +	 */
> > +	usleep_range(10, 15);
> > +	gpiod_set_value(reset, 1);
> > +
> > +	/*
> > +	 * Experimental Minimum Reset recovery delay value based on the
> > +	 * flash device spec.
> > +	 */
> > +	usleep_range(35, 40);
> > +
> > +	return 0;
> > +}
> > +
> >  int spi_nor_scan(struct spi_nor *nor, const char *name,
> >  		 const struct spi_nor_hwcaps *hwcaps)
> >  {
> > +	struct device_node *np = spi_nor_get_flash_node(nor);
> >  	const struct flash_info *info;
> >  	struct device *dev = nor->dev;
> >  	struct mtd_info *mtd = &nor->mtd;
> > @@ -2965,6 +2999,14 @@ int spi_nor_scan(struct spi_nor *nor, const char
> > *name,
> >  	if (!nor->bouncebuf)
> >  		return -ENOMEM;
> >
> > +	if (of_property_read_bool(np, "broken-flash-reset")) {
> > +		nor->flags |= SNOR_F_BROKEN_RESET;
> > +	} else {
> > +		ret = spi_nor_hw_reset(nor);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> This should be done unconditionally, no? Even if the reset
> pin is broken, we know we have one (otherwise the device
> tree would be broken) and we can do a reset in any case.
Agree, we can do it unconditionally without checking for reset
pin broken. If we have reset specifier in the device tree, then we
can do the reset.
I will update in v2.

> 
> Also, which tree are you using? That was moved into
> spi_nor_init_flags() some time ago. Please rebase to latest
> spi-next.
Yes, reset pin broken property moved to spi_nor_init_flags().
I moved this from spi_nor_init_flags() to spi_nor_scan() (which is part of this
patch) to decide on the flash reset part.
With the above agreement (reset not to depend on reset broken pin), I will
Revert this change.

Regards
Sai Krishna
> 
> -michael
> 
> > +
> >  	info = spi_nor_get_flash_info(nor, name);
> >  	if (IS_ERR(info))
> >  		return PTR_ERR(info);

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

Re: [PATCH 2/2] mtd: spi-nor: Add support for flash reset

[Michael Walle]

Hi,

Am 2022-08-30 08:32, schrieb Potthuri, Sai Krishna:

>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	/*
>> > +	 * Experimental Minimum Chip select high to Reset delay value
>> > +	 * based on the flash device spec.
>> > +	 */
>> 
>> Which flash device spec?
> I referred some of the qspi, ospi flash vendors datasheets like Micron,
> Macronix, ISSI, gigadevice, spansion.

Please mention here that you've looked at datasheets of different 
vendors.
And maybe instead of doing three comments, just one and then the reset
sequence.

>> 
>> > +	usleep_range(1, 5);
>> > +	gpiod_set_value(reset, 0);
>> 
>> Mh, is your logic inverted here? If I read the code correctly,
>> you should use a value of 1 to take the device into reset. The
>> device tree should then have a flag "active low", which will
> Reset Sequence which I implemented here is high(1)->low(0)->high(1).
> By doing this sequence (active low), flash device is getting reset,
> this sequence
> is tested using Octal SPI flash device.

How does the device tree property for your look like?
Has it the GPIO_ACTIVE_LOW flag set?

-michael

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

Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: Add reset-gpios property

[Krzysztof Kozlowski]

On 29/08/2022 12:05, Sai Krishna Potthuri wrote:
> SPI-NOR flashes have RESET pin which can be toggled using GPIO
> controller, for those platforms reset-gpios property can be used to
> reset the flash device.
> 
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> ---
>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> index 7149784a36ac..d2fc8e9c787f 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> @@ -70,6 +70,12 @@ properties:
>        be used on such systems, to denote the absence of a reliable reset
>        mechanism.
>  
> +  reset-gpios:
> +    description:
> +      contains a GPIO specifier. 

Skip this part - obvious.

> The reset GPIO is asserted and then deasserted
> +      to perform device reset. If "broken-flash-reset" is present then having
> +      this property does not make any difference.

Isn't then broken-flash-reset conflicting with this one (e.g.
disallowing it)?

Best regards,
Krzysztof

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

Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: Add reset-gpios property

[Michael Walle]

Am 2022-08-30 11:21, schrieb Krzysztof Kozlowski:
> On 29/08/2022 12:05, Sai Krishna Potthuri wrote:
>> SPI-NOR flashes have RESET pin which can be toggled using GPIO
>> controller, for those platforms reset-gpios property can be used to
>> reset the flash device.
>> 
>> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
>> ---
>>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml 
>> b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>> index 7149784a36ac..d2fc8e9c787f 100644
>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>> @@ -70,6 +70,12 @@ properties:
>>        be used on such systems, to denote the absence of a reliable 
>> reset
>>        mechanism.
>> 
>> +  reset-gpios:
>> +    description:
>> +      contains a GPIO specifier.
> 
> Skip this part - obvious.
> 
>> The reset GPIO is asserted and then deasserted
>> +      to perform device reset. If "broken-flash-reset" is present 
>> then having
>> +      this property does not make any difference.
> 
> Isn't then broken-flash-reset conflicting with this one (e.g.
> disallowing it)?

Sometimes the spi-nor driver needs to switch modes, which are persistent
until you either switch em back or do a hardware reset (or software
reset IIRC) of the flash. If broken-flash-reset is set, we try hard
to leave the flash in the mode which it is normally in after reset or
don't switch modes at all.
Of course we cannot make sure, our shutdown gets called in each case,
thus there is may be warning during startup.

So, even if you have a reset-gpio it might be broken I guess. Think
of it being high active, but someone forgot the pull-up. So, if you
do an unexpected reset, the flash chip might not be reset
automatically. So yes, I think, even if there is a dedicated reset
gpio, it could still be messed up. How likely is it? I don't know,
probably not very.

-michael

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

Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: Add reset-gpios property

[Krzysztof Kozlowski]

On 30/08/2022 12:36, Michael Walle wrote:
> Am 2022-08-30 11:21, schrieb Krzysztof Kozlowski:
>> On 29/08/2022 12:05, Sai Krishna Potthuri wrote:
>>> SPI-NOR flashes have RESET pin which can be toggled using GPIO
>>> controller, for those platforms reset-gpios property can be used to
>>> reset the flash device.
>>>
>>> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
>>> ---
>>>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml 
>>> b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>>> index 7149784a36ac..d2fc8e9c787f 100644
>>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>>> @@ -70,6 +70,12 @@ properties:
>>>        be used on such systems, to denote the absence of a reliable 
>>> reset
>>>        mechanism.
>>>
>>> +  reset-gpios:
>>> +    description:
>>> +      contains a GPIO specifier.
>>
>> Skip this part - obvious.
>>
>>> The reset GPIO is asserted and then deasserted
>>> +      to perform device reset. If "broken-flash-reset" is present 
>>> then having
>>> +      this property does not make any difference.
>>
>> Isn't then broken-flash-reset conflicting with this one (e.g.
>> disallowing it)?
> 
> Sometimes the spi-nor driver needs to switch modes, which are persistent
> until you either switch em back or do a hardware reset (or software
> reset IIRC) of the flash. If broken-flash-reset is set, we try hard
> to leave the flash in the mode which it is normally in after reset or
> don't switch modes at all.
> Of course we cannot make sure, our shutdown gets called in each case,
> thus there is may be warning during startup.
> 
> So, even if you have a reset-gpio it might be broken I guess. Think
> of it being high active, but someone forgot the pull-up. So, if you
> do an unexpected reset, the flash chip might not be reset
> automatically. So yes, I think, even if there is a dedicated reset
> gpio, it could still be messed up. How likely is it? I don't know,
> probably not very.

OK, so let's keep it and allow both.

Best regards,
Krzysztof

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

RE: [PATCH 2/2] mtd: spi-nor: Add support for flash reset

[Potthuri, Sai Krishna]

Hi Michael,

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Tuesday, August 30, 2022 12:40 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>
> Cc: Tudor Ambarus <tudor.ambarus@microchip.com>; Pratyush Yadav
> <pratyush@kernel.org>; Miquel Raynal <miquel.raynal@bootlin.com>;
> Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> <vigneshr@ti.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; devicetree@vger.kernel.org; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org;
> saikrishna12468@gmail.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH 2/2] mtd: spi-nor: Add support for flash reset
> 
> Hi,
> 
> Am 2022-08-30 08:32, schrieb Potthuri, Sai Krishna:
> 
> >> > +	if (ret)
> >> > +		return ret;
> >> > +
> >> > +	/*
> >> > +	 * Experimental Minimum Chip select high to Reset delay value
> >> > +	 * based on the flash device spec.
> >> > +	 */
> >>
> >> Which flash device spec?
> > I referred some of the qspi, ospi flash vendors datasheets like
> > Micron, Macronix, ISSI, gigadevice, spansion.
> 
> Please mention here that you've looked at datasheets of different vendors.
> And maybe instead of doing three comments, just one and then the reset
> sequence.
Ok, I will update in v2.
> 
> >>
> >> > +	usleep_range(1, 5);
> >> > +	gpiod_set_value(reset, 0);
> >>
> >> Mh, is your logic inverted here? If I read the code correctly, you
> >> should use a value of 1 to take the device into reset. The device
> >> tree should then have a flag "active low", which will
> > Reset Sequence which I implemented here is high(1)->low(0)->high(1).
> > By doing this sequence (active low), flash device is getting reset,
> > this sequence is tested using Octal SPI flash device.
> 
> How does the device tree property for your look like?
> Has it the GPIO_ACTIVE_LOW flag set?
Sorry, I misunderstand your initial ask, This logic is developed by keeping
the GPIO_ACTIVE_HIGH flag set in the device-tree. Agree this will violate the
flash reset sequence (active low reset).
I will update the sequence in v2 to match with the "active low" flag in the
device tree.

Regards
Sai Krishna
> 
> -michael

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

Re: [PATCH 2/2] mtd: spi-nor: Add support for flash reset

[Takahiro Kuwano]

Hello,

On 8/29/2022 6:05 PM, Sai Krishna Potthuri wrote:
> Add support for spi-nor flash reset via GPIO controller by reading the
> reset-gpio property. If there is a valid GPIO specifier then reset will
> be performed by asserting and deasserting the GPIO using gpiod APIs
> otherwise it will not perform any operation.
> 
> Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> ---
>  drivers/mtd/spi-nor/core.c | 50 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f2c64006f8d7..d4703ff69ad0 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2401,12 +2401,8 @@ static void spi_nor_no_sfdp_init_params(struct spi_nor *nor)
>   */
>  static void spi_nor_init_flags(struct spi_nor *nor)
>  {
> -	struct device_node *np = spi_nor_get_flash_node(nor);
>  	const u16 flags = nor->info->flags;
>  
> -	if (of_property_read_bool(np, "broken-flash-reset"))
> -		nor->flags |= SNOR_F_BROKEN_RESET;
> -
>  	if (flags & SPI_NOR_SWP_IS_VOLATILE)
>  		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
>  
> @@ -2933,9 +2929,47 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>  	mtd->_put_device = spi_nor_put_device;
>  }
>  
> +static int spi_nor_hw_reset(struct spi_nor *nor)
> +{
> +	struct gpio_desc *reset;
> +	int ret;
> +
> +	reset = devm_gpiod_get_optional(nor->dev, "reset", GPIOD_ASIS);
> +	if (IS_ERR_OR_NULL(reset))
> +		return PTR_ERR_OR_ZERO(reset);
> +
> +	/* Set the direction as output and enable the output */
> +	ret = gpiod_direction_output(reset, 1);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Experimental Minimum Chip select high to Reset delay value
> +	 * based on the flash device spec.
> +	 */
> +	usleep_range(1, 5);
> +	gpiod_set_value(reset, 0);
> +
> +	/*
> +	 * Experimental Minimum Reset pulse width value based on the
> +	 * flash device spec.
> +	 */
> +	usleep_range(10, 15);
> +	gpiod_set_value(reset, 1);
> +
> +	/*
> +	 * Experimental Minimum Reset recovery delay value based on the
> +	 * flash device spec.
> +	 */
> +	usleep_range(35, 40);
Infineon (Spansion/Cypress) SEMPER flash (S25HL/HS, S28HL/HS) family
specifies minimum tRH (Reset Pulse Hold - RESET# Low to CS# Low) as
450~600us. Please take care for this.

Please find datasheets at the following links.

https://www.infineon.com/dgdl/Infineon-S25HS256T_S25HS512T_S25HS01GT_S25HL256T_S25HL512T_S25HL01GT_256-Mb_(32-MB)_512-Mb_(64-MB)_1-Gb_(128-MB)_HS-T_(1.8-V)_HL-T_(3.0-V)_Semper_Flash_with_Quad_SPI-DataSheet-v02_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ee674b86ee3&da=t

https://www.infineon.com/dgdl/Infineon-S28HS256T_S28HS512T_S28HS01GT_S28HL256T_S28HL512T_S28HL01GT_256-Mb_(32-MB)_512-Mb_(64-MB)_1-Gb_(128-MB)_HS-T_(1.8-V)_HL-T_(3.0-V)_Semper_Flash_with_Octal_Interface-DataSheet-v03_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ee6bca96f97&da=t

Thanks,
Takahiro Kuwano

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

RE: [PATCH 2/2] mtd: spi-nor: Add support for flash reset

[Potthuri, Sai Krishna]

Hi Takahiro Kuwano,

> -----Original Message-----
> From: Takahiro Kuwano <tkuw584924@gmail.com>
> Sent: Thursday, September 1, 2022 7:28 AM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Tudor Ambarus
> <tudor.ambarus@microchip.com>; Pratyush Yadav <pratyush@kernel.org>;
> Michael Walle <michael@walle.cc>; Miquel Raynal
> <miquel.raynal@bootlin.com>; Richard Weinberger <richard@nod.at>;
> Vignesh Raghavendra <vigneshr@ti.com>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: devicetree@vger.kernel.org; linux-mtd@lists.infradead.org; linux-
> kernel@vger.kernel.org; saikrishna12468@gmail.com; git (AMD-Xilinx)
> <git@amd.com>; takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Subject: Re: [PATCH 2/2] mtd: spi-nor: Add support for flash reset
> 
> Hello,
> 
> On 8/29/2022 6:05 PM, Sai Krishna Potthuri wrote:
> > Add support for spi-nor flash reset via GPIO controller by reading the
> > reset-gpio property. If there is a valid GPIO specifier then reset
> > will be performed by asserting and deasserting the GPIO using gpiod
> > APIs otherwise it will not perform any operation.
> >
> > Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> > ---
> >  drivers/mtd/spi-nor/core.c | 50
> > +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 46 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index f2c64006f8d7..d4703ff69ad0 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2401,12 +2401,8 @@ static void spi_nor_no_sfdp_init_params(struct
> spi_nor *nor)
> >   */
> >  static void spi_nor_init_flags(struct spi_nor *nor)  {
> > -	struct device_node *np = spi_nor_get_flash_node(nor);
> >  	const u16 flags = nor->info->flags;
> >
> > -	if (of_property_read_bool(np, "broken-flash-reset"))
> > -		nor->flags |= SNOR_F_BROKEN_RESET;
> > -
> >  	if (flags & SPI_NOR_SWP_IS_VOLATILE)
> >  		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
> >
> > @@ -2933,9 +2929,47 @@ static void spi_nor_set_mtd_info(struct spi_nor
> *nor)
> >  	mtd->_put_device = spi_nor_put_device;  }
> >
> > +static int spi_nor_hw_reset(struct spi_nor *nor) {
> > +	struct gpio_desc *reset;
> > +	int ret;
> > +
> > +	reset = devm_gpiod_get_optional(nor->dev, "reset", GPIOD_ASIS);
> > +	if (IS_ERR_OR_NULL(reset))
> > +		return PTR_ERR_OR_ZERO(reset);
> > +
> > +	/* Set the direction as output and enable the output */
> > +	ret = gpiod_direction_output(reset, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Experimental Minimum Chip select high to Reset delay value
> > +	 * based on the flash device spec.
> > +	 */
> > +	usleep_range(1, 5);
> > +	gpiod_set_value(reset, 0);
> > +
> > +	/*
> > +	 * Experimental Minimum Reset pulse width value based on the
> > +	 * flash device spec.
> > +	 */
> > +	usleep_range(10, 15);
> > +	gpiod_set_value(reset, 1);
> > +
> > +	/*
> > +	 * Experimental Minimum Reset recovery delay value based on the
> > +	 * flash device spec.
> > +	 */
> > +	usleep_range(35, 40);
> Infineon (Spansion/Cypress) SEMPER flash (S25HL/HS, S28HL/HS) family
> specifies minimum tRH (Reset Pulse Hold - RESET# Low to CS# Low) as
> 450~600us. Please take care for this.
> 
> Please find datasheets at the following links.
> 
> https://www.infineon.com/dgdl/Infineon-
> S25HS256T_S25HS512T_S25HS01GT_S25HL256T_S25HL512T_S25HL01GT_256
> -Mb_(32-MB)_512-Mb_(64-MB)_1-Gb_(128-MB)_HS-T_(1.8-V)_HL-T_(3.0-
> V)_Semper_Flash_with_Quad_SPI-DataSheet-v02_00-
> EN.pdf?fileId=8ac78c8c7d0d8da4017d0ee674b86ee3&da=t
> 
> https://www.infineon.com/dgdl/Infineon-
> S28HS256T_S28HS512T_S28HS01GT_S28HL256T_S28HL512T_S28HL01GT_256
> -Mb_(32-MB)_512-Mb_(64-MB)_1-Gb_(128-MB)_HS-T_(1.8-V)_HL-T_(3.0-
> V)_Semper_Flash_with_Octal_Interface-DataSheet-v03_00-
> EN.pdf?fileId=8ac78c8c7d0d8da4017d0ee6bca96f97&da=t
Thanks for sharing this information.
I will take care of this in v2.

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