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 AA046C4332F for ; Tue, 18 Oct 2022 08:03:08 +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=DQKvlEiQdrt1f4bSsKLWs6ICdilnnckNe2JPc59VJEI=; b=ufnjvUoBbwnk51 t54nm/Sgf4NO6CI5zmw7Ycl4TPyx1q5m9UvEWE6n8EYLzrSKa4p8ihDjWT9ZJz+risAbq3jebHLdO WJdRikisnyo3y7UHXzmdUYTen6utdcNfCRTE4Im9Im25IeyZwevjV1ZZWdaLxT8gaIKzJCxHzxDBt eCL+Tsk0djjMoYAabsn/ofnvNLS4/xPCVctl5bGiCE0IwHXMTUJ2hA74VOu6bA6MqEgYn9/7RyThw jmXUGvUffKvvZVlByYd7f/dMfuzY9xIUims4oiefo8jPmQn8YGQzNykeRgflfAzazSYiVV2rNBmU2 zty9NJYjtmBUsRfqzHCg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1okhY3-004EkN-Lv; Tue, 18 Oct 2022 08:02:03 +0000 Received: from relay9-d.mail.gandi.net ([217.70.183.199]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1okhXz-004EYf-Br for linux-arm-kernel@lists.infradead.org; Tue, 18 Oct 2022 08:02:01 +0000 Received: (Authenticated sender: kamel.bouhara@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id 6144EFF80F; Tue, 18 Oct 2022 08:01:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1666080116; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=welOLbT3ZpaoT8B3l5jN6gPn4wncpOQpCLvjvE83gnc=; b=XYt/uf7xr+1WOKUPPtZSIY4+cHfJxSClpg1gGSRHopquD4fDKKYFNtXcLyC0swQd2DuQKY E16E/2csLtxPp585CZuIbmIzLruTkwb4vCW8/G7UsvADezZwrG/xvP3o1Db8YRalzETEh/ /CcH+F377z41Wc6xk1qmZXaESGvlH4BVfwCasQoR/8c6QxDAU9n3jyIr2wJg5NOXAUbWKY oan7tt+sEA8Ueu9CRvTIcrPFUKFtybcKny6krcH8oij6qkJDs3rVLlyd5MhTx7+PnSasBN UqXmpoDnTwM7bhrAh9oTz5lRsP7pzCkcUdFJA82Ydj6rGJ4DHj9oxZn9SRVOuw== Date: Tue, 18 Oct 2022 10:01:49 +0200 From: Kamel Bouhara To: William Breathitt Gray Cc: linux-arm-kernel@lists.infradead.org, linux-iio@vger.kernel.org Subject: Re: Handling Signal1 in microchip-tcb-capture Message-ID: References: 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-20221018_010159_699666_6D5FEEFB X-CRM114-Status: GOOD ( 50.97 ) 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 Mon, Oct 17, 2022 at 05:57:59PM -0400, William Breathitt Gray wrote: > On Mon, Oct 17, 2022 at 07:06:26PM +0200, Kamel Bouhara wrote: > > On Mon, Oct 17, 2022 at 08:53:18AM -0400, William Breathitt Gray wrote: > > > On Mon, Oct 17, 2022 at 11:59:37AM +0200, Kamel Bouhara wrote: > > > > On Sat, Oct 15, 2022 at 09:52:27AM -0400, William Breathitt Gray wrote: > > > > > I was looking over the microchip-tcb-capture driver recently and noticed > > > > > that the code doesn't seem to account for Signal1. In particular, it > > > > > appears that mchp_tc_count_signal_read() and mchp_tc_count_action_read() > > > > > don't check the Signal id at all and just assume they are handling > > > > > Signal0. This creates a situation where the information returned for the > > > > > Signal1 sysfs attributes are just duplicated reports of Signal0. > > > > > > > > > > What exactly is the relationship of Signal0 ("Channel A") and Signal1 > > > > > ("Channel B"); is SignalB only relevant when the counter device is > > > > > configured for quadrature mode? > > > > > > > > Indeed both signals are required when in quadrature mode, where the > > > > signal0 is representing the speed and signal1 the revolution or number > > > > of rotation. > > > > > > > > We have described all availables modes in details in the following blog post: https://bootlin.com/blog/timer-counters-linux-microchip/ > > > > > > > > Regards, > > > > Kamel > > > > > > Thank you for the link, the block diagram helps illustrate how the > > > signals correlate to the TCB channels. > > > > > > Let me check if I understand correctly. In microchip-tcb-capture.c, > > > mchp_tc_count_signals[0] is TIOA0 while mchp_tc_count_signals[1] is > > > TIOB0? In quadrature mode, are TIOA and TIOB the two phases of a > > > quadrature encoder? You mentioned one signal is speed while the other is > > > the number of rotations; does this mean one signal serves as the > > > position incrementation from a rotary wheel while the other signal is > > > the index (z-phase) indicate for each full rotation? > > > > > > > IIRC this is indeed both signal edges (phase A and B) are accumulated on > > channel 0 and channel 1 stores the revolution or number of rotation of > > the qdec encoder. > > Ah, I think I understand now: Signal0 and Signal1 are TIOA and TIOB > respectively; channel 0 and channel 1 are data registers; channel 0 > holds the Count0 count value; channel 1 holds the revolution value (but > the microchip-tcb-capture driver does not expose it). Exact, I assumed that in qdec mode only the position value is relevant. > > It might be nice to expose the channel 1 revolution value as Count1 at > some point in the future. However, channel 1 seems unrelated to the > current issue we're dicussing so we can avoid it for now. OK do you suggest to read both count values in the same mchp_tc_count_read() ? > > > > In particular, I'm having trouble understanding > > > mchp_tc_count_signal_read(). I suspect it is unintentionally always > > > returning the signal status for TIOA:: > > > > > > regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr); > > > > > > if (priv->trig_inverted) > > > sigstatus = (sr & ATMEL_TC_MTIOB); > > > else > > > sigstatus = (sr & ATMEL_TC_MTIOA); > > > > > > *lvl = sigstatus ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW; > > > > > > Here we read the status register for channel 0, select between TIOA and > > > TIOB based on priv->trig_inverted, and then return the signal level. > > > > > > I don't see priv->trig_inverted referenced anywhere else so it appears > > > that priv->trig_inverted will always be 0, thus resulting in > > > mchp_tc_count_signal_read() always returning the TIOA status. I think > > > the intended behavior is to return the status of the selected signal:: > > > > IIRC the trig_inverted shall be used when signals are inverted which > > means we read position on TIOB and revolution on TIOA. > > Sure, that seems like a reasonable option to expose, but it does not > appear that trig_inverted is being set or otherwise configured in the > current code, unless I'm missing something. It might be best to remove > trig_inverted if the functionality is not supported yet by this driver. > OK. > > > > > > if (signal->id == 1) > > > sigstatus = (sr & ATMEL_TC_MTIOB); > > > else > > > sigstatus = (sr & ATMEL_TC_MTIOA); > > > > > > As for mchp_tc_count_action_read(), we have a similar problem: no > > > distinction is made for the Synapse requested. The channel mode register > > > for channel 0 is read and then masked against ATMEL_TC_ETRGEDG to > > > determine the action mode. It appears that this code is always assuming > > > the Synapse for TIOA is requested, but the Synapse for TIOB could be > > > passed. You can determine which corresponding Signal you have by > > > checking synapse->signal->id before deciding what action mode to return. > > > > > > > That is indeed a good point as both signals are eligible to trigger the > > TC for both modes (capture/qdec). > > > > > To clarify, in COUNTER_FUNCTION_INCREASE mode, does the Count value > > > increment based on the edge of TIOA and not TIOB? In > > > > Yes, currently the driver only support TIOA. > > > > > COUNTER_FUNCTION_QUADRATURE_X4 mode, does the Count value increment > > > based on both edges of TIOA and TIOB serving as quadrature encoding > > > phase A and B signals? > > > > Yes as explained above. > > Okay this should be simple to resolve then: return -EINVAL if Synapse is > for TIOB in mchp_tc_count_action_write(), and pass back > COUNTER_SYNAPSE_ACTION_NONE for TIOB during non-quadrature mode in > mchp_tc_counter_action_read(). > > I'll submit a patch fixing these changes and the signal_read() callback > mentioned previously. > That's clear, thanks. > By the way, I suspect there are race conditions present in > mcho_tc_count_function_write() that could be resolved by adding a lock > to the mchp_tc_data structure and acquiring it before accessing the > device state and registers. It's unrelated to the Signal1 issues so I > haven't looked any further into it, but it's something you might want to > investigate to make sure you don't get weird behavior from the driver. > You mean between function_write() and action_write() ? > Thanks, > > William Breathitt Gray -- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel