Determine the number of DMA channels by 'dma-channels' property
Subsystems (6)
v4 cover Determine the number of DMA channels by 'dma-channels' property
The PDMA driver currently assumes there are four channels by default, it might cause the error if there is actually less than four channels. Change that by getting number of channel dynamically from device tree. For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
This patch set contains the dts and dt-bindings change.
Changed in v4:
- Remove cflags of debug use reported-by: kernel test robot <[email protected]>
Changed in v3:
- Fix allocating wrong size
- Return error if 'dma-channels' is larger than maximum
Changed in v2:
- Rebase on tag v5.16
- Use 4 as default value of dma-channels
Zong Li (3):
riscv: dts: Add dma-channels property in dma node
dt-bindings: Add dma-channels for pdma device node
dmaengine: sf-pdma: Get number of channel by device tree
.../bindings/dma/sifive,fu540-c000-pdma.yaml | 7 +++++++ .../boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + drivers/dma/sf-pdma/sf-pdma.c | 20 ++++++++++++------- drivers/dma/sf-pdma/sf-pdma.h | 8 ++------ 5 files changed, 24 insertions(+), 13 deletions(-)
Signature · 1 line
2.31.1
PATCH v4 1/3 riscv: dts: Add dma-channels property in dma node
Add dma-channels property, then we can determine how many channels there by device tree.
--- arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + 2 files changed, 2 insertions(+)
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +1 -0
@@ -188,6 +188,7 @@ dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; };
arch/riscv/boot/dts/sifive/fu540-c000.dtsi +1 -0
@@ -171,6 +171,7 @@ dma: dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic0>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; }; uart1: serial@10011000 {
Signature · 1 line
2.31.1
[email protected]
maintainerRe: [PATCH v4 1/3] riscv: dts: Add dma-channels property in dma node
Quoted 16 lines · Zong Li
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Add dma-channels property, then we can determine how many channels there by device tree.
Signed-off-by Zong Li <[email protected]>--- arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + 2 files changed, 2 insertions(+)arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +0
-0
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi dma@3000000 { · line 193
Quoted 6 lines
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +1
-0@@ -188,6 +188,7 @@ dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; };
For mpfs: Acked-by: Conor Dooley <[email protected]>
Quoted 16 lines
arch/riscv/boot/dts/sifive/fu540-c000.dtsi +1
-1@@ -171,6 +171,7 @@ dma: dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic0>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; }; uart1: serial@10011000 { --2.31.1
Re: [PATCH v4 1/3] riscv: dts: Add dma-channels property in dma node
In reply to Zong LiQuoted 26 lines · On Sun, 16 Jan 2022 17:35:26 PST (-0800), [email protected] wrote:
Add dma-channels property, then we can determine how many channels there by device tree.
Signed-off-by Zong Li <[email protected]>--- arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + 2 files changed, 2 insertions(+)arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +1
-0@@ -188,6 +188,7 @@ dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; };arch/riscv/boot/dts/sifive/fu540-c000.dtsi +0
-0
arch/riscv/boot/dts/sifive/fu540-c000.dtsi dma: dma@3000000 { · line 177
Quoted 7 lines
arch/riscv/boot/dts/sifive/fu540-c000.dtsi +1
-0@@ -171,6 +171,7 @@ dma: dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic0>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; }; uart1: serial@10011000 {
I've got some comments in on the bindings, but I'm assuming it's easier to keep these all together. Happy to take this through the RISC-V tree, but I'm guessing it'll be easier to take these via dmaengine as there's a lot more diff there so
Thanks!
Vinod Koul
maintainerRe: [PATCH v4 1/3] riscv: dts: Add dma-channels property in dma node
In reply to Zong Liarch/riscv/boot/dts/microchip/microchip-mpfs.dtsi dma@3000000 {
Quoted 14 lines · Zong Li
Add dma-channels property, then we can determine how many channels there by device tree.
Signed-off-by Zong Li <[email protected]>--- arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + 2 files changed, 2 insertions(+)arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +0
-0@@ -188,6 +188,7 @@ dma@3000000 {
Unrelated but the node name should be dma-controller@...
Quoted 21 lines
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +1
-0reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; };arch/riscv/boot/dts/sifive/fu540-c000.dtsi +1
-0@@ -171,6 +171,7 @@ dma: dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic0>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; }; uart1: serial@10011000 {Signature · 1 line
2.31.1
Signature · 1 line
~Vinod
Re: [PATCH v4 1/3] riscv: dts: Add dma-channels property in dma node
Quoted 11 lines · Vinod Koul
Quoted 9 lines · Zong Li
Add dma-channels property, then we can determine how many channels there by device tree.
Signed-off-by Zong Li <[email protected]>--- arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + 2 files changed, 2 insertions(+)
Quoted 8 lines
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi dma@3000000 {
Quoted 5 lines
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +0
-0@@ -188,6 +188,7 @@ dma@3000000 {Unrelated but the node name should be dma-controller@...
Thanks for the correction, I guess we could give another patch for changing the node name.
Quoted 24 lines
Quoted 20 lines
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +1
-0reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; };arch/riscv/boot/dts/sifive/fu540-c000.dtsi +1
-1@@ -171,6 +171,7 @@ dma: dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic0>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; }; uart1: serial@10011000 { --Quoted 1 line
2.31.1
--
~Vinod
PATCH v4 2/3 dt-bindings: Add dma-channels for pdma device node
In reply to Zong LiAdd dma-channels property, then we can determine how many channels there by device tree, rather than statically defines it in PDMA driver
--- .../devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml | 7 +++++++ 1 file changed, 7 insertions(+)
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +7 -0
@@ -34,6 +34,12 @@ properties: minItems: 1 maxItems: 8 + dma-channels: + description: For backwards-compatible, the default value is 4 + minimum: 1 + maximum: 4 + default: 4 + '#dma-cells': const: 1
@@ -50,6 +56,7 @@ examples: dma@3000000 { compatible = "sifive,fu540-c000-pdma"; reg = <0x3000000 0x8000>; + dma-channels = <4>; interrupts = <23 24 25 26 27 28 29 30>; #dma-cells = <1>; };
Signature · 1 line
2.31.1
Palmer Dabbelt
maintainerRe: [PATCH v4 2/3] dt-bindings: Add dma-channels for pdma device node
Quoted 2 lines · On Sun, 16 Jan 2022 17:35:27 PST (-0800), [email protected] wrote:
Add dma-channels property, then we can determine how many channels there by device tree, rather than statically defines it in PDMA driver
Maybe "statically defining it" is better here?
Quoted 11 lines
Signed-off-by Zong Li <[email protected]>--- .../devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml | 7 +++++++ 1 file changed, 7 insertions(+)Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +0
-0
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml properties: · line 38
Quoted 5 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +2
-0@@ -34,6 +34,12 @@ properties: minItems: 1 maxItems: 8 + dma-channels: + description: For backwards-compatible, the default value is 4
Maybe "backwards-compatibility" is better here?
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml examples: · line 57
Quoted 10 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +4
-0+ minimum: 1 + maximum: 4 + default: 4 + '#dma-cells': const: 1@@ -50,6 +56,7 @@ examples: dma@3000000 { compatible = "sifive,fu540-c000-pdma";
IMO we should have a "sifive,pdma-1.0.0" (or whatever the versioning scheme ended up being) here, in addition to the SOC-specific DT entry. It's kind of odd to start extending the SOC-specific DT entry, as the whole idea there is to let us have an out in case we find future compatibility issues.
Quoted 5 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +1
-0reg = <0x3000000 0x8000>; + dma-channels = <4>; interrupts = <23 24 25 26 27 28 29 30>; #dma-cells = <1>; };
Re: [PATCH v4 2/3] dt-bindings: Add dma-channels for pdma device node
Quoted 7 lines · Palmer Dabbelt
Quoted 2 lines · On Sun, 16 Jan 2022 17:35:27 PST (-0800), [email protected] wrote:
Add dma-channels property, then we can determine how many channels there by device tree, rather than statically defines it in PDMA driver
Maybe "statically defining it" is better here?
Thanks for the correction, I would modify it in the next version.
Quoted 14 lines
Quoted 14 lines
Signed-off-by Zong Li <[email protected]>--- .../devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml | 7 +++++++ 1 file changed, 7 insertions(+)Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +0
-0@@ -34,6 +34,12 @@ properties: minItems: 1 maxItems: 8
Quoted 4 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml properties: · line 38
Quoted 2 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +2
-0+ dma-channels: + description: For backwards-compatible, the default value is 4Maybe "backwards-compatibility" is better here?
Thanks, let me change it in the next version together.
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +4
-0+ minimum: 1 + maximum: 4 + default: 4 + '#dma-cells': const: 1
Quoted 9 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml examples: · line 57
Quoted 3 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +0
-0@@ -50,6 +56,7 @@ examples: dma@3000000 { compatible = "sifive,fu540-c000-pdma";IMO we should have a "sifive,pdma-1.0.0" (or whatever the versioning scheme ended up being) here, in addition to the SOC-specific DT entry. It's kind of odd to start extending the SOC-specific DT entry, as the whole idea there is to let us have an out in case we find future compatibility issues.
yes, I notice that we usually have a entry with the version in other device nodes, such as "sifive,xxx0", I guess we should have a "sifive,pdma0" there, thanks for your advice, let me add it and use it in match table of pdma driver in the next version.
Quoted 6 lines
Quoted 6 lines
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +1
-0reg = <0x3000000 0x8000>; + dma-channels = <4>; interrupts = <23 24 25 26 27 28 29 30>; #dma-cells = <1>; };
PATCH v4 3/3 dmaengine: sf-pdma: Get number of channel by device tree
In reply to Zong LiIt currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
--- drivers/dma/sf-pdma/sf-pdma.c | 20 +++++++++++++------- drivers/dma/sf-pdma/sf-pdma.h | 8 ++------ 2 files changed, 15 insertions(+), 13 deletions(-)
drivers/dma/sf-pdma/sf-pdma.c +13 -7
@@ -482,9 +482,7 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma)static int sf_pdma_probe (struct platform_device *pdev) {struct sf_pdma *pdma; -struct sf_pdma_chan *chan;struct resource *res; -int len, chans;int ret;const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES |
@@ -492,13 +490,21 @@ static int sf_pdma_probe(struct platform_device *pdev)DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; - pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL ); + pdma =devm_kzalloc (&pdev->dev,sizeof (*pdma),GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } + +if (pdma->n_chans >PDMA_MAX_NR_CH ) { +dev_err (&pdev->dev,"the number of channels exceeds the maximum\n" ); +return -EINVAL ; + } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);
@@ -556,7 +562,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i; -for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];devm_free_irq (&pdev->dev, ch->txirq, ch);
drivers/dma/sf-pdma/sf-pdma.h +2 -6
@@ -22,11 +22,7 @@#include "../dmaengine.h" #include "../virt-dma.h" -#define PDMA_NR_CH 4 - -#if (PDMA_NR_CH !=4 ) -#error "Please define PDMA_NR_CH to 4" -#endif +#define PDMA_MAX_NR_CH 4#define PDMA_BASE_ADDR 0x3000000#define PDMA_CHAN_OFFSET 0x1000
@@ -118,7 +114,7 @@ struct sf_pdma {void __iomem *membase;void __iomem *mappedbase;u32 n_chans; - struct sf_pdma_chan chans[PDMA_NR_CH]; + struct sf_pdma_chan chans[PDMA_MAX_NR_CH]; };#endif /* _SF_PDMA_H */
Signature · 1 line
2.31.1
Palmer Dabbelt
maintainerRe: [PATCH v4 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Quoted 6 lines · On Sun, 16 Jan 2022 17:35:28 PST (-0800), [email protected] wrote:
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Some of the same wording issues here as those I pointed out in the DT bindings patch.
Quoted 32 lines
Signed-off-by Zong Li <[email protected]>--- drivers/dma/sf-pdma/sf-pdma.c | 20 +++++++++++++------- drivers/dma/sf-pdma/sf-pdma.h | 8 ++------ 2 files changed, 15 insertions(+), 13 deletions(-)drivers/dma/sf-pdma/sf-pdma.c +1
-6@@ -482,9 +482,7 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma)static int sf_pdma_probe (struct platform_device *pdev) {struct sf_pdma *pdma; -struct sf_pdma_chan *chan;struct resource *res; -int len, chans;int ret;const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES |@@ -492,13 +490,21 @@ static int sf_pdma_probe(struct platform_device *pdev)DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; - pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL ); + pdma =devm_kzalloc (&pdev->dev,sizeof (*pdma),GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans;
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 506
Quoted 10 lines
drivers/dma/sf-pdma/sf-pdma.c +10
-0+ ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } + +if (pdma->n_chans >PDMA_MAX_NR_CH ) { +dev_err (&pdev->dev,"the number of channels exceeds the maximum\n" ); +return -EINVAL ;
Can we get away with just using only the number of channels the driver actually supports? ie, just never sending an op to the channels above MAX_NR_CH? That should leave us with nothing to track.
Quoted 39 lines
drivers/dma/sf-pdma/sf-pdma.c +2
-1+ } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);@@ -556,7 +562,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i; -for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];devm_free_irq (&pdev->dev, ch->txirq, ch);drivers/dma/sf-pdma/sf-pdma.h +2
-6@@ -22,11 +22,7 @@#include "../dmaengine.h" #include "../virt-dma.h" -#define PDMA_NR_CH 4 - -#if (PDMA_NR_CH !=4 ) -#error "Please define PDMA_NR_CH to 4" -#endif +#define PDMA_MAX_NR_CH 4#define PDMA_BASE_ADDR 0x3000000#define PDMA_CHAN_OFFSET 0x1000@@ -118,7 +114,7 @@ struct sf_pdma {void __iomem *membase;void __iomem *mappedbase;u32 n_chans; - struct sf_pdma_chan chans[PDMA_NR_CH]; + struct sf_pdma_chan chans[PDMA_MAX_NR_CH]; };#endif /* _SF_PDMA_H */
Re: [PATCH v4 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Quoted 43 lines · Palmer Dabbelt
Quoted 6 lines · On Sun, 16 Jan 2022 17:35:28 PST (-0800), [email protected] wrote:
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Some of the same wording issues here as those I pointed out in the DT bindings patch.
Quoted 31 lines
Signed-off-by Zong Li <[email protected]>--- drivers/dma/sf-pdma/sf-pdma.c | 20 +++++++++++++------- drivers/dma/sf-pdma/sf-pdma.h | 8 ++------ 2 files changed, 15 insertions(+), 13 deletions(-)drivers/dma/sf-pdma/sf-pdma.c +1
-5@@ -482,9 +482,7 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma)static int sf_pdma_probe (struct platform_device *pdev) {struct sf_pdma *pdma; -struct sf_pdma_chan *chan;struct resource *res; -int len, chans;int ret;const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES |@@ -492,13 +490,21 @@ static int sf_pdma_probe(struct platform_device *pdev)DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; - pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL ); + pdma =devm_kzalloc (&pdev->dev,sizeof (*pdma),GFP_KERNEL );if (!pdma)return -ENOMEM ;
Quoted 15 lines
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 506
Quoted 11 lines
drivers/dma/sf-pdma/sf-pdma.c +10
-1- pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } + +if (pdma->n_chans >PDMA_MAX_NR_CH ) { +dev_err (&pdev->dev,"the number of channels exceeds the maximum\n" ); +return -EINVAL ;Can we get away with just using only the number of channels the driver actually supports? ie, just never sending an op to the channels above
MAX_NR_CH? That should leave us with nothing to track.
It might be a bit like when pdma->n_chans is bigger than the maximum, set the pdma->chans to PDMA_MAX_NR_CH, then we could ensure that we don't access the channels above the maximum. If I understand correctly, I gave the similar thought in the thread of v2 patch, and there are some discussions on that, but this way seems to lead to hard-to-track problems.
Quoted 40 lines
Quoted 40 lines
drivers/dma/sf-pdma/sf-pdma.c +2
-1+ } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);@@ -556,7 +562,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i; -for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];devm_free_irq (&pdev->dev, ch->txirq, ch);drivers/dma/sf-pdma/sf-pdma.h +2
-6@@ -22,11 +22,7 @@#include "../dmaengine.h" #include "../virt-dma.h" -#define PDMA_NR_CH 4 - -#if (PDMA_NR_CH !=4 ) -#error "Please define PDMA_NR_CH to 4" -#endif +#define PDMA_MAX_NR_CH 4#define PDMA_BASE_ADDR 0x3000000#define PDMA_CHAN_OFFSET 0x1000@@ -118,7 +114,7 @@ struct sf_pdma {void __iomem *membase;void __iomem *mappedbase;u32 n_chans; - struct sf_pdma_chan chans[PDMA_NR_CH]; + struct sf_pdma_chan chans[PDMA_MAX_NR_CH]; };#endif /* _SF_PDMA_H */
Re: [PATCH v4 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Hi Zong, Palmer,
Quoted 37 lines · Zong Li
Quoted 36 lines · Palmer Dabbelt
Quoted 6 lines · On Sun, 16 Jan 2022 17:35:28 PST (-0800), [email protected] wrote:
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Some of the same wording issues here as those I pointed out in the DT bindings patch.
Quoted 25 lines
Signed-off-by Zong Li <[email protected]>drivers/dma/sf-pdma/sf-pdma.c +1
-5@@ -482,9 +482,7 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma)static int sf_pdma_probe (struct platform_device *pdev) {struct sf_pdma *pdma; -struct sf_pdma_chan *chan;struct resource *res; -int len, chans;int ret;const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES |@@ -492,13 +490,21 @@ static int sf_pdma_probe(struct platform_device *pdev)DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; - pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL ); + pdma =devm_kzalloc (&pdev->dev,sizeof (*pdma),GFP_KERNEL );if (!pdma)return -ENOMEM ;
Quoted 15 lines
Quoted 12 lines
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 506
Quoted 11 lines
drivers/dma/sf-pdma/sf-pdma.c +10
-1- pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } + +if (pdma->n_chans >PDMA_MAX_NR_CH ) { +dev_err (&pdev->dev,"the number of channels exceeds the maximum\n" ); +return -EINVAL ;Quoted 3 lines
Can we get away with just using only the number of channels the driver actually supports? ie, just never sending an op to the channels above
MAX_NR_CH? That should leave us with nothing to track.
In theory we can...
Quoted 6 lines
It might be a bit like when
pdma->n_chansis bigger than the maximum, set thepdma->chanstoPDMA_MAX_NR_CH, then we could ensure that we don't access the channels above the maximum. If I understand correctly, I gave the similar thought in the thread of v2 patch, and there are some discussions on that, but this way seems to lead to hard-to-track problems.
... but that would mean that when a new variant appears that supports more channels, no error is printed, and people might not notice immediately that the higher channels are never used.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Quoted 66 lines · Geert Uytterhoeven
Hi Zong, Palmer,
Quoted 37 lines · Zong Li
Quoted 36 lines · Palmer Dabbelt
Quoted 6 lines · On Sun, 16 Jan 2022 17:35:28 PST (-0800), [email protected] wrote:
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Some of the same wording issues here as those I pointed out in the DT bindings patch.
Quoted 25 lines
Signed-off-by Zong Li <[email protected]>drivers/dma/sf-pdma/sf-pdma.c +1
-5@@ -482,9 +482,7 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma)static int sf_pdma_probe (struct platform_device *pdev) {struct sf_pdma *pdma; -struct sf_pdma_chan *chan;struct resource *res; -int len, chans;int ret;const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES |@@ -492,13 +490,21 @@ static int sf_pdma_probe(struct platform_device *pdev)DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; - pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL ); + pdma =devm_kzalloc (&pdev->dev,sizeof (*pdma),GFP_KERNEL );if (!pdma)return -ENOMEM ;Quoted 15 lines
Quoted 12 lines
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 506
Quoted 11 lines
drivers/dma/sf-pdma/sf-pdma.c +10
-1- pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } + +if (pdma->n_chans >PDMA_MAX_NR_CH ) { +dev_err (&pdev->dev,"the number of channels exceeds the maximum\n" ); +return -EINVAL ;Quoted 3 lines
Can we get away with just using only the number of channels the driver actually supports? ie, just never sending an op to the channels above
MAX_NR_CH? That should leave us with nothing to track.In theory we can...
Quoted 6 lines
It might be a bit like when
pdma->n_chansis bigger than the maximum, set thepdma->chanstoPDMA_MAX_NR_CH, then we could ensure that we don't access the channels above the maximum. If I understand correctly, I gave the similar thought in the thread of v2 patch, and there are some discussions on that, but this way seems to lead to hard-to-track problems.
Quoted 4 lines
... but that would mean that when a new variant appears that supports more channels, no error is printed, and people might not notice immediately that the higher channels are never used.
I guess people might need to follow the dt-bindings, so they couldn't specify the number of channels to the value which is more than maximum. But as you mentioned, if people don't notice that and specify it more than maximum, they wouldn't be aware that the higher channels are never used. It seems to me that we could keep returning the error there, or show a warning message and use PDMA_MAX_NR_CH in that situation, both looks good to me.
Quoted 10 lines
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Quoted 73 lines · Zong Li
Quoted 66 lines · Geert Uytterhoeven
Hi Zong, Palmer,
Quoted 37 lines · Zong Li
Quoted 36 lines · Palmer Dabbelt
Quoted 6 lines · On Sun, 16 Jan 2022 17:35:28 PST (-0800), [email protected] wrote:
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Some of the same wording issues here as those I pointed out in the DT bindings patch.
Quoted 25 lines
Signed-off-by Zong Li <[email protected]>drivers/dma/sf-pdma/sf-pdma.c +1
-5@@ -482,9 +482,7 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma)static int sf_pdma_probe (struct platform_device *pdev) {struct sf_pdma *pdma; -struct sf_pdma_chan *chan;struct resource *res; -int len, chans;int ret;const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES |@@ -492,13 +490,21 @@ static int sf_pdma_probe(struct platform_device *pdev)DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; - pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL ); + pdma =devm_kzalloc (&pdev->dev,sizeof (*pdma),GFP_KERNEL );if (!pdma)return -ENOMEM ;Quoted 15 lines
Quoted 12 lines
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 506
Quoted 11 lines
drivers/dma/sf-pdma/sf-pdma.c +10
-1- pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } + +if (pdma->n_chans >PDMA_MAX_NR_CH ) { +dev_err (&pdev->dev,"the number of channels exceeds the maximum\n" ); +return -EINVAL ;Quoted 3 lines
Can we get away with just using only the number of channels the driver actually supports? ie, just never sending an op to the channels above
MAX_NR_CH? That should leave us with nothing to track.In theory we can...
Quoted 6 lines
It might be a bit like when
pdma->n_chansis bigger than the maximum, set thepdma->chanstoPDMA_MAX_NR_CH, then we could ensure that we don't access the channels above the maximum. If I understand correctly, I gave the similar thought in the thread of v2 patch, and there are some discussions on that, but this way seems to lead to hard-to-track problems.Quoted 4 lines
... but that would mean that when a new variant appears that supports more channels, no error is printed, and people might not notice immediately that the higher channels are never used.
Quoted 8 lines
I guess people might need to follow the dt-bindings, so they couldn't specify the number of channels to the value which is more than maximum. But as you mentioned, if people don't notice that and specify it more than maximum, they wouldn't be aware that the higher channels are never used. It seems to me that we could keep returning the error there, or show a warning message and use
PDMA_MAX_NR_CHin that situation, both looks good to me.
Hi all, thank you for the review, I'd like to prepare the next version patch, if current implementation of this part is ok to you, I will keep it in the next version. Please let me know if anything can be improved. Thanks
Quoted 10 lines
Quoted 10 lines
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv