Re: Marvell NFC timings on CN9130

[Miquel Raynal]

Hi Chris,

Chris.Packham@xxxxxxxxxxxxxxxxxxx wrote on Sun, 21 May 2023 22:53:35
+0000:

> On 17/05/23 14:22, Chris Packham wrote:
> >
> > On 17/05/23 05:25, Miquel Raynal wrote:  
> >> Hi Chris!
> >>
> >> Chris.Packham@xxxxxxxxxxxxxxxxxxx wrote on Tue, 16 May 2023 04:46:38
> >> +0000:
> >>  
> >>> Hi Miquel, Thomas,
> >>>
> >>> A hardware colleague reported a concern to me about a new design we 
> >>> have
> >>> using the Marvell CN9130 SoC (which I think was called Armada-8K before
> >>> they rebranded).
> >>>
> >>> Basically their concern is that the tWC timing they observe is faster
> >>> (~18ns) than the documented minimum in the hardware datasheet for the
> >>> CN9130 (25ns). Aside from not meeting the datasheet spec we've not
> >>> observed any other issue (yet).  
> >> I would have expected the controller to support almost any kind of
> >> timings, including SDR EDO mode 5. tWC is 25ns with mode 4, but 20 on
> >> mode 5 (ONFI). So I believe you're running a system with a chip that is
> >> not compatible with the fastest mode. If that is the case, it may
> >> explain why you don't see errors with this chip: it may support
> >> slightly faster timings than it advertises.
> >>
> >> Anyway, if your findings are true, it means the current implementation
> >> is slightly out of spec and the timing calculation might require to be
> >> tweaked a little bit to reduce tWC.
> >>  
> >>> I notice in the marvell_nand.c driver that marvell_nfc_init() sets the
> >>> NAND Clock Frequency Select bit (0xF2440700:0) to 1 which runs 
> >>> according
> >>> to the datasheet the NAND flash at 400MHz . But the calculations in
> >>> marvell_nfc_setup_interface() use the value from
> >>> clk_get_rate(nfc->core_clk) which is still 250MHz so I'm wondering if
> >>> maybe the fact that the NAND flash is being run faster is having an
> >>> impact on timings that are calculated around the core_clk frequency.  
> >> What if you reset this bit? Do you observe different timings? I hope
> >> you do, otherwise this is a dead-end.  
> > Yes if we clear the bit the timings go from ~18ns to about 30ns.  
> >> The timings are derived from this clock but I remember seeing different
> >> rates than the ones I expected with no obvious explanation (see the "*
> >> 2" in the calculation of period_ns and the comment right below). So
> >> maybe this is due to the 400MHz vs. 250MHz issue you are reporting, or
> >> there is an undocumented pre-scaler in-between (this is my original
> >> guess).  
> >
> > I wondered if the * 2 was because of this or because of the comment 
> > that the ECC_CLK is 2*NF_CLK. That probably also means that a number 
> > of SoCs are running with an extra *2 that don't need to be (e.g. 
> > Armada-385).  
> Interestingly cp110-system-controller.c is aware of the 400MHz option 
> but that's only effective if it's been set prior to the kernel booting. 
> I'm not really familiar with clk drivers but I assume it must be 
> possible to make it look up the frequency dynamically instead of using a 
> single fixed value.

We are accessing the bit directly in the NAND controller driver,
perhaps there should be some ops exposed and used in order to pick one
or the other of the two clock sources.

Thanks,
Miquèl