[RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent

[Adriana Reus]

On Ma, 2017-07-04 at 13:01 +0200, Lucas Stach wrote:
> Am Dienstag, den 04.07.2017, 10:57 +0000 schrieb Adriana Reus:
> > 
> > On Lu, 2017-07-03 at 16:42 +0200, Lucas Stach wrote:
> > > 
> > > Am Montag, den 03.07.2017, 17:28 +0300 schrieb Adriana Reus:
> > > > 
> > > > 
> > > > Hi all,
> > > > 
> > > > I am currently trying to fix an issue occurring when trying to
> > > > set
> > > > the
> > > > CLK_SET_RATE_GATE flag for the uart root clks. This causes the
> > > > imx
> > > > serial
> > > > driver to fail when setting up devicetree specified frequency
> > > > or
> > > > parent because
> > > > the uart clk is already in use by the ccm driver that starts up
> > > > all
> > > > uart clocks
> > > > for earlycon.
> > > > 
> > > > Some context:
> > > > 
> > > > There are three main types of clock slices on imx7d ccm: "Core
> > > > clock slice",
> > > > "Bus clock slice (AXI/AHB)", and "Peripheral clock slice". Vast
> > > > majority of
> > > > clock roots fall in the peripheral slice category:
> > > > 
> > > >     8:1 mux -> gate -> div -> div -> gate
> > > > 
> > > > The root clocks that derive from peripheral slices are expected
> > > > to
> > > > be
> > > > gated when changing frequency [0].
> > > > 
> > > > The first patch of this series sets this up. (It is only a
> > > > reference point for
> > > > this conversation, not to be merged as it WIP and needs more
> > > > testing).
> > > > 
> > > > When setting this up, the imx serial driver
> > > > (platform_driver_register -> of_clk_set_defaults) will fail
> > > > to set the rate or parent specified in the devicetree:
> > > > 
> > > >     imx-uart 30a70000.serial: clk_set_rate() failed
> > > >     imx-uart: probe of 30a70000.serial failed with error -16
> > > > 
> > > > The reason for this is that the uart clks are already prepared
> > > > and
> > > > enabled in
> > > > imx7d_clocks_init by the imx_register_uart_clocks function if
> > > > earlycon is
> > > > enabled. Functionality added in patches:
> > > > 'commit 55adc61c568af99419be1dc0412f8eae019c71f2
> > > > ("clk: imx: add common logic to detect early UART usage")'
> > > > 'commit 1b9af68f325cb91ac9fc691f52d69dfb0826afd7
> > > > ("clk: imx7d: retain early UART clocks during kernel init")'
> > > > 
> > > > One option is to remove the imx_register_uart_clocks from the
> > > > ccm
> > > > driver and
> > > > leave the uart clock set up and enable entirely to the serial
> > > > driver and DT.
> > > > The second RFC patch does this for imx7d.
> > > 
> > > This is not an option. The CCM holds a reference to those clocks,
> > > specifically to prevent them from being disabled before a real
> > > serial
> > > driver is bound. If this isn't done you might end up in a
> > > situation
> > > where PM or driver probe deferal disables the UART clock/the
> > > parent
> > > of
> > > this clock, even if it was previously enabled by the bootloader.
> > > With
> > > earlycon this might lead to a full system hang at this point,
> > > when
> > > earlycon tries to access a clock gated UART.
> > > 
> > > Regards,
> > > Lucas
> > > 
> > 
> > Hi Lucas, Thank you for your reply.
> > 
> > Looking at clk_core_unprepare and clk_core_disable they seem to
> > return
> > without hardware disabling the clock if enable_count or
> > prepare_count
> > are 0. So, disabling a bootloader enabled only clock does not seem
> > to
> > be a problem. Let me know if I misunderstand.
> 
> That's correct, however image a situation where some other driver
> probes
> that uses the same parent clock as the earlycon UART, before the real
> serial driver is up. This driver requests and enables the parent
> clock,
> then goes into runtime suspended state, disabling the clock. As the
> clock has been enabled by this driver we now have the transition from
> enabled to disabled, disabling the bootloader enabled clock.
> 
> The same can happen with driver probe deferal.
> 
> Regards,
> Lucas
> 

Ok, thanks for further explaining.
Sounds like this would be a general theoretical case, not
bound to a specific platform, and possibly could apply to other
booloader-on clocks, I guess.

I am not sure as to what a proper solution should be.
Would adding logic in Clock Framework to handle
bootloader-on clocks differently be an option ?


Thanks,
Adriana