[PATCH RESEND v2 0/3] Tegra GCPDMA: Add dma-channel-mask support

[PATCH RESEND v2 0/3] Tegra GCPDMA: Add dma-channel-mask support

[Akhil R]

Read dma-channel-mask from device tree and register only the
specified channels. This is useful to reserve some channels for the
firmware.

Also update the channel number and interrupts to include all 32
channels. The current driver was using only 31 channels as one
channel was reserved for firmware. Now with this change, the driver
can align more to the actual hardware.

v1->v2:
  * Reversed the operands and used BIT macro in 'if' condition.
  * Fixed warning reported-by: kernel test robot <lkp@intel.com>

Akhil R (3):
  dt-bindings: dmaengine: Add dma-channel-mask to Tegra GPCDMA
  arm64: tegra: Add dma-channel-mask in GPCDMA node
  dmaengine: tegra: Add support for dma-channel-mask

 .../bindings/dma/nvidia,tegra186-gpc-dma.yaml |  7 +++-
 arch/arm64/boot/dts/nvidia/tegra186.dtsi      |  4 +-
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |  4 +-
 arch/arm64/boot/dts/nvidia/tegra234.dtsi      |  4 +-
 drivers/dma/tegra186-gpc-dma.c                | 37 +++++++++++++++----
 5 files changed, 45 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH RESEND v2 1/3] dt-bindings: dmaengine: Add dma-channel-mask to Tegra GPCDMA

[Akhil R]

Add dma-channel-mask property in Tegra GPCDMA document.

The property would help to specify the channels to be used in
kernel and reserve few for the firmware. This was previously
achieved by limiting the channel number to 31 in the driver.
Now since we can list all 32 channels, update the interrupts
property as well to list all 32 interrupts.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/dma/nvidia,tegra186-gpc-dma.yaml   | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra186-gpc-dma.yaml b/Documentation/devicetree/bindings/dma/nvidia,tegra186-gpc-dma.yaml
index c8894476b6ab..851bd50ee67f 100644
--- a/Documentation/devicetree/bindings/dma/nvidia,tegra186-gpc-dma.yaml
+++ b/Documentation/devicetree/bindings/dma/nvidia,tegra186-gpc-dma.yaml
@@ -39,7 +39,7 @@ properties:
       Should contain all of the per-channel DMA interrupts in
       ascending order with respect to the DMA channel index.
     minItems: 1
-    maxItems: 31
+    maxItems: 32
 
   resets:
     maxItems: 1
@@ -52,6 +52,9 @@ properties:
 
   dma-coherent: true
 
+  dma-channel-mask:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -60,6 +63,7 @@ required:
   - reset-names
   - "#dma-cells"
   - iommus
+  - dma-channel-mask
 
 additionalProperties: false
 
@@ -108,5 +112,6 @@ examples:
         #dma-cells = <1>;
         iommus = <&smmu TEGRA186_SID_GPCDMA_0>;
         dma-coherent;
+        dma-channel-mask = <0xfffffffe>;
     };
 ...
-- 
2.17.1

[PATCH RESEND v2 2/3] arm64: tegra: Add dma-channel-mask in GPCDMA node

[Akhil R]

Add dma-channel-mask property in Tegra GPCDMA device tree node.

The property would help to specify the channels to be used in
kernel and reserve few for the firmware. This was previously
achieved by limiting the channel number to 31 in the driver.
Now since we can list all 32 channels, update the interrupts
property as well to list all 32 interrupts.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 4 +++-
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 4 +++-
 arch/arm64/boot/dts/nvidia/tegra234.dtsi | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 6602fe421ee8..db479064ff72 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -78,7 +78,8 @@
 		reg = <0x0 0x2600000 0x0 0x210000>;
 		resets = <&bpmp TEGRA186_RESET_GPCDMA>;
 		reset-names = "gpcdma";
-		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
+		interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>,
@@ -112,6 +113,7 @@
 		#dma-cells = <1>;
 		iommus = <&smmu TEGRA186_SID_GPCDMA_0>;
 		dma-coherent;
+		dma-channel-mask = <0xfffffffe>;
 		status = "okay";
 	};
 
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 41f3a7e188d0..b009f8145016 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -142,7 +142,8 @@
 			reg = <0x2600000 0x210000>;
 			resets = <&bpmp TEGRA194_RESET_GPCDMA>;
 			reset-names = "gpcdma";
-			interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
+			interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>,
@@ -176,6 +177,7 @@
 			#dma-cells = <1>;
 			iommus = <&smmu TEGRA194_SID_GPCDMA_0>;
 			dma-coherent;
+			dma-channel-mask = <0xfffffffe>;
 			status = "okay";
 		};
 
diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index 0170bfa8a467..ccc1a4bd094d 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -27,7 +27,8 @@
 			reg = <0x2600000 0x210000>;
 			resets = <&bpmp TEGRA234_RESET_GPCDMA>;
 			reset-names = "gpcdma";
-			interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
+			interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>,
@@ -60,6 +61,7 @@
 				     <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
 			#dma-cells = <1>;
 			iommus = <&smmu_niso0 TEGRA234_SID_GPCDMA>;
+			dma-channel-mask = <0xfffffffe>;
 			dma-coherent;
 		};
 
-- 
2.17.1

[PATCH RESEND v2 3/3] dmaengine: tegra: Add support for dma-channel-mask

[Akhil R]

Add support for dma-channel-mask so that only the specified channels
are used. This helps to reserve some channels for the firmware.

This was initially achieved by limiting the channel number to 31 in
the driver and adjusting the register address to skip channel0 which
was reserved for a firmware. Now, with this change, the driver can
align more to the actual hardware which has 32 channels.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/dma/tegra186-gpc-dma.c | 37 +++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
index fa9bda4a2bc6..1d1180db6d4e 100644
--- a/drivers/dma/tegra186-gpc-dma.c
+++ b/drivers/dma/tegra186-gpc-dma.c
@@ -161,7 +161,10 @@
 #define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT	5000 /* 5 msec */
 
 /* Channel base address offset from GPCDMA base address */
-#define TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET	0x20000
+#define TEGRA_GPCDMA_CHANNEL_BASE_ADDR_OFFSET	0x10000
+
+/* Default channel mask reserving channel0 */
+#define TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK	0xfffffffe
 
 struct tegra_dma;
 struct tegra_dma_channel;
@@ -246,6 +249,7 @@ struct tegra_dma {
 	const struct tegra_dma_chip_data *chip_data;
 	unsigned long sid_m2d_reserved;
 	unsigned long sid_d2m_reserved;
+	u32 chan_mask;
 	void __iomem *base_addr;
 	struct device *dev;
 	struct dma_device dma_dev;
@@ -1288,7 +1292,7 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
 }
 
 static const struct tegra_dma_chip_data tegra186_dma_chip_data = {
-	.nr_channels = 31,
+	.nr_channels = 32,
 	.channel_reg_size = SZ_64K,
 	.max_dma_count = SZ_1G,
 	.hw_support_pause = false,
@@ -1296,7 +1300,7 @@ static const struct tegra_dma_chip_data tegra186_dma_chip_data = {
 };
 
 static const struct tegra_dma_chip_data tegra194_dma_chip_data = {
-	.nr_channels = 31,
+	.nr_channels = 32,
 	.channel_reg_size = SZ_64K,
 	.max_dma_count = SZ_1G,
 	.hw_support_pause = true,
@@ -1304,7 +1308,7 @@ static const struct tegra_dma_chip_data tegra194_dma_chip_data = {
 };
 
 static const struct tegra_dma_chip_data tegra234_dma_chip_data = {
-	.nr_channels = 31,
+	.nr_channels = 32,
 	.channel_reg_size = SZ_64K,
 	.max_dma_count = SZ_1G,
 	.hw_support_pause = true,
@@ -1380,15 +1384,28 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	}
 	stream_id = iommu_spec->ids[0] & 0xffff;
 
+	ret = device_property_read_u32(&pdev->dev, "dma-channel-mask",
+				       &tdma->chan_mask);
+	if (ret) {
+		dev_warn(&pdev->dev,
+			 "Missing dma-channel-mask property, using default channel mask %#x\n",
+			 TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK);
+		tdma->chan_mask = TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK;
+	}
+
 	INIT_LIST_HEAD(&tdma->dma_dev.channels);
 	for (i = 0; i < cdata->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 
+		/* Check for channel mask */
+		if (!(tdma->chan_mask & BIT(i)))
+			continue;
+
 		tdc->irq = platform_get_irq(pdev, i);
 		if (tdc->irq < 0)
 			return tdc->irq;
 
-		tdc->chan_base_offset = TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET +
+		tdc->chan_base_offset = TEGRA_GPCDMA_CHANNEL_BASE_ADDR_OFFSET +
 					i * cdata->channel_reg_size;
 		snprintf(tdc->name, sizeof(tdc->name), "gpcdma.%d", i);
 		tdc->tdma = tdma;
@@ -1449,8 +1466,8 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	dev_info(&pdev->dev, "GPC DMA driver register %d channels\n",
-		 cdata->nr_channels);
+	dev_info(&pdev->dev, "GPC DMA driver register %lu channels\n",
+		 hweight_long(tdma->chan_mask));
 
 	return 0;
 }
@@ -1473,6 +1490,9 @@ static int __maybe_unused tegra_dma_pm_suspend(struct device *dev)
 	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 
+		if (!(tdma->chan_mask & BIT(i)))
+			continue;
+
 		if (tdc->dma_desc) {
 			dev_err(tdma->dev, "channel %u busy\n", i);
 			return -EBUSY;
@@ -1492,6 +1512,9 @@ static int __maybe_unused tegra_dma_pm_resume(struct device *dev)
 	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 
+		if (!(tdma->chan_mask & BIT(i)))
+			continue;
+
 		tegra_dma_program_sid(tdc, tdc->stream_id);
 	}
 
-- 
2.17.1

Re: [PATCH RESEND v2 3/3] dmaengine: tegra: Add support for dma-channel-mask

[Rob Herring]

On Thu, Oct 20, 2022 at 02:03:22PM +0530, Akhil R wrote:
> Add support for dma-channel-mask so that only the specified channels
> are used. This helps to reserve some channels for the firmware.
> 
> This was initially achieved by limiting the channel number to 31 in
> the driver and adjusting the register address to skip channel0 which
> was reserved for a firmware. Now, with this change, the driver can
> align more to the actual hardware which has 32 channels.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/dma/tegra186-gpc-dma.c | 37 +++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
> index fa9bda4a2bc6..1d1180db6d4e 100644
> --- a/drivers/dma/tegra186-gpc-dma.c
> +++ b/drivers/dma/tegra186-gpc-dma.c
> @@ -161,7 +161,10 @@
>  #define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT	5000 /* 5 msec */
>  
>  /* Channel base address offset from GPCDMA base address */
> -#define TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET	0x20000
> +#define TEGRA_GPCDMA_CHANNEL_BASE_ADDR_OFFSET	0x10000
> +
> +/* Default channel mask reserving channel0 */
> +#define TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK	0xfffffffe
>  
>  struct tegra_dma;
>  struct tegra_dma_channel;
> @@ -246,6 +249,7 @@ struct tegra_dma {
>  	const struct tegra_dma_chip_data *chip_data;
>  	unsigned long sid_m2d_reserved;
>  	unsigned long sid_d2m_reserved;
> +	u32 chan_mask;
>  	void __iomem *base_addr;
>  	struct device *dev;
>  	struct dma_device dma_dev;
> @@ -1288,7 +1292,7 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
>  }
>  
>  static const struct tegra_dma_chip_data tegra186_dma_chip_data = {
> -	.nr_channels = 31,
> +	.nr_channels = 32,

This is an ABI break. A new kernel with an old DTB will use 32 channels 
instead of 31. You should leave this and use the dma-channel-mask to 
enable all 32 channels.

Rob

Re: [PATCH RESEND v2 2/3] arm64: tegra: Add dma-channel-mask in GPCDMA node

[Rob Herring]

On Thu, Oct 20, 2022 at 02:03:21PM +0530, Akhil R wrote:
> Add dma-channel-mask property in Tegra GPCDMA device tree node.
> 
> The property would help to specify the channels to be used in
> kernel and reserve few for the firmware. This was previously
> achieved by limiting the channel number to 31 in the driver.
> Now since we can list all 32 channels, update the interrupts
> property as well to list all 32 interrupts.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra186.dtsi | 4 +++-
>  arch/arm64/boot/dts/nvidia/tegra194.dtsi | 4 +++-
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 4 +++-
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> index 6602fe421ee8..db479064ff72 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
> @@ -78,7 +78,8 @@
>  		reg = <0x0 0x2600000 0x0 0x210000>;
>  		resets = <&bpmp TEGRA186_RESET_GPCDMA>;
>  		reset-names = "gpcdma";
> -		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
> +		interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,

Adding an interrupt onto the beginning? Also an ABI issue.

If you want to break the ABI, you have to explain that you are and why 
it is okay on that platform.

Rob

RE: [PATCH RESEND v2 3/3] dmaengine: tegra: Add support for dma-channel-mask

[Akhil R]

> On Thu, Oct 20, 2022 at 02:03:22PM +0530, Akhil R wrote:
> > Add support for dma-channel-mask so that only the specified channels
> > are used. This helps to reserve some channels for the firmware.
> >
> > This was initially achieved by limiting the channel number to 31 in
> > the driver and adjusting the register address to skip channel0 which
> > was reserved for a firmware. Now, with this change, the driver can
> > align more to the actual hardware which has 32 channels.
> >
> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> > ---
> >  drivers/dma/tegra186-gpc-dma.c | 37 +++++++++++++++++++++++++++-------
> >  1 file changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-
> dma.c
> > index fa9bda4a2bc6..1d1180db6d4e 100644
> > --- a/drivers/dma/tegra186-gpc-dma.c
> > +++ b/drivers/dma/tegra186-gpc-dma.c
> > @@ -161,7 +161,10 @@
> >  #define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT        5000 /* 5
> msec */
> >
> >  /* Channel base address offset from GPCDMA base address */
> > -#define TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET 0x20000
> > +#define TEGRA_GPCDMA_CHANNEL_BASE_ADDR_OFFSET        0x10000
> > +
> > +/* Default channel mask reserving channel0 */
> > +#define TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK    0xfffffffe
> >
> >  struct tegra_dma;
> >  struct tegra_dma_channel;
> > @@ -246,6 +249,7 @@ struct tegra_dma {
> >       const struct tegra_dma_chip_data *chip_data;
> >       unsigned long sid_m2d_reserved;
> >       unsigned long sid_d2m_reserved;
> > +     u32 chan_mask;
> >       void __iomem *base_addr;
> >       struct device *dev;
> >       struct dma_device dma_dev;
> > @@ -1288,7 +1292,7 @@ static struct dma_chan *tegra_dma_of_xlate(struct
> of_phandle_args *dma_spec,
> >  }
> >
> >  static const struct tegra_dma_chip_data tegra186_dma_chip_data = {
> > -     .nr_channels = 31,
> > +     .nr_channels = 32,
> 
> This is an ABI break. A new kernel with an old DTB will use 32 channels
> instead of 31. You should leave this and use the dma-channel-mask to
> enable all 32 channels.
> 
Hi Rob,

If using an old DTB, tdma->chan_mask will be default to 0xfffffffe since it
would not have the dma-channel-mask property. The driver would still 
use 31 channels even if it uses an old DTB. Shouldn't it prevent the
ABI break? 

Regards,
Akhil

Re: [PATCH RESEND v2 3/3] dmaengine: tegra: Add support for dma-channel-mask

[Jon Hunter]

On 26/10/2022 05:44, Akhil R wrote:
>> On Thu, Oct 20, 2022 at 02:03:22PM +0530, Akhil R wrote:
>>> Add support for dma-channel-mask so that only the specified channels
>>> are used. This helps to reserve some channels for the firmware.
>>>
>>> This was initially achieved by limiting the channel number to 31 in
>>> the driver and adjusting the register address to skip channel0 which
>>> was reserved for a firmware. Now, with this change, the driver can
>>> align more to the actual hardware which has 32 channels.
>>>
>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>   drivers/dma/tegra186-gpc-dma.c | 37 +++++++++++++++++++++++++++-------
>>>   1 file changed, 30 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-
>> dma.c
>>> index fa9bda4a2bc6..1d1180db6d4e 100644
>>> --- a/drivers/dma/tegra186-gpc-dma.c
>>> +++ b/drivers/dma/tegra186-gpc-dma.c
>>> @@ -161,7 +161,10 @@
>>>   #define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT        5000 /* 5
>> msec */
>>>
>>>   /* Channel base address offset from GPCDMA base address */
>>> -#define TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET 0x20000
>>> +#define TEGRA_GPCDMA_CHANNEL_BASE_ADDR_OFFSET        0x10000
>>> +
>>> +/* Default channel mask reserving channel0 */
>>> +#define TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK    0xfffffffe
>>>
>>>   struct tegra_dma;
>>>   struct tegra_dma_channel;
>>> @@ -246,6 +249,7 @@ struct tegra_dma {
>>>        const struct tegra_dma_chip_data *chip_data;
>>>        unsigned long sid_m2d_reserved;
>>>        unsigned long sid_d2m_reserved;
>>> +     u32 chan_mask;
>>>        void __iomem *base_addr;
>>>        struct device *dev;
>>>        struct dma_device dma_dev;
>>> @@ -1288,7 +1292,7 @@ static struct dma_chan *tegra_dma_of_xlate(struct
>> of_phandle_args *dma_spec,
>>>   }
>>>
>>>   static const struct tegra_dma_chip_data tegra186_dma_chip_data = {
>>> -     .nr_channels = 31,
>>> +     .nr_channels = 32,
>>
>> This is an ABI break. A new kernel with an old DTB will use 32 channels
>> instead of 31. You should leave this and use the dma-channel-mask to
>> enable all 32 channels.
>>
> Hi Rob,
> 
> If using an old DTB, tdma->chan_mask will be default to 0xfffffffe since it
> would not have the dma-channel-mask property. The driver would still
> use 31 channels even if it uses an old DTB. Shouldn't it prevent the
> ABI break?

Unfortunately no. Yes for an old DTB without the dma-channel-mask 
property, we set the channel mask to 0xfffffffe, but this is not correct 
because it only has 31 interrupts/channels and not 32. So I think we 
will need to use of_irq_count() here.

Jon
-- 
nvpublic

RE: [PATCH RESEND v2 3/3] dmaengine: tegra: Add support for dma-channel-mask

[Akhil R]

> On 26/10/2022 05:44, Akhil R wrote:
> >> On Thu, Oct 20, 2022 at 02:03:22PM +0530, Akhil R wrote:
> >>> Add support for dma-channel-mask so that only the specified channels
> >>> are used. This helps to reserve some channels for the firmware.
> >>>
> >>> This was initially achieved by limiting the channel number to 31 in
> >>> the driver and adjusting the register address to skip channel0 which
> >>> was reserved for a firmware. Now, with this change, the driver can
> >>> align more to the actual hardware which has 32 channels.
> >>>
> >>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> >>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> >>> ---
> >>>   drivers/dma/tegra186-gpc-dma.c | 37 +++++++++++++++++++++++++++----
> ---
> >>>   1 file changed, 30 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-
> >> dma.c
> >>> index fa9bda4a2bc6..1d1180db6d4e 100644
> >>> --- a/drivers/dma/tegra186-gpc-dma.c
> >>> +++ b/drivers/dma/tegra186-gpc-dma.c
> >>> @@ -161,7 +161,10 @@
> >>>   #define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT        5000 /* 5
> >> msec */
> >>>
> >>>   /* Channel base address offset from GPCDMA base address */
> >>> -#define TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET 0x20000
> >>> +#define TEGRA_GPCDMA_CHANNEL_BASE_ADDR_OFFSET        0x10000
> >>> +
> >>> +/* Default channel mask reserving channel0 */
> >>> +#define TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK    0xfffffffe
> >>>
> >>>   struct tegra_dma;
> >>>   struct tegra_dma_channel;
> >>> @@ -246,6 +249,7 @@ struct tegra_dma {
> >>>        const struct tegra_dma_chip_data *chip_data;
> >>>        unsigned long sid_m2d_reserved;
> >>>        unsigned long sid_d2m_reserved;
> >>> +     u32 chan_mask;
> >>>        void __iomem *base_addr;
> >>>        struct device *dev;
> >>>        struct dma_device dma_dev;
> >>> @@ -1288,7 +1292,7 @@ static struct dma_chan
> *tegra_dma_of_xlate(struct
> >> of_phandle_args *dma_spec,
> >>>   }
> >>>
> >>>   static const struct tegra_dma_chip_data tegra186_dma_chip_data = {
> >>> -     .nr_channels = 31,
> >>> +     .nr_channels = 32,
> >>
> >> This is an ABI break. A new kernel with an old DTB will use 32 channels
> >> instead of 31. You should leave this and use the dma-channel-mask to
> >> enable all 32 channels.
> >>
> > Hi Rob,
> >
> > If using an old DTB, tdma->chan_mask will be default to 0xfffffffe since it
> > would not have the dma-channel-mask property. The driver would still
> > use 31 channels even if it uses an old DTB. Shouldn't it prevent the
> > ABI break?
> 
> Unfortunately no. Yes for an old DTB without the dma-channel-mask
> property, we set the channel mask to 0xfffffffe, but this is not correct
> because it only has 31 interrupts/channels and not 32. So I think we
> will need to use of_irq_count() here.
>

Shall I put it in a way that only the used interrupts are mentioned in the DT?
With this I can revert the interrupt change in the DT and would not break
the ABI as well.

The code would look something like this.

int chan_count = 0;

if (of_irq_count(pdev->dev.of_node) != hweight_long(tdma->chan_mask)) {
        dev_err(&pdev->dev, "Interrupt count doesn't match with channels\n");
        return -EINVAL;
}

for (i = 0; i < cdata->nr_channels; i++) {
        struct tegra_dma_channel *tdc = &tdma->channels[i];
    
        /* Check for channel mask */
        if (!(tdma->chan_mask & BIT(i)))
            continue;

        tdc->irq = platform_get_irq(pdev, chan_count);
        chan_count++;
        if (tdc->irq < 0)
            return tdc->irq;
    ...
    ...
}

Regards,
Akhil

Re: [PATCH RESEND v2 1/3] dt-bindings: dmaengine: Add dma-channel-mask to Tegra GPCDMA

[Krzysztof Kozlowski]

On 20/10/2022 04:33, Akhil R wrote:
> Add dma-channel-mask property in Tegra GPCDMA document.
> 
> The property would help to specify the channels to be used in
> kernel and reserve few for the firmware. This was previously
> achieved by limiting the channel number to 31 in the driver.
> Now since we can list all 32 channels, update the interrupts
> property as well to list all 32 interrupts.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH RESEND v2 3/3] dmaengine: tegra: Add support for dma-channel-mask

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5620 bytes --]

On Thu, Oct 27, 2022 at 10:13:00AM +0000, Akhil R wrote:
> > On 26/10/2022 05:44, Akhil R wrote:
> > >> On Thu, Oct 20, 2022 at 02:03:22PM +0530, Akhil R wrote:
> > >>> Add support for dma-channel-mask so that only the specified channels
> > >>> are used. This helps to reserve some channels for the firmware.
> > >>>
> > >>> This was initially achieved by limiting the channel number to 31 in
> > >>> the driver and adjusting the register address to skip channel0 which
> > >>> was reserved for a firmware. Now, with this change, the driver can
> > >>> align more to the actual hardware which has 32 channels.
> > >>>
> > >>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > >>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> > >>> ---
> > >>>   drivers/dma/tegra186-gpc-dma.c | 37 +++++++++++++++++++++++++++----
> > ---
> > >>>   1 file changed, 30 insertions(+), 7 deletions(-)
> > >>>
> > >>> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-
> > >> dma.c
> > >>> index fa9bda4a2bc6..1d1180db6d4e 100644
> > >>> --- a/drivers/dma/tegra186-gpc-dma.c
> > >>> +++ b/drivers/dma/tegra186-gpc-dma.c
> > >>> @@ -161,7 +161,10 @@
> > >>>   #define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT        5000 /* 5
> > >> msec */
> > >>>
> > >>>   /* Channel base address offset from GPCDMA base address */
> > >>> -#define TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET 0x20000
> > >>> +#define TEGRA_GPCDMA_CHANNEL_BASE_ADDR_OFFSET        0x10000
> > >>> +
> > >>> +/* Default channel mask reserving channel0 */
> > >>> +#define TEGRA_GPCDMA_DEFAULT_CHANNEL_MASK    0xfffffffe
> > >>>
> > >>>   struct tegra_dma;
> > >>>   struct tegra_dma_channel;
> > >>> @@ -246,6 +249,7 @@ struct tegra_dma {
> > >>>        const struct tegra_dma_chip_data *chip_data;
> > >>>        unsigned long sid_m2d_reserved;
> > >>>        unsigned long sid_d2m_reserved;
> > >>> +     u32 chan_mask;
> > >>>        void __iomem *base_addr;
> > >>>        struct device *dev;
> > >>>        struct dma_device dma_dev;
> > >>> @@ -1288,7 +1292,7 @@ static struct dma_chan
> > *tegra_dma_of_xlate(struct
> > >> of_phandle_args *dma_spec,
> > >>>   }
> > >>>
> > >>>   static const struct tegra_dma_chip_data tegra186_dma_chip_data = {
> > >>> -     .nr_channels = 31,
> > >>> +     .nr_channels = 32,
> > >>
> > >> This is an ABI break. A new kernel with an old DTB will use 32 channels
> > >> instead of 31. You should leave this and use the dma-channel-mask to
> > >> enable all 32 channels.
> > >>
> > > Hi Rob,
> > >
> > > If using an old DTB, tdma->chan_mask will be default to 0xfffffffe since it
> > > would not have the dma-channel-mask property. The driver would still
> > > use 31 channels even if it uses an old DTB. Shouldn't it prevent the
> > > ABI break?
> > 
> > Unfortunately no. Yes for an old DTB without the dma-channel-mask
> > property, we set the channel mask to 0xfffffffe, but this is not correct
> > because it only has 31 interrupts/channels and not 32. So I think we
> > will need to use of_irq_count() here.
> >
> 
> Shall I put it in a way that only the used interrupts are mentioned in the DT?
> With this I can revert the interrupt change in the DT and would not break
> the ABI as well.
> 
> The code would look something like this.
> 
> int chan_count = 0;
> 
> if (of_irq_count(pdev->dev.of_node) != hweight_long(tdma->chan_mask)) {
>         dev_err(&pdev->dev, "Interrupt count doesn't match with channels\n");
>         return -EINVAL;
> }
> 
> for (i = 0; i < cdata->nr_channels; i++) {
>         struct tegra_dma_channel *tdc = &tdma->channels[i];
>     
>         /* Check for channel mask */
>         if (!(tdma->chan_mask & BIT(i)))
>             continue;
> 
>         tdc->irq = platform_get_irq(pdev, chan_count);
>         chan_count++;
>         if (tdc->irq < 0)
>             return tdc->irq;
>     ...
>     ...
> }

This is all getting quite complicated for what is essentially a bug fix.
The root of this problem is that the original bindings were simply wrong
and didn't accurately represent the hardware.

The GPC DMA controller has 32 channels and each channel has one DMA
interrupt. So the right way to describe this in DT is by listing all of
the interrupts. If the firmware then ends up using one of those channels
the right way is not to make it seem like the controller supports only
31 channels, but rather to mark the used channels as reserved.

So I think the bottom line here is that the original binding has a bug
that we're trying to address. Any workaround for this is problematic,
and I think breaking backwards-compatibility is the cleanest solution
here.

Now, GPC DMA was introduced on Tegra186 and the DT bindings were added
in 5.19. Any products released with this IP were released with kernels
prior to 5.19 and bindings that were never in-tree. Any of those
products that are supported upstream we know have replaceable DTB images
(i.e. by default they are flashed at the same time as the kernel image),
so breaking DT ABI should be okay.

In fact, there are slight incompatibilities between product and upstream
DT bindings (we're in the process of removing those as part of the
upstreaming process), so being able to reflash the DTB is a prerequisite
for supporting upstream Linux on these devices.

I think going forward we should be extra careful about these kinds of
problems, so that we don't have to keep dealing with fallout like this.

Rob, given the above, do you think it's okay to proceed with the ABI
break?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH RESEND v2 0/3] Tegra GCPDMA: Add dma-channel-mask support

[Akhil R]

Read dma-channel-mask from device tree and register only the
specified channels. This is useful to reserve some channels for the
firmware.

Also update the channel number and interrupts to include all 32
channels. The current driver was using only 31 channels as one
channel was reserved for firmware. Now with this change, the driver
can align more to the actual hardware.

v1->v2:
  * Reversed the operands and used BIT macro in 'if' condition.
  * Fixed warning reported-by: kernel test robot <lkp@intel.com>

Akhil R (3):
  dt-bindings: dmaengine: Add dma-channel-mask to Tegra GPCDMA
  arm64: tegra: Add dma-channel-mask in GPCDMA node
  dmaengine: tegra: Add support for dma-channel-mask

 .../bindings/dma/nvidia,tegra186-gpc-dma.yaml |  7 +++-
 arch/arm64/boot/dts/nvidia/tegra186.dtsi      |  4 +-
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |  4 +-
 arch/arm64/boot/dts/nvidia/tegra234.dtsi      |  4 +-
 drivers/dma/tegra186-gpc-dma.c                | 37 +++++++++++++++----
 5 files changed, 45 insertions(+), 11 deletions(-)

-- 
2.17.1