| Message ID | 20231108171149.258656-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add set_iofv() callback | expand |
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
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
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
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
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
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
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
> 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
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
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
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