From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andri Yngvason Subject: Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling. Date: Mon, 1 Dec 2014 11:51:58 +0000 Message-ID: <20141201115158.1538.64214@shannon> References: <3103cd54-1421-4594-855f-7bfca8f4ef49@GRBSR0089.marel.net> <547B7CDD.2080202@grandegger.com> <20141201110923.29602.38961@shannon> <6a5747c2d61b278876b6c3154c6a3130@grandegger.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: Received: from mail-db3on0083.outbound.protection.outlook.com ([157.55.234.83]:62784 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753010AbaLALwS convert rfc822-to-8bit (ORCPT ); Mon, 1 Dec 2014 06:52:18 -0500 In-Reply-To: <6a5747c2d61b278876b6c3154c6a3130@grandegger.com> Sender: linux-can-owner@vger.kernel.org List-ID: To: Wolfgang Grandegger Cc: linux-can@vger.kernel.org, mkl@pengutronix.de Quoting Wolfgang Grandegger (2014-12-01 11:37:03) > On Mon, 1 Dec 2014 11:09:23 +0000, Andri Yngvason > wrote: > > Quoting Wolfgang Grandegger (2014-11-30 20:23:57) > >> On 11/28/2014 01:12 PM, Andri Yngvason wrote: > >> > Replacing error state change handling with the new mechanism. > >> > > >> > Signed-off-by: Andri Yngvason > >> > --- > >> > Changes made since last proposal: > >> > can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6 > >> > can: flexcan: adapt to newer can_change_state > >> > > >> > drivers/net/can/flexcan.c | 103 > >> > +++++++++------------------------------------- > >> > 1 file changed, 19 insertions(+), 84 deletions(-) > >> > > >> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > >> > index 60f86bd..9f91735 100644 > >> > --- a/drivers/net/can/flexcan.c > >> > +++ b/drivers/net/can/flexcan.c > >> > @@ -266,7 +266,7 @@ static struct flexcan_devtype_data > >> > fsl_p1010_devtype_data = { > >> > }; > >> > static struct flexcan_devtype_data fsl_imx28_devtype_data; > >> > static struct flexcan_devtype_data fsl_imx6q_devtype_data = { > >> > - .features = FLEXCAN_HAS_V10_FEATURES, > >> > + .features = FLEXCAN_HAS_V10_FEATURES | > >> > FLEXCAN_HAS_BROKEN_ERR_STATE, > >> > >> Oops, this change is not related to the subject! Anyway, did it cure > >> your problems with state handling. Is it required for all i.MX6 cores > >> then? > >> > > Yeah, I accidentally squashed this; intended to make this a separate > > patch. This > > helps. It has the same effect as enabling berr-reporting. However, this > > only > > cures one of the problems. This does not help with the case where no one > > else is > > sending on the bus. > > > > This problem is not limited to i.MX6. This is a problem for ALL FlexCAN > > cores. > > This is a design flaw in FlexCAN cores. Maybe they did this part on a > > monday > > after a weekend of binge-drinking. In any case, they really messed up > with > > this > > one. > > #define FLEXCAN_HAS_BROKEN_ERR_STATE BIT(2) /* [TR]WRN_INT not > connected */ > > They actually forgot to connect the [TR]WRN_INT line. Don't know what they > did the days before ;). > That's not a problem on the i.MX6, but that doesn't matter. The problem is that there are no interrupts for the other state transitions. Whether WRN_INT is connected or not isn't even relevant. WRN_INT is only triggered when transitioning from error-active to error-warning. Everything else is missing by design. > > > I suggest that we remove the FLEXCAN_HAS_BROKEN_ERR_STATE flag and turn > on > > error > > interrupts permanently because they ALL have broken error state. > > Hm, how did you come to this conclusion? You just tested on an i.MX6, > right? > Anyway, the bus error reporting can cause very high CPU load and it should > be off when it's not needed. > I don't need to test this on other cores because, it can be logically derived from the manual that interrupts for error state transitions were not a part of the design. This may sound quite incredible, but it is my understanding that this is indeed the case. I really do hope that I'm wrong, but I doubt it. -- Andri.