[linux-sunxi] Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree
On Mittwoch, 27. September 2017 11:09:22 CEST Maxime Ripard wrote:
> On Sat, Sep 23, 2017 at 12:00:15AM +, Brüns, Stefan wrote:
> > On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:
> > > On Tue, Sep 19, 2017 at 04:17:59PM +, 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 +, 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.
> Using an improper request ID or out of bounds will be just as much as
> a bug. You will not get your DMA transfer to the proper device you
> were trying to, the data will not reach the device or memory, your
> driver will not work => a bug.
>
> It will not be for the same reasons, you will not overwrite other
> registers, but the end result is just the same: your
[linux-sunxi] Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller
On Mittwoch, 20. September 2017 22:53:00 CEST Rob Herring wrote:
> On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
> > The A64 is register compatible with the H3, but has a different number
> > of dma channels and request ports.
> >
> > Attach additional properties to the node to allow future reuse of the
> > compatible for controllers with different number of channels/requests.
> >
> > If dma-requests is not specified, the register layout defined maximum
> > of 32 is used.
> >
> > Signed-off-by: Stefan Brüns
> > ---
> >
> > .../devicetree/bindings/dma/sun6i-dma.txt | 26
> > ++ 1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index
> > 98fbe1a5c6dd..6ebc79f95202 100644
> > --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> >
> > @@ -27,6 +27,32 @@ Example:
> > #dma-cells = <1>;
> >
> > };
> >
> > +-
> > - +For A64 DMA controller:
> > +
> > +Required properties:
> > +- compatible: "allwinner,sun50i-a64-dma"
> > +- dma-channels: Number of DMA channels supported by the controller.
> > + Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +- all properties above, i.e. reg, interrupts, clocks, resets and
> > #dma-cells +
> > +Optional properties:
> > +- dma-requests: Number of DMA request signals supported by the
> > controller.
> > + Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +
> > +Example:
> > + dma: dma-controller@01c02000 {
>
> Drop the leading 0. Building dtbs with W=2 will tell you this.
>
> With that,
>
> Acked-by: Rob Herring
The leading 0 was copied from the A31 example just a few lines above. Should I
also correct that one, or should that go in a separate patch?
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 08/10] dmaengine: sun6i: Add support for Allwinner A64 and compatibles
On Montag, 4. September 2017 01:37:58 CEST André Przywara wrote:
> > @@ -1090,6 +1101,7 @@ MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> >
> > static int sun6i_dma_probe(struct platform_device *pdev)
> > {
> >
> > const struct of_device_id *device;
> >
> > + struct device_node *np = pdev->dev.of_node;
>
> Is this some rebase/split artefact?
>
> Cheers,
> Andre.
>
Yes, that one should be in patch 7/10 ...
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree
On Montag, 4. September 2017 01:44:54 CEST André Przywara wrote:
> Hi,
>
> On 03/09/17 23:40, Stefan Brüns wrote:
> > To avoid introduction of a new compatible for each small SoC/DMA
> > controller
> > variation, move the definition of the channel count to the devicetree.
> >
> > The number of vchans is no longer explicit, but limited by the highest
> > port/DMA request number. The result is a slight overallocation for SoCs
> > with a sparse port mapping.
> >
> > Signed-off-by: Stefan Brüns
> > ---
> >
> > drivers/dma/sun6i-dma.c | 35 ++-
> > 1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index c69dadb853d2..bd4c2e4a759b 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -42,6 +42,9 @@
> >
> > #define DMA_STAT 0x30
> >
> > +/* Offset between DMA_IRQ_EN and DMA_IRQ_STAT limits number of channels
> > */
> > +#define DMA_MAX_CHANNELS (DMA_IRQ_CHAN_NR * 0x10 / 4)
> > +
> >
> > /*
> >
> > * sun8i specific registers
> > */
> >
> > @@ -65,7 +68,8 @@
> >
> > #define DMA_CHAN_LLI_ADDR 0x08
> >
> > #define DMA_CHAN_CUR_CFG 0x0c
> >
> > -#define DMA_CHAN_CFG_SRC_DRQ(x)((x) & 0x1f)
> > +#define DMA_CHAN_MAX_DRQ 0x1f
> > +#define DMA_CHAN_CFG_SRC_DRQ(x)((x) & DMA_CHAN_MAX_DRQ)
> >
> > #define DMA_CHAN_CFG_SRC_IO_MODE BIT(5)
> > #define DMA_CHAN_CFG_SRC_LINEAR_MODE (0 << 5)
> > #define DMA_CHAN_CFG_SRC_BURST_A31(x) (((x) & 0x3) << 7)
> >
> > @@ -1173,6 +1177,35 @@ static int sun6i_dma_probe(struct platform_device
> > *pdev)>
> > sdc->num_vchans = sdc->cfg->nr_max_vchans;
> > sdc->max_request = sdc->cfg->nr_max_requests;
> >
> > + 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);
>
> Mmmh, is this mapping of "!sdc->max_request" -> DMA_CHAN_MAX_DRQ
> implemented somewhere else? Or is it just missing here:
> sdc->max_request = DMA_CHAN_MAX_DRQ;
Well spotted, that assignment is actually missing.
With this line added, your comment for patch 8/10 should also be addressed
(regarding the default value).
> Otherwise this is looking good, thanks for picking up the DT property
> approach!
>
> Cheers,
> Andre.
>
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64
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
--
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64
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+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3
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;
> > + ds
[linux-sunxi] Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64
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 = ;
> 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
--
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
