[U-Boot] [PATCH] spi: Fix manual relocation calling more times
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com ---
drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 88cb2a126227..6d63006d186c 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) #endif #if defined(CONFIG_NEEDS_MANUAL_RELOC) struct dm_spi_ops *ops = spi_get_ops(bus); - - if (ops->claim_bus) - ops->claim_bus += gd->reloc_off; - if (ops->release_bus) - ops->release_bus += gd->reloc_off; - if (ops->set_wordlen) - ops->set_wordlen += gd->reloc_off; - if (ops->xfer) - ops->xfer += gd->reloc_off; - if (ops->set_speed) - ops->set_speed += gd->reloc_off; - if (ops->set_mode) - ops->set_mode += gd->reloc_off; - if (ops->cs_info) - ops->cs_info += gd->reloc_off; + static int reloc_done; + + if (!reloc_done) { + if (ops->claim_bus) + ops->claim_bus += gd->reloc_off; + if (ops->release_bus) + ops->release_bus += gd->reloc_off; + if (ops->set_wordlen) + ops->set_wordlen += gd->reloc_off; + if (ops->xfer) + ops->xfer += gd->reloc_off; + if (ops->set_speed) + ops->set_speed += gd->reloc_off; + if (ops->set_mode) + ops->set_mode += gd->reloc_off; + if (ops->cs_info) + ops->cs_info += gd->reloc_off; + reloc_done++; + } #endif
return 0;
On Tue, Sep 17, 2019 at 4:52 PM Michal Simek michal.simek@xilinx.com wrote:
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com
Reviewed-by: Jagan Teki jagan@amarulasolutions.com
Would like to take it coming MW? since we are close to release.
On 17. 09. 19 14:41, Jagan Teki wrote:
On Tue, Sep 17, 2019 at 4:52 PM Michal Simek michal.simek@xilinx.com wrote:
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com
Reviewed-by: Jagan Teki jagan@amarulasolutions.com
Would like to take it coming MW? since we are close to release.
Taking it for next release is fine.
M
On Tue, Sep 17, 2019 at 1:22 PM Michal Simek michal.simek@xilinx.com wrote:
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 88cb2a126227..6d63006d186c 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) #endif #if defined(CONFIG_NEEDS_MANUAL_RELOC) struct dm_spi_ops *ops = spi_get_ops(bus);
if (ops->claim_bus)ops->claim_bus += gd->reloc_off;if (ops->release_bus)ops->release_bus += gd->reloc_off;if (ops->set_wordlen)ops->set_wordlen += gd->reloc_off;if (ops->xfer)ops->xfer += gd->reloc_off;if (ops->set_speed)ops->set_speed += gd->reloc_off;if (ops->set_mode)ops->set_mode += gd->reloc_off;if (ops->cs_info)ops->cs_info += gd->reloc_off;
static int reloc_done;
So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple (differen) spi (or watchdog, sf, gpio) drivers?
Regards, Simon
if (!reloc_done) {if (ops->claim_bus)ops->claim_bus += gd->reloc_off;if (ops->release_bus)ops->release_bus += gd->reloc_off;if (ops->set_wordlen)ops->set_wordlen += gd->reloc_off;if (ops->xfer)ops->xfer += gd->reloc_off;if (ops->set_speed)ops->set_speed += gd->reloc_off;if (ops->set_mode)ops->set_mode += gd->reloc_off;if (ops->cs_info)ops->cs_info += gd->reloc_off;reloc_done++;}#endif
return 0;-- 2.17.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 17. 09. 19 14:43, Simon Goldschmidt wrote:
On Tue, Sep 17, 2019 at 1:22 PM Michal Simek michal.simek@xilinx.com wrote:
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 88cb2a126227..6d63006d186c 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) #endif #if defined(CONFIG_NEEDS_MANUAL_RELOC) struct dm_spi_ops *ops = spi_get_ops(bus);
if (ops->claim_bus)ops->claim_bus += gd->reloc_off;if (ops->release_bus)ops->release_bus += gd->reloc_off;if (ops->set_wordlen)ops->set_wordlen += gd->reloc_off;if (ops->xfer)ops->xfer += gd->reloc_off;if (ops->set_speed)ops->set_speed += gd->reloc_off;if (ops->set_mode)ops->set_mode += gd->reloc_off;if (ops->cs_info)ops->cs_info += gd->reloc_off;
static int reloc_done;So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple (differen) spi (or watchdog, sf, gpio) drivers?
If you look at commits above you will see that several subsystems support multiple instances already. We can check others as well.
M
On Tue, Sep 17, 2019 at 2:48 PM Michal Simek michal.simek@xilinx.com wrote:
On 17. 09. 19 14:43, Simon Goldschmidt wrote:
On Tue, Sep 17, 2019 at 1:22 PM Michal Simek michal.simek@xilinx.com wrote:
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 88cb2a126227..6d63006d186c 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) #endif #if defined(CONFIG_NEEDS_MANUAL_RELOC) struct dm_spi_ops *ops = spi_get_ops(bus);
if (ops->claim_bus)ops->claim_bus += gd->reloc_off;if (ops->release_bus)ops->release_bus += gd->reloc_off;if (ops->set_wordlen)ops->set_wordlen += gd->reloc_off;if (ops->xfer)ops->xfer += gd->reloc_off;if (ops->set_speed)ops->set_speed += gd->reloc_off;if (ops->set_mode)ops->set_mode += gd->reloc_off;if (ops->cs_info)ops->cs_info += gd->reloc_off;
static int reloc_done;So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple (differen) spi (or watchdog, sf, gpio) drivers?
If you look at commits above you will see that several subsystems support multiple instances already. We can check others as well.
I have looked at the commits above, that's why I ask. To me, this code relocates the ops of the device called first. If a second device has different ops, it won't get relocated, or am I wrong?
Given your other similar commits that were already accepted, I don't want to hold this one, but I still would like to understand.
Regards, Simon
M
On 17. 09. 19 14:53, Simon Goldschmidt wrote:
On Tue, Sep 17, 2019 at 2:48 PM Michal Simek michal.simek@xilinx.com wrote:
On 17. 09. 19 14:43, Simon Goldschmidt wrote:
On Tue, Sep 17, 2019 at 1:22 PM Michal Simek michal.simek@xilinx.com wrote:
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 88cb2a126227..6d63006d186c 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) #endif #if defined(CONFIG_NEEDS_MANUAL_RELOC) struct dm_spi_ops *ops = spi_get_ops(bus);
if (ops->claim_bus)ops->claim_bus += gd->reloc_off;if (ops->release_bus)ops->release_bus += gd->reloc_off;if (ops->set_wordlen)ops->set_wordlen += gd->reloc_off;if (ops->xfer)ops->xfer += gd->reloc_off;if (ops->set_speed)ops->set_speed += gd->reloc_off;if (ops->set_mode)ops->set_mode += gd->reloc_off;if (ops->cs_info)ops->cs_info += gd->reloc_off;
static int reloc_done;So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple (differen) spi (or watchdog, sf, gpio) drivers?
If you look at commits above you will see that several subsystems support multiple instances already. We can check others as well.
I have looked at the commits above, that's why I ask. To me, this code relocates the ops of the device called first. If a second device has different ops, it won't get relocated, or am I wrong?
Given your other similar commits that were already accepted, I don't want to hold this one, but I still would like to understand.
I think you are right. It should be called once per driver and after relocation. That means that this should be the part variable should be the part of every ops and there should be also handling in connection to bind/unbind method. On microblaze which is one architecture common systems only one driver is in place that's why none has found any issue with it.
Let me play with this a little bit to see if we can setup system with also bind/unbind to see how this is exactly working.
Thanks, Michal
Michal Simek michal.simek@xilinx.com schrieb am Do., 19. Sep. 2019, 16:22:
On 17. 09. 19 14:53, Simon Goldschmidt wrote:
On Tue, Sep 17, 2019 at 2:48 PM Michal Simek michal.simek@xilinx.com
wrote:
On 17. 09. 19 14:43, Simon Goldschmidt wrote:
On Tue, Sep 17, 2019 at 1:22 PM Michal Simek michal.simek@xilinx.com
wrote:
From: Ashok Reddy Soma ashok.reddy.soma@xilinx.com
When two instances of AXI QSPI with flash are added and tested simultaneously the spi driver operations are relocated twice. As a result code is accessing addresses outside of RAM when relocated second time which is causing a crash.
Tested on Microblaze.
Similar change was done in past by: commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for
watchdogs")
commit 2588f2ddfd60 ("dm: sf: Add support for all targets which
requires MANUAL_RELOC")
commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio")
Signed-off-by: Ashok Reddy Soma ashok.reddy.soma@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 88cb2a126227..6d63006d186c 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) #endif #if defined(CONFIG_NEEDS_MANUAL_RELOC) struct dm_spi_ops *ops = spi_get_ops(bus);
if (ops->claim_bus)ops->claim_bus += gd->reloc_off;if (ops->release_bus)ops->release_bus += gd->reloc_off;if (ops->set_wordlen)ops->set_wordlen += gd->reloc_off;if (ops->xfer)ops->xfer += gd->reloc_off;if (ops->set_speed)ops->set_speed += gd->reloc_off;if (ops->set_mode)ops->set_mode += gd->reloc_off;if (ops->cs_info)ops->cs_info += gd->reloc_off;
static int reloc_done;So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple (differen) spi (or watchdog, sf, gpio) drivers?
If you look at commits above you will see that several subsystems support multiple instances already. We can check others as well.
I have looked at the commits above, that's why I ask. To me, this code relocates the ops of the device called first. If a second device has
different
ops, it won't get relocated, or am I wrong?
Given your other similar commits that were already accepted, I don't
want to
hold this one, but I still would like to understand.
I think you are right. It should be called once per driver and after relocation. That means that this should be the part variable should be the part of every ops and there should be also handling in connection to bind/unbind method. On microblaze which is one architecture common systems only one driver is in place that's why none has found any issue with it.
Let me play with this a little bit to see if we can setup system with also bind/unbind to see how this is exactly working.
Right, that's what I came to when thinking about it again. That way, it could even be the framework that relocates all ops...
Regards, Simon
Thanks, Michal
On 19. 09. 19 16:35, Simon Goldschmidt wrote:
Michal Simek <michal.simek@xilinx.com mailto:michal.simek@xilinx.com> schrieb am Do., 19. Sep. 2019, 16:22:
On 17. 09. 19 14:53, Simon Goldschmidt wrote: > On Tue, Sep 17, 2019 at 2:48 PM Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>> wrote: >> >> On 17. 09. 19 14:43, Simon Goldschmidt wrote: >>> On Tue, Sep 17, 2019 at 1:22 PM Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>> wrote: >>>> >>>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com>> >>>> >>>> When two instances of AXI QSPI with flash are added and tested >>>> simultaneously the spi driver operations are relocated twice. >>>> As a result code is accessing addresses outside of RAM when >>>> relocated second time which is causing a crash. >>>> >>>> Tested on Microblaze. >>>> >>>> Similar change was done in past by: >>>> commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") >>>> commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") >>>> commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio") >>>> >>>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com>> >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>> >>>> --- >>>> >>>> drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- >>>> 1 file changed, 19 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >>>> index 88cb2a126227..6d63006d186c 100644 >>>> --- a/drivers/spi/spi-uclass.c >>>> +++ b/drivers/spi/spi-uclass.c >>>> @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) >>>> #endif >>>> #if defined(CONFIG_NEEDS_MANUAL_RELOC) >>>> struct dm_spi_ops *ops = spi_get_ops(bus); >>>> - >>>> - if (ops->claim_bus) >>>> - ops->claim_bus += gd->reloc_off; >>>> - if (ops->release_bus) >>>> - ops->release_bus += gd->reloc_off; >>>> - if (ops->set_wordlen) >>>> - ops->set_wordlen += gd->reloc_off; >>>> - if (ops->xfer) >>>> - ops->xfer += gd->reloc_off; >>>> - if (ops->set_speed) >>>> - ops->set_speed += gd->reloc_off; >>>> - if (ops->set_mode) >>>> - ops->set_mode += gd->reloc_off; >>>> - if (ops->cs_info) >>>> - ops->cs_info += gd->reloc_off; >>>> + static int reloc_done; >>> >>> So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple >>> (differen) spi (or watchdog, sf, gpio) drivers? >> >> If you look at commits above you will see that several subsystems >> support multiple instances already. >> We can check others as well. > > I have looked at the commits above, that's why I ask. To me, this code > relocates the ops of the device called first. If a second device has different > ops, it won't get relocated, or am I wrong? > > Given your other similar commits that were already accepted, I don't want to > hold this one, but I still would like to understand. I think you are right. It should be called once per driver and after relocation. That means that this should be the part variable should be the part of every ops and there should be also handling in connection to bind/unbind method. On microblaze which is one architecture common systems only one driver is in place that's why none has found any issue with it. Let me play with this a little bit to see if we can setup system with also bind/unbind to see how this is exactly working.Right, that's what I came to when thinking about it again. That way, it could even be the framework that relocates all ops...
Framework is doing it already but we should do it without static variable.
Thanks, Michal
Michal Simek michal.simek@xilinx.com schrieb am Fr., 20. Sep. 2019, 10:17:
On 19. 09. 19 16:35, Simon Goldschmidt wrote:
Michal Simek <michal.simek@xilinx.com mailto:michal.simek@xilinx.com> schrieb am Do., 19. Sep. 2019, 16:22:
On 17. 09. 19 14:53, Simon Goldschmidt wrote: > On Tue, Sep 17, 2019 at 2:48 PM Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>> wrote: >> >> On 17. 09. 19 14:43, Simon Goldschmidt wrote: >>> On Tue, Sep 17, 2019 at 1:22 PM Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>> wrote: >>>> >>>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com>> >>>> >>>> When two instances of AXI QSPI with flash are added and tested >>>> simultaneously the spi driver operations are relocated twice. >>>> As a result code is accessing addresses outside of RAM when >>>> relocated second time which is causing a crash. >>>> >>>> Tested on Microblaze. >>>> >>>> Similar change was done in past by: >>>> commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation for watchdogs") >>>> commit 2588f2ddfd60 ("dm: sf: Add support for all targets which requires MANUAL_RELOC") >>>> commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for gpio") >>>> >>>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com>> >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>> >>>> --- >>>> >>>> drivers/spi/spi-uclass.c | 34+++++++++++++++++++---------------
>>>> 1 file changed, 19 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >>>> index 88cb2a126227..6d63006d186c 100644 >>>> --- a/drivers/spi/spi-uclass.c >>>> +++ b/drivers/spi/spi-uclass.c >>>> @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice *bus) >>>> #endif >>>> #if defined(CONFIG_NEEDS_MANUAL_RELOC) >>>> struct dm_spi_ops *ops = spi_get_ops(bus); >>>> - >>>> - if (ops->claim_bus) >>>> - ops->claim_bus += gd->reloc_off; >>>> - if (ops->release_bus) >>>> - ops->release_bus += gd->reloc_off; >>>> - if (ops->set_wordlen) >>>> - ops->set_wordlen += gd->reloc_off; >>>> - if (ops->xfer) >>>> - ops->xfer += gd->reloc_off; >>>> - if (ops->set_speed) >>>> - ops->set_speed += gd->reloc_off; >>>> - if (ops->set_mode) >>>> - ops->set_mode += gd->reloc_off; >>>> - if (ops->cs_info) >>>> - ops->cs_info += gd->reloc_off; >>>> + static int reloc_done; >>> >>> So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple >>> (differen) spi (or watchdog, sf, gpio) drivers? >> >> If you look at commits above you will see that several subsystems >> support multiple instances already. >> We can check others as well. > > I have looked at the commits above, that's why I ask. To me, thiscode
> relocates the ops of the device called first. If a second device has different > ops, it won't get relocated, or am I wrong? > > Given your other similar commits that were already accepted, I don't want to > hold this one, but I still would like to understand. I think you are right. It should be called once per driver and after relocation. That means that this should be the part variable shouldbe
the part of every ops and there should be also handling inconnection to
bind/unbind method. On microblaze which is one architecture common systems only onedriver
is in place that's why none has found any issue with it. Let me play with this a little bit to see if we can setup system with also bind/unbind to see how this is exactly working.Right, that's what I came to when thinking about it again. That way, it could even be the framework that relocates all ops...
Framework is doing it already but we should do it without static variable.
Right, it's done in the UCLASSes. What I meant was it could already be done in the core DM framework.
Regards, Simon
Thanks, Michal
On 20. 09. 19 10:26, Simon Goldschmidt wrote:
Michal Simek <michal.simek@xilinx.com mailto:michal.simek@xilinx.com> schrieb am Fr., 20. Sep. 2019, 10:17:
On 19. 09. 19 16:35, Simon Goldschmidt wrote: > > > Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com> <mailto:michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>>> > schrieb am Do., 19. Sep. 2019, 16:22: > > On 17. 09. 19 14:53, Simon Goldschmidt wrote: > > On Tue, Sep 17, 2019 at 2:48 PM Michal Simek > <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com> <mailto:michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>>> wrote: > >> > >> On 17. 09. 19 14:43, Simon Goldschmidt wrote: > >>> On Tue, Sep 17, 2019 at 1:22 PM Michal Simek > <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com> <mailto:michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>>> wrote: > >>>> > >>>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com> > <mailto:ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com>>> > >>>> > >>>> When two instances of AXI QSPI with flash are added and tested > >>>> simultaneously the spi driver operations are relocated twice. > >>>> As a result code is accessing addresses outside of RAM when > >>>> relocated second time which is causing a crash. > >>>> > >>>> Tested on Microblaze. > >>>> > >>>> Similar change was done in past by: > >>>> commit f238b3f0fbc9 ("watchdog: dm: Support manual relocation > for watchdogs") > >>>> commit 2588f2ddfd60 ("dm: sf: Add support for all targets which > requires MANUAL_RELOC") > >>>> commit 1b4c2aa25bdf ("gpio: dm: Support manual relocation for > gpio") > >>>> > >>>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com> > <mailto:ashok.reddy.soma@xilinx.com <mailto:ashok.reddy.soma@xilinx.com>>> > >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com <mailto:michal.simek@xilinx.com> > <mailto:michal.simek@xilinx.com <mailto:michal.simek@xilinx.com>>> > >>>> --- > >>>> > >>>> drivers/spi/spi-uclass.c | 34 +++++++++++++++++++--------------- > >>>> 1 file changed, 19 insertions(+), 15 deletions(-) > >>>> > >>>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c > >>>> index 88cb2a126227..6d63006d186c 100644 > >>>> --- a/drivers/spi/spi-uclass.c > >>>> +++ b/drivers/spi/spi-uclass.c > >>>> @@ -129,21 +129,25 @@ static int spi_post_probe(struct udevice > *bus) > >>>> #endif > >>>> #if defined(CONFIG_NEEDS_MANUAL_RELOC) > >>>> struct dm_spi_ops *ops = spi_get_ops(bus); > >>>> - > >>>> - if (ops->claim_bus) > >>>> - ops->claim_bus += gd->reloc_off; > >>>> - if (ops->release_bus) > >>>> - ops->release_bus += gd->reloc_off; > >>>> - if (ops->set_wordlen) > >>>> - ops->set_wordlen += gd->reloc_off; > >>>> - if (ops->xfer) > >>>> - ops->xfer += gd->reloc_off; > >>>> - if (ops->set_speed) > >>>> - ops->set_speed += gd->reloc_off; > >>>> - if (ops->set_mode) > >>>> - ops->set_mode += gd->reloc_off; > >>>> - if (ops->cs_info) > >>>> - ops->cs_info += gd->reloc_off; > >>>> + static int reloc_done; > >>> > >>> So with CONFIG_NEEDS_MANUAL_RELOC, we don't support multiple > >>> (differen) spi (or watchdog, sf, gpio) drivers? > >> > >> If you look at commits above you will see that several subsystems > >> support multiple instances already. > >> We can check others as well. > > > > I have looked at the commits above, that's why I ask. To me, this code > > relocates the ops of the device called first. If a second device > has different > > ops, it won't get relocated, or am I wrong? > > > > Given your other similar commits that were already accepted, I > don't want to > > hold this one, but I still would like to understand. > > I think you are right. It should be called once per driver and after > relocation. That means that this should be the part variable should be > the part of every ops and there should be also handling in connection to > bind/unbind method. > On microblaze which is one architecture common systems only one driver > is in place that's why none has found any issue with it. > > Let me play with this a little bit to see if we can setup system with > also bind/unbind to see how this is exactly working. > > > Right, that's what I came to when thinking about it again. That way, it > could even be the framework that relocates all ops... Framework is doing it already but we should do it without static variable.Right, it's done in the UCLASSes. What I meant was it could already be done in the core DM framework.
DM framework could call it and we can get it out of post_probe functions but still actual relocation should be likely done in uclasses because every device type has different ops.
Thanks, Michal
participants (3)
-
Jagan Teki -
Michal Simek -
Simon Goldschmidt