[PATCH RFC 0/4] Add set_iofv() callback

[PATCH RFC 0/4] Add set_iofv() callback

[Biju Das]

As per section 8.14 on the AT25QL128A hardware manual[1],
IO0..IO3 must be set to Hi-Z state for this flash for fast read quad IO.
Snippet from HW manual section 8.14:
The upper nibble of the Mode(M7-4) controls the length of the next FAST
Read Quad IO instruction through the inclusion or exclusion of the first
byte instruction code. The lower nibble bits of the Mode(M3-0) are don't
care. However, the IO pins must be high-impedance before the falling edge
of the first data out clock.

Add set_iofv() callback for configuring IO fixed values to control the
pin state.

[1]
https://www.renesas.com/eu/en/document/dst/at25ql128a-datasheet?r=1608586

Ref:
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230830145835.296690-1-biju.das.jz@bp.renesas.com/

Biju Das (4):
  spi: spi-mem: Add set_iofv() callback
  mtd: spi-nor: Add post_sfdp() callback
  memory: renesas-rpc-if: Add support for overriding IO fixed values
  spi: rpc-if: Add set_iofv() callback

 drivers/memory/renesas-rpc-if.c | 20 ++++++++++++++++++++
 drivers/mtd/spi-nor/core.c      | 20 ++++++++++++++++++++
 drivers/spi/spi-mem.c           | 20 ++++++++++++++++++++
 drivers/spi/spi-rpc-if.c        |  9 +++++++++
 include/linux/spi/spi-mem.h     |  4 ++++
 include/memory/renesas-rpc-if.h |  1 +
 6 files changed, 74 insertions(+)

-- 
2.25.1

[PATCH RFC 1/4] spi: spi-mem: Add set_iofv() callback

[Biju Das]

As per section 8.14 on the AT25QL128A hardware manual,
IO0..IO3 must be set to Hi-Z state for this flash for fast read quad IO.
Snippet from HW manual section 8.14:
The upper nibble of the Mode(M7-4) controls the length of the next FAST
Read Quad IO instruction through the inclusion or exclusion of the first
byte instruction code. The lower nibble bits of the Mode(M3-0) are don't
care. However, the IO pins must be high-impedance before the falling edge
of the first data out clock.

Add set_iofv() callback for configuring IO fixed values to control the
pin state.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/spi/spi-mem.c       | 20 ++++++++++++++++++++
 include/linux/spi/spi-mem.h |  4 ++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index edd7430d4c05..0cfca8c438e3 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -297,6 +297,26 @@ static void spi_mem_access_end(struct spi_mem *mem)
 		pm_runtime_put(ctlr->dev.parent);
 }
 
+/**
+ * spi_mem_set_iofv() - Set IO fixed values to control the pin state
+ * @mem: the SPI memory
+ * @val: the IO fixed values
+ *
+ * Set IO fixed values to control the pin state.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int spi_mem_set_iofv(struct spi_mem *mem, u32 val)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	if (ctlr->mem_ops && ctlr->mem_ops->set_iofv)
+		return ctlr->mem_ops->set_iofv(mem, val);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_mem_set_iofv);
+
 /**
  * spi_mem_exec_op() - Execute a memory operation
  * @mem: the SPI memory
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 6b0a7dc48a4b..e50f89bf5ba9 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -232,6 +232,7 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
  *		    limitations (can be alignment or max RX/TX size
  *		    limitations)
  * @supports_op: check if an operation is supported by the controller
+ * @set_iofv: set IO fixed values to control the pin state
  * @exec_op: execute a SPI memory operation
  * @get_name: get a custom name for the SPI mem device from the controller.
  *	      This might be needed if the controller driver has been ported
@@ -274,6 +275,7 @@ struct spi_controller_mem_ops {
 	int (*adjust_op_size)(struct spi_mem *mem, struct spi_mem_op *op);
 	bool (*supports_op)(struct spi_mem *mem,
 			    const struct spi_mem_op *op);
+	int (*set_iofv)(struct spi_mem *mem, u32 val);
 	int (*exec_op)(struct spi_mem *mem,
 		       const struct spi_mem_op *op);
 	const char *(*get_name)(struct spi_mem *mem);
@@ -367,6 +369,8 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op);
 bool spi_mem_supports_op(struct spi_mem *mem,
 			 const struct spi_mem_op *op);
 
+int spi_mem_set_iofv(struct spi_mem *mem, u32 val);
+
 int spi_mem_exec_op(struct spi_mem *mem,
 		    const struct spi_mem_op *op);
 
-- 
2.25.1

Re: [PATCH RFC 1/4] spi: spi-mem: Add set_iofv() callback

[Geert Uytterhoeven]

Hi Biju,

On Wed, Nov 8, 2023 at 6:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per section 8.14 on the AT25QL128A hardware manual,
> IO0..IO3 must be set to Hi-Z state for this flash for fast read quad IO.
> Snippet from HW manual section 8.14:
> The upper nibble of the Mode(M7-4) controls the length of the next FAST
> Read Quad IO instruction through the inclusion or exclusion of the first
> byte instruction code. The lower nibble bits of the Mode(M3-0) are don't
> care. However, the IO pins must be high-impedance before the falling edge
> of the first data out clock.
>
> Add set_iofv() callback for configuring IO fixed values to control the
> pin state.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -297,6 +297,26 @@ static void spi_mem_access_end(struct spi_mem *mem)
>                 pm_runtime_put(ctlr->dev.parent);
>  }
>
> +/**
> + * spi_mem_set_iofv() - Set IO fixed values to control the pin state
> + * @mem: the SPI memory
> + * @val: the IO fixed values

Please document the meaning of this value (i.e. what does a
set or cleared bit mean?).

> + *
> + * Set IO fixed values to control the pin state.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int spi_mem_set_iofv(struct spi_mem *mem, u32 val)
> +{
> +       struct spi_controller *ctlr = mem->spi->controller;
> +
> +       if (ctlr->mem_ops && ctlr->mem_ops->set_iofv)
> +               return ctlr->mem_ops->set_iofv(mem, val);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_mem_set_iofv);
> +
>  /**
>   * spi_mem_exec_op() - Execute a memory operation
>   * @mem: the SPI memory

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH RFC 4/4] spi: rpc-if: Add set_iofv() callback

[Biju Das]

Add set_iofv() callback for configuring IO fixed values to control
the pin state.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/spi/spi-rpc-if.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/spi/spi-rpc-if.c b/drivers/spi/spi-rpc-if.c
index e11146932828..8b5c41de99af 100644
--- a/drivers/spi/spi-rpc-if.c
+++ b/drivers/spi/spi-rpc-if.c
@@ -75,6 +75,14 @@ static bool rpcif_spi_mem_supports_op(struct spi_mem *mem,
 	return true;
 }
 
+static int rpcif_spi_mem_set_iofv(struct spi_mem *mem, const u32 val)
+{
+	struct rpcif *rpc =
+		spi_controller_get_devdata(mem->spi->controller);
+
+	return rpcif_set_iofv(rpc->dev, val);
+}
+
 static ssize_t rpcif_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
 					 u64 offs, size_t len, void *buf)
 {
@@ -121,6 +129,7 @@ static int rpcif_spi_mem_exec_op(struct spi_mem *mem,
 }
 
 static const struct spi_controller_mem_ops rpcif_spi_mem_ops = {
+	.set_iofv	= rpcif_spi_mem_set_iofv,
 	.supports_op	= rpcif_spi_mem_supports_op,
 	.exec_op	= rpcif_spi_mem_exec_op,
 	.dirmap_create	= rpcif_spi_mem_dirmap_create,
-- 
2.25.1

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

> As per section 8.14 on the AT25QL128A hardware manual[1],
> IO0..IO3 must be set to Hi-Z state for this flash for fast read quad 
> IO.
> Snippet from HW manual section 8.14:
> The upper nibble of the Mode(M7-4) controls the length of the next FAST
> Read Quad IO instruction through the inclusion or exclusion of the 
> first
> byte instruction code. The lower nibble bits of the Mode(M3-0) are 
> don't
> care. However, the IO pins must be high-impedance before the falling 
> edge
> of the first data out clock.

I'm still not sure what you are trying to fix here. For any quad I/O 
mode,
the pins of the controller must be in hiZ during the data phase on a 
read
operation. Otherwise the flash couldn't send any data, there would
be two drivers for one signal. So being in hiZ state should be the 
default
and shouldn't depend on any connected flash.

You've mentioned the micron flash which needs a '1' on its hold/reset
pin. I would have expected a fixup for this flash, not for the flash 
which
behaves normal.

-michael

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

>> > As per section 8.14 on the AT25QL128A hardware manual[1],
>> > IO0..IO3 must be set to Hi-Z state for this flash for fast read quad
>> > IO.
>> > Snippet from HW manual section 8.14:
>> > The upper nibble of the Mode(M7-4) controls the length of the next
>> > FAST Read Quad IO instruction through the inclusion or exclusion of
>> > the first byte instruction code. The lower nibble bits of the
>> > Mode(M3-0) are don't care. However, the IO pins must be high-impedance
>> > before the falling edge of the first data out clock.
>> 
>> I'm still not sure what you are trying to fix here. For any quad I/O 
>> mode,
>> the pins of the controller must be in hiZ during the data phase on a 
>> read
>> operation. Otherwise the flash couldn't send any data, there would be 
>> two
>> drivers for one signal. So being in hiZ state should be the default 
>> and
>> shouldn't depend on any connected flash.
> 
> OK, I will make hiZ state as the default.

I still think this iofv setting is the wrong approach, though. Do you
have a link to the spi controller datasheet where I can look up what
the controller is doing.

This seem to be a general problem with what we are sending during the
command phase and I'm curious why there wasn't more reports on non
working micron flashes for now.

>> You've mentioned the micron flash which needs a '1' on its hold/reset 
>> pin.
>> I would have expected a fixup for this flash, not for the flash which
>> behaves normal.
> 
> I will drop fixup for Renesas AT25QL128A  and will add fixup for micron 
> flash.

btw, what will happen if you always use the {3,3,3,1} setting? I guess
the atmel flash will also work? because HiZ should mean "don't care" 
from
the point of view of the flash.

> 
> With iofv settings {3,3,3,3} (all pins on Hi-Z state) with Micron flash
> -----------------------------------------------------------------------
> 
> ./rpcif_t_001.sh
> [   37.950986] spi-nor spi1.0: unrecognized JEDEC id bytes: ff ff ff ff 
> ff ff

As mentioned earlier, I suspect that HiZ on IO3 means low and the flash
will be in reset. Could you perhaps verify that by probing IO3?
I know that other flashes will *either* support RESET#/HOLD# or quad 
mode.
Thus I was saying, that we probably wont support that and the easiest
fix should be to disable this behavior for the atmel flash (there was
nv setting).

I guess, the correct fix would be to somehow add support to control
IO1-IO3 during the (single bit) command phase.

-michael

> 
> EXIT|FAIL|rpcif_t_001.sh|[00:00:01] Failed to detect mt25qu512a 
> flash!||
> 
> 
> With iofv settings {3,3,3,1} with Micron falsh
> ---------------------------------------------
> root@smarc-rzg2l:/cip-test-scripts# ./rpcif_t_001.sh
> [   26.500035] spi-nor spi1.0: mt25qu512a (65536 Kbytes)
> [   26.533995] 2 fixed-partitions partitions found on MTD device spi1.0
> [   26.540410] Creating 2 MTD partitions on "spi1.0":
> [   26.545239] 0x000000000000-0x000002000000 : "boot"
> [   26.554381] 0x000002000000-0x000004000000 : "user"
> 
> EXIT|PASS|rpcif_t_001.sh|[00:03:01] ||
> 
> Cheers,
> Biju

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

>> >> > As per section 8.14 on the AT25QL128A hardware manual[1],
>> >> > IO0..IO3 must be set to Hi-Z state for this flash for fast read
>> >> > quad IO.
>> >> > Snippet from HW manual section 8.14:
>> >> > The upper nibble of the Mode(M7-4) controls the length of the next
>> >> > FAST Read Quad IO instruction through the inclusion or exclusion of
>> >> > the first byte instruction code. The lower nibble bits of the
>> >> > Mode(M3-0) are don't care. However, the IO pins must be
>> >> > high-impedance before the falling edge of the first data out clock.
>> >>
>> >> I'm still not sure what you are trying to fix here. For any quad I/O
>> >> mode, the pins of the controller must be in hiZ during the data phase
>> >> on a read operation. Otherwise the flash couldn't send any data,
>> >> there would be two drivers for one signal. So being in hiZ state
>> >> should be the default and shouldn't depend on any connected flash.
>> >
>> > OK, I will make hiZ state as the default.
>> 
>> I still think this iofv setting is the wrong approach, though. Do you 
>> have
>> a link to the spi controller datasheet where I can look up what the
>> controller is doing.
> 
> Please find the below link.
> 
> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz-mpus/rzg2l-general-purpose-microprocessors-dual-core-arm-cortex-a55-12-ghz-cpus-and-single-core-arm-cortex-m33#overview

Thanks.

>> This seem to be a general problem with what we are sending during the
>> command phase and I'm curious why there wasn't more reports on non 
>> working
>> micron flashes for now.
> 
> 1-bit mode, we don't have any issue. Once we switch to 4-bit mode we 
> have this
> issue with micron MT25QU512A flash and we need to set the correct IO 
> fixed values.
> 
> Maybe others are testing with 1-bit mode and not testing the full 
> capability of the flash.
> 
>> 
>> >> You've mentioned the micron flash which needs a '1' on its hold/reset
>> >> pin.
>> >> I would have expected a fixup for this flash, not for the flash which
>> >> behaves normal.
>> >
>> > I will drop fixup for Renesas AT25QL128A  and will add fixup for
>> > micron flash.
>> 
>> btw, what will happen if you always use the {3,3,3,1} setting? I guess 
>> the
>> atmel flash will also work? because HiZ should mean "don't care"
>> from
>> the point of view of the flash.
> 
> With atmel flash if use {3,3,3,1} setting, I get below error.
> 
> root@smarc-rzg2ul:/cip-test-scripts# ./rpcif_t_001.sh
> [  144.078854] spi-nor spi1.0: spi-nor-generic (16384 Kbytes)
> [  144.120468] 2 fixed-partitions partitions found on MTD device spi1.0
> [  144.126982] Creating 2 MTD partitions on "spi1.0":
> [  144.133004] 0x000000000000-0x000000200000 : "boot"
> [  144.141879] 0x000000200000-0x000001000000 : "user"
> [  358.476963] jffs2: notice: (230) read_dnode: node CRC failed on 
> dnode at 0xdfe084: read 0x336ebbbc, calculated 0x961503c7
> [  358.488509] jffs2: notice: (230) read_dnode: node CRC failed on 
> dnode at 0xdfd118: read 0xff6a5df6, calculated 0x786a5df6
> [  358.502963] jffs2: notice: (230) read_dnode: node CRC failed on 
> dnode at 0xdfa2d4: read 0x1fc99948, calculated 0xbab22133
> [  358.515357] jffs2: notice: (230) read_dnode: node CRC failed on 
> dnode at 0xdf9368: read 0xffd184a7, calculated 0x3d184a7
> [  358.528175] jffs2: notice: (230) read_dnode: node CRC failed on 
> dnode at 0xdf6524: read 0x5deb2462, calculated 0xf8909c19

Strange. Can't make any sense of this at the moment.

>> > With iofv settings {3,3,3,3} (all pins on Hi-Z state) with Micron
>> > flash
>> > ----------------------------------------------------------------------
>> > -
>> >
>> > ./rpcif_t_001.sh
>> > [   37.950986] spi-nor spi1.0: unrecognized JEDEC id bytes: ff ff ff ff
>> > ff ff
>> 
>> As mentioned earlier, I suspect that HiZ on IO3 means low and the 
>> flash
>> will be in reset. Could you perhaps verify that by probing IO3?
>> I know that other flashes will *either* support RESET#/HOLD# or quad 
>> mode.
>> Thus I was saying, that we probably wont support that and the easiest 
>> fix
>> should be to disable this behavior for the atmel flash (there was nv
>> setting).
> 
> The fix up is invoked only for quad mode, I believe it is safe to add 
> fixup for micron flash
> As it is the one deviating from normal according to you, rather than 
> adding fixup
> for generic flash like ATMEL flash(Now Renesas flash)

Could you please try setting bit 4 in the Nonvolatile Configuration
Register (Table 7) and see if the problem goes away?

Also could you have a look at the schematics, does the IO3/RESET#
have a pull-up? If not, who is in control of driving the correct
value here? If it has a pull-up, I'm puzzled why you need any
other setting than HiZ.

The correct fix would be to the information about the missing
IO state in the "struct spi_mem_op". That is, what should be the
default values of all the IO lines which are unused. For example
if we have a 1s1s4s transaction, what should be the state of IO0,
IO2 and IO3 during the command and address phase. If we have a
1s2s2s, what should be the state of IO0 during the command phase
etc.

That can then be used within your driver to set the corresponding
IOFV values (for each spi-mem op).

But I'm not sure if other SPI controllers will support that, though.

-michael

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

>> >> Thus I was saying, that we probably wont support that and the easiest
>> >> fix should be to disable this behavior for the atmel flash (there was
>> >> nv setting).
>> >
>> > The fix up is invoked only for quad mode, I believe it is safe to add
>> > fixup for micron flash As it is the one deviating from normal
>> > according to you, rather than adding fixup for generic flash like
>> > ATMEL flash(Now Renesas flash)
>> 
>> Could you please try setting bit 4 in the Nonvolatile Configuration
>> Register (Table 7) and see if the problem goes away?
> 
> You mean, if it works, we need to disable reset for all the boards, 
> maybe at bootloader level??

Not necessarily. First, just to confirm that it is actually the reset
circuit. You can also compare the part numbers of the flash. There
is a flash with IO3/RESET# and IO3/HOLD# (and a flash with a dedicated
reset pin).

If that's the case, it looks like a hardware bug on your board. You
left the reset pin floating. So you'd also not be able to boot from
the NOR flash, right?

> OK, I will check that. Currently I have read that register and it is 
> showing a value
> Of 0xffbb. I need to do write operation. Before that how do we recover 
> flash, if
> something goes wrong during writing for NV register?

You should always be able to write that register from the bootloader.
Maybe also through raw commands (like sspi in uboot).

>> Also could you have a look at the schematics, does the IO3/RESET# have 
>> a
>> pull-up? If not, who is in control of driving the correct value here? 
>> If
>> it has a pull-up, I'm puzzled why you need any other setting than HiZ.
> 
> Unfortunately, there is no pullup on IO3 line and also there is no SoC 
> pullup.

See above.

-michael

>> The correct fix would be to the information about the missing IO state 
>> in
>> the "struct spi_mem_op". That is, what should be the default values of 
>> all
>> the IO lines which are unused. For example if we have a 1s1s4s
>> transaction, what should be the state of IO0,
>> IO2 and IO3 during the command and address phase. If we have a 1s2s2s,
>> what should be the state of IO0 during the command phase etc.
>> 
>> That can then be used within your driver to set the corresponding IOFV
>> values (for each spi-mem op).
>> 
>> But I'm not sure if other SPI controllers will support that, though.
> 
> Currently driving SoC IOFV register, it fixes the issue and it is just 
> one time
> operation during post sfdp. I take this as a SoC feature which other 
> controllers
> don't have.
> 
> Cheers,
> Biju

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Am 2023-11-11 13:26, schrieb Biju Das:
> Hi Michael Walle,
> 
>> Subject: RE: [PATCH RFC 0/4] Add set_iofv() callback
>> 
> 
>> 
>> > Subject: Re: [PATCH RFC 0/4] Add set_iofv() callback
>> >
>> > Hi Biju,
>> >
>> > >> >> Thus I was saying, that we probably wont support that and the
>> > >> >> easiest fix should be to disable this behavior for the atmel
>> > >> >> flash (there was nv setting).
>> > >> >
>> > >> > The fix up is invoked only for quad mode, I believe it is safe to
>> > >> > add fixup for micron flash As it is the one deviating from normal
>> > >> > according to you, rather than adding fixup for generic flash like
>> > >> > ATMEL flash(Now Renesas flash)
>> > >>
>> > >> Could you please try setting bit 4 in the Nonvolatile Configuration
>> > >> Register (Table 7) and see if the problem goes away?
>> > >
>> > > You mean, if it works, we need to disable reset for all the boards,
>> > > maybe at bootloader level??
>> >
>> > Not necessarily. First, just to confirm that it is actually the reset
>> > circuit. You can also compare the part numbers of the flash. There is
>> > a flash with IO3/RESET# and IO3/HOLD# (and a flash with a dedicated
>> > reset pin).
>> 
>> Part is MT25QU512ABB8E12-0SIT, As per the schematic, flash has a 
>> dedicated
>> RESET# with 10K pullup connected to SoC QSPI_RESET pin.
>> 
>> DQ0, DQ1, W#/DQ2 and DQ3 lines on the flash are connected without any
>> pullups to the SoC QSPI0_{0..3} pins.
>> 
>> >
>> > If that's the case, it looks like a hardware bug on your board. You
>> > left the reset pin floating. So you'd also not be able to boot from
>> > the NOR flash, right?
>> 
>> I am booting from NOR flash. BootRom code reads SPI flash and executes
>> BL2.
>> BL2 loads BL33 and U-boot from NOR flash. If this is the case, do you
>> think it is a Hw bug on the board?
>> 
>> >
>> > > OK, I will check that. Currently I have read that register and it is
>> > > showing a value Of 0xffbb. I need to do write operation. Before that
>> > > how do we recover flash, if something goes wrong during writing for
>> > > NV register?
>> >
>> > You should always be able to write that register from the bootloader.
>> > Maybe also through raw commands (like sspi in uboot).
>> 
>> Thanks for the pointer, I haven't explored the uboot path.
> 
> I have disabled RESET# bit in the Nonvolatile Configuration
> Register (Table 7) and borad doesn't boot any more.
> 
> By default that bit is set.
> 
> [    2.530291] ###### Before write Read cmd=b5 val=ff
> [    2.530431] ###### write cmd=b1 val=ef
> [    2.535518] ###### Read cmd=b5 val=ef
> 
> 
> NOTICE:  BL2: Built : 14:59:28, Nov 10 2023
> ERROR:   BL2: Failed to load image id 3 (-2)
> NOTICE:  BL2: v2.9(release):v2.5/rzg2l-1.00-3883-gc314a391c
> NOTICE:  BL2: Built : 14:59:28, Nov 10 2023
> ERROR:   BL2: Failed to load image id 3 (-2)
> NOTICE:  BL2: v2.9(release):v2.5/rzg2l-1.00-3883-gc314a391c
> 
> What is your thoughts on this? How do we proceed now?

I guessed you fixed this? Because.. if you boot from NOR the BL2
should come from the NOR flash too, correct? And that is actually
working.

-michael

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

> After that I will send a patch using IOFV {3,3,3,3} for both micron and 
> Adesto flash.

Just to be clear, that will just touch the spi controller as a global 
default, right?
That shouldn't go through spi-nor. Otherwise I'd prefer to use fix (2) 
from my previous
mail.

-michael

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

>> > After that I will send a patch using IOFV {3,3,3,3} for both micron
>> > and Adesto flash.
>> 
>> Just to be clear, that will just touch the spi controller as a global
>> default, right?
> 
> Yes, it is in SoC specific bus controller 
> driver(driver/memory/renesas-rpc-if.c)
> 
>> That shouldn't go through spi-nor. Otherwise I'd prefer to use fix (2)
>> from my previous mail.
> 
> Agreed. Fix(2) won't work as renesas-rpc-if probe which sets {3,3,3,3} 
> is called before flash detection.
> and that will make flash detection to fail. So we cannot use fixup. The 
> only way (2) to work
> is to like patch[1].

Ohh I see. Makes sense. Can you ask your SoC engineers, why they
choose the IO3 default to high? I'd guess because it's usually
shared with HOLD# or RESET#. But that really begs the question
why the Atmel flash isn't working with that setting. I suspect
some problems during the turn around of the direction of IO3.
You'd really have to probe with an oscilloscope though.

-michael

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

>> But that really begs the question why the Atmel flash isn't
>> working with that setting. I suspect some problems during the turn 
>> around
>> of the direction of IO3.
>> You'd really have to probe with an oscilloscope though.
> 
> OK, but as per [1], 8.14, IO3 must be HiZ for Atmel flash.

Sure, but the question is *why*? The flash shouldn't drive IO3
during the command phase. Also, because it might be in HiZ it
cannot read the state (as it is undefined at this point). So
what is going wrong here?

As mentioned, I suspect something is going wrong during the
change of direction of the IO3 line. Either the SoC is driving
it for too long, or the flash is driving it too early?

-michael

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

Hi Biju,

>> >> >> Thus I was saying, that we probably wont support that and the
>> >> >> easiest fix should be to disable this behavior for the atmel flash
>> >> >> (there was nv setting).
>> >> >
>> >> > The fix up is invoked only for quad mode, I believe it is safe to
>> >> > add fixup for micron flash As it is the one deviating from normal
>> >> > according to you, rather than adding fixup for generic flash like
>> >> > ATMEL flash(Now Renesas flash)
>> >>
>> >> Could you please try setting bit 4 in the Nonvolatile Configuration
>> >> Register (Table 7) and see if the problem goes away?
>> >
>> > You mean, if it works, we need to disable reset for all the boards,
>> > maybe at bootloader level??
>> 
>> Not necessarily. First, just to confirm that it is actually the reset
>> circuit. You can also compare the part numbers of the flash. There is 
>> a
>> flash with IO3/RESET# and IO3/HOLD# (and a flash with a dedicated 
>> reset
>> pin).
>> 
>> If that's the case, it looks like a hardware bug on your board. You 
>> left
>> the reset pin floating. So you'd also not be able to boot from the NOR
>> flash, right?
>> 
>> > OK, I will check that. Currently I have read that register and it is
>> > showing a value Of 0xffbb. I need to do write operation. Before that
>> > how do we recover flash, if something goes wrong during writing for NV
>> > register?
>> 
>> You should always be able to write that register from the bootloader.
>> Maybe also through raw commands (like sspi in uboot).
> 
> Just an update, now clearing bit4 on Micron flash, I am able to test 
> erase/read/write
> Micron flash with IOFV state {3,3,3,3}. Not sure what went wrong 
> previously.
> only thing I changed is related to enabling the QUAD spi mode by 
> disabling bit 2 and 3 on NV register.

This enables QPI mode, that is the command is also expected to be
transferred over the four IO lines. That's not something linux supports.

We'll always send the command in single bit mode (the only exception is
octal DTR mode).

> This again result in boot failure as boot ROM expects extended SPI 
> mode.
> Then restored the extended SPI mode by sending the command FFh (Power 
> Loss and Interface Rescue)
> by booting from eMMC.
> 
> At least one board with micron flash is now working with IOFV state 
> {3,3,3,3}.
> I need to test more boards to confirm the behaviour.

I'm still trying to make sense of this (and trying to figure out
if we need something or not).

So you have a MT25QU512ABB8E12-0SIT, which according to to Figure 4 and
Figure 5 has a dedicated RESET# line and the IO3 is multiplexed with 
HOLD#.
Thus, it seems the flash will go into hold mode if IO3 is not high 
during
command phase. Wether this is a hardware bug? I'd tend to say yes. As 
with
the RESET# you depend on the software/bootROM whatever to set the state 
of
IO3 correctly. But it might depend on the SPI controller used etc. Maybe
it will already drive IO3 high by default?! (and esp. what the bootROM 
is doing
if that SoC is able to load the first stage bootloader from NOR).

I still see two possible fixes here:
(1) Disable HOLD#, either during board production, in your 
bootloader/TFA
     or by introducing a fixup for this flash in linux.
     (And configure your SPI controller to use IOFV state 3,3,3,3 as that
     should be the sane default).
(2) Introduce a mechanism to spi_mem_op to control the unused bits (as
     explained in an earlier reply). Then somehow integrate that into
     the spi-nor micron driver to set IO3 to this pariticular value for
     this operation.

Alternatively, fix your board to have a weak pull-up on IO3 so the IOFV
state 3,3,3,3 will work.

HTH,
-michael

Re: [PATCH RFC 0/4] Add set_iofv() callback

[Michael Walle]

> Maybe
> it will already drive IO3 high by default?! (and esp. what the bootROM 
> is doing
> if that SoC is able to load the first stage bootloader from NOR).

I just had another look at your Renesas SoC and indeed, the register 
default is
to set IO3 high if not used. Mh. I still think 3,3,3,3 is the saner 
default.
But I might be wrong. Hard to tell, as the sample size is just Micron 
and Atmel
for now. And it's still unclear to me why the Atmel isn't working with 
3,3,3,1.

-michael