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 5D255C433F5 for ; Fri, 24 Sep 2021 19:08:31 +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 813EF61250 for ; Fri, 24 Sep 2021 19:08:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 813EF61250 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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 DAD0A8347F; Fri, 24 Sep 2021 21:08:27 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="dRRf79NI"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 8664683480; Fri, 24 Sep 2021 21:08:25 +0200 (CEST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 6AC21832E0 for ; Fri, 24 Sep 2021 21:08:21 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pali@kernel.org Received: by mail.kernel.org (Postfix) with ESMTPSA id 0E27561250; Fri, 24 Sep 2021 19:08:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1632510498; bh=k7Ywv6EUoGnGgVs42LshHu+BHLL0gkBTzfK+63d1lPw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dRRf79NIk9ykUFCcQk2S7gzF39j4ksrA+wC47aA8zflYRoi9CvqYH/dwJO41wISaV m37igQzP0gFaXmbEmL5KIIr1Y8w/hIeGYAaMvjqbHvMgPD2NusMpkI/ce19R0szIxX DFFNd1QQh4tHXvJG7//2Oouy7NIRg0nATxjbkMfXZ/plpQSP3istwQKUnLpQVYb0ad c6qkLKJVz3r5SKxToi/nDM2NnunEraDL4K2LEg6u5jf9CpDvOZ3PbDRFudED3K5ehx Mvsl4h9QExhH9Rvy4xQNmJMDQrk5sVx77yGPUr052/7kq6vMn+wY+s8Vcovh7jLPDJ /tkrx0x5cbBew== Received: by pali.im (Postfix) id 8CA1D7A6; Fri, 24 Sep 2021 21:08:15 +0200 (CEST) Date: Fri, 24 Sep 2021 21:08:15 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Tom Rini Cc: "Alex G." , u-boot@lists.denx.de Subject: Re: [PATCH 5/5] serial: Rework CONFIG_SYS_BAUDRATE_TABLE Message-ID: <20210924190815.zykw6jsaimkhgfyv@pali> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210913221138.GT12964@bill-the-cat> User-Agent: NeoMutt/20180716 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 Hello! On Monday 13 September 2021 18:11:38 Tom Rini wrote: > On Mon, Sep 13, 2021 at 05:03:13PM -0500, Alex G. wrote: > > > > > > On 9/13/21 4:24 PM, Tom Rini wrote: > > > In order to move CONFIG_SYS_BAUDRATE_TABLE to Kconfig, we need to rework > > > 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 users. > > > The help for each entry specifies what the exact table is, for a given > > > option. Define what SYS_BAUDRATE_TABLE will be in include/serial.h now. > > > > > > Signed-off-by: Tom Rini > > > --- > > > > > 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, 19200, \ > > > + 38400, 115200 } > > > +#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_115200) > > > +#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 19200, \ > > > + 38400, 57600, 115200 } > > > +#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_230400) > > > +#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 2400, 4800, 9600, 19200, \ > > > + 38400, 57600, 115200, 230400 } > > > +#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_6000000) > > > +#define SYS_BAUDRATE_TABLE { 300, 600, 1200, 1800, 2400, 4800, 9600, \ > > > + 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, 115200 } > > > +#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, 230400 } > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_460800) > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400, 460800 } > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_921600) > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400, \ > > > + 460800, 921600 } > > > +#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_230400_500000_1500000) > > > +#define SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600, 115200, 230400, \ > > > + 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]; > > > > > > This opens the gates to #ifdefing the heck out of serial.h. What happens 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. > > 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. 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... What should be done is to completely kill this CONFIG_SYS_BAUDRATE_TABLE option and not to convert it to another set of options. Why to kill it? 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). 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. 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. I think the proper way is to do: 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. 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). 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. 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. 4) Add some optional Kconfig option to limit upper and lower UART baudrate. > > I actually think this change can make the situation worse. We trade having > > an antiquated and inconvenient SYS_BAUDRATE_TABLE for one Kconfig per each > > possible baudrate combination. How does this make sense? > > I'm going, as much as possible, for migrating the current situation. > There's many places things could be cleaner, but "we'll clean this up > and then migrate ..." is why we're so very very far behind where I hoped > to be. > > > I've seen situations were SPL boots with 2Mbaud and executes succesfully, > > u-boot starts up with 2Mbaud just fine. few lines later, u-boot, > > downswitches to 115200 because CONFIG_SYS_BAUDRATE_TABLE says so. > > Well, the table and CONFIG_BAUDRATE. > > > Suggestion I: Can we have a MIN/MAX value for baudrates, and have the code > > work from there ? > > > > Suggestion II: Define the Kconfig SYS_BAUDRATE_TABLE table to a C array, > > like 'default "{ 300, 420, 690}" ' and forego the #ifdefs in serial.h > > > > Suggestion III: Get rid of the logic that says "baudrate must be one of > > these predefined values" and let the serial driver return -ENOBUENO or > > -EINVAL if the hardware really can't do that baudrate. Most UARTs nowadays > > can do a wide range of values, and the baudrate table doesn't model that > > very well. Combine this with a CONFIG_MAX_BAUDRATE so that boards with > > shitty RS232 converters can set a safe upper limit -- and make sure > > CONFIG_BAUDRATE also enforces this. > > > > There's a lot of unrealized potential here. > > I'd certainly like to see something done as a follow-up that makes it > easier to support platforms that can do something faster. > > -- > Tom