[{"id":1769939,"web_url":"http://patchwork.ozlabs.org/comment/1769939/","msgid":"<20170918075732.wptp75sh3ovhdukp@flea.lan>","list_archive_url":null,"date":"2017-09-18T07:57:32","subject":"Re: [PATCH v2 01/10] dmaengine: sun6i: Correct setting of clock\n\tautogating register for A83T/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 Sun, Sep 17, 2017 at 05:19:47AM +0200, Stefan Brüns wrote:\n> The H83T uses a compatible string different from the A23, but requires\n> the same clock autogating register setting.\n> \n> The H3 also requires setting the clock autogating register, but has\n> the register at a different offset.\n> \n> Add three suitable callbacks for the existing controller generations\n> and set it in the controller config structure.\n> \n> Signed-off-by: Stefan Brüns \n> ---\n> drivers/dma/sun6i-dma.c | 31 ++++++++++++++++++++++++++-----\n> 1 file changed, 26 insertions(+), 5 deletions(-)\n> \n> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c\n> index bcd496edc70f..45bcd5271d94 100644\n> --- a/drivers/dma/sun6i-dma.c\n> +++ b/drivers/dma/sun6i-dma.c\n> @@ -48,6 +48,9 @@\n> #define SUN8I_DMA_GATE\t\t0x20\n> #define SUN8I_DMA_GATE_ENABLE\t0x4\n> \n> +#define SUNXI_H3_SECURE_REG\t\t0x20\n> +#define SUNXI_H3_DMA_GATE\t\t0x28\n> +#define SUNXI_H3_DMA_GATE_ENABLE\t0x4\n> /*\n> * Channels specific registers\n> */\n> @@ -111,7 +114,7 @@ struct sun6i_dma_config {\n> \t * however these SoCs really have and need this bit, as seen in the\n> \t * BSP kernel source code.\n> \t */\n> -\tbool gate_needed;\n> +\tvoid (*clock_autogate_enable)();\n> };\n> \n> /*\n> @@ -267,6 +270,20 @@ static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)\n> \treturn addr_width >> 1;\n> }\n> \n> +static void sun6i_enable_clock_autogate_noop(struct sun6i_dma_dev *sdev)\n> +{\n> +}\n\nI guess instead of that one, we could just test for the pointer and\nnot call the function if it's NULL?\n\nLooks good otherwise, once fixed, you have\nAcked-by: Maxime Ripard \n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwdds2zXfz9s7G\n\tfor ;\n\tMon, 18 Sep 2017 17:57:37 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752231AbdIRH5f (ORCPT\n\t);\n\tMon, 18 Sep 2017 03:57:35 -0400","from mail.free-electrons.com ([62.4.15.54]:54918 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751980AbdIRH5e (ORCPT\n\t); Mon, 18 Sep 2017 03:57:34 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 0556120A2E; Mon, 18 Sep 2017 09:57:33 +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 CA19020A1A;\n\tMon, 18 Sep 2017 09:57:32 +0200 (CEST)"],"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":"Mon, 18 Sep 2017 09:57:32 +0200","From":"Maxime Ripard ","To":"Stefan =?iso-8859-1?q?Br=FCns?= ","Cc":"linux-sunxi@googlegroups.com, devicetree@vger.kernel.org,\n\tdmaengine@vger.kernel.org, Vinod Koul ,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 01/10] dmaengine: sun6i: Correct setting of clock\n\tautogating register for A83T/H3","Message-ID":"<20170918075732.wptp75sh3ovhdukp@flea.lan>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<20170917031956.28010-2-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"gb3s5rf5jqs52cu2\"","Content-Disposition":"inline","In-Reply-To":"<20170917031956.28010-2-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1769941,"web_url":"http://patchwork.ozlabs.org/comment/1769941/","msgid":"<20170918075837.quzrgygzzpdsooh2@flea.lan>","list_archive_url":null,"date":"2017-09-18T07:58:37","subject":"Re: [PATCH v2 02/10] dmaengine: sun6i: Correct burst length field\n\toffsets for H3","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Sun, Sep 17, 2017 at 05:19:48AM +0200, Stefan Brüns wrote:\n> For the H3, the burst lengths field offsets in the channel configuration\n> register differs from earlier SoC generations.\n> \n> Using the A31 register macros actually configured the H3 controller\n> do to bursts of length 1 always, which although working leads to higher\n> bus utilisation.\n> \n> Signed-off-by: Stefan Brüns \n\nAcked-by: Maxime Ripard \n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwdg51c4cz9s3w\n\tfor ;\n\tMon, 18 Sep 2017 17:58:41 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751661AbdIRH6j (ORCPT\n\t);\n\tMon, 18 Sep 2017 03:58:39 -0400","from mail.free-electrons.com ([62.4.15.54]:54994 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751568AbdIRH6i (ORCPT\n\t); Mon, 18 Sep 2017 03:58:38 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 0366D20A1A; Mon, 18 Sep 2017 09:58: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 CE3E320945;\n\tMon, 18 Sep 2017 09:58:36 +0200 (CEST)"],"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":"Mon, 18 Sep 2017 09:58:37 +0200","From":"Maxime Ripard ","To":"Stefan =?iso-8859-1?q?Br=FCns?= ","Cc":"linux-sunxi@googlegroups.com, devicetree@vger.kernel.org,\n\tdmaengine@vger.kernel.org, Vinod Koul ,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 02/10] dmaengine: sun6i: Correct burst length field\n\toffsets for H3","Message-ID":"<20170918075837.quzrgygzzpdsooh2@flea.lan>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<20170917031956.28010-3-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"fkwssu6i56622utj\"","Content-Disposition":"inline","In-Reply-To":"<20170917031956.28010-3-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1769948,"web_url":"http://patchwork.ozlabs.org/comment/1769948/","msgid":"<20170918080855.yxxfcfg2yttpyi5y@flea.lan>","list_archive_url":null,"date":"2017-09-18T08:08:55","subject":"Re: [PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow\n\textension for new SoCs","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Sun, Sep 17, 2017 at 05:19:49AM +0200, Stefan Brüns wrote:\n> The current code mixes three distinct operations when transforming\n> the slave config to register settings:\n> \n> 1. special handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, maxburst == 0\n> 2. range checking\n> 3. conversion of raw to register values\n> \n> As the range checks depend on the specific SoC, move these out of the\n> conversion to distinct operations.\n> \n> Signed-off-by: Stefan Brüns \n\nAcked-by: Maxime Ripard \n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwdvM2Wgvz9s7G\n\tfor ;\n\tMon, 18 Sep 2017 18:09:19 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752664AbdIRII6 (ORCPT\n\t);\n\tMon, 18 Sep 2017 04:08:58 -0400","from mail.free-electrons.com ([62.4.15.54]:55897 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751700AbdIRII5 (ORCPT\n\t); Mon, 18 Sep 2017 04:08:57 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid BF9C22081F; Mon, 18 Sep 2017 10:08:55 +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 949A6207D2;\n\tMon, 18 Sep 2017 10:08:55 +0200 (CEST)"],"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":"Mon, 18 Sep 2017 10:08:55 +0200","From":"Maxime Ripard ","To":"Stefan =?iso-8859-1?q?Br=FCns?= ","Cc":"linux-sunxi@googlegroups.com, devicetree@vger.kernel.org,\n\tdmaengine@vger.kernel.org, Vinod Koul ,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow\n\textension for new SoCs","Message-ID":"<20170918080855.yxxfcfg2yttpyi5y@flea.lan>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<20170917031956.28010-4-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"dix4vvap7hvwuzv2\"","Content-Disposition":"inline","In-Reply-To":"<20170917031956.28010-4-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1769949,"web_url":"http://patchwork.ozlabs.org/comment/1769949/","msgid":"<20170918080927.ghfahn2xwz52hmsx@flea.lan>","list_archive_url":null,"date":"2017-09-18T08:09:27","subject":"Re: [PATCH v2 04/10] dmaengine: sun6i: Enable additional burst\n\tlengths/widths on H3","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Sun, Sep 17, 2017 at 05:19:50AM +0200, Stefan Brüns wrote:\n> The H3 supports bursts lengths of 1, 4, 8 and 16 transfers, each with\n> a width of 1, 2, 4 or 8 bytes.\n> \n> The register value for the the width is log2-encoded, change the\n> conversion function to provide the correct value for width == 8.\n> \n> Signed-off-by: Stefan Brüns \n\nAcked-by: Maxime Ripard \n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwdvb4cHmz9s7G\n\tfor ;\n\tMon, 18 Sep 2017 18:09:31 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750898AbdIRIJa (ORCPT\n\t);\n\tMon, 18 Sep 2017 04:09:30 -0400","from mail.free-electrons.com ([62.4.15.54]:55924 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750791AbdIRIJ3 (ORCPT\n\t); Mon, 18 Sep 2017 04:09:29 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid A1C7720A4E; Mon, 18 Sep 2017 10:09:27 +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 759EA20A27;\n\tMon, 18 Sep 2017 10:09:27 +0200 (CEST)"],"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, 18 Sep 2017 10:09:27 +0200","From":"Maxime Ripard ","To":"Stefan =?iso-8859-1?q?Br=FCns?= ","Cc":"linux-sunxi@googlegroups.com, devicetree@vger.kernel.org,\n\tdmaengine@vger.kernel.org, Vinod Koul ,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 04/10] dmaengine: sun6i: Enable additional burst\n\tlengths/widths on H3","Message-ID":"<20170918080927.ghfahn2xwz52hmsx@flea.lan>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<20170917031956.28010-5-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"yi6pq3fxiamd3slb\"","Content-Disposition":"inline","In-Reply-To":"<20170917031956.28010-5-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1769953,"web_url":"http://patchwork.ozlabs.org/comment/1769953/","msgid":"<20170918081207.5hagvmrc37cnqgvx@flea.lan>","list_archive_url":null,"date":"2017-09-18T08:12:07","subject":"Re: [PATCH v2 05/10] dmaengine: sun6i: Move number of\n\tpchans/vchans/request to device struct","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"1;4803;0c\nOn Sun, Sep 17, 2017 at 05:19:51AM +0200, Stefan Brüns wrote:\n> Preparatory patch: If the same compatible is used for different SoCs which\n> have a common register layout, but different number of channels, the\n> channel count can no longer be stored in the config. Store it in the\n> device structure instead.\n> \n> Signed-off-by: Stefan Brüns \n\nAcked-by: Maxime Ripard \n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwdz720j3z9s7G\n\tfor ;\n\tMon, 18 Sep 2017 18:12:35 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752461AbdIRIMM (ORCPT\n\t);\n\tMon, 18 Sep 2017 04:12:12 -0400","from mail.free-electrons.com ([62.4.15.54]:56112 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752718AbdIRIMJ (ORCPT\n\t); Mon, 18 Sep 2017 04:12:09 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 7C02120A27; Mon, 18 Sep 2017 10:12:07 +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 2F0042081F;\n\tMon, 18 Sep 2017 10:12:07 +0200 (CEST)"],"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":"Mon, 18 Sep 2017 10:12:07 +0200","From":"Maxime Ripard ","To":"Stefan =?iso-8859-1?q?Br=FCns?= ","Cc":"linux-sunxi@googlegroups.com, devicetree@vger.kernel.org,\n\tdmaengine@vger.kernel.org, Vinod Koul ,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 05/10] dmaengine: sun6i: Move number of\n\tpchans/vchans/request to device struct","Message-ID":"<20170918081207.5hagvmrc37cnqgvx@flea.lan>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<20170917031956.28010-6-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"5u7sndkkugms62ap\"","Content-Disposition":"inline","In-Reply-To":"<20170917031956.28010-6-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1769961,"web_url":"http://patchwork.ozlabs.org/comment/1769961/","msgid":"<20170918081824.iiebcj63wvnean57@flea.lan>","list_archive_url":null,"date":"2017-09-18T08:18:24","subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"Hi,\n\nOn Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:\n> +\tret = of_property_read_u32(np, \"dma-channels\", &sdc->num_pchans);\n> +\tif (ret && !sdc->num_pchans) {\n> +\t\tdev_err(&pdev->dev, \"Can't get dma-channels.\\n\");\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (sdc->num_pchans > DMA_MAX_CHANNELS) {\n> +\t\tdev_err(&pdev->dev, \"Number of dma-channels out of range.\\n\");\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tret = of_property_read_u32(np, \"dma-requests\", &sdc->max_request);\n> +\tif (ret && !sdc->max_request) {\n> +\t\tdev_info(&pdev->dev, \"Missing dma-requests, using %u.\\n\",\n> +\t\t\t DMA_CHAN_MAX_DRQ);\n> +\t\tsdc->max_request = DMA_CHAN_MAX_DRQ;\n> +\t}\n> +\n> +\tif (sdc->max_request > DMA_CHAN_MAX_DRQ) {\n> +\t\tdev_err(&pdev->dev, \"Value of dma-requests out of range.\\n\");\n> +\t\treturn -EINVAL;\n> +\t}\n\nI'm not really convinced about these two checks. They don't catch all\nerrors (the range between the actual number of channels / DRQ and the\nmaximum allowed per the registers), they might increase in the future\ntoo, and if we want to make that check actually working, we would have\nto duplicate the number of requests and channels into the driver.\n\nWhich is kind of the opposite of what we're trying to do here :)\n\nOnce removed,\nAcked-by: Maxime Ripard \n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwf674LQYz9s7G\n\tfor ;\n\tMon, 18 Sep 2017 18:18:39 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751800AbdIRISh (ORCPT\n\t);\n\tMon, 18 Sep 2017 04:18:37 -0400","from mail.free-electrons.com ([62.4.15.54]:56521 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751568AbdIRISh (ORCPT\n\t); Mon, 18 Sep 2017 04:18:37 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid E0E6720858; Mon, 18 Sep 2017 10:18:34 +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 B6D1B207EB;\n\tMon, 18 Sep 2017 10:18:24 +0200 (CEST)"],"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":"Mon, 18 Sep 2017 10:18:24 +0200","From":"Maxime Ripard ","To":"Stefan =?iso-8859-1?q?Br=FCns?= ","Cc":"linux-sunxi@googlegroups.com, devicetree@vger.kernel.org,\n\tdmaengine@vger.kernel.org, Vinod Koul ,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Message-ID":"<20170918081824.iiebcj63wvnean57@flea.lan>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<20170917031956.28010-8-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"a5lxixo24iqhcmfo\"","Content-Disposition":"inline","In-Reply-To":"<20170917031956.28010-8-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1769963,"web_url":"http://patchwork.ozlabs.org/comment/1769963/","msgid":"<20170918081928.eex7njub6atasj6x@flea.lan>","list_archive_url":null,"date":"2017-09-18T08:19:28","subject":"Re: [PATCH v2 08/10] dmaengine: sun6i: Add support for Allwinner A64\n\tand compatibles","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Sun, Sep 17, 2017 at 05:19:54AM +0200, Stefan Brüns wrote:\n> The A64 SoC has the same dma engine as the H3 (sun8i), with a\n> reduced amount of physical channels. To allow future reuse of the\n> compatible, leave the channel count etc. in the config data blank\n> and retrieve it from the devicetree.\n> \n> Signed-off-by: Stefan Brüns \n> ---\n> drivers/dma/sun6i-dma.c | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n> \n> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c\n> index b5ecc97a0d5a..118b29bb1eac 100644\n> --- a/drivers/dma/sun6i-dma.c\n> +++ b/drivers/dma/sun6i-dma.c\n> @@ -1127,6 +1127,28 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {\n> \t\t\t BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);\n> };\n> \n> +/*\n> + * The A64 binding uses the number of dma channels from the\n> + * device tree node.\n> + */\n> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {\n> +\t.nr_max_channels = 0,\n> +\t.nr_max_requests = 0,\n> +\t.nr_max_vchans = 0,\n\nThose are the default values. I appreciate that you wanted them here\nfor documentation, but the comment above already fills up that role\nfine.\n\nOnce removed,\n\nAcked-by: Maxime Ripard \n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwf7b5g7xz9s3w\n\tfor ;\n\tMon, 18 Sep 2017 18:19:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751551AbdIRITx (ORCPT\n\t);\n\tMon, 18 Sep 2017 04:19:53 -0400","from mail.free-electrons.com ([62.4.15.54]:56612 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750813AbdIRITx (ORCPT\n\t); Mon, 18 Sep 2017 04:19:53 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid A0EE52083C; Mon, 18 Sep 2017 10:19:51 +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 8A8B320A58;\n\tMon, 18 Sep 2017 10:19:28 +0200 (CEST)"],"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, 18 Sep 2017 10:19:28 +0200","From":"Maxime Ripard ","To":"Stefan =?iso-8859-1?q?Br=FCns?= ","Cc":"linux-sunxi@googlegroups.com, devicetree@vger.kernel.org,\n\tdmaengine@vger.kernel.org, Vinod Koul ,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 08/10] dmaengine: sun6i: Add support for Allwinner A64\n\tand compatibles","Message-ID":"<20170918081928.eex7njub6atasj6x@flea.lan>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<20170917031956.28010-9-stefan.bruens@rwth-aachen.de>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"7jsgfwzuw3d76rey\"","Content-Disposition":"inline","In-Reply-To":"<20170917031956.28010-9-stefan.bruens@rwth-aachen.de>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1771193,"web_url":"http://patchwork.ozlabs.org/comment/1771193/","msgid":"<6028407.VKu5LCmQv0@sbruens-linux>","list_archive_url":null,"date":"2017-09-19T16:17:59","subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","submitter":{"id":67055,"url":"http://patchwork.ozlabs.org/api/people/67055/","name":"Stefan Brüns","email":"stefan.bruens@rwth-aachen.de"},"content":"On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:\n> On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:\n> > On Montag, 18. September 2017 10:18:24 CEST you wrote:\n> > > Hi,\n> > > \n> > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:\n> > > > +\tret = of_property_read_u32(np, \"dma-channels\", &sdc->num_pchans);\n> > > > +\tif (ret && !sdc->num_pchans) {\n> > > > +\t\tdev_err(&pdev->dev, \"Can't get dma-channels.\\n\");\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tif (sdc->num_pchans > DMA_MAX_CHANNELS) {\n> > > > +\t\tdev_err(&pdev->dev, \"Number of dma-channels out of range.\\n\");\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tret = of_property_read_u32(np, \"dma-requests\", &sdc->max_request);\n> > > > +\tif (ret && !sdc->max_request) {\n> > > > +\t\tdev_info(&pdev->dev, \"Missing dma-requests, using %u.\\n\",\n> > > > +\t\t\t DMA_CHAN_MAX_DRQ);\n> > > > +\t\tsdc->max_request = DMA_CHAN_MAX_DRQ;\n> > > > +\t}\n> > > > +\n> > > > +\tif (sdc->max_request > DMA_CHAN_MAX_DRQ) {\n> > > > +\t\tdev_err(&pdev->dev, \"Value of dma-requests out of range.\\n\");\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > \n> > > I'm not really convinced about these two checks. They don't catch all\n> > > errors (the range between the actual number of channels / DRQ and the\n> > > maximum allowed per the registers), they might increase in the future\n> > > too, and if we want to make that check actually working, we would have\n> > > to duplicate the number of requests and channels into the driver.\n> > \n> > 1. If these values increase, we have a new register layout and and\n> > need a new compatible anyway.\n> \n> And you want to store a new maximum attached to the compatible? Isn't\n> that exactly the situation you're trying to get away from?\n\nYes, and no. H3, H5, A64 and R40 have the exact same register layout, but \ndifferent number of channels and ports. They could share a compatible (if DMA \nchannels were generalized), and we already have several register offsets/\nwidths (implicitly via the callbacks) attached to the compatible (so these \ndon't need generalization via DT).\n\nNow, we could also move everything that is currently attached to the \ncompatible, i.e. clock gate register offset, burst widths/lengths etc. into \nthe devicetree binding, but that would just be too much.\n\nThe idea is to find a middle ground here, using common patterns in the \nexisting SoCs. The register layout has hardly changed, while the number of DMA \nchannels and ports changes all the time. Moving the number of DMA channels and \nports to the DT is trivial, and a pattern also found in other DMA controller \ndrivers. *If* the number of dma channels and ports is ever increased, \nexceeding the current maximum, this would amount to major changes in the \ndriver and maybe even warrant a completely new driver.\n\n> > 2. As long as the the limits are adhered to, no other registers/register\n> > fields are overwritten. As the channel number and port are used to\n> > calculate memory offsets bounds checking is IMHO a good idea.\n> \n> And this is true for many other resources, starting with the one\n> defined in reg. We don't error check every register range, clock\n> index, reset line, interrupt, DMA channel, the memory size, etc. yet\n> you could make the same argument.\n> \n> The DT has to be right, and we have to trust it. Otherwise we can just\n> throw it away.\n\nSo your argument here basically is - don't do any checks on DT provided \nvalues, these are always correct. So, following this argument, not only the \nrange check, but also the of_property_read return values should be ignored, as \nthe DT is correct, thus of_property_read will never return an error.\n\nThat clearly does not match the implementation of drivers throughout the \nvarious subsystems for DT properties, which is in general - do all the checks \nthat can be done, trust everything you can not verify.\n\nKind regards,\n\nStefan\n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xxSht0Hscz9sMN\n\tfor ;\n\tWed, 20 Sep 2017 02:18:05 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751394AbdISQSE convert rfc822-to-8bit (ORCPT\n\t);\n\tTue, 19 Sep 2017 12:18:04 -0400","from mail-out-1.itc.rwth-aachen.de ([134.130.5.46]:46500 \"EHLO\n\tmail-out-1.itc.rwth-aachen.de\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751283AbdISQSC (ORCPT\n\t); Tue, 19 Sep 2017 12:18:02 -0400","from rwthex-s3-a.rwth-ad.de ([134.130.26.160])\n\tby mail-in-1.itc.rwth-aachen.de with ESMTP; 19 Sep 2017 18:18:00 +0200","from rwthex-w2-a.rwth-ad.de (2002:8682:1a9e::8682:1a9e) by\n\trwthex-s3-a.rwth-ad.de (2002:8682:1aa0::8682:1aa0) with Microsoft\n\tSMTP Server\n\t(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id\n\t15.1.1034.26; Tue, 19 Sep 2017 18:17:59 +0200","from rwthex-w2-a.rwth-ad.de ([fe80::18f3:313d:3e:42ff]) by\n\trwthex-w2-a.rwth-ad.de ([fe80::18f3:313d:3e:42ff%21]) with mapi id\n\t15.01.1034.026; Tue, 19 Sep 2017 18:17:59 +0200"],"X-IronPort-AV":"E=Sophos;i=\"5.42,418,1500933600\"; d=\"scan'208\";a=\"14129079\"","From":"=?iso-8859-1?q?Br=FCns=2C_Stefan?= ","To":"Maxime Ripard ","CC":"\"linux-sunxi@googlegroups.com\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"dmaengine@vger.kernel.org\" ,\n\tVinod Koul ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Thread-Topic":"[PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Thread-Index":"AQHTMFlrb6IcqOdVukugQ6LTh2jDNqK8Q3uA","Date":"Tue, 19 Sep 2017 16:17:59 +0000","Message-ID":"<6028407.VKu5LCmQv0@sbruens-linux>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<2791817.czGZyN6WKS@sbruens-linux>\n\t<20170919142508.woslovwjtecgygpo@flea.lan>","In-Reply-To":"<20170919142508.woslovwjtecgygpo@flea.lan>","Accept-Language":"en-US, de-DE","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[78.35.13.203]","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-ID":"<60C30A0C0F582C4E9A9378499AFB4D7D@rwth-ad.de>","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1773899,"web_url":"http://patchwork.ozlabs.org/comment/1773899/","msgid":"<20170922213027.xpnaut3an5or6edl@flea.home>","list_archive_url":null,"date":"2017-09-22T21:30:27","subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:\n> On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:\n> > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:\n> > > On Montag, 18. September 2017 10:18:24 CEST you wrote:\n> > > > Hi,\n> > > > \n> > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:\n> > > > > +\tret = of_property_read_u32(np, \"dma-channels\", &sdc->num_pchans);\n> > > > > +\tif (ret && !sdc->num_pchans) {\n> > > > > +\t\tdev_err(&pdev->dev, \"Can't get dma-channels.\\n\");\n> > > > > +\t\treturn ret;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (sdc->num_pchans > DMA_MAX_CHANNELS) {\n> > > > > +\t\tdev_err(&pdev->dev, \"Number of dma-channels out of range.\\n\");\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tret = of_property_read_u32(np, \"dma-requests\", &sdc->max_request);\n> > > > > +\tif (ret && !sdc->max_request) {\n> > > > > +\t\tdev_info(&pdev->dev, \"Missing dma-requests, using %u.\\n\",\n> > > > > +\t\t\t DMA_CHAN_MAX_DRQ);\n> > > > > +\t\tsdc->max_request = DMA_CHAN_MAX_DRQ;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tif (sdc->max_request > DMA_CHAN_MAX_DRQ) {\n> > > > > +\t\tdev_err(&pdev->dev, \"Value of dma-requests out of range.\\n\");\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > \n> > > > I'm not really convinced about these two checks. They don't catch all\n> > > > errors (the range between the actual number of channels / DRQ and the\n> > > > maximum allowed per the registers), they might increase in the future\n> > > > too, and if we want to make that check actually working, we would have\n> > > > to duplicate the number of requests and channels into the driver.\n> > > \n> > > 1. If these values increase, we have a new register layout and and\n> > > need a new compatible anyway.\n> > \n> > And you want to store a new maximum attached to the compatible? Isn't\n> > that exactly the situation you're trying to get away from?\n> \n> Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but \n> different number of channels and ports. They could share a compatible (if DMA \n> channels were generalized), and we already have several register offsets/\n> widths (implicitly via the callbacks) attached to the compatible (so these \n> don't need generalization via DT).\n> \n> Now, we could also move everything that is currently attached to the \n> compatible, i.e. clock gate register offset, burst widths/lengths etc. into \n> the devicetree binding, but that would just be too much.\n> \n> The idea is to find a middle ground here, using common patterns in the \n> existing SoCs. The register layout has hardly changed, while the number of DMA \n> channels and ports changes all the time. Moving the number of DMA channels and \n> ports to the DT is trivial, and a pattern also found in other DMA controller \n> drivers.\n\nI'm sorry, but the code is inconsistent here. You basically have two\nvariables from one SoC to the other, the number of channels and\nrequests.\n\nIn one case (channels), it mandates that the property is provided in\nthe device tree, and doesn't default to anything.\n\nIn the other case (requests), the property is optional and it will\nprovide a default. All that in 20 lines.\n\nI guess we already reached that middle ground by providing them\nthrough the DT, we just have to make sure we remain consistent.\n\n> *If* the number of dma channels and ports is ever increased,\n> exceeding the current maximum, this would amount to major changes in\n> the driver and maybe even warrant a completely new driver.\n> \n> > > 2. As long as the the limits are adhered to, no other registers/register\n> > > fields are overwritten. As the channel number and port are used to\n> > > calculate memory offsets bounds checking is IMHO a good idea.\n> > \n> > And this is true for many other resources, starting with the one\n> > defined in reg. We don't error check every register range, clock\n> > index, reset line, interrupt, DMA channel, the memory size, etc. yet\n> > you could make the same argument.\n> > \n> > The DT has to be right, and we have to trust it. Otherwise we can just\n> > throw it away.\n> \n> So your argument here basically is - don't do any checks on DT provided \n> values, these are always correct. So, following this argument, not only the \n> range check, but also the of_property_read return values should be ignored, as \n> the DT is correct, thus of_property_read will never return an error.\n\nNo, my argument is don't do a check if you can catch only half of the\nerrors, and with no hope of fixing it.\n\nThe functions you mentionned have a 100% error catch rate. This is the\ndifference.\n\n> That clearly does not match the implementation of drivers throughout the \n> various subsystems for DT properties, which is in general - do all the checks \n> that can be done, trust everything you can not verify.\n\nAnd my point is that we're falling into the latter here. You cannot\nverify it properly.\n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzRZt1KtHz9s9Y\n\tfor ;\n\tSat, 23 Sep 2017 07:34:46 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752430AbdIVVe3 (ORCPT\n\t);\n\tFri, 22 Sep 2017 17:34:29 -0400","from mail.free-electrons.com ([62.4.15.54]:50187 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752402AbdIVVaj (ORCPT\n\t); Fri, 22 Sep 2017 17:30:39 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 2B29720A1A; Fri, 22 Sep 2017 23:30:37 +0200 (CEST)","from localhost (LFbn-TOU-1-209-191.w86-201.abo.wanadoo.fr\n\t[86.201.56.191])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id F15C420A10;\n\tFri, 22 Sep 2017 23:30:26 +0200 (CEST)"],"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, 22 Sep 2017 23:30:27 +0200","From":"Maxime Ripard ","To":"=?iso-8859-1?q?Br=FCns=2C?= Stefan ","Cc":"\"linux-sunxi@googlegroups.com\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"dmaengine@vger.kernel.org\" ,\n\tVinod Koul ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Message-ID":"<20170922213027.xpnaut3an5or6edl@flea.home>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<2791817.czGZyN6WKS@sbruens-linux>\n\t<20170919142508.woslovwjtecgygpo@flea.lan>\n\t<6028407.VKu5LCmQv0@sbruens-linux>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"jvxhcs6lrr6gn7cn\"","Content-Disposition":"inline","In-Reply-To":"<6028407.VKu5LCmQv0@sbruens-linux>","User-Agent":"NeoMutt/20170914 (1.9.0)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1773941,"web_url":"http://patchwork.ozlabs.org/comment/1773941/","msgid":"<3154933.I7MMVk5mkV@sbruens-linux>","list_archive_url":null,"date":"2017-09-23T00:00:15","subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","submitter":{"id":67055,"url":"http://patchwork.ozlabs.org/api/people/67055/","name":"Stefan Brüns","email":"stefan.bruens@rwth-aachen.de"},"content":"On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:\n> On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:\n> > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:\n> > > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:\n> > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:\n> > > > > Hi,\n> > > > > \n> > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:\n> > > > > > +\tret = of_property_read_u32(np, \"dma-channels\",\n> > > > > > &sdc->num_pchans);\n> > > > > > +\tif (ret && !sdc->num_pchans) {\n> > > > > > +\t\tdev_err(&pdev->dev, \"Can't get dma-channels.\\n\");\n> > > > > > +\t\treturn ret;\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tif (sdc->num_pchans > DMA_MAX_CHANNELS) {\n> > > > > > +\t\tdev_err(&pdev->dev, \"Number of dma-channels out of range.\n\\n\");\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tret = of_property_read_u32(np, \"dma-requests\",\n> > > > > > &sdc->max_request);\n> > > > > > +\tif (ret && !sdc->max_request) {\n> > > > > > +\t\tdev_info(&pdev->dev, \"Missing dma-requests, using %u.\\n\",\n> > > > > > +\t\t\t DMA_CHAN_MAX_DRQ);\n> > > > > > +\t\tsdc->max_request = DMA_CHAN_MAX_DRQ;\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tif (sdc->max_request > DMA_CHAN_MAX_DRQ) {\n> > > > > > +\t\tdev_err(&pdev->dev, \"Value of dma-requests out of range.\\n\");\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\t}\n> > > > > \n> > > > > I'm not really convinced about these two checks. They don't catch\n> > > > > all\n> > > > > errors (the range between the actual number of channels / DRQ and\n> > > > > the\n> > > > > maximum allowed per the registers), they might increase in the\n> > > > > future\n> > > > > too, and if we want to make that check actually working, we would\n> > > > > have\n> > > > > to duplicate the number of requests and channels into the driver.\n> > > > \n> > > > 1. If these values increase, we have a new register layout and and\n> > > > need a new compatible anyway.\n> > > \n> > > And you want to store a new maximum attached to the compatible? Isn't\n> > > that exactly the situation you're trying to get away from?\n> > \n> > Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but\n> > different number of channels and ports. They could share a compatible (if\n> > DMA channels were generalized), and we already have several register\n> > offsets/ widths (implicitly via the callbacks) attached to the compatible\n> > (so these don't need generalization via DT).\n> > \n> > Now, we could also move everything that is currently attached to the\n> > compatible, i.e. clock gate register offset, burst widths/lengths etc.\n> > into\n> > the devicetree binding, but that would just be too much.\n> > \n> > The idea is to find a middle ground here, using common patterns in the\n> > existing SoCs. The register layout has hardly changed, while the number of\n> > DMA channels and ports changes all the time. Moving the number of DMA\n> > channels and ports to the DT is trivial, and a pattern also found in\n> > other DMA controller drivers.\n> \n> I'm sorry, but the code is inconsistent here. You basically have two\n> variables from one SoC to the other, the number of channels and\n> requests.\n> \n> In one case (channels), it mandates that the property is provided in\n> the device tree, and doesn't default to anything.\n> \n> In the other case (requests), the property is optional and it will\n> provide a default. All that in 20 lines.\n\nThe channel number is a hardware property. Using more channels than the \nhardware provides is a bug. There is no default.\n\nThe port/request is just some lax property to limit the resource allocation \nupfront. As long as the bindings of the different IP blocks (SPI, audio, ...) \nprovide the correct port numbers, all required information is available.\n \n> I guess we already reached that middle ground by providing them\n> through the DT, we just have to make sure we remain consistent.\n> \n> > *If* the number of dma channels and ports is ever increased,\n> > exceeding the current maximum, this would amount to major changes in\n> > the driver and maybe even warrant a completely new driver.\n> > \n> > > > 2. As long as the the limits are adhered to, no other\n> > > > registers/register\n> > > > fields are overwritten. As the channel number and port are used to\n> > > > calculate memory offsets bounds checking is IMHO a good idea.\n> > > \n> > > And this is true for many other resources, starting with the one\n> > > defined in reg. We don't error check every register range, clock\n> > > index, reset line, interrupt, DMA channel, the memory size, etc. yet\n> > > you could make the same argument.\n> > > \n> > > The DT has to be right, and we have to trust it. Otherwise we can just\n> > > throw it away.\n> > \n> > So your argument here basically is - don't do any checks on DT provided\n> > values, these are always correct. So, following this argument, not only\n> > the\n> > range check, but also the of_property_read return values should be\n> > ignored, as the DT is correct, thus of_property_read will never return an\n> > error.\n> No, my argument is don't do a check if you can catch only half of the\n> errors, and with no hope of fixing it.\n> \n> The functions you mentionned have a 100% error catch rate. This is the\n> difference.\n> \n> > That clearly does not match the implementation of drivers throughout the\n> > various subsystems for DT properties, which is in general - do all the\n> > checks that can be done, trust everything you can not verify.\n> \n> And my point is that we're falling into the latter here. You cannot\n> verify it properly.\n\nPlease check the following line:\n\nhttps://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/\ndrivers/dma/sun6i-dma.c#n951\n\nThats far from 100% - the highest allowed port for each SoC differs between RX \nand TX, and port allocation is sparse.\n\nRegards,\n\nStefan\n\n\n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzVpx3Nhcz9sNw\n\tfor ;\n\tSat, 23 Sep 2017 10:00:25 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751845AbdIWAAT convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 22 Sep 2017 20:00:19 -0400","from mail-out-1.itc.rwth-aachen.de ([134.130.5.46]:8797 \"EHLO\n\tmail-out-1.itc.rwth-aachen.de\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751763AbdIWAAS (ORCPT\n\t); Fri, 22 Sep 2017 20:00:18 -0400","from rwthex-s3-a.rwth-ad.de ([134.130.26.160])\n\tby mail-in-1.itc.rwth-aachen.de with ESMTP; 23 Sep 2017 02:00:15 +0200","from rwthex-w2-a.rwth-ad.de (2002:8682:1a9e::8682:1a9e) by\n\trwthex-s3-a.rwth-ad.de (2002:8682:1aa0::8682:1aa0) with Microsoft\n\tSMTP Server\n\t(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id\n\t15.1.1034.26; Sat, 23 Sep 2017 02:00:15 +0200","from rwthex-w2-a.rwth-ad.de ([fe80::18f3:313d:3e:42ff]) by\n\trwthex-w2-a.rwth-ad.de ([fe80::18f3:313d:3e:42ff%21]) with mapi id\n\t15.01.1034.026; Sat, 23 Sep 2017 02:00:15 +0200"],"X-IronPort-AV":"E=Sophos;i=\"5.42,428,1500933600\"; d=\"scan'208\";a=\"14710137\"","From":"=?iso-8859-1?q?Br=FCns=2C_Stefan?= ","To":"Maxime Ripard ","CC":"\"linux-sunxi@googlegroups.com\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"dmaengine@vger.kernel.org\" ,\n\tVinod Koul ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Thread-Topic":"[PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Thread-Index":"AQHTMFlrb6IcqOdVukugQ6LTh2jDNqLBe6GA","Date":"Sat, 23 Sep 2017 00:00:15 +0000","Message-ID":"<3154933.I7MMVk5mkV@sbruens-linux>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<6028407.VKu5LCmQv0@sbruens-linux>\n\t<20170922213027.xpnaut3an5or6edl@flea.home>","In-Reply-To":"<20170922213027.xpnaut3an5or6edl@flea.home>","Accept-Language":"en-US, de-DE","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[78.35.13.203]","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-ID":"<758DC10755E7CE4DAAECBDDD8174164F@rwth-ad.de>","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1776161,"web_url":"http://patchwork.ozlabs.org/comment/1776161/","msgid":"<20170927090922.u2hnvgz7yi55sjl3@flea>","list_archive_url":null,"date":"2017-09-27T09:09:22","subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Sat, Sep 23, 2017 at 12:00:15AM +0000, Brüns, Stefan wrote:\n> On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:\n> > On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:\n> > > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:\n> > > > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:\n> > > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:\n> > > > > > Hi,\n> > > > > > \n> > > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:\n> > > > > > > +\tret = of_property_read_u32(np, \"dma-channels\",\n> > > > > > > &sdc->num_pchans);\n> > > > > > > +\tif (ret && !sdc->num_pchans) {\n> > > > > > > +\t\tdev_err(&pdev->dev, \"Can't get dma-channels.\\n\");\n> > > > > > > +\t\treturn ret;\n> > > > > > > +\t}\n> > > > > > > +\n> > > > > > > +\tif (sdc->num_pchans > DMA_MAX_CHANNELS) {\n> > > > > > > +\t\tdev_err(&pdev->dev, \"Number of dma-channels out of range.\n> \\n\");\n> > > > > > > +\t\treturn -EINVAL;\n> > > > > > > +\t}\n> > > > > > > +\n> > > > > > > +\tret = of_property_read_u32(np, \"dma-requests\",\n> > > > > > > &sdc->max_request);\n> > > > > > > +\tif (ret && !sdc->max_request) {\n> > > > > > > +\t\tdev_info(&pdev->dev, \"Missing dma-requests, using %u.\\n\",\n> > > > > > > +\t\t\t DMA_CHAN_MAX_DRQ);\n> > > > > > > +\t\tsdc->max_request = DMA_CHAN_MAX_DRQ;\n> > > > > > > +\t}\n> > > > > > > +\n> > > > > > > +\tif (sdc->max_request > DMA_CHAN_MAX_DRQ) {\n> > > > > > > +\t\tdev_err(&pdev->dev, \"Value of dma-requests out of range.\\n\");\n> > > > > > > +\t\treturn -EINVAL;\n> > > > > > > +\t}\n> > > > > > \n> > > > > > I'm not really convinced about these two checks. They don't catch\n> > > > > > all\n> > > > > > errors (the range between the actual number of channels / DRQ and\n> > > > > > the\n> > > > > > maximum allowed per the registers), they might increase in the\n> > > > > > future\n> > > > > > too, and if we want to make that check actually working, we would\n> > > > > > have\n> > > > > > to duplicate the number of requests and channels into the driver.\n> > > > > \n> > > > > 1. If these values increase, we have a new register layout and and\n> > > > > need a new compatible anyway.\n> > > > \n> > > > And you want to store a new maximum attached to the compatible? Isn't\n> > > > that exactly the situation you're trying to get away from?\n> > > \n> > > Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but\n> > > different number of channels and ports. They could share a compatible (if\n> > > DMA channels were generalized), and we already have several register\n> > > offsets/ widths (implicitly via the callbacks) attached to the compatible\n> > > (so these don't need generalization via DT).\n> > > \n> > > Now, we could also move everything that is currently attached to the\n> > > compatible, i.e. clock gate register offset, burst widths/lengths etc.\n> > > into\n> > > the devicetree binding, but that would just be too much.\n> > > \n> > > The idea is to find a middle ground here, using common patterns in the\n> > > existing SoCs. The register layout has hardly changed, while the number of\n> > > DMA channels and ports changes all the time. Moving the number of DMA\n> > > channels and ports to the DT is trivial, and a pattern also found in\n> > > other DMA controller drivers.\n> > \n> > I'm sorry, but the code is inconsistent here. You basically have two\n> > variables from one SoC to the other, the number of channels and\n> > requests.\n> > \n> > In one case (channels), it mandates that the property is provided in\n> > the device tree, and doesn't default to anything.\n> > \n> > In the other case (requests), the property is optional and it will\n> > provide a default. All that in 20 lines.\n> \n> The channel number is a hardware property. Using more channels than the \n> hardware provides is a bug. There is no default.\n> \n> The port/request is just some lax property to limit the resource allocation \n> upfront. As long as the bindings of the different IP blocks (SPI, audio, ...) \n> provide the correct port numbers, all required information is available.\n\nUsing an improper request ID or out of bounds will be just as much as\na bug. You will not get your DMA transfer to the proper device you\nwere trying to, the data will not reach the device or memory, your\ndriver will not work => a bug.\n\nIt will not be for the same reasons, you will not overwrite other\nregisters, but the end result is just the same: your transfer will not\nwork.\n\n> > I guess we already reached that middle ground by providing them\n> > through the DT, we just have to make sure we remain consistent.\n> > \n> > > *If* the number of dma channels and ports is ever increased,\n> > > exceeding the current maximum, this would amount to major changes in\n> > > the driver and maybe even warrant a completely new driver.\n> > > \n> > > > > 2. As long as the the limits are adhered to, no other\n> > > > > registers/register\n> > > > > fields are overwritten. As the channel number and port are used to\n> > > > > calculate memory offsets bounds checking is IMHO a good idea.\n> > > > \n> > > > And this is true for many other resources, starting with the one\n> > > > defined in reg. We don't error check every register range, clock\n> > > > index, reset line, interrupt, DMA channel, the memory size, etc. yet\n> > > > you could make the same argument.\n> > > > \n> > > > The DT has to be right, and we have to trust it. Otherwise we can just\n> > > > throw it away.\n> > > \n> > > So your argument here basically is - don't do any checks on DT provided\n> > > values, these are always correct. So, following this argument, not only\n> > > the\n> > > range check, but also the of_property_read return values should be\n> > > ignored, as the DT is correct, thus of_property_read will never return an\n> > > error.\n> > No, my argument is don't do a check if you can catch only half of the\n> > errors, and with no hope of fixing it.\n> > \n> > The functions you mentionned have a 100% error catch rate. This is the\n> > difference.\n> > \n> > > That clearly does not match the implementation of drivers throughout the\n> > > various subsystems for DT properties, which is in general - do all the\n> > > checks that can be done, trust everything you can not verify.\n> > \n> > And my point is that we're falling into the latter here. You cannot\n> > verify it properly.\n> \n> Please check the following line:\n> \n> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/dma/sun6i-dma.c#n951\n> \n> Thats far from 100% - the highest allowed port for each SoC differs between RX \n> and TX, and port allocation is sparse.\n\nBut until your patches, you *could* fix it and reach that 100%.\n\nAnd I guess now we could indeed remove it.\n\nLook, this discussion is going nowhere. I told you what the condition\nfor my Acked-by was already.\n\nMaxime","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2Bq14Xm0z9tXW\n\tfor ;\n\tWed, 27 Sep 2017 19:09:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752339AbdI0JJ3 (ORCPT\n\t);\n\tWed, 27 Sep 2017 05:09:29 -0400","from mail.free-electrons.com ([62.4.15.54]:55721 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752292AbdI0JJY (ORCPT\n\t); Wed, 27 Sep 2017 05:09:24 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid B5D7D208A9; Wed, 27 Sep 2017 11:09:21 +0200 (CEST)","from localhost (unknown [195.81.232.10])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 8C17520807;\n\tWed, 27 Sep 2017 11:09:21 +0200 (CEST)"],"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":"Wed, 27 Sep 2017 11:09:22 +0200","From":"Maxime Ripard ","To":"=?iso-8859-1?q?Br=FCns=2C?= Stefan ","Cc":"\"linux-sunxi@googlegroups.com\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"dmaengine@vger.kernel.org\" ,\n\tVinod Koul ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Message-ID":"<20170927090922.u2hnvgz7yi55sjl3@flea>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<6028407.VKu5LCmQv0@sbruens-linux>\n\t<20170922213027.xpnaut3an5or6edl@flea.home>\n\t<3154933.I7MMVk5mkV@sbruens-linux>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"n6w3s3tcm42bqjww\"","Content-Disposition":"inline","In-Reply-To":"<3154933.I7MMVk5mkV@sbruens-linux>","User-Agent":"NeoMutt/20170914 (1.9.0)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1776685,"web_url":"http://patchwork.ozlabs.org/comment/1776685/","msgid":"<9434998.NR9mI8DG1i@pebbles.site>","list_archive_url":null,"date":"2017-09-27T23:10:46","subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","submitter":{"id":67055,"url":"http://patchwork.ozlabs.org/api/people/67055/","name":"Stefan Brüns","email":"stefan.bruens@rwth-aachen.de"},"content":"On Mittwoch, 27. September 2017 11:09:22 CEST Maxime Ripard wrote:\n> On Sat, Sep 23, 2017 at 12:00:15AM +0000, Brüns, Stefan wrote:\n> > On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:\n> > > On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:\n> > > > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:\n> > > > > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:\n> > > > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:\n> > > > > > > Hi,\n> > > > > > > \n> > > > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:\n> > > > > > > > +\tret = of_property_read_u32(np, \"dma-channels\",\n> > > > > > > > &sdc->num_pchans);\n> > > > > > > > +\tif (ret && !sdc->num_pchans) {\n> > > > > > > > +\t\tdev_err(&pdev->dev, \"Can't get dma-channels.\\n\");\n> > > > > > > > +\t\treturn ret;\n> > > > > > > > +\t}\n> > > > > > > > +\n> > > > > > > > +\tif (sdc->num_pchans > DMA_MAX_CHANNELS) {\n> > > > > > > > +\t\tdev_err(&pdev->dev, \"Number of dma-channels out of range.\n> > \n> > \\n\");\n> > \n> > > > > > > > +\t\treturn -EINVAL;\n> > > > > > > > +\t}\n> > > > > > > > +\n> > > > > > > > +\tret = of_property_read_u32(np, \"dma-requests\",\n> > > > > > > > &sdc->max_request);\n> > > > > > > > +\tif (ret && !sdc->max_request) {\n> > > > > > > > +\t\tdev_info(&pdev->dev, \"Missing dma-requests, using %u.\\n\",\n> > > > > > > > +\t\t\t DMA_CHAN_MAX_DRQ);\n> > > > > > > > +\t\tsdc->max_request = DMA_CHAN_MAX_DRQ;\n> > > > > > > > +\t}\n> > > > > > > > +\n> > > > > > > > +\tif (sdc->max_request > DMA_CHAN_MAX_DRQ) {\n> > > > > > > > +\t\tdev_err(&pdev->dev, \"Value of dma-requests out of\n> > > > > > > > range.\\n\");\n> > > > > > > > +\t\treturn -EINVAL;\n> > > > > > > > +\t}\n> > > > > > > \n> > > > > > > I'm not really convinced about these two checks. They don't\n> > > > > > > catch\n> > > > > > > all\n> > > > > > > errors (the range between the actual number of channels / DRQ\n> > > > > > > and\n> > > > > > > the\n> > > > > > > maximum allowed per the registers), they might increase in the\n> > > > > > > future\n> > > > > > > too, and if we want to make that check actually working, we\n> > > > > > > would\n> > > > > > > have\n> > > > > > > to duplicate the number of requests and channels into the\n> > > > > > > driver.\n> > > > > > \n> > > > > > 1. If these values increase, we have a new register layout and and\n> > > > > > need a new compatible anyway.\n> > > > > \n> > > > > And you want to store a new maximum attached to the compatible?\n> > > > > Isn't\n> > > > > that exactly the situation you're trying to get away from?\n> > > > \n> > > > Yes, and no. H3, H5, A64 and R40 have the exact same register layout,\n> > > > but\n> > > > different number of channels and ports. They could share a compatible\n> > > > (if\n> > > > DMA channels were generalized), and we already have several register\n> > > > offsets/ widths (implicitly via the callbacks) attached to the\n> > > > compatible\n> > > > (so these don't need generalization via DT).\n> > > > \n> > > > Now, we could also move everything that is currently attached to the\n> > > > compatible, i.e. clock gate register offset, burst widths/lengths etc.\n> > > > into\n> > > > the devicetree binding, but that would just be too much.\n> > > > \n> > > > The idea is to find a middle ground here, using common patterns in the\n> > > > existing SoCs. The register layout has hardly changed, while the\n> > > > number of\n> > > > DMA channels and ports changes all the time. Moving the number of DMA\n> > > > channels and ports to the DT is trivial, and a pattern also found in\n> > > > other DMA controller drivers.\n> > > \n> > > I'm sorry, but the code is inconsistent here. You basically have two\n> > > variables from one SoC to the other, the number of channels and\n> > > requests.\n> > > \n> > > In one case (channels), it mandates that the property is provided in\n> > > the device tree, and doesn't default to anything.\n> > > \n> > > In the other case (requests), the property is optional and it will\n> > > provide a default. All that in 20 lines.\n> > \n> > The channel number is a hardware property. Using more channels than the\n> > hardware provides is a bug. There is no default.\n> > \n> > The port/request is just some lax property to limit the resource\n> > allocation\n> > upfront. As long as the bindings of the different IP blocks (SPI, audio,\n> > ...) provide the correct port numbers, all required information is\n> > available.\n> Using an improper request ID or out of bounds will be just as much as\n> a bug. You will not get your DMA transfer to the proper device you\n> were trying to, the data will not reach the device or memory, your\n> driver will not work => a bug.\n> \n> It will not be for the same reasons, you will not overwrite other\n> registers, but the end result is just the same: your transfer will not\n> work.\n\nWriting adjacent registers breaks other users of the DMA controller. \n\"Everytime I play a sound, my MMC breaks\" - oh, what fun.\n\n> > > I guess we already reached that middle ground by providing them\n> > > through the DT, we just have to make sure we remain consistent.\n> > > \n> > > > *If* the number of dma channels and ports is ever increased,\n> > > > exceeding the current maximum, this would amount to major changes in\n> > > > the driver and maybe even warrant a completely new driver.\n> > > > \n> > > > > > 2. As long as the the limits are adhered to, no other\n> > > > > > registers/register\n> > > > > > fields are overwritten. As the channel number and port are used to\n> > > > > > calculate memory offsets bounds checking is IMHO a good idea.\n> > > > > \n> > > > > And this is true for many other resources, starting with the one\n> > > > > defined in reg. We don't error check every register range, clock\n> > > > > index, reset line, interrupt, DMA channel, the memory size, etc. yet\n> > > > > you could make the same argument.\n> > > > > \n> > > > > The DT has to be right, and we have to trust it. Otherwise we can\n> > > > > just\n> > > > > throw it away.\n> > > > \n> > > > So your argument here basically is - don't do any checks on DT\n> > > > provided\n> > > > values, these are always correct. So, following this argument, not\n> > > > only\n> > > > the\n> > > > range check, but also the of_property_read return values should be\n> > > > ignored, as the DT is correct, thus of_property_read will never return\n> > > > an\n> > > > error.\n> > > \n> > > No, my argument is don't do a check if you can catch only half of the\n> > > errors, and with no hope of fixing it.\n> > > \n> > > The functions you mentionned have a 100% error catch rate. This is the\n> > > difference.\n> > > \n> > > > That clearly does not match the implementation of drivers throughout\n> > > > the\n> > > > various subsystems for DT properties, which is in general - do all the\n> > > > checks that can be done, trust everything you can not verify.\n> > > \n> > > And my point is that we're falling into the latter here. You cannot\n> > > verify it properly.\n> > \n> > Please check the following line:\n> > \n> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dr\n> > ivers/dma/sun6i-dma.c#n951\n> > \n> > Thats far from 100% - the highest allowed port for each SoC differs\n> > between RX and TX, and port allocation is sparse.\n> \n> But until your patches, you *could* fix it and reach that 100%.\n\n1. You had 3 years to do that, but you never cared.\n2. Its still possible to do, just add a property to the devicetree.\n\n> And I guess now we could indeed remove it.\n> \n> Look, this discussion is going nowhere. I told you what the condition\n> for my Acked-by was already.\n\nYeah, and its your power as a so called maintainer to force your opinion on \nanyone crossing your way. Fine, go for it ...\n\nStefan","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2YTT06hKz9t6W\n\tfor ;\n\tThu, 28 Sep 2017 09:10:53 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752192AbdI0XKv convert rfc822-to-8bit (ORCPT\n\t);\n\tWed, 27 Sep 2017 19:10:51 -0400","from mail-out-1.itc.rwth-aachen.de ([134.130.5.46]:57810 \"EHLO\n\tmail-out-1.itc.rwth-aachen.de\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1752189AbdI0XKu (ORCPT\n\t); Wed, 27 Sep 2017 19:10:50 -0400","from rwthex-w2-a.rwth-ad.de ([134.130.26.158])\n\tby mail-in-1.itc.rwth-aachen.de with ESMTP; 28 Sep 2017 01:10:48 +0200","from pebbles.site (77.182.56.60) by rwthex-w2-a.rwth-ad.de\n\t(2002:8682:1a9e::8682:1a9e) with Microsoft SMTP Server\n\t(version=TLS1_2, \n\tcipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1034.26;\n\tThu, 28 Sep 2017 01:10:47 +0200"],"X-IronPort-AV":"E=Sophos;i=\"5.42,446,1500933600\"; d=\"scan'208\";a=\"15557831\"","From":"Stefan Bruens ","To":"Maxime Ripard ","CC":"\"linux-sunxi@googlegroups.com\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"dmaengine@vger.kernel.org\" ,\n\tVinod Koul ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-kernel@vger.kernel.org\" ,\n\tChen-Yu Tsai , Rob Herring ,\n\tCode Kipper ,\n\tAndre Przywara ","Subject":"Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max\n\trequest from devicetree","Date":"Thu, 28 Sep 2017 01:10:46 +0200","Message-ID":"<9434998.NR9mI8DG1i@pebbles.site>","In-Reply-To":"<20170927090922.u2hnvgz7yi55sjl3@flea>","References":"<20170917031956.28010-1-stefan.bruens@rwth-aachen.de>\n\t<3154933.I7MMVk5mkV@sbruens-linux>\n\t<20170927090922.u2hnvgz7yi55sjl3@flea>","MIME-Version":"1.0","Content-Transfer-Encoding":"8BIT","Content-Type":"text/plain; charset=\"iso-8859-1\"","X-Originating-IP":"[77.182.56.60]","X-ClientProxiedBy":"rwthex-s1-a.rwth-ad.de (2002:8682:1a98::8682:1a98) To\n\trwthex-w2-a.rwth-ad.de (2002:8682:1a9e::8682:1a9e)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]