Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752176AbdIAOmw convert rfc822-to-8bit (ORCPT ); Fri, 1 Sep 2017 10:42:52 -0400 Received: from mail-out-2.itc.rwth-aachen.de ([134.130.5.47]:8211 "EHLO mail-out-2.itc.rwth-aachen.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751995AbdIAOmu (ORCPT ); Fri, 1 Sep 2017 10:42:50 -0400 X-IronPort-AV: E=Sophos;i="5.41,458,1498514400"; d="scan'208";a="11402815" From: =?iso-8859-1?Q?Br=FCns=2C_Stefan?= To: Maxime Ripard CC: linux-sunxi , "devicetree@vger.kernel.org" , "dmaengine@vger.kernel.org" , "vinod.koul@intel.com" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Chen-Yu Tsai , Rob Herring Subject: Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 Thread-Topic: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3 Thread-Index: AQHTIymuFy/HxNm7v0eY+51WgUeuOKKf+TyA Date: Fri, 1 Sep 2017 14:42:47 +0000 Message-ID: <5722405.lpx0YHbn2k@sbruens-linux> References: <20170830233609.13855-1-stefan.bruens@rwth-aachen.de> <20170901133549.cr2ivmfr5mnrdujg@flea> In-Reply-To: <20170901133549.cr2ivmfr5mnrdujg@flea> Accept-Language: en-US, de-DE Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [78.35.13.203] Content-Type: text/plain; charset="iso-8859-1" Content-ID: Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2562 Lines: 65 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