* [PATCH v2 0/3] Add GPCDMA support to Tegra I2C
@ 2022-09-06 14:47 Akhil R
2022-09-06 14:47 ` [PATCH v2 1/3] i2c: tegra: Add GPCDMA support Akhil R
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Akhil R @ 2022-09-06 14:47 UTC (permalink / raw)
To: christian.koenig, devicetree, digetx, jonathanh, ldewangan,
linux-i2c, linux-kernel, linux-tegra, robh+dt, sumit.semwal,
thierry.reding, wsa
Cc: akhilrajeev
Updates in Tegra I2C driver, device tree and defconfig to support
GPCDMA for I2C in Tegra 186 and later chips.
Akhil R (3):
i2c: tegra: Add GPCDMA support
arm64: tegra: Add GPCDMA support for Tegra I2C
arm64: defconfig: Make TEGRA186_GPC_DMA built-in
arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++
arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++
arch/arm64/configs/defconfig | 2 +-
drivers/i2c/busses/i2c-tegra.c | 11 +++++---
5 files changed, 105 insertions(+), 4 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 1/3] i2c: tegra: Add GPCDMA support 2022-09-06 14:47 [PATCH v2 0/3] Add GPCDMA support to Tegra I2C Akhil R @ 2022-09-06 14:47 ` Akhil R 2022-09-10 9:03 ` Dmitry Osipenko 2022-09-16 19:47 ` Wolfram Sang 2022-09-06 14:47 ` [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C Akhil R 2022-09-06 14:47 ` [PATCH v2 3/3] arm64: defconfig: Make TEGRA186_GPC_DMA built-in Akhil R 2 siblings, 2 replies; 11+ messages in thread From: Akhil R @ 2022-09-06 14:47 UTC (permalink / raw) To: christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, robh+dt, sumit.semwal, thierry.reding, wsa Cc: akhilrajeev Enable support for GPCDMA, which is used in I2C controllers in Tegra 186 and above. The chips before that used APB DMA. This change works under the presumption that all chips apart from those supporting APB DMA is using GPCDMA. Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- drivers/i2c/busses/i2c-tegra.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 031c78ac42e6..954022c04cc4 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -443,11 +443,16 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) u32 *dma_buf; int err; - if (!i2c_dev->hw->has_apb_dma || i2c_dev->is_vi) + if (i2c_dev->is_vi) return 0; - if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) { - dev_dbg(i2c_dev->dev, "DMA support not enabled\n"); + if (!i2c_dev->hw->has_apb_dma) { + if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) { + dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n"); + return 0; + } + } else if (!IS_ENABLED(CONFIG_TEGRA186_GPC_DMA)) { + dev_dbg(i2c_dev->dev, "GPC DMA support not enabled\n"); return 0; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] i2c: tegra: Add GPCDMA support 2022-09-06 14:47 ` [PATCH v2 1/3] i2c: tegra: Add GPCDMA support Akhil R @ 2022-09-10 9:03 ` Dmitry Osipenko 2022-09-16 19:47 ` Wolfram Sang 1 sibling, 0 replies; 11+ messages in thread From: Dmitry Osipenko @ 2022-09-10 9:03 UTC (permalink / raw) To: Akhil R, christian.koenig, devicetree, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, robh+dt, sumit.semwal, thierry.reding, wsa 06.09.2022 17:47, Akhil R пишет: > Enable support for GPCDMA, which is used in I2C controllers > in Tegra 186 and above. The chips before that used APB DMA. > This change works under the presumption that all chips apart from > those supporting APB DMA is using GPCDMA. > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 031c78ac42e6..954022c04cc4 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -443,11 +443,16 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) > u32 *dma_buf; > int err; > > - if (!i2c_dev->hw->has_apb_dma || i2c_dev->is_vi) > + if (i2c_dev->is_vi) > return 0; > > - if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) { > - dev_dbg(i2c_dev->dev, "DMA support not enabled\n"); > + if (!i2c_dev->hw->has_apb_dma) { > + if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) { > + dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n"); > + return 0; > + } > + } else if (!IS_ENABLED(CONFIG_TEGRA186_GPC_DMA)) { > + dev_dbg(i2c_dev->dev, "GPC DMA support not enabled\n"); > return 0; > } > To me the symmetrical if-else will look a bit better, but that's a too minor nit. if () { if () } else { if () } Reviewed-by: Dmitry Osipenko <digetx@gmail.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] i2c: tegra: Add GPCDMA support 2022-09-06 14:47 ` [PATCH v2 1/3] i2c: tegra: Add GPCDMA support Akhil R 2022-09-10 9:03 ` Dmitry Osipenko @ 2022-09-16 19:47 ` Wolfram Sang 1 sibling, 0 replies; 11+ messages in thread From: Wolfram Sang @ 2022-09-16 19:47 UTC (permalink / raw) To: Akhil R Cc: christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, robh+dt, sumit.semwal, thierry.reding [-- Attachment #1: Type: text/plain, Size: 387 bytes --] On Tue, Sep 06, 2022 at 08:17:14PM +0530, Akhil R wrote: > Enable support for GPCDMA, which is used in I2C controllers > in Tegra 186 and above. The chips before that used APB DMA. > This change works under the presumption that all chips apart from > those supporting APB DMA is using GPCDMA. > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> Applied to for-next, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C 2022-09-06 14:47 [PATCH v2 0/3] Add GPCDMA support to Tegra I2C Akhil R 2022-09-06 14:47 ` [PATCH v2 1/3] i2c: tegra: Add GPCDMA support Akhil R @ 2022-09-06 14:47 ` Akhil R 2022-10-07 13:20 ` Thierry Reding 2022-09-06 14:47 ` [PATCH v2 3/3] arm64: defconfig: Make TEGRA186_GPC_DMA built-in Akhil R 2 siblings, 1 reply; 11+ messages in thread From: Akhil R @ 2022-09-06 14:47 UTC (permalink / raw) To: christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, robh+dt, sumit.semwal, thierry.reding, wsa Cc: akhilrajeev Add dma properties to support GPCDMA for I2C in Tegra 186 and later chips Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++ arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++ arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++ 3 files changed, 96 insertions(+) diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi index 59a10fb184f8..3580fbf99091 100644 --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi @@ -672,6 +672,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA186_RESET_I2C1>; reset-names = "i2c"; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 21>, <&gpcdma 21>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -685,6 +689,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA186_RESET_I2C3>; reset-names = "i2c"; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 23>, <&gpcdma 23>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -702,6 +710,10 @@ pinctrl-names = "default", "idle"; pinctrl-0 = <&state_dpaux1_i2c>; pinctrl-1 = <&state_dpaux1_off>; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 26>, <&gpcdma 26>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -733,6 +745,10 @@ pinctrl-names = "default", "idle"; pinctrl-0 = <&state_dpaux_i2c>; pinctrl-1 = <&state_dpaux_off>; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 30>, <&gpcdma 30>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -746,6 +762,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA186_RESET_I2C7>; reset-names = "i2c"; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 27>, <&gpcdma 27>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -759,6 +779,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA186_RESET_I2C9>; reset-names = "i2c"; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 31>, <&gpcdma 31>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -1176,6 +1200,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA186_RESET_I2C2>; reset-names = "i2c"; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 22>, <&gpcdma 22>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -1189,6 +1217,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA186_RESET_I2C8>; reset-names = "i2c"; + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 0>, <&gpcdma 0>; + dma-names = "rx", "tx"; status = "disabled"; }; diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi index d0ed55e5c860..9176c4b27133 100644 --- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi +++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi @@ -805,6 +805,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA194_RESET_I2C1>; reset-names = "i2c"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 21>, <&gpcdma 21>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -830,6 +834,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA194_RESET_I2C3>; reset-names = "i2c"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 23>, <&gpcdma 23>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -847,6 +855,10 @@ pinctrl-0 = <&state_dpaux1_i2c>; pinctrl-1 = <&state_dpaux1_off>; pinctrl-names = "default", "idle"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 26>, <&gpcdma 26>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -864,6 +876,10 @@ pinctrl-0 = <&state_dpaux0_i2c>; pinctrl-1 = <&state_dpaux0_off>; pinctrl-names = "default", "idle"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 30>, <&gpcdma 30>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -881,6 +897,10 @@ pinctrl-0 = <&state_dpaux2_i2c>; pinctrl-1 = <&state_dpaux2_off>; pinctrl-names = "default", "idle"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 27>, <&gpcdma 27>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -898,6 +918,10 @@ pinctrl-0 = <&state_dpaux3_i2c>; pinctrl-1 = <&state_dpaux3_off>; pinctrl-names = "default", "idle"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 31>, <&gpcdma 31>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -1565,6 +1589,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA194_RESET_I2C2>; reset-names = "i2c"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 22>, <&gpcdma 22>; + dma-names = "rx", "tx"; status = "disabled"; }; @@ -1578,6 +1606,10 @@ clock-names = "div-clk"; resets = <&bpmp TEGRA194_RESET_I2C8>; reset-names = "i2c"; + iommus = <&smmu TEGRA194_SID_GPCDMA_0>; + dma-coherent; + dmas = <&gpcdma 0>, <&gpcdma 0>; + dma-names = "rx", "tx"; status = "disabled"; }; diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi index 81a0f599685f..5852e765ad90 100644 --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi @@ -737,6 +737,10 @@ clock-names = "div-clk", "parent"; resets = <&bpmp TEGRA234_RESET_I2C1>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 21>, <&gpcdma 21>; + dma-names = "rx", "tx"; }; cam_i2c: i2c@3180000 { @@ -752,6 +756,10 @@ clock-names = "div-clk", "parent"; resets = <&bpmp TEGRA234_RESET_I2C3>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 23>, <&gpcdma 23>; + dma-names = "rx", "tx"; }; dp_aux_ch1_i2c: i2c@3190000 { @@ -767,6 +775,10 @@ clock-names = "div-clk", "parent"; resets = <&bpmp TEGRA234_RESET_I2C4>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 26>, <&gpcdma 26>; + dma-names = "rx", "tx"; }; dp_aux_ch0_i2c: i2c@31b0000 { @@ -782,6 +794,10 @@ clock-names = "div-clk", "parent"; resets = <&bpmp TEGRA234_RESET_I2C6>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 30>, <&gpcdma 30>; + dma-names = "rx", "tx"; }; dp_aux_ch2_i2c: i2c@31c0000 { @@ -797,6 +813,10 @@ clock-names = "div-clk", "parent"; resets = <&bpmp TEGRA234_RESET_I2C7>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 27>, <&gpcdma 27>; + dma-names = "rx", "tx"; }; dp_aux_ch3_i2c: i2c@31e0000 { @@ -812,6 +832,10 @@ clock-names = "div-clk", "parent"; resets = <&bpmp TEGRA234_RESET_I2C9>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 31>, <&gpcdma 31>; + dma-names = "rx", "tx"; }; spi@3270000 { @@ -1109,6 +1133,10 @@ assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLP_OUT0>; resets = <&bpmp TEGRA234_RESET_I2C2>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 22>, <&gpcdma 22>; + dma-names = "rx", "tx"; }; gen8_i2c: i2c@c250000 { @@ -1125,6 +1153,10 @@ assigned-clock-parents = <&bpmp TEGRA234_CLK_PLLP_OUT0>; resets = <&bpmp TEGRA234_RESET_I2C8>; reset-names = "i2c"; + iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>; + dma-coherent; + dmas = <&gpcdma 0>, <&gpcdma 0>; + dma-names = "rx", "tx"; }; rtc@c2a0000 { -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C 2022-09-06 14:47 ` [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C Akhil R @ 2022-10-07 13:20 ` Thierry Reding 2022-10-07 14:34 ` Thierry Reding 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2022-10-07 13:20 UTC (permalink / raw) To: Akhil R Cc: christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, robh+dt, sumit.semwal, wsa [-- Attachment #1: Type: text/plain, Size: 1365 bytes --] On Tue, Sep 06, 2022 at 08:17:15PM +0530, Akhil R wrote: > Add dma properties to support GPCDMA for I2C in Tegra 186 and later > chips > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++ > arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++ > arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++ > 3 files changed, 96 insertions(+) > > diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi > index 59a10fb184f8..3580fbf99091 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi > +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi > @@ -672,6 +672,10 @@ > clock-names = "div-clk"; > resets = <&bpmp TEGRA186_RESET_I2C1>; > reset-names = "i2c"; > + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; > + dma-coherent; I wonder: why do we need the iommus and dma-coherent properties here? The I2C controllers are not directly accessing memory, instead it's the GPCDMA via the dmas/dma-names properties. The GPCDMA already has these properties set, so they seem to be useless here. I'm asking because they cause a lot of warnings from the DT validators, so we either need to remove them (if they are not necessary) or add the DT binding documentation for them. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C 2022-10-07 13:20 ` Thierry Reding @ 2022-10-07 14:34 ` Thierry Reding 2022-10-07 14:53 ` Robin Murphy 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2022-10-07 14:34 UTC (permalink / raw) To: Rob Herring, Robin Murphy, Will Deacon, Lars-Peter Clausen, Vinod Koul Cc: Akhil R, christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa [-- Attachment #1: Type: text/plain, Size: 3164 bytes --] On Fri, Oct 07, 2022 at 03:20:37PM +0200, Thierry Reding wrote: > On Tue, Sep 06, 2022 at 08:17:15PM +0530, Akhil R wrote: > > Add dma properties to support GPCDMA for I2C in Tegra 186 and later > > chips > > > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > --- > > arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++ > > arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++ > > arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++ > > 3 files changed, 96 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi > > index 59a10fb184f8..3580fbf99091 100644 > > --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi > > +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi > > @@ -672,6 +672,10 @@ > > clock-names = "div-clk"; > > resets = <&bpmp TEGRA186_RESET_I2C1>; > > reset-names = "i2c"; > > + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; > > + dma-coherent; > > I wonder: why do we need the iommus and dma-coherent properties here? > The I2C controllers are not directly accessing memory, instead it's the > GPCDMA via the dmas/dma-names properties. The GPCDMA already has these > properties set, so they seem to be useless here. Looking at this some more, the reason why we need these is so that the struct device backing these I2C controllers is attached to an IOMMU and the DMA ops are set up correspondingly. Without these, the DMA memory allocated by the I2C controllers will not be mapped through the IOMMU and cause faults because the GPCDMA is the one that needs to access those. I do recall that we have a similar case for audio where the "sound card" needs to have an iommus property to make sure it allocates memory through the same IOMMU domain as the ADMA, which is the device that ends up doing the actual memory accesses. Rob, Robin, Will, do you know of a good way other than the DT workaround here to address this? I think ideally we would want to obtain the "DMA parent" of these devices so that we allocate memory for that parent instead of the child. We do have some existing infrastructure for this type of relationship with the __of_get_dma_parent() function as well as the interconnects property, but I wonder if that's really the right way to represent this. Adding "interconnects" properties would also duplicate the "dmas" properties we already use to obtain the TX and RX DMA channels. One simple way to more accurately do this would be to reach into the DMA engine internals (dma_chan->device->dev) and pass that to dma_alloc_*() to make sure we allocate for the correct device. For audio that could be a bit complicated because most of that code is shared across multiple vendors. I couldn't find any examples where a driver would reach into DMA channels to allocate for the parent, so I'm wondering what other people do to solve this issue. Or if anyone else even has the same issue. Adding Lars-Peter for the sound/dmaengine helpers and Vinod for general dmaengine. Perhaps they have some thoughts on or experience with this. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C 2022-10-07 14:34 ` Thierry Reding @ 2022-10-07 14:53 ` Robin Murphy 2022-10-07 15:17 ` Thierry Reding 0 siblings, 1 reply; 11+ messages in thread From: Robin Murphy @ 2022-10-07 14:53 UTC (permalink / raw) To: Thierry Reding, Rob Herring, Will Deacon, Lars-Peter Clausen, Vinod Koul Cc: Akhil R, christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa On 2022-10-07 15:34, Thierry Reding wrote: > On Fri, Oct 07, 2022 at 03:20:37PM +0200, Thierry Reding wrote: >> On Tue, Sep 06, 2022 at 08:17:15PM +0530, Akhil R wrote: >>> Add dma properties to support GPCDMA for I2C in Tegra 186 and later >>> chips >>> >>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com> >>> --- >>> arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++ >>> arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++ >>> arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++ >>> 3 files changed, 96 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi >>> index 59a10fb184f8..3580fbf99091 100644 >>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi >>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi >>> @@ -672,6 +672,10 @@ >>> clock-names = "div-clk"; >>> resets = <&bpmp TEGRA186_RESET_I2C1>; >>> reset-names = "i2c"; >>> + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; >>> + dma-coherent; >> >> I wonder: why do we need the iommus and dma-coherent properties here? >> The I2C controllers are not directly accessing memory, instead it's the >> GPCDMA via the dmas/dma-names properties. The GPCDMA already has these >> properties set, so they seem to be useless here. > > Looking at this some more, the reason why we need these is so that the > struct device backing these I2C controllers is attached to an IOMMU and > the DMA ops are set up correspondingly. Without these, the DMA memory > allocated by the I2C controllers will not be mapped through the IOMMU > and cause faults because the GPCDMA is the one that needs to access > those. > > I do recall that we have a similar case for audio where the "sound card" > needs to have an iommus property to make sure it allocates memory > through the same IOMMU domain as the ADMA, which is the device that ends > up doing the actual memory accesses. > > Rob, Robin, Will, do you know of a good way other than the DT workaround > here to address this? I think ideally we would want to obtain the "DMA > parent" of these devices so that we allocate memory for that parent > instead of the child. We do have some existing infrastructure for this > type of relationship with the __of_get_dma_parent() function as well as > the interconnects property, but I wonder if that's really the right way > to represent this. > > Adding "interconnects" properties would also duplicate the "dmas" > properties we already use to obtain the TX and RX DMA channels. One > simple way to more accurately do this would be to reach into the DMA > engine internals (dma_chan->device->dev) and pass that to dma_alloc_*() > to make sure we allocate for the correct device. For audio that could be > a bit complicated because most of that code is shared across multiple > vendors. I couldn't find any examples where a driver would reach into > DMA channels to allocate for the parent, so I'm wondering what other > people do to solve this issue. Or if anyone else even has the same > issue. As far as I'm aware that's the correct approach, i.e. if a driver is using an external dmaengine then it's responsible for making DMA mappings for the correct DMA channel device. We ended up being a bit asymmetrical in that the dmaengine driver itself has to take care of its own mapping for the non-memory end of a transfer when an IOMMU is involved - that's what dma_map_resource() was created for, see pl330 and rcar-dmac for examples. The only driver I have first-hand experience with in this context is amba-pl011, using pl330 through an SMMU on the Arm Juno board, but that definitely works fine without DT hacks. Robin. > Adding Lars-Peter for the sound/dmaengine helpers and Vinod for general > dmaengine. Perhaps they have some thoughts on or experience with this. > > Thierry ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C 2022-10-07 14:53 ` Robin Murphy @ 2022-10-07 15:17 ` Thierry Reding 2022-10-07 17:40 ` Robin Murphy 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2022-10-07 15:17 UTC (permalink / raw) To: Robin Murphy Cc: Rob Herring, Will Deacon, Lars-Peter Clausen, Vinod Koul, Akhil R, christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa [-- Attachment #1: Type: text/plain, Size: 4500 bytes --] On Fri, Oct 07, 2022 at 03:53:21PM +0100, Robin Murphy wrote: > On 2022-10-07 15:34, Thierry Reding wrote: > > On Fri, Oct 07, 2022 at 03:20:37PM +0200, Thierry Reding wrote: > > > On Tue, Sep 06, 2022 at 08:17:15PM +0530, Akhil R wrote: > > > > Add dma properties to support GPCDMA for I2C in Tegra 186 and later > > > > chips > > > > > > > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > > > --- > > > > arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++ > > > > arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++ > > > > arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++ > > > > 3 files changed, 96 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi > > > > index 59a10fb184f8..3580fbf99091 100644 > > > > --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi > > > > +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi > > > > @@ -672,6 +672,10 @@ > > > > clock-names = "div-clk"; > > > > resets = <&bpmp TEGRA186_RESET_I2C1>; > > > > reset-names = "i2c"; > > > > + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; > > > > + dma-coherent; > > > > > > I wonder: why do we need the iommus and dma-coherent properties here? > > > The I2C controllers are not directly accessing memory, instead it's the > > > GPCDMA via the dmas/dma-names properties. The GPCDMA already has these > > > properties set, so they seem to be useless here. > > > > Looking at this some more, the reason why we need these is so that the > > struct device backing these I2C controllers is attached to an IOMMU and > > the DMA ops are set up correspondingly. Without these, the DMA memory > > allocated by the I2C controllers will not be mapped through the IOMMU > > and cause faults because the GPCDMA is the one that needs to access > > those. > > > > I do recall that we have a similar case for audio where the "sound card" > > needs to have an iommus property to make sure it allocates memory > > through the same IOMMU domain as the ADMA, which is the device that ends > > up doing the actual memory accesses. > > > > Rob, Robin, Will, do you know of a good way other than the DT workaround > > here to address this? I think ideally we would want to obtain the "DMA > > parent" of these devices so that we allocate memory for that parent > > instead of the child. We do have some existing infrastructure for this > > type of relationship with the __of_get_dma_parent() function as well as > > the interconnects property, but I wonder if that's really the right way > > to represent this. > > > > Adding "interconnects" properties would also duplicate the "dmas" > > properties we already use to obtain the TX and RX DMA channels. One > > simple way to more accurately do this would be to reach into the DMA > > engine internals (dma_chan->device->dev) and pass that to dma_alloc_*() > > to make sure we allocate for the correct device. For audio that could be > > a bit complicated because most of that code is shared across multiple > > vendors. I couldn't find any examples where a driver would reach into > > DMA channels to allocate for the parent, so I'm wondering what other > > people do to solve this issue. Or if anyone else even has the same > > issue. > > As far as I'm aware that's the correct approach, i.e. if a driver is using > an external dmaengine then it's responsible for making DMA mappings for the > correct DMA channel device. We ended up being a bit asymmetrical in that the > dmaengine driver itself has to take care of its own mapping for the > non-memory end of a transfer when an IOMMU is involved - that's what > dma_map_resource() was created for, see pl330 and rcar-dmac for examples. > > The only driver I have first-hand experience with in this context is > amba-pl011, using pl330 through an SMMU on the Arm Juno board, but that > definitely works fine without DT hacks. Oh yeah, I see. That's exactly what I had in mind. And now that I'm using the right regex I can also find more occurrences of this. Thanks for those pointers. Looks like that's the right approach then. I'll try to make corresponding changes and see if there's any fallout. Thanks, Thierry > > Robin. > > > Adding Lars-Peter for the sound/dmaengine helpers and Vinod for general > > dmaengine. Perhaps they have some thoughts on or experience with this. > > > > Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C 2022-10-07 15:17 ` Thierry Reding @ 2022-10-07 17:40 ` Robin Murphy 0 siblings, 0 replies; 11+ messages in thread From: Robin Murphy @ 2022-10-07 17:40 UTC (permalink / raw) To: Thierry Reding Cc: Rob Herring, Will Deacon, Lars-Peter Clausen, Vinod Koul, Akhil R, christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, sumit.semwal, wsa On 2022-10-07 16:17, Thierry Reding wrote: > On Fri, Oct 07, 2022 at 03:53:21PM +0100, Robin Murphy wrote: >> On 2022-10-07 15:34, Thierry Reding wrote: >>> On Fri, Oct 07, 2022 at 03:20:37PM +0200, Thierry Reding wrote: >>>> On Tue, Sep 06, 2022 at 08:17:15PM +0530, Akhil R wrote: >>>>> Add dma properties to support GPCDMA for I2C in Tegra 186 and later >>>>> chips >>>>> >>>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com> >>>>> --- >>>>> arch/arm64/boot/dts/nvidia/tegra186.dtsi | 32 ++++++++++++++++++++++++ >>>>> arch/arm64/boot/dts/nvidia/tegra194.dtsi | 32 ++++++++++++++++++++++++ >>>>> arch/arm64/boot/dts/nvidia/tegra234.dtsi | 32 ++++++++++++++++++++++++ >>>>> 3 files changed, 96 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi >>>>> index 59a10fb184f8..3580fbf99091 100644 >>>>> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi >>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi >>>>> @@ -672,6 +672,10 @@ >>>>> clock-names = "div-clk"; >>>>> resets = <&bpmp TEGRA186_RESET_I2C1>; >>>>> reset-names = "i2c"; >>>>> + iommus = <&smmu TEGRA186_SID_GPCDMA_0>; >>>>> + dma-coherent; >>>> >>>> I wonder: why do we need the iommus and dma-coherent properties here? >>>> The I2C controllers are not directly accessing memory, instead it's the >>>> GPCDMA via the dmas/dma-names properties. The GPCDMA already has these >>>> properties set, so they seem to be useless here. >>> >>> Looking at this some more, the reason why we need these is so that the >>> struct device backing these I2C controllers is attached to an IOMMU and >>> the DMA ops are set up correspondingly. Without these, the DMA memory >>> allocated by the I2C controllers will not be mapped through the IOMMU >>> and cause faults because the GPCDMA is the one that needs to access >>> those. >>> >>> I do recall that we have a similar case for audio where the "sound card" >>> needs to have an iommus property to make sure it allocates memory >>> through the same IOMMU domain as the ADMA, which is the device that ends >>> up doing the actual memory accesses. >>> >>> Rob, Robin, Will, do you know of a good way other than the DT workaround >>> here to address this? I think ideally we would want to obtain the "DMA >>> parent" of these devices so that we allocate memory for that parent >>> instead of the child. We do have some existing infrastructure for this >>> type of relationship with the __of_get_dma_parent() function as well as >>> the interconnects property, but I wonder if that's really the right way >>> to represent this. >>> >>> Adding "interconnects" properties would also duplicate the "dmas" >>> properties we already use to obtain the TX and RX DMA channels. One >>> simple way to more accurately do this would be to reach into the DMA >>> engine internals (dma_chan->device->dev) and pass that to dma_alloc_*() >>> to make sure we allocate for the correct device. For audio that could be >>> a bit complicated because most of that code is shared across multiple >>> vendors. I couldn't find any examples where a driver would reach into >>> DMA channels to allocate for the parent, so I'm wondering what other >>> people do to solve this issue. Or if anyone else even has the same >>> issue. >> >> As far as I'm aware that's the correct approach, i.e. if a driver is using >> an external dmaengine then it's responsible for making DMA mappings for the >> correct DMA channel device. We ended up being a bit asymmetrical in that the >> dmaengine driver itself has to take care of its own mapping for the >> non-memory end of a transfer when an IOMMU is involved - that's what >> dma_map_resource() was created for, see pl330 and rcar-dmac for examples. >> >> The only driver I have first-hand experience with in this context is >> amba-pl011, using pl330 through an SMMU on the Arm Juno board, but that >> definitely works fine without DT hacks. > > Oh yeah, I see. That's exactly what I had in mind. And now that I'm > using the right regex I can also find more occurrences of this. Thanks > for those pointers. Looks like that's the right approach then. I'll try > to make corresponding changes and see if there's any fallout. Eww, from a quick look the DMA API usage in i2c-tegra is worse than just this. Calling dma_sync_* on a buffer from dma_alloc_coherent() is both egregiously wrong and semantically pointless - the fundamental purpose of a coherent allocation is that both CPU and device can access it at any time without any explicit synchronisation. If the driver's doing its own bounce-buffering for reasons of data alignment then the syncs should just go; otherwise get rid of the coherent buffer and replace the syncs with proper map/unmap. Either way, be sure to throw CONFIG_DMA_API_DEBUG at it, hard. Cheers, Robin. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] arm64: defconfig: Make TEGRA186_GPC_DMA built-in 2022-09-06 14:47 [PATCH v2 0/3] Add GPCDMA support to Tegra I2C Akhil R 2022-09-06 14:47 ` [PATCH v2 1/3] i2c: tegra: Add GPCDMA support Akhil R 2022-09-06 14:47 ` [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C Akhil R @ 2022-09-06 14:47 ` Akhil R 2 siblings, 0 replies; 11+ messages in thread From: Akhil R @ 2022-09-06 14:47 UTC (permalink / raw) To: christian.koenig, devicetree, digetx, jonathanh, ldewangan, linux-i2c, linux-kernel, linux-tegra, robh+dt, sumit.semwal, thierry.reding, wsa Cc: akhilrajeev Make TEGRA186_GPC_DMA driver as built-in since the clients using the DMA (like I2C_TEGRA etc) are built-in. This would avoid the potential long delay probe deferral can cause. Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- arch/arm64/configs/defconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index d5b2d2dd4904..9e8c532126aa 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -995,7 +995,7 @@ CONFIG_MV_XOR=y CONFIG_MV_XOR_V2=y CONFIG_OWL_DMA=y CONFIG_PL330_DMA=y -CONFIG_TEGRA186_GPC_DMA=m +CONFIG_TEGRA186_GPC_DMA=y CONFIG_TEGRA20_APB_DMA=y CONFIG_TEGRA210_ADMA=m CONFIG_QCOM_BAM_DMA=y -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-10-07 17:40 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-06 14:47 [PATCH v2 0/3] Add GPCDMA support to Tegra I2C Akhil R 2022-09-06 14:47 ` [PATCH v2 1/3] i2c: tegra: Add GPCDMA support Akhil R 2022-09-10 9:03 ` Dmitry Osipenko 2022-09-16 19:47 ` Wolfram Sang 2022-09-06 14:47 ` [PATCH v2 2/3] arm64: tegra: Add GPCDMA support for Tegra I2C Akhil R 2022-10-07 13:20 ` Thierry Reding 2022-10-07 14:34 ` Thierry Reding 2022-10-07 14:53 ` Robin Murphy 2022-10-07 15:17 ` Thierry Reding 2022-10-07 17:40 ` Robin Murphy 2022-09-06 14:47 ` [PATCH v2 3/3] arm64: defconfig: Make TEGRA186_GPC_DMA built-in Akhil R
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).