Re: [PATCH v3 5/7] mmc: bcmstb: Add support for bcm2712 SD controller

From: "Ivan T. Ivanov" <iivanov@suse.de>
To: Stefan Wahren <wahrenst@gmx.net>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>,
	 Matthias Brugger <mbrugger@suse.com>,
	Peter Robinson <pbrobinson@gmail.com>,
	 Dmitry Malkin <dmitry@bedrocksystems.com>,
	Thomas Fitzsimmons <fitzsim@fitzsim.org>,
	 Peng Fan <peng.fan@nxp.com>,
	Jaehoon Chung <jh80.chung@samsung.com>,
	 Anatolij Gustschin <agust@denx.de>,
	u-boot@lists.denx.de
Subject: Re: [PATCH v3 5/7] mmc: bcmstb: Add support for bcm2712 SD controller
Date: Fri, 22 Dec 2023 10:22:51 +0200	[thread overview]
Message-ID: <ae2fhlqdltlrdkbaai7s3b7rqz44xxmabb6klqsnf2rez7vvjb@hkhxm42xpn3x> (raw)
In-Reply-To: <db91a259-d504-4e8b-8178-dad3e5709b93@gmx.net>

On 12-21 16:39, Stefan Wahren wrote:
> To: Florian Fainelli <florian.fainelli@broadcom.com>,  "Ivan T. Ivanov"
> > 
> > On 12/18/2023 10:03 PM, Ivan T. Ivanov wrote:
> > > Borrow SD quirks from vendor Linux driver.
> > > 
> > > "BCM2712 unfortunately carries with it a perennial bug with the SD
> > > controller register interface present on previous chips
> > > (2711/2709/2708).
> > > Accesses must be dword-sized and a read-modify-write cycle to the 32-bit
> > > registers containing the COMMAND, TRANSFER_MODE, BLOCK_SIZE and
> > > BLOCK_COUNT registers tramples the upper/lower 16 bits of data written.
> > > BCM2712 does not seem to need the extreme delay between each write as on
> > > previous chips, just the serialisation of writes to these registers in a
> > > single 32-bit operation."
> > > 
> > > Signed-off-by: Ivan T. Ivanov <iivanov@suse.de>
> > 
> > This is diverging from the Linux sdhci-brcmstb.c driver where no such
> > quirk needs to be carried out, rather the logic for such quirks has
> > been present in sdhci-iproc.c...
> 
> it seems this patch based the downstream kernel changes [1].

Yep.

> I would
> suggest to use an existing driver which already handle this bug
> (iproc_sdhci or bcm2835_sdhci).
> 
> Does the Rpi 5 still works, if the compatible "brcm,bcm2712-sdhci" is
> added to mmc/iproc_sdhci.c?

No, it is not working :-)

Even after I added shadow variables usage in iproc_readw, which do not
have it.

Even after I used hard coded values for "clock-freq-min-max", which are
missing in RPi5 device tree.

Even after I added 2712 specific "cfg" space initialization procedure
from the brcnstb driver.

On the other hand they are some tuning procedures in iproc driver
which I am not sure that are relevant for 2712 controller.

And the diff stat is getting bigger that equivalent for brcmstb driver.

To me it is  a bit easier to add 2712 support to brcmstb driver,
because it will be easier to follow any vendor Linux driver changes.

It looks like hardware engineers are just making the same mistake
when integrating 2712 SDHCI controller as for IPROC controller.

Regards,
Ivan

> 
> [1] -
> https://github.com/raspberrypi/linux/commit/b627647c4500d39cb026924b608841fdf4d4d7e9