Message-Id: <20220224230224.A0C49C340F1@smtp.kernel.org>
Date:   Thu, 24 Feb 2022 15:02:22 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Marek Behún <kabel@...nel.org>
Cc:     Gregory Clement <gregory.clement@...tlin.com>,
        Pali Rohár <pali@...nel.org>,
        linux-clk@...r.kernel.org, linux-serial@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Marek Behún <kabel@...nel.org>
Subject: Re: [PATCH v9 3/6] serial: mvebu-uart: implement UART clock driver for configuring UART base clock

Quoting Marek Behún (2022-02-19 07:28:15)
> From: Pali Rohár <pali@...nel.org>
> 
> Implement a new device driver for controlling UART clocks on Marvell
> Armada 3700 SoC. This device driver is loaded for devices which match
> the compatible string "marvell,armada-3700-uart-clock".
> 
> There are more pitfalls related to UART clocks:
> - both UARTs use same parent clock source (which can be xtal or one of
>   the TBG clocks),
> - if a TBG clock is used as the parent clock, there are two additional
>   divisors that can both be configured to divide the rate by 1, 2, ... 6,
>   but these divisors are again shared between the two UART controllers
>   on the SOC,
> - the configuration of the parent clock source and divisors is done in
>   the address space of the first UART controller, UART1. Clocks can be
>   gated separately for UART1 and UART2, but this setting also lives in
>   the address space of UART1,
> - Marvell's Functional Specification for Armada 3720 document has the
>   clock gating bits swapped, so the one described to gate UART1 clock
>   actually gates UART2 and vice versa,
> - each UART has it's own "special divisor", and this uses the parent
>   clock described above. These divisors are configure in each UART's
>   address space separately.
> 
> Thus the driver for UART2 controller needs to have access to UART1
> address space, since UART1 address space contains some bits exclusive
> for UART2 and also some bits which are shared between UART1 and UART2.
> 
> Also, during boot, when early console is active on one of the UARTs,
> and we want to switch parent clock from xtal (default) to TBG (to be
> more flexible with baudrates), the driver changing UART clocks also
> needs to be able to change the "special divisor", so that the baudrate
> of earlycon is not changed when swtiching to normal console. Thus the
> clock driver also needs to be able to access UART2 register space,
> for UART2's "special divisor".
> 
> For these reasons, this new UART clock driver does not use
> ioremap_resource(), but only ioremap() to prevent resource conflicts
> between UART clock driver and UART driver.
> 
> We need to share only two 32-bit registers between the UART driver and
> the UART clock driver:
> - UART Clock Control
> - UART 2 Baud Rate Divisor
> Access to these two registers are protected by one spinlock to prevent
> any conflicts. Access is required only during probing, when changing
> baudrate or during suspend/resume.
> 
> Hardware can be configured to use one of following clocks as UART parent
> clock: TBG-A-P, TBG-B-P, TBG-A-S, TBG-B-S, xtal. Not every clock is
> usable for higher buadrates. Any subset can be specified in the
> device-tree and the driver will choose the best one which also still
> supports the mandatory baudrate of 9600 Bd. For smooth boot log output
> it is needed to specify clock used by early console, otherwise garbage
> would be printed on UART during probe of UART clock driver and
> transitioning from early console to normal console.
> 
> We are implementing this to be able to configure TBG clock as UART
> parent clock, which is required to be able to achieve higher baudrates
> than 230400 Bd. We achieve this by referencing this new UART clock
> device node in UART's device node. UART clock device driver
> automatically chooses the best clock source for UART driver.
> 
> Until now, UART's device-tree node needed to reference one of the static
> clocks (xtal or one of the TBGs) as parent clock in the `clocks`
> phandle - the parent clock which was configured before booting the
> kernel. If bootloader changed UART's parent clock, it needed to change
> the `clocks` phandle in DTB correspondingly before booting.
> 
> From now on both the old mechanism (xtal or TBG referenced as parent
> clock in `clocks` phandle) and the new one (UART clock referenced in the
> `clocks` phandle) are supported, to provide full backward compatibility
> with existing DTS files, full backward compatibility with existing boot
> loaders, and to provide new features (runtime clock configuration to
> allow higher baudrates than 230400 Bd). New features are available only
> with new DTS files.
> 
> There was also a discussion about how the UART node and the
> clock-controller node could be wrapped together in a new binding [1, 2].
> As explained there, this is not possible if we want to keep backwards
> compatibility with existing bootloaders, and thus we are doing this by
> putting the UART clock-controller node inside the UART1 node.
> 
> [1] https://lore.kernel.org/linux-serial/20220120000651.in7s6nazif5qjkme@pali/
> [2] https://lore.kernel.org/linux-serial/20220125204006.A6D09C340E0@smtp.kernel.org/
> 
> Signed-off-by: Pali Rohár <pali@...nel.org>
> Reviewed-by: Marek Behún <kabel@...nel.org>
> Signed-off-by: Marek Behún <kabel@...nel.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@...nel.org>

Powered by blists - more mailing lists