Hi Michael,
> -----Original Message-----
> From: Michael Walle <michael@xxxxxxxx>
> Sent: Monday, August 29, 2022 3:35 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@xxxxxxx>
> Cc: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>; Pratyush Yadav
> <pratyush@xxxxxxxxxx>; Miquel Raynal <miquel.raynal@xxxxxxxxxxx>;
> Richard Weinberger <richard@xxxxxx>; Vignesh Raghavendra
> <vigneshr@xxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> saikrishna12468@xxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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);
