Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1733376 > unrolled thread

[PATCH v2 00/10] dmaengine: sun6i: Fixes for H3/A83T, enable A64

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/10] dmaengine: sun6i: Fixes for H3/A83T, enable A64 Stefan Brüns <stefan.bruens@rwth-aachen.de> - 2017-09-17 05:30 +0200
    [PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow extension for new SoCs Stefan Brüns <stefan.bruens@rwth-aachen.de> - 2017-09-17 05:30 +0200
      Re: [PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow  extension for new SoCs Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-09-18 10:10 +0200
    Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max  request from devicetree Brüns, Stefan <Stefan.Bruens@rwth-aachen.de> - 2017-09-19 18:20 +0200
      Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max  request from devicetree Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-09-22 23:40 +0200
        Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max  request from devicetree Brüns, Stefan <Stefan.Bruens@rwth-aachen.de> - 2017-09-23 02:10 +0200

#1733376 — [PATCH v2 00/10] dmaengine: sun6i: Fixes for H3/A83T, enable A64

Commit 3a03ea763a67 ("dmaengine: sun6i: Add support for Allwinner A83T
(sun8i) variant") and commit f008db8c00c1 ("dmaengine: sun6i: Add support for
Allwinner H3 (sun8i) variant") added support for the A83T resp. H3, but missed
some differences between the original A31 and A83T/H3.

The first patch adds a callback to the controller config to set the clock
autogating register of different SoC generations, i.e. A31, A23+A83T, H3+later,
and uses it to for the correct clock autogating setting.

The second patch adds a callback for the burst length setting in the channel
config register, which has different field offsets and new burst widths/lengths,
which differs between H3 and earlier generations

The third patch restructures some code required for the fourth patch and adds the
burst lengths to the controller config.

The fourth patch adds the burst widths to the config and adds the handling of the
H3 specific burst widths.

Patch 5 restructures the code to decouple some controller details (e.g. channel
count) from the compatible string/the config.

Patches 6, 7 and 8 introduce and use the "dma-chans" property for the A64. Although
register compatible to the H3, the channel count differs and thus it requires a
new compatible. To avoid introduction of new compatibles for each minor variation,
anything but the register model is moved to devicetree properties. There
is at least one SoC (R40) which can then reuse the A64 compatible, the same
would have worked for A83T+V3s.

Patches 9 and 10 add the DMA controller node to the devicetree and add the DMA
controller reference to the SPI nodes.

This patch series could be called v2, but the patches were split and significantly
restructured, thus listing changes individually is not to meaningful.

Changes in v2:
- Use callback for autogating instead of variable for different SoC generations
- Use controller specific callback for burst length setting
- Store burst lengths in config instead of device structure
- Store burst widths in config
- Set default number of dma-request if not provided in config or devicetree

Stefan Brüns (10):
  dmaengine: sun6i: Correct setting of clock autogating register for
    A83T/H3
  dmaengine: sun6i: Correct burst length field offsets for H3
  dmaengine: sun6i: Restructure code to allow extension for new SoCs
  dmaengine: sun6i: Enable additional burst lengths/widths on H3
  dmaengine: sun6i: Move number of pchans/vchans/request to device
    struct
  arm64: allwinner: a64: Add devicetree binding for DMA controller
  dmaengine: sun6i: Retrieve channel count/max request from devicetree
  dmaengine: sun6i: Add support for Allwinner A64 and compatibles
  arm64: allwinner: a64: Add device node for DMA controller
  arm64: allwinner: a64: add dma controller references to spi nodes

 .../devicetree/bindings/dma/sun6i-dma.txt          |  26 ++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  15 ++
 drivers/dma/sun6i-dma.c                            | 265 ++++++++++++++++-----
 3 files changed, 248 insertions(+), 58 deletions(-)

-- 
2.14.1

[toc] | [next] | [standalone]

#1733377 — [PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow extension for new SoCs

The current code mixes three distinct operations when transforming
the slave config to register settings:

  1. special handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, maxburst == 0
  2. range checking
  3. conversion of raw to register values

As the range checks depend on the specific SoC, move these out of the
conversion to distinct operations.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 66 ++++++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a6fc066a0ac6..663f4b0b450e 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -118,6 +118,8 @@ struct sun6i_dma_config {
 	 */
 	void (*clock_autogate_enable)();
 	void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst);
+	u32 src_burst_lengths;
+	u32 dst_burst_lengths;
 };
 
 /*
@@ -266,10 +268,6 @@ 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;
 }
 
@@ -542,41 +540,43 @@ 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;
 	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;
+
 	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;
+	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) & sdev->cfg->src_burst_lengths))
+		return -EINVAL;
+	if (!(BIT(dst_maxburst) & sdev->cfg->dst_burst_lengths))
+		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);
 
 	*p_cfg = DMA_CHAN_CFG_SRC_WIDTH(src_width) |
 		DMA_CHAN_CFG_DST_WIDTH(dst_width);
@@ -1043,6 +1043,8 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
 	.nr_max_vchans   = 53,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_noop;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 /*
@@ -1056,6 +1058,8 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
 	.nr_max_vchans   = 37,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
@@ -1064,6 +1068,8 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
 	.nr_max_vchans   = 39,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 /*
@@ -1077,6 +1083,8 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 	.nr_max_vchans   = 34,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_h3;
 	.set_burst_length = sun6i_set_burst_length_h3;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 /*
@@ -1090,6 +1098,8 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
 	.nr_max_vchans   = 24,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 static const struct of_device_id sun6i_dma_match[] = {
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1733721 — Re: [PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow extension for new SoCs

[Multipart message — attachments visible in raw view] — view raw

On Sun, Sep 17, 2017 at 05:19:49AM +0200, Stefan Brüns wrote:
> The current code mixes three distinct operations when transforming
> the slave config to register settings:
> 
>   1. special handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, maxburst == 0
>   2. range checking
>   3. conversion of raw to register values
> 
> As the range checks depend on the specific SoC, move these out of the
> conversion to distinct operations.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1735073 — Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > Hi,
> > > 
> > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > +	ret = of_property_read_u32(np, "dma-channels", &sdc->num_pchans);
> > > > +	if (ret && !sdc->num_pchans) {
> > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = of_property_read_u32(np, "dma-requests", &sdc->max_request);
> > > > +	if (ret && !sdc->max_request) {
> > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > +			 DMA_CHAN_MAX_DRQ);
> > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > +	}
> > > > +
> > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > 
> > > I'm not really convinced about these two checks. They don't catch all
> > > errors (the range between the actual number of channels / DRQ and the
> > > maximum allowed per the registers), they might increase in the future
> > > too, and if we want to make that check actually working, we would have
> > > to duplicate the number of requests and channels into the driver.
> > 
> > 1. If these values increase, we have a new register layout and and
> > need a new compatible anyway.
> 
> And you want to store a new maximum attached to the compatible? Isn't
> that exactly the situation you're trying to get away from?

Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but 
different number of channels and ports. They could share a compatible (if DMA 
channels were generalized), and we already have several register offsets/
widths (implicitly via the callbacks) attached to the compatible (so these 
don't need generalization via DT).

Now, we could also move everything that is currently attached to the 
compatible, i.e. clock gate register offset, burst widths/lengths etc. into 
the devicetree binding, but that would just be too much.

The idea is to find a middle ground here, using common patterns in the 
existing SoCs. The register layout has hardly changed, while the number of DMA 
channels and ports changes all the time. Moving the number of DMA channels and 
ports to the DT is trivial, and a pattern also found in other DMA controller 
drivers. *If* the number of dma channels and ports is ever increased, 
exceeding the current maximum, this would amount to major changes in the 
driver and maybe even warrant a completely new driver.

> > 2. As long as the the limits are adhered to, no other registers/register
> > fields are overwritten. As the channel number and port are used to
> > calculate memory offsets bounds checking is IMHO a good idea.
> 
> And this is true for many other resources, starting with the one
> defined in reg. We don't error check every register range, clock
> index, reset line, interrupt, DMA channel, the memory size, etc. yet
> you could make the same argument.
> 
> The DT has to be right, and we have to trust it. Otherwise we can just
> throw it away.

So your argument here basically is - don't do any checks on DT provided 
values, these are always correct. So, following this argument, not only the 
range check, but also the of_property_read return values should be ignored, as 
the DT is correct, thus of_property_read will never return an error.

That clearly does not match the implementation of drivers throughout the 
various subsystems for DT properties, which is in general - do all the checks 
that can be done, trust everything you can not verify.

Kind regards,

Stefan

[toc] | [prev] | [next] | [standalone]

#1737876 — Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:
> On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > +	ret = of_property_read_u32(np, "dma-channels", &sdc->num_pchans);
> > > > > +	if (ret && !sdc->num_pchans) {
> > > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	ret = of_property_read_u32(np, "dma-requests", &sdc->max_request);
> > > > > +	if (ret && !sdc->max_request) {
> > > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > > +			 DMA_CHAN_MAX_DRQ);
> > > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > +	}
> > > > > +
> > > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > 
> > > > I'm not really convinced about these two checks. They don't catch all
> > > > errors (the range between the actual number of channels / DRQ and the
> > > > maximum allowed per the registers), they might increase in the future
> > > > too, and if we want to make that check actually working, we would have
> > > > to duplicate the number of requests and channels into the driver.
> > > 
> > > 1. If these values increase, we have a new register layout and and
> > > need a new compatible anyway.
> > 
> > And you want to store a new maximum attached to the compatible? Isn't
> > that exactly the situation you're trying to get away from?
> 
> Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but 
> different number of channels and ports. They could share a compatible (if DMA 
> channels were generalized), and we already have several register offsets/
> widths (implicitly via the callbacks) attached to the compatible (so these 
> don't need generalization via DT).
> 
> Now, we could also move everything that is currently attached to the 
> compatible, i.e. clock gate register offset, burst widths/lengths etc. into 
> the devicetree binding, but that would just be too much.
> 
> The idea is to find a middle ground here, using common patterns in the 
> existing SoCs. The register layout has hardly changed, while the number of DMA 
> channels and ports changes all the time. Moving the number of DMA channels and 
> ports to the DT is trivial, and a pattern also found in other DMA controller 
> drivers.

I'm sorry, but the code is inconsistent here. You basically have two
variables from one SoC to the other, the number of channels and
requests.

In one case (channels), it mandates that the property is provided in
the device tree, and doesn't default to anything.

In the other case (requests), the property is optional and it will
provide a default. All that in 20 lines.

I guess we already reached that middle ground by providing them
through the DT, we just have to make sure we remain consistent.

> *If* the number of dma channels and ports is ever increased,
> exceeding the current maximum, this would amount to major changes in
> the driver and maybe even warrant a completely new driver.
> 
> > > 2. As long as the the limits are adhered to, no other registers/register
> > > fields are overwritten. As the channel number and port are used to
> > > calculate memory offsets bounds checking is IMHO a good idea.
> > 
> > And this is true for many other resources, starting with the one
> > defined in reg. We don't error check every register range, clock
> > index, reset line, interrupt, DMA channel, the memory size, etc. yet
> > you could make the same argument.
> > 
> > The DT has to be right, and we have to trust it. Otherwise we can just
> > throw it away.
> 
> So your argument here basically is - don't do any checks on DT provided 
> values, these are always correct. So, following this argument, not only the 
> range check, but also the of_property_read return values should be ignored, as 
> the DT is correct, thus of_property_read will never return an error.

No, my argument is don't do a check if you can catch only half of the
errors, and with no hope of fixing it.

The functions you mentionned have a 100% error catch rate. This is the
difference.

> That clearly does not match the implementation of drivers throughout the 
> various subsystems for DT properties, which is in general - do all the checks 
> that can be done, trust everything you can not verify.

And my point is that we're falling into the latter here. You cannot
verify it properly.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1737930 — Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:
> On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:
> > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > > +	ret = of_property_read_u32(np, "dma-channels",
> > > > > > &sdc->num_pchans);
> > > > > > +	if (ret && !sdc->num_pchans) {
> > > > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.
\n");
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = of_property_read_u32(np, "dma-requests",
> > > > > > &sdc->max_request);
> > > > > > +	if (ret && !sdc->max_request) {
> > > > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > > > +			 DMA_CHAN_MAX_DRQ);
> > > > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > > +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > 
> > > > > I'm not really convinced about these two checks. They don't catch
> > > > > all
> > > > > errors (the range between the actual number of channels / DRQ and
> > > > > the
> > > > > maximum allowed per the registers), they might increase in the
> > > > > future
> > > > > too, and if we want to make that check actually working, we would
> > > > > have
> > > > > to duplicate the number of requests and channels into the driver.
> > > > 
> > > > 1. If these values increase, we have a new register layout and and
> > > > need a new compatible anyway.
> > > 
> > > And you want to store a new maximum attached to the compatible? Isn't
> > > that exactly the situation you're trying to get away from?
> > 
> > Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but
> > different number of channels and ports. They could share a compatible (if
> > DMA channels were generalized), and we already have several register
> > offsets/ widths (implicitly via the callbacks) attached to the compatible
> > (so these don't need generalization via DT).
> > 
> > Now, we could also move everything that is currently attached to the
> > compatible, i.e. clock gate register offset, burst widths/lengths etc.
> > into
> > the devicetree binding, but that would just be too much.
> > 
> > The idea is to find a middle ground here, using common patterns in the
> > existing SoCs. The register layout has hardly changed, while the number of
> > DMA channels and ports changes all the time. Moving the number of DMA
> > channels and ports to the DT is trivial, and a pattern also found in
> > other DMA controller drivers.
> 
> I'm sorry, but the code is inconsistent here. You basically have two
> variables from one SoC to the other, the number of channels and
> requests.
> 
> In one case (channels), it mandates that the property is provided in
> the device tree, and doesn't default to anything.
> 
> In the other case (requests), the property is optional and it will
> provide a default. All that in 20 lines.

The channel number is a hardware property. Using more channels than the 
hardware provides is a bug. There is no default.

The port/request is just some lax property to limit the resource allocation 
upfront. As long as the bindings of the different IP blocks (SPI, audio, ...) 
provide the correct port numbers, all required information is available.
 
> I guess we already reached that middle ground by providing them
> through the DT, we just have to make sure we remain consistent.
> 
> > *If* the number of dma channels and ports is ever increased,
> > exceeding the current maximum, this would amount to major changes in
> > the driver and maybe even warrant a completely new driver.
> > 
> > > > 2. As long as the the limits are adhered to, no other
> > > > registers/register
> > > > fields are overwritten. As the channel number and port are used to
> > > > calculate memory offsets bounds checking is IMHO a good idea.
> > > 
> > > And this is true for many other resources, starting with the one
> > > defined in reg. We don't error check every register range, clock
> > > index, reset line, interrupt, DMA channel, the memory size, etc. yet
> > > you could make the same argument.
> > > 
> > > The DT has to be right, and we have to trust it. Otherwise we can just
> > > throw it away.
> > 
> > So your argument here basically is - don't do any checks on DT provided
> > values, these are always correct. So, following this argument, not only
> > the
> > range check, but also the of_property_read return values should be
> > ignored, as the DT is correct, thus of_property_read will never return an
> > error.
> No, my argument is don't do a check if you can catch only half of the
> errors, and with no hope of fixing it.
> 
> The functions you mentionned have a 100% error catch rate. This is the
> difference.
> 
> > That clearly does not match the implementation of drivers throughout the
> > various subsystems for DT properties, which is in general - do all the
> > checks that can be done, trust everything you can not verify.
> 
> And my point is that we're falling into the latter here. You cannot
> verify it properly.

Please check the following line:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
drivers/dma/sun6i-dma.c#n951

Thats far from 100% - the highest allowed port for each SoC differs between RX 
and TX, and port allocation is sparse.

Regards,

Stefan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web