[PATCH v2 3/4] ARM: dts: stm32: Add MDMA support for STM32H743 SoC

[Pierre-Yves MORDRET]

This patch adds MDMA support for STM32H743 SoC.

Signed-off-by: Pierre-Yves MORDRET <pierre-***@st.com>
---
Version history:
v2:
* Add MDMA support in DT for H7
---
---
arch/arm/boot/dts/stm32h743.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/stm32h743.dtsi b/arch/arm/boot/dts/stm32h743.dtsi
index 4685629..cc566df 100644
--- a/arch/arm/boot/dts/stm32h743.dtsi
+++ b/arch/arm/boot/dts/stm32h743.dtsi
@@ -74,6 +74,16 @@
interrupts = <50>;
clocks = <&timer_clk>;
};
+
+ mdma1: ***@52000000 {
+ compatible = "st,stm32h7-mdma";
+ reg = <0x52000000 0x1000>;
+ interrupts = <122>;
+ clocks = <&timer_clk>;
+ #dma-cells = <5>;
+ dma-channels = <16>;
+ dma-requests = <32>;
+ };
};
};

[Pierre-Yves MORDRET]

This patch adds MDMA support in STM32 defconfig file

Signed-off-by: Pierre-Yves MORDRET <pierre-***@st.com>
---
Version history:
v2:
* Add MDMA support in STM32 defconfig
---
---
arch/arm/configs/stm32_defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
index a097538..daa5c91 100644
--- a/arch/arm/configs/stm32_defconfig
+++ b/arch/arm/configs/stm32_defconfig
@@ -64,6 +64,7 @@ CONFIG_RTC_CLASS=y
CONFIG_RTC_DRV_STM32=y
CONFIG_DMADEVICES=y
CONFIG_STM32_DMA=y
+CONFIG_STM32_MDMA=y
CONFIG_IIO=y
CONFIG_STM32_ADC_CORE=y
CONFIG_STM32_ADC=y

[Pierre-Yves MORDRET]

This patch adds documentation of device tree bindings for the STM32 MDMA
controller.

Signed-off-by: M'boumba Cedric Madianga <***@gmail.com>
Signed-off-by: Pierre-Yves MORDRET <pierre-***@st.com>
---
Version history:
v2:
* change compatible into st,stm32h7-mdma to be more SoC specific
---
---
.../devicetree/bindings/dma/stm32-mdma.txt | 94 ++++++++++++++++++++++
1 file changed, 94 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/stm32-mdma.txt

diff --git a/Documentation/devicetree/bindings/dma/stm32-mdma.txt b/Documentation/devicetree/bindings/dma/stm32-mdma.txt
new file mode 100644
index 0000000..d18772d
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/stm32-mdma.txt
@@ -0,0 +1,94 @@
+* STMicroelectronics STM32 MDMA controller
+
+The STM32 MDMA is a general-purpose direct memory access controller capable of
+supporting 64 independent DMA channels with 256 HW requests.
+
+Required properties:
+- compatible: Should be "st,stm32h7-mdma"
+- reg: Should contain MDMA registers location and length. This should include
+ all of the per-channel registers.
+- interrupts: Should contain the MDMA interrupt.
+- clocks: Should contain the input clock of the DMA instance.
+- resets: Reference to a reset controller asserting the DMA controller.
+- #dma-cells : Must be <5>. See DMA client paragraph for more details.
+
+Optional properties:
+- dma-channels: Number of DMA channels supported by the controller.
+- dma-requests: Number of DMA request signals supported by the controller.
+- st,ahb-addr-masks: Array of u32 mask to list memory devices addressed via
+ AHB bus.
+
+Example:
+
+ mdma1: ***@52000000 {
+ compatible = "st,stm32h7-mdma";
+ reg = <0x52000000 0x1000>;
+ interrupts = <122>;
+ clocks = <&timer_clk>;
+ resets = <&rcc 992>;
+ #dma-cells = <5>;
+ dma-channels = <16>;
+ dma-requests = <32>;
+ st,ahb-addr-masks = <0x20000000>, <0x00000000>;
+ };
+
+* DMA client
+
+DMA clients connected to the STM32 MDMA controller must use the format
+described in the dma.txt file, using a five-cell specifier for each channel:
+a phandle to the MDMA controller plus the following five integer cells:
+
+1. The request line number
+2. The priority level
+ 0x00: Low
+ 0x01: Medium
+ 0x10: High
+ 0x11: Very high
+3. A 32bit mask specifying the DMA channel configuration
+ -bit 0-1: Source increment mode
+ 0x00: Source address pointer is fixed
+ 0x10: Source address pointer is incremented after each data transfer
+ 0x11: Source address pointer is decremented after each data transfer
+ -bit 2-3: Destination increment mode
+ 0x00: Destination address pointer is fixed
+ 0x10: Destination address pointer is incremented after each data
+ transfer
+ 0x11: Destination address pointer is decremented after each data
+ transfer
+ -bit 8-9: Source increment offset size
+ 0x00: byte (8bit)
+ 0x01: half-word (16bit)
+ 0x10: word (32bit)
+ 0x11: double-word (64bit)
+ -bit 10-11: Destination increment offset size
+ 0x00: byte (8bit)
+ 0x01: half-word (16bit)
+ 0x10: word (32bit)
+ 0x11: double-word (64bit)
+-bit 25-18: The number of bytes to be transferred in a single transfer
+ (min = 1 byte, max = 128 bytes)
+-bit 29:28: Trigger Mode
+ 0x00: Each MDMA request triggers a buffer transfer (max 128 bytes)
+ 0x01: Each MDMA request triggers a block transfer (max 64K bytes)
+ 0x10: Each MDMA request triggers a repeated block transfer
+ 0x11: Each MDMA request triggers a linked list transfer
+4. A 32bit value specifying the register to be used to acknowledge the request
+ if no HW ack signal is used by the MDMA client
+5. A 32bit mask specifying the value to be written to acknowledge the request
+ if no HW ack signal is used by the MDMA client
+
+Example:
+
+ i2c4: ***@5c002000 {
+ compatible = "st,stm32f7-i2c";
+ reg = <0x5c002000 0x400>;
+ interrupts = <95>,
+ <96>;
+ clocks = <&timer_clk>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ dmas = <&mdma1 36 0x0 0x40008 0x0 0x0>,
+ <&mdma1 37 0x0 0x40002 0x0 0x0>;
+ dma-names = "rx", "tx";
+ status = "disabled";
+ };

[Rob Herring]

Post by Pierre-Yves MORDRET
This patch adds documentation of device tree bindings for the STM32 MDMA
controller.
---
* change compatible into st,stm32h7-mdma to be more SoC specific
---
---
.../devicetree/bindings/dma/stm32-mdma.txt | 94 ++++++++++++++++++++++
1 file changed, 94 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/stm32-mdma.txt

Acked-by: Rob Herring <***@kernel.org>

[Pierre-Yves MORDRET]

This post might be inappropriate. Click to display it.

[Vinod Koul]

Post by Pierre-Yves MORDRET
+config STM32_MDMA
+ bool "STMicroelectronics STM32 master dma support"
+ depends on ARCH_STM32 || COMPILE_TEST

^^^
why multiple spaces

Post by Pierre-Yves MORDRET
+static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
+{
+ enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
+
+ while (((buf_len % max_width) || (tlen < max_width)) &&
+ (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
+ max_width = max_width >> 1;

ok, this is a bit hard to read...

Post by Pierre-Yves MORDRET
+static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
+ enum dma_transfer_direction direction,
+ u32 *mdma_ccr, u32 *mdma_ctcr,
+ u32 *mdma_ctbr, u32 buf_len)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
+ enum dma_slave_buswidth src_addr_width, dst_addr_width;
+ phys_addr_t src_addr, dst_addr;
+ int src_bus_width, dst_bus_width;
+ u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
+ u32 ccr, ctcr, ctbr, tlen;
+
+ src_addr_width = chan->dma_config.src_addr_width;
+ dst_addr_width = chan->dma_config.dst_addr_width;
+ src_maxburst = chan->dma_config.src_maxburst;
+ dst_maxburst = chan->dma_config.dst_maxburst;
+ src_addr = chan->dma_config.src_addr;
+ dst_addr = chan->dma_config.dst_addr;

this doesn't seem right to me, only the periphral address would come from
slave_config, the memory address is passed as an arg to transfer..

...

Post by Pierre-Yves MORDRET
+static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
+ struct stm32_mdma_desc *desc,
+ struct scatterlist *sgl, u32 sg_len,
+ enum dma_transfer_direction direction)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct dma_slave_config *dma_config = &chan->dma_config;
+ struct scatterlist *sg;
+ dma_addr_t src_addr, dst_addr;
+ u32 ccr, ctcr, ctbr;
+ int i, ret = 0;
+
+ for_each_sg(sgl, sg, sg_len, i) {
+ if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
+ dev_err(chan2dev(chan), "Invalid block len\n");
+ return -EINVAL;
+ }
+
+ ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
+ &ctbr, sg_dma_len(sg));
+ if (ret < 0)
+ return ret;
+
+ if (direction == DMA_MEM_TO_DEV) {
+ src_addr = sg_dma_address(sg);
+ dst_addr = dma_config->dst_addr;

and this seems correct, but then why are we doing it in
stm32_mdma_set_xfer_param()

Post by Pierre-Yves MORDRET
+static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
+ struct dma_chan *c, struct scatterlist *sgl,
+ u32 sg_len, enum dma_transfer_direction direction,
+ unsigned long flags, void *context)

right justfied these please, it makes a terrible read

Post by Pierre-Yves MORDRET
+{
+ struct stm32_mdma_chan *chan = to_stm32_mdma_chan(c);
+ struct stm32_mdma_desc *desc;
+ int ret;
+
+ desc = stm32_mdma_alloc_desc(chan, sg_len);
+ if (!desc)
+ return NULL;
+
+ ret = stm32_mdma_setup_xfer(chan, desc, sgl, sg_len, direction);
+ if (ret < 0)
+ goto xfer_setup_err;
+
+ desc->cyclic = false;
+
+ return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
+
+ dma_pool_free(chan->desc_pool, &desc->hwdesc, desc->hwdesc_phys);
+ kfree(desc);
+ return NULL;
+}
+
+static struct dma_async_tx_descriptor *stm32_mdma_prep_dma_cyclic(
+ struct dma_chan *c, dma_addr_t buf_addr, size_t buf_len,
+ size_t period_len, enum dma_transfer_direction direction,
+ unsigned long flags)

here too and few other places

Post by Pierre-Yves MORDRET
+static int stm32_mdma_probe(struct platform_device *pdev)
+{
+ struct stm32_mdma_chan *chan;
+ struct stm32_mdma_device *dmadev;
+ struct dma_device *dd;
+ struct device_node *of_node;
+ struct resource *res;
+ u32 nr_channels, nr_requests;
+ int i, count, ret;
+
+ of_node = pdev->dev.of_node;
+ if (!of_node)
+ return -ENODEV;
+
+ ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
+ if (ret)
+ nr_channels = STM32_MDMA_MAX_CHANNELS;
+
+ ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
+ if (ret)
+ nr_requests = STM32_MDMA_MAX_REQUESTS;

wouldn't it make sense to print error about these properties not being
present and continuing w/ defaults..?

and can we have device_property_xxx instead of of_ variants?

Post by Pierre-Yves MORDRET
+static int __init stm32_mdma_init(void)
+{
+ return platform_driver_probe(&stm32_mdma_driver, stm32_mdma_probe);
+}
+
+subsys_initcall(stm32_mdma_init);

Where are the MODULE_xx tags, license is mandatory. You should add author
too.

[Pierre Yves MORDRET]

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+config STM32_MDMA
+ bool "STMicroelectronics STM32 master dma support"
+ depends on ARCH_STM32 || COMPILE_TEST

^^^
why multiple spaces

typo I guess

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
+{
+ enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
+
+ while (((buf_len % max_width) || (tlen < max_width)) &&
+ (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
+ max_width = max_width >> 1;

ok, this is a bit hard to read...

This code snippet has already been reworked and optimized. Would you mind to
provide me a example with your expectation ? Thanks

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
+ enum dma_transfer_direction direction,
+ u32 *mdma_ccr, u32 *mdma_ctcr,
+ u32 *mdma_ctbr, u32 buf_len)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
+ enum dma_slave_buswidth src_addr_width, dst_addr_width;
+ phys_addr_t src_addr, dst_addr;
+ int src_bus_width, dst_bus_width;
+ u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
+ u32 ccr, ctcr, ctbr, tlen;
+
+ src_addr_width = chan->dma_config.src_addr_width;
+ dst_addr_width = chan->dma_config.dst_addr_width;
+ src_maxburst = chan->dma_config.src_maxburst;
+ dst_maxburst = chan->dma_config.dst_maxburst;
+ src_addr = chan->dma_config.src_addr;
+ dst_addr = chan->dma_config.dst_addr;

this doesn't seem right to me, only the periphral address would come from
slave_config, the memory address is passed as an arg to transfer..
...

Correct. But these locals are managed in the case statement below. if direction
is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
Dev2Mem direction only src_addr(Peripheral) is considered.
However to disambiguate I can move src_addr & dst_addr affectation in the
corresponding case statement if you'd like.

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
+ struct stm32_mdma_desc *desc,
+ struct scatterlist *sgl, u32 sg_len,
+ enum dma_transfer_direction direction)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct dma_slave_config *dma_config = &chan->dma_config;
+ struct scatterlist *sg;
+ dma_addr_t src_addr, dst_addr;
+ u32 ccr, ctcr, ctbr;
+ int i, ret = 0;
+
+ for_each_sg(sgl, sg, sg_len, i) {
+ if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
+ dev_err(chan2dev(chan), "Invalid block len\n");
+ return -EINVAL;
+ }
+
+ ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
+ &ctbr, sg_dma_len(sg));
+ if (ret < 0)
+ return ret;
+
+ if (direction == DMA_MEM_TO_DEV) {
+ src_addr = sg_dma_address(sg);
+ dst_addr = dma_config->dst_addr;

and this seems correct, but then why are we doing it in
stm32_mdma_set_xfer_param()

See comment above.

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
+ struct dma_chan *c, struct scatterlist *sgl,
+ u32 sg_len, enum dma_transfer_direction direction,
+ unsigned long flags, void *context)

right justfied these please, it makes a terrible read

Given the amount of parameters difficult to right align.
Agree with this formatting ?

static struct dma_async_tx_descriptor
*stm32_mdma_prep_slave_sg(struct dma_chan *c, struct scatterlist *sgl,
u32 sg_len, enum dma_transfer_direction direction,
unsigned long flags, void *context)

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+{
+ struct stm32_mdma_chan *chan = to_stm32_mdma_chan(c);
+ struct stm32_mdma_desc *desc;
+ int ret;
+
+ desc = stm32_mdma_alloc_desc(chan, sg_len);
+ if (!desc)
+ return NULL;
+
+ ret = stm32_mdma_setup_xfer(chan, desc, sgl, sg_len, direction);
+ if (ret < 0)
+ goto xfer_setup_err;
+
+ desc->cyclic = false;
+
+ return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
+
+ dma_pool_free(chan->desc_pool, &desc->hwdesc, desc->hwdesc_phys);
+ kfree(desc);
+ return NULL;
+}
+
+static struct dma_async_tx_descriptor *stm32_mdma_prep_dma_cyclic(
+ struct dma_chan *c, dma_addr_t buf_addr, size_t buf_len,
+ size_t period_len, enum dma_transfer_direction direction,
+ unsigned long flags)

here too and few other places

ok. See comment above.

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_probe(struct platform_device *pdev)
+{
+ struct stm32_mdma_chan *chan;
+ struct stm32_mdma_device *dmadev;
+ struct dma_device *dd;
+ struct device_node *of_node;
+ struct resource *res;
+ u32 nr_channels, nr_requests;
+ int i, count, ret;
+
+ of_node = pdev->dev.of_node;
+ if (!of_node)
+ return -ENODEV;
+
+ ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
+ if (ret)
+ nr_channels = STM32_MDMA_MAX_CHANNELS;
+
+ ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
+ if (ret)
+ nr_requests = STM32_MDMA_MAX_REQUESTS;

wouldn't it make sense to print error about these properties not being
present and continuing w/ defaults..?

Those are optional parameters as stated by bindings. I can print out a warning
or info if you'd like but not error.

Post by Vinod Koul
and can we have device_property_xxx instead of of_ variants?

of course !

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int __init stm32_mdma_init(void)
+{
+ return platform_driver_probe(&stm32_mdma_driver, stm32_mdma_probe);
+}
+
+subsys_initcall(stm32_mdma_init);

Where are the MODULE_xx tags, license is mandatory. You should add author
too.

Correct. I will change the Header at the same time.

Thanks.

[Vinod Koul]

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
+{
+ enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
+
+ while (((buf_len % max_width) || (tlen < max_width)) &&
+ (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
+ max_width = max_width >> 1;

ok, this is a bit hard to read...

This code snippet has already been reworked and optimized. Would you mind to
provide me a example with your expectation ? Thanks

Code is optimized yes, but readable no

I would like readability to be improved upon...

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
+ enum dma_transfer_direction direction,
+ u32 *mdma_ccr, u32 *mdma_ctcr,
+ u32 *mdma_ctbr, u32 buf_len)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
+ enum dma_slave_buswidth src_addr_width, dst_addr_width;
+ phys_addr_t src_addr, dst_addr;
+ int src_bus_width, dst_bus_width;
+ u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
+ u32 ccr, ctcr, ctbr, tlen;
+
+ src_addr_width = chan->dma_config.src_addr_width;
+ dst_addr_width = chan->dma_config.dst_addr_width;
+ src_maxburst = chan->dma_config.src_maxburst;
+ dst_maxburst = chan->dma_config.dst_maxburst;
+ src_addr = chan->dma_config.src_addr;
+ dst_addr = chan->dma_config.dst_addr;

this doesn't seem right to me, only the periphral address would come from
slave_config, the memory address is passed as an arg to transfer..
...

Correct. But these locals are managed in the case statement below. if direction
is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
Dev2Mem direction only src_addr(Peripheral) is considered.
However to disambiguate I can move src_addr & dst_addr affectation in the
corresponding case statement if you'd like.

But below you are over writing both, so in effect this is wasted cycles..
anyway latter one is more clear, so lets remove from here.

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
+ struct stm32_mdma_desc *desc,
+ struct scatterlist *sgl, u32 sg_len,
+ enum dma_transfer_direction direction)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct dma_slave_config *dma_config = &chan->dma_config;
+ struct scatterlist *sg;
+ dma_addr_t src_addr, dst_addr;
+ u32 ccr, ctcr, ctbr;
+ int i, ret = 0;
+
+ for_each_sg(sgl, sg, sg_len, i) {
+ if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
+ dev_err(chan2dev(chan), "Invalid block len\n");
+ return -EINVAL;
+ }
+
+ ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
+ &ctbr, sg_dma_len(sg));
+ if (ret < 0)
+ return ret;
+
+ if (direction == DMA_MEM_TO_DEV) {
+ src_addr = sg_dma_address(sg);
+ dst_addr = dma_config->dst_addr;

and this seems correct, but then why are we doing it in
stm32_mdma_set_xfer_param()

See comment above.

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
+ struct dma_chan *c, struct scatterlist *sgl,
+ u32 sg_len, enum dma_transfer_direction direction,
+ unsigned long flags, void *context)

right justfied these please, it makes a terrible read

Given the amount of parameters difficult to right align.
Agree with this formatting ?
static struct dma_async_tx_descriptor
*stm32_mdma_prep_slave_sg(struct dma_chan *c, struct scatterlist *sgl,
u32 sg_len, enum dma_transfer_direction direction,
unsigned long flags, void *context)

Yes looks much better :)

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_probe(struct platform_device *pdev)
+{
+ struct stm32_mdma_chan *chan;
+ struct stm32_mdma_device *dmadev;
+ struct dma_device *dd;
+ struct device_node *of_node;
+ struct resource *res;
+ u32 nr_channels, nr_requests;
+ int i, count, ret;
+
+ of_node = pdev->dev.of_node;
+ if (!of_node)
+ return -ENODEV;
+
+ ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
+ if (ret)
+ nr_channels = STM32_MDMA_MAX_CHANNELS;
+
+ ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
+ if (ret)
+ nr_requests = STM32_MDMA_MAX_REQUESTS;

wouldn't it make sense to print error about these properties not being
present and continuing w/ defaults..?

Those are optional parameters as stated by bindings. I can print out a warning
or info if you'd like but not error.

Are these mandatory properties or optional. In case of latter warn should
suffice.

[Pierre Yves MORDRET]

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
+{
+ enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
+
+ while (((buf_len % max_width) || (tlen < max_width)) &&
+ (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
+ max_width = max_width >> 1;

ok, this is a bit hard to read...

This code snippet has already been reworked and optimized. Would you mind to
provide me a example with your expectation ? Thanks

Code is optimized yes, but readable no
I would like readability to be improved upon...

gotcha

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
+ enum dma_transfer_direction direction,
+ u32 *mdma_ccr, u32 *mdma_ctcr,
+ u32 *mdma_ctbr, u32 buf_len)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
+ enum dma_slave_buswidth src_addr_width, dst_addr_width;
+ phys_addr_t src_addr, dst_addr;
+ int src_bus_width, dst_bus_width;
+ u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
+ u32 ccr, ctcr, ctbr, tlen;
+
+ src_addr_width = chan->dma_config.src_addr_width;
+ dst_addr_width = chan->dma_config.dst_addr_width;
+ src_maxburst = chan->dma_config.src_maxburst;
+ dst_maxburst = chan->dma_config.dst_maxburst;
+ src_addr = chan->dma_config.src_addr;
+ dst_addr = chan->dma_config.dst_addr;

this doesn't seem right to me, only the periphral address would come from
slave_config, the memory address is passed as an arg to transfer..
...

Correct. But these locals are managed in the case statement below. if direction
is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
Dev2Mem direction only src_addr(Peripheral) is considered.
However to disambiguate I can move src_addr & dst_addr affectation in the
corresponding case statement if you'd like.

But below you are over writing both, so in effect this is wasted cycles..
anyway latter one is more clear, so lets remove from here.

Sorry I don't follow ... or miss something
For instance if direction is Mem2Dev ..._xfer_param is going to configure
Destination Bus width and Addr given by slave_config. ..._setup_xfer in its turn
will configure source given as parameter.
Don't the see the over-writing

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
+ struct stm32_mdma_desc *desc,
+ struct scatterlist *sgl, u32 sg_len,
+ enum dma_transfer_direction direction)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct dma_slave_config *dma_config = &chan->dma_config;
+ struct scatterlist *sg;
+ dma_addr_t src_addr, dst_addr;
+ u32 ccr, ctcr, ctbr;
+ int i, ret = 0;
+
+ for_each_sg(sgl, sg, sg_len, i) {
+ if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
+ dev_err(chan2dev(chan), "Invalid block len\n");
+ return -EINVAL;
+ }
+
+ ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
+ &ctbr, sg_dma_len(sg));
+ if (ret < 0)
+ return ret;
+
+ if (direction == DMA_MEM_TO_DEV) {
+ src_addr = sg_dma_address(sg);
+ dst_addr = dma_config->dst_addr;

and this seems correct, but then why are we doing it in
stm32_mdma_set_xfer_param()

See comment above.

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
+ struct dma_chan *c, struct scatterlist *sgl,
+ u32 sg_len, enum dma_transfer_direction direction,
+ unsigned long flags, void *context)

right justfied these please, it makes a terrible read

Given the amount of parameters difficult to right align.
Agree with this formatting ?
static struct dma_async_tx_descriptor
*stm32_mdma_prep_slave_sg(struct dma_chan *c, struct scatterlist *sgl,
u32 sg_len, enum dma_transfer_direction direction,
unsigned long flags, void *context)

Yes looks much better :)

Good :)

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_probe(struct platform_device *pdev)
+{
+ struct stm32_mdma_chan *chan;
+ struct stm32_mdma_device *dmadev;
+ struct dma_device *dd;
+ struct device_node *of_node;
+ struct resource *res;
+ u32 nr_channels, nr_requests;
+ int i, count, ret;
+
+ of_node = pdev->dev.of_node;
+ if (!of_node)
+ return -ENODEV;
+
+ ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
+ if (ret)
+ nr_channels = STM32_MDMA_MAX_CHANNELS;
+
+ ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
+ if (ret)
+ nr_requests = STM32_MDMA_MAX_REQUESTS;

wouldn't it make sense to print error about these properties not being
present and continuing w/ defaults..?

Those are optional parameters as stated by bindings. I can print out a warning
or info if you'd like but not error.

Are these mandatory properties or optional. In case of latter warn should
suffice.

optional. Let pick out warn then.

Thanks

[Vinod Koul]

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
+ enum dma_transfer_direction direction,
+ u32 *mdma_ccr, u32 *mdma_ctcr,
+ u32 *mdma_ctbr, u32 buf_len)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
+ enum dma_slave_buswidth src_addr_width, dst_addr_width;
+ phys_addr_t src_addr, dst_addr;
+ int src_bus_width, dst_bus_width;
+ u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
+ u32 ccr, ctcr, ctbr, tlen;
+
+ src_addr_width = chan->dma_config.src_addr_width;
+ dst_addr_width = chan->dma_config.dst_addr_width;
+ src_maxburst = chan->dma_config.src_maxburst;
+ dst_maxburst = chan->dma_config.dst_maxburst;
+ src_addr = chan->dma_config.src_addr;
+ dst_addr = chan->dma_config.dst_addr;

this doesn't seem right to me, only the periphral address would come from
slave_config, the memory address is passed as an arg to transfer..
...

Correct. But these locals are managed in the case statement below. if direction
is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
Dev2Mem direction only src_addr(Peripheral) is considered.
However to disambiguate I can move src_addr & dst_addr affectation in the
corresponding case statement if you'd like.

But below you are over writing both, so in effect this is wasted cycles..
anyway latter one is more clear, so lets remove from here.

Sorry I don't follow ... or miss something
For instance if direction is Mem2Dev ..._xfer_param is going to configure
Destination Bus width and Addr given by slave_config. ..._setup_xfer in its turn
will configure source given as parameter.
Don't the see the over-writing

ah re-looking at it, yes you are right.

The above two assignments threw me off, I should have read it properly.

But I think calculating for src and dstn always might not be optimal as you
would use one only, so should these be moved to respective case where they
are used...

[Pierre Yves MORDRET]

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
+{
+ enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
+
+ while (((buf_len % max_width) || (tlen < max_width)) &&
+ (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
+ max_width = max_width >> 1;

ok, this is a bit hard to read...

This code snippet has already been reworked and optimized. Would you mind to
provide me a example with your expectation ? Thanks

Code is optimized yes, but readable no
I would like readability to be improved upon...

gotcha

Doest he code snippet below has a better looking for you ?

for (max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
max_width > DMA_SLAVE_BUSWIDTH_1_BYTE; max_width >>= 1)
if (((buf_len % max_width) == 0) && (tlen >= max_width))
break;

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET

Post by Pierre-Yves MORDRET
+static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
+ enum dma_transfer_direction direction,
+ u32 *mdma_ccr, u32 *mdma_ctcr,
+ u32 *mdma_ctbr, u32 buf_len)
+{
+ struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
+ struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
+ enum dma_slave_buswidth src_addr_width, dst_addr_width;
+ phys_addr_t src_addr, dst_addr;
+ int src_bus_width, dst_bus_width;
+ u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
+ u32 ccr, ctcr, ctbr, tlen;
+
+ src_addr_width = chan->dma_config.src_addr_width;
+ dst_addr_width = chan->dma_config.dst_addr_width;
+ src_maxburst = chan->dma_config.src_maxburst;
+ dst_maxburst = chan->dma_config.dst_maxburst;
+ src_addr = chan->dma_config.src_addr;
+ dst_addr = chan->dma_config.dst_addr;

this doesn't seem right to me, only the periphral address would come from
slave_config, the memory address is passed as an arg to transfer..
...

Correct. But these locals are managed in the case statement below. if direction
is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
Dev2Mem direction only src_addr(Peripheral) is considered.
However to disambiguate I can move src_addr & dst_addr affectation in the
corresponding case statement if you'd like.

But below you are over writing both, so in effect this is wasted cycles..
anyway latter one is more clear, so lets remove from here.

Sorry I don't follow ... or miss something
For instance if direction is Mem2Dev ..._xfer_param is going to configure
Destination Bus width and Addr given by slave_config. ..._setup_xfer in its turn
will configure source given as parameter.
Don't the see the over-writing

ah re-looking at it, yes you are right.
The above two assignments threw me off, I should have read it properly.
But I think calculating for src and dstn always might not be optimal as you
would use one only, so should these be moved to respective case where they
are used...

Agree. This is my planned btw

Thanks
Py.

[Vinod Koul]

Post by Pierre Yves MORDRET

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32

tlen)

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre Yves MORDRET

Post by Vinod Koul

Post by Pierre-Yves MORDRET
+{
+ enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
+
+ while (((buf_len % max_width) || (tlen < max_width)) &&
+ (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
+ max_width = max_width >> 1;

ok, this is a bit hard to read...

This code snippet has already been reworked and optimized. Would you mind to
provide me a example with your expectation ? Thanks

Code is optimized yes, but readable no
I would like readability to be improved upon...

gotcha

Doest he code snippet below has a better looking for you ?
for (max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
max_width > DMA_SLAVE_BUSWIDTH_1_BYTE; max_width >>= 1)
if (((buf_len % max_width) == 0) && (tlen >= max_width))
break;

Am actually not sure :(

Indentation wise it is still a bit messy to follow..

How about:

for (max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
max_width > DMA_SLAVE_BUSWIDTH_1_BYTE;
max_width >>=1) {
if (((buf_len % max_width) == 0) && (tlen >= max_width))
break;
}

Thanks

[Pierre Yves MORDRET]

This patchset adds support for the STM32 MDMA controller.
The Master Direct memory access (MDMA) provides high-speed data transfer
between memory and memory or between peripherals and memory.
Contrary to STM32 DMA, the STM32 MDMA controller supports hardware LLI and
uses a larger integrated FIFO (128 vs 16 bytes)

Gentle ping for driver review since DT Bindings has been acked by Rob
Herring.

Thanks
Py

---
* change compatible into st,stm32h7-mdma to be more SoC specific
* Add MDMA support in DT for H7
* Add MDMA support in STM32 defconfig
---
dt-bindings: Document the STM32 MDMA bindings
dmaengine: Add STM32 MDMA driver
ARM: dts: stm32: Add MDMA support for STM32H743 SoC
ARM: configs: stm32: Add MDMA support in STM32 defconfig
.../devicetree/bindings/dma/stm32-mdma.txt | 94 ++
arch/arm/boot/dts/stm32h743.dtsi | 10 +
arch/arm/configs/stm32_defconfig | 1 +
drivers/dma/Kconfig | 12 +
drivers/dma/Makefile | 1 +
drivers/dma/stm32-mdma.c | 1536 ++++++++++++++++++++
6 files changed, 1654 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/stm32-mdma.txt
create mode 100644 drivers/dma/stm32-mdma.c

[Vinod Koul]

Post by Pierre Yves MORDRET

This patchset adds support for the STM32 MDMA controller.
The Master Direct memory access (MDMA) provides high-speed data transfer
between memory and memory or between peripherals and memory.
Contrary to STM32 DMA, the STM32 MDMA controller supports hardware LLI and
uses a larger integrated FIFO (128 vs 16 bytes)

Gentle ping for driver review since DT Bindings has been acked by Rob
Herring.

Please allow reasonable time for review, this was sent during merge window
and I dont apply patches during that time.

Post by Pierre Yves MORDRET
Thanks
Py

---
* change compatible into st,stm32h7-mdma to be more SoC specific
* Add MDMA support in DT for H7
* Add MDMA support in STM32 defconfig
---
dt-bindings: Document the STM32 MDMA bindings
dmaengine: Add STM32 MDMA driver
ARM: dts: stm32: Add MDMA support for STM32H743 SoC
ARM: configs: stm32: Add MDMA support in STM32 defconfig
.../devicetree/bindings/dma/stm32-mdma.txt | 94 ++
arch/arm/boot/dts/stm32h743.dtsi | 10 +
arch/arm/configs/stm32_defconfig | 1 +
drivers/dma/Kconfig | 12 +
drivers/dma/Makefile | 1 +
drivers/dma/stm32-mdma.c | 1536 ++++++++++++++++++++
6 files changed, 1654 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/stm32-mdma.txt
create mode 100644 drivers/dma/stm32-mdma.c