Message-ID: <d397d87b-5004-c8af-e590-037befc9b2b6@arm.com>
Date:   Fri, 1 Sep 2017 23:32:50 +0100
From:   André Przywara <andre.przywara@....com>
To:     Stefan Bruens <stefan.bruens@...h-aachen.de>
Cc:     linux-sunxi@...glegroups.com,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Chen-Yu Tsai <wens@...e.org>, devicetree@...r.kernel.org,
        dmaengine@...r.kernel.org, Vinod Koul <vinod.koul@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64

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@...02000 {
>> 		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.

Powered by blists - more mailing lists