GPMI iMX6ull timeout on DMA
Miquel Raynal
miquel.raynal at bootlin.com
Mon Oct 18 00:43:08 PDT 2021
Hi Michael,
> > > > > > > > commit f8e6ad14388067f91b26d044185d95623fbc9535
> > > > > > > > Author: Michael Trimarchi <michael at amarulasolutions.com>
> > > > > > > > Date: Fri Jan 29 08:46:53 2021 +0100
> > > > > > > >
> > > > > > > > mtd: nand: Calculate the clock before enable it
> > > > > > > >
> > > > > > > > Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
> > > > > > > > Change-Id: I79b0da39de0a9b32ea0b002fa200d7f44d4f8ce7
> > > > > > > >
> > > > > > > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
> > > > > > > > b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
> > > > > > > > index 322a008290e5..0bca52b3bc8f 100644
> > > > > > > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
> > > > > > > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-lib.c
> > > > > > > > @@ -377,6 +377,7 @@ static void gpmi_nfc_compute_timings(struct
> > > > > > > > gpmi_nand_data *this,
> > > > > > > > const struct nand_sdr_timings *sdr)
> > > > > > > > {
> > > > > > > > struct gpmi_nfc_hardware_timing *hw = &this->hw;
> > > > > > > > + struct resources *r = &this->resources;
> > > > > > > > unsigned int dll_threshold_ps = this->devdata->max_chain_delay;
> > > > > > > > unsigned int period_ps, reference_period_ps;
> > > > > > > > unsigned int data_setup_cycles, data_hold_cycles, addr_setup_cycles;
> > > > > > > > @@ -440,6 +441,8 @@ static void gpmi_nfc_compute_timings(struct
> > > > > > > > gpmi_nand_data *this,
> > > > > > > > hw->ctrl1n |= BF_GPMI_CTRL1_RDN_DELAY(sample_delay_factor) |
> > > > > > > > BM_GPMI_CTRL1_DLL_ENABLE |
> > > > > > > > (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : 0);
> > > > > > > > +
> > > > > > > > + clk_set_rate(r->clock[0], hw->clk_rate);
> > > > > > > > }
> > > > > > > >
> > > > > > > > void gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
> > > > > > > > @@ -449,8 +452,6 @@ void gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
> > > > > > > > void __iomem *gpmi_regs = r->gpmi_regs;
> > > > > > > > unsigned int dll_wait_time_us;
> > > > > > > >
> > > > > > > > - clk_set_rate(r->clock[0], hw->clk_rate);
> > > > > > > > -
> > > > > > > > writel(hw->timing0, gpmi_regs + HW_GPMI_TIMING0);
> > > > > > > > writel(hw->timing1, gpmi_regs + HW_GPMI_TIMING1);
> > > > > > > >
> > > > > > > > Right now I have this change applied and seems fine. That is the only
> > > > > > > > difference I get. Clock is apply a bit earlier that when is enabled
> > > > > > > > it.
> > > > > > >
> > > > > > > This is very interesting. So this would mean the issue you are
> > > > > > > experiencing comes from the clock driver which kind of returns too
> > > > > > > early from clk_set_rate()? Could you report this to the clk ML/NXP clk
> > > > > > > maintainers and keep us in copy? If it is as global as it sounds, we
> > > > > > > might not be the only ones affected.
> > > > > > >
> > > > > >
> > > > > > The imx28 is broken too, so it's a general problem. I need to trace it down
> > > > > > I have a reverting for lts but it\s not the way to go
> > > > > >
> > > > >
> > > > > For imx28 you ask to set the rate to 22Mhz but you don't care about the clock
> > > > > that you get back. You get back 12Mhz because the base clock is 24 Mhz and seems
> > > > > that it can not get the point. You need to check if the clock
> > > > > requested is in range or ask
> > > > > for set_rate_clk_min to avoid to have somenthing lower. Then for
> > > > > imx6ull because is sporadic
> > > > > I think that is more connected to the clk_set_rate and when you change
> > > > > the register. Can not be a
> > > > > setting time?
> > > >
> > > > So, if I understand correctly, we face two different problems:
> > > > - imx6*: seems like a clock issue regarding the clock settlement
> > > > - imx28: actual NAND driver issue (does not check the validity of the
> > > > new frequency). This should be handled properly in
> > > > ->setup_interface().
> > > >
> > >
> > > Somenthing like this? Not compile/tested
> > >
> > > diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> > > index 4d08e4ab5c1b..cc8146ab1b78 100644
> > > --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> > > +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> > > @@ -644,7 +644,7 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
> > > * RDN_DELAY = ----------------------- {3}
> > > * RP
> > > */
> > > -static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
> > > +static int gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
> > > const struct nand_sdr_timings *sdr)
> > > {
> > > struct gpmi_nfc_hardware_timing *hw = &this->hw;
> > > @@ -656,6 +656,7 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
> > > int sample_delay_ps, sample_delay_factor;
> > > u16 busy_timeout_cycles;
> > > u8 wrn_dly_sel;
> > > + long clk_rate;
> > >
> > > if (sdr->tRC_min >= 30000) {
> > > /* ONFI non-EDO modes [0-3] */
> > > @@ -671,6 +672,10 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
> > > wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> > > }
> > >
> > > + clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
> > > + if (clk_rate < hw->clk_rate || clk_rate <= 0)
> > > + return -ENOTSUPP;
> >
> > I believe clk_rate < hw->clk_rate will always match cases where
> > clk_rate <= 0 ?
> >
> > The check looks very strict though. Will it even pass on i.MX6? Perhaps
> > we could verify something like a 10% error which might grab all the
> > erroneous situations?
>
> According to what I read the clk is the min that we can accept. So any clock
> from EDO4 to EDO5 should be ok. My concern is that calculation. I need to read
> it properly. I don't think that put 10% or any will help us, until we
> now that is possible or not.
10% was for the example, what I mean is that it is very common to
request a clock to run at 100MHz and to read it at eg. 93MHz. Your
check won't pass in this case and we cannot get the necessary test
coverage in order to ensure that we won't break working boards.
The thing is, if the calculation are made using hw->clk_rate we will
always get the register values wrong anyway and in this case it's true
that only experience will tell us if such a clock works or not.
However, if the calculations are made with clk_rate instead, it is
likely that we will get more accurate timings which very likely will
work. So perhaps the right solution would be to use the real clock rate
instead than refusing clock rates which do not match our strict
expectations?
> >
> > if (abs(clk_rate - hw->clk_rate) > (hw->clk_rate / 10))
> > return -ENOTSUPP;
> >
> > > +
> > > /* SDR core timings are given in picoseconds */
> > > period_ps = div_u64((u64)NSEC_PER_SEC * 1000, hw->clk_rate);
> > >
> > > @@ -746,6 +751,7 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
> > > {
> > > struct gpmi_nand_data *this = nand_get_controller_data(chip);
> > > const struct nand_sdr_timings *sdr;
> > > + int ret = 0;
> > >
> > > /* Retrieve required NAND timings */
> > > sdr = nand_get_sdr_timings(conf);
> > > @@ -761,11 +767,11 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
> > > return 0;
> > >
> > > /* Do the actual derivation of the controller timings */
> > > - gpmi_nfc_compute_timings(this, sdr);
> > > -
> > > - this->hw.must_apply_timings = true;
> > > + ret = gpmi_nfc_compute_timings(this, sdr);
> > > + if (!ret)
> > > + this->hw.must_apply_timings = true;
> > >
> > > - return 0;
> > > + return ret;
> > > }
> > >
> > > /* Clears a BCH interrupt. */
> > > > Thanks,
> > > > Miquèl
> >
> > Otherwise looks good, thanks!
> >
> > Miquèl
>
Thanks,
Miquèl
More information about the linux-mtd
mailing list