Re: [PATCH v3 5/7] mmc: bcmstb: Add support for bcm2712 SD controller
On 12-21 16:39, Stefan Wahren wrote: > To: Florian Fainelli , "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 > > > > 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
Re: [PATCH v3 5/7] mmc: bcmstb: Add support for bcm2712 SD controller
Hi, Am 21.12.23 um 16:13 schrieb Florian Fainelli: 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 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]. 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? [1] - https://github.com/raspberrypi/linux/commit/b627647c4500d39cb026924b608841fdf4d4d7e9
Re: [PATCH v3 5/7] mmc: bcmstb: Add support for bcm2712 SD controller
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 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... -- Florian smime.p7s Description: S/MIME Cryptographic Signature
[PATCH v3 5/7] mmc: bcmstb: Add support for bcm2712 SD controller
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
---
drivers/mmc/bcmstb_sdhci.c | 173 -
1 file changed, 172 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/bcmstb_sdhci.c b/drivers/mmc/bcmstb_sdhci.c
index dc96818cff..21489e66c0 100644
--- a/drivers/mmc/bcmstb_sdhci.c
+++ b/drivers/mmc/bcmstb_sdhci.c
@@ -38,6 +38,16 @@
*/
#define BCMSTB_SDHCI_MINIMUM_CLOCK_FREQUENCY 40
+#define SDIO_CFG_CTRL 0x0
+#define SDIO_CFG_CTRL_SDCD_N_TEST_EN BIT(31)
+#define SDIO_CFG_CTRL_SDCD_N_TEST_LEV BIT(30)
+
+#define SDIO_CFG_SD_PIN_SEL0x44
+#define SDIO_CFG_SD_PIN_SEL_MASK 0x3
+#define SDIO_CFG_SD_PIN_SEL_CARD BIT(1)
+
+#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
+
/*
* This driver has only been tested with eMMC devices; SD devices may
* not work.
@@ -47,6 +57,53 @@ struct sdhci_bcmstb_plat {
struct mmc mmc;
};
+struct sdhci_bcmstb_host {
+ struct sdhci_host host;
+ u32 shadow_cmd;
+ u32 shadow_blk;
+ bool is_cmd_shadowed;
+ bool is_blk_shadowed;
+};
+
+struct sdhci_brcmstb_dev_priv {
+ int (*init)(struct udevice *dev);
+ struct sdhci_ops *ops;
+};
+
+static inline struct sdhci_bcmstb_host *to_bcmstb_host(struct sdhci_host *host)
+{
+ return container_of(host, struct sdhci_bcmstb_host, host);
+}
+
+static int sdhci_brcmstb_init_2712(struct udevice *dev)
+{
+ struct sdhci_host *host = dev_get_priv(dev);
+ void *cfg_regs;
+ u32 reg;
+
+ /* Map in the non-standard CFG registers */
+ cfg_regs = dev_remap_addr_name(dev, "cfg");
+ if (!cfg_regs)
+ return -ENOENT;
+
+ if ((host->mmc->host_caps & MMC_CAP_NONREMOVABLE) ||
+ (host->mmc->host_caps & MMC_CAP_NEEDS_POLL)) {
+ /* Force presence */
+ reg = readl(cfg_regs + SDIO_CFG_CTRL);
+ reg &= ~SDIO_CFG_CTRL_SDCD_N_TEST_LEV;
+ reg |= SDIO_CFG_CTRL_SDCD_N_TEST_EN;
+ writel(reg, cfg_regs + SDIO_CFG_CTRL);
+ } else {
+ /* Enable card detection line */
+ reg = readl(cfg_regs + SDIO_CFG_SD_PIN_SEL);
+ reg &= ~SDIO_CFG_SD_PIN_SEL_MASK;
+ reg |= SDIO_CFG_SD_PIN_SEL_CARD;
+ writel(reg, cfg_regs + SDIO_CFG_SD_PIN_SEL);
+ }
+
+ return 0;
+}
+
static int sdhci_bcmstb_bind(struct udevice *dev)
{
struct sdhci_bcmstb_plat *plat = dev_get_plat(dev);
@@ -58,10 +115,14 @@ static int sdhci_bcmstb_probe(struct udevice *dev)
{
struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
struct sdhci_bcmstb_plat *plat = dev_get_plat(dev);
- struct sdhci_host *host = dev_get_priv(dev);
+ struct sdhci_bcmstb_host *bcmstb = dev_get_priv(dev);
+ struct sdhci_host *host = &bcmstb->host;
+ struct sdhci_brcmstb_dev_priv *dev_priv;
fdt_addr_t base;
int ret;
+ dev_priv = (struct sdhci_brcmstb_dev_priv *)dev_get_driver_data(dev);
+
base = dev_read_addr(dev);
if (base == FDT_ADDR_T_NONE)
return -EINVAL;
@@ -75,6 +136,10 @@ static int sdhci_bcmstb_probe(struct udevice *dev)
host->mmc = &plat->mmc;
host->mmc->dev = dev;
+
+ if (dev_priv && dev_priv->ops)
+ host->ops = dev_priv->ops;
+
ret = sdhci_setup_cfg(&plat->cfg, host,
BCMSTB_SDHCI_MAXIMUM_CLOCK_FREQUENCY,
BCMSTB_SDHCI_MINIMUM_CLOCK_FREQUENCY);
@@ -84,10 +149,116 @@ static int sdhci_bcmstb_probe(struct udevice *dev)
upriv->mmc = &plat->mmc;
host->mmc->priv = host;
+ if (dev_priv && dev_priv->init) {
+ ret = dev_priv->init(dev);
+ if (ret)
+ return ret;
+ }
+
return sdhci_probe(dev);
}
+static u16 sdhci_brcmstb_32bits_readw(struct sdhci_host *host, int reg)
+{
+ struct sdhci_bcmstb_host *bcmstb = to_bcmstb_host(host);
+ u16 word;
+ u32 val;
+
+ if (reg == SDHCI_TRANSFER_MODE && bcmstb->is_cmd_shadowed) {
+ /* Get the saved transfer mode */
+ val = bcmstb->shadow_cmd;
+ } else if ((reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) &&
+ bcmstb->is_blk_shadowed) {
+ /* Get the saved block info */
+
