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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 57B69C433F5 for ; Fri, 24 Sep 2021 22:07:49 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 968A161212 for ; Fri, 24 Sep 2021 22:07:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 968A161212 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 286C9832E8; Sat, 25 Sep 2021 00:07:46 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="eEEnD0D3"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 93614832E9; Sat, 25 Sep 2021 00:07:44 +0200 (CEST) Received: from mail-qv1-xf34.google.com (mail-qv1-xf34.google.com [IPv6:2607:f8b0:4864:20::f34]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 439E5832C4 for ; Sat, 25 Sep 2021 00:07:40 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qv1-xf34.google.com with SMTP id a14so7204104qvb.6 for ; Fri, 24 Sep 2021 15:07:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=/BxhhWyMdWB9G8NXQRX1rFq32XVnV8pocdasKPm3GFc=; b=eEEnD0D3evE7QOpFCPZa8m8agaaVUPtMxt9mm3xGJ7eNzcz1lGbZyQ/iL/3tOJ5Jyo U3K76Ugyo5lm3mkKGYWDtoUGzrJTSgIdBmDhZVZ13Tj/eWT33gDjG15sY5SgieAE8LnL dLUg74RQhcPxfosPKul/a8IVWrAXKd0+fJK3o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=/BxhhWyMdWB9G8NXQRX1rFq32XVnV8pocdasKPm3GFc=; b=HujmWJ77GDC1WIRBkVkE2btTbC5HuK99h+PqOLzzxYwHoCjzKtY1nZXbLNOwX2ADw4 G8JFvSRrdKYuet3F9LL1abzgUE3zz668vp6Tz8SDcHTDq9Qzpplje9JcYmNicl/WCVyS x4Kxwabxdev2flx2hHlvMeB0X8rBnBTvaR7ICxxvgAcAEo0qa1Ndy33tfnjXhHqcMBDg wV3VT939GR8ex/tJ7ksNk7CavU6M3eypZ5PVYlwr9AoNhHJFDFcFDUmzuhIApMQRKUXb L9w0WYfZIqpSimlT4/aoknhFNSvt+3/VkKOrNDmLzT7gF6KLq0sK8F4E7dOMUZtONfgs Mo7Q== X-Gm-Message-State: AOAM531+V1UQLoZMaSes1P5ogHAYL+2oDGv8y+9tIWjbwKMi+BhKIA1y SfjDRO33Fp7q3gaTgjTv//XfKg== X-Google-Smtp-Source: ABdhPJwKEMLBFL+FqCSzbKDPdxCq2Bas/L/5L+89atiQdxNWZMXDg1f8sB85n9oeoCvoJUUEccnW+g== X-Received: by 2002:ad4:45c3:: with SMTP id v3mr12968474qvt.41.1632521258871; Fri, 24 Sep 2021 15:07:38 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b01-cbda-9907-3395-6a0a-dd86.res6.spectrum.com. [2603:6081:7b01:cbda:9907:3395:6a0a:dd86]) by smtp.gmail.com with ESMTPSA id r5sm5955869qta.26.2021.09.24.15.07.37 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Fri, 24 Sep 2021 15:07:38 -0700 (PDT) Date: Fri, 24 Sep 2021 18:07:36 -0400 From: Tom Rini To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: "Alex G." , u-boot@lists.denx.de Subject: Re: [PATCH 5/5] serial: Rework CONFIG_SYS_BAUDRATE_TABLE Message-ID: <20210924220736.GO31748@bill-the-cat> References: <20210913212455.29165-1-trini@konsulko.com> <20210913212455.29165-5-trini@konsulko.com> <596e1bf3-0c51-c528-77e0-2be943c18e28@gmail.com> <20210913221138.GT12964@bill-the-cat> <20210924190815.zykw6jsaimkhgfyv@pali> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="yLCsp91D2sDKcV7m" Content-Disposition: inline In-Reply-To: <20210924190815.zykw6jsaimkhgfyv@pali> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean --yLCsp91D2sDKcV7m Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 24, 2021 at 09:08:15PM +0200, Pali Roh=E1r wrote: > Hello! >=20 > On Monday 13 September 2021 18:11:38 Tom Rini wrote: > > On Mon, Sep 13, 2021 at 05:03:13PM -0500, Alex G. wrote: > > >=20 > > >=20 > > > On 9/13/21 4:24 PM, Tom Rini wrote: > > > > In order to move CONFIG_SYS_BAUDRATE_TABLE to Kconfig, we need to r= ework > > > > the logic a bit. Rename the users of CONFIG_SYS_BAUDRATE_TABLE to > > > > SYS_BAUDRATE_TABLE. Introduce a series of CONFIG_BAUDRATE_TABLE_... > > > > that include some number of baud rates. These match all existing u= sers. > > > > The help for each entry specifies what the exact table is, for a gi= ven > > > > option. Define what SYS_BAUDRATE_TABLE will be in include/serial.h= now. > > > >=20 > > > > Signed-off-by: Tom Rini > > > > --- > > >=20 > > > > diff --git a/include/serial.h b/include/serial.h > > > > index 6d1e62c6770c..150644c4c3d4 100644 > > > > --- a/include/serial.h > > > > +++ b/include/serial.h > > > > @@ -3,6 +3,42 @@ > > > > #include > > > > +#if defined(CONFIG_BAUDRATE_TABLE_300_TO_38400_115200) > > > > +#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 192= 00, \ > > > > + 38400, 115200 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_115200) > > > > +#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 192= 00, \ > > > > + 38400, 57600, 115200 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_230400) > > > > +#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 192= 00, \ > > > > + 38400, 57600, 115200, 230400 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_6000000) > > > > +#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 1800, 2400, 4800, 960= 0, \ > > > > + 19200, 38400, 57600, 115200, 230400, \ > > > > + 460800, 500000, 576000, 921600, 1000000, \ > > > > + 1152000, 1500000, 2000000, 2500000, \ > > > > + 3000000, 3500000, 4000000, 4500000, \ > > > > + 5000000, 5500000, 6000000 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_4800_TO_115200) > > > > +#define SYS_BAUDRATE_TABLE { 4800, 9600, 19200, 38400, 57600, 1152= 00 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_115200) > > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_230400) > > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 23= 0400 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_460800) > > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 23= 0400, 460800 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_921600) > > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 23= 0400, \ > > > > + 460800, 921600 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_230400_500000_1500000) > > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 23= 0400, \ > > > > + 500000, 1500000 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_38400_115200_ONLY) > > > > +#define SYS_BAUDRATE_TABLE { 38400, 115200 } > > > > +#elif defined(CONFIG_BAUDRATE_TABLE_115200_ONLY) > > > > +#define SYS_BAUDRATE_TABLE { 115200 } > > > > +#endif > > > > + > > > > struct serial_device { > > > > /* enough bytes to match alignment of following func pointer */ > > > > char name[16]; > > >=20 > > >=20 > > > This opens the gates to #ifdefing the heck out of serial.h. What happ= ens to > > > my board that goes from 300 to 2000000? > > > * We need a new Kconfig and new ifdef > > > What happens to my other board that goes from 300 to 2500000? > > > * We need a new Kconfig and new ifdef > > > The pattern doesn't look promising. > >=20 > > This reminds me I was tempted to do a cover letter, but didn't. What > > happens is I tell you no. Most boards are using the standard table of > > common rates from 9600 to 115200. A nice follow-up would be to change > > every board with a special case that's not above 115200 to just use the > > normal table. Everyone else? There's the maximal table. That's it. > > That's even come in fairly recently, for mvebu platforms. >=20 > I think that above #ifdef hell is the worst what could be done. It does > not solve existing problems and just introduce new way which opens doors > for problems for new boards... I don't think this will open new problems, since I do intend to fire off the "easy" reduction option of switching to either standard (current default) or maximal tables, rather than N tables. > What should be done is to completely kill this CONFIG_SYS_BAUDRATE_TABLE > option and not to convert it to another set of options. >=20 > Why to kill it? >=20 > 1) Probably none of CONFIG_SYS_BAUDRATE_TABLE in include board files is > correctly defined. Some of them contains only just few baudrates > copied from other header files without checking that it is correct > or contains some "radom" definition where is also 9600 and 115200 (as > probably only these values are tested). >=20 > 2) Available baudrate values are not specific to board, but rather to > UART chip used on the board. So only UART driver knowns what are > supported values. >=20 > 3) Some boards may have integrated USB<-->UART chip which is "glued" > directly to the board UART chip. So baudrates possible to set is > limited to what together USB<-->UART chip and SoC UART supports. >=20 >=20 > I think the proper way is to do: >=20 > 1) Extend serial UART DM API with a callback which takes baudrate > argument and returns the nearest baudrate value which hardware can > set. Then serial UART drivers should implement it. This > implementation is simple, because most UART drivers already do it. > They take baudrate and calculates the best UART clock divisor and set > it. And from this calculated divisor can be reconstructed the real > baudrate value. >=20 > 2) Introduce some board specific function which can limit / filter > particular baudrate (used in scenario when board has directly USB > port with integrated USB<-->UART). >=20 > 2) Currently CONFIG_SYS_BAUDRATE_TABLE is used for checking if baudrate > value is supported prior setting it. So replace this table by a new > callback from step 1) and 2) and allow baudrate if is e.g. in 2-3% > tolerance. >=20 > 3) Add some platform / SoC option to allow specifying upper or lower > limit for baudrate. Not as Kconfig option, this is mean as platform > limitation exposed by e.g. used clock source. Some platforms have > limitation that for specific clock must have some minimal clock UART > divisor. >=20 > 4) Add some optional Kconfig option to limit upper and lower UART > baudrate. The problem I have is none of this is new, and predates Kconfig being introduced. Now, if you're saying you'll fire off a patch series to do the above, in the next few months even, OK, yes, I can set aside this particular series for now. But setting aside some of the ugly defines until we can convert them to something cleaner is why we're at this stagnant point in the conversion. --=20 Tom --yLCsp91D2sDKcV7m Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmFOTCQACgkQFHw5/5Y0 tyx51Qv/fJt801wuDlHzvrqUEx7E61H2P8FsJZdKMNbzkOIZPbJihG9k4yy8wXjZ bWfTvkmtKr5TZr1B9O4PNwZChV87sioHsLPqKFX2G9vykZYL/s58YVgZkfOOkS6L WWTIjMHqXIp24gKrdHsnwe1cebG1s6ciAsuJFRACtf/VTUT1TxeBqEoqF2hhtC29 8v3Whz36g8sRMmErho/eflzC5vjeodwUhMRoej7fVoAmjPOhOAvw95uCVwSpodBW QuxxgIdmjiLGiYOw1ObmwlRgr4ebli0TeyACZ6VwN722sB+JpzYU2GrkKeHFt3Co Iy4TVM/5kc5Y9XpLCd+iYr3IHGOZk9nBYVSK69i+d64yoqivNfQimIXUV2xPGMou prCdAG521QVsX3CKgBFeHRi6hjCVu0jzQSLc74tegvVzjU/VcWbLF2QOTHdG3XJe pbM4u7CRvxnqGV/q8PphoTn3Mj3GU+e7w+Tdu+h49CuWKsloqQKyKBaKubydnvr0 zVf4XyZF =vZNO -----END PGP SIGNATURE----- --yLCsp91D2sDKcV7m--