Problem loading environment from spi-nor flash partition since barebox 2017.01.0

Problem loading environment from spi-nor flash partition since barebox 2017.01.0

[Ian Abbott]

Hi!

I thought I'd try updating my custom SoCFPGA-based board from barebox 
2016.11.0 to 2017.01.0, and have only run into one problem, which is 
that it is no longer loading the barebox environment during boot:

---------------------------------------------------------
barebox 2017.01.0 #1 Tue Jan 10 14:01:57 GMT 2017


Board: Belltech dbPod SoCFPGA
designware_eth ff702000.ethernet: user ID: 0x10, Synopsys ID: 0x37
mdio_bus: miibus0: probed
flash@0: s25fl256s1 (32768 Kbytes)
cadence_qspi ff705000.spi: Cadence QSPI NOR flash driver
malloc space: 0x0ff7dd00 -> 0x1fefb9ff (size 255.5 MiB)
barebox-environment environment.5: probe failed: No such device
environment load /dev/env0: No such file or directory
Maybe you have to create the partition.
---------------------------------------------------------


After a bit of poking around, I discovered that it is failing in 
__of_find_path() at these lines:

	if (!dev->driver)
		return -ENODEV;

Those lines were added by commit 
82eb3dff10ae0c0436e0fcd5ff0c9cd2a2caab3c ("of_path: handle no driver for 
device").

I'm not sure which bit of code is responsible for setting dev->driver 
(or whether it is appropriate in this case), but if I remove that test, 
the environment loads OK.

Here is the barebox DTS I am compiling for my board (sorry about any 
line-wrapping):

---------------------------------------------------------
#include <arm/socfpga_cyclone5.dtsi>
#include "socfpga.dtsi"

/ {
	model = "Belltech dbPod SoCFPGA";
	compatible = "belltech,dbpod", "altr,socfpga-cyclone5", "altr,socfpga";

	chosen {

		environment@0 {
			compatible = "barebox,environment";
			device-path = &flash, "partname:barebox-environment";
			/delete-property/file-path;
		};
	};

	aliases {
		ethernet0 = &gmac1;
		/delete-property/ethernet1;
	};

	memory {
		name = "memory";
		device_type = "memory";
		reg = <0x0 0x20000000>; /* 512GB */
	};
};

&gmac1 {
	phy-mode = "rgmii";
	status = "okay";
};

&gpio0 {
	status = "okay";
};

&i2c0 {
	status = "okay";
};

&qspi {
	status = "okay";

	flash: flash@0 {
		#address-cells = <1>;
		#size-cells = <1>;
		compatible = "n25q00";
		reg = <0>;
		spi-max-frequency = <100000000>;
		m25p,fast-read;
		cdns,page-size = <256>;
		cdns,block-size = <16>;
		cdns,read-delay = <4>;
		cdns,tshsl-ns = <50>;
		cdns,tsd2d-ns = <50>;
		cdns,tchsh-ns = <4>;
		cdns,tslch-ns = <4>;

		partition@0 {
			label = "prebootloader0";
			reg = <0x00000 0x10000>;
		};

		partition@10000 {
			label = "prebootloader1";
			reg = <0x10000 0x10000>;
		};

		partition@20000 {
			label = "prebootloader2";
			reg = <0x20000 0x10000>;
		};

		partition@30000 {
			label = "prebootloader3";
			reg = <0x30000 0x10000>;
		};

		partition@40000 {
			label = "barebox";
			reg = <0x40000 0x80000>;
		};

		partition@c0000 {
			label = "barebox-environment";
			reg = <0xc0000 0x20000>;
		};

		partition@e0000 {
			label = "barebox-state";
			reg = <0xe0000 0x20000>;
		};

		partition@100000 {
			label = "ubi";
			reg = <0x100000 0x0>;
		};
	};
};
---------------------------------------------------------

Best regards,
Ian Abbott.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Problem loading environment from spi-nor flash partition since barebox 2017.01.0

[Trent Piepho]

On Tue, 2017-01-10 at 16:01 +0000, Ian Abbott wrote:
> Hi!
> 
> I thought I'd try updating my custom SoCFPGA-based board from barebox 
> 2016.11.0 to 2017.01.0, and have only run into one problem, which is 
> that it is no longer loading the barebox environment during boot:
> 
> barebox-environment environment.5: probe failed: No such device
> environment load /dev/env0: No such file or directory

> After a bit of poking around, I discovered that it is failing in 
> __of_find_path() at these lines:
> 
> 	if (!dev->driver)
> 		return -ENODEV;
> 
> Those lines were added by commit 
> 82eb3dff10ae0c0436e0fcd5ff0c9cd2a2caab3c ("of_path: handle no driver for 
> device").

I assume the call to __of_find_path() is traced back to
environment_probe()?

It looks to me like the environment driver needs to be updated to
support PROBE_DEFER.  It should detect ENODEV from of_find_path() as a
possible device not probed yet error and return PROBE_DEFER.

I wonder if of_find_path should return EPROBE_DEFER itself?  It seems
like it might be easier to catch this in of_find_path() once instead of
in all callers of of_find_path().

> I'm not sure which bit of code is responsible for setting dev->driver 
> (or whether it is appropriate in this case), but if I remove that test, 
> the environment loads OK.

This makes me wonder if probe defer, which I think is still problem, is
not your problem.  

> 		environment@0 {
> 			compatible = "barebox,environment";
> 			device-path = &flash, "partname:barebox-environment";

> &qspi {
> 	flash: flash@0 {
> 		partition@c0000 {
> 			label = "barebox-environment";
> 			reg = <0xc0000 0x20000>;
> 		};

In this case, the env points to the flash node for the specific chip
select, which is a child of the node for the qspi device.  qspi has a
driver.  But the child for the chip select isn't really a device in its
own right, it just part of the qspi device.  I don't believe there is a
driver for it.  I wonder if dev->driver will always be NULL for that
device?

Maybe the bug here is that a device node for a flash chip select or a
partition doesn't have a driver itself.  So either the !dev->driver test
is wrong, or the device should have ->driver point to the driver of its
parent device.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Problem loading environment from spi-nor flash partition since barebox 2017.01.0

[Ian Abbott]

On 10/01/17 19:42, Trent Piepho wrote:
> On Tue, 2017-01-10 at 16:01 +0000, Ian Abbott wrote:
>> Hi!
>>
>> I thought I'd try updating my custom SoCFPGA-based board from barebox
>> 2016.11.0 to 2017.01.0, and have only run into one problem, which is
>> that it is no longer loading the barebox environment during boot:
>>
>> barebox-environment environment.5: probe failed: No such device
>> environment load /dev/env0: No such file or directory
>
>> After a bit of poking around, I discovered that it is failing in
>> __of_find_path() at these lines:
>>
>> 	if (!dev->driver)
>> 		return -ENODEV;
>>
>> Those lines were added by commit
>> 82eb3dff10ae0c0436e0fcd5ff0c9cd2a2caab3c ("of_path: handle no driver for
>> device").
>
> I assume the call to __of_find_path() is traced back to
> environment_probe()?

Yes.

> It looks to me like the environment driver needs to be updated to
> support PROBE_DEFER.  It should detect ENODEV from of_find_path() as a
> possible device not probed yet error and return PROBE_DEFER.
>
> I wonder if of_find_path should return EPROBE_DEFER itself?  It seems
> like it might be easier to catch this in of_find_path() once instead of
> in all callers of of_find_path().

Probably.  The only other (indirect) user of __of_find_path() I can find 
seems to be the "barebox,state" driver ("drivers/misc/state.c" and 
common/state/state.c).  If __of_find_path() could return -EPROBE_DEFER 
itself, on first glance it looks like state_probe() in 
"drivers/misc/state.c" wouldn't need to bother converting -ENODEV to 
-EPROBE_DEFER.

>
>> I'm not sure which bit of code is responsible for setting dev->driver
>> (or whether it is appropriate in this case), but if I remove that test,
>> the environment loads OK.
>
> This makes me wonder if probe defer, which I think is still problem, is
> not your problem.
>
>> 		environment@0 {
>> 			compatible = "barebox,environment";
>> 			device-path = &flash, "partname:barebox-environment";
>
>> &qspi {
>> 	flash: flash@0 {
>> 		partition@c0000 {
>> 			label = "barebox-environment";
>> 			reg = <0xc0000 0x20000>;
>> 		};
>
> In this case, the env points to the flash node for the specific chip
> select, which is a child of the node for the qspi device.  qspi has a
> driver.  But the child for the chip select isn't really a device in its
> own right, it just part of the qspi device.  I don't believe there is a
> driver for it.  I wonder if dev->driver will always be NULL for that
> device?
>
> Maybe the bug here is that a device node for a flash chip select or a
> partition doesn't have a driver itself.  So either the !dev->driver test
> is wrong, or the device should have ->driver point to the driver of its
> parent device.

Yes, the problem seems to be that the cadence-qspi driver doesn't behave 
as a proper bus driver (in the barebox/Linux driver model sense).

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Problem loading environment from spi-nor flash partition since barebox 2017.01.0

[Sascha Hauer]

On Tue, Jan 10, 2017 at 04:01:40PM +0000, Ian Abbott wrote:
> Hi!
> 
> I thought I'd try updating my custom SoCFPGA-based board from barebox
> 2016.11.0 to 2017.01.0, and have only run into one problem, which is that it
> is no longer loading the barebox environment during boot:
> 
> &qspi {
> 	status = "okay";
> 
> 	flash: flash@0 {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		compatible = "n25q00";
> 		reg = <0>;
> 		spi-max-frequency = <100000000>;
> 		m25p,fast-read;
> 		cdns,page-size = <256>;
> 		cdns,block-size = <16>;
> 		cdns,read-delay = <4>;
> 		cdns,tshsl-ns = <50>;
> 		cdns,tsd2d-ns = <50>;
> 		cdns,tchsh-ns = <4>;
> 		cdns,tslch-ns = <4>;

The problem is that your environment path points to the flash node which
does not have a driver as the corresponding device is not registered
properly.

With (real) SPI this is a little different and works as expected: If the
qspi node would be handled by the SPI layer then the SPI core would
register the child nodes as devices on a SPI bus. The normal probe
mechanism would then bind the device and the driver together.

With the cadence-quadspi driver a device is registered in
cqspi_setup_flash(), but there is never a driver attached to it, thus
the dev->driver test fails.

The proper way if probably to register the n25q00 device on a qspi bus
and to provide a qspi-nor-flash driver which gets probed then.
The not-so-proper, faster way could be to just create a dummy driver
struct and attach it to the device allocated in cqspi_setup_flash().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Problem loading environment from spi-nor flash partition since barebox 2017.01.0

[Ian Abbott]

On 11/01/17 08:32, Sascha Hauer wrote:
> On Tue, Jan 10, 2017 at 04:01:40PM +0000, Ian Abbott wrote:
>> Hi!
>>
>> I thought I'd try updating my custom SoCFPGA-based board from barebox
>> 2016.11.0 to 2017.01.0, and have only run into one problem, which is that it
>> is no longer loading the barebox environment during boot:
>>
>> &qspi {
>> 	status = "okay";
>>
>> 	flash: flash@0 {
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>> 		compatible = "n25q00";
>> 		reg = <0>;
>> 		spi-max-frequency = <100000000>;
>> 		m25p,fast-read;
>> 		cdns,page-size = <256>;
>> 		cdns,block-size = <16>;
>> 		cdns,read-delay = <4>;
>> 		cdns,tshsl-ns = <50>;
>> 		cdns,tsd2d-ns = <50>;
>> 		cdns,tchsh-ns = <4>;
>> 		cdns,tslch-ns = <4>;
>
> The problem is that your environment path points to the flash node which
> does not have a driver as the corresponding device is not registered
> properly.
>
> With (real) SPI this is a little different and works as expected: If the
> qspi node would be handled by the SPI layer then the SPI core would
> register the child nodes as devices on a SPI bus. The normal probe
> mechanism would then bind the device and the driver together.
>
> With the cadence-quadspi driver a device is registered in
> cqspi_setup_flash(), but there is never a driver attached to it, thus
> the dev->driver test fails.
>
> The proper way if probably to register the n25q00 device on a qspi bus
> and to provide a qspi-nor-flash driver which gets probed then.
> The not-so-proper, faster way could be to just create a dummy driver
> struct and attach it to the device allocated in cqspi_setup_flash().

The 'compatible' string in the flash device node is not used by the 
current cadence-quadspi driver.  "n25q00" is not the most appropriate 
value for the 'compatible' string in this device node (I merely copied 
it from other socfpga-based boards!).  The most appropriate value would 
be "jedec,spi-nor", as registered by the m25p80 driver, but "n25q00" 
could also be registered by the new device driver for backwards 
compatibility.  I guess it's okay for more than one driver to register 
the same compatible id as long as the drivers are registered to 
different bus types.

I just had another thought about the '!dev->driver' test in 
__of_find_path().  Would modifying the test to also check 'dev->bus' as 
follows solve the problem?:

	if (dev->bus && !dev->driver)
		return -ENODEV;

(Or possibly 'return -EPROBE_DEFER' would be better, as suggested by 
Trent Piepho's reply, but that would be a separate patch.)

The devices created by the cadence-quadspi driver do not set 'dev->bus', 
but devices created by proper bus drivers should do so.  I just tried 
it, and it seems to work around my problem.

Best regards,
Ian.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Problem loading environment from spi-nor flash partition since barebox 2017.01.0

[Trent Piepho]

On Wed, 2017-01-11 at 09:32 +0100, Sascha Hauer wrote:
> With (real) SPI this is a little different and works as expected: If the
> qspi node would be handled by the SPI layer then the SPI core would
> register the child nodes as devices on a SPI bus. The normal probe
> mechanism would then bind the device and the driver together.
> 
> With the cadence-quadspi driver a device is registered in
> cqspi_setup_flash(), but there is never a driver attached to it, thus
> the dev->driver test fails.
> 
> The proper way if probably to register the n25q00 device on a qspi bus
> and to provide a qspi-nor-flash driver which gets probed then.
> The not-so-proper, faster way could be to just create a dummy driver
> struct and attach it to the device allocated in cqspi_setup_flash().

The qspi device is more like an MTD device than a SPI master.  It just
supports memory devices, not arbitrary SPI slaves that have their own
drivers.

But that said, there is sort of a driver for the SPI NOR chips attached
to the qspi in spi-nor.c, but it is not a real 'struct driver_d' driver.
Maybe it could be?

qspi could create a "qspi" or "spi-flash" bus (I don't think it will fit
well as a generic SPI bus) with the flash devices on it, and then
spi-nor could bind to them like a normal driver.

Or spi-nor could have a driver_d that's not registered and make
nor->dev->driver point to it in spi_nor_scan().

Or spi_nor_scan() could just set nor->dev->driver =
nor->dev->parent->driver.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Problem loading environment from spi-nor flash partition since barebox 2017.01.0

[s.hauer]

On Fri, Jan 13, 2017 at 05:46:41PM +0000, Trent Piepho wrote:
> On Wed, 2017-01-11 at 09:32 +0100, Sascha Hauer wrote:
> > With (real) SPI this is a little different and works as expected: If the
> > qspi node would be handled by the SPI layer then the SPI core would
> > register the child nodes as devices on a SPI bus. The normal probe
> > mechanism would then bind the device and the driver together.
> > 
> > With the cadence-quadspi driver a device is registered in
> > cqspi_setup_flash(), but there is never a driver attached to it, thus
> > the dev->driver test fails.
> > 
> > The proper way if probably to register the n25q00 device on a qspi bus
> > and to provide a qspi-nor-flash driver which gets probed then.
> > The not-so-proper, faster way could be to just create a dummy driver
> > struct and attach it to the device allocated in cqspi_setup_flash().
> 
> The qspi device is more like an MTD device than a SPI master.  It just
> supports memory devices, not arbitrary SPI slaves that have their own
> drivers.
> 
> But that said, there is sort of a driver for the SPI NOR chips attached
> to the qspi in spi-nor.c, but it is not a real 'struct driver_d' driver.
> Maybe it could be?
> 
> qspi could create a "qspi" or "spi-flash" bus (I don't think it will fit
> well as a generic SPI bus) with the flash devices on it, and then
> spi-nor could bind to them like a normal driver.

Yes, that was what I tried to express in my mail.

> 
> Or spi-nor could have a driver_d that's not registered and make
> nor->dev->driver point to it in spi_nor_scan().

Yes.

> 
> Or spi_nor_scan() could just set nor->dev->driver =
> nor->dev->parent->driver.

Probably not a good idea. I looks wrong.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox