Re: [PATCH v5 2/2] usb: host: xhci-plat: add support for multi memory regions

[Robin Murphy <robin.murphy@arm.com>]

On 2024-03-11 9:49 am, Howard Yen wrote:
> The reason why it needs multiple regions is that in my system there is
> an always-on subsystem which includes a small size memory, and several
> functions need to run and occupy the memory from the small memory if
> they need to run on the always-on subsystem. These functions must
> allocate the memory from the small memory region, so that they can get
> benefit from the always-on subsystem. So the small memory is split for
> multiple functions which are satisfied with their generic use cases.
> But in specific use cases, like USB3 devices which support the stream
> trasnsfer or multiple devices connect to the host, they required more
> memory than their pre-allocated memory region, so I tried to propose
> this patch to give it the ability to get the memory from the other
> larger memory to solve the issue.

Once again this still fails to make sense - The USB controller has a 
special always-on pool from which it "must allocate", yet it's fine if 
it also allocates from elsewhere? How on Earth is that supposed to work?

As I said before, if it's actually the case that only certain specific 
allocations (based on driver-level knowledge) must come from the special 
pool, then this is not something which can realistically be abstracted 
by the generic dma-coherent API as it stands (and if so, do the 
non-special allocations even need a dedicated second reserved region, or 
is that in fact just a massive hack around the dma_coherent_mem design 
intentionally not falling back to the regular allocator?)

Thanks,
Robin.

> Signed-off-by: Howard Yen <howardyen@google.com>
> ---
>   drivers/usb/host/xhci-plat.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 3d071b875308..7892d3eb26d2 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -14,6 +14,7 @@
>   #include <linux/pci.h>
>   #include <linux/of.h>
>   #include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
>   #include <linux/platform_device.h>
>   #include <linux/usb/phy.h>
>   #include <linux/slab.h>
> @@ -149,7 +150,7 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
>   	struct xhci_hcd		*xhci;
>   	struct resource         *res;
>   	struct usb_hcd		*hcd, *usb3_hcd;
> -	int			ret;
> +	int			i, count, ret;
>   	int			irq;
>   	struct xhci_plat_priv	*priv = NULL;
>   	bool			of_match;
> @@ -194,6 +195,19 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
>   
>   	xhci->allow_single_roothub = 1;
>   
> +	count = of_property_count_u32_elems(sysdev->of_node, "memory-region");
> +
> +	for (i = 0; i < count; i++) {
> +		ret = of_reserved_mem_device_init_by_idx(sysdev, sysdev->of_node, i);
> +		if (ret) {
> +			dev_err(sysdev, "Could not get reserved memory\n");
> +			if (i > 0)
> +				of_reserved_mem_device_release(sysdev);
> +
> +			return ret;
> +		}
> +	}
> +
>   	/*
>   	 * Not all platforms have clks so it is not an error if the
>   	 * clock do not exist.
> @@ -431,6 +445,9 @@ void xhci_plat_remove(struct platform_device *dev)
>   	clk_disable_unprepare(clk);
>   	clk_disable_unprepare(reg_clk);
>   	reset_control_assert(xhci->reset);
> +
> +	of_reserved_mem_device_release(hcd->self.sysdev);
> +
>   	usb_put_hcd(hcd);
>   
>   	pm_runtime_disable(&dev->dev);