GPMI iMX6ull timeout on DMA

[Miquel Raynal]

miquel.raynal at bootlin.com wrote on Fri, 8 Oct 2021 15:29:05 +0200:

> Hi Christian,
> 
> ceggers at arri.de wrote on Fri, 8 Oct 2021 15:11:59 +0200:
> 
> > On Friday, 8 October 2021, 11:55:56 CEST, Christian Eggers wrote:
> >   
> > > I got another solution from PHYTEC ([2], not lengthy tested yet), which
> > > disables all GPMI/BCH clocks on CCGR4    
> >   
> > > -static void gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
> > > +static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
> > > 
> > >  {
> > >  
> > >  	struct gpmi_nfc_hardware_timing *hw = &this->hw;
> > >  	struct resources *r = &this->resources;
> > >  	void __iomem *gpmi_regs = r->gpmi_regs;
> > >  	unsigned int dll_wait_time_us;
> > > 
> > > +	int ret;
> > > 
> > > +	ret = __gpmi_enable_clk(this, false);
> > > +	if (ret)
> > > +		return ret;
> > > 
> > >  	clk_set_rate(r->clock[0], hw->clk_rate);
> > > 
> > > +	ret = __gpmi_enable_clk(this, true);
> > > +	if (ret)
> > > +		return ret;
> > > 
> > >  	writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0);
> > >  	writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1);    
> > 
> > 
> > I think that this is also required for the first call to clk_set_rate() in
> > gpmi_get_clks(). From the kernel's point of view, the clocks are not enabled
> > yet, so no "guard" is required. Putting the same guard here will actually give
> > a warning at runtime that I am trying to disable a clock which is not enabled.
> > But on my system, all clock gates are enabled by the bootloader (barebox)
> > and therefore the glitch could also happen here.
> > 
> > If setting the clock to a "default" rate is really required (on my system, the
> > clock is switched from 99 MHz to 22 MHz, and back to 99 MHz on a later call...),
> > I suggest moving this call to gpmi_nand_probe() (below __gpmi_enable_clock())
> > and guard it. The result would look like:
> > 
> > ...
> > 	ret = acquire_resources(this);
> > 	if (ret)
> > 		goto exit_acquire_resources;
> > 
> > 	ret = __gpmi_enable_clk(this, true);
> > 	if (ret)
> > 		goto exit_acquire_resources;
> > 
> > 	if (GPMI_IS_MX6(this)) {
> > 		/*
> > 		 * Set the default value for the gpmi clock.
> > 		 *
> > 		 * If you want to use the ONFI nand which is in the
> > 		 * Synchronous Mode, you should change the clock as you need.
> > 		 */
> > 		__gpmi_enable_clk(this, false);
> > 		clk_set_rate(this->resources.clock[0], 22000000);
> > 		__gpmi_enable_clk(this, true);
> > 	}
> > 
> > 	pm_runtime_set_autosuspend_delay(&pdev->dev, 500);  
> 
> 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.
> Then, the first call for an IO operation with ->must_apply_timings
> should:
> 
> 	if (imx6)
> 		disable_clk();
> 
> 	clk_set_rate();
> 
> 	if (imx6)
> 		enable_clk();

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.

Cheers,
Miquèl