Re: [PATCH] mmc: sdhci-xenon: Fix 2G limitation on AC5 SoC

[Vadym Kochan <vadym.kochan@plvision.eu>]

Hi Robin,

On Wed, Aug 17, 2022 at 06:23:02PM +0100, Robin Murphy wrote:
> On 2022-08-17 17:07, Vadym Kochan wrote:
> > Hi Robin, Adrian,
> > 
> > On Wed, Aug 17, 2022 at 02:43:46PM +0100, Robin Murphy wrote:
> > > On 2022-08-16 21:51, Vadym Kochan wrote:
> > > [...]
> > > > > The one thing to watch out for is that SWIOTLB doesn't necessarily interact
> > > > > very well with DMA offsets. Given the intent of
> > > > > of_dma_get_max_cpu_address(), I think it ought to work out OK now for
> > > > > current kernels on DT systems if everything is described correctly, but
> > > > > otherwise it's likely that you end up with ZONE_DMA either being empty or
> > > > > containing all memory, so the SWIOTLB buffer ends up being allocated
> > > > > anywhere such that it might not actually work as expected.
> > > > > 
> > > > > Robin.
> > > > 
> > > > Hi Robin,
> > > > 
> > > > Thank you for the reply.
> > > > 
> > > > My understanding is that swiotlb is allocated (in case of arm64)
> > > > in the following cases:
> > > > 
> > > >      #1 when it is forced from the kernel cmdline
> > > > 
> > > >      #2 when max_pfn is greater than arm64_dma_phys_limit (and this is used
> > > >         as the end from which to allocate the swiotlb pool in the
> > > >         top-botom direction via memblock API).
> > > > 
> > > >      #3 using restricted dma-pool
> > > > 
> > > > Of course option #3 works fine because swiotlb is kind of forced to use
> > > > particulary this range of memory.
> > > > 
> > > > Both options #1 & #2 causes to use full memory mask even if to specify
> > > > dma-ranges in the DT:
> > > > 
> > > >       dma-ranges = <0x0 0x0 0x2 0x0 0x0 0x80000000>;
> > > > 
> > > > or if to specify the opposite:
> > > > 
> > > >       dma-ranges = <0x2 0x0 0x0 0x0 0x0 0x80000000>;
> > > > 
> > > >       just to make it lower than U32 to pass
> > > > 
> > > >           zone_dma_bits = min3(32U, dt_zone_dma_bits, acpi_zone_dma_bits)
> > > > 
> > > >       condition, but then it will be re-set in max_zone_phys() by:
> > > > 
> > > > 	if (phys_start > U32_MAX)
> > > > 		zone_mask = PHYS_ADDR_MAX;
> > > > 	else if (phys_start > zone_mask)
> > > > 		zone_mask = U32_MAX;
> > > 
> > > Ah, indeed I missed that, sorry. It seems that that change to stop assuming
> > > an offset kind of crossed over with the introduction of
> > > *_dma_get_max_cpu_address(), but now that that firmware property parsing
> > > *is* implemented, in principle it should be equally possible to evaluate the
> > > actual offsets as well, and decide whether an offset ZONE_DMA is appropriate
> > > or not. Either way, this is definitely the area which needs work if we want
> > > to to able to support topologies like this properly.
> > > 
> > > > So, currently I dont see how to pin swiotlb (I see it as a main problem) to some specific range of physical
> > > > memory (particulary to the first 2G of RAM).
> > > 
> > > Indeed, if ZONE_DMA and/or ZONE_DMA32 can't be set appropriately, then
> > > there's no way to guarantee correct allocation of any DMA buffers, short of
> > > hacking it with explicitly placed reserved-memory carveouts.
> > > 
> > 
> > I have sent some time ago a solution which binds restricted-dma pool to
> > the eMMC device, so Adrian, Robin do you think this can be acceptable as
> > a temporary solution (at least conceptually) ?
> > 
> > I was also thinking would it be OK to introduce something like
> > bounced-dma pool (similar to the restricted one) which will reserve
> > memory for the bounced buffers only ? It should not be hard as looks
> > like it will re-use existing interface between dma and swiotlb ? In that
> > case it would allow to map first 2G of memory to eMMC controller.
> 
> TBH I'd prefer to fix it (or at least work around it) more generally.
> Putting made-up things in devicetree to work around shortcomings in
> kernel code tends to be a hole that's hard to dig yourself back out of.
> As a bodge that would be just about justifiable in its own terms, does
> the diff below help at all?
> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index b9af30be813e..88f7b26f49db 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -451,7 +451,14 @@ void __init bootmem_init(void)
>   */
>  void __init mem_init(void)
>  {
> +	/*
> +	 * Some platforms still manage to elude our attempt to calculate
> +	 * ZONE_DMA appropriately, so encourage the SWIOTLB allocation to go
> +	 * as low as it can anyway for the best chance of being usable.
> +	 */
> +	memblock_set_bottom_up(true);
>  	swiotlb_init(max_pfn > PFN_DOWN(arm64_dma_phys_limit), SWIOTLB_VERBOSE);
> +	memblock_set_bottom_up(false);
>  	/* this will put all unused low memory onto the freelists */
>  	memblock_free_all();

It works with the following changes:

    #1 dma-ranges = <0x0 0x0 0x2 0x0 0x0 0x80000000>;

    #3 swiotlb="force"

Is it OK to force the memory allocation from the start for the swiotlb ?
Or may be do it by new CONFIG which will be enforced by the new
CONFIG for the device on that particular SoC ?

Thanks,
Vadym