Subject: Re: [PATCH 3/3] dmaengine: sun6i: Add support for Allwinner A64
To: Maxime Ripard <maxime.ripard@free-electrons.com>
References: <20170830233609.13855-4-stefan.bruens@rwth-aachen.de>
 <20170901003135.10058-1-andre.przywara@arm.com>
 <20170901060445.vboici7qxfkztp3s@flea>
Cc: =?UTF-8?Q?Stefan_Br=c3=bcns?= <stefan.bruens@rwth-aachen.de>,
        linux-sunxi@googlegroups.com, Chen-Yu Tsai <wens@csie.org>,
        devicetree@vger.kernel.org, dmaengine@vger.kernel.org,
        Vinod Koul <vinod.koul@intel.com>, Rob Herring <robh+dt@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
From: =?UTF-8?Q?Andr=c3=a9_Przywara?= <andre.przywara@arm.com>
Organization: ARM Ltd.
Message-ID: <743ae23a-372a-762b-c345-b914f09fd718@arm.com>
Date: Fri, 1 Sep 2017 23:35:40 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170901060445.vboici7qxfkztp3s@flea>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2968
Lines: 73

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.