From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Date: Tue, 5 Sep 2023 14:55:19 +0300 Subject: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver In-Reply-To: References: <20230714074522.23827-1-ryan_chen@aspeedtech.com> <20230714074522.23827-3-ryan_chen@aspeedtech.com> Message-ID: List-Id: To: linux-aspeed@lists.ozlabs.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tue, Sep 05, 2023 at 06:52:37AM +0000, Ryan Chen wrote: > > From: Andy Shevchenko > > Sent: Friday, July 14, 2023 4:55 PM > > On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote: ... > > > +#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) & > > GENMASK(7, 0)) > > > > > +#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) & > > GENMASK(5, 0)) > > > > > +#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) & > > GENMASK(4, 0)) + 1) > > > > With right shifts it's better to have GENMASK to be applied first, it will show > > exact MSB of the bitfield. > > > > (Ditto for other cases similar to these) > It will update next version. > #define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24)) >> 24) > #define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8)) >> 8) + 1) Note, these were just an example, check _all_ of the similar cases. In general any reviewer's comment should be considered for the entire code where it makes sense. ... > divisor = DIV_ROUND_UP(base_clk[3], i2c_bus->timing_info.bus_freq_hz); > for_each_set_bit(divisor, &divisor, 32) { This is wrong. > if ((divisor + 1) <= 32) > break; > divisor >>= 1; And this. > baseclk_idx++; > } for_each_set_bit() should use two different variables. > } else { > baseclk_idx = i + 1; > divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); > } > } ... > divisor = min_t(unsigned long, divisor, 32); Can't you use min()? min_t is a beast with some subtle corner cases. ... > Sorry I don't catch this split slave out to separate change? > Do you mean go for another file name example ast2600_i2c_slave.c ? No, I mean patch 1: Introduce the driver with only master support patch 2: Add slave capability (all what is under ifdeffery for I2C_SLAVE) ... > static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) > { > struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > int ret; This is not needed, you may return directly. > /* send start */ > dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), > msg->len, msg->len > 1 ? "s" : "", > msg->flags & I2C_M_RD ? "from" : "to", msg->addr); > > i2c_bus->master_xfer_cnt = 0; > i2c_bus->buf_index = 0; > if (msg->flags & I2C_M_RD) { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_rx(i2c_bus); return ...; if ... > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_rx(i2c_bus); > else > ret = ast2600_i2c_setup_byte_rx(i2c_bus); > } else { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus); > else > ret = ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); Same way. > } > > return ret; > } ... > > Wrong memory accessors. You should use something from asm/byteorder.h > > which includes linux/byteorder/generic.h. > > Sorry, about these parts. I quit no idea. > This is chip register limited, it only support dword write, not support byte write. > So the only way I have is use get_unaligned_le32 function get the byte buffer to align dword write. > Or I may need your help point me a good way. > for (i = 0; i < xfer_len; i++) { > wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > if (i % 4 == 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - 3); > } > } > > if (--i % 4 != 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); > } Something like that. The most problematic part in your original code is dereferencing byte memory as 32-bit memory with all possible problems behind. With this code it's gone. The code itself might be improved even more, you can think about it, you still have time (we are now in v6.7 cycle). -- With Best Regards, Andy Shevchenko From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4A5C5CA0FFD for ; Tue, 5 Sep 2023 16:19:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238253AbjIEQTW (ORCPT ); Tue, 5 Sep 2023 12:19:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40070 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1354473AbjIELzh (ORCPT ); Tue, 5 Sep 2023 07:55:37 -0400 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0D1921AD; Tue, 5 Sep 2023 04:55:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1693914933; x=1725450933; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=/cTDMMGwDXqGrvj5wts+NYLjOYzxjei9B4vYLZWvOR0=; b=m4MIBKoiPBgQTE54zvHPp8kcyxzD3ladLowAL+2VMTSqwzXvyzCZiZE+ 4wMKNY7aow/77tDEkZiL6cz0IgfSOwDrPNf24AJBUiaH521UuBLjIEkAf RBPUxbOcVXa+83u/cnbjU8WOawh+AkzUAXhVBeXcBcVYf1QGbcqK+QoQX qc2hwaE4DcQcr3SvFqKCGgLn5s97FfZVUFMtB/yspQhtx461pGa8uNL4+ 2FtmOL+JxtjxGHqKmo4Xtpt7nlLJoOLrfm6yiCUfD4iBs0HJ08bYTrEsL iwbl07w7f1RKIYB/a6Hq/97ZW2zPU7oBcCNzNKEnrP5hwBYzc5RqqBPUA w==; X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="440751289" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="440751289" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="884290537" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="884290537" Received: from unknown (HELO smile.fi.intel.com) ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:17 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1qdUeN-006fwq-2t; Tue, 05 Sep 2023 14:55:19 +0300 Date: Tue, 5 Sep 2023 14:55:19 +0300 From: Andy Shevchenko To: Ryan Chen Cc: "jk@codeconstruct.com.au" , Brendan Higgins , Benjamin Herrenschmidt , Joel Stanley , Rob Herring , Krzysztof Kozlowski , Andrew Jeffery , Philipp Zabel , Wolfram Sang , "linux-i2c@vger.kernel.org" , Florian Fainelli , Jean Delvare , William Zhang , Tyrone Ting , Tharun Kumar P , Conor Dooley , Phil Edworthy , "openbmc@lists.ozlabs.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-aspeed@lists.ozlabs.org" , "=linux-kernel@vger.kernel.org" <=linux-kernel@vger.kernel.org>, Andi Shyti Subject: Re: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver Message-ID: References: <20230714074522.23827-1-ryan_chen@aspeedtech.com> <20230714074522.23827-3-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org On Tue, Sep 05, 2023 at 06:52:37AM +0000, Ryan Chen wrote: > > From: Andy Shevchenko > > Sent: Friday, July 14, 2023 4:55 PM > > On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote: ... > > > +#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) & > > GENMASK(7, 0)) > > > > > +#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) & > > GENMASK(5, 0)) > > > > > +#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) & > > GENMASK(4, 0)) + 1) > > > > With right shifts it's better to have GENMASK to be applied first, it will show > > exact MSB of the bitfield. > > > > (Ditto for other cases similar to these) > It will update next version. > #define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24)) >> 24) > #define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8)) >> 8) + 1) Note, these were just an example, check _all_ of the similar cases. In general any reviewer's comment should be considered for the entire code where it makes sense. ... > divisor = DIV_ROUND_UP(base_clk[3], i2c_bus->timing_info.bus_freq_hz); > for_each_set_bit(divisor, &divisor, 32) { This is wrong. > if ((divisor + 1) <= 32) > break; > divisor >>= 1; And this. > baseclk_idx++; > } for_each_set_bit() should use two different variables. > } else { > baseclk_idx = i + 1; > divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); > } > } ... > divisor = min_t(unsigned long, divisor, 32); Can't you use min()? min_t is a beast with some subtle corner cases. ... > Sorry I don't catch this split slave out to separate change? > Do you mean go for another file name example ast2600_i2c_slave.c ? No, I mean patch 1: Introduce the driver with only master support patch 2: Add slave capability (all what is under ifdeffery for I2C_SLAVE) ... > static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) > { > struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > int ret; This is not needed, you may return directly. > /* send start */ > dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), > msg->len, msg->len > 1 ? "s" : "", > msg->flags & I2C_M_RD ? "from" : "to", msg->addr); > > i2c_bus->master_xfer_cnt = 0; > i2c_bus->buf_index = 0; > if (msg->flags & I2C_M_RD) { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_rx(i2c_bus); return ...; if ... > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_rx(i2c_bus); > else > ret = ast2600_i2c_setup_byte_rx(i2c_bus); > } else { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus); > else > ret = ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); Same way. > } > > return ret; > } ... > > Wrong memory accessors. You should use something from asm/byteorder.h > > which includes linux/byteorder/generic.h. > > Sorry, about these parts. I quit no idea. > This is chip register limited, it only support dword write, not support byte write. > So the only way I have is use get_unaligned_le32 function get the byte buffer to align dword write. > Or I may need your help point me a good way. > for (i = 0; i < xfer_len; i++) { > wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > if (i % 4 == 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - 3); > } > } > > if (--i % 4 != 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); > } Something like that. The most problematic part in your original code is dereferencing byte memory as 32-bit memory with all possible problems behind. With this code it's gone. The code itself might be improved even more, you can think about it, you still have time (we are now in v6.7 cycle). -- With Best Regards, Andy Shevchenko From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3DDD8C83F33 for ; Tue, 5 Sep 2023 11:56:41 +0000 (UTC) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=Yk+t/9K9; dkim-atps=neutral Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4Rg3rR5CDRz3c3k for ; Tue, 5 Sep 2023 21:56:39 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=Yk+t/9K9; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=linux.intel.com (client-ip=134.134.136.31; helo=mgamail.intel.com; envelope-from=andriy.shevchenko@linux.intel.com; receiver=lists.ozlabs.org) Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Rg3qG5C8Rz2xdb; Tue, 5 Sep 2023 21:55:36 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1693914939; x=1725450939; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=/cTDMMGwDXqGrvj5wts+NYLjOYzxjei9B4vYLZWvOR0=; b=Yk+t/9K9bTAFuVVnJhH+niAX+Kt6j0FP55CseM4iAZOXPTyekZVFlASE Nmo5XMsbThOG5nLCYYronTiQtFvMDD71n3s+DewV4ei7OD72J+Gjgbtgg ZoOUU7Jq9XVOQ+Jaas77YKKlYy8Gdoc8t2n+jzrmyh77GbpIkc2cdQWbr zlcDHLcTZobJLdw2J1htZ/4u1x6wV3H7KAYt7HBa52cGMYiHp3jCbv8I2 sToiRhLA0R3ao8/p8XjNNoxRY1sA45gdobcM43BkbvNMShaRbTMuCaz9a aJr3o2rb2ji1YNPcjL8uuWvPcCOpoZbM9DAr6N5r/JT/h6E4TRskjL6rj g==; X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="440751291" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="440751291" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="884290537" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="884290537" Received: from unknown (HELO smile.fi.intel.com) ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:17 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1qdUeN-006fwq-2t; Tue, 05 Sep 2023 14:55:19 +0300 Date: Tue, 5 Sep 2023 14:55:19 +0300 From: Andy Shevchenko To: Ryan Chen Subject: Re: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver Message-ID: References: <20230714074522.23827-1-ryan_chen@aspeedtech.com> <20230714074522.23827-3-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "linux-aspeed@lists.ozlabs.org" , Brendan Higgins , Conor Dooley , "linux-i2c@vger.kernel.org" , Krzysztof Kozlowski , "jk@codeconstruct.com.au" , Jean Delvare , Andi Shyti , Phil Edworthy , Florian Fainelli , "=linux-kernel@vger.kernel.org" <=linux-kernel@vger.kernel.org>, "openbmc@lists.ozlabs.org" , Joel Stanley , "devicetree@vger.kernel.org" , William Zhang , Rob Herring , "linux-arm-kernel@lists.infradead.org" , Tharun Kumar P , Andrew Jeffery , Wolfram Sang , Tyrone Ting , Philipp Zabel Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" On Tue, Sep 05, 2023 at 06:52:37AM +0000, Ryan Chen wrote: > > From: Andy Shevchenko > > Sent: Friday, July 14, 2023 4:55 PM > > On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote: ... > > > +#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) & > > GENMASK(7, 0)) > > > > > +#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) & > > GENMASK(5, 0)) > > > > > +#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) & > > GENMASK(4, 0)) + 1) > > > > With right shifts it's better to have GENMASK to be applied first, it will show > > exact MSB of the bitfield. > > > > (Ditto for other cases similar to these) > It will update next version. > #define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24)) >> 24) > #define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8)) >> 8) + 1) Note, these were just an example, check _all_ of the similar cases. In general any reviewer's comment should be considered for the entire code where it makes sense. ... > divisor = DIV_ROUND_UP(base_clk[3], i2c_bus->timing_info.bus_freq_hz); > for_each_set_bit(divisor, &divisor, 32) { This is wrong. > if ((divisor + 1) <= 32) > break; > divisor >>= 1; And this. > baseclk_idx++; > } for_each_set_bit() should use two different variables. > } else { > baseclk_idx = i + 1; > divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); > } > } ... > divisor = min_t(unsigned long, divisor, 32); Can't you use min()? min_t is a beast with some subtle corner cases. ... > Sorry I don't catch this split slave out to separate change? > Do you mean go for another file name example ast2600_i2c_slave.c ? No, I mean patch 1: Introduce the driver with only master support patch 2: Add slave capability (all what is under ifdeffery for I2C_SLAVE) ... > static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) > { > struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > int ret; This is not needed, you may return directly. > /* send start */ > dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), > msg->len, msg->len > 1 ? "s" : "", > msg->flags & I2C_M_RD ? "from" : "to", msg->addr); > > i2c_bus->master_xfer_cnt = 0; > i2c_bus->buf_index = 0; > if (msg->flags & I2C_M_RD) { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_rx(i2c_bus); return ...; if ... > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_rx(i2c_bus); > else > ret = ast2600_i2c_setup_byte_rx(i2c_bus); > } else { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus); > else > ret = ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); Same way. > } > > return ret; > } ... > > Wrong memory accessors. You should use something from asm/byteorder.h > > which includes linux/byteorder/generic.h. > > Sorry, about these parts. I quit no idea. > This is chip register limited, it only support dword write, not support byte write. > So the only way I have is use get_unaligned_le32 function get the byte buffer to align dword write. > Or I may need your help point me a good way. > for (i = 0; i < xfer_len; i++) { > wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > if (i % 4 == 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - 3); > } > } > > if (--i % 4 != 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); > } Something like that. The most problematic part in your original code is dereferencing byte memory as 32-bit memory with all possible problems behind. With this code it's gone. The code itself might be improved even more, you can think about it, you still have time (we are now in v6.7 cycle). -- With Best Regards, Andy Shevchenko From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 510B8C83F33 for ; Tue, 5 Sep 2023 11:56:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Hg/pBs3IsyIutfFA++aNbYmAcFZkQ73mMAQZqQYTENQ=; b=qcKbJOKugPy7K8 VBXGljR6NTLIGTlc0jPSLqtidd54zRkpl/ccSNn3Leby1MHKFie60F9nJSfQtqro1n4P+5uwiBFkU 7giSQdO3+0qIEbiPFoo/KxzDTIF52F+oPnZExBeNEFj9K0muk0eP6Tfu+3UalWuy5DTeJpAmVzi9k 0sna/oLVZbyFwgDj/IXHKv7so55067p2nIPMQc1pxixtmzIGS2BCAX2T6AzCuBKJ5UWvRGkdq5IxU w56AdMPOVpev2xacPVAi1viqHg553jlOIH9ybSRVQlQZgB1BsBuJaF6S03uBBWeEJEKwKkI5ifaPg aL31PaD9Cumt+5GYhAxw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qdUeh-005xK4-2s; Tue, 05 Sep 2023 11:55:39 +0000 Received: from mgamail.intel.com ([134.134.136.31]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qdUee-005xIS-1g for linux-arm-kernel@lists.infradead.org; Tue, 05 Sep 2023 11:55:38 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1693914936; x=1725450936; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=/cTDMMGwDXqGrvj5wts+NYLjOYzxjei9B4vYLZWvOR0=; b=KrPBYtt6J05oZptHafCOJFqIpPjjXlnSl4aDnrLG7C7TLajRBBdqb4mo iUBl+BQOna5fIu47NEHQt6bEoi0w6DCKv5RsaGjDHZjbMhc6Uksv+w7kp /fGPivDx1Y643KpHzSGvG9SW9AOCj7tSvLsdgArFLaWAnvv7wOQrbJtlx JDs+aGPXNHP7Q5eLFOpjNjxqRCFujWee8h0mHPUTGvyMxWfxild/npHFP NoqvZv2YF0nZrspDY9S0lqX3G9got4QKND+ANIInE5akpWkrsOEY6pyJA wFy3Zwpu0xCDsH16Iu0GTjersf0tphjDWYnnQAmavvx055j3e5vfDWqKY Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="440751298" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="440751298" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10823"; a="884290537" X-IronPort-AV: E=Sophos;i="6.02,229,1688454000"; d="scan'208";a="884290537" Received: from unknown (HELO smile.fi.intel.com) ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Sep 2023 04:55:17 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1qdUeN-006fwq-2t; Tue, 05 Sep 2023 14:55:19 +0300 Date: Tue, 5 Sep 2023 14:55:19 +0300 From: Andy Shevchenko To: Ryan Chen Cc: "jk@codeconstruct.com.au" , Brendan Higgins , Benjamin Herrenschmidt , Joel Stanley , Rob Herring , Krzysztof Kozlowski , Andrew Jeffery , Philipp Zabel , Wolfram Sang , "linux-i2c@vger.kernel.org" , Florian Fainelli , Jean Delvare , William Zhang , Tyrone Ting , Tharun Kumar P , Conor Dooley , Phil Edworthy , "openbmc@lists.ozlabs.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-aspeed@lists.ozlabs.org" , "=linux-kernel@vger.kernel.org" <=linux-kernel@vger.kernel.org>, Andi Shyti Subject: Re: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver Message-ID: References: <20230714074522.23827-1-ryan_chen@aspeedtech.com> <20230714074522.23827-3-ryan_chen@aspeedtech.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230905_045536_622917_B0CD4FAE X-CRM114-Status: GOOD ( 31.76 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Sep 05, 2023 at 06:52:37AM +0000, Ryan Chen wrote: > > From: Andy Shevchenko > > Sent: Friday, July 14, 2023 4:55 PM > > On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote: ... > > > +#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) & > > GENMASK(7, 0)) > > > > > +#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) & > > GENMASK(5, 0)) > > > > > +#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) & > > GENMASK(4, 0)) + 1) > > > > With right shifts it's better to have GENMASK to be applied first, it will show > > exact MSB of the bitfield. > > > > (Ditto for other cases similar to these) > It will update next version. > #define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24)) >> 24) > #define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8)) >> 8) + 1) Note, these were just an example, check _all_ of the similar cases. In general any reviewer's comment should be considered for the entire code where it makes sense. ... > divisor = DIV_ROUND_UP(base_clk[3], i2c_bus->timing_info.bus_freq_hz); > for_each_set_bit(divisor, &divisor, 32) { This is wrong. > if ((divisor + 1) <= 32) > break; > divisor >>= 1; And this. > baseclk_idx++; > } for_each_set_bit() should use two different variables. > } else { > baseclk_idx = i + 1; > divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); > } > } ... > divisor = min_t(unsigned long, divisor, 32); Can't you use min()? min_t is a beast with some subtle corner cases. ... > Sorry I don't catch this split slave out to separate change? > Do you mean go for another file name example ast2600_i2c_slave.c ? No, I mean patch 1: Introduce the driver with only master support patch 2: Add slave capability (all what is under ifdeffery for I2C_SLAVE) ... > static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) > { > struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > int ret; This is not needed, you may return directly. > /* send start */ > dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), > msg->len, msg->len > 1 ? "s" : "", > msg->flags & I2C_M_RD ? "from" : "to", msg->addr); > > i2c_bus->master_xfer_cnt = 0; > i2c_bus->buf_index = 0; > if (msg->flags & I2C_M_RD) { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_rx(i2c_bus); return ...; if ... > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_rx(i2c_bus); > else > ret = ast2600_i2c_setup_byte_rx(i2c_bus); > } else { > if (i2c_bus->mode == DMA_MODE) > ret = ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); > else if (i2c_bus->mode == BUFF_MODE) > ret = ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus); > else > ret = ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); Same way. > } > > return ret; > } ... > > Wrong memory accessors. You should use something from asm/byteorder.h > > which includes linux/byteorder/generic.h. > > Sorry, about these parts. I quit no idea. > This is chip register limited, it only support dword write, not support byte write. > So the only way I have is use get_unaligned_le32 function get the byte buffer to align dword write. > Or I may need your help point me a good way. > for (i = 0; i < xfer_len; i++) { > wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > if (i % 4 == 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - 3); > } > } > > if (--i % 4 != 3) { > wbuf_dword = get_unaligned_le32(wbuf); > writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); > } Something like that. The most problematic part in your original code is dereferencing byte memory as 32-bit memory with all possible problems behind. With this code it's gone. The code itself might be improved even more, you can think about it, you still have time (we are now in v6.7 cycle). -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel