[U-Boot] spi flash env and driver model
Hi,
I'm looking at a problem on the db-88f6820-amc board (which has it's env on spi flash). Somewhere in the init sequence we get a crash.
git bisect leads me to commit 8fee8845e7 ("enf_sf: reuse setup_flash_device instead of open coding it") but I don't think that's actually the problem. The real problem seems to be the way setup_spi_device() relies on the speed to come from the device-tree. Even when I specify a speed for the spi-flash in the device tree it doesn't seem to stick. The only thing that seems to work is restoring the CONFIG_ENV_SPI_MAX_HZ in the DM_SPI_FLASH case.
I can submit that as a patch but it seems contrary to the intentions of commit 96907c0fe5 ("dm: spi: Read default speed and mode values from DT").
Does anyone have any thoughts as to where to go with this?
Thanks, Chris
On Mon, Aug 7, 2017 at 10:31 PM, Chris Packham judge.packham@gmail.com wrote:
Hi,
I'm looking at a problem on the db-88f6820-amc board (which has it's env on spi flash). Somewhere in the init sequence we get a crash.
git bisect leads me to commit 8fee8845e7 ("enf_sf: reuse setup_flash_device instead of open coding it") but I don't think that's actually the problem. The real problem seems to be the way setup_spi_device() relies on the speed to come from the device-tree. Even when I specify a speed for the spi-flash in the device tree it doesn't seem to stick. The only thing that seems to work is restoring the CONFIG_ENV_SPI_MAX_HZ in the DM_SPI_FLASH case.
I can submit that as a patch but it seems contrary to the intentions of commit 96907c0fe5 ("dm: spi: Read default speed and mode values from DT").
Does anyone have any thoughts as to where to go with this?
So now I've actually been able to see the failure with an ICD attached the problem is a divide by 0 in mvebu_spi_set_speed() because the speed is being passed down as 0. Moving up the stack this appears to be coming from the parent_platdata looked up by spi_get_bus_and_cs(). So it looks like the spi-max-frequency property hasn't been parsed up from the device tree prior to this call.
"jedec,spi-nor" is used by Linux for many boards with spi flash. In fact according to the binding documentation this must be included for any SPI NOR flash that can be identified by the JEDEC READ ID opcode (0x9F). Make device trees more portable between Linux an U-Boot by supporting "jedec,spi-nor" in addition to the U-Boot specific "spi-flash".
Signed-off-by: Chris Packham judge.packham@gmail.com --- This fixes my issues with the spi speed selection on db-88f6820-amc. I didn't find anything in the commit message from the initial implementation saying that "jedec,spi-nor" was omitted intentionally so I've settled on this instead of updating armada-385-amc.dts.
If there is a good reason not to make this change I'm happy to submit a patch that just updates armada-385-amc.dts.
drivers/mtd/spi/sf_probe.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 7b296378d2be..1953ec3cb748 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -164,6 +164,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {
static const struct udevice_id spi_flash_std_ids[] = { { .compatible = "spi-flash" }, + { .compatible = "jedec,spi-nor" }, { } };
On Tuesday 08 August 2017 10:27 AM, Chris Packham wrote:
"jedec,spi-nor" is used by Linux for many boards with spi flash. In fact according to the binding documentation this must be included for any SPI NOR flash that can be identified by the JEDEC READ ID opcode (0x9F). Make device trees more portable between Linux an U-Boot by supporting "jedec,spi-nor" in addition to the U-Boot specific "spi-flash".
Signed-off-by: Chris Packham judge.packham@gmail.com
+1
Use of U-Boot specific compatible "spi-flash" has caused quite a bit of confusions and also problems while syncing U-Boot and kernel DTs. IMO, its better if we could probe JEDEC SPI NOR flashes based on generic "jedec,spi-nor" compatible as well like Linux.
This fixes my issues with the spi speed selection on db-88f6820-amc. I didn't find anything in the commit message from the initial implementation saying that "jedec,spi-nor" was omitted intentionally so I've settled on this instead of updating armada-385-amc.dts.
If there is a good reason not to make this change I'm happy to submit a patch that just updates armada-385-amc.dts.> drivers/mtd/spi/sf_probe.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 7b296378d2be..1953ec3cb748 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -164,6 +164,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {
static const struct udevice_id spi_flash_std_ids[] = { { .compatible = "spi-flash" },
- { .compatible = "jedec,spi-nor" }, { }
};
+Jagan@gmail not sure what your primary u-boot address is
On Wed, Aug 9, 2017 at 1:33 AM, Vignesh R vigneshr@ti.com wrote:
On Tuesday 08 August 2017 10:27 AM, Chris Packham wrote:
"jedec,spi-nor" is used by Linux for many boards with spi flash. In fact according to the binding documentation this must be included for any SPI NOR flash that can be identified by the JEDEC READ ID opcode (0x9F). Make device trees more portable between Linux an U-Boot by supporting "jedec,spi-nor" in addition to the U-Boot specific "spi-flash".
Signed-off-by: Chris Packham judge.packham@gmail.com
+1
Use of U-Boot specific compatible "spi-flash" has caused quite a bit of confusions and also problems while syncing U-Boot and kernel DTs. IMO, its better if we could probe JEDEC SPI NOR flashes based on generic "jedec,spi-nor" compatible as well like Linux.
This fixes my issues with the spi speed selection on db-88f6820-amc. I didn't find anything in the commit message from the initial implementation saying that "jedec,spi-nor" was omitted intentionally so I've settled on this instead of updating armada-385-amc.dts.
If there is a good reason not to make this change I'm happy to submit a patch that just updates armada-385-amc.dts.> drivers/mtd/spi/sf_probe.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 7b296378d2be..1953ec3cb748 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -164,6 +164,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {
static const struct udevice_id spi_flash_std_ids[] = { { .compatible = "spi-flash" },
{ .compatible = "jedec,spi-nor" }, { }};
-- Regards Vignesh
On Tue, Aug 8, 2017 at 10:27 AM, Chris Packham judge.packham@gmail.com wrote:
"jedec,spi-nor" is used by Linux for many boards with spi flash. In fact according to the binding documentation this must be included for any SPI NOR flash that can be identified by the JEDEC READ ID opcode (0x9F). Make device trees more portable between Linux an U-Boot by supporting "jedec,spi-nor" in addition to the U-Boot specific "spi-flash".
Signed-off-by: Chris Packham judge.packham@gmail.com
This fixes my issues with the spi speed selection on db-88f6820-amc. I didn't find anything in the commit message from the initial implementation saying that "jedec,spi-nor" was omitted intentionally so I've settled on this instead of updating armada-385-amc.dts.
If there is a good reason not to make this change I'm happy to submit a patch that just updates armada-385-amc.dts.
At this point try to use spi-flash in dts since we're in WIP for spi-nor support once ie in-place then only we have sync the compatible of what Linux is using.
thanks!
participants (3)
-
Chris Packham -
Jagan Teki -
Vignesh R