| Message ID | 1472033154-31475-1-git-send-email-oe5hpm@oevsv.at |
|---|---|
| State | Rejected |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
Hi, On Wednesday 24 August 2016 03:35 PM, Hannes Schmelzer wrote: > During probing flashes on the spi bus using the "sf probe" command, a > maybe existing flash (from fdt) is unbound and removed to force the > 'spi_flash_probe_bus_cs' really scanning the bus. > > Today the bus is probed with speed 0, this triggers several fall-back > mechanism (mostly in the low-level drivers) to catch the impossible zero > speed. > Result of this is, that the spi-flash runs at very low speed depending > on the minimum given by low-level driver/hardware. > > Values like 'spi-max-frequency' from devicetree are ignored totally > today. > > This commit changes as following: > - if there was already some flash binding in devicetree (having some > spi-max-frequency within) speed is taken from it > - if no flash binding was present for speed the 'spi-max-frequency' from > the responsible spi node is taken. > > Signed-off-by: Hannes Schmelzer <oe5hpm@oevsv.at> With commit 96907c0fe50a8 ("dm: spi: Read default speed and mode values from DT") sf probe picks spi-max-frequency from DT if not specified as argument. But when sf probe is called second time, the command fails to pick up speed from DT. This is because flash node is unbound from the SPI controller children nodes. Below patch should fix this issue: https://patchwork.ozlabs.org/patch/659979/
On 08/24/2016 12:35 PM, Vignesh R wrote: > Hi, Hi Vignesh, > On Wednesday 24 August 2016 03:35 PM, Hannes Schmelzer wrote: >> During probing flashes on the spi bus using the "sf probe" command, a >> maybe existing flash (from fdt) is unbound and removed to force the >> 'spi_flash_probe_bus_cs' really scanning the bus. >> >> Today the bus is probed with speed 0, this triggers several fall-back >> mechanism (mostly in the low-level drivers) to catch the impossible zero >> speed. >> Result of this is, that the spi-flash runs at very low speed depending >> on the minimum given by low-level driver/hardware. >> >> Values like 'spi-max-frequency' from devicetree are ignored totally >> today. >> >> This commit changes as following: >> - if there was already some flash binding in devicetree (having some >> spi-max-frequency within) speed is taken from it >> - if no flash binding was present for speed the 'spi-max-frequency' from >> the responsible spi node is taken. >> >> Signed-off-by: Hannes Schmelzer <oe5hpm@oevsv.at> > > With commit 96907c0fe50a8 ("dm: spi: Read default speed and mode values > from DT") sf probe picks spi-max-frequency from DT if not specified as > argument. > > But when sf probe is called second time, the command fails to pick up > speed from DT. This is because flash node is unbound from the SPI > controller children nodes. Below patch should fix this issue: > https://patchwork.ozlabs.org/patch/659979/ sry ... i didn't took notice about this patch at the beginning of mine. Just reviewed and tested it. The named patch makes things a bit better but not good. Speed for flash still has no relationship to 'spi-max-frequency' from the spi-bus nor with a maybe defined flash in dts. Rather the #define CONFIG_ENV_SPI_MAX_HZ is used. But for my thinking the way we should walk is having more and more and finally (if possible) all within device-tree and dynamic configuration rather than in #define ... cheers, Hannes
On Wednesday 24 August 2016 04:21 PM, Hannes Schmelzer wrote: > On 08/24/2016 12:35 PM, Vignesh R wrote: >> Hi, > Hi Vignesh, >> On Wednesday 24 August 2016 03:35 PM, Hannes Schmelzer wrote: >>> During probing flashes on the spi bus using the "sf probe" command, a >>> maybe existing flash (from fdt) is unbound and removed to force the >>> 'spi_flash_probe_bus_cs' really scanning the bus. >>> >>> Today the bus is probed with speed 0, this triggers several fall-back >>> mechanism (mostly in the low-level drivers) to catch the impossible zero >>> speed. >>> Result of this is, that the spi-flash runs at very low speed depending >>> on the minimum given by low-level driver/hardware. >>> >>> Values like 'spi-max-frequency' from devicetree are ignored totally >>> today. >>> >>> This commit changes as following: >>> - if there was already some flash binding in devicetree (having some >>> spi-max-frequency within) speed is taken from it >>> - if no flash binding was present for speed the 'spi-max-frequency' from >>> the responsible spi node is taken. >>> >>> Signed-off-by: Hannes Schmelzer <oe5hpm@oevsv.at> >> >> With commit 96907c0fe50a8 ("dm: spi: Read default speed and mode values >> from DT") sf probe picks spi-max-frequency from DT if not specified as >> argument. >> >> But when sf probe is called second time, the command fails to pick up >> speed from DT. This is because flash node is unbound from the SPI >> controller children nodes. Below patch should fix this issue: >> https://patchwork.ozlabs.org/patch/659979/ > > sry ... i didn't took notice about this patch at the beginning of mine. > Just reviewed and tested it. > > The named patch makes things a bit better but not good. > Speed for flash still has no relationship to 'spi-max-frequency' from > the spi-bus nor with a maybe defined flash in dts. > > Rather the #define CONFIG_ENV_SPI_MAX_HZ is used. > sf probe calls do_spi_flash_probe() -spi_flash_probe_bus_cs() -spi_get_bus_and_cs() In spi_get_bus_and_cs(): if (!speed) { speed = plat->max_hz; mode = plat->mode; } This should set speed to spi-max-frequency as per flash DT node. AFAIU, saveenv() uses CONFIG_ENV_SPI_MAX_HZ only when CONFIG_DM_SPI_FLASH is not defined. Could please explain how CONFIG_ENV_SPI_MAX_HZ takes precedence over spi-max-frequency during sf probe?
On 08/24/2016 01:12 PM, Vignesh R wrote: > > On Wednesday 24 August 2016 04:21 PM, Hannes Schmelzer wrote: >> On 08/24/2016 12:35 PM, Vignesh R wrote: >>> Hi, >> Hi Vignesh, >>> On Wednesday 24 August 2016 03:35 PM, Hannes Schmelzer wrote: >>>> During probing flashes on the spi bus using the "sf probe" command, a >>>> maybe existing flash (from fdt) is unbound and removed to force the >>>> 'spi_flash_probe_bus_cs' really scanning the bus. >>>> >>>> Today the bus is probed with speed 0, this triggers several fall-back >>>> mechanism (mostly in the low-level drivers) to catch the impossible zero >>>> speed. >>>> Result of this is, that the spi-flash runs at very low speed depending >>>> on the minimum given by low-level driver/hardware. >>>> >>>> Values like 'spi-max-frequency' from devicetree are ignored totally >>>> today. >>>> >>>> This commit changes as following: >>>> - if there was already some flash binding in devicetree (having some >>>> spi-max-frequency within) speed is taken from it >>>> - if no flash binding was present for speed the 'spi-max-frequency' from >>>> the responsible spi node is taken. >>>> >>>> Signed-off-by: Hannes Schmelzer <oe5hpm@oevsv.at> >>> With commit 96907c0fe50a8 ("dm: spi: Read default speed and mode values >>> from DT") sf probe picks spi-max-frequency from DT if not specified as >>> argument. >>> >>> But when sf probe is called second time, the command fails to pick up >>> speed from DT. This is because flash node is unbound from the SPI >>> controller children nodes. Below patch should fix this issue: >>> https://patchwork.ozlabs.org/patch/659979/ >> sry ... i didn't took notice about this patch at the beginning of mine. >> Just reviewed and tested it. >> >> The named patch makes things a bit better but not good. >> Speed for flash still has no relationship to 'spi-max-frequency' from >> the spi-bus nor with a maybe defined flash in dts. >> >> Rather the #define CONFIG_ENV_SPI_MAX_HZ is used. >> > sf probe calls do_spi_flash_probe() > -spi_flash_probe_bus_cs() > -spi_get_bus_and_cs() > > In spi_get_bus_and_cs(): > if (!speed) { > speed = plat->max_hz; > mode = plat->mode; > } > This should set speed to spi-max-frequency as per flash DT node. yes that should do. But have look a few lines up, around 296, there the newly created flash node has been modified with wrong values before. No glue why a new flash node is being created. > AFAIU, saveenv() uses CONFIG_ENV_SPI_MAX_HZ only when > CONFIG_DM_SPI_FLASH is not defined. Could please explain how > CONFIG_ENV_SPI_MAX_HZ takes precedence over spi-max-frequency during sf > probe? Thanks the discussion, I think we coming closer to the problem. Your'e right saveenv() behaves as you described, but not so env_relocate_spec(). There the flash is probed like this: env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); and thats the point where some existing dt node becomes modified with those values. Thats probably wrong doing so. I think there should be same logic applied as in saveen(). Just for testing i fixed the probe there. Now i'm a bit confused about that the function spi_find_chip_select(bus, cs, &dev) in spi-uclass doesn't find child devices on the spi node. Instead there a new flash binding is created. spi_get_bus_and_cs: Binding new device 'spi_flash@0:0', busnum=0, cs=0, driver=spi_flash_std my devicetree look like this: => fdt print spi0 spi@e000d000 { clock-names = "ref_clk", "pclk"; clocks = <0x00000001 0x0000000a 0x00000001 0x0000002b>; compatible = "xlnx,zynq-qspi-1.0"; status = "okay"; interrupt-parent = <0x00000003>; interrupts = <0x00000000 0x00000013 0x00000004>; reg = <0xe000d000 0x00001000>; #address-cells = <0x00000001>; #size-cells = <0x00000000>; u-boot,dm-pre-reloc; spi-max-frequency = <0x05f5e100>; S25FL256S@0 { #address-cells = <0x00000001>; #size-cells = <0x00000001>; compatible = "spansion,S25FL256S"; reg = <0x00000000>; spi-max-frequency = <0x05f5e100>; }; }; => cheers, Hannes
On 08/24/2016 02:21 PM, Hannes Schmelzer wrote: > On 08/24/2016 01:12 PM, Vignesh R wrote: >> >> On Wednesday 24 August 2016 04:21 PM, Hannes Schmelzer wrote: >>> On 08/24/2016 12:35 PM, Vignesh R wrote: >>>> Hi, >>> Hi Vignesh, >>>> On Wednesday 24 August 2016 03:35 PM, Hannes Schmelzer wrote: >>>>> During probing flashes on the spi bus using the "sf probe" command, a >>>>> maybe existing flash (from fdt) is unbound and removed to force the >>>>> 'spi_flash_probe_bus_cs' really scanning the bus. >>>>> >>>>> Today the bus is probed with speed 0, this triggers several fall-back >>>>> mechanism (mostly in the low-level drivers) to catch the >>>>> impossible zero >>>>> speed. >>>>> Result of this is, that the spi-flash runs at very low speed >>>>> depending >>>>> on the minimum given by low-level driver/hardware. >>>>> >>>>> Values like 'spi-max-frequency' from devicetree are ignored totally >>>>> today. >>>>> >>>>> This commit changes as following: >>>>> - if there was already some flash binding in devicetree (having some >>>>> spi-max-frequency within) speed is taken from it >>>>> - if no flash binding was present for speed the >>>>> 'spi-max-frequency' from >>>>> the responsible spi node is taken. >>>>> >>>>> Signed-off-by: Hannes Schmelzer <oe5hpm@oevsv.at> >>>> With commit 96907c0fe50a8 ("dm: spi: Read default speed and mode >>>> values >>>> from DT") sf probe picks spi-max-frequency from DT if not specified as >>>> argument. >>>> >>>> But when sf probe is called second time, the command fails to pick up >>>> speed from DT. This is because flash node is unbound from the SPI >>>> controller children nodes. Below patch should fix this issue: >>>> https://patchwork.ozlabs.org/patch/659979/ >>> sry ... i didn't took notice about this patch at the beginning of mine. >>> Just reviewed and tested it. >>> >>> The named patch makes things a bit better but not good. >>> Speed for flash still has no relationship to 'spi-max-frequency' from >>> the spi-bus nor with a maybe defined flash in dts. >>> >>> Rather the #define CONFIG_ENV_SPI_MAX_HZ is used. >>> >> sf probe calls do_spi_flash_probe() >> -spi_flash_probe_bus_cs() >> -spi_get_bus_and_cs() >> >> In spi_get_bus_and_cs(): >> if (!speed) { >> speed = plat->max_hz; >> mode = plat->mode; >> } >> This should set speed to spi-max-frequency as per flash DT node. > yes that should do. > But have look a few lines up, around 296, there the newly created > flash node has been modified with wrong values before. > No glue why a new flash node is being created. >> AFAIU, saveenv() uses CONFIG_ENV_SPI_MAX_HZ only when >> CONFIG_DM_SPI_FLASH is not defined. Could please explain how >> CONFIG_ENV_SPI_MAX_HZ takes precedence over spi-max-frequency during sf >> probe? > Thanks the discussion, I think we coming closer to the problem. > Your'e right saveenv() behaves as you described, but not so > env_relocate_spec(). > > There the flash is probed like this: > env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, > CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); > > and thats the point where some existing dt node becomes modified with > those values. Thats probably wrong doing so. > > I think there should be same logic applied as in saveen(). Tested again, and confirming this behaviour. > > Just for testing i fixed the probe there. > Now i'm a bit confused about that the function > spi_find_chip_select(bus, cs, &dev) in spi-uclass doesn't find child > devices on the spi node. > Instead there a new flash binding is created. > spi_get_bus_and_cs: Binding new device 'spi_flash@0:0', busnum=0, > cs=0, driver=spi_flash_std Debugged into DM and found out that my devicetree didn't represent a valid flash-child. I've now modified it to: => fdt print spi0 spi@e000d000 { clock-names = "ref_clk", "pclk"; clocks = <0x00000001 0x0000000a 0x00000001 0x0000002b>; compatible = "xlnx,zynq-qspi-1.0"; status = "okay"; interrupt-parent = <0x00000003>; interrupts = <0x00000000 0x00000013 0x00000004>; reg = <0xe000d000 0x00001000>; #address-cells = <0x00000001>; #size-cells = <0x00000000>; u-boot,dm-pre-reloc; spi-max-frequency = <0x05f5e100>; spiflash@0 { u-boot,dm-pre-reloc; compatible = "spidev", "spi-flash"; spi-max-frequency = <0x05f5e100>; reg = <0x00000000>; }; }; => So that works and u-boot knows the flash from beginning and uses as correct flash-node from fdt instead creating a new one. Finally i think, we can drop my patch and somebody (maybe me?) should fix the probing during environment relocation. cheers, Hannes
On Wednesday 24 August 2016 07:40 PM, Hannes Schmelzer wrote: > On 08/24/2016 02:21 PM, Hannes Schmelzer wrote: >> On 08/24/2016 01:12 PM, Vignesh R wrote: >>> >>> On Wednesday 24 August 2016 04:21 PM, Hannes Schmelzer wrote: >>>> On 08/24/2016 12:35 PM, Vignesh R wrote: [...] >>> AFAIU, saveenv() uses CONFIG_ENV_SPI_MAX_HZ only when >>> CONFIG_DM_SPI_FLASH is not defined. Could please explain how >>> CONFIG_ENV_SPI_MAX_HZ takes precedence over spi-max-frequency during sf >>> probe? >> Thanks the discussion, I think we coming closer to the problem. >> Your'e right saveenv() behaves as you described, but not so >> env_relocate_spec(). >> >> There the flash is probed like this: >> env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, >> CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); >> >> and thats the point where some existing dt node becomes modified with >> those values. Thats probably wrong doing so. >> >> I think there should be same logic applied as in saveen(). >Tested again, and confirming this behaviour. Yeah, I think relocate_spec() was not updated during dm conversion. >> >> Just for testing i fixed the probe there. >> Now i'm a bit confused about that the function >> spi_find_chip_select(bus, cs, &dev) in spi-uclass doesn't find child >> devices on the spi node. >> Instead there a new flash binding is created. >> spi_get_bus_and_cs: Binding new device 'spi_flash@0:0', busnum=0, >> cs=0, driver=spi_flash_std > Debugged into DM and found out that my devicetree didn't represent a > valid flash-child. > I've now modified it to: > > => fdt print spi0 > spi@e000d000 { > clock-names = "ref_clk", "pclk"; > clocks = <0x00000001 0x0000000a 0x00000001 0x0000002b>; > compatible = "xlnx,zynq-qspi-1.0"; > status = "okay"; > interrupt-parent = <0x00000003>; > interrupts = <0x00000000 0x00000013 0x00000004>; > reg = <0xe000d000 0x00001000>; > #address-cells = <0x00000001>; > #size-cells = <0x00000000>; > u-boot,dm-pre-reloc; > spi-max-frequency = <0x05f5e100>; > spiflash@0 { > u-boot,dm-pre-reloc; > compatible = "spidev", "spi-flash"; Shouldn't this be: compatible = "spansion,S25FL256S_64K", "spi-flash"; as per doc/device-tree-bindings/mtd/spi/spi-flash.txt
On 08/25/2016 10:39 AM, Vignesh R wrote: > > On Wednesday 24 August 2016 07:40 PM, Hannes Schmelzer wrote: >> On 08/24/2016 02:21 PM, Hannes Schmelzer wrote: >>> On 08/24/2016 01:12 PM, Vignesh R wrote: >>>> On Wednesday 24 August 2016 04:21 PM, Hannes Schmelzer wrote: >>>>> On 08/24/2016 12:35 PM, Vignesh R wrote: > [...] > >>>> AFAIU, saveenv() uses CONFIG_ENV_SPI_MAX_HZ only when >>>> CONFIG_DM_SPI_FLASH is not defined. Could please explain how >>>> CONFIG_ENV_SPI_MAX_HZ takes precedence over spi-max-frequency during sf >>>> probe? >>> Thanks the discussion, I think we coming closer to the problem. >>> Your'e right saveenv() behaves as you described, but not so >>> env_relocate_spec(). >>> >>> There the flash is probed like this: >>> env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, >>> CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); >>> >>> and thats the point where some existing dt node becomes modified with >>> those values. Thats probably wrong doing so. >>> >>> I think there should be same logic applied as in saveen(). >> Tested again, and confirming this behaviour. > Yeah, I think relocate_spec() was not updated during dm conversion. looks like, should i submit some patch? > >>> Just for testing i fixed the probe there. >>> Now i'm a bit confused about that the function >>> spi_find_chip_select(bus, cs, &dev) in spi-uclass doesn't find child >>> devices on the spi node. >>> Instead there a new flash binding is created. >>> spi_get_bus_and_cs: Binding new device 'spi_flash@0:0', busnum=0, >>> cs=0, driver=spi_flash_std >> Debugged into DM and found out that my devicetree didn't represent a >> valid flash-child. >> I've now modified it to: >> >> => fdt print spi0 >> spi@e000d000 { >> clock-names = "ref_clk", "pclk"; >> clocks = <0x00000001 0x0000000a 0x00000001 0x0000002b>; >> compatible = "xlnx,zynq-qspi-1.0"; >> status = "okay"; >> interrupt-parent = <0x00000003>; >> interrupts = <0x00000000 0x00000013 0x00000004>; >> reg = <0xe000d000 0x00001000>; >> #address-cells = <0x00000001>; >> #size-cells = <0x00000000>; >> u-boot,dm-pre-reloc; >> spi-max-frequency = <0x05f5e100>; >> spiflash@0 { >> u-boot,dm-pre-reloc; >> compatible = "spidev", "spi-flash"; > Shouldn't this be: compatible = "spansion,S25FL256S_64K", "spi-flash"; > as per doc/device-tree-bindings/mtd/spi/spi-flash.txt yes you're right ... i will fix this.
On Thursday 25 August 2016 03:02 PM, Hannes Schmelzer wrote: > On 08/25/2016 10:39 AM, Vignesh R wrote: >> >> On Wednesday 24 August 2016 07:40 PM, Hannes Schmelzer wrote: >>> On 08/24/2016 02:21 PM, Hannes Schmelzer wrote: >>>> On 08/24/2016 01:12 PM, Vignesh R wrote: >>>>> On Wednesday 24 August 2016 04:21 PM, Hannes Schmelzer wrote: >>>>>> On 08/24/2016 12:35 PM, Vignesh R wrote: >> [...] >> >>>>> AFAIU, saveenv() uses CONFIG_ENV_SPI_MAX_HZ only when >>>>> CONFIG_DM_SPI_FLASH is not defined. Could please explain how >>>>> CONFIG_ENV_SPI_MAX_HZ takes precedence over spi-max-frequency >>>>> during sf >>>>> probe? >>>> Thanks the discussion, I think we coming closer to the problem. >>>> Your'e right saveenv() behaves as you described, but not so >>>> env_relocate_spec(). >>>> >>>> There the flash is probed like this: >>>> env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS, >>>> CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE); >>>> >>>> and thats the point where some existing dt node becomes modified with >>>> those values. Thats probably wrong doing so. >>>> >>>> I think there should be same logic applied as in saveen(). >>> Tested again, and confirming this behaviour. >> Yeah, I think relocate_spec() was not updated during dm conversion. > looks like, should i submit some patch? Patches are welcome. Please Cc U-Boot SPI maintainer Jagan Teki <jteki@openedev.com>. Thanks!
diff --git a/cmd/sf.c b/cmd/sf.c index 286906c..9483806 100644 --- a/cmd/sf.c +++ b/cmd/sf.c @@ -87,6 +87,7 @@ static int do_spi_flash_probe(int argc, char * const argv[]) char *endp; #ifdef CONFIG_DM_SPI_FLASH struct udevice *new, *bus_dev; + struct dm_spi_bus *spi; int ret; /* In DM mode defaults will be taken from DT */ speed = 0, mode = 0; @@ -124,6 +125,15 @@ static int do_spi_flash_probe(int argc, char * const argv[]) /* Remove the old device, otherwise probe will just be a nop */ ret = spi_find_bus_and_cs(bus, cs, &bus_dev, &new); if (!ret) { + if (new != NULL) { + flash = dev_get_uclass_priv(new); + if (flash != NULL) { + speed = flash->spi->speed; + } else { + spi = dev_get_uclass_priv(bus_dev); + speed = spi->max_hz; + } + } device_remove(new); device_unbind(new); }
During probing flashes on the spi bus using the "sf probe" command, a maybe existing flash (from fdt) is unbound and removed to force the 'spi_flash_probe_bus_cs' really scanning the bus. Today the bus is probed with speed 0, this triggers several fall-back mechanism (mostly in the low-level drivers) to catch the impossible zero speed. Result of this is, that the spi-flash runs at very low speed depending on the minimum given by low-level driver/hardware. Values like 'spi-max-frequency' from devicetree are ignored totally today. This commit changes as following: - if there was already some flash binding in devicetree (having some spi-max-frequency within) speed is taken from it - if no flash binding was present for speed the 'spi-max-frequency' from the responsible spi node is taken. Signed-off-by: Hannes Schmelzer <oe5hpm@oevsv.at> --- cmd/sf.c | 10 ++++++++++ 1 file changed, 10 insertions(+)