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 20833C6FA99 for ; Sun, 12 Mar 2023 12:33:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229494AbjCLMd3 (ORCPT ); Sun, 12 Mar 2023 08:33:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46298 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229776AbjCLMd0 (ORCPT ); Sun, 12 Mar 2023 08:33:26 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AA8911D935; Sun, 12 Mar 2023 05:33:23 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id CB8AFB80B18; Sun, 12 Mar 2023 12:33:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C379EC433D2; Sun, 12 Mar 2023 12:33:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678624400; bh=wKK18JhLAidYuz7urwViijJ7ZzWu18IoQlSj7/PTI4Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eymBdvbLzTtRs6nBQ8S8SsE7NG6OOyokbEEAu2+UYecDgnuFGBPRsp2lyq+aixcOe 4QUQPnA5Ml3/EPjJ/78qW1dIZlnKn8l0z64Pzcc007TntpiL0AUTfSEJTPzBuh5JOL OIt2F7U3ccQLmcwVxxkFhWUYVmPuDas4QL8rbd9sN7t708A++tjaPP0z9rm6+bb4gp 8pm/U9GRpBatLyhesGz582RrB2Myu1qla1USA6uUjasI5FHMTFwCPdgcfAm1uRVfwl MKumMM1UiIsGwnR9jrt904v3srZFfEGFQPGRp322wd47pnK1zWgv3d2DT2Usj26Y9i es2PWSPYPJtCQ== Date: Sun, 12 Mar 2023 13:33:16 +0100 From: Andi Shyti To: Krzysztof Kozlowski Cc: Ryan Chen , Wolfram Sang , Joel Stanley , Brendan Higgins , Krzysztof Kozlowski , Andrew Jeffery , "devicetree@vger.kernel.org" , Philipp Zabel , Rob Herring , Benjamin Herrenschmidt , "linux-aspeed@lists.ozlabs.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "openbmc@lists.ozlabs.org" , "linux-i2c@vger.kernel.org" Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Message-ID: <20230312123316.rn2qm3jnw7iy5yts@intel.intel> References: <20230226031321.3126756-1-ryan_chen@aspeedtech.com> <20230226031321.3126756-2-ryan_chen@aspeedtech.com> <53090449-58c9-bc03-56df-aa8ae93c0c26@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Krzysztof and Ryan, > >>> + aspeed,timeout: > >>> + type: boolean > >>> + description: I2C bus timeout enable for master/slave mode > >> > >> Nothing improved here in regards to my last comment. > > > > Yes, as I know your require is about " DT binding to represent hardware setup" > > So I add more description about aspeed,timeout as blow. > > > > ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug. > > The following is board-specific design example. > > Board A Board B > > ------------------------- ------------------------ > > |i2c bus#1(master/slave) <===fingerprint ===> i2c bus#x (master/slave)| > > |i2c bus#2(master)-> tmp i2c device | | | > > |i2c bus#3(master)-> adc i2c device | | | > > ------------------------- ------------------------ > > > > aspeed,timout properites: > > For example I2C controller as slave mode, and suddenly disconnected. > > Slave state machine will keep waiting for master clock in for rx/tx transmit. > > So it need timeout setting to enable timeout unlock controller state. > > And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL. > > > > Do you mean add those description into ore aspeed,timout properites description? > > You are describing here one particular feature you want to enable in the > driver which looks non-scalable and more difficult to configure/use. > What I was looking for is to describe the actual configuration you have > (e.g. multi-master) which leads to enable or disable such feature in > your hardware. Especially that bool value does not scale later to actual > timeout values in time (ms)... > > I don't know I2C that much, but I wonder - why this should be specific > to Aspeed I2C and no other I2C controllers implement it? IOW, this looks > quite generic and every I2C controller should have it. Adding it > specific to Aspeed suggests that either we miss a generic property or > this should not be in DT at all (because no one else has it...). this property is missing in the i2c devicetree property and because this is the second driver needing it, I think it should be added. To be clear, this timeout means that the SCL is kept low for some number of milliseconds in order to force the slave to enter a wait state. This is done when the master has some particular needs as Ryan is describing. It's defined in the i2c specification, while smbus defines it in a range from 25 to 35 ms. In any case it's not a boolean value unless the controller has it defined internally by the firmware. So... nack! Please, hold a bit, I'm sending a patch. Andi 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 B693FC6FA99 for ; Sun, 12 Mar 2023 12:34:56 +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=j0yoTMGbTFenxkaZi7CqU/1bHZognR6jrsRN3N/D7NE=; b=ANGZu/fVj/UYFm yWXIExA7IG0QEOdKyKzcScojy0g+/R3LR9oVPa12o/iorh9LxE92rmKle9ZRwVQyFHD+QTkmVCByB vx29o4DZgQZ6KQ8Y0tMREYl53xhLYK+c+PsgE8NU+AqN77CIfdwp1UnQ7XFmv5nhA83Sqk42B7sIR SZzBwQzyWnwhWLS6NktETaKAgC1XG6J0+I4ALlgnPQBJOm8as/g9QNPWz7yLlWSWbPE0aFpCy+NoZ wXwxo5Gv6DSAuEsmhJJrh/aG+Gb68t7mGfUofGaaLr49jqIrv9uDcUll3lhW/5h0/aMwgdmegkuLr 7WKf1NsZFh6pe3GXJV1Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pbKtM-002E62-OX; Sun, 12 Mar 2023 12:33:36 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pbKtG-002E53-3d for linux-arm-kernel@lists.infradead.org; Sun, 12 Mar 2023 12:33:34 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A49F460F0F; Sun, 12 Mar 2023 12:33:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C379EC433D2; Sun, 12 Mar 2023 12:33:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678624400; bh=wKK18JhLAidYuz7urwViijJ7ZzWu18IoQlSj7/PTI4Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eymBdvbLzTtRs6nBQ8S8SsE7NG6OOyokbEEAu2+UYecDgnuFGBPRsp2lyq+aixcOe 4QUQPnA5Ml3/EPjJ/78qW1dIZlnKn8l0z64Pzcc007TntpiL0AUTfSEJTPzBuh5JOL OIt2F7U3ccQLmcwVxxkFhWUYVmPuDas4QL8rbd9sN7t708A++tjaPP0z9rm6+bb4gp 8pm/U9GRpBatLyhesGz582RrB2Myu1qla1USA6uUjasI5FHMTFwCPdgcfAm1uRVfwl MKumMM1UiIsGwnR9jrt904v3srZFfEGFQPGRp322wd47pnK1zWgv3d2DT2Usj26Y9i es2PWSPYPJtCQ== Date: Sun, 12 Mar 2023 13:33:16 +0100 From: Andi Shyti To: Krzysztof Kozlowski Cc: Ryan Chen , Wolfram Sang , Joel Stanley , Brendan Higgins , Krzysztof Kozlowski , Andrew Jeffery , "devicetree@vger.kernel.org" , Philipp Zabel , Rob Herring , Benjamin Herrenschmidt , "linux-aspeed@lists.ozlabs.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "openbmc@lists.ozlabs.org" , "linux-i2c@vger.kernel.org" Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Message-ID: <20230312123316.rn2qm3jnw7iy5yts@intel.intel> References: <20230226031321.3126756-1-ryan_chen@aspeedtech.com> <20230226031321.3126756-2-ryan_chen@aspeedtech.com> <53090449-58c9-bc03-56df-aa8ae93c0c26@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230312_053330_301821_6EDA99EA X-CRM114-Status: GOOD ( 27.13 ) 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 Hi Krzysztof and Ryan, > >>> + aspeed,timeout: > >>> + type: boolean > >>> + description: I2C bus timeout enable for master/slave mode > >> > >> Nothing improved here in regards to my last comment. > > > > Yes, as I know your require is about " DT binding to represent hardware setup" > > So I add more description about aspeed,timeout as blow. > > > > ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug. > > The following is board-specific design example. > > Board A Board B > > ------------------------- ------------------------ > > |i2c bus#1(master/slave) <===fingerprint ===> i2c bus#x (master/slave)| > > |i2c bus#2(master)-> tmp i2c device | | | > > |i2c bus#3(master)-> adc i2c device | | | > > ------------------------- ------------------------ > > > > aspeed,timout properites: > > For example I2C controller as slave mode, and suddenly disconnected. > > Slave state machine will keep waiting for master clock in for rx/tx transmit. > > So it need timeout setting to enable timeout unlock controller state. > > And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL. > > > > Do you mean add those description into ore aspeed,timout properites description? > > You are describing here one particular feature you want to enable in the > driver which looks non-scalable and more difficult to configure/use. > What I was looking for is to describe the actual configuration you have > (e.g. multi-master) which leads to enable or disable such feature in > your hardware. Especially that bool value does not scale later to actual > timeout values in time (ms)... > > I don't know I2C that much, but I wonder - why this should be specific > to Aspeed I2C and no other I2C controllers implement it? IOW, this looks > quite generic and every I2C controller should have it. Adding it > specific to Aspeed suggests that either we miss a generic property or > this should not be in DT at all (because no one else has it...). this property is missing in the i2c devicetree property and because this is the second driver needing it, I think it should be added. To be clear, this timeout means that the SCL is kept low for some number of milliseconds in order to force the slave to enter a wait state. This is done when the master has some particular needs as Ryan is describing. It's defined in the i2c specification, while smbus defines it in a range from 25 to 35 ms. In any case it's not a boolean value unless the controller has it defined internally by the firmware. So... nack! Please, hold a bit, I'm sending a patch. Andi _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 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 39253C61DA4 for ; Mon, 13 Mar 2023 09:42:46 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4PZsC825ddz3c7S for ; Mon, 13 Mar 2023 20:42:44 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=eymBdvbL; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=kernel.org (client-ip=139.178.84.217; helo=dfw.source.kernel.org; envelope-from=andi.shyti@kernel.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=eymBdvbL; dkim-atps=neutral Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) (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 4PZK2W2k3bz3c6t; Sun, 12 Mar 2023 23:33:23 +1100 (AEDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A49F460F0F; Sun, 12 Mar 2023 12:33:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C379EC433D2; Sun, 12 Mar 2023 12:33:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678624400; bh=wKK18JhLAidYuz7urwViijJ7ZzWu18IoQlSj7/PTI4Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eymBdvbLzTtRs6nBQ8S8SsE7NG6OOyokbEEAu2+UYecDgnuFGBPRsp2lyq+aixcOe 4QUQPnA5Ml3/EPjJ/78qW1dIZlnKn8l0z64Pzcc007TntpiL0AUTfSEJTPzBuh5JOL OIt2F7U3ccQLmcwVxxkFhWUYVmPuDas4QL8rbd9sN7t708A++tjaPP0z9rm6+bb4gp 8pm/U9GRpBatLyhesGz582RrB2Myu1qla1USA6uUjasI5FHMTFwCPdgcfAm1uRVfwl MKumMM1UiIsGwnR9jrt904v3srZFfEGFQPGRp322wd47pnK1zWgv3d2DT2Usj26Y9i es2PWSPYPJtCQ== Date: Sun, 12 Mar 2023 13:33:16 +0100 From: Andi Shyti To: Krzysztof Kozlowski Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2 Message-ID: <20230312123316.rn2qm3jnw7iy5yts@intel.intel> References: <20230226031321.3126756-1-ryan_chen@aspeedtech.com> <20230226031321.3126756-2-ryan_chen@aspeedtech.com> <53090449-58c9-bc03-56df-aa8ae93c0c26@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailman-Approved-At: Mon, 13 Mar 2023 20:41:51 +1100 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: "devicetree@vger.kernel.org" , Ryan Chen , Philipp Zabel , "linux-aspeed@lists.ozlabs.org" , Andrew Jeffery , "openbmc@lists.ozlabs.org" , "linux-kernel@vger.kernel.org" , Wolfram Sang , Brendan Higgins , Rob Herring , Joel Stanley , Krzysztof Kozlowski , "linux-arm-kernel@lists.infradead.org" , "linux-i2c@vger.kernel.org" Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" Hi Krzysztof and Ryan, > >>> + aspeed,timeout: > >>> + type: boolean > >>> + description: I2C bus timeout enable for master/slave mode > >> > >> Nothing improved here in regards to my last comment. > > > > Yes, as I know your require is about " DT binding to represent hardware setup" > > So I add more description about aspeed,timeout as blow. > > > > ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug. > > The following is board-specific design example. > > Board A Board B > > ------------------------- ------------------------ > > |i2c bus#1(master/slave) <===fingerprint ===> i2c bus#x (master/slave)| > > |i2c bus#2(master)-> tmp i2c device | | | > > |i2c bus#3(master)-> adc i2c device | | | > > ------------------------- ------------------------ > > > > aspeed,timout properites: > > For example I2C controller as slave mode, and suddenly disconnected. > > Slave state machine will keep waiting for master clock in for rx/tx transmit. > > So it need timeout setting to enable timeout unlock controller state. > > And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL. > > > > Do you mean add those description into ore aspeed,timout properites description? > > You are describing here one particular feature you want to enable in the > driver which looks non-scalable and more difficult to configure/use. > What I was looking for is to describe the actual configuration you have > (e.g. multi-master) which leads to enable or disable such feature in > your hardware. Especially that bool value does not scale later to actual > timeout values in time (ms)... > > I don't know I2C that much, but I wonder - why this should be specific > to Aspeed I2C and no other I2C controllers implement it? IOW, this looks > quite generic and every I2C controller should have it. Adding it > specific to Aspeed suggests that either we miss a generic property or > this should not be in DT at all (because no one else has it...). this property is missing in the i2c devicetree property and because this is the second driver needing it, I think it should be added. To be clear, this timeout means that the SCL is kept low for some number of milliseconds in order to force the slave to enter a wait state. This is done when the master has some particular needs as Ryan is describing. It's defined in the i2c specification, while smbus defines it in a range from 25 to 35 ms. In any case it's not a boolean value unless the controller has it defined internally by the firmware. So... nack! Please, hold a bit, I'm sending a patch. Andi