[6/6] mmc: sdhci-brcmstb: Add BCM2712 SD Express support

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@suse.com>
---
 drivers/mmc/host/Kconfig         |   1 +
 drivers/mmc/host/sdhci-brcmstb.c | 147 ++++++++++++++++++++++++++++++-
 2 files changed, 147 insertions(+), 1 deletion(-)

Le 14/04/2024 à 00:14, Andrea della Porta a écrit :
> 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-IBi9RG/b67k@public.gmane.org>
> ---
>   drivers/mmc/host/Kconfig         |   1 +
>   drivers/mmc/host/sdhci-brcmstb.c | 147 ++++++++++++++++++++++++++++++-
>   2 files changed, 147 insertions(+), 1 deletion(-)

...

> +	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);
> +	}
> +
> +	dev_dbg(dev, "%s -> %d\n", __func__, ret);

Is this really useful?

> +	return ret;
> +}
> +

...

> @@ -468,6 +581,24 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>   	if (res)
>   		goto err;
>   
> +	priv->sde_1v8 = devm_regulator_get_optional(&pdev->dev, "sde-1v8");
> +	if (IS_ERR(priv->sde_1v8))
> +		priv->flags &= ~BRCMSTB_PRIV_FLAGS_HAS_SD_EXPRESS;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	if (iomem) {
> +		priv->sde_ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
> +		if (IS_ERR(priv->sde_ioaddr))
> +			priv->sde_ioaddr = NULL;
> +	}
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> +	if (iomem) {
> +		priv->sde_ioaddr2 = devm_ioremap_resource(&pdev->dev, iomem);
> +		if (IS_ERR(priv->sde_ioaddr2))
> +			priv->sde_ioaddr = NULL;

sde_ioaddr2 ?

> +	}
> +
>   	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
>   	if (IS_ERR(priv->pinctrl)) {
>   			no_pinctrl = true;
> @@ -478,8 +609,16 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>   			no_pinctrl = true;
>   	}
>   
> -	if (no_pinctrl )
> +	priv->pins_sdex = pinctrl_lookup_state(priv->pinctrl, "sd-express");
> +	if (IS_ERR(priv->pins_sdex)) {
> +			dev_dbg(&pdev->dev, "No pinctrl sd-express state\n");
> +			no_pinctrl = true;

Indentation looks too large.

> +	}
> +
> +	if (no_pinctrl || !priv->sde_ioaddr || !priv->sde_ioaddr2) {
>   		priv->pinctrl = NULL;
> +		priv->flags &= ~BRCMSTB_PRIV_FLAGS_HAS_SD_EXPRESS;
> +	}
>   
>   	/*
>   	 * Automatic clock gating does not work for SD cards that may

...

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@suse.com>
> ---
>   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.

On 09:34 Sun 14 Apr     , Christophe JAILLET wrote:
> Le 14/04/2024 à 00:14, Andrea della Porta a écrit :
> > 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-IBi9RG/b67k@public.gmane.org>
> > ---
> >   drivers/mmc/host/Kconfig         |   1 +
> >   drivers/mmc/host/sdhci-brcmstb.c | 147 ++++++++++++++++++++++++++++++-
> >   2 files changed, 147 insertions(+), 1 deletion(-)
> 
> ...
> 
> > +	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);
> > +	}
> > +
> > +	dev_dbg(dev, "%s -> %d\n", __func__, ret);
> 
> Is this really useful?

Not really. Removed.

> 
> > +	return ret;
> > +}
> > +
> 
> ...
> 
> > @@ -468,6 +581,24 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
> >   	if (res)
> >   		goto err;
> > +	priv->sde_1v8 = devm_regulator_get_optional(&pdev->dev, "sde-1v8");
> > +	if (IS_ERR(priv->sde_1v8))
> > +		priv->flags &= ~BRCMSTB_PRIV_FLAGS_HAS_SD_EXPRESS;
> > +
> > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > +	if (iomem) {
> > +		priv->sde_ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
> > +		if (IS_ERR(priv->sde_ioaddr))
> > +			priv->sde_ioaddr = NULL;
> > +	}
> > +
> > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> > +	if (iomem) {
> > +		priv->sde_ioaddr2 = devm_ioremap_resource(&pdev->dev, iomem);
> > +		if (IS_ERR(priv->sde_ioaddr2))
> > +			priv->sde_ioaddr = NULL;
> 
> sde_ioaddr2 ?
> 
> > +	}
> > +
> >   	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
> >   	if (IS_ERR(priv->pinctrl)) {
> >   			no_pinctrl = true;
> > @@ -478,8 +609,16 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
> >   			no_pinctrl = true;
> >   	}
> > -	if (no_pinctrl )
> > +	priv->pins_sdex = pinctrl_lookup_state(priv->pinctrl, "sd-express");
> > +	if (IS_ERR(priv->pins_sdex)) {
> > +			dev_dbg(&pdev->dev, "No pinctrl sd-express state\n");
> > +			no_pinctrl = true;
> 
> Indentation looks too large.

Ack.

> 
> > +	}
> > +
> > +	if (no_pinctrl || !priv->sde_ioaddr || !priv->sde_ioaddr2) {
> >   		priv->pinctrl = NULL;
> > +		priv->flags &= ~BRCMSTB_PRIV_FLAGS_HAS_SD_EXPRESS;
> > +	}
> >   	/*
> >   	 * Automatic clock gating does not work for SD cards that may
> 
> ...
> 

In general I'll drop SD express patch for now, it will be re-introduced in a future patch.

Best regards,
Andrea

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@suse.com>
> > ---
> >   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