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 X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_GIT autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 28072C63697 for ; Sat, 14 Nov 2020 01:18:54 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C8ED922261 for ; Sat, 14 Nov 2020 01:18:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="B7ULLaSS"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.b="nyIdzzSJ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C8ED922261 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:MIME-Version:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:References:In-Reply-To:Message-Id:Date:Subject:To: From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=JFXDX4f7HlZks75xCpWfxVHqsT1DVXMs77ZCuaAsCmo=; b=B7ULLaSSeU0z84MasuxBJQmIZR 6TO3kUlwmjloCdjNz0+YRbDAc+6mfVTST3Xz2zm5qaby+NBJpcTyO9aOL6ElfPs0fc5bvzJ68omwB UbTlwvIL4awcBYhGQ50SjahH8FvXFzGmw9+tBlSGAoWTpUR+m3vlwlweL639R9iQ7wPiNI+EYV0Vd 3YMSkPQRUwi7nIpwwbt9zj/TgV/DzoPasHt1HyWfWzigDhkc+68vNVuhlvg7oSiWAN/b63prSLqLX ymLE6MV7aXFvFozErLRmTOOR31cxfdXjRlV2XM5wMn8vThFYeMZgs1x7JNAVUSV6lxmweAJ5xCYfW /QLnFXSg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdkCG-000138-B4; Sat, 14 Nov 2020 01:17:44 +0000 Received: from linux.microsoft.com ([13.77.154.182]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdkCD-00012W-Og for linux-arm-kernel@lists.infradead.org; Sat, 14 Nov 2020 01:17:42 +0000 Received: by linux.microsoft.com (Postfix, from userid 1046) id 6B85E20B71B7; Fri, 13 Nov 2020 17:17:39 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 6B85E20B71B7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1605316659; bh=yUF3L6bAkQSx57oUz6pPfzc487YJPjSYBv9gnynSNRc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=nyIdzzSJoZHNLbkf+LBkN/YUO6dxnk6BDkVtbkFafsayjDjlVxKzhLONudEhqYDv6 D9npTEhjg/JOevaU0Q36G00on/hick+/lKyn+OmBYn2kIr5iL7JudDm1y/04WskFz2 eX/flqr4sjvmhvP3OTEHutoRn+qE7jFScyau/vkU= From: Dhananjay Phadke To: ray.jui@broadcom.com Subject: Re: [PATCH v3 5/6] i2c: iproc: handle master read request Date: Fri, 13 Nov 2020 17:17:39 -0800 Message-Id: <1605316659-3422-1-git-send-email-dphadke@linux.microsoft.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <38a23afc-57da-a01f-286c-15f8b3d61705@broadcom.com> References: <38a23afc-57da-a01f-286c-15f8b3d61705@broadcom.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201113_201741_948658_63677487 X-CRM114-Status: GOOD ( 14.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: lori.hikichi@broadcom.com, f.fainelli@gmail.com, sbranden@broadcom.com, rayagonda.kokatanur@broadcom.com, rjui@broadcom.com, brendanhiggins@google.com, linux-kernel@vger.kernel.org, wsa@kernel.org, bcm-kernel-feedback-list@broadcom.com, dphadke@linux.microsoft.com, andriy.shevchenko@linux.intel.com, linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org MIME-Version: 1.0 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, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote: >>>> Yes it's true that for master write-read events both >>>> IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT are coming together. >>>> So before the slave starts transmitting data to the master, it should >>>> first read all data from rx-fifo i.e. complete master write and then >>>> process master read. >>>> >>>> To minimise interrupt overhead, we are batching 64bytes. >>>> To keep isr running for less time, we are using a tasklet. >>>> Again to keep the tasklet not running for more than 20u, we have set >>>> max of 10 bytes data read from rx-fifo per tasklet run. >>>> >>>> If we start processing everything in isr and using rx threshold >>>> interrupt, then isr will run for a longer time and this may hog the >>>> system. >>>> For example, to process 10 bytes it takes 20us, to process 30 bytes it >>>> takes 60us and so on. >>>> So is it okay to run isr for so long ? >>>> >>>> Keeping all this in mind we thought a tasklet would be a good option >>>> and kept max of 10 bytes read per tasklet. >>>> >>>> Please let me know if you still feel we should not use a tasklet and >>>> don't batch 64 bytes. >>> >>> Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq) >>> as i2c rate is quite low. >>> > >kernel thread was proposed in the internal review. I don't see much >benefit of using tasklet. If a thread is blocked from running for more >than several tenth of ms, that's really a system-level issue than an >issue with this driver. > >IMO, it's an overkill to use tasklet here but we can probably leave it >as it is since it does not have a adverse effect and the code ran in >tasklet is short. > >How much time is expected to read 64 bytes from an RX FIFO? Even with >APB bus each register read is expected to be in the tenth or hundreds of >nanosecond range. Reading the entire FIFO of 64 bytes should take less >than 10 us. The interrupt context switch overhead is probably longer >than that. It's much more effective to read all of them in a single >batch than breaking them into multiple batches. OK, there's a general discussions towards removing tasklets, if this fix works with threaded isr, strongly recommend that route. This comment in the code suggested that register reads take long time to drain 64 bytes. >+/* >+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet >+ * running for less time, max slave read per tasklet is set to 10 >bytes. @Rayagonda, please take care of hand-off mentioned below, once the tasklet is scheduled, isr should just return and clear status at the end of tasklet. >> >> Few other comments - >> >>> + /* schedule tasklet to read data later */ >>> + tasklet_schedule(&iproc_i2c->slave_rx_tasklet); >>> + >>> + /* clear only IS_S_RX_EVENT_SHIFT interrupt */ >>> + iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, >>> + BIT(IS_S_RX_EVENT_SHIFT)); >>> + } >> >> Why clearing one rx interrupt bit here after scheduling tasklet? Should all that >> be done by tasklet? Also should just return after scheduling tasklet? Regards, Dhananjay _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel