On Tue, 29 Oct 2024 at 14:02, Janne Grunau <[email protected]> wrote: > > Hej, > > On Tue, Oct 29, 2024 at 12:48:46PM +0530, Sughosh Ganu wrote: > > The LMB module is typically used for managing the platform's RAM > > memory. RAM memory gets added to the module's memory map, and > > subsequently allocated through various LMB API's. > > > > The IOMMU driver for the apple platforms also uses the LMB API's for > > allocating device virtual addresses(VA). These addresses are different > > from the RAM addresses, and cannot be added to the RAM memory map. Add > > a separate instance of LMB memory map for the device VA's, which gets > > managed by the IOMMU driver. Use lmb_push() and lmb_pop() functions to > > set-up the relevant lmb instance. > > thanks, this fixes the initial brokenness when setting up dma mappings > but I still see SError due to translation fault. I don't have time right > now to look into it so it could be unrelated to the iommu.
Good to know. Thanks for testing the changes. > > > Signed-off-by: Sughosh Ganu <[email protected]> > > --- > > drivers/iommu/apple_dart.c | 67 +++++++++++++++++++++++++++++++++++++- > > include/lmb.h | 2 ++ > > lib/lmb.c | 1 - > > 3 files changed, 68 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/apple_dart.c b/drivers/iommu/apple_dart.c > > index 611ac7cd6de..55f60287b0e 100644 > > --- a/drivers/iommu/apple_dart.c > > +++ b/drivers/iommu/apple_dart.c > > @@ -3,6 +3,7 @@ > > * Copyright (C) 2021 Mark Kettenis <[email protected]> > > */ > > > > +#include <alist.h> > > #include <cpu_func.h> > > #include <dm.h> > > #include <iommu.h> > > @@ -70,6 +71,8 @@ > > > > struct apple_dart_priv { > > void *base; > > + struct lmb apple_dart_lmb; > > + struct lmb ram_lmb; > > u64 *l1, *l2; > > int bypass, shift; > > > > @@ -87,6 +90,56 @@ struct apple_dart_priv { > > void (*flush_tlb)(struct apple_dart_priv *priv); > > }; > > > > +static int apple_dart_lmb_init(struct apple_dart_priv *priv) > > +{ > > + bool ret; > > + struct lmb *almb = &priv->apple_dart_lmb; > > + > > + ret = alist_init(&almb->free_mem, sizeof(struct lmb_region), > > + (uint)LMB_ALIST_INITIAL_SIZE); > > + if (!ret) { > > + log_debug("Unable to initialise the list for LMB free > > memory\n"); > > + return -ENOMEM; > > + } > > + > > + ret = alist_init(&almb->used_mem, sizeof(struct lmb_region), > > + (uint)LMB_ALIST_INITIAL_SIZE); > > + if (!ret) { > > + log_debug("Unable to initialise the list for LMB used > > memory\n"); > > + return -ENOMEM; > > + } > > + > > + return 0; > > +} > > + > > +static void apple_dart_lmb_uninit(struct apple_dart_priv *priv) > > +{ > > + struct lmb *almb = &priv->apple_dart_lmb; > > + > > + alist_uninit(&almb->free_mem); > > + alist_uninit(&almb->used_mem); > > +} > > + > > +static void apple_dart_lmb_setup(struct apple_dart_priv *priv) > > +{ > > + struct lmb *almb = &priv->apple_dart_lmb; > > + struct lmb *rlmb = &priv->ram_lmb; > > + > > + lmb_push(rlmb); > > + > > + lmb_pop(almb); > > This doesn't look look like a good solution to me. We're conflating two > different concepts into the global lmb. This looks error prone to me. I > was looking into adding iomb_* entry points into the lmb points which > take a pointer. Yes, even I was trying to avoid having another instance of LMB as much as possible, but given that this is breaking the platform, I thought this would be the quickest to fix the break. Personally, I would prefer LMB to have a consistent window of RAM addresses for all platforms, i.e. ram_base to ram_top. Also, apart from this peculiar scenario, I am not sure if there would be other uses of having to allocate anything other than RAM addresses by LMB. -sughosh > > Janne
