Re: [PATCH 4/4] ARM: dts: Fix-up EMMC2 controller's frequency

[Nicolas Saenz Julienne]

On Thu, 2021-03-25 at 20:11 +0100, Stefan Wahren wrote:
> Am 24.03.21 um 16:34 schrieb Nicolas Saenz Julienne:
> > Hi Stefan,
> > 
> > On Wed, 2021-03-24 at 16:16 +0100, Stefan Wahren wrote:
> > > Hi Nicolas,
> > > 
> > > Am 22.03.21 um 19:58 schrieb Nicolas Saenz Julienne:
> > > > From: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
> > > > 
> > > > Force emmc2's frequency to 150MHz as the default 100MHz (set by FW)
> > > > seems to interfere with the VPU clock when setup at frequencies bigger
> > > > than 500MHz (a pretty common case). This ends up causing unwarranted
> > > > SDHCI CMD hangs  when no SD card is present.
> > > > 
> > > > Signed-off-by: Nicolas Saenz Julienne <nsaenz@xxxxxxxxxx>
> > > > ---
> > > >  arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> > > > index 3b4ab947492a..9aa8408d9960 100644
> > > > --- a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> > > > +++ b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> > > > @@ -257,6 +257,12 @@ &emmc2 {
> > > >  	vqmmc-supply = <&sd_io_1v8_reg>;
> > > >  	vmmc-supply = <&sd_vcc_reg>;
> > > >  	broken-cd;
> > > > +	/*
> > > > +	 * Force the frequency to 150MHz as the default 100MHz seems to
> > > > +	 * interfere with the VPU clock when setup at frequencies bigger than
> > > > +	 * 500MHz, causing unwarranted CMD hangs.
> > > > +	 */
> > > > +	clock-frequency = <150000000>;
> > > i don't want to bike-shed here, but is there any chance to solve this in
> > > clk-bcm2835 in a less hacky way?
> > What do you have in mind?
> Sorry, nothing specific.
> > 
> > All I can think of is adding some kind of heuristic to the clock's prepare()
> > callback. That said, I don't feel it would be a better solution than this.
> 
> Based on my limited knowledge and an old SD card specification, all
> possibly connected devices could have different frequencies. So my
> concern here is, that in case we limit the frequency to a specific value
> we could break things just to suppress a warning.

SDHCI should be able to handle up to 233MHz IIRC, and there are divisors
available, it depends on the implementation but the worst kind provide /2^n.
Not perfect, but good enough for things to work.

Now, I've been having a deeper look into how clocks are handled, and found two
new clues:

 - First of all RPi4's sdhci-iproc needs SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
   that is, the controller isn't properly identifying the clock frequency fed
   into it, and defaults to saying it's configured at 100MHz. I'm not an SDHCI
   expert, so it's possible changing frequencies also needs a special operation
   to recalculate this variable. But this was making all internal calculations
   wrong when paired with this series.

 - With this flag set SDHCI's core now properly calculates divisor values based
   on whatever clock frequency I set in DT. And guess what, the issue reappears
   even when running on 150MHz. It turns out, as I had some debugging enabled,
   the issue only happens when the controller is configured at 100KHz (that
   only happens while running the card detect thread).

So, I can now do this (note that for card detection try to communicate with the
card starting at 400KHz down to 100KHz in 100KHz steps):

----->8-----

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 536c382e2486..e5a5de63f347 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -173,6 +173,11 @@ static unsigned int sdhci_iproc_get_max_clock(struct sdhci_host *host)
                return pltfm_host->clock;
 }
 
+static unsigned int sdhci_iproc_bcm2711_get_min_clock(struct sdhci_host *host)
+{
+       return 200000;
+}
+
 static const struct sdhci_ops sdhci_iproc_ops = {
        .set_clock = sdhci_set_clock,
        .get_max_clock = sdhci_iproc_get_max_clock,
@@ -271,13 +276,15 @@ static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
        .set_clock = sdhci_set_clock,
        .set_power = sdhci_set_power_and_bus_voltage,
        .get_max_clock = sdhci_iproc_get_max_clock,
+       .get_min_clock = sdhci_iproc_bcm2711_get_min_clock,
        .set_bus_width = sdhci_set_bus_width,
        .reset = sdhci_reset,
        .set_uhs_signaling = sdhci_set_uhs_signaling,
 };

----->8-----

 Which is rather nicer than what this series introduces. But I can't still
 explain why configuring the controller at 100KHz is causing the hangs (while
 having the core clock setup at 500MHz), and I'm not sure if excluding 100KHz
 from the polling frequency list is going to break support for older SD cards.

 Regards,
 Nicolas

Attachment: signature.asc
Description: This is a digitally signed message part