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.gnu.org (lists.gnu.org [209.51.188.17]) (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 66D5FC433EF for ; Thu, 2 Jun 2022 15:45:46 +0000 (UTC) Received: from localhost ([::1]:54868 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nwn17-0003L2-EW for qemu-devel@archiver.kernel.org; Thu, 02 Jun 2022 11:45:45 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:45452) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nwmwF-0008UW-P7; Thu, 02 Jun 2022 11:40:44 -0400 Received: from 3.mo552.mail-out.ovh.net ([178.33.254.192]:49717) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nwmwD-0003vl-34; Thu, 02 Jun 2022 11:40:43 -0400 Received: from mxplan5.mail.ovh.net (unknown [10.108.16.173]) by mo552.mail-out.ovh.net (Postfix) with ESMTPS id F15D124840; Thu, 2 Jun 2022 15:40:34 +0000 (UTC) Received: from kaod.org (37.59.142.99) by DAG4EX1.mxp5.local (172.16.2.31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.9; Thu, 2 Jun 2022 17:40:34 +0200 Authentication-Results: garm.ovh; auth=pass (GARM-99G003c96a2334-4ef5-4876-9cda-2102976dd278, 123C0E545D135716686AEE7B3F9DB382F3F098BA) smtp.auth=clg@kaod.org X-OVh-ClientIp: 82.64.250.170 Message-ID: Date: Thu, 2 Jun 2022 17:40:33 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [RFC PATCH v2 0/6] hw/i2c: i2c slave mode support Content-Language: en-US To: Jae Hyun Yoo , Klaus Jensen CC: , Jonathan Cameron , , Peter Delevoryas , Peter Maydell , Corey Minyard , Padmakar Kalghatgi , Damien Hedde , Andrew Jeffery , Joel Stanley , Arun Kumar Kashinath Agasar , Klaus Jensen , Zev Weiss References: <20220601210831.67259-1-its@irrelevant.dk> <6e0eb197-25c2-6b1e-2c19-f93597e29cff@kaod.org> <00e2d10a-20f5-8357-5b13-41791940ce19@kaod.org> <5683a737-8a15-20c5-5716-f5216d6c33c8@quicinc.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= In-Reply-To: <5683a737-8a15-20c5-5716-f5216d6c33c8@quicinc.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [37.59.142.99] X-ClientProxiedBy: DAG6EX1.mxp5.local (172.16.2.51) To DAG4EX1.mxp5.local (172.16.2.31) X-Ovh-Tracer-GUID: dd4ce5a8-fdfe-46e1-b117-d05e5dd65248 X-Ovh-Tracer-Id: 15704614854151670657 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedvfedrleefucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuqfggjfdpvefjgfevmfevgfenuceurghilhhouhhtmecuhedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhepkfffgggfuffvvehfhfgjtgfgihesthekredttdefjeenucfhrhhomhepveorughrihgtpgfnvggpifhorghtvghruceotghlgheskhgrohgurdhorhhgqeenucggtffrrghtthgvrhhnpefhfeevveegueejhfettdeuvdejtefgkeffkeejgeeuteejgfduieelhedttefhtdenucffohhmrghinhepkhgvrhhnvghlrdhorhhgpdhgihhthhhusgdrtghomhenucfkpheptddrtddrtddrtddpfeejrdehledrudegvddrleelnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmohguvgepshhmthhpohhuthdphhgvlhhopehmgihplhgrnhehrdhmrghilhdrohhvhhdrnhgvthdpihhnvghtpedtrddtrddtrddtpdhmrghilhhfrhhomheptghlgheskhgrohgurdhorhhgpdhnsggprhgtphhtthhopedupdhrtghpthhtohepiigvvhessggvfihilhguvghrsggvvghsthdrnhgvthdpoffvtefjohhsthepmhhoheehvd Received-SPF: pass client-ip=178.33.254.192; envelope-from=clg@kaod.org; helo=3.mo552.mail-out.ovh.net X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, NICE_REPLY_A=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 6/2/22 16:29, Jae Hyun Yoo wrote: > Hi Klaus, > > On 6/2/2022 6:50 AM, Cédric Le Goater wrote: >> On 6/2/22 10:21, Klaus Jensen wrote: >>> On Jun  2 09:52, Cédric Le Goater wrote: >>>> On 6/1/22 23:08, Klaus Jensen wrote: >>>>> From: Klaus Jensen >>>>> >>>>> Hi all, >>>>> >>>>> This RFC series adds I2C "slave mode" support for the Aspeed I2C >>>> >>>> I think you can remove the RFC prefix. >>>> >>>>> controller as well as the necessary infrastructure in the i2c core to >>>>> support this. >>>>> >>>>> v2 changes >>>>> ~~~~~~~~~~ >>>>> I finally got around to working on this again. I'm sorry for not >>>>> bringing a v2 to the table earlier. >>>>> >>>>> Mad props to Peter and Jonathan for putting this series to work and >>>>> pushing it forward! Thanks! >>>>> >>>>> This series is based off Cédric's aspeed-7.1 tree, so it includes the >>>>> register fields. This is all "old register mode", but Peter seems to >>>>> have added support in new mode. >>>>> >>>>> There are some loose ends of course, i.e send_async doesn't handle >>>>> broadcast and asynchronous slaves being sent stuff can't nack. But I >>>>> wanted to get some feedback on the interface before I tackle that. >>>>> >>>>> This series >>>>> ~~~~~~~~~~~ >>>>> Patch 1 and 2 are small Aspeed I2C changes/additions. >>>>> >>>>> Patch 3 adds support for multiple masters in the i2c core, allowing >>>>> slaves to master the bus and (safely) issue i2c_send/recv(). >>>>> >>>>> Patch 4 adds an asynchronous send i2c_send_async(I2CBus *, uint8) on the >>>>> bus that must be paired with an explicit ack using i2c_ack(I2CBus *). We >>>>> have previously discussed how we wanted to handle the issue that some >>>>> slaves implement this and some do not. Using a QOM interface was up, but >>>>> couldn't figure out a good way to do it. I ended up decided against it >>>>> since I believe this have to be a run-time check anyway. The problem is >>>>> that a slave can master the bus and try to communicate with *anyone* on >>>>> the bus - and there is no reason why we should only allow asynchronous >>>>> slaves on the bus in that case, or whatever we would want to do when >>>>> devices are plugged. So, instead, the current master can issue an >>>>> i2c_start_send() and if that fails (because it isnt implemented by the >>>>> target slave) it can either bail out or use i2c_start_send_async() if it >>>>> itself supports it. This works the other way around as well of course, >>>>> but it is probably simpler to handle slaves that respond to >>>>> i2c_start_send(). This approach relies on adding a new i2c_event, which >>>>> is why a bunch of other devices needs changes in their event handling. >>>>> >>>>> Patch 5 adds *partial* slave mode functionality to the emulated Aspeed >>>>> I2C controller, that is, it only supports asynchronous sends started by >>>>> another slave that is currently mastering the bus. No asynchronous >>>>> receive. >>>> >>>> If there are no objections, I think this is a good way to move forward >>>> and improve this initial implementation when the need arises. >>>> >>> >>> There is an outstanding issue with the SLAVE_ADDR_RX_MATCH interrupt bit >>> (bit 7). Remember from my first series I had a workaround to make sure >>> it wasnt masked. >>> >>> I posted this upstream to linux >>> >>> https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/ >>> >>> Not sure if that is the right way to fix it. >> >> That's weird. I would have thought it was already enabled [ Adding Jae ] > > Slave mode support in Aspeed I2C driver is already enabled and it has > worked well so far. The fix Klaus made in the link is incorrect. > > https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/ > > The patch is adding ASPEED_I2CD_INTR_SLAVE_MATCH as a mask bit for > I2CD0C (Interrupt Control Register) but actually this bit is part of > I2CD10 (Interrupt Status Register). Means that the slave match interrupt > can be enabled without enabling any mask bit in I2CD0C. Thanks Jae. So we should enable this interrupt always independently of the Interrupt Control Register value. I would simply extend the mask value (bus->regs[intr_ctrl_reg]) with the SLAVE_ADDR_RX_MATCH bit when interrupts are raised in aspeed_i2c_bus_raise_interrupt(). Other topics, The mask used in : case A_I2CD_INTR_CTRL: bus->regs[R_I2CD_INTR_CTRL] = value & 0x7FFF; break; is incorrect. It should be: 0x0000707f for ast2400 0x0000f07f for ast2500 and ast2600 I think we can use 0x0000f07f for all to reduce a bit the complexity. aspeed_i2c_bus_reset() needs a fix to reset the new mode registers also. Thanks, C. > Thanks, > Jae > >>> You mentioned something about "fixing" a mask on the ast2600? >> >> This can be addressed later. >> >> The model could be more precise since the driver is masking the value >> already we should be fine. See commit 3fb2e2aeafb2 ("i2c: aspeed: disable >> additional device addresses on ast2[56]xx") from Zeiv. >> >>  From the datasheet. >> On the AST2400 (only 1 slave address) >> >>    * no upper bits >> >> On the AST2500 (2 possible slave addresses), >> >>    * bit[31] : Slave Address match indicator >>    * bit[30] : Slave Address Receiving pending >> >> On the AST2600 (3 possible slave addresses), >> >>    * bit[31-30] : Slave Address match indicator >>    * bit[29] : Slave Address Receiving pending >> >> Thanks, >> >> C. >> >>> >>> But with the above patch, all works an intended and no "workaround" >>> required. >>> >>>>> Finally, patch 6 adds an example device using this new API. The device >>>>> is a simple "echo" device that upon being sent a set of bytes uses the >>>>> first byte as the address of the slave to echo to. >>>>> >>>>> With this combined I am able to boot up Linux on an emulated Aspeed 2600 >>>>> evaluation board and have the i2c echo device write into a Linux slave >>>>> EEPROM. Assuming the echo device is on address 0x42: >>>>> >>>>>     # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-15/new_device >>>>>     i2c i2c-15: new_device: Instantiated device slave-24c02 at 0x64 >>>>>     # i2cset -y 15 0x42 0x64 0x00 0xaa i >>>>>     # hexdump /sys/bus/i2c/devices/15-1064/slave-eeprom >>>>>     0000000 ffaa ffff ffff ffff ffff ffff ffff ffff >>>>>     0000010 ffff ffff ffff ffff ffff ffff ffff ffff >>>>>     * >>>>>     0000100 >>>> >>>> I have started working on buildroot images  : >>>> >>>>    https://github.com/legoater/buildroot/commits/aspeed >>>> >>>> The resulting files are quite small : >>>> >>>>      $ ll output/images/ >>>>      total 86040 >>>>      drwxr-xr-x 2 legoater legoater     4096 Jun  1 20:01 ./ >>>>      drwxrwxr-x 6 legoater legoater     4096 Jun  1 19:40 ../ >>>>      -rwxr-xr-x 1 legoater legoater    36837 Jun  1 20:01 aspeed-ast2600-evb.dtb* >>>>      -rw-r--r-- 1 legoater legoater 67108864 Jun  1 20:01 flash.img >>>>      -rw-r--r-- 1 legoater legoater  6682796 Jun  1 20:01 image.itb >>>>      -rw-r--r-- 1 legoater legoater     1846 Jun  1 20:01 image.its >>>>      -rw-r--r-- 1 legoater legoater  3168768 Jun  1 20:01 rootfs.cpio >>>>      -rw-r--r-- 1 legoater legoater  1026660 Jun  1 20:01 rootfs.cpio.xz >>>>      -rw-r--r-- 1 legoater legoater  3788800 Jun  1 20:01 rootfs.tar >>>>      -rw-r--r-- 1 legoater legoater   653777 Jun  1 20:00 u-boot.bin >>>>      -rw-r--r-- 1 legoater legoater  5617280 Jun  1 20:01 zImage >>>> >>>> I will probably host them on GH and we could use them under avocado >>>> to extend the tests. >>>> >>>> >>>> They should boot real HW. I will submit the defconfigs to buildroot >>>> after more tests and cleanups. >>>> >>> >>> Nice! >>