| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e4a6feaf-a9cd-4092-a083-c356a7d954b2@collabora.com>
Date: Mon, 22 Sep 2025 12:42:32 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: wctrl@...ton.me, Sean Wang <sean.wang@...iatek.com>,
Vinod Koul <vkoul@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
Long Cheng <long.cheng@...iatek.com>
Cc: dmaengine@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] dmaengine: mediatek: mtk-uart-apdma: support more
than 33 bits for DMA bitmask
Il 21/09/25 13:03, Max Shevchenko via B4 Relay ha scritto:
> From: Max Shevchenko <wctrl@...ton.me>
>
> Drop mediatek,dma-33bits property and introduce a platform data with
> field representing DMA bitmask.
>
> The reference SoCs were taken from the downstream kernel (6.6) for
> the MT6991 SoC.
>
That's a good idea - but it doesn't work like that.
The VFF_4G_SUPPORT register really is called {RX,TX}_VFF_ADDR2 - and on all of
the newer SoCs that support more than 33 bits, this register holds the upper
X bits of the TX/RX addr, where X is (dma_bits - 32) meaning that, for example,
for MT6985 X=(36-32) -> X=4.
The downstream driver does have a reference implementation for this - and there
is no simpler way around it: you either implement it all, or you don't.
Simply put: with your code, you're not supporting more than 33 bits, because even
though you're setting the dma mask, you're never correctly using the hardware (as
in, you're never programming the additional registers to make use of that).
> Signed-off-by: Max Shevchenko <wctrl@...ton.me>
> ---
> drivers/dma/mediatek/mtk-uart-apdma.c | 47 +++++++++++++++++++++++++----------
> 1 file changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
> index 08e15177427b94246951d38a2a1d76875c1e452e..68dd3a4ee0d88fd508870a5de24ae67505023495 100644
> --- a/drivers/dma/mediatek/mtk-uart-apdma.c
> +++ b/drivers/dma/mediatek/mtk-uart-apdma.c
> @@ -42,6 +42,7 @@
> #define VFF_EN_CLR_B 0
> #define VFF_INT_EN_CLR_B 0
> #define VFF_4G_SUPPORT_CLR_B 0
> +#define VFF_ORI_ADDR_BITS_NUM 32
>
> /*
> * interrupt trigger level for tx
> @@ -74,10 +75,14 @@
> #define VFF_DEBUG_STATUS 0x50
> #define VFF_4G_SUPPORT 0x54
>
> +struct mtk_uart_apdma_data {
> + unsigned int dma_bits;
> +};
> +
> struct mtk_uart_apdmadev {
> struct dma_device ddev;
> struct clk *clk;
> - bool support_33bits;
> + unsigned int support_bits;
You don't really need to carry support_bits... there's no real usage of that
information across the code, if not at probe time.
bool support_extended_addr; /* rename to your liking */
> unsigned int dma_requests;
> };
>
> @@ -148,7 +153,7 @@ static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> mtk_uart_apdma_write(c, VFF_WPT, 0);
> mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
>
> - if (mtkd->support_33bits)
> + if (mtkd->support_bits > VFF_ORI_ADDR_BITS_NUM)
if (mtkd->support_extended_addr)
mtk_uart_apdma_write(c, VFF_4G_SUPPORT, upper_32_bits(d->addr);
... do the same for RX and you should be 99.9% done :-)
> mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_EN_B);
> }
>
> @@ -191,7 +196,7 @@ static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> mtk_uart_apdma_write(c, VFF_RPT, 0);
> mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_CLR_B);
>
> - if (mtkd->support_33bits)
> + if (mtkd->support_bits > VFF_ORI_ADDR_BITS_NUM)
> mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_EN_B);
> }
>
> @@ -297,7 +302,7 @@ static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> goto err_pm;
> }
>
> - if (mtkd->support_33bits)
> + if (mtkd->support_bits > VFF_ORI_ADDR_BITS_NUM)
> mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
>
> err_pm:
> @@ -467,8 +472,27 @@ static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> }
> }
>
> +static const struct mtk_uart_apdma_data mt6577_data = {
> + .dma_bits = 32
> +};
> +
> +static const struct mtk_uart_apdma_data mt6795_data = {
> + .dma_bits = 33
> +};
> +
> +static const struct mtk_uart_apdma_data mt6779_data = {
> + .dma_bits = 34
> +};
> +
> +static const struct mtk_uart_apdma_data mt6985_data = {
> + .dma_bits = 35
> +};
> +
> static const struct of_device_id mtk_uart_apdma_match[] = {
> - { .compatible = "mediatek,mt6577-uart-dma", },
> + { .compatible = "mediatek,mt6577-uart-dma", .data = &mt6577_data },
What about doing, instead...
{ .compatible = "mediatek,mt6577-uart-dma", .data = (void *)32 },
> + { .compatible = "mediatek,mt6795-uart-dma", .data = &mt6795_data },
> + { .compatible = "mediatek,mt6779-uart-dma", .data = &mt6779_data },
> + { .compatible = "mediatek,mt6985-uart-dma", .data = &mt6985_data },
> { /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> @@ -477,7 +501,8 @@ static int mtk_uart_apdma_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct mtk_uart_apdmadev *mtkd;
> - int bit_mask = 32, rc;
> + const struct mtk_uart_apdma_data *data;
> + int rc;
> struct mtk_chan *c;
> unsigned int i;
>
> @@ -492,13 +517,9 @@ static int mtk_uart_apdma_probe(struct platform_device *pdev)
> return rc;
> }
>
> - if (of_property_read_bool(np, "mediatek,dma-33bits"))
> - mtkd->support_33bits = true;
> -
> - if (mtkd->support_33bits)
> - bit_mask = 33;
> -
> - rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(bit_mask));
> + data = of_device_get_match_data(&pdev->dev);
> + mtkd->support_bits = data->dma_bits;
...and there you just get that single number you need, store it locally, then you
can do
mtkd->support_extended_addr = apdma_num_bits > 32;
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(data->dma_bits));
> if (rc)
> return rc;
>
Cheers,
Angelo
Powered by blists - more mailing lists