On 4/13/2024 3:14 PM, Andrea della Porta wrote: > Broadcom BCM2712 SDHCI controller is SD Express capable. Add support > for sde capability where the implementation is based on downstream driver, > diverging from it in the way that init_sd_express callback is invoked: > in downstream the sdhci_ops structure has been augmented with a new > function ptr 'init_sd_express' that just propagate the call to the > driver specific callback so that the callstack from a structure > standpoint is mmc_host_ops -> sdhci_ops. The drawback here is in the > added level of indirection (the newly added init_sd_express is > redundant) and the sdhci_ops structure declaration has to be changed. > To overcome this the presented approach consist in patching the mmc_host_ops > init_sd_express callback to point directly to the custom function defined in > this driver (see struct brcmstb_match_priv). > > Signed-off-by: Andrea della Porta > --- > drivers/mmc/host/Kconfig | 1 + > drivers/mmc/host/sdhci-brcmstb.c | 147 ++++++++++++++++++++++++++++++- > 2 files changed, 147 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index aebc587f77a7..343ccac1a4e4 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -1018,6 +1018,7 @@ config MMC_SDHCI_BRCMSTB > depends on ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST > depends on MMC_SDHCI_PLTFM > select MMC_CQHCI > + select OF_DYNAMIC > default ARCH_BRCMSTB || BMIPS_GENERIC > help > This selects support for the SDIO/SD/MMC Host Controller on > diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c > index 907a4947abe5..56fb34a75ec2 100644 > --- a/drivers/mmc/host/sdhci-brcmstb.c > +++ b/drivers/mmc/host/sdhci-brcmstb.c > @@ -29,6 +29,7 @@ > > #define BRCMSTB_PRIV_FLAGS_HAS_CQE BIT(0) > #define BRCMSTB_PRIV_FLAGS_GATE_CLOCK BIT(1) > +#define BRCMSTB_PRIV_FLAGS_HAS_SD_EXPRESS BIT(2) > > #define SDHCI_ARASAN_CQE_BASE_ADDR 0x200 > > @@ -50,13 +51,19 @@ struct sdhci_brcmstb_priv { > unsigned int flags; > struct clk *base_clk; > u32 base_freq_hz; > + struct regulator *sde_1v8; > + struct device_node *sde_pcie; > + void *__iomem sde_ioaddr; > + void *__iomem sde_ioaddr2; > struct pinctrl *pinctrl; > struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_sdex; > }; > > struct brcmstb_match_priv { > void (*hs400es)(struct mmc_host *mmc, struct mmc_ios *ios); > void (*cfginit)(struct sdhci_host *host); > + int (*init_sd_express)(struct mmc_host *mmc, struct mmc_ios *ios); > struct sdhci_ops *ops; > const unsigned int flags; > }; > @@ -263,6 +270,105 @@ static void sdhci_brcmstb_cfginit_2712(struct sdhci_host *host) > } > } > > +static int bcm2712_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios) > +{ > + struct sdhci_host *host = mmc_priv(mmc); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_brcmstb_priv *brcmstb_priv = sdhci_pltfm_priv(pltfm_host); > + struct device *dev = host->mmc->parent; > + u32 ctrl_val; > + u32 present_state; > + int ret; > + > + if (!brcmstb_priv->sde_ioaddr || !brcmstb_priv->sde_ioaddr2) > + return -EINVAL; > + > + if (!brcmstb_priv->pinctrl) > + return -EINVAL; > + > + /* Turn off the SD clock first */ > + sdhci_set_clock(host, 0); > + > + /* Disable SD DAT0-3 pulls */ > + pinctrl_select_state(brcmstb_priv->pinctrl, brcmstb_priv->pins_sdex); > + > + ctrl_val = readl(brcmstb_priv->sde_ioaddr); > + dev_dbg(dev, "ctrl_val 1 %08x\n", ctrl_val); > + > + /* Tri-state the SD pins */ > + ctrl_val |= 0x1ff8; No magic values please. > + writel(ctrl_val, brcmstb_priv->sde_ioaddr); > + dev_dbg(dev, "ctrl_val 1->%08x (%08x)\n", ctrl_val, readl(brcmstb_priv->sde_ioaddr)); > + /* Let voltages settle */ > + udelay(100); Why not usleep_range()? > + > + /* Enable the PCIe sideband pins */ > + ctrl_val &= ~0x6000; No magic values please. > + writel(ctrl_val, brcmstb_priv->sde_ioaddr); > + dev_dbg(dev, "ctrl_val 1->%08x (%08x)\n", ctrl_val, readl(brcmstb_priv->sde_ioaddr)); > + /* Let voltages settle */ > + udelay(100); Likewise. > + > + /* Turn on the 1v8 VDD2 regulator */ > + ret = regulator_enable(brcmstb_priv->sde_1v8); > + if (ret) > + return ret; > + > + /* Wait for Tpvcrl */ > + msleep(1); > + > + /* Sample DAT2 (CLKREQ#) - if low, card is in PCIe mode */ > + present_state = sdhci_readl(host, SDHCI_PRESENT_STATE); > + present_state = (present_state & SDHCI_DATA_LVL_MASK) >> SDHCI_DATA_LVL_SHIFT; > + dev_dbg(dev, "state = 0x%08x\n", present_state); > + > + if (present_state & BIT(2)) { Likewise, replace with constant. > + dev_err(dev, "DAT2 still high, abandoning SDex switch\n"); > + return -ENODEV; > + } > + > + /* Turn on the LCPLL PTEST mux */ > + ctrl_val = readl(brcmstb_priv->sde_ioaddr2 + 20); // misc5 > + ctrl_val &= ~(0x7 << 7); > + ctrl_val |= 3 << 7; > + writel(ctrl_val, brcmstb_priv->sde_ioaddr2 + 20); > + dev_dbg(dev, "misc 5->%08x (%08x)\n", ctrl_val, readl(brcmstb_priv->sde_ioaddr2 + 20)); > + > + /* PTEST diff driver enable */ > + ctrl_val = readl(brcmstb_priv->sde_ioaddr2); > + ctrl_val |= BIT(21); > + writel(ctrl_val, brcmstb_priv->sde_ioaddr2); > + > + dev_dbg(dev, "misc 0->%08x (%08x)\n", ctrl_val, readl(brcmstb_priv->sde_ioaddr2)); > + > + /* Wait for more than the minimum Tpvpgl time */ > + msleep(100); > + > + if (brcmstb_priv->sde_pcie) { > + struct of_changeset changeset; > + static struct property okay_property = { > + .name = "status", > + .value = "okay", > + .length = 5, > + }; > + > + /* Enable the pcie controller */ > + of_changeset_init(&changeset); > + ret = of_changeset_update_property(&changeset, > + brcmstb_priv->sde_pcie, > + &okay_property); > + if (ret) { > + dev_err(dev, "%s: failed to update property - %d\n", __func__, > + ret); > + return -ENODEV; > + } > + ret = of_changeset_apply(&changeset); > + } Why are you doing this? Cannot the firmware enable/disable the node according to the boot mode or something else? This is not going to fly for upstream, sorry. -- Florian