[PATCH 30/40] sandbox: iommu: remove lmb allocation in the driver

[Simon Glass]

Hi Sughosh,

On Fri, 2 Aug 2024 at 01:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Mon, 29 Jul 2024 at 20:58, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 29 Jul 2024 at 02:52, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > The sandbox iommu driver uses the LMB module to allocate a particular
> > > > > range of memory for the device virtual address(DVA). This used to work
> > > > > earlier since the LMB memory map was caller specific and not
> > > > > global. But with the change to make the LMB allocations global and
> > > > > persistent, adding this memory range has other side effects. On the
> > > > > other hand, the sandbox iommu test expects to see this particular
> > > > > value of the DVA. Use the DVA address directly, instead of mapping it
> > > > > in the LMB memory map, and then have it allocated.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > ---
> > > > > Changes since rfc: None
> > > > >
> > > > >  drivers/iommu/sandbox_iommu.c | 9 ++-------
> > > > >  1 file changed, 2 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/sandbox_iommu.c b/drivers/iommu/sandbox_iommu.c
> > > > > index 505f2c3250..15ecaacbb3 100644
> > > > > --- a/drivers/iommu/sandbox_iommu.c
> > > > > +++ b/drivers/iommu/sandbox_iommu.c
> > > > > @@ -5,10 +5,10 @@
> > > > >
> > > > >  #include <dm.h>
> > > > >  #include <iommu.h>
> > > > > -#include <lmb.h>
> > > > >  #include <asm/io.h>
> > > > >  #include <linux/sizes.h>
> > > > >
> > > > > +#define DVA_ADDR               0x89abc000
> > > >
> > > > This is a very strange address. You can put this in
> > > > arch/sandbox/include/asm/test.h and check it in the test. I suggest
> > > > using something like 0x12345678 so it is obviously meaningless.
> > >
> > > You will see that I am using the same address that is being used
> > > currently. I did not want to make any changes to what is working, nor
> > > did I want to spend time debugging if this logic can be improved --
> > > that is not the purpose of this series.
> >
> > Please just move it. You can keep the same address, but put the
> > constant in the header file.
>
> Looking into this, there isn't an appropriate header file where I can
> put this constant. And creating one only for a macro seems like an
> overkill. Can you propose a header file where I can put this. If there
> isn't an existing one, maybe I can keep the macro as is?

arch/sandbox/include/asm/test.h

It is used for quite a few constants.


>
> -sughosh
>
> >
> > >
> > > >
> > > > >  #define IOMMU_PAGE_SIZE                SZ_4K
> > > > >
> > > > >  static dma_addr_t sandbox_iommu_map(struct udevice *dev, void *addr,
> > > > > @@ -20,8 +20,7 @@ static dma_addr_t sandbox_iommu_map(struct udevice *dev, void *addr,
> > > > >         paddr = ALIGN_DOWN(virt_to_phys(addr), IOMMU_PAGE_SIZE);
> > > > >         off = virt_to_phys(addr) - paddr;
> > > > >         psize = ALIGN(size + off, IOMMU_PAGE_SIZE);
> > > > > -
> > > > > -       dva = lmb_alloc(psize, IOMMU_PAGE_SIZE, LMB_NONE);
> > > > > +       dva = (phys_addr_t)DVA_ADDR;
> > > > >
> > > > >         return dva + off;
> > > > >  }
> > > > > @@ -35,8 +34,6 @@ static void sandbox_iommu_unmap(struct udevice *dev, dma_addr_t addr,
> > > > >         dva = ALIGN_DOWN(addr, IOMMU_PAGE_SIZE);
> > > > >         psize = size + (addr - dva);
> > > > >         psize = ALIGN(psize, IOMMU_PAGE_SIZE);
> > > > > -
> > > > > -       lmb_free(dva, psize, LMB_NONE);
> > > > >  }
> > > > >
> > > > >  static struct iommu_ops sandbox_iommu_ops = {
> > > > > @@ -46,8 +43,6 @@ static struct iommu_ops sandbox_iommu_ops = {
> > > > >
> > > > >  static int sandbox_iommu_probe(struct udevice *dev)
> > > > >  {
> > > > > -       lmb_add(0x89abc000, SZ_16K, LMB_NONE);
> > > > > -
> > > > >         return 0;
> > > > >  }
> > > >
> > > > If this function is empty, you should remove it.
> > >
> > > Okay.
Regards,
Simon