Message-ID: <20241103020929.2298633f@minigeek.lan>
Date: Sun, 3 Nov 2024 02:09:29 +0000
From: Andre Przywara <andre.przywara@....com>
To: Cody Eksal <masterr3c0rd@...chal.quest>
Cc: Yangtao Li <tiny.windzz@...il.com>, Michael Turquette
 <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Chen-Yu Tsai
 <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland
 <samuel@...lland.org>, Conor Dooley <conor+dt@...nel.org>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Kishon Vijay Abraham I
 <kishon@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Maxime
 Ripard <mripard@...nel.org>, Nishanth Menon <nm@...com>, "Rafael J.
 Wysocki" <rafael@...nel.org>, Rob Herring <robh@...nel.org>, Vinod Koul
 <vkoul@...nel.org>, Viresh Kumar <vireshk@...nel.org>, Viresh Kumar
 <viresh.kumar@...aro.org>, Parthiban <parthiban@...umiz.com>,
 linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 08/13] clk: sunxi-ng: a100: enable MMC clock
 reparenting

On Sat, 02 Nov 2024 18:44:41 -0300
Cody Eksal <masterr3c0rd@...chal.quest> wrote:

Hi Cody,

thanks for staying on this issue!

> On 2024/10/31 9:08 am, Andre Przywara wrote:
> > Well, while this change indeed prevented that error message you mentioned,
> > but the SD card still doesn't work for me: it probes and I can mount a
> > filesystem on it, but then it hangs, for instance when running an "ls" on
> > it. It could be my setup (lacking DT or device issue or missing kernel
> > config), though, and the eMMC works for me this way, but it would be good
> > to have that sorted.   
> I'm investigating this now; it appears mmc2/eMMC is more consistent when
> CLK_NO_REPARENT is set

What do you mean with "more consistent", exactly?
I still don't get why NO_REPARENT would help here in the first place:
we have three clocks as potential parents: OSC24MHz, PLL_PERIPH0,
PLL_PERIPH1. The first one is too slow for typical MMC rates, and
PERIPH1 is typically disabled (it's the same rates as PERIPH0, so
there is little need for it). So PERIPH0 is to clock to go, and I don't
see what NO_REPARENT would change here.

So those are my observations:
With NO_REPARENT (current mainline):
- SD card fails to probe:
  sunxi-mmc 4020000.mmc: fatal err update clk timeout
- SD card is still parented to PERIPH0-2x (probably because U-Boot set
  that up), but uses a divider of 256 for a clock rate of 4687500 Hz.
  This probably leads to the failures.
- eMMC works, but is parented to the 24MHz OSC, probably because U-Boot
  did not touch it. The clock rate is 12MHz, the read speed is 10MB/s.
With removing NO_REPARENT, so with this patch:
- SD cards probes, I can mount a VFAT fs on it, and sometimes "ls"
  that, but it hangs soon afterwards, for instance when trying to
  benchmark it.
- SD clock is set up correctly: parent is PLL_PERIPH0-2x, rate is 50
  MHz, correct for High Speed@...t and its 25MB/s bus speed.
- eMMC works fine, clock parent is PLL-PERIPH0-2x, rate is 100 MHz,
  correct for HS-200 (100 MHz * 8 bit * 2(DDR)). The read speed is
  72MB/s, which sounds alright, and might be a limitation of the flash
  chip.

So NO_REPARENT is always worse for me.

> > Also it would be good to know why CLK_SET_RATE_NO_REPARENT was put there
> > in the first place: I don't see it in any other MMC clocks in sunxi-ng, so
> > it wasn't just copied&pasted.  
> Seeing that mmc2 acts better with the flag, perhaps it was copy + pasted
> from that config. Or perhaps the issues we're running into comes from
> elsewhere in the chain. At the moment, that's only speculation, though;
> I'm waiting on a device that has an SD card slot so I can perform more
> testing myself and debug these issues.
> 
> > So was there a problem that this flag was supposed to fix? Is that
> > something that only applied to older kernels (back when the MMC patches
> > were first posted), and which has now been fixed/changed elsewhere?  
> Yangtao Li/Frank Lee assumably no longer works at Allwinner, as the email
> he used to submit this originally no longer exists, but I believe the same
> Yangtao is now a maintainer of the Allwinner cpufreq subsystem, and is
> CC'd on these patches. I'm sending this reply to him as well; perhaps he
> may have some additional insight.
> 
> > I feel a bit uneasy of just removing this just because it works(TM),
> > especially if it doesn't really (SD card for me, for instance).  
> I agree; I was quickly preparing V2 to hopefully get this in before the
> 6.13 window for the sunxi tree closed, and added this in last minute after
> verifying it worked on my current device, which lacks an SD card slot.
> 
> This patch can be skipped for now, as it's apparent MMC0/1 require a little
> more love before we can merge it in. I'll submit new patches in the future
> once this is figured out.

This patch would be a fix anyway (with a Fixes: tag), so we can push it
still into 6.13, after -rc1, and it would be backported. So it's not as
critical, timing-wise.

Cheers,
Andre

> 
> Thanks!
> - Cody
> 
> > Cheers,
> > Andre
> >   
> >> Signed-off-by: Cody Eksal <masterr3c0rd@...chal.quest>
> >> ---
> >>  drivers/clk/sunxi-ng/ccu-sun50i-a100.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)  
> 

Powered by blists - more mailing lists