[U-Boot] [PATCH] spi: Fix manual relocation calling more times

[Simon Goldschmidt]

Michal Simek <michal.simek at 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 at 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 at xilinx.com>
> wrote:
> >>>>
> >>>> From: Ashok Reddy Soma <ashok.reddy.soma at 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 at xilinx.com>
> >>>> Signed-off-by: Michal Simek <michal.simek at 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
>