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

[Michal Simek]

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.

Thanks,
Michal