GPMI iMX6ull timeout on DMA

[Christian Eggers]

On Friday, 8 October 2021, 18:07:52 CEST, Miquel Raynal wrote:
> Hi Christian,
> 
> ceggers at arri.de wrote on Fri, 8 Oct 2021 15:49:21 +0200:
> 
> > On Friday, 8 October 2021, 15:36:31 CEST, Miquel Raynal wrote:
> > > 
> > > miquel.raynal at bootlin.com wrote on Fri, 8 Oct 2021 15:29:05 +0200:
> > >   
> > > > 
> > > > If this clock (as I understand) does not prevent us to access the
> > > > registers but only feeds the external NAND bus part, then there is no
> > > > need to enable it in the probe, just acquiring it will be enough.  
> > 
> > clocks[0] is "gpmi_io" which sounds more like i/o than registers. So lets
> > try to remove the initial call to clk_set_rate().

>From the GPMI description (i.MX6 ULL):
> [GPMI] Registers are clocked on the HCLK domain. The I/O and pin timing are 
> clocked on a dedicated GPMICLK domain. GPMICLK can be set to maximize I/O
> performance.

Additionally, figure 17-1 in IMX6ULLRM.pdf shows that both (BCH and GPMI)
registers are connected to APBH.

I checked this with the debugger: For accessing the BCH and GPMI
registers, only CCGR0::CG2 [APBHDMA_HCLK_ENABLE] is required. This bit is
enabled in mxs_dma_alloc_chan_resources():

-000|mxs_dma_alloc_chan_resources(chan = 0xC2090154)
-001|__refcount_inc_not_zero(inline)
-001|refcount_inc_not_zero(inline)
-001|kref_get_unless_zero(inline)
-001|dma_chan_get(:chan = 0xC2090154)
-002|find_candidate(device = 0xC2090050, :mask = 0xC209BD38, :fn = 0xC0254679, :fn_param = 0xC209BD3C)
-003|__dma_request_channel(:mask = 0xC209BD38, :fn = 0xC0254679, :fn_param = 0xC209BD3C, np = 0xC7EEB0B0)
-004|mxs_dma_xlate(:dma_spec = 0xC209BD60, :ofdma = 0xC2264840)
-005|of_dma_request_slave_channel(np = 0xC7EEB2B4, name = 0xC04F923A -> "rx-tx")
-006|dma_request_chan(dev = 0xC2182410, :name = 0xC04F923A -> "rx-tx")
-007|acquire_dma_channels(inline)
-007|acquire_resources(:this = 0xC200F840)
-008|gpmi_nand_probe(:pdev = 0xC2182400)

The root clock is BCH_CLK_ROOT. It doesn't depend on ENFC_PRED or ENFC_PODF
(the dividers which are actually set in clk_set_rate(r->clock[0], 22000000)).

--> clk_set_rate(r->clock[0], 22000000) is not required for accessing the
registers. I removed the call entirely and everything works fine.

> > 
> > > > Then, the first call for an IO operation with ->must_apply_timings
> > > > should:
> > > > 
> > > > 	if (imx6)
> > > > 		disable_clk();
> > > > 
> > > > 	clk_set_rate();
> > > > 
> > > > 	if (imx6)
> > > > 		enable_clk();  
> > 
> > Do you think that the need for avoiding clock glitches is i.MX6 specific?
> > The errata I mentioned is specific for the bootloader software, but (I think)
> > the requirement for switching off the clocks gates prior changing the dividers
> > may apply also for other series.
> 
> I honestly don't know, perhaps Han have more details about it. If you
> think it's a wider issue, then we can just do the disable/enable step
> without any further checks.
I also don't know. I can not find the required sequence in the reference manual
(only in the errata sheet), so I cannot compare with other series. For best
performance we can start with checking for GPMI_IS_MX6Q(x) and extend it later
if this issue comes up on other devices.

I sent a question for this on NXP community:
https://community.nxp.com/t5/i-MX-Processors/ERR007117-Which-i-MX-devices-require-gating-the-clocks-when/m-p/1353018

> > > Actually we should ensure clks are enabled in the !imx6 case anyway,
> > > but this is needed only once so either we keep enabling the clock in
> > > the probe or we check here if the clk has already been enabled or not.  
> > The clocks are already enabled (and kept on) in probe. The initial call to
> > clk_set_rate() is just above this (but the clocks are not disabled at this
> > stage as all gates have been enabled by the boot loader).
> 
> The IO clock should be enabled and set to a particular rate the first
> time the die is selected to perform a NAND operation, or when we switch
> from one device to the other (this does not apply to the GPMI driver
> for now). So we can drop the enable/set_rate call in the probe if the
> assumption that this clock only feeds the external bus is right.

I think that this assumption is right.

regards
Christian