On 08:55 Sun 14 Apr , Florian Fainelli wrote: > > > 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 <andrea.porta@xxxxxxxx> > > --- > > 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. Ack. > > > + 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()? No real reason. I assume only the lower boundary is critical so I can use usleep_range instead. Will be fixed in a future patch, the SD express support will be drpped in V2 since nto strictly necessary. > > > + > > + /* 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. Ditto. > > > + > > + /* 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. Ack. > > > + 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