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

[Michal Simek]

On 20. 09. 19 10:26, Simon Goldschmidt wrote:
> 
> 
> Michal Simek <michal.simek at xilinx.com <mailto:michal.simek at xilinx.com>>
> schrieb am Fr., 20. Sep. 2019, 10:17:
> 
>     On 19. 09. 19 16:35, Simon Goldschmidt wrote:
>     >
>     >
>     > Michal Simek <michal.simek at xilinx.com
>     <mailto:michal.simek at xilinx.com> <mailto:michal.simek at xilinx.com
>     <mailto: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 <mailto:michal.simek at xilinx.com>
>     <mailto:michal.simek at xilinx.com <mailto: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 <mailto:michal.simek at xilinx.com>
>     <mailto:michal.simek at xilinx.com <mailto:michal.simek at xilinx.com>>>
>     wrote:
>     >     >>>>
>     >     >>>> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com
>     <mailto:ashok.reddy.soma at xilinx.com>
>     >     <mailto:ashok.reddy.soma at xilinx.com
>     <mailto: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 <mailto:ashok.reddy.soma at xilinx.com>
>     >     <mailto:ashok.reddy.soma at xilinx.com
>     <mailto:ashok.reddy.soma at xilinx.com>>>
>     >     >>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com
>     <mailto:michal.simek at xilinx.com>
>     >     <mailto:michal.simek at xilinx.com <mailto: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...
> 
>     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