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=-1.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED autolearn=ham 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 AD74AC10F11 for ; Mon, 22 Apr 2019 18:01:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 691AC217D4 for ; Mon, 22 Apr 2019 18:01:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=synopsys.com header.i=@synopsys.com header.b="iZrb3C5m" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728571AbfDVSAz (ORCPT ); Mon, 22 Apr 2019 14:00:55 -0400 Received: from dc8-smtprelay2.synopsys.com ([198.182.47.102]:34508 "EHLO smtprelay-out1.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728532AbfDVSAw (ORCPT ); Mon, 22 Apr 2019 14:00:52 -0400 X-Greylist: delayed 376 seconds by postgrey-1.27 at vger.kernel.org; Mon, 22 Apr 2019 14:00:51 EDT Received: from mailhost.synopsys.com (dc8-mailhost2.synopsys.com [10.13.135.210]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by smtprelay-out1.synopsys.com (Postfix) with ESMTPS id 7F0A0C1D3C; Mon, 22 Apr 2019 17:54:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=synopsys.com; s=mail; t=1555955677; bh=MMYQYXnFutfIj92PqRo2Z2dDDn//b5y2FNPPepSuym0=; h=From:To:CC:Subject:Date:References:In-Reply-To:From; b=iZrb3C5mZ/hv/Uz4J91i9wwF+8xYod1iHSUYxAYvF2/YnX8o9Zi/h/pErTe7NgMBE EaZOZ2VS52s9rYEqv+K/OQ5SxFrv2mw6EPAnB4PfjTSP9tdsYI3nV3GhLHGgxc9PgO BLwzaGGLoO+HKJHRUZjjqaOrSBBSliZPWXjcFMNuDXUl4BC4O33MKNXzjwJceqN69b Dyb7KceosCfOOZh0ar25Dz7BSEQ+IBWEHdZ4Bk2+ZFh4mhQYuFpkhg6BCrBhyv88io ro/NAYJIbPuyzFM9+tJAk7YN2MJ1nDRdEsVffb/aXcFbUxqD3qiK8tnpZcl1UlKCNU TR9F6XRhflx2g== Received: from us01wehtc1.internal.synopsys.com (us01wehtc1-vip.internal.synopsys.com [10.12.239.236]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mailhost.synopsys.com (Postfix) with ESMTPS id 8ECB9A0065; Mon, 22 Apr 2019 17:54:32 +0000 (UTC) Received: from DE02WEHTCA.internal.synopsys.com (10.225.19.92) by us01wehtc1.internal.synopsys.com (10.12.239.235) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 22 Apr 2019 10:54:32 -0700 Received: from DE02WEMBXB.internal.synopsys.com ([fe80::95ce:118a:8321:a099]) by DE02WEHTCA.internal.synopsys.com ([::1]) with mapi id 14.03.0415.000; Mon, 22 Apr 2019 19:54:30 +0200 From: Vitor Soares To: Boris Brezillon , Vitor Soares CC: "linux-i3c@lists.infradead.org" , "joao.pinto@synopsys.com" , Boris Brezillon , "stable@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: RE: [PATCH 1/3] i3c: fix i2c and i3c scl rate by bus mode Thread-Topic: [PATCH 1/3] i3c: fix i2c and i3c scl rate by bus mode Thread-Index: AQHU87sLhgtQ1+d000+4YAeSeMHzMaY+J6iAgACom0D//+7fAIAJjifg///0oYCAADajAA== Date: Mon, 22 Apr 2019 17:54:29 +0000 Message-ID: <13D59CF9CEBAF94592A12E8AE55501350A61C415@DE02WEMBXB.internal.synopsys.com> References: <05fdeea79db83970e9ecb0d7045b4dd98f206f06.1555350118.git.vitor.soares@synopsys.com> <20190416075041.22f8e849@collabora.com> <13D59CF9CEBAF94592A12E8AE55501350A61596D@DE02WEMBXB.internal.synopsys.com> <20190416165250.0606e5a2@collabora.com> <13D59CF9CEBAF94592A12E8AE55501350A61B3B4@DE02WEMBXB.internal.synopsys.com> <20190422180715.40abe1b9@collabora.com> In-Reply-To: <20190422180715.40abe1b9@collabora.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-dg-ref: =?us-ascii?Q?PG1ldGE+PGF0IG5tPSJib2R5LnR4dCIgcD0iYzpcdXNlcnNcc29hcmVzXGFw?= =?us-ascii?Q?cGRhdGFccm9hbWluZ1wwOWQ4NDliNi0zMmQzLTRhNDAtODVlZS02Yjg0YmEy?= =?us-ascii?Q?OWUzNWJcbXNnc1xtc2ctYWFmZmJjY2QtNjUyNy0xMWU5LTgyM2EtZDQ4MWQ3?= =?us-ascii?Q?ODUxODBmXGFtZS10ZXN0XGFhZmZiY2NlLTY1MjctMTFlOS04MjNhLWQ0ODFk?= =?us-ascii?Q?Nzg1MTgwZmJvZHkudHh0IiBzej0iMzQ0NCIgdD0iMTMyMDA0MjkyNjgyMzI5?= =?us-ascii?Q?MzEwIiBoPSJuTWRsZ0FuMVNmYWEzcVhKdkp1VVd2VnNKTW89IiBpZD0iIiBi?= =?us-ascii?Q?bD0iMCIgYm89IjEiIGNpPSJjQUFBQUVSSFUxUlNSVUZOQ2dVQUFCUUpBQURl?= =?us-ascii?Q?TENodU5QblVBYzVhUTZjejNuOEJ6bHBEcHpQZWZ3RU9BQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBSEFBQUFDa0NBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?RUFBUUFCQUFBQVZ6ZGhHZ0FBQUFBQUFBQUFBQUFBQUo0QUFBQm1BR2tBYmdC?= =?us-ascii?Q?aEFHNEFZd0JsQUY4QWNBQnNBR0VBYmdCdUFHa0FiZ0JuQUY4QWR3QmhBSFFB?= =?us-ascii?Q?WlFCeUFHMEFZUUJ5QUdzQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFF?= =?us-ascii?Q?QUFBQUFBQUFBQWdBQUFBQUFuZ0FBQUdZQWJ3QjFBRzRBWkFCeUFIa0FYd0J3?= =?us-ascii?Q?QUdFQWNnQjBBRzRBWlFCeUFITUFYd0JuQUdZQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBUUFBQUFBQUFBQUNBQUFB?= =?us-ascii?Q?QUFDZUFBQUFaZ0J2QUhVQWJnQmtBSElBZVFCZkFIQUFZUUJ5QUhRQWJnQmxB?= =?us-ascii?Q?SElBY3dCZkFITUFZUUJ0QUhNQWRRQnVBR2NBWHdCakFHOEFiZ0JtQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQkFBQUFBQUFBQUFJQUFBQUFBSjRBQUFCbUFHOEFk?= =?us-ascii?Q?UUJ1QUdRQWNnQjVBRjhBY0FCaEFISUFkQUJ1QUdVQWNnQnpBRjhBY3dCaEFH?= =?us-ascii?Q?MEFjd0IxQUc0QVp3QmZBSElBWlFCekFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUVBQUFBQUFBQUFBZ0FBQUFBQW5nQUFBR1lBYndCMUFHNEFaQUJ5QUhrQVh3?= =?us-ascii?Q?QndBR0VBY2dCMEFHNEFaUUJ5QUhNQVh3QnpBRzBBYVFCakFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFRQUFBQUFBQUFBQ0FB?= =?us-ascii?Q?QUFBQUNlQUFBQVpnQnZBSFVBYmdCa0FISUFlUUJmQUhBQVlRQnlBSFFBYmdC?= =?us-ascii?Q?bEFISUFjd0JmQUhNQWRBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFCQUFBQUFBQUFBQUlBQUFBQUFKNEFBQUJtQUc4?= =?us-ascii?Q?QWRRQnVBR1FBY2dCNUFGOEFjQUJoQUhJQWRBQnVBR1VBY2dCekFGOEFkQUJ6?= =?us-ascii?Q?QUcwQVl3QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBRUFBQUFBQUFBQUFnQUFBQUFBbmdBQUFHWUFid0IxQUc0QVpBQnlBSGtB?= =?us-ascii?Q?WHdCd0FHRUFjZ0IwQUc0QVpRQnlBSE1BWHdCMUFHMEFZd0FBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQVFBQUFBQUFBQUFD?= =?us-ascii?Q?QUFBQUFBQ2VBQUFBWndCMEFITUFYd0J3QUhJQWJ3QmtBSFVBWXdCMEFGOEFk?= =?us-ascii?Q?QUJ5QUdFQWFRQnVBR2tBYmdCbkFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUJBQUFBQUFBQUFBSUFBQUFBQUo0QUFBQnpB?= =?us-ascii?Q?R0VBYkFCbEFITUFYd0JoQUdNQVl3QnZBSFVBYmdCMEFGOEFjQUJzQUdFQWJn?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFFQUFBQUFBQUFBQWdBQUFBQUFuZ0FBQUhNQVlRQnNBR1VBY3dCZkFI?= =?us-ascii?Q?RUFkUUJ2QUhRQVpRQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBUUFBQUFBQUFB?= =?us-ascii?Q?QUNBQUFBQUFDZUFBQUFjd0J1QUhBQWN3QmZBR3dBYVFCakFHVUFiZ0J6QUdV?= =?us-ascii?Q?QVh3QjBBR1VBY2dCdEFGOEFNUUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQkFBQUFBQUFBQUFJQUFBQUFBSjRBQUFC?= =?us-ascii?Q?ekFHNEFjQUJ6QUY4QWJBQnBBR01BWlFCdUFITUFaUUJmQUhRQVpRQnlBRzBB?= =?us-ascii?Q?WHdCekFIUUFkUUJrQUdVQWJnQjBBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUVBQUFBQUFBQUFBZ0FBQUFBQW5nQUFBSFlBWndCZkFHc0FaUUI1?= =?us-ascii?Q?QUhjQWJ3QnlBR1FBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB?= =?us-ascii?Q?QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFRQUFBQUFB?= =?us-ascii?Q?QUFBQ0FBQUFBQUE9Ii8+PC9tZXRhPg=3D=3D?= x-originating-ip: [10.107.25.87] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Boris Brezillon Date: Mon, Apr 22, 2019 at 17:07:15 > On Mon, 22 Apr 2019 15:54:33 +0000 > Vitor Soares wrote: >=20 >=20 > > > > > =20 > > > > > > { > > > > > > i3cbus->mode =3D mode; > > > > > > =20 > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > > > - i3cbus->scl_rate.i3c =3D I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > - > > > > > > - if (!i3cbus->scl_rate.i2c) { > > > > > > - if (i3cbus->mode =3D=3D I3C_BUS_MODE_MIXED_SLOW) > > > > > > - i3cbus->scl_rate.i2c =3D I3C_BUS_I2C_FM_SCL_RATE; > > > > > > - else > > > > > > - i3cbus->scl_rate.i2c =3D I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > > > > > + switch (i3cbus->mode) { > > > > > > + case I3C_BUS_MODE_PURE: > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > + i3cbus->scl_rate.i3c =3D I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > + break; > > > > > > + case I3C_BUS_MODE_MIXED_FAST: > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > + i3cbus->scl_rate.i3c =3D I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > + i3cbus->scl_rate.i2c =3D i2c_scl_rate; > > > > > > + break; > > > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > + i3cbus->scl_rate.i2c =3D i2c_scl_rate; > > > > > > + i3cbus->scl_rate.i3c =3D i3cbus->scl_rate.i2c; =20 > > > > >=20 > > > > > Maybe we should do > > > > >=20 > > > > > if (!i3cbus->scl_rate.i3c || > > > > > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > > > i3cbus->scl_rate.i3c =3D i3cbus->scl_rate.i2c; > > > > > =20 > > > > > Just in case the I3C rate forced by the user is lower than the ma= x I2C > > > > > rate. =20 > > > >=20 > > > > That was something that I considered but TBH it isn't a real use ca= se. =20 > > >=20 > > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we > > > should add a dev_warn() when the user-defined rates do not match > > > the mode/LVR constraints. It's easy to do a mistake when writing a dt= s. =20 > >=20 > > I think the WARN_ON() is too evasive on the screen and won't provide th= e=20 > > information we want. > > The dev_warn() should work perfectly here. > >=20 > > if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > dev_warn(&i3cbus->cur_master->dev->dev, > > "%s: i3c-scl-hz lower then i2c-scl-hz\n", __func__); >=20 > Using dev_warn() sounds good, though I don't think you need the > __func__ here. Also, please print the i2c/i3c rates in the message, and > align the second line on the open parens. >=20 > > if (i3cbus->scl_rate.i2c !=3D I3C_BUS_I2C_FM_SCL_RATE || > > i3cbus->scl_rate.i2c !=3D I3C_BUS_I2C_FM_PLUS_SCL_RATE) > > dev_warn(&i3cbus->cur_master->dev->dev, > > "%s: i2c-scl-hz not defined according MIPI I3C spec\n",=20 > > __func__); >=20 > Is that really a problem? Having an i2c rate that is less than FM speed > sounds like a valid case to me. I'm addressing the spec constrains. In the practice it can be SM or even HS, it depends on the interface. >=20 > >=20 > > Maybe it make more sense to do this check on of_populate_i3c_bus(), wha= t=20 > > do you think? > >=20 >=20 > No, we really want to have this check here, because we might support > other HW description formats at some point (board-files, ACPI, ...). Yes, you are right. I forgot that point.=20