| Message ID | 20240724060224.3071065-31-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make LMB memory map global and persistent | expand |
Hi Sughosh, On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@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@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. > #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. > > -- > 2.34.1 > Regards, Simon
On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@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@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. > > > #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. -sughosh > > > > > -- > > 2.34.1 > > > > Regards, > Simon
Hi Sughosh, On Mon, 29 Jul 2024 at 02:52, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@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@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. > > > > > > #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
On Mon, 29 Jul 2024 at 20:58, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 29 Jul 2024 at 02:52, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@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@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? -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
Hi Sughosh, On Fri, 2 Aug 2024 at 01:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 29 Jul 2024 at 20:58, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Mon, 29 Jul 2024 at 02:52, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Fri, 26 Jul 2024 at 05:02, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 24 Jul 2024 at 00:05, Sughosh Ganu <sughosh.ganu@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@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
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 #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; }
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@linaro.org> --- Changes since rfc: None drivers/iommu/sandbox_iommu.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)