[{"id":1761081,"web_url":"http://patchwork.ozlabs.org/comment/1761081/","msgid":"<20170831145135.c6a2wubcf6xu34tz@flea.lan>","list_archive_url":null,"date":"2017-08-31T14:51:35","subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"Hi,\n\nOn Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:\n> +/* Between SoC generations, there are some significant differences:\n> + * - A23 added a clock gate register\n> + * - the H3 burst length field has a different offset\n> + */\n\nThis is not the proper comment style.\n\n> +enum dmac_variant {\n> +\tDMAC_VARIANT_A31,\n> +\tDMAC_VARIANT_A23,\n> +\tDMAC_VARIANT_H3,\n> +};\n> +\n\nAnd this is redundant with what we already have in our structures.\n\n>  /*\n>   * Hardware channels / ports representation\n>   *\n> @@ -101,6 +116,7 @@ struct sun6i_dma_config {\n>  \tu32 nr_max_channels;\n>  \tu32 nr_max_requests;\n>  \tu32 nr_max_vchans;\n> +\tenum dmac_variant dmac_variant;\n>  };\n>  \n>  /*\n> @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst)\n>  \tswitch (maxburst) {\n>  \tcase 1:\n>  \t\treturn 0;\n> +\tcase 4:\n> +\t\treturn 1;\n>  \tcase 8:\n>  \t\treturn 2;\n> +\tcase 16:\n> +\t\treturn 3;\n>  \tdefault:\n>  \t\treturn -EINVAL;\n>  \t}\n> @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst)\n>  \n>  static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)\n>  {\n> -\tif ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) ||\n> -\t    (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES))\n> -\t\treturn -EINVAL;\n> -\n> -\treturn addr_width >> 1;\n> +\treturn ilog2(addr_width);\n>  }\n\nThis isn't really the same operation. There should be some explanation\nabout why it's the right thing to do.\n\n>  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)\n> @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev,\n>  \t\t\tenum dma_transfer_direction direction,\n>  \t\t\tu32 *p_cfg)\n>  {\n> +\tenum dma_slave_buswidth src_addr_width, dst_addr_width;\n> +\tu32 src_maxburst, dst_maxburst, supported_burst_length;\n>  \ts8 src_width, dst_width, src_burst, dst_burst;\n>  \n> +\tsrc_addr_width = sconfig->src_addr_width;\n> +\tdst_addr_width = sconfig->dst_addr_width;\n> +\tsrc_maxburst = sconfig->src_maxburst;\n> +\tdst_maxburst = sconfig->dst_maxburst;\n> +\n> +\tif (sdev->cfg->dmac_variant == DMAC_VARIANT_H3)\n> +\t\tsupported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16);\n> +\telse\n> +\t\tsupported_burst_length = BIT(1) | BIT(8);\n\nThis could be stored in the structure for example.\n\n>  \tswitch (direction) {\n>  \tcase DMA_MEM_TO_DEV:\n> -\t\tsrc_burst = convert_burst(sconfig->src_maxburst ?\n> -\t\t\t\t\tsconfig->src_maxburst : 8);\n> -\t\tsrc_width = convert_buswidth(sconfig->src_addr_width !=\n> -\t\t\t\t\t\tDMA_SLAVE_BUSWIDTH_UNDEFINED ?\n> -\t\t\t\tsconfig->src_addr_width :\n> -\t\t\t\tDMA_SLAVE_BUSWIDTH_4_BYTES);\n> -\t\tdst_burst = convert_burst(sconfig->dst_maxburst);\n> -\t\tdst_width = convert_buswidth(sconfig->dst_addr_width);\n> +\t\tif (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)\n> +\t\t\tsrc_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;\n> +\t\tsrc_maxburst = src_maxburst ? src_maxburst : 8;\n>  \t\tbreak;\n>  \tcase DMA_DEV_TO_MEM:\n> -\t\tsrc_burst = convert_burst(sconfig->src_maxburst);\n> -\t\tsrc_width = convert_buswidth(sconfig->src_addr_width);\n> -\t\tdst_burst = convert_burst(sconfig->dst_maxburst ?\n> -\t\t\t\t\tsconfig->dst_maxburst : 8);\n> -\t\tdst_width = convert_buswidth(sconfig->dst_addr_width !=\n> -\t\t\t\t\t\tDMA_SLAVE_BUSWIDTH_UNDEFINED ?\n> -\t\t\t\tsconfig->dst_addr_width :\n> -\t\t\t\tDMA_SLAVE_BUSWIDTH_4_BYTES);\n> +\t\tif (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)\n> +\t\t\tdst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;\n> +\t\tdst_maxburst = dst_maxburst ? dst_maxburst : 8;\n>  \t\tbreak;\n>  \tdefault:\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> -\tif (src_burst < 0)\n> -\t\treturn src_burst;\n> -\tif (src_width < 0)\n> -\t\treturn src_width;\n> -\tif (dst_burst < 0)\n> -\t\treturn dst_burst;\n> -\tif (dst_width < 0)\n> -\t\treturn dst_width;\n> -\n> -\t*p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) |\n> -\t\tDMA_CHAN_CFG_SRC_WIDTH(src_width) |\n> -\t\tDMA_CHAN_CFG_DST_BURST(dst_burst) |\n> +\tif (!(BIT(src_addr_width) & sdev->slave.src_addr_widths))\n> +\t\treturn -EINVAL;\n> +\tif (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths))\n> +\t\treturn -EINVAL;\n> +\tif (!(BIT(src_maxburst) & supported_burst_length))\n> +\t\treturn -EINVAL;\n> +\tif (!(BIT(dst_maxburst) & supported_burst_length))\n> +\t\treturn -EINVAL;\n> +\n> +\tsrc_width = convert_buswidth(src_addr_width);\n> +\tdst_width = convert_buswidth(dst_addr_width);\n> +\tdst_burst = convert_burst(dst_maxburst);\n> +\tsrc_burst = convert_burst(src_maxburst);\n\nI'm not sure what you're trying to do here. Could you split your patch\nby logical change, this doesn't seem related to just supporting the\nH3, but a heavier refactoring.\n\nMaxime","headers":{"Return-Path":"<linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org>","X-Original-To":"incoming-imx@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-imx@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.infradead.org\n\t(client-ip=65.50.211.133; helo=bombadil.infradead.org;\n\tenvelope-from=linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org\n\theader.b=\"PfP/kYjg\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xjlhc5Hv9z9s75\n\tfor <incoming-imx@patchwork.ozlabs.org>;\n\tFri,  1 Sep 2017 00:52:15 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dnQpI-0001oi-FN; Thu, 31 Aug 2017 14:52:12 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dnQpD-0001mw-GX for linux-arm-kernel@lists.infradead.org;\n\tThu, 31 Aug 2017 14:52:09 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 8316A208B8; Thu, 31 Aug 2017 16:51:44 +0200 (CEST)","from localhost (LStLambert-657-1-97-87.w90-63.abo.wanadoo.fr\n\t[90.63.216.87])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 5020020882;\n\tThu, 31 Aug 2017 16:51:34 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type:Cc:\n\tList-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:\n\tIn-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To:\n\tContent-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:\n\tResent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner;\n\tbh=PnPVPY+K7pVFaIVLZyRB/2dzhmghy7HV5tuJ4YcqYUs=;\n\tb=PfP/kYjgtmN7KhoQQ9psBAtZJ\n\tIS1sNJomTvPBgPWes+GkPW6yprjqBQsB87B5kAswNySpY0U1iItbG6BFalr35gtXbaohU0FJIg4mB\n\t+vupB9m78FBUmvYLFqE5wk3YD6jsR07/UdoKYQcfZYBjsT7fg7Ya2lm7uqvbrYdBcaqTQDDe0BbYa\n\tDF4oyBdAQZFJmJbx87vDp3Qs24QDAT/8vT4KJizTqX+mM2yxy0lzqbClHCDz9Uuet9XCpgzH48OnI\n\t2WDpmcBy5Y1we3OleYnJ0jR0b0wLA2AwD9q9ZU6m1Ua7UoV/VDhSH3Winn50C7pLLw3gKFSUry4Ks\n\tvHC4Fwdow==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT\n\tshortcircuit=ham autolearn=disabled version=3.4.0","Date":"Thu, 31 Aug 2017 16:51:35 +0200","From":"Maxime Ripard <maxime.ripard@free-electrons.com>","To":"Stefan =?iso-8859-1?q?Br=FCns?= <stefan.bruens@rwth-aachen.de>","Subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","Message-ID":"<20170831145135.c6a2wubcf6xu34tz@flea.lan>","References":"<20170830233609.13855-1-stefan.bruens@rwth-aachen.de>\n\t<20170830233609.13855-2-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","In-Reply-To":"<20170830233609.13855-2-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170831_075207_853975_3F250FE0 ","X-CRM114-Status":"GOOD (  21.11  )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details:   (-1.9 points)\n\tpts rule name              description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS               SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00               BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-arm-kernel@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Unsubscribe":"<http://lists.infradead.org/mailman/options/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=unsubscribe>","List-Archive":"<http://lists.infradead.org/pipermail/linux-arm-kernel/>","List-Post":"<mailto:linux-arm-kernel@lists.infradead.org>","List-Help":"<mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>","List-Subscribe":"<http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>","Cc":"devicetree@vger.kernel.org, Vinod Koul <vinod.koul@intel.com>,\n\tlinux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com,\n\tRob Herring <robh+dt@kernel.org>, dmaengine@vger.kernel.org,\n\tChen-Yu Tsai <wens@csie.org>, linux-arm-kernel@lists.infradead.org","Content-Type":"multipart/mixed;\n\tboundary=\"===============2948027022804678369==\"","Sender":"\"linux-arm-kernel\" <linux-arm-kernel-bounces@lists.infradead.org>","Errors-To":"linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org","List-Id":"linux-imx-kernel.lists.patchwork.ozlabs.org"}},{"id":1761448,"web_url":"http://patchwork.ozlabs.org/comment/1761448/","msgid":"<a649f98d4ac44abc873c5110ce9363e5@rwthex-w2-b.rwth-ad.de>","list_archive_url":null,"date":"2017-09-01T03:04:54","subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","submitter":{"id":67055,"url":"http://patchwork.ozlabs.org/api/people/67055/","name":"Stefan Brüns","email":"stefan.bruens@rwth-aachen.de"},"content":"On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:\n> Hi,\n> \n> On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:\n> > +/* Between SoC generations, there are some significant differences:\n> > + * - A23 added a clock gate register\n> > + * - the H3 burst length field has a different offset\n> > + */\n> \n> This is not the proper comment style.\n> \n> > +enum dmac_variant {\n> > +\tDMAC_VARIANT_A31,\n> > +\tDMAC_VARIANT_A23,\n> > +\tDMAC_VARIANT_H3,\n> > +};\n> > +\n> \n> And this is redundant with what we already have in our structures.\n\nActually, its not. For H3, there are currently at least 3 register compatible \nSoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the \ncurrent config structure is kept, we need 3 different compatible strings. Same \nfor the A23, which is register compatible to e.g. A83t and V3s, but with \ndifferent numbers of DMA channels.\n\nSo either you decorate the code with a cascade of\n\nif ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {\n} else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) {\n} else { /* A31 */\n}\n\nin a number of places, or you do it just once.\n\n> \n> >  /*\n> >  \n> >   * Hardware channels / ports representation\n> >   *\n> > \n> > @@ -101,6 +116,7 @@ struct sun6i_dma_config {\n> > \n> >  \tu32 nr_max_channels;\n> >  \tu32 nr_max_requests;\n> >  \tu32 nr_max_vchans;\n> > \n> > +\tenum dmac_variant dmac_variant;\n> > \n> >  };\n> >  \n> >  /*\n> > \n> > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst)\n> > \n> >  \tswitch (maxburst) {\n> >  \t\n> >  \tcase 1:\n> >  \t\treturn 0;\n> > \n> > +\tcase 4:\n> > +\t\treturn 1;\n> > \n> >  \tcase 8:\n> >  \t\treturn 2;\n> > \n> > +\tcase 16:\n> > +\t\treturn 3;\n> > \n> >  \tdefault:\n> >  \t\treturn -EINVAL;\n> >  \t\n> >  \t}\n> > \n> > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst)\n> > \n> >  static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)\n> >  {\n> > \n> > -\tif ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) ||\n> > -\t    (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES))\n> > -\t\treturn -EINVAL;\n> > -\n> > -\treturn addr_width >> 1;\n> > +\treturn ilog2(addr_width);\n> > \n> >  }\n> \n> This isn't really the same operation. There should be some explanation\n> about why it's the right thing to do.\n\nFor 1, 2 and 4 it is the same. The correct register value for 8 bytes, \nsupported by H3, H5, A64 and R40, is 3.\n\n> \n> >  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)\n> > \n> > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev,\n> > \n> >  \t\t\tenum dma_transfer_direction direction,\n> >  \t\t\tu32 *p_cfg)\n> >  \n> >  {\n> > \n> > +\tenum dma_slave_buswidth src_addr_width, dst_addr_width;\n> > +\tu32 src_maxburst, dst_maxburst, supported_burst_length;\n> > \n> >  \ts8 src_width, dst_width, src_burst, dst_burst;\n> > \n> > +\tsrc_addr_width = sconfig->src_addr_width;\n> > +\tdst_addr_width = sconfig->dst_addr_width;\n> > +\tsrc_maxburst = sconfig->src_maxburst;\n> > +\tdst_maxburst = sconfig->dst_maxburst;\n> > +\n> > +\tif (sdev->cfg->dmac_variant == DMAC_VARIANT_H3)\n> > +\t\tsupported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16);\n> > +\telse\n> > +\t\tsupported_burst_length = BIT(1) | BIT(8);\n> \n> This could be stored in the structure for example.\n\nWhich one? sun6i_dma_dev? sun6i_dma_config?\n \n> >  \tswitch (direction) {\n> > \n> >  \tcase DMA_MEM_TO_DEV:\n> > -\t\tsrc_burst = convert_burst(sconfig->src_maxburst ?\n> > -\t\t\t\t\tsconfig->src_maxburst : 8);\n> > -\t\tsrc_width = convert_buswidth(sconfig->src_addr_width !=\n> > -\t\t\t\t\t\tDMA_SLAVE_BUSWIDTH_UNDEFINED ?\n> > -\t\t\t\tsconfig->src_addr_width :\n> > -\t\t\t\tDMA_SLAVE_BUSWIDTH_4_BYTES);\n> > -\t\tdst_burst = convert_burst(sconfig->dst_maxburst);\n> > -\t\tdst_width = convert_buswidth(sconfig->dst_addr_width);\n> > +\t\tif (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)\n> > +\t\t\tsrc_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;\n> > +\t\tsrc_maxburst = src_maxburst ? src_maxburst : 8;\n> > \n> >  \t\tbreak;\n> >  \t\n> >  \tcase DMA_DEV_TO_MEM:\n> > -\t\tsrc_burst = convert_burst(sconfig->src_maxburst);\n> > -\t\tsrc_width = convert_buswidth(sconfig->src_addr_width);\n> > -\t\tdst_burst = convert_burst(sconfig->dst_maxburst ?\n> > -\t\t\t\t\tsconfig->dst_maxburst : 8);\n> > -\t\tdst_width = convert_buswidth(sconfig->dst_addr_width !=\n> > -\t\t\t\t\t\tDMA_SLAVE_BUSWIDTH_UNDEFINED ?\n> > -\t\t\t\tsconfig->dst_addr_width :\n> > -\t\t\t\tDMA_SLAVE_BUSWIDTH_4_BYTES);\n> > +\t\tif (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)\n> > +\t\t\tdst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;\n> > +\t\tdst_maxburst = dst_maxburst ? dst_maxburst : 8;\n> > \n> >  \t\tbreak;\n> >  \t\n> >  \tdefault:\n> >  \t\treturn -EINVAL;\n> >  \t\n> >  \t}\n> > \n> > -\tif (src_burst < 0)\n> > -\t\treturn src_burst;\n> > -\tif (src_width < 0)\n> > -\t\treturn src_width;\n> > -\tif (dst_burst < 0)\n> > -\t\treturn dst_burst;\n> > -\tif (dst_width < 0)\n> > -\t\treturn dst_width;\n> > -\n> > -\t*p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) |\n> > -\t\tDMA_CHAN_CFG_SRC_WIDTH(src_width) |\n> > -\t\tDMA_CHAN_CFG_DST_BURST(dst_burst) |\n> > +\tif (!(BIT(src_addr_width) & sdev->slave.src_addr_widths))\n> > +\t\treturn -EINVAL;\n> > +\tif (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths))\n> > +\t\treturn -EINVAL;\n> > +\tif (!(BIT(src_maxburst) & supported_burst_length))\n> > +\t\treturn -EINVAL;\n> > +\tif (!(BIT(dst_maxburst) & supported_burst_length))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tsrc_width = convert_buswidth(src_addr_width);\n> > +\tdst_width = convert_buswidth(dst_addr_width);\n> > +\tdst_burst = convert_burst(dst_maxburst);\n> > +\tsrc_burst = convert_burst(src_maxburst);\n> \n> I'm not sure what you're trying to do here. Could you split your patch\n> by logical change, this doesn't seem related to just supporting the\n> H3, but a heavier refactoring.\n\nUntangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, \nrange checking, and conversion to register value. As the valid ranges depend \non the controller, the code is much easier to read if the range check is done \nfirst, and then the conversion.\n\nKind regards,\n\nStefan","headers":{"Return-Path":"<linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org>","X-Original-To":"incoming-imx@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-imx@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.infradead.org\n\t(client-ip=65.50.211.133; helo=bombadil.infradead.org;\n\tenvelope-from=linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org\n\theader.b=\"OyYgxOmO\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xk3yg1Kz1z9s83\n\tfor <incoming-imx@patchwork.ozlabs.org>;\n\tFri,  1 Sep 2017 13:05:31 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dncGr-0006HU-Ol; Fri, 01 Sep 2017 03:05:25 +0000","from mail-out-2.itc.rwth-aachen.de ([134.130.5.47])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dncGm-0004s6-Le for linux-arm-kernel@lists.infradead.org;\n\tFri, 01 Sep 2017 03:05:22 +0000","from rwthex-w2-b.rwth-ad.de ([134.130.26.159])\n\tby mail-in-2.itc.rwth-aachen.de with ESMTP; 01 Sep 2017 05:04:58 +0200","from pebbles.site (78.49.70.254) by rwthex-w2-b.rwth-ad.de\n\t(2002:8682:1a9f::8682:1a9f) with Microsoft SMTP Server (TLS) id\n\t15.0.1320.4; Fri, 1 Sep 2017 05:04:55 +0200"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:Message-ID:MIME-Version:References:\n\tIn-Reply-To:Date:Subject:To:From:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=3qNx6DBZnjKPgQpevHN5OQ68o9vNy5focMDQJJIm+I0=;\n\tb=OyYgxOmOUVT8lk\n\tz1kEyYDKtBqSV4gDI1jh5bzhO0hXRiEPZg0YytJFzIdn4w3ywZHbHjKBgslkGnJH2cVTmnjjbAd0y\n\tIivTrtsH7Qis8+FhbSjtFTRCKzAx8ugNFzGgC48JGp2prw3L20w+MoAYQT7voZRgGgmYVUeGnCIgw\n\t3Zj9LW+kZ8jKZo/TRSOzQTug8iXVOjDA+JYiohbqXGCfFGv1zp3wsCVLUgl37y2EsPA/bST10a6Jc\n\tyY2UcK3/7uR8XKwjBb3037s3kxJaxEq0jmLZtEoIB2s/TRJhwlTWBSyRFePg1ihHMlu5cJOD/kBy7\n\tNMXqkag4+Fvrir4Kov4Q==;","X-IronPort-AV":"E=Sophos;i=\"5.41,456,1498514400\"; d=\"scan'208\";a=\"11235768\"","From":"Stefan Bruens <stefan.bruens@rwth-aachen.de>","To":"Maxime Ripard <maxime.ripard@free-electrons.com>","Subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","Date":"Fri, 1 Sep 2017 05:04:54 +0200","In-Reply-To":"<20170831145135.c6a2wubcf6xu34tz@flea.lan>","References":"<20170830233609.13855-1-stefan.bruens@rwth-aachen.de>\n\t<20170830233609.13855-2-stefan.bruens@rwth-aachen.de>\n\t<20170831145135.c6a2wubcf6xu34tz@flea.lan>","MIME-Version":"1.0","X-Originating-IP":"[78.49.70.254]","X-ClientProxiedBy":"rwthex-s1-a.rwth-ad.de (2002:8682:1a98::8682:1a98) To\n\trwthex-w2-b.rwth-ad.de (2002:8682:1a9f::8682:1a9f)","Message-ID":"<a649f98d4ac44abc873c5110ce9363e5@rwthex-w2-b.rwth-ad.de>","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170831_200521_167955_0248F2D7 ","X-CRM114-Status":"GOOD (  21.33  )","X-Spam-Score":"-4.2 (----)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details:   (-4.2 points)\n\tpts rule name              description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\n\tmedium trust [134.130.5.47 listed in list.dnswl.org]\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00               BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-arm-kernel@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Unsubscribe":"<http://lists.infradead.org/mailman/options/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=unsubscribe>","List-Archive":"<http://lists.infradead.org/pipermail/linux-arm-kernel/>","List-Post":"<mailto:linux-arm-kernel@lists.infradead.org>","List-Help":"<mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>","List-Subscribe":"<http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>","Cc":"devicetree@vger.kernel.org, vinod.koul@intel.com,\n\tlinux-kernel@vger.kernel.org,\n\tlinux-sunxi <linux-sunxi@googlegroups.com>, Rob Herring\n\t<robh+dt@kernel.org>, dmaengine@vger.kernel.org, Chen-Yu Tsai\n\t<wens@csie.org>, Stefan =?iso-8859-1?q?Br=FCns?=\n\t<stefan.bruens@rwth-aachen.de>,  linux-arm-kernel@lists.infradead.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Sender":"\"linux-arm-kernel\" <linux-arm-kernel-bounces@lists.infradead.org>","Errors-To":"linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org","List-Id":"linux-imx-kernel.lists.patchwork.ozlabs.org"}},{"id":1761689,"web_url":"http://patchwork.ozlabs.org/comment/1761689/","msgid":"<20170901133549.cr2ivmfr5mnrdujg@flea>","list_archive_url":null,"date":"2017-09-01T13:35:49","subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote:\n> On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:\n> > Hi,\n> > \n> > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:\n> > > +/* Between SoC generations, there are some significant differences:\n> > > + * - A23 added a clock gate register\n> > > + * - the H3 burst length field has a different offset\n> > > + */\n> > \n> > This is not the proper comment style.\n> > \n> > > +enum dmac_variant {\n> > > +\tDMAC_VARIANT_A31,\n> > > +\tDMAC_VARIANT_A23,\n> > > +\tDMAC_VARIANT_H3,\n> > > +};\n> > > +\n> > \n> > And this is redundant with what we already have in our structures.\n> \n> Actually, its not. For H3, there are currently at least 3 register compatible \n> SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8 channels. So if the \n> current config structure is kept, we need 3 different compatible strings. Same \n> for the A23, which is register compatible to e.g. A83t and V3s, but with \n> different numbers of DMA channels.\n> \n> So either you decorate the code with a cascade of\n> \n> if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {\n> } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...) {\n> } else { /* A31 */\n> }\n> \n> in a number of places, or you do it just once.\n\nThat's not how you retrieve the structures. They are already\nassociated to the compatible, and you need to do a single lookup to\nget them. So that's nowhere near what you're suggesting. You can have\na look at the of_match_device in the probe function.\n\n> \n> > \n> > >  /*\n> > >  \n> > >   * Hardware channels / ports representation\n> > >   *\n> > > \n> > > @@ -101,6 +116,7 @@ struct sun6i_dma_config {\n> > > \n> > >  \tu32 nr_max_channels;\n> > >  \tu32 nr_max_requests;\n> > >  \tu32 nr_max_vchans;\n> > > \n> > > +\tenum dmac_variant dmac_variant;\n> > > \n> > >  };\n> > >  \n> > >  /*\n> > > \n> > > @@ -240,8 +256,12 @@ static inline s8 convert_burst(u32 maxburst)\n> > > \n> > >  \tswitch (maxburst) {\n> > >  \t\n> > >  \tcase 1:\n> > >  \t\treturn 0;\n> > > \n> > > +\tcase 4:\n> > > +\t\treturn 1;\n> > > \n> > >  \tcase 8:\n> > >  \t\treturn 2;\n> > > \n> > > +\tcase 16:\n> > > +\t\treturn 3;\n> > > \n> > >  \tdefault:\n> > >  \t\treturn -EINVAL;\n> > >  \t\n> > >  \t}\n> > > \n> > > @@ -249,11 +269,7 @@ static inline s8 convert_burst(u32 maxburst)\n> > > \n> > >  static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)\n> > >  {\n> > > \n> > > -\tif ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) ||\n> > > -\t    (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES))\n> > > -\t\treturn -EINVAL;\n> > > -\n> > > -\treturn addr_width >> 1;\n> > > +\treturn ilog2(addr_width);\n> > > \n> > >  }\n> > \n> > This isn't really the same operation. There should be some explanation\n> > about why it's the right thing to do.\n> \n> For 1, 2 and 4 it is the same. The correct register value for 8 bytes, \n> supported by H3, H5, A64 and R40, is 3.\n\nThat should be in a separate patch, with that in the commit log.\n\n> > >  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)\n> > > \n> > > @@ -499,45 +515,58 @@ static int set_config(struct sun6i_dma_dev *sdev,\n> > > \n> > >  \t\t\tenum dma_transfer_direction direction,\n> > >  \t\t\tu32 *p_cfg)\n> > >  \n> > >  {\n> > > \n> > > +\tenum dma_slave_buswidth src_addr_width, dst_addr_width;\n> > > +\tu32 src_maxburst, dst_maxburst, supported_burst_length;\n> > > \n> > >  \ts8 src_width, dst_width, src_burst, dst_burst;\n> > > \n> > > +\tsrc_addr_width = sconfig->src_addr_width;\n> > > +\tdst_addr_width = sconfig->dst_addr_width;\n> > > +\tsrc_maxburst = sconfig->src_maxburst;\n> > > +\tdst_maxburst = sconfig->dst_maxburst;\n> > > +\n> > > +\tif (sdev->cfg->dmac_variant == DMAC_VARIANT_H3)\n> > > +\t\tsupported_burst_length = BIT(1) | BIT(4) | BIT(8) | BIT(16);\n> > > +\telse\n> > > +\t\tsupported_burst_length = BIT(1) | BIT(8);\n> > \n> > This could be stored in the structure for example.\n> \n> Which one? sun6i_dma_dev? sun6i_dma_config?\n\nThe one associated with the compatible, so sun6i_dma_config.\n\n>  \n> > >  \tswitch (direction) {\n> > > \n> > >  \tcase DMA_MEM_TO_DEV:\n> > > -\t\tsrc_burst = convert_burst(sconfig->src_maxburst ?\n> > > -\t\t\t\t\tsconfig->src_maxburst : 8);\n> > > -\t\tsrc_width = convert_buswidth(sconfig->src_addr_width !=\n> > > -\t\t\t\t\t\tDMA_SLAVE_BUSWIDTH_UNDEFINED ?\n> > > -\t\t\t\tsconfig->src_addr_width :\n> > > -\t\t\t\tDMA_SLAVE_BUSWIDTH_4_BYTES);\n> > > -\t\tdst_burst = convert_burst(sconfig->dst_maxburst);\n> > > -\t\tdst_width = convert_buswidth(sconfig->dst_addr_width);\n> > > +\t\tif (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)\n> > > +\t\t\tsrc_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;\n> > > +\t\tsrc_maxburst = src_maxburst ? src_maxburst : 8;\n> > > \n> > >  \t\tbreak;\n> > >  \t\n> > >  \tcase DMA_DEV_TO_MEM:\n> > > -\t\tsrc_burst = convert_burst(sconfig->src_maxburst);\n> > > -\t\tsrc_width = convert_buswidth(sconfig->src_addr_width);\n> > > -\t\tdst_burst = convert_burst(sconfig->dst_maxburst ?\n> > > -\t\t\t\t\tsconfig->dst_maxburst : 8);\n> > > -\t\tdst_width = convert_buswidth(sconfig->dst_addr_width !=\n> > > -\t\t\t\t\t\tDMA_SLAVE_BUSWIDTH_UNDEFINED ?\n> > > -\t\t\t\tsconfig->dst_addr_width :\n> > > -\t\t\t\tDMA_SLAVE_BUSWIDTH_4_BYTES);\n> > > +\t\tif (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)\n> > > +\t\t\tdst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;\n> > > +\t\tdst_maxburst = dst_maxburst ? dst_maxburst : 8;\n> > > \n> > >  \t\tbreak;\n> > >  \t\n> > >  \tdefault:\n> > >  \t\treturn -EINVAL;\n> > >  \t\n> > >  \t}\n> > > \n> > > -\tif (src_burst < 0)\n> > > -\t\treturn src_burst;\n> > > -\tif (src_width < 0)\n> > > -\t\treturn src_width;\n> > > -\tif (dst_burst < 0)\n> > > -\t\treturn dst_burst;\n> > > -\tif (dst_width < 0)\n> > > -\t\treturn dst_width;\n> > > -\n> > > -\t*p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) |\n> > > -\t\tDMA_CHAN_CFG_SRC_WIDTH(src_width) |\n> > > -\t\tDMA_CHAN_CFG_DST_BURST(dst_burst) |\n> > > +\tif (!(BIT(src_addr_width) & sdev->slave.src_addr_widths))\n> > > +\t\treturn -EINVAL;\n> > > +\tif (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths))\n> > > +\t\treturn -EINVAL;\n> > > +\tif (!(BIT(src_maxburst) & supported_burst_length))\n> > > +\t\treturn -EINVAL;\n> > > +\tif (!(BIT(dst_maxburst) & supported_burst_length))\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tsrc_width = convert_buswidth(src_addr_width);\n> > > +\tdst_width = convert_buswidth(dst_addr_width);\n> > > +\tdst_burst = convert_burst(dst_maxburst);\n> > > +\tsrc_burst = convert_burst(src_maxburst);\n> > \n> > I'm not sure what you're trying to do here. Could you split your patch\n> > by logical change, this doesn't seem related to just supporting the\n> > H3, but a heavier refactoring.\n> \n> Untangling 3 independent steps - handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, \n> range checking, and conversion to register value. As the valid ranges depend \n> on the controller, the code is much easier to read if the range check is done \n> first, and then the conversion.\n\nPlease make separate patches as well for the splitting up of each of\nthose steps.\n\nThanks!\nMaxime","headers":{"Return-Path":"<linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org>","X-Original-To":"incoming-imx@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-imx@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.infradead.org\n\t(client-ip=65.50.211.133; helo=bombadil.infradead.org;\n\tenvelope-from=linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org\n\theader.b=\"n3thaLhx\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xkKyX18XXz9s2G\n\tfor <incoming-imx@patchwork.ozlabs.org>;\n\tFri,  1 Sep 2017 23:36:19 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dnm7L-0001Dw-Au; Fri, 01 Sep 2017 13:36:15 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dnm7H-0001Aj-A5 for linux-arm-kernel@lists.infradead.org;\n\tFri, 01 Sep 2017 13:36:13 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 439692098E; Fri,  1 Sep 2017 15:35:49 +0200 (CEST)","from localhost (LStLambert-657-1-97-87.w90-63.abo.wanadoo.fr\n\t[90.63.216.87])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 1202B20989;\n\tFri,  1 Sep 2017 15:35:49 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type:Cc:\n\tList-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:\n\tIn-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To:\n\tContent-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:\n\tResent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner;\n\tbh=XGF9QaB1Js4vlSEMQgtdbFtYehGACkE2+PHEyT7Br5s=;\n\tb=n3thaLhx6ueLOJmqTSNzwsgNO\n\tJcFRhn1K55eO3dJDIU+lAUK0Xu/kf0A7L9tuWoqujaqA7TYSVNeWyy6EytL7OQ9W7CThnhU5ue437\n\trTVZCu3hBTX5saYt28DeVfS/4IOtIS9Hsltrlis1w0WlXUrwbF4QSAH1HS9u7iewYDsrTvYzaZaWH\n\tcnYq11NhJhrxLZvT7h87PtZ9n4Fyk5FiWq79XN8T8yhcRki4iEfLhxKRG9079x+clrk6x2e/bS4vo\n\tCwgPqEgL2BQYQFuCu5rMBwwPNw306fS39ql0MkarkfqkJNL4DMWEhOdrFXw2ctPvWuYX5/mKEur5a\n\tZE+KPlOFw==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT,\n\tURIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.0","Date":"Fri, 1 Sep 2017 15:35:49 +0200","From":"Maxime Ripard <maxime.ripard@free-electrons.com>","To":"Stefan Bruens <stefan.bruens@rwth-aachen.de>","Subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","Message-ID":"<20170901133549.cr2ivmfr5mnrdujg@flea>","References":"<20170830233609.13855-1-stefan.bruens@rwth-aachen.de>\n\t<20170830233609.13855-2-stefan.bruens@rwth-aachen.de>\n\t<20170831145135.c6a2wubcf6xu34tz@flea.lan>\n\t<a649f98d4ac44abc873c5110ce9363e5@rwthex-w2-b.rwth-ad.de>","MIME-Version":"1.0","In-Reply-To":"<a649f98d4ac44abc873c5110ce9363e5@rwthex-w2-b.rwth-ad.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170901_063611_654719_D723E8BD ","X-CRM114-Status":"GOOD (  32.07  )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details:   (-1.9 points)\n\tpts rule name              description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS               SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00               BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-arm-kernel@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Unsubscribe":"<http://lists.infradead.org/mailman/options/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=unsubscribe>","List-Archive":"<http://lists.infradead.org/pipermail/linux-arm-kernel/>","List-Post":"<mailto:linux-arm-kernel@lists.infradead.org>","List-Help":"<mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>","List-Subscribe":"<http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>","Cc":"devicetree@vger.kernel.org, vinod.koul@intel.com,\n\tlinux-kernel@vger.kernel.org,\n\tlinux-sunxi <linux-sunxi@googlegroups.com>, \n\tRob Herring <robh+dt@kernel.org>, dmaengine@vger.kernel.org,\n\tChen-Yu Tsai <wens@csie.org>, linux-arm-kernel@lists.infradead.org","Content-Type":"multipart/mixed;\n\tboundary=\"===============4474586509848040487==\"","Sender":"\"linux-arm-kernel\" <linux-arm-kernel-bounces@lists.infradead.org>","Errors-To":"linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org","List-Id":"linux-imx-kernel.lists.patchwork.ozlabs.org"}},{"id":1761744,"web_url":"http://patchwork.ozlabs.org/comment/1761744/","msgid":"<5722405.lpx0YHbn2k@sbruens-linux>","list_archive_url":null,"date":"2017-09-01T14:42:47","subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","submitter":{"id":67055,"url":"http://patchwork.ozlabs.org/api/people/67055/","name":"Stefan Brüns","email":"stefan.bruens@rwth-aachen.de"},"content":"On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote:\n> On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote:\n> > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:\n> > > Hi,\n> > > \n> > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:\n> > > > +/* Between SoC generations, there are some significant differences:\n> > > > + * - A23 added a clock gate register\n> > > > + * - the H3 burst length field has a different offset\n> > > > + */\n> > > \n> > > This is not the proper comment style.\n> > > \n> > > > +enum dmac_variant {\n> > > > +\tDMAC_VARIANT_A31,\n> > > > +\tDMAC_VARIANT_A23,\n> > > > +\tDMAC_VARIANT_H3,\n> > > > +};\n> > > > +\n> > > \n> > > And this is redundant with what we already have in our structures.\n> > \n> > Actually, its not. For H3, there are currently at least 3 register\n> > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8\n> > channels. So if the current config structure is kept, we need 3 different\n> > compatible strings. Same for the A23, which is register compatible to\n> > e.g. A83t and V3s, but with different numbers of DMA channels.\n> > \n> > So either you decorate the code with a cascade of\n> > \n> > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {\n> > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...)\n> > {\n> > } else { /* A31 */\n> > }\n> > \n> > in a number of places, or you do it just once.\n> \n> That's not how you retrieve the structures. They are already\n> associated to the compatible, and you need to do a single lookup to\n> get them. So that's nowhere near what you're suggesting. You can have\n> a look at the of_match_device in the probe function.\n> \n\n\nPlease have a look at the current implementation of how the clock autogating \nin the probe function is done - it matches with the compatible string.\n\nOf course we can replace this with a match between sdev->config and the \nvarious sun6i_dma_config instances, but we would still have to do 3 matches \nfor the A23 register configuration (A23 || A83T || V3s) and 3 matches for the \nH3 register configuration (H3 || R40 || A64). There are currently *7* \ndifferent configs (V3s, R40 and A64 taken into account), but only 3 different \nregister variants.\n\nThis is the same rationale as the \"gate_needed\" boolean property proposed by \nIcenowy Zheng in the \"Allwinner V3s DMA support\" patch series. Obviously we \ndon't need a boolean, but a ternary option to cater for the gate_needed \ndifferences - \"NO_GATE\", \"A23_STYLE_GATE\", \"H3_STYLE_GATE\".\n\nKind regards,\n\nStefan","headers":{"Return-Path":"<linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org>","X-Original-To":"incoming-imx@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-imx@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.infradead.org\n\t(client-ip=65.50.211.133; helo=bombadil.infradead.org;\n\tenvelope-from=linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org\n\theader.b=\"mWjWOTXE\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xkMRv41F8z9sPk\n\tfor <incoming-imx@patchwork.ozlabs.org>;\n\tSat,  2 Sep 2017 00:43:23 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dnnAE-0002Yi-Rt; Fri, 01 Sep 2017 14:43:18 +0000","from mail-out-2.itc.rwth-aachen.de ([134.130.5.47])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dnnAA-0002W9-L6 for linux-arm-kernel@lists.infradead.org;\n\tFri, 01 Sep 2017 14:43:16 +0000","from rwthex-s2-b.rwth-ad.de ([134.130.26.155])\n\tby mail-in-2.itc.rwth-aachen.de with ESMTP; 01 Sep 2017 16:42:48 +0200","from rwthex-w2-b.rwth-ad.de (2002:8682:1a9f::8682:1a9f) by\n\trwthex-s2-b.rwth-ad.de (2002:8682:1a9b::8682:1a9b) with Microsoft\n\tSMTP Server (TLS) id 15.0.1320.4; Fri, 1 Sep 2017 16:42:48 +0200","from rwthex-w2-b.rwth-ad.de ([fe80::200c:2ee4:85cf:8127]) by\n\trwthex-w2-b.rwth-ad.de ([fe80::200c:2ee4:85cf:8127%21]) with mapi id\n\t15.00.1320.000; Fri, 1 Sep 2017 16:42:47 +0200"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:MIME-Version:Content-ID:In-Reply-To:\n\tReferences:Message-ID:Date:Subject:To:From:Reply-To:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=Th9/GdbCeaXu/RcR59h7HbfZNPHgtC/guVgVrAXVm1E=;\n\tb=mWjWOTXEjbNIk7\n\tKg03pahDSthWDpSGEqeM+PiqaDplRJJ/C1mCxD7iuwkFp9T4GY5/v9mclgIO3USR2DIuY3aP3rH6Q\n\tnLVADY/BwH2i08kgUX8DO7sD6IeH63okDKs6V5zHDMA4EdavI9tT3l5mmUm10EJso9ev6ZrIdBMS8\n\tjcD+4dKUM02kBN7DEMi5w3sb5Qcl45V2jtpM3WZUU1B0acY8hl2bXvJt4rzIiaIsTJ0sAoKJ57Qzy\n\t6yWKpESQThOBquf0Gos9Xs+ue22Za60/MHhq+Nt6Y6WK5zMXbMymaIAcF25dD03lLgoBfGyw/H/6u\n\tRwhPoiGRHPd3t+rfViwg==;","X-IronPort-AV":"E=Sophos;i=\"5.41,458,1498514400\"; d=\"scan'208\";a=\"11402815\"","From":"=?iso-8859-1?q?Br=FCns=2C_Stefan?= <Stefan.Bruens@rwth-aachen.de>","To":"Maxime Ripard <maxime.ripard@free-electrons.com>","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>\n\t<a649f98d4ac44abc873c5110ce9363e5@rwthex-w2-b.rwth-ad.de>\n\t<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-ID":"<A6C847B31318F84DA1549A1C38771119@rwth-ad.de>","MIME-Version":"1.0","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170901_074314_991340_76E3B0E4 ","X-CRM114-Status":"GOOD (  17.59  )","X-Spam-Score":"-4.2 (----)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details:   (-4.2 points)\n\tpts rule name              description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\n\tmedium trust [134.130.5.47 listed in list.dnswl.org]\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00               BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-arm-kernel@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Unsubscribe":"<http://lists.infradead.org/mailman/options/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=unsubscribe>","List-Archive":"<http://lists.infradead.org/pipermail/linux-arm-kernel/>","List-Post":"<mailto:linux-arm-kernel@lists.infradead.org>","List-Help":"<mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>","List-Subscribe":"<http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>","Cc":"\"devicetree@vger.kernel.org\" <devicetree@vger.kernel.org>,\n\t\"vinod.koul@intel.com\" <vinod.koul@intel.com>,\n\t\"linux-kernel@vger.kernel.org\" <linux-kernel@vger.kernel.org>,\n\tlinux-sunxi <linux-sunxi@googlegroups.com>,\n\tRob Herring <robh+dt@kernel.org>, \n\t\"dmaengine@vger.kernel.org\" <dmaengine@vger.kernel.org>,\n\tChen-Yu Tsai <wens@csie.org>, \"linux-arm-kernel@lists.infradead.org\"\n\t<linux-arm-kernel@lists.infradead.org>","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Sender":"\"linux-arm-kernel\" <linux-arm-kernel-bounces@lists.infradead.org>","Errors-To":"linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org","List-Id":"linux-imx-kernel.lists.patchwork.ozlabs.org"}},{"id":1762453,"web_url":"http://patchwork.ozlabs.org/comment/1762453/","msgid":"<20170904065027.7splxlgxhpu5nj56@flea>","list_archive_url":null,"date":"2017-09-04T06:50:27","subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Fri, Sep 01, 2017 at 02:42:47PM +0000, Brüns, Stefan wrote:\n> On Freitag, 1. September 2017 15:35:49 CEST Maxime Ripard wrote:\n> > On Fri, Sep 01, 2017 at 05:04:54AM +0200, Stefan Bruens wrote:\n> > > On Donnerstag, 31. August 2017 16:51:35 CEST Maxime Ripard wrote:\n> > > > Hi,\n> > > > \n> > > > On Thu, Aug 31, 2017 at 01:36:07AM +0200, Stefan Brüns wrote:\n> > > > > +/* Between SoC generations, there are some significant differences:\n> > > > > + * - A23 added a clock gate register\n> > > > > + * - the H3 burst length field has a different offset\n> > > > > + */\n> > > > \n> > > > This is not the proper comment style.\n> > > > \n> > > > > +enum dmac_variant {\n> > > > > +\tDMAC_VARIANT_A31,\n> > > > > +\tDMAC_VARIANT_A23,\n> > > > > +\tDMAC_VARIANT_H3,\n> > > > > +};\n> > > > > +\n> > > > \n> > > > And this is redundant with what we already have in our structures.\n> > > \n> > > Actually, its not. For H3, there are currently at least 3 register\n> > > compatible SoCs: H5 is identical, R40 has 16 dma channels, A64 has 8\n> > > channels. So if the current config structure is kept, we need 3 different\n> > > compatible strings. Same for the A23, which is register compatible to\n> > > e.g. A83t and V3s, but with different numbers of DMA channels.\n> > > \n> > > So either you decorate the code with a cascade of\n> > > \n> > > if ((of_is_compatible(..A23..) || of_is_compatible(..A83T..) || ...) {\n> > > } else if ((of_is_compatible(..H3..) || of_is_compatible(..A64..) || ...)\n> > > {\n> > > } else { /* A31 */\n> > > }\n> > > \n> > > in a number of places, or you do it just once.\n> > \n> > That's not how you retrieve the structures. They are already\n> > associated to the compatible, and you need to do a single lookup to\n> > get them. So that's nowhere near what you're suggesting. You can have\n> > a look at the of_match_device in the probe function.\n> \n> Please have a look at the current implementation of how the clock autogating \n> in the probe function is done - it matches with the compatible string.\n\nYeah, and we should get rid of that as well.\n\n> Of course we can replace this with a match between sdev->config and the \n> various sun6i_dma_config instances, but we would still have to do 3 matches \n> for the A23 register configuration (A23 || A83T || V3s) and 3 matches for the \n> H3 register configuration (H3 || R40 || A64). There are currently *7* \n> different configs (V3s, R40 and A64 taken into account), but only 3 different \n> register variants.\n> \n> This is the same rationale as the \"gate_needed\" boolean property proposed by \n> Icenowy Zheng in the \"Allwinner V3s DMA support\" patch series. Obviously we \n> don't need a boolean, but a ternary option to cater for the gate_needed \n> differences - \"NO_GATE\", \"A23_STYLE_GATE\", \"H3_STYLE_GATE\".\n\nOr we can just have an extra field in sun6i_dma_config that would set\nthe gate register offset? If it's non-zero, use whatever you have set\nthere, and then you just have to care about two cases, no matter how\nmany offsets we'll have in the wild.\n\nMaxime","headers":{"Return-Path":"<linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org>","X-Original-To":"incoming-imx@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-imx@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.infradead.org\n\t(client-ip=65.50.211.133; helo=bombadil.infradead.org;\n\tenvelope-from=linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org\n\theader.b=\"kdmCptYv\"; dkim-atps=neutral"],"Received":["from bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xm0qb22YYz9s78\n\tfor <incoming-imx@patchwork.ozlabs.org>;\n\tMon,  4 Sep 2017 16:51:07 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dolDr-0005e2-N8; Mon, 04 Sep 2017 06:51:03 +0000","from mail.free-electrons.com ([62.4.15.54])\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dolDn-0005Xx-87 for linux-arm-kernel@lists.infradead.org;\n\tMon, 04 Sep 2017 06:51:01 +0000","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 2C9F221EC4; Mon,  4 Sep 2017 08:50:37 +0200 (CEST)","from localhost (LStLambert-657-1-97-87.w90-63.abo.wanadoo.fr\n\t[90.63.216.87])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id EFE8621EA0;\n\tMon,  4 Sep 2017 08:50:26 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type:Cc:\n\tList-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:\n\tIn-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To:\n\tContent-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:\n\tResent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner;\n\tbh=7dvoLfszkNvofgXD4L4yMlMDjLRP10ENYYBY6d87kG0=;\n\tb=kdmCptYv7atWTxymMIDoTdMOl\n\tMiPnpeBt3JehEMF7B+I+A0uUfoQNSfSOBwGXtw8DDpBVZIIF3+QfPpbOY/bQmwZEXTXKMvh8GnbFh\n\tYkui0oZtQYAmxv9+wOdOn3bL+zbCE/PluMwkXl0KXxYxu/pWvOXPMpQF6DvkhDLULwatZQNpzWTsq\n\t+Oa4E481wRjCnEvQHDUy8+itwRnK60IhUOVRH8BqM64+7eJe99rtvafifPuH9huuisCAFayWgtanK\n\tEB+us2uruWVE9EFImWkTrWmZjRGS1MvpNSIdtGl5V9mHT4b+xrD9mpArtxKgVz8smU6spvuvXwsNa\n\tRv8RziKmg==;","X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT,\n\tURIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.0","Date":"Mon, 4 Sep 2017 08:50:27 +0200","From":"Maxime Ripard <maxime.ripard@free-electrons.com>","To":"=?iso-8859-1?q?Br=FCns=2C?= Stefan <Stefan.Bruens@rwth-aachen.de>","Subject":"Re: [PATCH 1/3] dmaengine: sun6i: Correct DMA support on H3","Message-ID":"<20170904065027.7splxlgxhpu5nj56@flea>","References":"<20170830233609.13855-1-stefan.bruens@rwth-aachen.de>\n\t<a649f98d4ac44abc873c5110ce9363e5@rwthex-w2-b.rwth-ad.de>\n\t<20170901133549.cr2ivmfr5mnrdujg@flea>\n\t<5722405.lpx0YHbn2k@sbruens-linux>","MIME-Version":"1.0","In-Reply-To":"<5722405.lpx0YHbn2k@sbruens-linux>","User-Agent":"NeoMutt/20170714 (1.8.3)","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170903_235059_589631_DBF516B4 ","X-CRM114-Status":"GOOD (  27.99  )","X-Spam-Score":"-1.9 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details:   (-1.9 points)\n\tpts rule name              description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 SPF_PASS               SPF: sender matches SPF record\n\t-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay\n\tdomain\n\t-1.9 BAYES_00               BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]","X-BeenThere":"linux-arm-kernel@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Unsubscribe":"<http://lists.infradead.org/mailman/options/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=unsubscribe>","List-Archive":"<http://lists.infradead.org/pipermail/linux-arm-kernel/>","List-Post":"<mailto:linux-arm-kernel@lists.infradead.org>","List-Help":"<mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>","List-Subscribe":"<http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>,\n\t<mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>","Cc":"\"devicetree@vger.kernel.org\" <devicetree@vger.kernel.org>,\n\t\"vinod.koul@intel.com\" <vinod.koul@intel.com>,\n\t\"linux-kernel@vger.kernel.org\" <linux-kernel@vger.kernel.org>,\n\tlinux-sunxi <linux-sunxi@googlegroups.com>,\n\tRob Herring <robh+dt@kernel.org>, \n\t\"dmaengine@vger.kernel.org\" <dmaengine@vger.kernel.org>,\n\tChen-Yu Tsai <wens@csie.org>, \"linux-arm-kernel@lists.infradead.org\"\n\t<linux-arm-kernel@lists.infradead.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============5423352991197067652==\"","Sender":"\"linux-arm-kernel\" <linux-arm-kernel-bounces@lists.infradead.org>","Errors-To":"linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org","List-Id":"linux-imx-kernel.lists.patchwork.ozlabs.org"}}]