* [PATCH 0/3] dmaengine: Fix DMA on current allwinner SoCs, add A64 support @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring The latest generation of allwinner SoCs (e.g. H3, H5, A64) have some significant differences to older SoC generations (e.g. A23): - different register location for the clock autogating register (offset 0x28 vs 0x20) - different field offset for the src/dest burst length fields in the channel configuration register - additional burst lengths and widths This is the second change to the implementation of the DMA controller (first change being the addition of the clock autogating register in the A23). To differentiate between the different generations, the patch adds an enum value to the OF config data. The A83t uses the same register layout as the A23, so this series also fixes the missing clock gating setting for the A83t. The A64 is register compatible to the H3, but supports less channels and has a different set of endpoints. Add a new compatible string to the device tree and the driver. Stefan Brüns (3): dmaengine: sun6i: Correct DMA support on H3 arm64: allwinner: a64: Add device node for DMA controller dmaengine: sun6i: Add support for Allwinner A64 .../devicetree/bindings/dma/sun6i-dma.txt | 1 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 13 ++ drivers/dma/sun6i-dma.c | 135 +++++++++++++++------ 3 files changed, 109 insertions(+), 40 deletions(-) -- 2.14.1 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 0/3] dmaengine: Fix DMA on current allwinner SoCs, add A64 support @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-arm-kernel The latest generation of allwinner SoCs (e.g. H3, H5, A64) have some significant differences to older SoC generations (e.g. A23): - different register location for the clock autogating register (offset 0x28 vs 0x20) - different field offset for the src/dest burst length fields in the channel configuration register - additional burst lengths and widths This is the second change to the implementation of the DMA controller (first change being the addition of the clock autogating register in the A23). To differentiate between the different generations, the patch adds an enum value to the OF config data. The A83t uses the same register layout as the A23, so this series also fixes the missing clock gating setting for the A83t. The A64 is register compatible to the H3, but supports less channels and has a different set of endpoints. Add a new compatible string to the device tree and the driver. Stefan Br?ns (3): dmaengine: sun6i: Correct DMA support on H3 arm64: allwinner: a64: Add device node for DMA controller dmaengine: sun6i: Add support for Allwinner A64 .../devicetree/bindings/dma/sun6i-dma.txt | 1 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 13 ++ drivers/dma/sun6i-dma.c | 135 +++++++++++++++------ 3 files changed, 109 insertions(+), 40 deletions(-) -- 2.14.1 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 0/3] dmaengine: Fix DMA on current allwinner SoCs, add A64 support @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring The latest generation of allwinner SoCs (e.g. H3, H5, A64) have some significant differences to older SoC generations (e.g. A23): - different register location for the clock autogating register (offset 0x28 vs 0x20) - different field offset for the src/dest burst length fields in the channel configuration register - additional burst lengths and widths This is the second change to the implementation of the DMA controller (first change being the addition of the clock autogating register in the A23). To differentiate between the different generations, the patch adds an enum value to the OF config data. The A83t uses the same register layout as the A23, so this series also fixes the missing clock gating setting for the A83t. The A64 is register compatible to the H3, but supports less channels and has a different set of endpoints. Add a new compatible string to the device tree and the driver. Stefan Brüns (3): dmaengine: sun6i: Correct DMA support on H3 arm64: allwinner: a64: Add device node for DMA controller dmaengine: sun6i: Add support for Allwinner A64 .../devicetree/bindings/dma/sun6i-dma.txt | 1 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 13 ++ drivers/dma/sun6i-dma.c | 135 +++++++++++++++------ 3 files changed, 109 insertions(+), 40 deletions(-) -- 2.14.1 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 2017-08-30 23:36 ` Stefan Brüns (?) @ 2017-08-30 23:36 ` Stefan Brüns -1 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring H3 (and A64/H5) have a sligthly different DMA controller compared with older SoC generations: - it supports a buswidth of 8 bytes - it supports burst length of 4 and 16 transfers - the register offset for the burst lengths are different, it uses bits [6:7]/[22:23] instead of [7:8]/[23:24] for the src/dest lengths. Set the src_addr_widths/dest_addr_widths fields in struct dma_device according to the supported widths and use these for verification of the slave configuration. As struct dma_device has no detailed information for supported burst lengths (only maxburst), the information is added to the config. Separating verification of the config and conversion to register values allows to keep both independent of the used controller. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/dma/sun6i-dma.c | 128 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 40 deletions(-) diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index a2358780ab2c..5f4eee4513e5 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -48,6 +48,9 @@ #define SUN8I_DMA_GATE 0x20 #define SUN8I_DMA_GATE_ENABLE 0x4 +#define SUNXI_H3_SECURITE_REG 0x20 +#define SUNXI_H3_DMA_GATE 0x28 +#define SUNXI_H3_DMA_GATE_ENABLE 0x4 /* * Channels specific registers */ @@ -65,13 +68,15 @@ #define DMA_CHAN_CFG_SRC_DRQ(x) ((x) & 0x1f) #define DMA_CHAN_CFG_SRC_IO_MODE BIT(5) #define DMA_CHAN_CFG_SRC_LINEAR_MODE (0 << 5) -#define DMA_CHAN_CFG_SRC_BURST(x) (((x) & 0x3) << 7) +#define DMA_CHAN_CFG_SRC_BURST_A31(x) (((x) & 0x3) << 7) +#define DMA_CHAN_CFG_SRC_BURST_H3(x) (((x) & 0x3) << 6) #define DMA_CHAN_CFG_SRC_WIDTH(x) (((x) & 0x3) << 9) #define DMA_CHAN_CFG_DST_DRQ(x) (DMA_CHAN_CFG_SRC_DRQ(x) << 16) #define DMA_CHAN_CFG_DST_IO_MODE (DMA_CHAN_CFG_SRC_IO_MODE << 16) #define DMA_CHAN_CFG_DST_LINEAR_MODE (DMA_CHAN_CFG_SRC_LINEAR_MODE << 16) -#define DMA_CHAN_CFG_DST_BURST(x) (DMA_CHAN_CFG_SRC_BURST(x) << 16) +#define DMA_CHAN_CFG_DST_BURST_A31(x) (DMA_CHAN_CFG_SRC_BURST_A31(x) << 16) +#define DMA_CHAN_CFG_DST_BURST_H3(x) (DMA_CHAN_CFG_SRC_BURST_H3(x) << 16) #define DMA_CHAN_CFG_DST_WIDTH(x) (DMA_CHAN_CFG_SRC_WIDTH(x) << 16) #define DMA_CHAN_CUR_SRC 0x10 @@ -90,6 +95,16 @@ #define NORMAL_WAIT 8 #define DRQ_SDRAM 1 +/* Between SoC generations, there are some significant differences: + * - A23 added a clock gate register + * - the H3 burst length field has a different offset + */ +enum dmac_variant { + DMAC_VARIANT_A31, + DMAC_VARIANT_A23, + DMAC_VARIANT_H3, +}; + /* * Hardware channels / ports representation * @@ -101,6 +116,7 @@ struct sun6i_dma_config { u32 nr_max_channels; u32 nr_max_requests; u32 nr_max_vchans; + enum dmac_variant dmac_variant; }; /* @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) switch (maxburst) { case 1: return 0; + case 4: + return 1; case 8: return 2; + case 16: + return 3; default: return -EINVAL; } @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) { - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) - return -EINVAL; - - return addr_width >> 1; + return ilog2(addr_width); } static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, enum dma_transfer_direction direction, u32 *p_cfg) { + enum dma_slave_buswidth src_addr_width, dst_addr_width; + u32 src_maxburst, dst_maxburst, supported_burst_length; s8 src_width, dst_width, src_burst, dst_burst; + src_addr_width = sconfig->src_addr_width; + dst_addr_width = sconfig->dst_addr_width; + src_maxburst = sconfig->src_maxburst; + dst_maxburst = sconfig->dst_maxburst; + + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); + else + supported_burst_length = BIT(1) | BIT(8); + switch (direction) { case DMA_MEM_TO_DEV: - src_burst = convert_burst(sconfig->src_maxburst ? - sconfig->src_maxburst : 8); - src_width = convert_buswidth(sconfig->src_addr_width != - DMA_SLAVE_BUSWIDTH_UNDEFINED ? - sconfig->src_addr_width : - DMA_SLAVE_BUSWIDTH_4_BYTES); - dst_burst = convert_burst(sconfig->dst_maxburst); - dst_width = convert_buswidth(sconfig->dst_addr_width); + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + src_maxburst = src_maxburst ? src_maxburst : 8; break; case DMA_DEV_TO_MEM: - src_burst = convert_burst(sconfig->src_maxburst); - src_width = convert_buswidth(sconfig->src_addr_width); - dst_burst = convert_burst(sconfig->dst_maxburst ? - sconfig->dst_maxburst : 8); - dst_width = convert_buswidth(sconfig->dst_addr_width != - DMA_SLAVE_BUSWIDTH_UNDEFINED ? - sconfig->dst_addr_width : - DMA_SLAVE_BUSWIDTH_4_BYTES); + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + dst_maxburst = dst_maxburst ? dst_maxburst : 8; break; default: return -EINVAL; } - if (src_burst < 0) - return src_burst; - if (src_width < 0) - return src_width; - if (dst_burst < 0) - return dst_burst; - if (dst_width < 0) - return dst_width; - - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | - DMA_CHAN_CFG_SRC_WIDTH(src_width) | - DMA_CHAN_CFG_DST_BURST(dst_burst) | + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) + return -EINVAL; + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) + return -EINVAL; + if (!(BIT(src_maxburst) & supported_burst_length)) + return -EINVAL; + if (!(BIT(dst_maxburst) & supported_burst_length)) + return -EINVAL; + + src_width = convert_buswidth(src_addr_width); + dst_width = convert_buswidth(dst_addr_width); + dst_burst = convert_burst(dst_maxburst); + src_burst = convert_burst(src_maxburst); + + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) { + *p_cfg = DMA_CHAN_CFG_SRC_BURST_H3(src_burst) | + DMA_CHAN_CFG_DST_BURST_H3(dst_burst); + } else { + *p_cfg = DMA_CHAN_CFG_SRC_BURST_A31(src_burst) | + DMA_CHAN_CFG_DST_BURST_A31(dst_burst); + } + + *p_cfg |= DMA_CHAN_CFG_SRC_WIDTH(src_width) | DMA_CHAN_CFG_DST_WIDTH(dst_width); return 0; @@ -582,11 +611,17 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_memcpy( DMA_CHAN_CFG_DST_DRQ(DRQ_SDRAM) | DMA_CHAN_CFG_DST_LINEAR_MODE | DMA_CHAN_CFG_SRC_LINEAR_MODE | - DMA_CHAN_CFG_SRC_BURST(burst) | DMA_CHAN_CFG_SRC_WIDTH(width) | - DMA_CHAN_CFG_DST_BURST(burst) | DMA_CHAN_CFG_DST_WIDTH(width); + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) { + v_lli->cfg |= DMA_CHAN_CFG_SRC_BURST_H3(burst) | + DMA_CHAN_CFG_DST_BURST_H3(burst); + } else { + v_lli->cfg |= DMA_CHAN_CFG_SRC_BURST_A31(burst) | + DMA_CHAN_CFG_DST_BURST_A31(burst); + } + sun6i_dma_lli_add(NULL, v_lli, p_lli, txd); sun6i_dma_dump_lli(vchan, v_lli); @@ -998,6 +1033,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = { .nr_max_channels = 16, .nr_max_requests = 30, .nr_max_vchans = 53, + .dmac_variant = DMAC_VARIANT_A31, }; /* @@ -1009,23 +1045,29 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = { .nr_max_channels = 8, .nr_max_requests = 24, .nr_max_vchans = 37, + .dmac_variant = DMAC_VARIANT_A23, }; static struct sun6i_dma_config sun8i_a83t_dma_cfg = { .nr_max_channels = 8, .nr_max_requests = 28, .nr_max_vchans = 39, + .dmac_variant = DMAC_VARIANT_A23, }; /* * The H3 has 12 physical channels, a maximum DRQ port id of 27, * and a total of 34 usable source and destination endpoints. + * It also supports additional burst lengths and bus widths, + * and the burst length fields have different offsets. */ static struct sun6i_dma_config sun8i_h3_dma_cfg = { .nr_max_channels = 12, .nr_max_requests = 27, .nr_max_vchans = 34, + .dmac_variant = DMAC_VARIANT_H3, +}; }; static const struct of_device_id sun6i_dma_match[] = { @@ -1110,6 +1152,10 @@ static int sun6i_dma_probe(struct platform_device *pdev) sdc->slave.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | BIT(DMA_SLAVE_BUSWIDTH_4_BYTES); + if (sdc->cfg->dmac_variant == DMAC_VARIANT_H3) { + sdc->slave.src_addr_widths |= BIT(DMA_SLAVE_BUSWIDTH_8_BYTES); + sdc->slave.dst_addr_widths |= BIT(DMA_SLAVE_BUSWIDTH_8_BYTES); + } sdc->slave.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV); sdc->slave.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; @@ -1177,10 +1223,12 @@ static int sun6i_dma_probe(struct platform_device *pdev) /* * sun8i variant requires us to toggle a dma gating register, * as seen in Allwinner's SDK. This register is not documented - * in the A23 user manual. + * in the A23 user manual, but appears in e.g. the H83T manual. + * For the H3, H5 and A64, the register has a different location */ - if (of_device_is_compatible(pdev->dev.of_node, - "allwinner,sun8i-a23-dma")) + if (sdc->cfg->dmac_variant == DMAC_VARIANT_H3) + writel(SUNXI_H3_DMA_GATE_ENABLE, sdc->base + SUNXI_H3_DMA_GATE); + else if (sdc->cfg->dmac_variant == DMAC_VARIANT_A23) writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); return 0; -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-arm-kernel H3 (and A64/H5) have a sligthly different DMA controller compared with older SoC generations: - it supports a buswidth of 8 bytes - it supports burst length of 4 and 16 transfers - the register offset for the burst lengths are different, it uses bits [6:7]/[22:23] instead of [7:8]/[23:24] for the src/dest lengths. Set the src_addr_widths/dest_addr_widths fields in struct dma_device according to the supported widths and use these for verification of the slave configuration. As struct dma_device has no detailed information for supported burst lengths (only maxburst), the information is added to the config. Separating verification of the config and conversion to register values allows to keep both independent of the used controller. Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de> --- drivers/dma/sun6i-dma.c | 128 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 40 deletions(-) diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index a2358780ab2c..5f4eee4513e5 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -48,6 +48,9 @@ #define SUN8I_DMA_GATE 0x20 #define SUN8I_DMA_GATE_ENABLE 0x4 +#define SUNXI_H3_SECURITE_REG 0x20 +#define SUNXI_H3_DMA_GATE 0x28 +#define SUNXI_H3_DMA_GATE_ENABLE 0x4 /* * Channels specific registers */ @@ -65,13 +68,15 @@ #define DMA_CHAN_CFG_SRC_DRQ(x) ((x) & 0x1f) #define DMA_CHAN_CFG_SRC_IO_MODE BIT(5) #define DMA_CHAN_CFG_SRC_LINEAR_MODE (0 << 5) -#define DMA_CHAN_CFG_SRC_BURST(x) (((x) & 0x3) << 7) +#define DMA_CHAN_CFG_SRC_BURST_A31(x) (((x) & 0x3) << 7) +#define DMA_CHAN_CFG_SRC_BURST_H3(x) (((x) & 0x3) << 6) #define DMA_CHAN_CFG_SRC_WIDTH(x) (((x) & 0x3) << 9) #define DMA_CHAN_CFG_DST_DRQ(x) (DMA_CHAN_CFG_SRC_DRQ(x) << 16) #define DMA_CHAN_CFG_DST_IO_MODE (DMA_CHAN_CFG_SRC_IO_MODE << 16) #define DMA_CHAN_CFG_DST_LINEAR_MODE (DMA_CHAN_CFG_SRC_LINEAR_MODE << 16) -#define DMA_CHAN_CFG_DST_BURST(x) (DMA_CHAN_CFG_SRC_BURST(x) << 16) +#define DMA_CHAN_CFG_DST_BURST_A31(x) (DMA_CHAN_CFG_SRC_BURST_A31(x) << 16) +#define DMA_CHAN_CFG_DST_BURST_H3(x) (DMA_CHAN_CFG_SRC_BURST_H3(x) << 16) #define DMA_CHAN_CFG_DST_WIDTH(x) (DMA_CHAN_CFG_SRC_WIDTH(x) << 16) #define DMA_CHAN_CUR_SRC 0x10 @@ -90,6 +95,16 @@ #define NORMAL_WAIT 8 #define DRQ_SDRAM 1 +/* Between SoC generations, there are some significant differences: + * - A23 added a clock gate register + * - the H3 burst length field has a different offset + */ +enum dmac_variant { + DMAC_VARIANT_A31, + DMAC_VARIANT_A23, + DMAC_VARIANT_H3, +}; + /* * Hardware channels / ports representation * @@ -101,6 +116,7 @@ struct sun6i_dma_config { u32 nr_max_channels; u32 nr_max_requests; u32 nr_max_vchans; + enum dmac_variant dmac_variant; }; /* @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) switch (maxburst) { case 1: return 0; + case 4: + return 1; case 8: return 2; + case 16: + return 3; default: return -EINVAL; } @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) { - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) - return -EINVAL; - - return addr_width >> 1; + return ilog2(addr_width); } static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, enum dma_transfer_direction direction, u32 *p_cfg) { + enum dma_slave_buswidth src_addr_width, dst_addr_width; + u32 src_maxburst, dst_maxburst, supported_burst_length; s8 src_width, dst_width, src_burst, dst_burst; + src_addr_width = sconfig->src_addr_width; + dst_addr_width = sconfig->dst_addr_width; + src_maxburst = sconfig->src_maxburst; + dst_maxburst = sconfig->dst_maxburst; + + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); + else + supported_burst_length = BIT(1) | BIT(8); + switch (direction) { case DMA_MEM_TO_DEV: - src_burst = convert_burst(sconfig->src_maxburst ? - sconfig->src_maxburst : 8); - src_width = convert_buswidth(sconfig->src_addr_width != - DMA_SLAVE_BUSWIDTH_UNDEFINED ? - sconfig->src_addr_width : - DMA_SLAVE_BUSWIDTH_4_BYTES); - dst_burst = convert_burst(sconfig->dst_maxburst); - dst_width = convert_buswidth(sconfig->dst_addr_width); + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + src_maxburst = src_maxburst ? src_maxburst : 8; break; case DMA_DEV_TO_MEM: - src_burst = convert_burst(sconfig->src_maxburst); - src_width = convert_buswidth(sconfig->src_addr_width); - dst_burst = convert_burst(sconfig->dst_maxburst ? - sconfig->dst_maxburst : 8); - dst_width = convert_buswidth(sconfig->dst_addr_width != - DMA_SLAVE_BUSWIDTH_UNDEFINED ? - sconfig->dst_addr_width : - DMA_SLAVE_BUSWIDTH_4_BYTES); + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + dst_maxburst = dst_maxburst ? dst_maxburst : 8; break; default: return -EINVAL; } - if (src_burst < 0) - return src_burst; - if (src_width < 0) - return src_width; - if (dst_burst < 0) - return dst_burst; - if (dst_width < 0) - return dst_width; - - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | - DMA_CHAN_CFG_SRC_WIDTH(src_width) | - DMA_CHAN_CFG_DST_BURST(dst_burst) | + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) + return -EINVAL; + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) + return -EINVAL; + if (!(BIT(src_maxburst) & supported_burst_length)) + return -EINVAL; + if (!(BIT(dst_maxburst) & supported_burst_length)) + return -EINVAL; + + src_width = convert_buswidth(src_addr_width); + dst_width = convert_buswidth(dst_addr_width); + dst_burst = convert_burst(dst_maxburst); + src_burst = convert_burst(src_maxburst); + + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) { + *p_cfg = DMA_CHAN_CFG_SRC_BURST_H3(src_burst) | + DMA_CHAN_CFG_DST_BURST_H3(dst_burst); + } else { + *p_cfg = DMA_CHAN_CFG_SRC_BURST_A31(src_burst) | + DMA_CHAN_CFG_DST_BURST_A31(dst_burst); + } + + *p_cfg |= DMA_CHAN_CFG_SRC_WIDTH(src_width) | DMA_CHAN_CFG_DST_WIDTH(dst_width); return 0; @@ -582,11 +611,17 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_memcpy( DMA_CHAN_CFG_DST_DRQ(DRQ_SDRAM) | DMA_CHAN_CFG_DST_LINEAR_MODE | DMA_CHAN_CFG_SRC_LINEAR_MODE | - DMA_CHAN_CFG_SRC_BURST(burst) | DMA_CHAN_CFG_SRC_WIDTH(width) | - DMA_CHAN_CFG_DST_BURST(burst) | DMA_CHAN_CFG_DST_WIDTH(width); + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) { + v_lli->cfg |= DMA_CHAN_CFG_SRC_BURST_H3(burst) | + DMA_CHAN_CFG_DST_BURST_H3(burst); + } else { + v_lli->cfg |= DMA_CHAN_CFG_SRC_BURST_A31(burst) | + DMA_CHAN_CFG_DST_BURST_A31(burst); + } + sun6i_dma_lli_add(NULL, v_lli, p_lli, txd); sun6i_dma_dump_lli(vchan, v_lli); @@ -998,6 +1033,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = { .nr_max_channels = 16, .nr_max_requests = 30, .nr_max_vchans = 53, + .dmac_variant = DMAC_VARIANT_A31, }; /* @@ -1009,23 +1045,29 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = { .nr_max_channels = 8, .nr_max_requests = 24, .nr_max_vchans = 37, + .dmac_variant = DMAC_VARIANT_A23, }; static struct sun6i_dma_config sun8i_a83t_dma_cfg = { .nr_max_channels = 8, .nr_max_requests = 28, .nr_max_vchans = 39, + .dmac_variant = DMAC_VARIANT_A23, }; /* * The H3 has 12 physical channels, a maximum DRQ port id of 27, * and a total of 34 usable source and destination endpoints. + * It also supports additional burst lengths and bus widths, + * and the burst length fields have different offsets. */ static struct sun6i_dma_config sun8i_h3_dma_cfg = { .nr_max_channels = 12, .nr_max_requests = 27, .nr_max_vchans = 34, + .dmac_variant = DMAC_VARIANT_H3, +}; }; static const struct of_device_id sun6i_dma_match[] = { @@ -1110,6 +1152,10 @@ static int sun6i_dma_probe(struct platform_device *pdev) sdc->slave.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | BIT(DMA_SLAVE_BUSWIDTH_4_BYTES); + if (sdc->cfg->dmac_variant == DMAC_VARIANT_H3) { + sdc->slave.src_addr_widths |= BIT(DMA_SLAVE_BUSWIDTH_8_BYTES); + sdc->slave.dst_addr_widths |= BIT(DMA_SLAVE_BUSWIDTH_8_BYTES); + } sdc->slave.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV); sdc->slave.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; @@ -1177,10 +1223,12 @@ static int sun6i_dma_probe(struct platform_device *pdev) /* * sun8i variant requires us to toggle a dma gating register, * as seen in Allwinner's SDK. This register is not documented - * in the A23 user manual. + * in the A23 user manual, but appears in e.g. the H83T manual. + * For the H3, H5 and A64, the register has a different location */ - if (of_device_is_compatible(pdev->dev.of_node, - "allwinner,sun8i-a23-dma")) + if (sdc->cfg->dmac_variant == DMAC_VARIANT_H3) + writel(SUNXI_H3_DMA_GATE_ENABLE, sdc->base + SUNXI_H3_DMA_GATE); + else if (sdc->cfg->dmac_variant == DMAC_VARIANT_A23) writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); return 0; -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring H3 (and A64/H5) have a sligthly different DMA controller compared with older SoC generations: - it supports a buswidth of 8 bytes - it supports burst length of 4 and 16 transfers - the register offset for the burst lengths are different, it uses bits [6:7]/[22:23] instead of [7:8]/[23:24] for the src/dest lengths. Set the src_addr_widths/dest_addr_widths fields in struct dma_device according to the supported widths and use these for verification of the slave configuration. As struct dma_device has no detailed information for supported burst lengths (only maxburst), the information is added to the config. Separating verification of the config and conversion to register values allows to keep both independent of the used controller. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/dma/sun6i-dma.c | 128 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 40 deletions(-) diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index a2358780ab2c..5f4eee4513e5 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -48,6 +48,9 @@ #define SUN8I_DMA_GATE 0x20 #define SUN8I_DMA_GATE_ENABLE 0x4 +#define SUNXI_H3_SECURITE_REG 0x20 +#define SUNXI_H3_DMA_GATE 0x28 +#define SUNXI_H3_DMA_GATE_ENABLE 0x4 /* * Channels specific registers */ @@ -65,13 +68,15 @@ #define DMA_CHAN_CFG_SRC_DRQ(x) ((x) & 0x1f) #define DMA_CHAN_CFG_SRC_IO_MODE BIT(5) #define DMA_CHAN_CFG_SRC_LINEAR_MODE (0 << 5) -#define DMA_CHAN_CFG_SRC_BURST(x) (((x) & 0x3) << 7) +#define DMA_CHAN_CFG_SRC_BURST_A31(x) (((x) & 0x3) << 7) +#define DMA_CHAN_CFG_SRC_BURST_H3(x) (((x) & 0x3) << 6) #define DMA_CHAN_CFG_SRC_WIDTH(x) (((x) & 0x3) << 9) #define DMA_CHAN_CFG_DST_DRQ(x) (DMA_CHAN_CFG_SRC_DRQ(x) << 16) #define DMA_CHAN_CFG_DST_IO_MODE (DMA_CHAN_CFG_SRC_IO_MODE << 16) #define DMA_CHAN_CFG_DST_LINEAR_MODE (DMA_CHAN_CFG_SRC_LINEAR_MODE << 16) -#define DMA_CHAN_CFG_DST_BURST(x) (DMA_CHAN_CFG_SRC_BURST(x) << 16) +#define DMA_CHAN_CFG_DST_BURST_A31(x) (DMA_CHAN_CFG_SRC_BURST_A31(x) << 16) +#define DMA_CHAN_CFG_DST_BURST_H3(x) (DMA_CHAN_CFG_SRC_BURST_H3(x) << 16) #define DMA_CHAN_CFG_DST_WIDTH(x) (DMA_CHAN_CFG_SRC_WIDTH(x) << 16) #define DMA_CHAN_CUR_SRC 0x10 @@ -90,6 +95,16 @@ #define NORMAL_WAIT 8 #define DRQ_SDRAM 1 +/* Between SoC generations, there are some significant differences: + * - A23 added a clock gate register + * - the H3 burst length field has a different offset + */ +enum dmac_variant { + DMAC_VARIANT_A31, + DMAC_VARIANT_A23, + DMAC_VARIANT_H3, +}; + /* * Hardware channels / ports representation * @@ -101,6 +116,7 @@ struct sun6i_dma_config { u32 nr_max_channels; u32 nr_max_requests; u32 nr_max_vchans; + enum dmac_variant dmac_variant; }; /* @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) switch (maxburst) { case 1: return 0; + case 4: + return 1; case 8: return 2; + case 16: + return 3; default: return -EINVAL; } @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) { - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) - return -EINVAL; - - return addr_width >> 1; + return ilog2(addr_width); } static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, enum dma_transfer_direction direction, u32 *p_cfg) { + enum dma_slave_buswidth src_addr_width, dst_addr_width; + u32 src_maxburst, dst_maxburst, supported_burst_length; s8 src_width, dst_width, src_burst, dst_burst; + src_addr_width = sconfig->src_addr_width; + dst_addr_width = sconfig->dst_addr_width; + src_maxburst = sconfig->src_maxburst; + dst_maxburst = sconfig->dst_maxburst; + + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); + else + supported_burst_length = BIT(1) | BIT(8); + switch (direction) { case DMA_MEM_TO_DEV: - src_burst = convert_burst(sconfig->src_maxburst ? - sconfig->src_maxburst : 8); - src_width = convert_buswidth(sconfig->src_addr_width != - DMA_SLAVE_BUSWIDTH_UNDEFINED ? - sconfig->src_addr_width : - DMA_SLAVE_BUSWIDTH_4_BYTES); - dst_burst = convert_burst(sconfig->dst_maxburst); - dst_width = convert_buswidth(sconfig->dst_addr_width); + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + src_maxburst = src_maxburst ? src_maxburst : 8; break; case DMA_DEV_TO_MEM: - src_burst = convert_burst(sconfig->src_maxburst); - src_width = convert_buswidth(sconfig->src_addr_width); - dst_burst = convert_burst(sconfig->dst_maxburst ? - sconfig->dst_maxburst : 8); - dst_width = convert_buswidth(sconfig->dst_addr_width != - DMA_SLAVE_BUSWIDTH_UNDEFINED ? - sconfig->dst_addr_width : - DMA_SLAVE_BUSWIDTH_4_BYTES); + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + dst_maxburst = dst_maxburst ? dst_maxburst : 8; break; default: return -EINVAL; } - if (src_burst < 0) - return src_burst; - if (src_width < 0) - return src_width; - if (dst_burst < 0) - return dst_burst; - if (dst_width < 0) - return dst_width; - - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | - DMA_CHAN_CFG_SRC_WIDTH(src_width) | - DMA_CHAN_CFG_DST_BURST(dst_burst) | + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) + return -EINVAL; + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) + return -EINVAL; + if (!(BIT(src_maxburst) & supported_burst_length)) + return -EINVAL; + if (!(BIT(dst_maxburst) & supported_burst_length)) + return -EINVAL; + + src_width = convert_buswidth(src_addr_width); + dst_width = convert_buswidth(dst_addr_width); + dst_burst = convert_burst(dst_maxburst); + src_burst = convert_burst(src_maxburst); + + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) { + *p_cfg = DMA_CHAN_CFG_SRC_BURST_H3(src_burst) | + DMA_CHAN_CFG_DST_BURST_H3(dst_burst); + } else { + *p_cfg = DMA_CHAN_CFG_SRC_BURST_A31(src_burst) | + DMA_CHAN_CFG_DST_BURST_A31(dst_burst); + } + + *p_cfg |= DMA_CHAN_CFG_SRC_WIDTH(src_width) | DMA_CHAN_CFG_DST_WIDTH(dst_width); return 0; @@ -582,11 +611,17 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_memcpy( DMA_CHAN_CFG_DST_DRQ(DRQ_SDRAM) | DMA_CHAN_CFG_DST_LINEAR_MODE | DMA_CHAN_CFG_SRC_LINEAR_MODE | - DMA_CHAN_CFG_SRC_BURST(burst) | DMA_CHAN_CFG_SRC_WIDTH(width) | - DMA_CHAN_CFG_DST_BURST(burst) | DMA_CHAN_CFG_DST_WIDTH(width); + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) { + v_lli->cfg |= DMA_CHAN_CFG_SRC_BURST_H3(burst) | + DMA_CHAN_CFG_DST_BURST_H3(burst); + } else { + v_lli->cfg |= DMA_CHAN_CFG_SRC_BURST_A31(burst) | + DMA_CHAN_CFG_DST_BURST_A31(burst); + } + sun6i_dma_lli_add(NULL, v_lli, p_lli, txd); sun6i_dma_dump_lli(vchan, v_lli); @@ -998,6 +1033,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = { .nr_max_channels = 16, .nr_max_requests = 30, .nr_max_vchans = 53, + .dmac_variant = DMAC_VARIANT_A31, }; /* @@ -1009,23 +1045,29 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = { .nr_max_channels = 8, .nr_max_requests = 24, .nr_max_vchans = 37, + .dmac_variant = DMAC_VARIANT_A23, }; static struct sun6i_dma_config sun8i_a83t_dma_cfg = { .nr_max_channels = 8, .nr_max_requests = 28, .nr_max_vchans = 39, + .dmac_variant = DMAC_VARIANT_A23, }; /* * The H3 has 12 physical channels, a maximum DRQ port id of 27, * and a total of 34 usable source and destination endpoints. + * It also supports additional burst lengths and bus widths, + * and the burst length fields have different offsets. */ static struct sun6i_dma_config sun8i_h3_dma_cfg = { .nr_max_channels = 12, .nr_max_requests = 27, .nr_max_vchans = 34, + .dmac_variant = DMAC_VARIANT_H3, +}; }; static const struct of_device_id sun6i_dma_match[] = { @@ -1110,6 +1152,10 @@ static int sun6i_dma_probe(struct platform_device *pdev) sdc->slave.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | BIT(DMA_SLAVE_BUSWIDTH_4_BYTES); + if (sdc->cfg->dmac_variant == DMAC_VARIANT_H3) { + sdc->slave.src_addr_widths |= BIT(DMA_SLAVE_BUSWIDTH_8_BYTES); + sdc->slave.dst_addr_widths |= BIT(DMA_SLAVE_BUSWIDTH_8_BYTES); + } sdc->slave.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV); sdc->slave.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; @@ -1177,10 +1223,12 @@ static int sun6i_dma_probe(struct platform_device *pdev) /* * sun8i variant requires us to toggle a dma gating register, * as seen in Allwinner's SDK. This register is not documented - * in the A23 user manual. + * in the A23 user manual, but appears in e.g. the H83T manual. + * For the H3, H5 and A64, the register has a different location */ - if (of_device_is_compatible(pdev->dev.of_node, - "allwinner,sun8i-a23-dma")) + if (sdc->cfg->dmac_variant == DMAC_VARIANT_H3) + writel(SUNXI_H3_DMA_GATE_ENABLE, sdc->base + SUNXI_H3_DMA_GATE); + else if (sdc->cfg->dmac_variant == DMAC_VARIANT_A23) writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE); return 0; -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 2017-08-30 23:36 ` Stefan Brüns @ 2017-08-31 14:51 ` Maxime Ripard -1 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-08-31 14:51 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi, devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring [-- Attachment #1: Type: text/plain, Size: 4634 bytes --] Hi, On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > +/* Between SoC generations, there are some significant differences: > + * - A23 added a clock gate register > + * - the H3 burst length field has a different offset > + */ This is not the proper comment style. > +enum dmac_variant { > + DMAC_VARIANT_A31, > + DMAC_VARIANT_A23, > + DMAC_VARIANT_H3, > +}; > + And this is redundant with what we already have in our structures. > /* > * Hardware channels / ports representation > * > @@ -101,6 +116,7 @@ struct sun6i_dma_config { > u32 nr_max_channels; > u32 nr_max_requests; > u32 nr_max_vchans; > + enum dmac_variant dmac_variant; > }; > > /* > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) > switch (maxburst) { > case 1: > return 0; > + case 4: > + return 1; > case 8: > return 2; > + case 16: > + return 3; > default: > return -EINVAL; > } > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) > > static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) > { > - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || > - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) > - return -EINVAL; > - > - return addr_width >> 1; > + return ilog2(addr_width); > } This isn't really the same operation. There should be some explanation about why it's the right thing to do. > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, > enum dma_transfer_direction direction, > u32 *p_cfg) > { > + enum dma_slave_buswidth src_addr_width, dst_addr_width; > + u32 src_maxburst, dst_maxburst, supported_burst_length; > s8 src_width, dst_width, src_burst, dst_burst; > > + src_addr_width = sconfig->src_addr_width; > + dst_addr_width = sconfig->dst_addr_width; > + src_maxburst = sconfig->src_maxburst; > + dst_maxburst = sconfig->dst_maxburst; > + > + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) > + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); > + else > + supported_burst_length = BIT(1) | BIT(8); This could be stored in the structure for example. > switch (direction) { > case DMA_MEM_TO_DEV: > - src_burst = convert_burst(sconfig->src_maxburst ? > - sconfig->src_maxburst : 8); > - src_width = convert_buswidth(sconfig->src_addr_width != > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > - sconfig->src_addr_width : > - DMA_SLAVE_BUSWIDTH_4_BYTES); > - dst_burst = convert_burst(sconfig->dst_maxburst); > - dst_width = convert_buswidth(sconfig->dst_addr_width); > + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + src_maxburst = src_maxburst ? src_maxburst : 8; > break; > case DMA_DEV_TO_MEM: > - src_burst = convert_burst(sconfig->src_maxburst); > - src_width = convert_buswidth(sconfig->src_addr_width); > - dst_burst = convert_burst(sconfig->dst_maxburst ? > - sconfig->dst_maxburst : 8); > - dst_width = convert_buswidth(sconfig->dst_addr_width != > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > - sconfig->dst_addr_width : > - DMA_SLAVE_BUSWIDTH_4_BYTES); > + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + dst_maxburst = dst_maxburst ? dst_maxburst : 8; > break; > default: > return -EINVAL; > } > > - if (src_burst < 0) > - return src_burst; > - if (src_width < 0) > - return src_width; > - if (dst_burst < 0) > - return dst_burst; > - if (dst_width < 0) > - return dst_width; > - > - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | > - DMA_CHAN_CFG_SRC_WIDTH(src_width) | > - DMA_CHAN_CFG_DST_BURST(dst_burst) | > + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) > + return -EINVAL; > + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) > + return -EINVAL; > + if (!(BIT(src_maxburst) & supported_burst_length)) > + return -EINVAL; > + if (!(BIT(dst_maxburst) & supported_burst_length)) > + return -EINVAL; > + > + src_width = convert_buswidth(src_addr_width); > + dst_width = convert_buswidth(dst_addr_width); > + dst_burst = convert_burst(dst_maxburst); > + src_burst = convert_burst(src_maxburst); I'm not sure what you're trying to do here. Could you split your patch by logical change, this doesn't seem related to just supporting the H3, but a heavier refactoring. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-08-31 14:51 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-08-31 14:51 UTC (permalink / raw) To: linux-arm-kernel Hi, On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Br?ns wrote: > +/* Between SoC generations, there are some significant differences: > + * - A23 added a clock gate register > + * - the H3 burst length field has a different offset > + */ This is not the proper comment style. > +enum dmac_variant { > + DMAC_VARIANT_A31, > + DMAC_VARIANT_A23, > + DMAC_VARIANT_H3, > +}; > + And this is redundant with what we already have in our structures. > /* > * Hardware channels / ports representation > * > @@ -101,6 +116,7 @@ struct sun6i_dma_config { > u32 nr_max_channels; > u32 nr_max_requests; > u32 nr_max_vchans; > + enum dmac_variant dmac_variant; > }; > > /* > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) > switch (maxburst) { > case 1: > return 0; > + case 4: > + return 1; > case 8: > return 2; > + case 16: > + return 3; > default: > return -EINVAL; > } > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) > > static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) > { > - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || > - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) > - return -EINVAL; > - > - return addr_width >> 1; > + return ilog2(addr_width); > } This isn't really the same operation. There should be some explanation about why it's the right thing to do. > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, > enum dma_transfer_direction direction, > u32 *p_cfg) > { > + enum dma_slave_buswidth src_addr_width, dst_addr_width; > + u32 src_maxburst, dst_maxburst, supported_burst_length; > s8 src_width, dst_width, src_burst, dst_burst; > > + src_addr_width = sconfig->src_addr_width; > + dst_addr_width = sconfig->dst_addr_width; > + src_maxburst = sconfig->src_maxburst; > + dst_maxburst = sconfig->dst_maxburst; > + > + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) > + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); > + else > + supported_burst_length = BIT(1) | BIT(8); This could be stored in the structure for example. > switch (direction) { > case DMA_MEM_TO_DEV: > - src_burst = convert_burst(sconfig->src_maxburst ? > - sconfig->src_maxburst : 8); > - src_width = convert_buswidth(sconfig->src_addr_width != > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > - sconfig->src_addr_width : > - DMA_SLAVE_BUSWIDTH_4_BYTES); > - dst_burst = convert_burst(sconfig->dst_maxburst); > - dst_width = convert_buswidth(sconfig->dst_addr_width); > + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + src_maxburst = src_maxburst ? src_maxburst : 8; > break; > case DMA_DEV_TO_MEM: > - src_burst = convert_burst(sconfig->src_maxburst); > - src_width = convert_buswidth(sconfig->src_addr_width); > - dst_burst = convert_burst(sconfig->dst_maxburst ? > - sconfig->dst_maxburst : 8); > - dst_width = convert_buswidth(sconfig->dst_addr_width != > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > - sconfig->dst_addr_width : > - DMA_SLAVE_BUSWIDTH_4_BYTES); > + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > + dst_maxburst = dst_maxburst ? dst_maxburst : 8; > break; > default: > return -EINVAL; > } > > - if (src_burst < 0) > - return src_burst; > - if (src_width < 0) > - return src_width; > - if (dst_burst < 0) > - return dst_burst; > - if (dst_width < 0) > - return dst_width; > - > - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | > - DMA_CHAN_CFG_SRC_WIDTH(src_width) | > - DMA_CHAN_CFG_DST_BURST(dst_burst) | > + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) > + return -EINVAL; > + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) > + return -EINVAL; > + if (!(BIT(src_maxburst) & supported_burst_length)) > + return -EINVAL; > + if (!(BIT(dst_maxburst) & supported_burst_length)) > + return -EINVAL; > + > + src_width = convert_buswidth(src_addr_width); > + dst_width = convert_buswidth(dst_addr_width); > + dst_burst = convert_burst(dst_maxburst); > + src_burst = convert_burst(src_maxburst); I'm not sure what you're trying to do here. Could you split your patch by logical change, this doesn't seem related to just supporting the H3, but a heavier refactoring. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170831/4a467528/attachment.sig> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-01 3:04 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-01 3:04 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi, devicetree, dmaengine, vinod.koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > Hi, > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > +/* Between SoC generations, there are some significant differences: > > + * - A23 added a clock gate register > > + * - the H3 burst length field has a different offset > > + */ > > This is not the proper comment style. > > > +enum dmac_variant { > > + DMAC_VARIANT_A31, > > + DMAC_VARIANT_A23, > > + DMAC_VARIANT_H3, > > +}; > > + > > And this is redundant with what we already have in our structures. Actually, its not. For H3, there are currently at least 3 register compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the current config structure is kept, we need 3 different compatible strings. Same for the A23, which is register compatible to e.g. A83t and V3s, but with different numbers of DMA channels. So either you decorate the code with a cascade of if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) { } else { /* A31 */ } in a number of places, or you do it just once. > > > /* > > > > * Hardware channels / ports representation > > * > > > > @@ -101,6 +116,7 @@ struct sun6i_dma_config { > > > > u32 nr_max_channels; > > u32 nr_max_requests; > > u32 nr_max_vchans; > > > > + enum dmac_variant dmac_variant; > > > > }; > > > > /* > > > > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) > > > > switch (maxburst) { > > > > case 1: > > return 0; > > > > + case 4: > > + return 1; > > > > case 8: > > return 2; > > > > + case 16: > > + return 3; > > > > default: > > return -EINVAL; > > > > } > > > > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) > > > > static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) > > { > > > > - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || > > - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) > > - return -EINVAL; > > - > > - return addr_width >> 1; > > + return ilog2(addr_width); > > > > } > > This isn't really the same operation. There should be some explanation > about why it's the right thing to do. For 1, 2 and 4 it is the same. The correct register value for 8 bytes, supported by H3, H5, A64 and R40, is 3. > > > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > > > > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, > > > > enum dma_transfer_direction direction, > > u32 *p_cfg) > > > > { > > > > + enum dma_slave_buswidth src_addr_width, dst_addr_width; > > + u32 src_maxburst, dst_maxburst, supported_burst_length; > > > > s8 src_width, dst_width, src_burst, dst_burst; > > > > + src_addr_width = sconfig->src_addr_width; > > + dst_addr_width = sconfig->dst_addr_width; > > + src_maxburst = sconfig->src_maxburst; > > + dst_maxburst = sconfig->dst_maxburst; > > + > > + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) > > + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); > > + else > > + supported_burst_length = BIT(1) | BIT(8); > > This could be stored in the structure for example. Which one? sun6i_dma_dev? sun6i_dma_config? > > switch (direction) { > > > > case DMA_MEM_TO_DEV: > > - src_burst = convert_burst(sconfig->src_maxburst ? > > - sconfig->src_maxburst : 8); > > - src_width = convert_buswidth(sconfig->src_addr_width != > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > - sconfig->src_addr_width : > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > - dst_burst = convert_burst(sconfig->dst_maxburst); > > - dst_width = convert_buswidth(sconfig->dst_addr_width); > > + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + src_maxburst = src_maxburst ? src_maxburst : 8; > > > > break; > > > > case DMA_DEV_TO_MEM: > > - src_burst = convert_burst(sconfig->src_maxburst); > > - src_width = convert_buswidth(sconfig->src_addr_width); > > - dst_burst = convert_burst(sconfig->dst_maxburst ? > > - sconfig->dst_maxburst : 8); > > - dst_width = convert_buswidth(sconfig->dst_addr_width != > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > - sconfig->dst_addr_width : > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + dst_maxburst = dst_maxburst ? dst_maxburst : 8; > > > > break; > > > > default: > > return -EINVAL; > > > > } > > > > - if (src_burst < 0) > > - return src_burst; > > - if (src_width < 0) > > - return src_width; > > - if (dst_burst < 0) > > - return dst_burst; > > - if (dst_width < 0) > > - return dst_width; > > - > > - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | > > - DMA_CHAN_CFG_SRC_WIDTH(src_width) | > > - DMA_CHAN_CFG_DST_BURST(dst_burst) | > > + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) > > + return -EINVAL; > > + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) > > + return -EINVAL; > > + if (!(BIT(src_maxburst) & supported_burst_length)) > > + return -EINVAL; > > + if (!(BIT(dst_maxburst) & supported_burst_length)) > > + return -EINVAL; > > + > > + src_width = convert_buswidth(src_addr_width); > > + dst_width = convert_buswidth(dst_addr_width); > > + dst_burst = convert_burst(dst_maxburst); > > + src_burst = convert_burst(src_maxburst); > > I'm not sure what you're trying to do here. Could you split your patch > by logical change, this doesn't seem related to just supporting the > H3, but a heavier refactoring. Untangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, range checking, and conversion to register value. As the valid ranges depend on the controller, the code is much easier to read if the range check is done first, and then the conversion. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-01 3:04 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-01 3:04 UTC (permalink / raw) To: linux-arm-kernel On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > Hi, > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Br?ns wrote: > > +/* Between SoC generations, there are some significant differences: > > + * - A23 added a clock gate register > > + * - the H3 burst length field has a different offset > > + */ > > This is not the proper comment style. > > > +enum dmac_variant { > > + DMAC_VARIANT_A31, > > + DMAC_VARIANT_A23, > > + DMAC_VARIANT_H3, > > +}; > > + > > And this is redundant with what we already have in our structures. Actually, its not. For H3, there are currently at least 3 register compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the current config structure is kept, we need 3 different compatible strings. Same for the A23, which is register compatible to e.g. A83t and V3s, but with different numbers of DMA channels. So either you decorate the code with a cascade of if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) { } else { /* A31 */ } in a number of places, or you do it just once. > > > /* > > > > * Hardware channels / ports representation > > * > > > > @@ -101,6 +116,7 @@ struct sun6i_dma_config { > > > > u32 nr_max_channels; > > u32 nr_max_requests; > > u32 nr_max_vchans; > > > > + enum dmac_variant dmac_variant; > > > > }; > > > > /* > > > > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) > > > > switch (maxburst) { > > > > case 1: > > return 0; > > > > + case 4: > > + return 1; > > > > case 8: > > return 2; > > > > + case 16: > > + return 3; > > > > default: > > return -EINVAL; > > > > } > > > > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) > > > > static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) > > { > > > > - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || > > - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) > > - return -EINVAL; > > - > > - return addr_width >> 1; > > + return ilog2(addr_width); > > > > } > > This isn't really the same operation. There should be some explanation > about why it's the right thing to do. For 1, 2 and 4 it is the same. The correct register value for 8 bytes, supported by H3, H5, A64 and R40, is 3. > > > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > > > > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, > > > > enum dma_transfer_direction direction, > > u32 *p_cfg) > > > > { > > > > + enum dma_slave_buswidth src_addr_width, dst_addr_width; > > + u32 src_maxburst, dst_maxburst, supported_burst_length; > > > > s8 src_width, dst_width, src_burst, dst_burst; > > > > + src_addr_width = sconfig->src_addr_width; > > + dst_addr_width = sconfig->dst_addr_width; > > + src_maxburst = sconfig->src_maxburst; > > + dst_maxburst = sconfig->dst_maxburst; > > + > > + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) > > + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); > > + else > > + supported_burst_length = BIT(1) | BIT(8); > > This could be stored in the structure for example. Which one? sun6i_dma_dev? sun6i_dma_config? > > switch (direction) { > > > > case DMA_MEM_TO_DEV: > > - src_burst = convert_burst(sconfig->src_maxburst ? > > - sconfig->src_maxburst : 8); > > - src_width = convert_buswidth(sconfig->src_addr_width != > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > - sconfig->src_addr_width : > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > - dst_burst = convert_burst(sconfig->dst_maxburst); > > - dst_width = convert_buswidth(sconfig->dst_addr_width); > > + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + src_maxburst = src_maxburst ? src_maxburst : 8; > > > > break; > > > > case DMA_DEV_TO_MEM: > > - src_burst = convert_burst(sconfig->src_maxburst); > > - src_width = convert_buswidth(sconfig->src_addr_width); > > - dst_burst = convert_burst(sconfig->dst_maxburst ? > > - sconfig->dst_maxburst : 8); > > - dst_width = convert_buswidth(sconfig->dst_addr_width != > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > - sconfig->dst_addr_width : > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + dst_maxburst = dst_maxburst ? dst_maxburst : 8; > > > > break; > > > > default: > > return -EINVAL; > > > > } > > > > - if (src_burst < 0) > > - return src_burst; > > - if (src_width < 0) > > - return src_width; > > - if (dst_burst < 0) > > - return dst_burst; > > - if (dst_width < 0) > > - return dst_width; > > - > > - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | > > - DMA_CHAN_CFG_SRC_WIDTH(src_width) | > > - DMA_CHAN_CFG_DST_BURST(dst_burst) | > > + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) > > + return -EINVAL; > > + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) > > + return -EINVAL; > > + if (!(BIT(src_maxburst) & supported_burst_length)) > > + return -EINVAL; > > + if (!(BIT(dst_maxburst) & supported_burst_length)) > > + return -EINVAL; > > + > > + src_width = convert_buswidth(src_addr_width); > > + dst_width = convert_buswidth(dst_addr_width); > > + dst_burst = convert_burst(dst_maxburst); > > + src_burst = convert_burst(src_maxburst); > > I'm not sure what you're trying to do here. Could you split your patch > by logical change, this doesn't seem related to just supporting the > H3, but a heavier refactoring. Untangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, range checking, and conversion to register value. As the valid ranges depend on the controller, the code is much easier to read if the range check is done first, and then the conversion. Kind regards, Stefan -- Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-01 3:04 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-01 3:04 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, vinod.koul-ral2JQCrhuEAvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chen-Yu Tsai, Rob Herring On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > Hi, > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > +/* Between SoC generations, there are some significant differences: > > + * - A23 added a clock gate register > > + * - the H3 burst length field has a different offset > > + */ > > This is not the proper comment style. > > > +enum dmac_variant { > > + DMAC_VARIANT_A31, > > + DMAC_VARIANT_A23, > > + DMAC_VARIANT_H3, > > +}; > > + > > And this is redundant with what we already have in our structures. Actually, its not. For H3, there are currently at least 3 register compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the current config structure is kept, we need 3 different compatible strings. Same for the A23, which is register compatible to e.g. A83t and V3s, but with different numbers of DMA channels. So either you decorate the code with a cascade of if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) { } else { /* A31 */ } in a number of places, or you do it just once. > > > /* > > > > * Hardware channels / ports representation > > * > > > > @@ -101,6 +116,7 @@ struct sun6i_dma_config { > > > > u32 nr_max_channels; > > u32 nr_max_requests; > > u32 nr_max_vchans; > > > > + enum dmac_variant dmac_variant; > > > > }; > > > > /* > > > > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) > > > > switch (maxburst) { > > > > case 1: > > return 0; > > > > + case 4: > > + return 1; > > > > case 8: > > return 2; > > > > + case 16: > > + return 3; > > > > default: > > return -EINVAL; > > > > } > > > > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) > > > > static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) > > { > > > > - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || > > - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) > > - return -EINVAL; > > - > > - return addr_width >> 1; > > + return ilog2(addr_width); > > > > } > > This isn't really the same operation. There should be some explanation > about why it's the right thing to do. For 1, 2 and 4 it is the same. The correct register value for 8 bytes, supported by H3, H5, A64 and R40, is 3. > > > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > > > > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, > > > > enum dma_transfer_direction direction, > > u32 *p_cfg) > > > > { > > > > + enum dma_slave_buswidth src_addr_width, dst_addr_width; > > + u32 src_maxburst, dst_maxburst, supported_burst_length; > > > > s8 src_width, dst_width, src_burst, dst_burst; > > > > + src_addr_width = sconfig->src_addr_width; > > + dst_addr_width = sconfig->dst_addr_width; > > + src_maxburst = sconfig->src_maxburst; > > + dst_maxburst = sconfig->dst_maxburst; > > + > > + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) > > + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); > > + else > > + supported_burst_length = BIT(1) | BIT(8); > > This could be stored in the structure for example. Which one? sun6i_dma_dev? sun6i_dma_config? > > switch (direction) { > > > > case DMA_MEM_TO_DEV: > > - src_burst = convert_burst(sconfig->src_maxburst ? > > - sconfig->src_maxburst : 8); > > - src_width = convert_buswidth(sconfig->src_addr_width != > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > - sconfig->src_addr_width : > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > - dst_burst = convert_burst(sconfig->dst_maxburst); > > - dst_width = convert_buswidth(sconfig->dst_addr_width); > > + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + src_maxburst = src_maxburst ? src_maxburst : 8; > > > > break; > > > > case DMA_DEV_TO_MEM: > > - src_burst = convert_burst(sconfig->src_maxburst); > > - src_width = convert_buswidth(sconfig->src_addr_width); > > - dst_burst = convert_burst(sconfig->dst_maxburst ? > > - sconfig->dst_maxburst : 8); > > - dst_width = convert_buswidth(sconfig->dst_addr_width != > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > - sconfig->dst_addr_width : > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > + dst_maxburst = dst_maxburst ? dst_maxburst : 8; > > > > break; > > > > default: > > return -EINVAL; > > > > } > > > > - if (src_burst < 0) > > - return src_burst; > > - if (src_width < 0) > > - return src_width; > > - if (dst_burst < 0) > > - return dst_burst; > > - if (dst_width < 0) > > - return dst_width; > > - > > - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | > > - DMA_CHAN_CFG_SRC_WIDTH(src_width) | > > - DMA_CHAN_CFG_DST_BURST(dst_burst) | > > + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) > > + return -EINVAL; > > + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) > > + return -EINVAL; > > + if (!(BIT(src_maxburst) & supported_burst_length)) > > + return -EINVAL; > > + if (!(BIT(dst_maxburst) & supported_burst_length)) > > + return -EINVAL; > > + > > + src_width = convert_buswidth(src_addr_width); > > + dst_width = convert_buswidth(dst_addr_width); > > + dst_burst = convert_burst(dst_maxburst); > > + src_burst = convert_burst(src_maxburst); > > I'm not sure what you're trying to do here. Could you split your patch > by logical change, this doesn't seem related to just supporting the > H3, but a heavier refactoring. Untangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, range checking, and conversion to register value. As the valid ranges depend on the controller, the code is much easier to read if the range check is done first, and then the conversion. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 2017-09-01 3:04 ` Stefan Bruens @ 2017-09-01 13:35 ` Maxime Ripard -1 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-01 13:35 UTC (permalink / raw) To: Stefan Bruens Cc: linux-sunxi, devicetree, dmaengine, vinod.koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring [-- Attachment #1: Type: text/plain, Size: 7151 bytes --] On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > Hi, > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > > +/* Between SoC generations, there are some significant differences: > > > + * - A23 added a clock gate register > > > + * - the H3 burst length field has a different offset > > > + */ > > > > This is not the proper comment style. > > > > > +enum dmac_variant { > > > + DMAC_VARIANT_A31, > > > + DMAC_VARIANT_A23, > > > + DMAC_VARIANT_H3, > > > +}; > > > + > > > > And this is redundant with what we already have in our structures. > > Actually, its not. For H3, there are currently at least 3 register compatible > SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the > current config structure is kept, we need 3 different compatible strings. Same > for the A23, which is register compatible to e.g. A83t and V3s, but with > different numbers of DMA channels. > > So either you decorate the code with a cascade of > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) { > } else { /* A31 */ > } > > in a number of places, or you do it just once. That's not how you retrieve the structures. They are already associated to the compatible, and you need to do a single lookup to get them. So that's nowhere near what you're suggesting. You can have a look at the of_match_device in the probe function. > > > > > > /* > > > > > > * Hardware channels / ports representation > > > * > > > > > > @@ -101,6 +116,7 @@ struct sun6i_dma_config { > > > > > > u32 nr_max_channels; > > > u32 nr_max_requests; > > > u32 nr_max_vchans; > > > > > > + enum dmac_variant dmac_variant; > > > > > > }; > > > > > > /* > > > > > > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) > > > > > > switch (maxburst) { > > > > > > case 1: > > > return 0; > > > > > > + case 4: > > > + return 1; > > > > > > case 8: > > > return 2; > > > > > > + case 16: > > > + return 3; > > > > > > default: > > > return -EINVAL; > > > > > > } > > > > > > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) > > > > > > static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) > > > { > > > > > > - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || > > > - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) > > > - return -EINVAL; > > > - > > > - return addr_width >> 1; > > > + return ilog2(addr_width); > > > > > > } > > > > This isn't really the same operation. There should be some explanation > > about why it's the right thing to do. > > For 1, 2 and 4 it is the same. The correct register value for 8 bytes, > supported by H3, H5, A64 and R40, is 3. That should be in a separate patch, with that in the commit log. > > > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > > > > > > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, > > > > > > enum dma_transfer_direction direction, > > > u32 *p_cfg) > > > > > > { > > > > > > + enum dma_slave_buswidth src_addr_width, dst_addr_width; > > > + u32 src_maxburst, dst_maxburst, supported_burst_length; > > > > > > s8 src_width, dst_width, src_burst, dst_burst; > > > > > > + src_addr_width = sconfig->src_addr_width; > > > + dst_addr_width = sconfig->dst_addr_width; > > > + src_maxburst = sconfig->src_maxburst; > > > + dst_maxburst = sconfig->dst_maxburst; > > > + > > > + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) > > > + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); > > > + else > > > + supported_burst_length = BIT(1) | BIT(8); > > > > This could be stored in the structure for example. > > Which one? sun6i_dma_dev? sun6i_dma_config? The one associated with the compatible, so sun6i_dma_config. > > > > switch (direction) { > > > > > > case DMA_MEM_TO_DEV: > > > - src_burst = convert_burst(sconfig->src_maxburst ? > > > - sconfig->src_maxburst : 8); > > > - src_width = convert_buswidth(sconfig->src_addr_width != > > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > > - sconfig->src_addr_width : > > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > > - dst_burst = convert_burst(sconfig->dst_maxburst); > > > - dst_width = convert_buswidth(sconfig->dst_addr_width); > > > + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > > + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > > + src_maxburst = src_maxburst ? src_maxburst : 8; > > > > > > break; > > > > > > case DMA_DEV_TO_MEM: > > > - src_burst = convert_burst(sconfig->src_maxburst); > > > - src_width = convert_buswidth(sconfig->src_addr_width); > > > - dst_burst = convert_burst(sconfig->dst_maxburst ? > > > - sconfig->dst_maxburst : 8); > > > - dst_width = convert_buswidth(sconfig->dst_addr_width != > > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > > - sconfig->dst_addr_width : > > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > > + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > > + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > > + dst_maxburst = dst_maxburst ? dst_maxburst : 8; > > > > > > break; > > > > > > default: > > > return -EINVAL; > > > > > > } > > > > > > - if (src_burst < 0) > > > - return src_burst; > > > - if (src_width < 0) > > > - return src_width; > > > - if (dst_burst < 0) > > > - return dst_burst; > > > - if (dst_width < 0) > > > - return dst_width; > > > - > > > - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | > > > - DMA_CHAN_CFG_SRC_WIDTH(src_width) | > > > - DMA_CHAN_CFG_DST_BURST(dst_burst) | > > > + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) > > > + return -EINVAL; > > > + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) > > > + return -EINVAL; > > > + if (!(BIT(src_maxburst) & supported_burst_length)) > > > + return -EINVAL; > > > + if (!(BIT(dst_maxburst) & supported_burst_length)) > > > + return -EINVAL; > > > + > > > + src_width = convert_buswidth(src_addr_width); > > > + dst_width = convert_buswidth(dst_addr_width); > > > + dst_burst = convert_burst(dst_maxburst); > > > + src_burst = convert_burst(src_maxburst); > > > > I'm not sure what you're trying to do here. Could you split your patch > > by logical change, this doesn't seem related to just supporting the > > H3, but a heavier refactoring. > > Untangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, > range checking, and conversion to register value. As the valid ranges depend > on the controller, the code is much easier to read if the range check is done > first, and then the conversion. Please make separate patches as well for the splitting up of each of those steps. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-01 13:35 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-01 13:35 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > Hi, > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Br?ns wrote: > > > +/* Between SoC generations, there are some significant differences: > > > + * - A23 added a clock gate register > > > + * - the H3 burst length field has a different offset > > > + */ > > > > This is not the proper comment style. > > > > > +enum dmac_variant { > > > + DMAC_VARIANT_A31, > > > + DMAC_VARIANT_A23, > > > + DMAC_VARIANT_H3, > > > +}; > > > + > > > > And this is redundant with what we already have in our structures. > > Actually, its not. For H3, there are currently at least 3 register compatible > SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the > current config structure is kept, we need 3 different compatible strings. Same > for the A23, which is register compatible to e.g. A83t and V3s, but with > different numbers of DMA channels. > > So either you decorate the code with a cascade of > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) { > } else { /* A31 */ > } > > in a number of places, or you do it just once. That's not how you retrieve the structures. They are already associated to the compatible, and you need to do a single lookup to get them. So that's nowhere near what you're suggesting. You can have a look at the of_match_device in the probe function. > > > > > > /* > > > > > > * Hardware channels / ports representation > > > * > > > > > > @@ -101,6 +116,7 @@ struct sun6i_dma_config { > > > > > > u32 nr_max_channels; > > > u32 nr_max_requests; > > > u32 nr_max_vchans; > > > > > > + enum dmac_variant dmac_variant; > > > > > > }; > > > > > > /* > > > > > > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst) > > > > > > switch (maxburst) { > > > > > > case 1: > > > return 0; > > > > > > + case 4: > > > + return 1; > > > > > > case 8: > > > return 2; > > > > > > + case 16: > > > + return 3; > > > > > > default: > > > return -EINVAL; > > > > > > } > > > > > > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst) > > > > > > static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width) > > > { > > > > > > - if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) || > > > - (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES)) > > > - return -EINVAL; > > > - > > > - return addr_width >> 1; > > > + return ilog2(addr_width); > > > > > > } > > > > This isn't really the same operation. There should be some explanation > > about why it's the right thing to do. > > For 1, 2 and 4 it is the same. The correct register value for 8 bytes, > supported by H3, H5, A64 and R40, is 3. That should be in a separate patch, with that in the commit log. > > > static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan) > > > > > > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev, > > > > > > enum dma_transfer_direction direction, > > > u32 *p_cfg) > > > > > > { > > > > > > + enum dma_slave_buswidth src_addr_width, dst_addr_width; > > > + u32 src_maxburst, dst_maxburst, supported_burst_length; > > > > > > s8 src_width, dst_width, src_burst, dst_burst; > > > > > > + src_addr_width = sconfig->src_addr_width; > > > + dst_addr_width = sconfig->dst_addr_width; > > > + src_maxburst = sconfig->src_maxburst; > > > + dst_maxburst = sconfig->dst_maxburst; > > > + > > > + if (sdev->cfg->dmac_variant == DMAC_VARIANT_H3) > > > + supported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16); > > > + else > > > + supported_burst_length = BIT(1) | BIT(8); > > > > This could be stored in the structure for example. > > Which one? sun6i_dma_dev? sun6i_dma_config? The one associated with the compatible, so sun6i_dma_config. > > > > switch (direction) { > > > > > > case DMA_MEM_TO_DEV: > > > - src_burst = convert_burst(sconfig->src_maxburst ? > > > - sconfig->src_maxburst : 8); > > > - src_width = convert_buswidth(sconfig->src_addr_width != > > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > > - sconfig->src_addr_width : > > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > > - dst_burst = convert_burst(sconfig->dst_maxburst); > > > - dst_width = convert_buswidth(sconfig->dst_addr_width); > > > + if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > > + src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > > + src_maxburst = src_maxburst ? src_maxburst : 8; > > > > > > break; > > > > > > case DMA_DEV_TO_MEM: > > > - src_burst = convert_burst(sconfig->src_maxburst); > > > - src_width = convert_buswidth(sconfig->src_addr_width); > > > - dst_burst = convert_burst(sconfig->dst_maxburst ? > > > - sconfig->dst_maxburst : 8); > > > - dst_width = convert_buswidth(sconfig->dst_addr_width != > > > - DMA_SLAVE_BUSWIDTH_UNDEFINED ? > > > - sconfig->dst_addr_width : > > > - DMA_SLAVE_BUSWIDTH_4_BYTES); > > > + if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) > > > + dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > > > + dst_maxburst = dst_maxburst ? dst_maxburst : 8; > > > > > > break; > > > > > > default: > > > return -EINVAL; > > > > > > } > > > > > > - if (src_burst < 0) > > > - return src_burst; > > > - if (src_width < 0) > > > - return src_width; > > > - if (dst_burst < 0) > > > - return dst_burst; > > > - if (dst_width < 0) > > > - return dst_width; > > > - > > > - *p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) | > > > - DMA_CHAN_CFG_SRC_WIDTH(src_width) | > > > - DMA_CHAN_CFG_DST_BURST(dst_burst) | > > > + if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths)) > > > + return -EINVAL; > > > + if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths)) > > > + return -EINVAL; > > > + if (!(BIT(src_maxburst) & supported_burst_length)) > > > + return -EINVAL; > > > + if (!(BIT(dst_maxburst) & supported_burst_length)) > > > + return -EINVAL; > > > + > > > + src_width = convert_buswidth(src_addr_width); > > > + dst_width = convert_buswidth(dst_addr_width); > > > + dst_burst = convert_burst(dst_maxburst); > > > + src_burst = convert_burst(src_maxburst); > > > > I'm not sure what you're trying to do here. Could you split your patch > > by logical change, this doesn't seem related to just supporting the > > H3, but a heavier refactoring. > > Untangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, > range checking, and conversion to register value. As the valid ranges depend > on the controller, the code is much easier to read if the range check is done > first, and then the conversion. Please make separate patches as well for the splitting up of each of those steps. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170901/926b1635/attachment-0001.sig> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 2017-09-01 13:35 ` Maxime Ripard (?) @ 2017-09-01 14:42 ` Brüns, Stefan -1 siblings, 0 replies; 69+ messages in thread From: Brüns, Stefan @ 2017-09-01 14:42 UTC (permalink / raw) To: Maxime Ripard Cc: linux-sunxi, devicetree, dmaengine, vinod.koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > > Hi, > > > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > > > +/* Between SoC generations, there are some significant differences: > > > > + * - A23 added a clock gate register > > > > + * - the H3 burst length field has a different offset > > > > + */ > > > > > > This is not the proper comment style. > > > > > > > +enum dmac_variant { > > > > + DMAC_VARIANT_A31, > > > > + DMAC_VARIANT_A23, > > > > + DMAC_VARIANT_H3, > > > > +}; > > > > + > > > > > > And this is redundant with what we already have in our structures. > > > > Actually, its not. For H3, there are currently at least 3 register > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 > > channels. So if the current config structure is kept, we need 3 different > > compatible strings. Same for the A23, which is register compatible to > > e.g. A83t and V3s, but with different numbers of DMA channels. > > > > So either you decorate the code with a cascade of > > > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) > > { > > } else { /* A31 */ > > } > > > > in a number of places, or you do it just once. > > That's not how you retrieve the structures. They are already > associated to the compatible, and you need to do a single lookup to > get them. So that's nowhere near what you're suggesting. You can have > a look at the of_match_device in the probe function. > Please have a look at the current implementation of how the clock autogating in the probe function is done - it matches with the compatible string. Of course we can replace this with a match between sdev->config and the various sun6i_dma_config instances, but we would still have to do 3 matches for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the H3 register configuration (H3 || R40 || A64). There are currently *7* different configs (V3s, R40 and A64 taken into account), but only 3 different register variants. This is the same rationale as the "gate_needed" boolean property proposed by Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we don't need a boolean, but a ternary option to cater for the gate_needed differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE". Kind regards, Stefan ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-01 14:42 ` Brüns, Stefan 0 siblings, 0 replies; 69+ messages in thread From: Brüns, Stefan @ 2017-09-01 14:42 UTC (permalink / raw) To: linux-arm-kernel On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > > Hi, > > > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Br?ns wrote: > > > > +/* Between SoC generations, there are some significant differences: > > > > + * - A23 added a clock gate register > > > > + * - the H3 burst length field has a different offset > > > > + */ > > > > > > This is not the proper comment style. > > > > > > > +enum dmac_variant { > > > > + DMAC_VARIANT_A31, > > > > + DMAC_VARIANT_A23, > > > > + DMAC_VARIANT_H3, > > > > +}; > > > > + > > > > > > And this is redundant with what we already have in our structures. > > > > Actually, its not. For H3, there are currently at least 3 register > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 > > channels. So if the current config structure is kept, we need 3 different > > compatible strings. Same for the A23, which is register compatible to > > e.g. A83t and V3s, but with different numbers of DMA channels. > > > > So either you decorate the code with a cascade of > > > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) > > { > > } else { /* A31 */ > > } > > > > in a number of places, or you do it just once. > > That's not how you retrieve the structures. They are already > associated to the compatible, and you need to do a single lookup to > get them. So that's nowhere near what you're suggesting. You can have > a look at the of_match_device in the probe function. > Please have a look at the current implementation of how the clock autogating in the probe function is done - it matches with the compatible string. Of course we can replace this with a match between sdev->config and the various sun6i_dma_config instances, but we would still have to do 3 matches for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the H3 register configuration (H3 || R40 || A64). There are currently *7* different configs (V3s, R40 and A64 taken into account), but only 3 different register variants. This is the same rationale as the "gate_needed" boolean property proposed by Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we don't need a boolean, but a ternary option to cater for the gate_needed differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE". Kind regards, Stefan ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-01 14:42 ` Brüns, Stefan 0 siblings, 0 replies; 69+ messages in thread From: Brüns, Stefan @ 2017-09-01 14:42 UTC (permalink / raw) To: Maxime Ripard Cc: linux-sunxi, devicetree, dmaengine, vinod.koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > > Hi, > > > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > > > +/* Between SoC generations, there are some significant differences: > > > > + * - A23 added a clock gate register > > > > + * - the H3 burst length field has a different offset > > > > + */ > > > > > > This is not the proper comment style. > > > > > > > +enum dmac_variant { > > > > + DMAC_VARIANT_A31, > > > > + DMAC_VARIANT_A23, > > > > + DMAC_VARIANT_H3, > > > > +}; > > > > + > > > > > > And this is redundant with what we already have in our structures. > > > > Actually, its not. For H3, there are currently at least 3 register > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 > > channels. So if the current config structure is kept, we need 3 different > > compatible strings. Same for the A23, which is register compatible to > > e.g. A83t and V3s, but with different numbers of DMA channels. > > > > So either you decorate the code with a cascade of > > > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) > > { > > } else { /* A31 */ > > } > > > > in a number of places, or you do it just once. > > That's not how you retrieve the structures. They are already > associated to the compatible, and you need to do a single lookup to > get them. So that's nowhere near what you're suggesting. You can have > a look at the of_match_device in the probe function. > Please have a look at the current implementation of how the clock autogating in the probe function is done - it matches with the compatible string. Of course we can replace this with a match between sdev->config and the various sun6i_dma_config instances, but we would still have to do 3 matches for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the H3 register configuration (H3 || R40 || A64). There are currently *7* different configs (V3s, R40 and A64 taken into account), but only 3 different register variants. This is the same rationale as the "gate_needed" boolean property proposed by Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we don't need a boolean, but a ternary option to cater for the gate_needed differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE". Kind regards, Stefan ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 2017-09-01 14:42 ` Brüns, Stefan (?) (?) @ 2017-09-01 14:51 ` taskboxtester -1 siblings, 0 replies; 69+ messages in thread From: taskboxtester @ 2017-09-01 14:51 UTC (permalink / raw) To: Brüns, Stefan Cc: linux-sunxi, Rob Herring, linux-kernel, devicetree, vinod.koul, dmaengine, Maxime Ripard, linux-arm-kernel, Chen-Yu Tsai [-- Attachment #1: Type: text/plain, Size: 2711 bytes --] taskboxtester@gmail.com liked your message with Boxer for Android. On Sep 1, 2017 10:45 AM, "Brüns, Stefan" <Stefan.Bruens@rwth-aachen.de> wrote: On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > > Hi, > > > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > > > +/* Between SoC generations, there are some significant differences: > > > > + * - A23 added a clock gate register > > > > + * - the H3 burst length field has a different offset > > > > + */ > > > > > > This is not the proper comment style. > > > > > > > +enum dmac_variant { > > > > +DMAC_VARIANT_A31, > > > > +DMAC_VARIANT_A23, > > > > +DMAC_VARIANT_H3, > > > > +}; > > > > + > > > > > > And this is redundant with what we already have in our structures. > > > > Actually, its not. For H3, there are currently at least 3 register > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 > > channels. So if the current config structure is kept, we need 3 different > > compatible strings. Same for the A23, which is register compatible to > > e.g. A83t and V3s, but with different numbers of DMA channels. > > > > So either you decorate the code with a cascade of > > > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) > > { > > } else { /* A31 */ > > } > > > > in a number of places, or you do it just once. > > That's not how you retrieve the structures. They are already > associated to the compatible, and you need to do a single lookup to > get them. So that's nowhere near what you're suggesting. You can have > a look at the of_match_device in the probe function. > Please have a look at the current implementation of how the clock autogating in the probe function is done - it matches with the compatible string. Of course we can replace this with a match between sdev->config and the various sun6i_dma_config instances, but we would still have to do 3 matches for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the H3 register configuration (H3 || R40 || A64). There are currently *7* different configs (V3s, R40 and A64 taken into account), but only 3 different register variants. This is the same rationale as the "gate_needed" boolean property proposed by Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we don't need a boolean, but a ternary option to cater for the gate_needed differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE". Kind regards, Stefan [-- Attachment #2: Type: text/html, Size: 3895 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 2017-09-01 14:42 ` Brüns, Stefan (?) @ 2017-09-04 6:50 ` Maxime Ripard -1 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-04 6:50 UTC (permalink / raw) To: Brüns, Stefan Cc: linux-sunxi, devicetree, dmaengine, vinod.koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring [-- Attachment #1: Type: text/plain, Size: 3180 bytes --] On Fri, Sep 01, 2017 at 02:42:47PM +0000, Brüns, Stefan wrote: > On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote: > > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > > > > +/* Between SoC generations, there are some significant differences: > > > > > + * - A23 added a clock gate register > > > > > + * - the H3 burst length field has a different offset > > > > > + */ > > > > > > > > This is not the proper comment style. > > > > > > > > > +enum dmac_variant { > > > > > + DMAC_VARIANT_A31, > > > > > + DMAC_VARIANT_A23, > > > > > + DMAC_VARIANT_H3, > > > > > +}; > > > > > + > > > > > > > > And this is redundant with what we already have in our structures. > > > > > > Actually, its not. For H3, there are currently at least 3 register > > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 > > > channels. So if the current config structure is kept, we need 3 different > > > compatible strings. Same for the A23, which is register compatible to > > > e.g. A83t and V3s, but with different numbers of DMA channels. > > > > > > So either you decorate the code with a cascade of > > > > > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) > > > { > > > } else { /* A31 */ > > > } > > > > > > in a number of places, or you do it just once. > > > > That's not how you retrieve the structures. They are already > > associated to the compatible, and you need to do a single lookup to > > get them. So that's nowhere near what you're suggesting. You can have > > a look at the of_match_device in the probe function. > > Please have a look at the current implementation of how the clock autogating > in the probe function is done - it matches with the compatible string. Yeah, and we should get rid of that as well. > Of course we can replace this with a match between sdev->config and the > various sun6i_dma_config instances, but we would still have to do 3 matches > for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the > H3 register configuration (H3 || R40 || A64). There are currently *7* > different configs (V3s, R40 and A64 taken into account), but only 3 different > register variants. > > This is the same rationale as the "gate_needed" boolean property proposed by > Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we > don't need a boolean, but a ternary option to cater for the gate_needed > differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE". Or we can just have an extra field in sun6i_dma_config that would set the gate register offset? If it's non-zero, use whatever you have set there, and then you just have to care about two cases, no matter how many offsets we'll have in the wild. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-04 6:50 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-04 6:50 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 01, 2017 at 02:42:47PM +0000, Br?ns, Stefan wrote: > On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote: > > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Br?ns wrote: > > > > > +/* Between SoC generations, there are some significant differences: > > > > > + * - A23 added a clock gate register > > > > > + * - the H3 burst length field has a different offset > > > > > + */ > > > > > > > > This is not the proper comment style. > > > > > > > > > +enum dmac_variant { > > > > > + DMAC_VARIANT_A31, > > > > > + DMAC_VARIANT_A23, > > > > > + DMAC_VARIANT_H3, > > > > > +}; > > > > > + > > > > > > > > And this is redundant with what we already have in our structures. > > > > > > Actually, its not. For H3, there are currently at least 3 register > > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 > > > channels. So if the current config structure is kept, we need 3 different > > > compatible strings. Same for the A23, which is register compatible to > > > e.g. A83t and V3s, but with different numbers of DMA channels. > > > > > > So either you decorate the code with a cascade of > > > > > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) > > > { > > > } else { /* A31 */ > > > } > > > > > > in a number of places, or you do it just once. > > > > That's not how you retrieve the structures. They are already > > associated to the compatible, and you need to do a single lookup to > > get them. So that's nowhere near what you're suggesting. You can have > > a look at the of_match_device in the probe function. > > Please have a look at the current implementation of how the clock autogating > in the probe function is done - it matches with the compatible string. Yeah, and we should get rid of that as well. > Of course we can replace this with a match between sdev->config and the > various sun6i_dma_config instances, but we would still have to do 3 matches > for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the > H3 register configuration (H3 || R40 || A64). There are currently *7* > different configs (V3s, R40 and A64 taken into account), but only 3 different > register variants. > > This is the same rationale as the "gate_needed" boolean property proposed by > Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we > don't need a boolean, but a ternary option to cater for the gate_needed > differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE". Or we can just have an extra field in sun6i_dma_config that would set the gate register offset? If it's non-zero, use whatever you have set there, and then you just have to care about two cases, no matter how many offsets we'll have in the wild. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170904/ee8a13b2/attachment.sig> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 @ 2017-09-04 6:50 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-04 6:50 UTC (permalink / raw) To: Brüns, Stefan Cc: linux-sunxi, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, vinod.koul-ral2JQCrhuEAvxtiuMwx3w, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chen-Yu Tsai, Rob Herring [-- Attachment #1: Type: text/plain, Size: 3180 bytes --] On Fri, Sep 01, 2017 at 02:42:47PM +0000, Brüns, Stefan wrote: > On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote: > > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote: > > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote: > > > > > +/* Between SoC generations, there are some significant differences: > > > > > + * - A23 added a clock gate register > > > > > + * - the H3 burst length field has a different offset > > > > > + */ > > > > > > > > This is not the proper comment style. > > > > > > > > > +enum dmac_variant { > > > > > + DMAC_VARIANT_A31, > > > > > + DMAC_VARIANT_A23, > > > > > + DMAC_VARIANT_H3, > > > > > +}; > > > > > + > > > > > > > > And this is redundant with what we already have in our structures. > > > > > > Actually, its not. For H3, there are currently at least 3 register > > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 > > > channels. So if the current config structure is kept, we need 3 different > > > compatible strings. Same for the A23, which is register compatible to > > > e.g. A83t and V3s, but with different numbers of DMA channels. > > > > > > So either you decorate the code with a cascade of > > > > > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) { > > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) > > > { > > > } else { /* A31 */ > > > } > > > > > > in a number of places, or you do it just once. > > > > That's not how you retrieve the structures. They are already > > associated to the compatible, and you need to do a single lookup to > > get them. So that's nowhere near what you're suggesting. You can have > > a look at the of_match_device in the probe function. > > Please have a look at the current implementation of how the clock autogating > in the probe function is done - it matches with the compatible string. Yeah, and we should get rid of that as well. > Of course we can replace this with a match between sdev->config and the > various sun6i_dma_config instances, but we would still have to do 3 matches > for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the > H3 register configuration (H3 || R40 || A64). There are currently *7* > different configs (V3s, R40 and A64 taken into account), but only 3 different > register variants. > > This is the same rationale as the "gate_needed" boolean property proposed by > Icenowy Zheng in the "Allwinner V3s DMA support" patch series. Obviously we > don't need a boolean, but a ternary option to cater for the gate_needed > differences - "NO_GATE", "A23_STYLE_GATE", "H3_STYLE_GATE". Or we can just have an extra field in sun6i_dma_config that would set the gate register offset? If it's non-zero, use whatever you have set there, and then you just have to care about two cases, no matter how many offsets we'll have in the wild. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 2/3] arm64: allwinner: a64: Add device node for DMA controller 2017-08-30 23:36 ` Stefan Brüns (?) @ 2017-08-30 23:36 ` Stefan Brüns -1 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring The A64 SoC has a DMA controller that supports 8 DMA channels to and from various peripherals. Add a device node for it. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- Documentation/devicetree/bindings/dma/sun6i-dma.txt | 1 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index 6b267045f522..4398b990250c 100644 --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt @@ -9,6 +9,7 @@ Required properties: "allwinner,sun8i-a23-dma" "allwinner,sun8i-a83t-dma" "allwinner,sun8i-h3-dma" + "allwinner,sun50i-a64-dma" - reg: Should contain the registers base address and length - interrupts: Should contain a reference to the interrupt used by this device - clocks: Should contain a reference to the parent AHB clock diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index 7be9eb2ad83c..f96287d3043a 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -136,6 +136,15 @@ reg = <0x01c00000 0x1000>; }; + dma: dma-controller@01c02000 { + compatible = "allwinner,sun50i-a64-dma"; + reg = <0x01c02000 0x1000>; + interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_DMA>; + resets = <&ccu RST_BUS_DMA>; + #dma-cells = <1>; + }; + mmc0: mmc@1c0f000 { compatible = "allwinner,sun50i-a64-mmc"; reg = <0x01c0f000 0x1000>; -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH 2/3] arm64: allwinner: a64: Add device node for DMA controller @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-arm-kernel The A64 SoC has a DMA controller that supports 8 DMA channels to and from various peripherals. Add a device node for it. Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de> --- Documentation/devicetree/bindings/dma/sun6i-dma.txt | 1 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index 6b267045f522..4398b990250c 100644 --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt @@ -9,6 +9,7 @@ Required properties: "allwinner,sun8i-a23-dma" "allwinner,sun8i-a83t-dma" "allwinner,sun8i-h3-dma" + "allwinner,sun50i-a64-dma" - reg: Should contain the registers base address and length - interrupts: Should contain a reference to the interrupt used by this device - clocks: Should contain a reference to the parent AHB clock diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index 7be9eb2ad83c..f96287d3043a 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -136,6 +136,15 @@ reg = <0x01c00000 0x1000>; }; + dma: dma-controller at 01c02000 { + compatible = "allwinner,sun50i-a64-dma"; + reg = <0x01c02000 0x1000>; + interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_DMA>; + resets = <&ccu RST_BUS_DMA>; + #dma-cells = <1>; + }; + mmc0: mmc at 1c0f000 { compatible = "allwinner,sun50i-a64-mmc"; reg = <0x01c0f000 0x1000>; -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH 2/3] arm64: allwinner: a64: Add device node for DMA controller @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring The A64 SoC has a DMA controller that supports 8 DMA channels to and from various peripherals. Add a device node for it. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- Documentation/devicetree/bindings/dma/sun6i-dma.txt | 1 + arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index 6b267045f522..4398b990250c 100644 --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt @@ -9,6 +9,7 @@ Required properties: "allwinner,sun8i-a23-dma" "allwinner,sun8i-a83t-dma" "allwinner,sun8i-h3-dma" + "allwinner,sun50i-a64-dma" - reg: Should contain the registers base address and length - interrupts: Should contain a reference to the interrupt used by this device - clocks: Should contain a reference to the parent AHB clock diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index 7be9eb2ad83c..f96287d3043a 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -136,6 +136,15 @@ reg = <0x01c00000 0x1000>; }; + dma: dma-controller@01c02000 { + compatible = "allwinner,sun50i-a64-dma"; + reg = <0x01c02000 0x1000>; + interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_DMA>; + resets = <&ccu RST_BUS_DMA>; + #dma-cells = <1>; + }; + mmc0: mmc@1c0f000 { compatible = "allwinner,sun50i-a64-mmc"; reg = <0x01c0f000 0x1000>; -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH 2/3] arm64: allwinner: a64: Add device node for DMA controller @ 2017-09-11 22:00 ` Rob Herring 0 siblings, 0 replies; 69+ messages in thread From: Rob Herring @ 2017-09-11 22:00 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi, devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai On Thu, Aug 31, 2017 at 01:36:08AM +0200, Stefan Brüns wrote: > The A64 SoC has a DMA controller that supports 8 DMA channels > to and from various peripherals. > > Add a device node for it. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > Documentation/devicetree/bindings/dma/sun6i-dma.txt | 1 + > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++ > 2 files changed, 10 insertions(+) Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 2/3] arm64: allwinner: a64: Add device node for DMA controller @ 2017-09-11 22:00 ` Rob Herring 0 siblings, 0 replies; 69+ messages in thread From: Rob Herring @ 2017-09-11 22:00 UTC (permalink / raw) To: linux-arm-kernel On Thu, Aug 31, 2017 at 01:36:08AM +0200, Stefan Br?ns wrote: > The A64 SoC has a DMA controller that supports 8 DMA channels > to and from various peripherals. > > Add a device node for it. > > Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de> > --- > Documentation/devicetree/bindings/dma/sun6i-dma.txt | 1 + > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++ > 2 files changed, 10 insertions(+) Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 2/3] arm64: allwinner: a64: Add device node for DMA controller @ 2017-09-11 22:00 ` Rob Herring 0 siblings, 0 replies; 69+ messages in thread From: Rob Herring @ 2017-09-11 22:00 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Maxime Ripard, Chen-Yu Tsai On Thu, Aug 31, 2017 at 01:36:08AM +0200, Stefan Brüns wrote: > The A64 SoC has a DMA controller that supports 8 DMA channels > to and from various peripherals. > > Add a device node for it. > > Signed-off-by: Stefan Brüns <stefan.bruens-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org> > --- > Documentation/devicetree/bindings/dma/sun6i-dma.txt | 1 + > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++ > 2 files changed, 10 insertions(+) Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-08-30 23:36 ` Stefan Brüns (?) @ 2017-08-30 23:36 ` Stefan Brüns -1 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring The A64 SoC has the same dma engine as the H3 (sun8i), with a reduced amount of physical channels. Add the proper config data and compatible string to support it. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ drivers/dma/sun6i-dma.c | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index f96287d3043a..b86019238b77 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -494,6 +494,8 @@ interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; clock-names = "ahb", "mod"; + dmas = <&dma 23>, <&dma 23>; + dma-names = "rx", "tx"; pinctrl-names = "default"; pinctrl-0 = <&spi0_pins>; resets = <&ccu RST_BUS_SPI0>; @@ -509,6 +511,8 @@ interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>; clock-names = "ahb", "mod"; + dmas = <&dma 24>, <&dma 24>; + dma-names = "rx", "tx"; pinctrl-names = "default"; pinctrl-0 = <&spi1_pins>; resets = <&ccu RST_BUS_SPI1>; diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index 5f4eee4513e5..6a17c5d63582 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { .nr_max_vchans = 34, .dmac_variant = DMAC_VARIANT_H3, }; + +static struct sun6i_dma_config sun50i_a64_dma_cfg = { + .nr_max_channels = 8, + .nr_max_requests = 27, + .nr_max_vchans = 38, + .dmac_variant = DMAC_VARIANT_H3, }; static const struct of_device_id sun6i_dma_match[] = { @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun6i_dma_match); -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-arm-kernel The A64 SoC has the same dma engine as the H3 (sun8i), with a reduced amount of physical channels. Add the proper config data and compatible string to support it. Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de> --- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ drivers/dma/sun6i-dma.c | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index f96287d3043a..b86019238b77 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -494,6 +494,8 @@ interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; clock-names = "ahb", "mod"; + dmas = <&dma 23>, <&dma 23>; + dma-names = "rx", "tx"; pinctrl-names = "default"; pinctrl-0 = <&spi0_pins>; resets = <&ccu RST_BUS_SPI0>; @@ -509,6 +511,8 @@ interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>; clock-names = "ahb", "mod"; + dmas = <&dma 24>, <&dma 24>; + dma-names = "rx", "tx"; pinctrl-names = "default"; pinctrl-0 = <&spi1_pins>; resets = <&ccu RST_BUS_SPI1>; diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index 5f4eee4513e5..6a17c5d63582 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { .nr_max_vchans = 34, .dmac_variant = DMAC_VARIANT_H3, }; + +static struct sun6i_dma_config sun50i_a64_dma_cfg = { + .nr_max_channels = 8, + .nr_max_requests = 27, + .nr_max_vchans = 38, + .dmac_variant = DMAC_VARIANT_H3, }; static const struct of_device_id sun6i_dma_match[] = { @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun6i_dma_match); -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-30 23:36 ` Stefan Brüns 0 siblings, 0 replies; 69+ messages in thread From: Stefan Brüns @ 2017-08-30 23:36 UTC (permalink / raw) To: linux-sunxi Cc: devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring The A64 SoC has the same dma engine as the H3 (sun8i), with a reduced amount of physical channels. Add the proper config data and compatible string to support it. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ drivers/dma/sun6i-dma.c | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index f96287d3043a..b86019238b77 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -494,6 +494,8 @@ interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; clock-names = "ahb", "mod"; + dmas = <&dma 23>, <&dma 23>; + dma-names = "rx", "tx"; pinctrl-names = "default"; pinctrl-0 = <&spi0_pins>; resets = <&ccu RST_BUS_SPI0>; @@ -509,6 +511,8 @@ interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>; clock-names = "ahb", "mod"; + dmas = <&dma 24>, <&dma 24>; + dma-names = "rx", "tx"; pinctrl-names = "default"; pinctrl-0 = <&spi1_pins>; resets = <&ccu RST_BUS_SPI1>; diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index 5f4eee4513e5..6a17c5d63582 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { .nr_max_vchans = 34, .dmac_variant = DMAC_VARIANT_H3, }; + +static struct sun6i_dma_config sun50i_a64_dma_cfg = { + .nr_max_channels = 8, + .nr_max_requests = 27, + .nr_max_vchans = 38, + .dmac_variant = DMAC_VARIANT_H3, }; static const struct of_device_id sun6i_dma_match[] = { @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun6i_dma_match); -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-08-30 23:36 ` Stefan Brüns @ 2017-08-31 11:44 ` Code Kipper -1 siblings, 0 replies; 69+ messages in thread From: Code Kipper @ 2017-08-31 11:44 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi, devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Maxime Ripard, Chen-Yu Tsai, Rob Herring On 31 August 2017 at 01:36, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ > drivers/dma/sun6i-dma.c | 7 +++++++ > 2 files changed, 11 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > index f96287d3043a..b86019238b77 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > @@ -494,6 +494,8 @@ > interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; > clock-names = "ahb", "mod"; > + dmas = <&dma 23>, <&dma 23>; > + dma-names = "rx", "tx"; Hi Stefan, the dtsi parts should be in a separate patch > pinctrl-names = "default"; > pinctrl-0 = <&spi0_pins>; > resets = <&ccu RST_BUS_SPI0>; > @@ -509,6 +511,8 @@ > interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>; > clock-names = "ahb", "mod"; > + dmas = <&dma 24>, <&dma 24>; > + dma-names = "rx", "tx"; > pinctrl-names = "default"; > pinctrl-0 = <&spi1_pins>; > resets = <&ccu RST_BUS_SPI1>; > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > index 5f4eee4513e5..6a17c5d63582 100644 > --- a/drivers/dma/sun6i-dma.c > +++ b/drivers/dma/sun6i-dma.c > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > .nr_max_vchans = 34, > .dmac_variant = DMAC_VARIANT_H3, > }; > + > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > + .nr_max_channels = 8, > + .nr_max_requests = 27, > + .nr_max_vchans = 38, > + .dmac_variant = DMAC_VARIANT_H3, > }; > > static const struct of_device_id sun6i_dma_match[] = { > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, This all looks fine...it's similar to my patch here https://github.com/codekipper/linux-sunxi/commit/8c54d9852dfad6ceb478c579a1213f38fb12fa80 which I've been too lazy to post. I think the binding documentation should go with this patch and this should also be the 1st patch in the series, followed by the dtsi changes. BR, CK > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > -- > 2.14.1 > ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-31 11:44 ` Code Kipper 0 siblings, 0 replies; 69+ messages in thread From: Code Kipper @ 2017-08-31 11:44 UTC (permalink / raw) To: linux-arm-kernel On 31 August 2017 at 01:36, Stefan Br?ns <stefan.bruens@rwth-aachen.de> wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. > > Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ > drivers/dma/sun6i-dma.c | 7 +++++++ > 2 files changed, 11 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > index f96287d3043a..b86019238b77 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > @@ -494,6 +494,8 @@ > interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>; > clock-names = "ahb", "mod"; > + dmas = <&dma 23>, <&dma 23>; > + dma-names = "rx", "tx"; Hi Stefan, the dtsi parts should be in a separate patch > pinctrl-names = "default"; > pinctrl-0 = <&spi0_pins>; > resets = <&ccu RST_BUS_SPI0>; > @@ -509,6 +511,8 @@ > interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>; > clock-names = "ahb", "mod"; > + dmas = <&dma 24>, <&dma 24>; > + dma-names = "rx", "tx"; > pinctrl-names = "default"; > pinctrl-0 = <&spi1_pins>; > resets = <&ccu RST_BUS_SPI1>; > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > index 5f4eee4513e5..6a17c5d63582 100644 > --- a/drivers/dma/sun6i-dma.c > +++ b/drivers/dma/sun6i-dma.c > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > .nr_max_vchans = 34, > .dmac_variant = DMAC_VARIANT_H3, > }; > + > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > + .nr_max_channels = 8, > + .nr_max_requests = 27, > + .nr_max_vchans = 38, > + .dmac_variant = DMAC_VARIANT_H3, > }; > > static const struct of_device_id sun6i_dma_match[] = { > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, This all looks fine...it's similar to my patch here https://github.com/codekipper/linux-sunxi/commit/8c54d9852dfad6ceb478c579a1213f38fb12fa80 which I've been too lazy to post. I think the binding documentation should go with this patch and this should also be the 1st patch in the series, followed by the dtsi changes. BR, CK > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > -- > 2.14.1 > ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-31 14:52 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-08-31 14:52 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi, devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring [-- Attachment #1: Type: text/plain, Size: 678 bytes --] On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Brüns wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ > drivers/dma/sun6i-dma.c | 7 +++++++ > 2 files changed, 11 insertions(+) With what device did you test this? As far as I know, the SPI driver doesn't use DMA at all. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-31 14:52 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-08-31 14:52 UTC (permalink / raw) To: linux-arm-kernel On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Br?ns wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. > > Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ > drivers/dma/sun6i-dma.c | 7 +++++++ > 2 files changed, 11 insertions(+) With what device did you test this? As far as I know, the SPI driver doesn't use DMA at all. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170831/ed943cf9/attachment.sig> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-31 14:52 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-08-31 14:52 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chen-Yu Tsai, Rob Herring [-- Attachment #1: Type: text/plain, Size: 703 bytes --] On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Brüns wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. > > Signed-off-by: Stefan Brüns <stefan.bruens-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ > drivers/dma/sun6i-dma.c | 7 +++++++ > 2 files changed, 11 insertions(+) With what device did you test this? As far as I know, the SPI driver doesn't use DMA at all. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-31 16:35 ` Code Kipper 0 siblings, 0 replies; 69+ messages in thread From: Code Kipper @ 2017-08-31 16:35 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi, devicetree, dmaengine, Vinod Koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring On 31 August 2017 at 16:52, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Brüns wrote: >> The A64 SoC has the same dma engine as the H3 (sun8i), with a >> reduced amount of physical channels. Add the proper config data >> and compatible string to support it. >> >> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> >> --- >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ >> drivers/dma/sun6i-dma.c | 7 +++++++ >> 2 files changed, 11 insertions(+) > > With what device did you test this? As far as I know, the SPI driver > doesn't use DMA at all. > Hi Maxime, I've tested this on spdif and i2s on the Pine64. BR, CK > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [linux-sunxi] Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-31 16:35 ` Code Kipper 0 siblings, 0 replies; 69+ messages in thread From: Code Kipper @ 2017-08-31 16:35 UTC (permalink / raw) To: linux-arm-kernel On 31 August 2017 at 16:52, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Br?ns wrote: >> The A64 SoC has the same dma engine as the H3 (sun8i), with a >> reduced amount of physical channels. Add the proper config data >> and compatible string to support it. >> >> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de> >> --- >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ >> drivers/dma/sun6i-dma.c | 7 +++++++ >> 2 files changed, 11 insertions(+) > > With what device did you test this? As far as I know, the SPI driver > doesn't use DMA at all. > Hi Maxime, I've tested this on spdif and i2s on the Pine64. BR, CK > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-08-31 16:35 ` Code Kipper 0 siblings, 0 replies; 69+ messages in thread From: Code Kipper @ 2017-08-31 16:35 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi, devicetree, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, linux-arm-kernel, linux-kernel, Chen-Yu Tsai, Rob Herring On 31 August 2017 at 16:52, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote: > On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Brüns wrote: >> The A64 SoC has the same dma engine as the H3 (sun8i), with a >> reduced amount of physical channels. Add the proper config data >> and compatible string to support it. >> >> Signed-off-by: Stefan Brüns <stefan.bruens-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org> >> --- >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++ >> drivers/dma/sun6i-dma.c | 7 +++++++ >> 2 files changed, 11 insertions(+) > > With what device did you test this? As far as I know, the SPI driver > doesn't use DMA at all. > Hi Maxime, I've tested this on spdif and i2s on the Pine64. BR, CK > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 0:31 ` Andre Przywara 0 siblings, 0 replies; 69+ messages in thread From: Andre Przywara @ 2017-09-01 0:31 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel Hi, On 31/08/17 00:36, Stefan Brüns wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. ... > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > index 5f4eee4513e5..6a17c5d63582 100644 > --- a/drivers/dma/sun6i-dma.c > +++ b/drivers/dma/sun6i-dma.c > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > .nr_max_vchans = 34, > .dmac_variant = DMAC_VARIANT_H3, > }; > + > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > + .nr_max_channels = 8, > + .nr_max_requests = 27, > + .nr_max_vchans = 38, > + .dmac_variant = DMAC_VARIANT_H3, > }; > > static const struct of_device_id sun6i_dma_match[] = { > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun6i_dma_match); I was wondering if should use the opportunity to expose those values as DT properties instead of hard-wiring them to a compatible string in the driver every time we add support for a new SoC? We could introduce a new compatible string (say: "allwinner,sunxi-dma"), then describe properties for the number of channels and requests and vchans and parse those from the DT at probe time. With this we might be able to support future SoCs without Linux *driver* changes, by just providing the right DT. This would have worked already for instance for the A83T support, which just changed those values. For instance with this quick patch below (just compile tested, and without your refactoring). The DT node would then read something like: dma: dma-controller@01c02000 { compatible = "allwinner,sun50i-a64-dma", "allwinner,sunxi-dma"; reg = <0x01c02000 0x1000>; interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_DMA>; resets = <&ccu RST_BUS_DMA>; #dma-cells = <1>; allwinner,max_channels = <8>; allwinner,max_requests = <27>; allwinner,max_vchans = <38>; }; Cheers, Andre. diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index a2358780ab2c..5ae8032f2065 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -1033,6 +1033,7 @@ static const struct of_device_id sun6i_dma_match[] = { { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, + { .compatible = "allwinner,sunxi-dma", .data = NULL }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun6i_dma_match); @@ -1051,7 +1052,43 @@ static int sun6i_dma_probe(struct platform_device *pdev) device = of_match_device(sun6i_dma_match, &pdev->dev); if (!device) return -ENODEV; - sdc->cfg = device->data; + if (!device->data) { + struct sun6i_dma_config *config; + + config = devm_kmalloc(&pdev->dev, sizeof(*config), GFP_KERNEL); + if (!config) + return -ENOMEM; + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_channels", + &config->nr_max_channels); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_channels property\n"); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_requests", + &config->nr_max_requests); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_requests property\n"); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_vchans", + &config->nr_max_vchans); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_vchans property\n"); + return ret; + } + sdc->cfg = config; + } else { + sdc->cfg = device->data; + } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); sdc->base = devm_ioremap_resource(&pdev->dev, res); -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 0:31 ` Andre Przywara 0 siblings, 0 replies; 69+ messages in thread From: Andre Przywara @ 2017-09-01 0:31 UTC (permalink / raw) To: linux-arm-kernel Hi, On 31/08/17 00:36, Stefan Br?ns wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. ... > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > index 5f4eee4513e5..6a17c5d63582 100644 > --- a/drivers/dma/sun6i-dma.c > +++ b/drivers/dma/sun6i-dma.c > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > .nr_max_vchans = 34, > .dmac_variant = DMAC_VARIANT_H3, > }; > + > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > + .nr_max_channels = 8, > + .nr_max_requests = 27, > + .nr_max_vchans = 38, > + .dmac_variant = DMAC_VARIANT_H3, > }; > > static const struct of_device_id sun6i_dma_match[] = { > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun6i_dma_match); I was wondering if should use the opportunity to expose those values as DT properties instead of hard-wiring them to a compatible string in the driver every time we add support for a new SoC? We could introduce a new compatible string (say: "allwinner,sunxi-dma"), then describe properties for the number of channels and requests and vchans and parse those from the DT at probe time. With this we might be able to support future SoCs without Linux *driver* changes, by just providing the right DT. This would have worked already for instance for the A83T support, which just changed those values. For instance with this quick patch below (just compile tested, and without your refactoring). The DT node would then read something like: dma: dma-controller at 01c02000 { compatible = "allwinner,sun50i-a64-dma", "allwinner,sunxi-dma"; reg = <0x01c02000 0x1000>; interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_DMA>; resets = <&ccu RST_BUS_DMA>; #dma-cells = <1>; allwinner,max_channels = <8>; allwinner,max_requests = <27>; allwinner,max_vchans = <38>; }; Cheers, Andre. diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index a2358780ab2c..5ae8032f2065 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -1033,6 +1033,7 @@ static const struct of_device_id sun6i_dma_match[] = { { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, + { .compatible = "allwinner,sunxi-dma", .data = NULL }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun6i_dma_match); @@ -1051,7 +1052,43 @@ static int sun6i_dma_probe(struct platform_device *pdev) device = of_match_device(sun6i_dma_match, &pdev->dev); if (!device) return -ENODEV; - sdc->cfg = device->data; + if (!device->data) { + struct sun6i_dma_config *config; + + config = devm_kmalloc(&pdev->dev, sizeof(*config), GFP_KERNEL); + if (!config) + return -ENOMEM; + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_channels", + &config->nr_max_channels); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_channels property\n"); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_requests", + &config->nr_max_requests); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_requests property\n"); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_vchans", + &config->nr_max_vchans); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_vchans property\n"); + return ret; + } + sdc->cfg = config; + } else { + sdc->cfg = device->data; + } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); sdc->base = devm_ioremap_resource(&pdev->dev, res); -- 2.14.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 0:31 ` Andre Przywara 0 siblings, 0 replies; 69+ messages in thread From: Andre Przywara @ 2017-09-01 0:31 UTC (permalink / raw) To: Stefan Brüns Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA Hi, On 31/08/17 00:36, Stefan Brüns wrote: > The A64 SoC has the same dma engine as the H3 (sun8i), with a > reduced amount of physical channels. Add the proper config data > and compatible string to support it. ... > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > index 5f4eee4513e5..6a17c5d63582 100644 > --- a/drivers/dma/sun6i-dma.c > +++ b/drivers/dma/sun6i-dma.c > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > .nr_max_vchans = 34, > .dmac_variant = DMAC_VARIANT_H3, > }; > + > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > + .nr_max_channels = 8, > + .nr_max_requests = 27, > + .nr_max_vchans = 38, > + .dmac_variant = DMAC_VARIANT_H3, > }; > > static const struct of_device_id sun6i_dma_match[] = { > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun6i_dma_match); I was wondering if should use the opportunity to expose those values as DT properties instead of hard-wiring them to a compatible string in the driver every time we add support for a new SoC? We could introduce a new compatible string (say: "allwinner,sunxi-dma"), then describe properties for the number of channels and requests and vchans and parse those from the DT at probe time. With this we might be able to support future SoCs without Linux *driver* changes, by just providing the right DT. This would have worked already for instance for the A83T support, which just changed those values. For instance with this quick patch below (just compile tested, and without your refactoring). The DT node would then read something like: dma: dma-controller@01c02000 { compatible = "allwinner,sun50i-a64-dma", "allwinner,sunxi-dma"; reg = <0x01c02000 0x1000>; interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_DMA>; resets = <&ccu RST_BUS_DMA>; #dma-cells = <1>; allwinner,max_channels = <8>; allwinner,max_requests = <27>; allwinner,max_vchans = <38>; }; Cheers, Andre. diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c index a2358780ab2c..5ae8032f2065 100644 --- a/drivers/dma/sun6i-dma.c +++ b/drivers/dma/sun6i-dma.c @@ -1033,6 +1033,7 @@ static const struct of_device_id sun6i_dma_match[] = { { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, + { .compatible = "allwinner,sunxi-dma", .data = NULL }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun6i_dma_match); @@ -1051,7 +1052,43 @@ static int sun6i_dma_probe(struct platform_device *pdev) device = of_match_device(sun6i_dma_match, &pdev->dev); if (!device) return -ENODEV; - sdc->cfg = device->data; + if (!device->data) { + struct sun6i_dma_config *config; + + config = devm_kmalloc(&pdev->dev, sizeof(*config), GFP_KERNEL); + if (!config) + return -ENOMEM; + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_channels", + &config->nr_max_channels); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_channels property\n"); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_requests", + &config->nr_max_requests); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_requests property\n"); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, + "allwinner,max_vchans", + &config->nr_max_vchans); + if (ret) { + dev_err(&pdev->dev, + "missing allwinner,max_vchans property\n"); + return ret; + } + sdc->cfg = config; + } else { + sdc->cfg = device->data; + } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); sdc->base = devm_ioremap_resource(&pdev->dev, res); -- 2.14.1 -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 1:19 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-01 1:19 UTC (permalink / raw) To: Andre Przywara Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > Hi, > > On 31/08/17 00:36, Stefan Brüns wrote: > > The A64 SoC has the same dma engine as the H3 (sun8i), with a > > reduced amount of physical channels. Add the proper config data > > and compatible string to support it. > > ... > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > > index 5f4eee4513e5..6a17c5d63582 100644 > > --- a/drivers/dma/sun6i-dma.c > > +++ b/drivers/dma/sun6i-dma.c > > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > > > > .nr_max_vchans = 34, > > .dmac_variant = DMAC_VARIANT_H3, > > > > }; > > > > + > > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > > + .nr_max_channels = 8, > > + .nr_max_requests = 27, > > + .nr_max_vchans = 38, > > + .dmac_variant = DMAC_VARIANT_H3, > > > > }; > > > > static const struct of_device_id sun6i_dma_match[] = { > > > > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = > > {> > > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg > > }, > > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > > > > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg > > },> > > { /* sentinel */ } > > > > }; > > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > > I was wondering if should use the opportunity to expose those values as > DT properties instead of hard-wiring them to a compatible string in the > driver every time we add support for a new SoC? > We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > then describe properties for the number of channels and requests and > vchans and parse those from the DT at probe time. > With this we might be able to support future SoCs without Linux *driver* > changes, by just providing the right DT. This would have worked already > for instance for the A83T support, which just changed those values. > > For instance with this quick patch below (just compile tested, and without > your refactoring). > The DT node would then read something like: > dma: dma-controller@01c02000 { > compatible = "allwinner,sun50i-a64-dma", > "allwinner,sunxi-dma"; > reg = <0x01c02000 0x1000>; > interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_DMA>; > resets = <&ccu RST_BUS_DMA>; > #dma-cells = <1>; > allwinner,max_channels = <8>; > allwinner,max_requests = <27>; > allwinner,max_vchans = <38>; > }; For these 3 properties it likely is a good idea, but we would IMHO still have to care for the differences in the register settings: - A31 does not have a clock autogating register - A23 and A83t does have one at offset 0x20 - A64, H3, H5 and R40 have it at offset 0x28 There are also the incompatibilities in the "DMA channel configuration register" (burst length; burst width; burst length field offset). We can either have 3 different compatible strings, or another property for the register model. For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a better option - it can encode the allowed DRQ numbers much better (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel configuration register is 5 bits, so the hightest port/DRQ number is 31. For aw,max_channels my first thought is - why max? is it variable? is there a min_channels? My suggestion would be (in order of preference): "aw,channels", "aw,dma_channels", "aw,available_channels". Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 1:19 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-01 1:19 UTC (permalink / raw) To: linux-arm-kernel On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > Hi, > > On 31/08/17 00:36, Stefan Br?ns wrote: > > The A64 SoC has the same dma engine as the H3 (sun8i), with a > > reduced amount of physical channels. Add the proper config data > > and compatible string to support it. > > ... > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > > index 5f4eee4513e5..6a17c5d63582 100644 > > --- a/drivers/dma/sun6i-dma.c > > +++ b/drivers/dma/sun6i-dma.c > > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > > > > .nr_max_vchans = 34, > > .dmac_variant = DMAC_VARIANT_H3, > > > > }; > > > > + > > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > > + .nr_max_channels = 8, > > + .nr_max_requests = 27, > > + .nr_max_vchans = 38, > > + .dmac_variant = DMAC_VARIANT_H3, > > > > }; > > > > static const struct of_device_id sun6i_dma_match[] = { > > > > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = > > {> > > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg > > }, > > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > > > > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg > > },> > > { /* sentinel */ } > > > > }; > > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > > I was wondering if should use the opportunity to expose those values as > DT properties instead of hard-wiring them to a compatible string in the > driver every time we add support for a new SoC? > We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > then describe properties for the number of channels and requests and > vchans and parse those from the DT at probe time. > With this we might be able to support future SoCs without Linux *driver* > changes, by just providing the right DT. This would have worked already > for instance for the A83T support, which just changed those values. > > For instance with this quick patch below (just compile tested, and without > your refactoring). > The DT node would then read something like: > dma: dma-controller at 01c02000 { > compatible = "allwinner,sun50i-a64-dma", > "allwinner,sunxi-dma"; > reg = <0x01c02000 0x1000>; > interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_DMA>; > resets = <&ccu RST_BUS_DMA>; > #dma-cells = <1>; > allwinner,max_channels = <8>; > allwinner,max_requests = <27>; > allwinner,max_vchans = <38>; > }; For these 3 properties it likely is a good idea, but we would IMHO still have to care for the differences in the register settings: - A31 does not have a clock autogating register - A23 and A83t does have one at offset 0x20 - A64, H3, H5 and R40 have it at offset 0x28 There are also the incompatibilities in the "DMA channel configuration register" (burst length; burst width; burst length field offset). We can either have 3 different compatible strings, or another property for the register model. For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a better option - it can encode the allowed DRQ numbers much better (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel configuration register is 5 bits, so the hightest port/DRQ number is 31. For aw,max_channels my first thought is - why max? is it variable? is there a min_channels? My suggestion would be (in order of preference): "aw,channels", "aw,dma_channels", "aw,available_channels". Kind regards, Stefan -- Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 1:19 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-01 1:19 UTC (permalink / raw) To: Andre Przywara Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > Hi, > > On 31/08/17 00:36, Stefan Brüns wrote: > > The A64 SoC has the same dma engine as the H3 (sun8i), with a > > reduced amount of physical channels. Add the proper config data > > and compatible string to support it. > > ... > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > > index 5f4eee4513e5..6a17c5d63582 100644 > > --- a/drivers/dma/sun6i-dma.c > > +++ b/drivers/dma/sun6i-dma.c > > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > > > > .nr_max_vchans = 34, > > .dmac_variant = DMAC_VARIANT_H3, > > > > }; > > > > + > > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > > + .nr_max_channels = 8, > > + .nr_max_requests = 27, > > + .nr_max_vchans = 38, > > + .dmac_variant = DMAC_VARIANT_H3, > > > > }; > > > > static const struct of_device_id sun6i_dma_match[] = { > > > > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = > > {> > > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg > > }, > > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > > > > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg > > },> > > { /* sentinel */ } > > > > }; > > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > > I was wondering if should use the opportunity to expose those values as > DT properties instead of hard-wiring them to a compatible string in the > driver every time we add support for a new SoC? > We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > then describe properties for the number of channels and requests and > vchans and parse those from the DT at probe time. > With this we might be able to support future SoCs without Linux *driver* > changes, by just providing the right DT. This would have worked already > for instance for the A83T support, which just changed those values. > > For instance with this quick patch below (just compile tested, and without > your refactoring). > The DT node would then read something like: > dma: dma-controller@01c02000 { > compatible = "allwinner,sun50i-a64-dma", > "allwinner,sunxi-dma"; > reg = <0x01c02000 0x1000>; > interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_DMA>; > resets = <&ccu RST_BUS_DMA>; > #dma-cells = <1>; > allwinner,max_channels = <8>; > allwinner,max_requests = <27>; > allwinner,max_vchans = <38>; > }; For these 3 properties it likely is a good idea, but we would IMHO still have to care for the differences in the register settings: - A31 does not have a clock autogating register - A23 and A83t does have one at offset 0x20 - A64, H3, H5 and R40 have it at offset 0x28 There are also the incompatibilities in the "DMA channel configuration register" (burst length; burst width; burst length field offset). We can either have 3 different compatible strings, or another property for the register model. For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a better option - it can encode the allowed DRQ numbers much better (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel configuration register is 5 bits, so the hightest port/DRQ number is 31. For aw,max_channels my first thought is - why max? is it variable? is there a min_channels? My suggestion would be (in order of preference): "aw,channels", "aw,dma_channels", "aw,available_channels". Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-09-01 1:19 ` Stefan Bruens @ 2017-09-01 22:32 ` André Przywara -1 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-01 22:32 UTC (permalink / raw) To: Stefan Bruens Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel Hi, On 01/09/17 02:19, Stefan Bruens wrote: > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: >> Hi, >> >> On 31/08/17 00:36, Stefan Brüns wrote: >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>> reduced amount of physical channels. Add the proper config data >>> and compatible string to support it. >> >> ... >> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>> index 5f4eee4513e5..6a17c5d63582 100644 >>> --- a/drivers/dma/sun6i-dma.c >>> +++ b/drivers/dma/sun6i-dma.c >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { >>> >>> .nr_max_vchans = 34, >>> .dmac_variant = DMAC_VARIANT_H3, >>> >>> }; >>> >>> + >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>> + .nr_max_channels = 8, >>> + .nr_max_requests = 27, >>> + .nr_max_vchans = 38, >>> + .dmac_variant = DMAC_VARIANT_H3, >>> >>> }; >>> >>> static const struct of_device_id sun6i_dma_match[] = { >>> >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = >>> {> >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg > }, >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg >>> }, >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, >>> >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg >>> },> >>> { /* sentinel */ } >>> >>> }; >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); >> >> I was wondering if should use the opportunity to expose those values as >> DT properties instead of hard-wiring them to a compatible string in the >> driver every time we add support for a new SoC? >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), >> then describe properties for the number of channels and requests and >> vchans and parse those from the DT at probe time. >> With this we might be able to support future SoCs without Linux *driver* >> changes, by just providing the right DT. This would have worked already >> for instance for the A83T support, which just changed those values. >> >> For instance with this quick patch below (just compile tested, and without >> your refactoring). >> The DT node would then read something like: >> dma: dma-controller@01c02000 { >> compatible = "allwinner,sun50i-a64-dma", >> "allwinner,sunxi-dma"; >> reg = <0x01c02000 0x1000>; >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&ccu CLK_BUS_DMA>; >> resets = <&ccu RST_BUS_DMA>; >> #dma-cells = <1>; >> allwinner,max_channels = <8>; >> allwinner,max_requests = <27>; >> allwinner,max_vchans = <38>; >> }; > > For these 3 properties it likely is a good idea, but we would IMHO still have > to care for the differences in the register settings: > > - A31 does not have a clock autogating register > - A23 and A83t does have one at offset 0x20 > - A64, H3, H5 and R40 have it at offset 0x28 Fair enough - I didn't look too closely at your new changes, especially why it apparently worked before. But as your list shows, we would only need one compatible string per line - in the driver - to cover all SoCs (and possibly future SoCs!), and do the rest via the properties. We can't use the existing h3 compatible string or touch the already existing SoCs and compatible strings, of course, but I guess the A64, R40 and future SoCs could make use of a new (generic?) string. > There are also the incompatibilities in the "DMA channel configuration > register" (burst length; burst width; burst length field offset). > > We can either have 3 different compatible strings, or another property for the > register model. The latter is usually frowned upon, using separate compatible strings for each group of SoCs is the way to go here. > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a > better option - it can encode the allowed DRQ numbers much better (e.g. for > H3, the highest source DRQ is 24). The DRQ field in the channel configuration > register is 5 bits, so the hightest port/DRQ number is 31. So looking more closely at the manual and the code my understanding is that nr_max_requests is more or less some rough molly guard to prevent wrong settings? Derived from the DRQ table in the manual? So that trying to program port 28 on an H3 would fail? But source port 25 or dest port 26 wouldn't be caught by this check, though they would still be "illegal" according to the manual. (Which we are not sure of, I guess, it may just not be documented) So I wonder if this nr_max_requests is useful at all, and we should just check that it fits into 5 bits and assume that the DT has superior knowledge of what's allowed and what not. Now I see what you mean with the bitmask (to cover those gaps), but I am bit sceptical if that is actually useful. After all the DRQ number would be coming from the DT, which we can surely trust. And nr_max_vchans seems to describe the sum of documented DRQs, to just limit the memory allocation? So this could become just 64 to cover all possible cases without SoC specific configuration at all? > For aw,max_channels my first thought is - why max? is it variable? is there a > min_channels? My suggestion would be (in order of preference): "aw,channels", > "aw,dma_channels", "aw,available_channels". Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt I think we can even use the generic "dma-channels" property described there. And possibly the same with "dma-requests", should we keep this. So summarizing this: - We create a new compatible string, which drives an H3 compatible DMA (clock autogating at 0x28, 64-bit data width capable). This name could either be generic, or actually "allwinner,sun50i-a64-dma". - This one sets nr_max_requests to 31 and nr_max_vchans to 64. - Alternatively we expose those values in the DT as properties. - We take the number of DMA channels from the (now required) "dma-channels" property. - We let the A64 (and R40?) use this new binding. - Any future SoC which is close enough can then just piggy-back on this. - Any future *changes* in the Allwinner DMA device which require driver changes create a new compatible string, but still keep the above semantics. Chances are that there are more than one SoC using this kind of new DMA device, so they would work out of the box. Does that make sense? I am happy to provide the code for that, based on your H3 rework. Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 22:32 ` André Przywara 0 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-01 22:32 UTC (permalink / raw) To: linux-arm-kernel Hi, On 01/09/17 02:19, Stefan Bruens wrote: > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: >> Hi, >> >> On 31/08/17 00:36, Stefan Br?ns wrote: >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>> reduced amount of physical channels. Add the proper config data >>> and compatible string to support it. >> >> ... >> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>> index 5f4eee4513e5..6a17c5d63582 100644 >>> --- a/drivers/dma/sun6i-dma.c >>> +++ b/drivers/dma/sun6i-dma.c >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { >>> >>> .nr_max_vchans = 34, >>> .dmac_variant = DMAC_VARIANT_H3, >>> >>> }; >>> >>> + >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>> + .nr_max_channels = 8, >>> + .nr_max_requests = 27, >>> + .nr_max_vchans = 38, >>> + .dmac_variant = DMAC_VARIANT_H3, >>> >>> }; >>> >>> static const struct of_device_id sun6i_dma_match[] = { >>> >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = >>> {> >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg > }, >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg >>> }, >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, >>> >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg >>> },> >>> { /* sentinel */ } >>> >>> }; >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); >> >> I was wondering if should use the opportunity to expose those values as >> DT properties instead of hard-wiring them to a compatible string in the >> driver every time we add support for a new SoC? >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), >> then describe properties for the number of channels and requests and >> vchans and parse those from the DT at probe time. >> With this we might be able to support future SoCs without Linux *driver* >> changes, by just providing the right DT. This would have worked already >> for instance for the A83T support, which just changed those values. >> >> For instance with this quick patch below (just compile tested, and without >> your refactoring). >> The DT node would then read something like: >> dma: dma-controller at 01c02000 { >> compatible = "allwinner,sun50i-a64-dma", >> "allwinner,sunxi-dma"; >> reg = <0x01c02000 0x1000>; >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&ccu CLK_BUS_DMA>; >> resets = <&ccu RST_BUS_DMA>; >> #dma-cells = <1>; >> allwinner,max_channels = <8>; >> allwinner,max_requests = <27>; >> allwinner,max_vchans = <38>; >> }; > > For these 3 properties it likely is a good idea, but we would IMHO still have > to care for the differences in the register settings: > > - A31 does not have a clock autogating register > - A23 and A83t does have one at offset 0x20 > - A64, H3, H5 and R40 have it at offset 0x28 Fair enough - I didn't look too closely at your new changes, especially why it apparently worked before. But as your list shows, we would only need one compatible string per line - in the driver - to cover all SoCs (and possibly future SoCs!), and do the rest via the properties. We can't use the existing h3 compatible string or touch the already existing SoCs and compatible strings, of course, but I guess the A64, R40 and future SoCs could make use of a new (generic?) string. > There are also the incompatibilities in the "DMA channel configuration > register" (burst length; burst width; burst length field offset). > > We can either have 3 different compatible strings, or another property for the > register model. The latter is usually frowned upon, using separate compatible strings for each group of SoCs is the way to go here. > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a > better option - it can encode the allowed DRQ numbers much better (e.g. for > H3, the highest source DRQ is 24). The DRQ field in the channel configuration > register is 5 bits, so the hightest port/DRQ number is 31. So looking more closely at the manual and the code my understanding is that nr_max_requests is more or less some rough molly guard to prevent wrong settings? Derived from the DRQ table in the manual? So that trying to program port 28 on an H3 would fail? But source port 25 or dest port 26 wouldn't be caught by this check, though they would still be "illegal" according to the manual. (Which we are not sure of, I guess, it may just not be documented) So I wonder if this nr_max_requests is useful at all, and we should just check that it fits into 5 bits and assume that the DT has superior knowledge of what's allowed and what not. Now I see what you mean with the bitmask (to cover those gaps), but I am bit sceptical if that is actually useful. After all the DRQ number would be coming from the DT, which we can surely trust. And nr_max_vchans seems to describe the sum of documented DRQs, to just limit the memory allocation? So this could become just 64 to cover all possible cases without SoC specific configuration at all? > For aw,max_channels my first thought is - why max? is it variable? is there a > min_channels? My suggestion would be (in order of preference): "aw,channels", > "aw,dma_channels", "aw,available_channels". Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt I think we can even use the generic "dma-channels" property described there. And possibly the same with "dma-requests", should we keep this. So summarizing this: - We create a new compatible string, which drives an H3 compatible DMA (clock autogating at 0x28, 64-bit data width capable). This name could either be generic, or actually "allwinner,sun50i-a64-dma". - This one sets nr_max_requests to 31 and nr_max_vchans to 64. - Alternatively we expose those values in the DT as properties. - We take the number of DMA channels from the (now required) "dma-channels" property. - We let the A64 (and R40?) use this new binding. - Any future SoC which is close enough can then just piggy-back on this. - Any future *changes* in the Allwinner DMA device which require driver changes create a new compatible string, but still keep the above semantics. Chances are that there are more than one SoC using this kind of new DMA device, so they would work out of the box. Does that make sense? I am happy to provide the code for that, based on your H3 rework. Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-02 0:38 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-02 0:38 UTC (permalink / raw) To: André Przywara Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote: > Hi, > > On 01/09/17 02:19, Stefan Bruens wrote: > > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Brüns wrote: [...] > > > > For these 3 properties it likely is a good idea, but we would IMHO still > > have to care for the differences in the register settings: > > > > - A31 does not have a clock autogating register > > - A23 and A83t does have one at offset 0x20 > > - A64, H3, H5 and R40 have it at offset 0x28 > > Fair enough - I didn't look too closely at your new changes, especially > why it apparently worked before. > But as your list shows, we would only need one compatible string per > line - in the driver - to cover all SoCs (and possibly future SoCs!), > and do the rest via the properties. > We can't use the existing h3 compatible string or touch the already > existing SoCs and compatible strings, of course, but I guess > the A64, R40 and future SoCs could make use of a new (generic?) string. > > > There are also the incompatibilities in the "DMA channel configuration > > register" (burst length; burst width; burst length field offset). > > > > We can either have 3 different compatible strings, or another property for > > the register model. > > The latter is usually frowned upon, using separate compatible strings > for each group of SoCs is the way to go here. > > > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction > > is a better option - it can encode the allowed DRQ numbers much better > > (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel > > configuration register is 5 bits, so the hightest port/DRQ number is 31. > > So looking more closely at the manual and the code my understanding is > that nr_max_requests is more or less some rough molly guard to prevent > wrong settings? Derived from the DRQ table in the manual? > So that trying to program port 28 on an H3 would fail? > But source port 25 or dest port 26 wouldn't be caught by this check, > though they would still be "illegal" according to the manual. (Which we > are not sure of, I guess, it may just not be documented) > So I wonder if this nr_max_requests is useful at all, and we should just > check that it fits into 5 bits and assume that the DT has superior > knowledge of what's allowed and what not. > Now I see what you mean with the bitmask (to cover those gaps), but I am > bit sceptical if that is actually useful. After all the DRQ number would > be coming from the DT, which we can surely trust. > > And nr_max_vchans seems to describe the sum of documented DRQs, to just > limit the memory allocation? So this could become just 64 to cover all > possible cases without SoC specific configuration at all? Yes, thats my understanding as well. Allocating a few excess channels would waste a few kBytes (AFAICS 304 bytes per channel on 64bit). > > For aw,max_channels my first thought is - why max? is it variable? is > > there a min_channels? My suggestion would be (in order of preference): > > "aw,channels", "aw,dma_channels", "aw,available_channels". > > Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt > I think we can even use the generic "dma-channels" property described > there. And possibly the same with "dma-requests", should we keep this. > > So summarizing this: > - We create a new compatible string, which drives an H3 compatible DMA > (clock autogating at 0x28, 64-bit data width capable). This name could > either be generic, or actually "allwinner,sun50i-a64-dma". > - This one sets nr_max_requests to 31 and nr_max_vchans to 64. > - Alternatively we expose those values in the DT as properties. > - We take the number of DMA channels from the (now required) > "dma-channels" property. > - We let the A64 (and R40?) use this new binding. > - Any future SoC which is close enough can then just piggy-back on this. > - Any future *changes* in the Allwinner DMA device which require driver > changes create a new compatible string, but still keep the above > semantics. Chances are that there are more than one SoC using this kind > of new DMA device, so they would work out of the box. > > Does that make sense? > I am happy to provide the code for that, based on your H3 rework. Sounds good for me. I will send a cleaned up series later. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-02 0:38 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-02 0:38 UTC (permalink / raw) To: linux-arm-kernel On Samstag, 2. September 2017 00:32:50 CEST Andr? Przywara wrote: > Hi, > > On 01/09/17 02:19, Stefan Bruens wrote: > > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Br?ns wrote: [...] > > > > For these 3 properties it likely is a good idea, but we would IMHO still > > have to care for the differences in the register settings: > > > > - A31 does not have a clock autogating register > > - A23 and A83t does have one at offset 0x20 > > - A64, H3, H5 and R40 have it at offset 0x28 > > Fair enough - I didn't look too closely at your new changes, especially > why it apparently worked before. > But as your list shows, we would only need one compatible string per > line - in the driver - to cover all SoCs (and possibly future SoCs!), > and do the rest via the properties. > We can't use the existing h3 compatible string or touch the already > existing SoCs and compatible strings, of course, but I guess > the A64, R40 and future SoCs could make use of a new (generic?) string. > > > There are also the incompatibilities in the "DMA channel configuration > > register" (burst length; burst width; burst length field offset). > > > > We can either have 3 different compatible strings, or another property for > > the register model. > > The latter is usually frowned upon, using separate compatible strings > for each group of SoCs is the way to go here. > > > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction > > is a better option - it can encode the allowed DRQ numbers much better > > (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel > > configuration register is 5 bits, so the hightest port/DRQ number is 31. > > So looking more closely at the manual and the code my understanding is > that nr_max_requests is more or less some rough molly guard to prevent > wrong settings? Derived from the DRQ table in the manual? > So that trying to program port 28 on an H3 would fail? > But source port 25 or dest port 26 wouldn't be caught by this check, > though they would still be "illegal" according to the manual. (Which we > are not sure of, I guess, it may just not be documented) > So I wonder if this nr_max_requests is useful at all, and we should just > check that it fits into 5 bits and assume that the DT has superior > knowledge of what's allowed and what not. > Now I see what you mean with the bitmask (to cover those gaps), but I am > bit sceptical if that is actually useful. After all the DRQ number would > be coming from the DT, which we can surely trust. > > And nr_max_vchans seems to describe the sum of documented DRQs, to just > limit the memory allocation? So this could become just 64 to cover all > possible cases without SoC specific configuration at all? Yes, thats my understanding as well. Allocating a few excess channels would waste a few kBytes (AFAICS 304 bytes per channel on 64bit). > > For aw,max_channels my first thought is - why max? is it variable? is > > there a min_channels? My suggestion would be (in order of preference): > > "aw,channels", "aw,dma_channels", "aw,available_channels". > > Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt > I think we can even use the generic "dma-channels" property described > there. And possibly the same with "dma-requests", should we keep this. > > So summarizing this: > - We create a new compatible string, which drives an H3 compatible DMA > (clock autogating at 0x28, 64-bit data width capable). This name could > either be generic, or actually "allwinner,sun50i-a64-dma". > - This one sets nr_max_requests to 31 and nr_max_vchans to 64. > - Alternatively we expose those values in the DT as properties. > - We take the number of DMA channels from the (now required) > "dma-channels" property. > - We let the A64 (and R40?) use this new binding. > - Any future SoC which is close enough can then just piggy-back on this. > - Any future *changes* in the Allwinner DMA device which require driver > changes create a new compatible string, but still keep the above > semantics. Chances are that there are more than one SoC using this kind > of new DMA device, so they would work out of the box. > > Does that make sense? > I am happy to provide the code for that, based on your H3 rework. Sounds good for me. I will send a cleaned up series later. Kind regards, Stefan -- Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-02 0:38 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-02 0:38 UTC (permalink / raw) To: André Przywara Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote: > Hi, > > On 01/09/17 02:19, Stefan Bruens wrote: > > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Brüns wrote: [...] > > > > For these 3 properties it likely is a good idea, but we would IMHO still > > have to care for the differences in the register settings: > > > > - A31 does not have a clock autogating register > > - A23 and A83t does have one at offset 0x20 > > - A64, H3, H5 and R40 have it at offset 0x28 > > Fair enough - I didn't look too closely at your new changes, especially > why it apparently worked before. > But as your list shows, we would only need one compatible string per > line - in the driver - to cover all SoCs (and possibly future SoCs!), > and do the rest via the properties. > We can't use the existing h3 compatible string or touch the already > existing SoCs and compatible strings, of course, but I guess > the A64, R40 and future SoCs could make use of a new (generic?) string. > > > There are also the incompatibilities in the "DMA channel configuration > > register" (burst length; burst width; burst length field offset). > > > > We can either have 3 different compatible strings, or another property for > > the register model. > > The latter is usually frowned upon, using separate compatible strings > for each group of SoCs is the way to go here. > > > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction > > is a better option - it can encode the allowed DRQ numbers much better > > (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel > > configuration register is 5 bits, so the hightest port/DRQ number is 31. > > So looking more closely at the manual and the code my understanding is > that nr_max_requests is more or less some rough molly guard to prevent > wrong settings? Derived from the DRQ table in the manual? > So that trying to program port 28 on an H3 would fail? > But source port 25 or dest port 26 wouldn't be caught by this check, > though they would still be "illegal" according to the manual. (Which we > are not sure of, I guess, it may just not be documented) > So I wonder if this nr_max_requests is useful at all, and we should just > check that it fits into 5 bits and assume that the DT has superior > knowledge of what's allowed and what not. > Now I see what you mean with the bitmask (to cover those gaps), but I am > bit sceptical if that is actually useful. After all the DRQ number would > be coming from the DT, which we can surely trust. > > And nr_max_vchans seems to describe the sum of documented DRQs, to just > limit the memory allocation? So this could become just 64 to cover all > possible cases without SoC specific configuration at all? Yes, thats my understanding as well. Allocating a few excess channels would waste a few kBytes (AFAICS 304 bytes per channel on 64bit). > > For aw,max_channels my first thought is - why max? is it variable? is > > there a min_channels? My suggestion would be (in order of preference): > > "aw,channels", "aw,dma_channels", "aw,available_channels". > > Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt > I think we can even use the generic "dma-channels" property described > there. And possibly the same with "dma-requests", should we keep this. > > So summarizing this: > - We create a new compatible string, which drives an H3 compatible DMA > (clock autogating at 0x28, 64-bit data width capable). This name could > either be generic, or actually "allwinner,sun50i-a64-dma". > - This one sets nr_max_requests to 31 and nr_max_vchans to 64. > - Alternatively we expose those values in the DT as properties. > - We take the number of DMA channels from the (now required) > "dma-channels" property. > - We let the A64 (and R40?) use this new binding. > - Any future SoC which is close enough can then just piggy-back on this. > - Any future *changes* in the Allwinner DMA device which require driver > changes create a new compatible string, but still keep the above > semantics. Chances are that there are more than one SoC using this kind > of new DMA device, so they would work out of the box. > > Does that make sense? > I am happy to provide the code for that, based on your H3 rework. Sounds good for me. I will send a cleaned up series later. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-09-01 22:32 ` André Przywara (?) @ 2017-09-02 2:02 ` Stefan Bruens -1 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-02 2:02 UTC (permalink / raw) To: André Przywara Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote: > Hi, > > On 01/09/17 02:19, Stefan Bruens wrote: > > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Brüns wrote: > >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a > >>> reduced amount of physical channels. Add the proper config data > >>> and compatible string to support it. > >> > >> ... > >> > >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > >>> index 5f4eee4513e5..6a17c5d63582 100644 > >>> --- a/drivers/dma/sun6i-dma.c > >>> +++ b/drivers/dma/sun6i-dma.c > >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = > >>> { > >>> > >>> .nr_max_vchans = 34, > >>> .dmac_variant = DMAC_VARIANT_H3, > >>> > >>> }; > >>> > >>> + > >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > >>> + .nr_max_channels = 8, > >>> + .nr_max_requests = 27, > >>> + .nr_max_vchans = 38, > >>> + .dmac_variant = DMAC_VARIANT_H3, > >>> > >>> }; > >>> [...] > > There are also the incompatibilities in the "DMA channel configuration > > register" (burst length; burst width; burst length field offset). > > > > We can either have 3 different compatible strings, or another property for > > the register model. > > The latter is usually frowned upon, using separate compatible strings > for each group of SoCs is the way to go here. Just for clarification, I was not talking about a property in the devicetree, but about a struct member in the config data, i.e. the .dmac_variant above. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-02 2:02 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-02 2:02 UTC (permalink / raw) To: linux-arm-kernel On Samstag, 2. September 2017 00:32:50 CEST Andr? Przywara wrote: > Hi, > > On 01/09/17 02:19, Stefan Bruens wrote: > > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Br?ns wrote: > >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a > >>> reduced amount of physical channels. Add the proper config data > >>> and compatible string to support it. > >> > >> ... > >> > >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > >>> index 5f4eee4513e5..6a17c5d63582 100644 > >>> --- a/drivers/dma/sun6i-dma.c > >>> +++ b/drivers/dma/sun6i-dma.c > >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = > >>> { > >>> > >>> .nr_max_vchans = 34, > >>> .dmac_variant = DMAC_VARIANT_H3, > >>> > >>> }; > >>> > >>> + > >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > >>> + .nr_max_channels = 8, > >>> + .nr_max_requests = 27, > >>> + .nr_max_vchans = 38, > >>> + .dmac_variant = DMAC_VARIANT_H3, > >>> > >>> }; > >>> [...] > > There are also the incompatibilities in the "DMA channel configuration > > register" (burst length; burst width; burst length field offset). > > > > We can either have 3 different compatible strings, or another property for > > the register model. > > The latter is usually frowned upon, using separate compatible strings > for each group of SoCs is the way to go here. Just for clarification, I was not talking about a property in the devicetree, but about a struct member in the config data, i.e. the .dmac_variant above. Kind regards, Stefan -- Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-02 2:02 ` Stefan Bruens 0 siblings, 0 replies; 69+ messages in thread From: Stefan Bruens @ 2017-09-02 2:02 UTC (permalink / raw) To: André Przywara Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote: > Hi, > > On 01/09/17 02:19, Stefan Bruens wrote: > > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Brüns wrote: > >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a > >>> reduced amount of physical channels. Add the proper config data > >>> and compatible string to support it. > >> > >> ... > >> > >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > >>> index 5f4eee4513e5..6a17c5d63582 100644 > >>> --- a/drivers/dma/sun6i-dma.c > >>> +++ b/drivers/dma/sun6i-dma.c > >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = > >>> { > >>> > >>> .nr_max_vchans = 34, > >>> .dmac_variant = DMAC_VARIANT_H3, > >>> > >>> }; > >>> > >>> + > >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > >>> + .nr_max_channels = 8, > >>> + .nr_max_requests = 27, > >>> + .nr_max_vchans = 38, > >>> + .dmac_variant = DMAC_VARIANT_H3, > >>> > >>> }; > >>> [...] > > There are also the incompatibilities in the "DMA channel configuration > > register" (burst length; burst width; burst length field offset). > > > > We can either have 3 different compatible strings, or another property for > > the register model. > > The latter is usually frowned upon, using separate compatible strings > for each group of SoCs is the way to go here. Just for clarification, I was not talking about a property in the devicetree, but about a struct member in the config data, i.e. the .dmac_variant above. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-03 23:14 ` André Przywara 0 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-03 23:14 UTC (permalink / raw) To: Stefan Bruens Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel On 02/09/17 03:02, Stefan Bruens wrote: > On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote: >> Hi, >> >> On 01/09/17 02:19, Stefan Bruens wrote: >>> On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: >>>> Hi, >>>> >>>> On 31/08/17 00:36, Stefan Brüns wrote: >>>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>>>> reduced amount of physical channels. Add the proper config data >>>>> and compatible string to support it. >>>> >>>> ... >>>> >>>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>>>> index 5f4eee4513e5..6a17c5d63582 100644 >>>>> --- a/drivers/dma/sun6i-dma.c >>>>> +++ b/drivers/dma/sun6i-dma.c >>>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = >>>>> { >>>>> >>>>> .nr_max_vchans = 34, >>>>> .dmac_variant = DMAC_VARIANT_H3, >>>>> >>>>> }; >>>>> >>>>> + >>>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>>>> + .nr_max_channels = 8, >>>>> + .nr_max_requests = 27, >>>>> + .nr_max_vchans = 38, >>>>> + .dmac_variant = DMAC_VARIANT_H3, >>>>> >>>>> }; >>>>> > [...] >>> There are also the incompatibilities in the "DMA channel configuration >>> register" (burst length; burst width; burst length field offset). >>> >>> We can either have 3 different compatible strings, or another property for >>> the register model. >> >> The latter is usually frowned upon, using separate compatible strings >> for each group of SoCs is the way to go here. > > Just for clarification, I was not talking about a property in the devicetree, > but about a struct member in the config data, i.e. the .dmac_variant above. Ah, I see. I was indeed talking about device tree nodes. So in this case I would lean towards mapping the actual properties to member names in struct sun6i_dma_config, in this case something like: .auto_clock_gate = 0x28; .max_burst_width = 16; This looks more flexible to me and avoids hard to read code where property A is used in SoC X and Y, but property B in SoC X and Z, for instance. In the auto clock gate case this would result in an easy-to-read: writel(SUN8I_DMA_GATE_ENABLE, sdc->base + sdc->cfg->auto_clock_gate); (possibly guarded somehow to catch that A31 case) Sorry for the delay in this answer, I see that you kept the DMAC_VARIANT_ style for your new post, and the end result doesn't look too bad. But maybe still changing this is not too hard now that you have more fine grained patches? Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-03 23:14 ` André Przywara 0 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-03 23:14 UTC (permalink / raw) To: linux-arm-kernel On 02/09/17 03:02, Stefan Bruens wrote: > On Samstag, 2. September 2017 00:32:50 CEST Andr? Przywara wrote: >> Hi, >> >> On 01/09/17 02:19, Stefan Bruens wrote: >>> On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: >>>> Hi, >>>> >>>> On 31/08/17 00:36, Stefan Br?ns wrote: >>>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>>>> reduced amount of physical channels. Add the proper config data >>>>> and compatible string to support it. >>>> >>>> ... >>>> >>>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>>>> index 5f4eee4513e5..6a17c5d63582 100644 >>>>> --- a/drivers/dma/sun6i-dma.c >>>>> +++ b/drivers/dma/sun6i-dma.c >>>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = >>>>> { >>>>> >>>>> .nr_max_vchans = 34, >>>>> .dmac_variant = DMAC_VARIANT_H3, >>>>> >>>>> }; >>>>> >>>>> + >>>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>>>> + .nr_max_channels = 8, >>>>> + .nr_max_requests = 27, >>>>> + .nr_max_vchans = 38, >>>>> + .dmac_variant = DMAC_VARIANT_H3, >>>>> >>>>> }; >>>>> > [...] >>> There are also the incompatibilities in the "DMA channel configuration >>> register" (burst length; burst width; burst length field offset). >>> >>> We can either have 3 different compatible strings, or another property for >>> the register model. >> >> The latter is usually frowned upon, using separate compatible strings >> for each group of SoCs is the way to go here. > > Just for clarification, I was not talking about a property in the devicetree, > but about a struct member in the config data, i.e. the .dmac_variant above. Ah, I see. I was indeed talking about device tree nodes. So in this case I would lean towards mapping the actual properties to member names in struct sun6i_dma_config, in this case something like: .auto_clock_gate = 0x28; .max_burst_width = 16; This looks more flexible to me and avoids hard to read code where property A is used in SoC X and Y, but property B in SoC X and Z, for instance. In the auto clock gate case this would result in an easy-to-read: writel(SUN8I_DMA_GATE_ENABLE, sdc->base + sdc->cfg->auto_clock_gate); (possibly guarded somehow to catch that A31 case) Sorry for the delay in this answer, I see that you kept the DMAC_VARIANT_ style for your new post, and the end result doesn't look too bad. But maybe still changing this is not too hard now that you have more fine grained patches? Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-03 23:14 ` André Przywara 0 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-03 23:14 UTC (permalink / raw) To: Stefan Bruens Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA On 02/09/17 03:02, Stefan Bruens wrote: > On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote: >> Hi, >> >> On 01/09/17 02:19, Stefan Bruens wrote: >>> On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote: >>>> Hi, >>>> >>>> On 31/08/17 00:36, Stefan Brüns wrote: >>>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>>>> reduced amount of physical channels. Add the proper config data >>>>> and compatible string to support it. >>>> >>>> ... >>>> >>>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>>>> index 5f4eee4513e5..6a17c5d63582 100644 >>>>> --- a/drivers/dma/sun6i-dma.c >>>>> +++ b/drivers/dma/sun6i-dma.c >>>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = >>>>> { >>>>> >>>>> .nr_max_vchans = 34, >>>>> .dmac_variant = DMAC_VARIANT_H3, >>>>> >>>>> }; >>>>> >>>>> + >>>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>>>> + .nr_max_channels = 8, >>>>> + .nr_max_requests = 27, >>>>> + .nr_max_vchans = 38, >>>>> + .dmac_variant = DMAC_VARIANT_H3, >>>>> >>>>> }; >>>>> > [...] >>> There are also the incompatibilities in the "DMA channel configuration >>> register" (burst length; burst width; burst length field offset). >>> >>> We can either have 3 different compatible strings, or another property for >>> the register model. >> >> The latter is usually frowned upon, using separate compatible strings >> for each group of SoCs is the way to go here. > > Just for clarification, I was not talking about a property in the devicetree, > but about a struct member in the config data, i.e. the .dmac_variant above. Ah, I see. I was indeed talking about device tree nodes. So in this case I would lean towards mapping the actual properties to member names in struct sun6i_dma_config, in this case something like: .auto_clock_gate = 0x28; .max_burst_width = 16; This looks more flexible to me and avoids hard to read code where property A is used in SoC X and Y, but property B in SoC X and Z, for instance. In the auto clock gate case this would result in an easy-to-read: writel(SUN8I_DMA_GATE_ENABLE, sdc->base + sdc->cfg->auto_clock_gate); (possibly guarded somehow to catch that A31 case) Sorry for the delay in this answer, I see that you kept the DMAC_VARIANT_ style for your new post, and the end result doesn't look too bad. But maybe still changing this is not too hard now that you have more fine grained patches? Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 6:04 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-01 6:04 UTC (permalink / raw) To: Andre Przywara Cc: Stefan Brüns, linux-sunxi, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2825 bytes --] On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: > Hi, > > On 31/08/17 00:36, Stefan Brüns wrote: > > The A64 SoC has the same dma engine as the H3 (sun8i), with a > > reduced amount of physical channels. Add the proper config data > > and compatible string to support it. > > ... > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > > index 5f4eee4513e5..6a17c5d63582 100644 > > --- a/drivers/dma/sun6i-dma.c > > +++ b/drivers/dma/sun6i-dma.c > > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > > .nr_max_vchans = 34, > > .dmac_variant = DMAC_VARIANT_H3, > > }; > > + > > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > > + .nr_max_channels = 8, > > + .nr_max_requests = 27, > > + .nr_max_vchans = 38, > > + .dmac_variant = DMAC_VARIANT_H3, > > }; > > > > static const struct of_device_id sun6i_dma_match[] = { > > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > > I was wondering if should use the opportunity to expose those values as > DT properties instead of hard-wiring them to a compatible string in the > driver every time we add support for a new SoC? > We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > then describe properties for the number of channels and requests and > vchans and parse those from the DT at probe time. > With this we might be able to support future SoCs without Linux *driver* > changes, by just providing the right DT. This would have worked already > for instance for the A83T support, which just changed those values. > > For instance with this quick patch below (just compile tested, and without > your refactoring). > The DT node would then read something like: > dma: dma-controller@01c02000 { > compatible = "allwinner,sun50i-a64-dma", > "allwinner,sunxi-dma"; > reg = <0x01c02000 0x1000>; > interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_DMA>; > resets = <&ccu RST_BUS_DMA>; > #dma-cells = <1>; > allwinner,max_channels = <8>; > allwinner,max_requests = <27>; > allwinner,max_vchans = <38>; > }; We're still going to need a different compatible anyway, so it's not really like it would change anything. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 6:04 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-01 6:04 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: > Hi, > > On 31/08/17 00:36, Stefan Br?ns wrote: > > The A64 SoC has the same dma engine as the H3 (sun8i), with a > > reduced amount of physical channels. Add the proper config data > > and compatible string to support it. > > ... > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > > index 5f4eee4513e5..6a17c5d63582 100644 > > --- a/drivers/dma/sun6i-dma.c > > +++ b/drivers/dma/sun6i-dma.c > > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > > .nr_max_vchans = 34, > > .dmac_variant = DMAC_VARIANT_H3, > > }; > > + > > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > > + .nr_max_channels = 8, > > + .nr_max_requests = 27, > > + .nr_max_vchans = 38, > > + .dmac_variant = DMAC_VARIANT_H3, > > }; > > > > static const struct of_device_id sun6i_dma_match[] = { > > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > > I was wondering if should use the opportunity to expose those values as > DT properties instead of hard-wiring them to a compatible string in the > driver every time we add support for a new SoC? > We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > then describe properties for the number of channels and requests and > vchans and parse those from the DT at probe time. > With this we might be able to support future SoCs without Linux *driver* > changes, by just providing the right DT. This would have worked already > for instance for the A83T support, which just changed those values. > > For instance with this quick patch below (just compile tested, and without > your refactoring). > The DT node would then read something like: > dma: dma-controller at 01c02000 { > compatible = "allwinner,sun50i-a64-dma", > "allwinner,sunxi-dma"; > reg = <0x01c02000 0x1000>; > interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_DMA>; > resets = <&ccu RST_BUS_DMA>; > #dma-cells = <1>; > allwinner,max_channels = <8>; > allwinner,max_requests = <27>; > allwinner,max_vchans = <38>; > }; We're still going to need a different compatible anyway, so it's not really like it would change anything. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170901/a5b2b784/attachment-0001.sig> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 6:04 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-01 6:04 UTC (permalink / raw) To: Andre Przywara Cc: Stefan Brüns, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 3148 bytes --] On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: > Hi, > > On 31/08/17 00:36, Stefan Brüns wrote: > > The A64 SoC has the same dma engine as the H3 (sun8i), with a > > reduced amount of physical channels. Add the proper config data > > and compatible string to support it. > > ... > > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > > index 5f4eee4513e5..6a17c5d63582 100644 > > --- a/drivers/dma/sun6i-dma.c > > +++ b/drivers/dma/sun6i-dma.c > > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > > .nr_max_vchans = 34, > > .dmac_variant = DMAC_VARIANT_H3, > > }; > > + > > +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > > + .nr_max_channels = 8, > > + .nr_max_requests = 27, > > + .nr_max_vchans = 38, > > + .dmac_variant = DMAC_VARIANT_H3, > > }; > > > > static const struct of_device_id sun6i_dma_match[] = { > > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > > { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > > { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > > { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > > + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, sun6i_dma_match); > > I was wondering if should use the opportunity to expose those values as > DT properties instead of hard-wiring them to a compatible string in the > driver every time we add support for a new SoC? > We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > then describe properties for the number of channels and requests and > vchans and parse those from the DT at probe time. > With this we might be able to support future SoCs without Linux *driver* > changes, by just providing the right DT. This would have worked already > for instance for the A83T support, which just changed those values. > > For instance with this quick patch below (just compile tested, and without > your refactoring). > The DT node would then read something like: > dma: dma-controller@01c02000 { > compatible = "allwinner,sun50i-a64-dma", > "allwinner,sunxi-dma"; > reg = <0x01c02000 0x1000>; > interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&ccu CLK_BUS_DMA>; > resets = <&ccu RST_BUS_DMA>; > #dma-cells = <1>; > allwinner,max_channels = <8>; > allwinner,max_requests = <27>; > allwinner,max_vchans = <38>; > }; We're still going to need a different compatible anyway, so it's not really like it would change anything. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-09-01 6:04 ` Maxime Ripard (?) @ 2017-09-01 22:35 ` André Przywara -1 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-01 22:35 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel On 01/09/17 07:04, Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: >> Hi, >> >> On 31/08/17 00:36, Stefan Brüns wrote: >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>> reduced amount of physical channels. Add the proper config data >>> and compatible string to support it. >> >> ... >> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>> index 5f4eee4513e5..6a17c5d63582 100644 >>> --- a/drivers/dma/sun6i-dma.c >>> +++ b/drivers/dma/sun6i-dma.c >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { >>> .nr_max_vchans = 34, >>> .dmac_variant = DMAC_VARIANT_H3, >>> }; >>> + >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>> + .nr_max_channels = 8, >>> + .nr_max_requests = 27, >>> + .nr_max_vchans = 38, >>> + .dmac_variant = DMAC_VARIANT_H3, >>> }; >>> >>> static const struct of_device_id sun6i_dma_match[] = { >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, >>> { /* sentinel */ } >>> }; >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); >> >> I was wondering if should use the opportunity to expose those values as >> DT properties instead of hard-wiring them to a compatible string in the >> driver every time we add support for a new SoC? >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), >> then describe properties for the number of channels and requests and >> vchans and parse those from the DT at probe time. >> With this we might be able to support future SoCs without Linux *driver* >> changes, by just providing the right DT. This would have worked already >> for instance for the A83T support, which just changed those values. >> >> For instance with this quick patch below (just compile tested, and without >> your refactoring). >> The DT node would then read something like: >> dma: dma-controller@01c02000 { >> compatible = "allwinner,sun50i-a64-dma", >> "allwinner,sunxi-dma"; >> reg = <0x01c02000 0x1000>; >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&ccu CLK_BUS_DMA>; >> resets = <&ccu RST_BUS_DMA>; >> #dma-cells = <1>; >> allwinner,max_channels = <8>; >> allwinner,max_requests = <27>; >> allwinner,max_vchans = <38>; >> }; > > We're still going to need a different compatible anyway, so it's not > really like it would change anything. Well, not for now, but possibly in the future. And we should start with this at one point. If we would have had this type of binding already for H3, we could have added the A64 support without driver changes just by a DT change. Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 22:35 ` André Przywara 0 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-01 22:35 UTC (permalink / raw) To: linux-arm-kernel On 01/09/17 07:04, Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: >> Hi, >> >> On 31/08/17 00:36, Stefan Br?ns wrote: >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>> reduced amount of physical channels. Add the proper config data >>> and compatible string to support it. >> >> ... >> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>> index 5f4eee4513e5..6a17c5d63582 100644 >>> --- a/drivers/dma/sun6i-dma.c >>> +++ b/drivers/dma/sun6i-dma.c >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { >>> .nr_max_vchans = 34, >>> .dmac_variant = DMAC_VARIANT_H3, >>> }; >>> + >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>> + .nr_max_channels = 8, >>> + .nr_max_requests = 27, >>> + .nr_max_vchans = 38, >>> + .dmac_variant = DMAC_VARIANT_H3, >>> }; >>> >>> static const struct of_device_id sun6i_dma_match[] = { >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, >>> { /* sentinel */ } >>> }; >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); >> >> I was wondering if should use the opportunity to expose those values as >> DT properties instead of hard-wiring them to a compatible string in the >> driver every time we add support for a new SoC? >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), >> then describe properties for the number of channels and requests and >> vchans and parse those from the DT at probe time. >> With this we might be able to support future SoCs without Linux *driver* >> changes, by just providing the right DT. This would have worked already >> for instance for the A83T support, which just changed those values. >> >> For instance with this quick patch below (just compile tested, and without >> your refactoring). >> The DT node would then read something like: >> dma: dma-controller at 01c02000 { >> compatible = "allwinner,sun50i-a64-dma", >> "allwinner,sunxi-dma"; >> reg = <0x01c02000 0x1000>; >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&ccu CLK_BUS_DMA>; >> resets = <&ccu RST_BUS_DMA>; >> #dma-cells = <1>; >> allwinner,max_channels = <8>; >> allwinner,max_requests = <27>; >> allwinner,max_vchans = <38>; >> }; > > We're still going to need a different compatible anyway, so it's not > really like it would change anything. Well, not for now, but possibly in the future. And we should start with this at one point. If we would have had this type of binding already for H3, we could have added the A64 support without driver changes just by a DT change. Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-01 22:35 ` André Przywara 0 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-01 22:35 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA On 01/09/17 07:04, Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: >> Hi, >> >> On 31/08/17 00:36, Stefan Brüns wrote: >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>> reduced amount of physical channels. Add the proper config data >>> and compatible string to support it. >> >> ... >> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>> index 5f4eee4513e5..6a17c5d63582 100644 >>> --- a/drivers/dma/sun6i-dma.c >>> +++ b/drivers/dma/sun6i-dma.c >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { >>> .nr_max_vchans = 34, >>> .dmac_variant = DMAC_VARIANT_H3, >>> }; >>> + >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>> + .nr_max_channels = 8, >>> + .nr_max_requests = 27, >>> + .nr_max_vchans = 38, >>> + .dmac_variant = DMAC_VARIANT_H3, >>> }; >>> >>> static const struct of_device_id sun6i_dma_match[] = { >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, >>> { /* sentinel */ } >>> }; >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); >> >> I was wondering if should use the opportunity to expose those values as >> DT properties instead of hard-wiring them to a compatible string in the >> driver every time we add support for a new SoC? >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), >> then describe properties for the number of channels and requests and >> vchans and parse those from the DT at probe time. >> With this we might be able to support future SoCs without Linux *driver* >> changes, by just providing the right DT. This would have worked already >> for instance for the A83T support, which just changed those values. >> >> For instance with this quick patch below (just compile tested, and without >> your refactoring). >> The DT node would then read something like: >> dma: dma-controller@01c02000 { >> compatible = "allwinner,sun50i-a64-dma", >> "allwinner,sunxi-dma"; >> reg = <0x01c02000 0x1000>; >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&ccu CLK_BUS_DMA>; >> resets = <&ccu RST_BUS_DMA>; >> #dma-cells = <1>; >> allwinner,max_channels = <8>; >> allwinner,max_requests = <27>; >> allwinner,max_vchans = <38>; >> }; > > We're still going to need a different compatible anyway, so it's not > really like it would change anything. Well, not for now, but possibly in the future. And we should start with this at one point. If we would have had this type of binding already for H3, we could have added the A64 support without driver changes just by a DT change. Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-04 7:04 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-04 7:04 UTC (permalink / raw) To: André Przywara Cc: Stefan Brüns, linux-sunxi, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4146 bytes --] On Fri, Sep 01, 2017 at 11:35:40PM +0100, André Przywara wrote: > On 01/09/17 07:04, Maxime Ripard wrote: > > On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Brüns wrote: > >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a > >>> reduced amount of physical channels. Add the proper config data > >>> and compatible string to support it. > >> > >> ... > >> > >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > >>> index 5f4eee4513e5..6a17c5d63582 100644 > >>> --- a/drivers/dma/sun6i-dma.c > >>> +++ b/drivers/dma/sun6i-dma.c > >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > >>> .nr_max_vchans = 34, > >>> .dmac_variant = DMAC_VARIANT_H3, > >>> }; > >>> + > >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > >>> + .nr_max_channels = 8, > >>> + .nr_max_requests = 27, > >>> + .nr_max_vchans = 38, > >>> + .dmac_variant = DMAC_VARIANT_H3, > >>> }; > >>> > >>> static const struct of_device_id sun6i_dma_match[] = { > >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > >>> { /* sentinel */ } > >>> }; > >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); > >> > >> I was wondering if should use the opportunity to expose those values as > >> DT properties instead of hard-wiring them to a compatible string in the > >> driver every time we add support for a new SoC? > >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > >> then describe properties for the number of channels and requests and > >> vchans and parse those from the DT at probe time. > >> With this we might be able to support future SoCs without Linux *driver* > >> changes, by just providing the right DT. This would have worked already > >> for instance for the A83T support, which just changed those values. > >> > >> For instance with this quick patch below (just compile tested, and without > >> your refactoring). > >> The DT node would then read something like: > >> dma: dma-controller@01c02000 { > >> compatible = "allwinner,sun50i-a64-dma", > >> "allwinner,sunxi-dma"; > >> reg = <0x01c02000 0x1000>; > >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > >> clocks = <&ccu CLK_BUS_DMA>; > >> resets = <&ccu RST_BUS_DMA>; > >> #dma-cells = <1>; > >> allwinner,max_channels = <8>; > >> allwinner,max_requests = <27>; > >> allwinner,max_vchans = <38>; > >> }; > > > > We're still going to need a different compatible anyway, so it's not > > really like it would change anything. > > Well, not for now, but possibly in the future. And we should start with > this at one point. If we would have had this type of binding already for > H3, we could have added the A64 support without driver changes just by a > DT change. That's not true. As usual with these kinds of generic binding arguments (it's definitely not personal, you're far from the only one making them), it completely ignores the fact that the IP itself might change from one revision to another, and its behaviour might too. The A64 for example moved at least one register off, and has a different set of burst size / width. It's something you can't account for when you initially define the binding, unless you describe all the registers, plus all their parameters and the way you interact with them, which isn't going to happen if you want to keep your sanity. And obviously, while maintaining the stability of the binding of those hundreds properties. Or, you can base all this on the compatible, and be done with it once and for all. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-04 7:04 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-04 7:04 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 01, 2017 at 11:35:40PM +0100, Andr? Przywara wrote: > On 01/09/17 07:04, Maxime Ripard wrote: > > On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Br?ns wrote: > >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a > >>> reduced amount of physical channels. Add the proper config data > >>> and compatible string to support it. > >> > >> ... > >> > >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > >>> index 5f4eee4513e5..6a17c5d63582 100644 > >>> --- a/drivers/dma/sun6i-dma.c > >>> +++ b/drivers/dma/sun6i-dma.c > >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > >>> .nr_max_vchans = 34, > >>> .dmac_variant = DMAC_VARIANT_H3, > >>> }; > >>> + > >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > >>> + .nr_max_channels = 8, > >>> + .nr_max_requests = 27, > >>> + .nr_max_vchans = 38, > >>> + .dmac_variant = DMAC_VARIANT_H3, > >>> }; > >>> > >>> static const struct of_device_id sun6i_dma_match[] = { > >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > >>> { /* sentinel */ } > >>> }; > >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); > >> > >> I was wondering if should use the opportunity to expose those values as > >> DT properties instead of hard-wiring them to a compatible string in the > >> driver every time we add support for a new SoC? > >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > >> then describe properties for the number of channels and requests and > >> vchans and parse those from the DT at probe time. > >> With this we might be able to support future SoCs without Linux *driver* > >> changes, by just providing the right DT. This would have worked already > >> for instance for the A83T support, which just changed those values. > >> > >> For instance with this quick patch below (just compile tested, and without > >> your refactoring). > >> The DT node would then read something like: > >> dma: dma-controller at 01c02000 { > >> compatible = "allwinner,sun50i-a64-dma", > >> "allwinner,sunxi-dma"; > >> reg = <0x01c02000 0x1000>; > >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > >> clocks = <&ccu CLK_BUS_DMA>; > >> resets = <&ccu RST_BUS_DMA>; > >> #dma-cells = <1>; > >> allwinner,max_channels = <8>; > >> allwinner,max_requests = <27>; > >> allwinner,max_vchans = <38>; > >> }; > > > > We're still going to need a different compatible anyway, so it's not > > really like it would change anything. > > Well, not for now, but possibly in the future. And we should start with > this at one point. If we would have had this type of binding already for > H3, we could have added the A64 support without driver changes just by a > DT change. That's not true. As usual with these kinds of generic binding arguments (it's definitely not personal, you're far from the only one making them), it completely ignores the fact that the IP itself might change from one revision to another, and its behaviour might too. The A64 for example moved at least one register off, and has a different set of burst size / width. It's something you can't account for when you initially define the binding, unless you describe all the registers, plus all their parameters and the way you interact with them, which isn't going to happen if you want to keep your sanity. And obviously, while maintaining the stability of the binding of those hundreds properties. Or, you can base all this on the compatible, and be done with it once and for all. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170904/f376efb8/attachment.sig> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-04 7:04 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-04 7:04 UTC (permalink / raw) To: André Przywara Cc: Stefan Brüns, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA, dmaengine-u79uwXL29TY76Z2rM5mHXA, Vinod Koul, Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-kernel-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 4146 bytes --] On Fri, Sep 01, 2017 at 11:35:40PM +0100, André Przywara wrote: > On 01/09/17 07:04, Maxime Ripard wrote: > > On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: > >> Hi, > >> > >> On 31/08/17 00:36, Stefan Brüns wrote: > >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a > >>> reduced amount of physical channels. Add the proper config data > >>> and compatible string to support it. > >> > >> ... > >> > >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c > >>> index 5f4eee4513e5..6a17c5d63582 100644 > >>> --- a/drivers/dma/sun6i-dma.c > >>> +++ b/drivers/dma/sun6i-dma.c > >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { > >>> .nr_max_vchans = 34, > >>> .dmac_variant = DMAC_VARIANT_H3, > >>> }; > >>> + > >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { > >>> + .nr_max_channels = 8, > >>> + .nr_max_requests = 27, > >>> + .nr_max_vchans = 38, > >>> + .dmac_variant = DMAC_VARIANT_H3, > >>> }; > >>> > >>> static const struct of_device_id sun6i_dma_match[] = { > >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { > >>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, > >>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, > >>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, > >>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, > >>> { /* sentinel */ } > >>> }; > >>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); > >> > >> I was wondering if should use the opportunity to expose those values as > >> DT properties instead of hard-wiring them to a compatible string in the > >> driver every time we add support for a new SoC? > >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), > >> then describe properties for the number of channels and requests and > >> vchans and parse those from the DT at probe time. > >> With this we might be able to support future SoCs without Linux *driver* > >> changes, by just providing the right DT. This would have worked already > >> for instance for the A83T support, which just changed those values. > >> > >> For instance with this quick patch below (just compile tested, and without > >> your refactoring). > >> The DT node would then read something like: > >> dma: dma-controller@01c02000 { > >> compatible = "allwinner,sun50i-a64-dma", > >> "allwinner,sunxi-dma"; > >> reg = <0x01c02000 0x1000>; > >> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; > >> clocks = <&ccu CLK_BUS_DMA>; > >> resets = <&ccu RST_BUS_DMA>; > >> #dma-cells = <1>; > >> allwinner,max_channels = <8>; > >> allwinner,max_requests = <27>; > >> allwinner,max_vchans = <38>; > >> }; > > > > We're still going to need a different compatible anyway, so it's not > > really like it would change anything. > > Well, not for now, but possibly in the future. And we should start with > this at one point. If we would have had this type of binding already for > H3, we could have added the A64 support without driver changes just by a > DT change. That's not true. As usual with these kinds of generic binding arguments (it's definitely not personal, you're far from the only one making them), it completely ignores the fact that the IP itself might change from one revision to another, and its behaviour might too. The A64 for example moved at least one register off, and has a different set of burst size / width. It's something you can't account for when you initially define the binding, unless you describe all the registers, plus all their parameters and the way you interact with them, which isn't going to happen if you want to keep your sanity. And obviously, while maintaining the stability of the binding of those hundreds properties. Or, you can base all this on the compatible, and be done with it once and for all. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-09-04 7:04 ` Maxime Ripard @ 2017-09-04 8:14 ` André Przywara -1 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-04 8:14 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel Salut, On 04/09/17 08:04, Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 11:35:40PM +0100, André Przywara wrote: >> On 01/09/17 07:04, Maxime Ripard wrote: >>> On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: >>>> Hi, >>>> >>>> On 31/08/17 00:36, Stefan Brüns wrote: >>>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>>>> reduced amount of physical channels. Add the proper config data >>>>> and compatible string to support it. >>>> >>>> ... >>>> >>>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>>>> index 5f4eee4513e5..6a17c5d63582 100644 >>>>> --- a/drivers/dma/sun6i-dma.c >>>>> +++ b/drivers/dma/sun6i-dma.c >>>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { >>>>> .nr_max_vchans = 34, >>>>> .dmac_variant = DMAC_VARIANT_H3, >>>>> }; >>>>> + >>>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>>>> + .nr_max_channels = 8, >>>>> + .nr_max_requests = 27, >>>>> + .nr_max_vchans = 38, >>>>> + .dmac_variant = DMAC_VARIANT_H3, >>>>> }; >>>>> >>>>> static const struct of_device_id sun6i_dma_match[] = { >>>>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { >>>>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, >>>>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, >>>>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, >>>>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, >>>>> { /* sentinel */ } >>>>> }; >>>>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); >>>> >>>> I was wondering if should use the opportunity to expose those values as >>>> DT properties instead of hard-wiring them to a compatible string in the >>>> driver every time we add support for a new SoC? >>>> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), >>>> then describe properties for the number of channels and requests and >>>> vchans and parse those from the DT at probe time. >>>> With this we might be able to support future SoCs without Linux *driver* >>>> changes, by just providing the right DT. This would have worked already >>>> for instance for the A83T support, which just changed those values. >>>> >>>> For instance with this quick patch below (just compile tested, and without >>>> your refactoring). >>>> The DT node would then read something like: >>>> dma: dma-controller@01c02000 { >>>> compatible = "allwinner,sun50i-a64-dma", >>>> "allwinner,sunxi-dma"; >>>> reg = <0x01c02000 0x1000>; >>>> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; >>>> clocks = <&ccu CLK_BUS_DMA>; >>>> resets = <&ccu RST_BUS_DMA>; >>>> #dma-cells = <1>; >>>> allwinner,max_channels = <8>; >>>> allwinner,max_requests = <27>; >>>> allwinner,max_vchans = <38>; >>>> }; >>> >>> We're still going to need a different compatible anyway, so it's not >>> really like it would change anything. >> >> Well, not for now, but possibly in the future. And we should start with >> this at one point. If we would have had this type of binding already for >> H3, we could have added the A64 support without driver changes just by a >> DT change. > > That's not true. As usual with these kinds of generic binding > arguments (it's definitely not personal, you're far from the only one > making them), it completely ignores the fact that the IP itself might > change from one revision to another, and its behaviour might too. Not arguing that, and I totally see that those cases indeed require a new compatible string. > The A64 for example moved at least one register off, and has a > different set of burst size / width. But that is only compared to the original A23/A31 model? AFAICT compared to the H3 DMA it's really only the number of supported DMA channels that has changed. For which we don't even need to invent a property name. My point was that as the driver is *at the moment* all those different compatible strings behaved the same (apart from the missing A23 clock gating), see commits f008db8c00c1 and 3a03ea763a67. The differences we coded in struct sun6i_dma_config were more or less artificial. Now thanks to Stefan we learned that the H3 is more different than we thought, so this argument doesn't hold anymore (but see below). > It's something you can't account for when you initially define the > binding, unless you describe all the registers, plus all their > parameters and the way you interact with them, which isn't going to > happen if you want to keep your sanity. I agree on that. > And obviously, while maintaining the stability of the binding of those > hundreds properties. > > Or, you can base all this on the compatible, and be done with it once > and for all. What I am after is to cover SoCs which *don't* have differences in their register layout, for instance A83T, H3, A64, R40. In an ideal world we could have reused the H3 compatible string, adjusting the number of channels for each SoC in the DT. So I see that having a generic compatible name will not fly, as we now have differences which should not be modelled by DT properties. But I still think we should try to cover those non-register differences (number of channels) with a DT property, to allow reusing the existing driver code whenever possible. As is stands with this series, the R40 support should just be a matter of: compatible = "allwinner,sun8i-r40-dma", "allwinner,sun50i-a64-dma"; I am aware that future SoCs might require new compatibles (I actually hope that they introduce 64-bit memory addresses at some point), but having the "dma-channels" DT parsing code in should then reduce the frequency of driver changes (for SoCs just copying existing DMA IP). Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-04 8:14 ` André Przywara 0 siblings, 0 replies; 69+ messages in thread From: André Przywara @ 2017-09-04 8:14 UTC (permalink / raw) To: linux-arm-kernel Salut, On 04/09/17 08:04, Maxime Ripard wrote: > On Fri, Sep 01, 2017 at 11:35:40PM +0100, Andr? Przywara wrote: >> On 01/09/17 07:04, Maxime Ripard wrote: >>> On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote: >>>> Hi, >>>> >>>> On 31/08/17 00:36, Stefan Br?ns wrote: >>>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a >>>>> reduced amount of physical channels. Add the proper config data >>>>> and compatible string to support it. >>>> >>>> ... >>>> >>>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c >>>>> index 5f4eee4513e5..6a17c5d63582 100644 >>>>> --- a/drivers/dma/sun6i-dma.c >>>>> +++ b/drivers/dma/sun6i-dma.c >>>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = { >>>>> .nr_max_vchans = 34, >>>>> .dmac_variant = DMAC_VARIANT_H3, >>>>> }; >>>>> + >>>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = { >>>>> + .nr_max_channels = 8, >>>>> + .nr_max_requests = 27, >>>>> + .nr_max_vchans = 38, >>>>> + .dmac_variant = DMAC_VARIANT_H3, >>>>> }; >>>>> >>>>> static const struct of_device_id sun6i_dma_match[] = { >>>>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = { >>>>> { .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg }, >>>>> { .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg }, >>>>> { .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg }, >>>>> + { .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg }, >>>>> { /* sentinel */ } >>>>> }; >>>>> MODULE_DEVICE_TABLE(of, sun6i_dma_match); >>>> >>>> I was wondering if should use the opportunity to expose those values as >>>> DT properties instead of hard-wiring them to a compatible string in the >>>> driver every time we add support for a new SoC? >>>> We could introduce a new compatible string (say: "allwinner,sunxi-dma"), >>>> then describe properties for the number of channels and requests and >>>> vchans and parse those from the DT at probe time. >>>> With this we might be able to support future SoCs without Linux *driver* >>>> changes, by just providing the right DT. This would have worked already >>>> for instance for the A83T support, which just changed those values. >>>> >>>> For instance with this quick patch below (just compile tested, and without >>>> your refactoring). >>>> The DT node would then read something like: >>>> dma: dma-controller at 01c02000 { >>>> compatible = "allwinner,sun50i-a64-dma", >>>> "allwinner,sunxi-dma"; >>>> reg = <0x01c02000 0x1000>; >>>> interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>; >>>> clocks = <&ccu CLK_BUS_DMA>; >>>> resets = <&ccu RST_BUS_DMA>; >>>> #dma-cells = <1>; >>>> allwinner,max_channels = <8>; >>>> allwinner,max_requests = <27>; >>>> allwinner,max_vchans = <38>; >>>> }; >>> >>> We're still going to need a different compatible anyway, so it's not >>> really like it would change anything. >> >> Well, not for now, but possibly in the future. And we should start with >> this at one point. If we would have had this type of binding already for >> H3, we could have added the A64 support without driver changes just by a >> DT change. > > That's not true. As usual with these kinds of generic binding > arguments (it's definitely not personal, you're far from the only one > making them), it completely ignores the fact that the IP itself might > change from one revision to another, and its behaviour might too. Not arguing that, and I totally see that those cases indeed require a new compatible string. > The A64 for example moved at least one register off, and has a > different set of burst size / width. But that is only compared to the original A23/A31 model? AFAICT compared to the H3 DMA it's really only the number of supported DMA channels that has changed. For which we don't even need to invent a property name. My point was that as the driver is *at the moment* all those different compatible strings behaved the same (apart from the missing A23 clock gating), see commits f008db8c00c1 and 3a03ea763a67. The differences we coded in struct sun6i_dma_config were more or less artificial. Now thanks to Stefan we learned that the H3 is more different than we thought, so this argument doesn't hold anymore (but see below). > It's something you can't account for when you initially define the > binding, unless you describe all the registers, plus all their > parameters and the way you interact with them, which isn't going to > happen if you want to keep your sanity. I agree on that. > And obviously, while maintaining the stability of the binding of those > hundreds properties. > > Or, you can base all this on the compatible, and be done with it once > and for all. What I am after is to cover SoCs which *don't* have differences in their register layout, for instance A83T, H3, A64, R40. In an ideal world we could have reused the H3 compatible string, adjusting the number of channels for each SoC in the DT. So I see that having a generic compatible name will not fly, as we now have differences which should not be modelled by DT properties. But I still think we should try to cover those non-register differences (number of channels) with a DT property, to allow reusing the existing driver code whenever possible. As is stands with this series, the R40 support should just be a matter of: compatible = "allwinner,sun8i-r40-dma", "allwinner,sun50i-a64-dma"; I am aware that future SoCs might require new compatibles (I actually hope that they introduce 64-bit memory addresses at some point), but having the "dma-channels" DT parsing code in should then reduce the frequency of driver changes (for SoCs just copying existing DMA IP). Cheers, Andre. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-09-04 8:14 ` André Przywara @ 2017-09-08 14:39 ` Maxime Ripard -1 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-08 14:39 UTC (permalink / raw) To: André Przywara Cc: Stefan Brüns, linux-sunxi, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1238 bytes --] Hi, On Mon, Sep 04, 2017 at 09:14:52AM +0100, André Przywara wrote: > > And obviously, while maintaining the stability of the binding of those > > hundreds properties. > > > > Or, you can base all this on the compatible, and be done with it once > > and for all. > > What I am after is to cover SoCs which *don't* have differences in their > register layout, for instance A83T, H3, A64, R40. > In an ideal world we could have reused the H3 compatible string, > adjusting the number of channels for each SoC in the DT. > > So I see that having a generic compatible name will not fly, as we now > have differences which should not be modelled by DT properties. > But I still think we should try to cover those non-register differences > (number of channels) with a DT property, to allow reusing the existing > driver code whenever possible. As is stands with this series, the R40 > support should just be a matter of: > compatible = "allwinner,sun8i-r40-dma", > "allwinner,sun50i-a64-dma"; I just suggested the exact same thing, and then saw your mail, so I guess we have an agreement :) Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-08 14:39 ` Maxime Ripard 0 siblings, 0 replies; 69+ messages in thread From: Maxime Ripard @ 2017-09-08 14:39 UTC (permalink / raw) To: linux-arm-kernel Hi, On Mon, Sep 04, 2017 at 09:14:52AM +0100, Andr? Przywara wrote: > > And obviously, while maintaining the stability of the binding of those > > hundreds properties. > > > > Or, you can base all this on the compatible, and be done with it once > > and for all. > > What I am after is to cover SoCs which *don't* have differences in their > register layout, for instance A83T, H3, A64, R40. > In an ideal world we could have reused the H3 compatible string, > adjusting the number of channels for each SoC in the DT. > > So I see that having a generic compatible name will not fly, as we now > have differences which should not be modelled by DT properties. > But I still think we should try to cover those non-register differences > (number of channels) with a DT property, to allow reusing the existing > driver code whenever possible. As is stands with this series, the R40 > support should just be a matter of: > compatible = "allwinner,sun8i-r40-dma", > "allwinner,sun50i-a64-dma"; I just suggested the exact same thing, and then saw your mail, so I guess we have an agreement :) Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170908/01b3c576/attachment.sig> ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 2017-09-08 14:39 ` Maxime Ripard @ 2017-09-08 14:57 ` Andre Przywara -1 siblings, 0 replies; 69+ messages in thread From: Andre Przywara @ 2017-09-08 14:57 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Brüns, linux-sunxi, Chen-Yu Tsai, devicetree, dmaengine, Vinod Koul, Rob Herring, linux-arm-kernel, linux-kernel Hi Maxime, On 08/09/17 15:39, Maxime Ripard wrote: > Hi, > > On Mon, Sep 04, 2017 at 09:14:52AM +0100, André Przywara wrote: >>> And obviously, while maintaining the stability of the binding of those >>> hundreds properties. >>> >>> Or, you can base all this on the compatible, and be done with it once >>> and for all. >> >> What I am after is to cover SoCs which *don't* have differences in their >> register layout, for instance A83T, H3, A64, R40. >> In an ideal world we could have reused the H3 compatible string, >> adjusting the number of channels for each SoC in the DT. >> >> So I see that having a generic compatible name will not fly, as we now >> have differences which should not be modelled by DT properties. >> But I still think we should try to cover those non-register differences >> (number of channels) with a DT property, to allow reusing the existing >> driver code whenever possible. As is stands with this series, the R40 >> support should just be a matter of: >> compatible = "allwinner,sun8i-r40-dma", >> "allwinner,sun50i-a64-dma"; > > I just suggested the exact same thing, and then saw your mail, so I > guess we have an agreement :) Yes, I was thinking so as well. Since my DeLorean is in the garage ;-) we have no other choice than doing so. My original suggestion for a generic name was based on my naive reading of the existing code, which *looked like* it would be all compatible. But as we know better now, this is the way to go. Merci, André ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 @ 2017-09-08 14:57 ` Andre Przywara 0 siblings, 0 replies; 69+ messages in thread From: Andre Przywara @ 2017-09-08 14:57 UTC (permalink / raw) To: linux-arm-kernel Hi Maxime, On 08/09/17 15:39, Maxime Ripard wrote: > Hi, > > On Mon, Sep 04, 2017 at 09:14:52AM +0100, Andr? Przywara wrote: >>> And obviously, while maintaining the stability of the binding of those >>> hundreds properties. >>> >>> Or, you can base all this on the compatible, and be done with it once >>> and for all. >> >> What I am after is to cover SoCs which *don't* have differences in their >> register layout, for instance A83T, H3, A64, R40. >> In an ideal world we could have reused the H3 compatible string, >> adjusting the number of channels for each SoC in the DT. >> >> So I see that having a generic compatible name will not fly, as we now >> have differences which should not be modelled by DT properties. >> But I still think we should try to cover those non-register differences >> (number of channels) with a DT property, to allow reusing the existing >> driver code whenever possible. As is stands with this series, the R40 >> support should just be a matter of: >> compatible = "allwinner,sun8i-r40-dma", >> "allwinner,sun50i-a64-dma"; > > I just suggested the exact same thing, and then saw your mail, so I > guess we have an agreement :) Yes, I was thinking so as well. Since my DeLorean is in the garage ;-) we have no other choice than doing so. My original suggestion for a generic name was based on my naive reading of the existing code, which *looked like* it would be all compatible. But as we know better now, this is the way to go. Merci, Andr? ^ permalink raw reply [flat|nested] 69+ messages in thread
end of thread, other threads:[~2017-09-11 22:01 UTC | newest] Thread overview: 69+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-30 23:36 [PATCH 0/3] dmaengine: Fix DMA on current allwinner SoCs, add A64 support Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-08-30 23:36 ` [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-08-31 14:51 ` Maxime Ripard 2017-08-31 14:51 ` Maxime Ripard 2017-09-01 3:04 ` Stefan Bruens 2017-09-01 3:04 ` Stefan Bruens 2017-09-01 3:04 ` Stefan Bruens 2017-09-01 13:35 ` Maxime Ripard 2017-09-01 13:35 ` Maxime Ripard 2017-09-01 14:42 ` Brüns, Stefan 2017-09-01 14:42 ` Brüns, Stefan 2017-09-01 14:42 ` Brüns, Stefan 2017-09-01 14:51 ` taskboxtester 2017-09-04 6:50 ` Maxime Ripard 2017-09-04 6:50 ` Maxime Ripard 2017-09-04 6:50 ` Maxime Ripard 2017-08-30 23:36 ` [PATCH 2/3] arm64: allwinner: a64: Add device node for DMA controller Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-09-11 22:00 ` Rob Herring 2017-09-11 22:00 ` Rob Herring 2017-09-11 22:00 ` Rob Herring 2017-08-30 23:36 ` [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64 Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-08-30 23:36 ` Stefan Brüns 2017-08-31 11:44 ` Code Kipper 2017-08-31 11:44 ` Code Kipper 2017-08-31 14:52 ` Maxime Ripard 2017-08-31 14:52 ` Maxime Ripard 2017-08-31 14:52 ` Maxime Ripard 2017-08-31 16:35 ` [linux-sunxi] " Code Kipper 2017-08-31 16:35 ` Code Kipper 2017-08-31 16:35 ` Code Kipper 2017-09-01 0:31 ` Andre Przywara 2017-09-01 0:31 ` Andre Przywara 2017-09-01 0:31 ` Andre Przywara 2017-09-01 1:19 ` Stefan Bruens 2017-09-01 1:19 ` Stefan Bruens 2017-09-01 1:19 ` Stefan Bruens 2017-09-01 22:32 ` André Przywara 2017-09-01 22:32 ` André Przywara 2017-09-02 0:38 ` Stefan Bruens 2017-09-02 0:38 ` Stefan Bruens 2017-09-02 0:38 ` Stefan Bruens 2017-09-02 2:02 ` Stefan Bruens 2017-09-02 2:02 ` Stefan Bruens 2017-09-02 2:02 ` Stefan Bruens 2017-09-03 23:14 ` André Przywara 2017-09-03 23:14 ` André Przywara 2017-09-03 23:14 ` André Przywara 2017-09-01 6:04 ` Maxime Ripard 2017-09-01 6:04 ` Maxime Ripard 2017-09-01 6:04 ` Maxime Ripard 2017-09-01 22:35 ` André Przywara 2017-09-01 22:35 ` André Przywara 2017-09-01 22:35 ` André Przywara 2017-09-04 7:04 ` Maxime Ripard 2017-09-04 7:04 ` Maxime Ripard 2017-09-04 7:04 ` Maxime Ripard 2017-09-04 8:14 ` André Przywara 2017-09-04 8:14 ` André Przywara 2017-09-08 14:39 ` Maxime Ripard 2017-09-08 14:39 ` Maxime Ripard 2017-09-08 14:57 ` Andre Przywara 2017-09-08 14:57 ` Andre Przywara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.