Re: [PATCH v1 2/2] mmc: sdhci-of-dwcmshc: Add support for StarFive JHB100

[Changhuang Liang]

Hi, Adrian

Thanks for the review.

> On 19/09/2026 12:29, Changhuang Liang wrote:
> > Add support for the DWCMSHC controller found on the StarFive JHB100
> > SoC.
> >
> > The JHB100 CMSHC controller has only 32-bit DMA capability by default.
> > The higher DMA address bits [35:32] can be configured through the
> > PER1 SYSCON, extending the DMA address range up to 36 bits. Fix these
> > bits to 4, which limits the DMA range to [0x40000000 - 0x4ffffffff].
> 
> 0x40000000 missing a 0 ?

Yes, here it should be 0x400000000.

> >
> > The cclk_tx clock must be disabled before sdhci_set_clock() to
> > eliminate glitches, and re-enabled afterwards. It is enabled during
> > probe and disabled in the platform deinit callback, which is invoked
> > from dwcmshc_remove().
> >
> > Additionally, clear the PHY_DLLDL_CNFG_R register and re-initialize
> > the PHY when setting UHS signaling, and force 1.8V fixed I/O when the
> > DT specifies a DDR/HS200/HS400 1.8V mode.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 158
> > ++++++++++++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index 1baa2379c9c3..021b03c17312 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -48,6 +48,9 @@
> >  #define DWCMSHC_ENHANCED_STROBE		BIT(8)
> >  #define DWCMSHC_EMMC_ATCTRL		0x40
> >  #define DWCMSHC_AT_STAT			0x44
> > +/* StarFive JHB100 SoC MSHCCS register */
> > +#define DWCMSHC_ARWADDR			GENMASK(3, 0)
> > +#define DWCMSHC_ARWADDR_DEFAULT		4
> >  /* Tuning and auto-tuning fields in AT_CTRL_R control register */
> >  #define AT_CTRL_AT_EN			BIT(0) /* autotuning is enabled */
> >  #define AT_CTRL_CI_SEL			BIT(1) /* interval to drive center phase
> select */
> > @@ -294,6 +297,11 @@ struct k230_priv  {
> >  	struct regmap *hi_sys_regmap;
> >  };
> >
> > +struct jhb100_priv {
> > +	struct clk *cclk_tx;
> > +	bool cclk_tx_on;
> > +};
> > +
> >  #define DWCMSHC_MAX_OTHER_CLKS 3
> >
> >  struct dwcmshc_priv {
> > @@ -314,6 +322,7 @@ struct dwcmshc_pltfm_data {
> >  	const struct sdhci_pltfm_data pdata;
> >  	const struct cqhci_host_ops *cqhci_host_ops;
> >  	int (*init)(struct device *dev, struct sdhci_host *host, struct
> > dwcmshc_priv *dwc_priv);
> > +	void (*deinit)(struct dwcmshc_priv *dwc_priv);
> >  	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv
> > *dwc_priv);  };
> >
> > @@ -2257,6 +2266,145 @@ static const struct dwcmshc_pltfm_data
> sdhci_dwcmshc_hpe_gsc_pdata = {
> >  	.init = dwcmshc_hpe_gsc_init,
> >  };
> >
> > +static int sdhci_jhb100_set_dma_mask(struct sdhci_host *host) {
> > +	struct mmc_host *mmc = host->mmc;
> > +	struct device *dev = mmc_dev(mmc);
> > +	int ret;
> > +
> > +	/*
> > +	 * The StarFive JHB100's CMSHC controller only has 32-bit DMA
> > +	 * capability by default, but the higher DMA address bits [35:32]
> > +	 * can be configured through SYSCON, thus supporting up to 36-bit
> > +	 * DMA capability.
> 
> Doesn't really explain how a 36-bit DMA mask works if the valid range is
> 0x400000000 - 0x4ffffffff
> 
> Sashiko questions whether it works:
> 
> 	https://sashiko.dev/#/patchset/20260919092950.178617-1-changhuang
> .liang%40starfivetech.com
> 

Here, the 36-bit DMA mask only describes the hardware's capability. Since only 
the lower 4 bits of SYSCON are valid and they apply to both the start address and 
the end address, on the JHB100 SoC, if SYSCON is configured to 4, the effective
 range that this controller can address is only 0x4 0000 0000 - 0x4 ffff ffff, and it 
cannot fully use the 36-bit DMA capability. Usually we need to use dma-ranges to 
restrict the device's DMA address space to fall within this valid range:

bus_nioc_4g: bus_nioc_4g {
	compatible = "simple-bus";
	#address-cells = <2>;
	#size-cells = <2>;
	dma-noncoherent;
	ranges;
	dma-ranges = <0x4 0x00000000 0x0 0x40000000 0x1 0x0>,
		     <0x4 0x00000000 0x4 0x00000000 0x1 0x0>;

	emmc0: mmc@11b00000 {
		compatible = "starfive,jhb100-dwcmshc";
		reg = <0x0 0x11b00000 0x0 0x10000>;
	};
};

With the dma-ranges restriction in place, DMA_MAPPING_ERROR is returned for 
any address outside the mapped ranges. Invalid addresses will not be transferred 
to the hardware.

I will refine the comment here. Or do you have any other suggestions on your side?

> Perhaps also check its point about leaving cclk_tx on during suspend.

Will fix.

> 
> > +	 */
> > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> > +	if (ret)
> > +		dev_err(dev, "Failed to set 36-bit DMA mask.\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static void sdhci_jhb100_set_clock(struct sdhci_host *host, unsigned
> > +int clock) {
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> > +	struct jhb100_priv *priv = dwc_priv->priv;
> > +
> > +	/* the cclk_tx clock is disabled before sdhci_set_clock() to eliminate
> glitches */
> > +	if (priv->cclk_tx_on) {
> > +		clk_disable_unprepare(priv->cclk_tx);
> > +		priv->cclk_tx_on = false;
> > +	}
> > +
> > +	fsleep(15);
> 
> The delay could use a comment explaining why it is needed and where the
> value comes from.

OK, this value is also only a special requirement on the JHB100 SoC.
This value comes from our simulation team.

> 
> > +
> > +	sdhci_set_clock(host, clock);
> > +
> > +	if (clk_prepare_enable(priv->cclk_tx))
> > +		dev_err(mmc_dev(host->mmc), "failed to re-enable cclk_tx\n");
> > +	else
> > +		priv->cclk_tx_on = true;
> > +}
> > +
> > +static void sdhci_jhb100_set_uhs_signaling(struct sdhci_host *host,
> > +					   unsigned int timing)
> > +{
> > +	dwcmshc_set_uhs_signaling(host, timing);
> > +	sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> > +	th1520_sdhci_set_phy(host);
> > +}
> > +
> > +static const struct sdhci_ops starfive_jhb100_ops = {
> > +	.set_dma_mask		= sdhci_jhb100_set_dma_mask,
> > +	.set_clock		= sdhci_jhb100_set_clock,
> > +	.set_uhs_signaling	= sdhci_jhb100_set_uhs_signaling,
> > +	.set_bus_width		= sdhci_set_bus_width,
> > +	.reset			= sdhci_reset,
> 
> Everywhere else seems to reset via dwcmshc_reset().
> Is sdhci_reset() ok here?

Will replace it with dwcmshc_reset().

> 
> > +	.get_max_clock		= dwcmshc_get_max_clock,
> > +	.adma_write_desc	= dwcmshc_adma_write_desc,
> > +	.irq			= dwcmshc_cqe_irq_handler,
> > +	.voltage_switch		= dwcmshc_phy_init,
> > +	.platform_execute_tuning = th1520_execute_tuning, };
> > +
> > +static int dwcmshc_starfive_jhb100_init(struct device *dev, struct
> sdhci_host *host,
> > +					struct dwcmshc_priv *dwc_priv)
> > +{
> > +	struct reset_control *reset;
> > +	struct jhb100_priv *priv;
> > +	unsigned int reg_offset;
> > +	struct regmap *syscon;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(struct jhb100_priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	syscon = syscon_regmap_lookup_by_phandle_args(dev->of_node,
> > +						      "starfive,jhb100-per1-syscon",
> > +						      1, &reg_offset);
> > +	if (IS_ERR(syscon))
> > +		return PTR_ERR(syscon);
> > +
> > +	/* configure the higher DMA address bits [35:32] */
> > +	ret = regmap_update_bits(syscon, reg_offset, DWCMSHC_ARWADDR,
> > +				 DWCMSHC_ARWADDR_DEFAULT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	reset =
> devm_reset_control_get_exclusive_deasserted(mmc_dev(host->mmc),
> NULL);
> > +	if (IS_ERR(reset))
> > +		return dev_err_probe(mmc_dev(host->mmc), PTR_ERR(reset),
> > +				     "Failed to get&deassert reset control\n");
> > +
> > +	priv->cclk_tx = devm_clk_get(dev, "cclk_tx");
> > +	if (IS_ERR(priv->cclk_tx))
> > +		return dev_err_probe(dev, PTR_ERR(priv->cclk_tx),
> > +				     "Failed to get cclk_tx clock\n");
> > +
> > +	ret = clk_prepare_enable(priv->cclk_tx);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable cclk_tx clock\n");
> > +
> > +	priv->cclk_tx_on = true;
> > +
> > +	if (device_property_read_bool(mmc_dev(host->mmc), "mmc-ddr-1_8v")
> ||
> > +	    device_property_read_bool(mmc_dev(host->mmc),
> "mmc-hs200-1_8v") ||
> > +	    device_property_read_bool(mmc_dev(host->mmc),
> "mmc-hs400-1_8v"))
> > +		dwc_priv->flags |= FLAG_IO_FIXED_1V8;
> > +	else
> > +		dwc_priv->flags &= ~FLAG_IO_FIXED_1V8;
> > +
> > +	dwc_priv->delay_line = 0x10;
> 
> Everywhere else delay_line value is a named define.

I will add a define for it:

#define PHY_SDCLKDL_DC_JHB100 0x10 /* delay code for StarFive JHB100 */

> 
> > +	dwc_priv->priv = priv;
> > +
> > +	if (dwc_priv->flags & FLAG_IO_FIXED_1V8) {
> > +		host->flags &= ~SDHCI_SIGNALING_330;
> > +		host->flags |=  SDHCI_SIGNALING_180;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void dwcmshc_starfive_jhb100_deinit(struct dwcmshc_priv
> > +*dwc_priv) {
> > +	struct jhb100_priv *priv = dwc_priv->priv;
> > +
> > +	if (priv->cclk_tx_on)
> > +		clk_disable_unprepare(priv->cclk_tx);
> > +}
> > +
> > +static const struct dwcmshc_pltfm_data sdhci_dwcmshc_jhb100_pdata = {
> > +	.pdata = {
> > +		.ops = &starfive_jhb100_ops,
> > +		.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> > +		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> > +	},
> > +	.init = dwcmshc_starfive_jhb100_init,
> > +	.deinit = dwcmshc_starfive_jhb100_deinit, };
> > +
> >  static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
> >  	.enable		= dwcmshc_sdhci_cqe_enable,
> >  	.disable	= sdhci_cqe_disable,
> > @@ -2377,6 +2525,10 @@ static const struct of_device_id
> sdhci_dwcmshc_dt_ids[] = {
> >  		.compatible = "hpe,gsc-dwcmshc",
> >  		.data = &sdhci_dwcmshc_hpe_gsc_pdata,
> >  	},
> > +	{
> > +		.compatible = "starfive,jhb100-dwcmshc",
> > +		.data = &sdhci_dwcmshc_jhb100_pdata,
> > +	},
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(of, sdhci_dwcmshc_dt_ids); @@ -2503,6
> +2655,8 @@
> > static int dwcmshc_probe(struct platform_device *pdev)
> >  err_rpm:
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_put_noidle(dev);
> > +	if (pltfm_data->deinit)
> > +		pltfm_data->deinit(priv);
> >  err_bus_clk:
> >  	clk_disable_unprepare(priv->bus_clk);
> >  err_clk:
> > @@ -2527,6 +2681,7 @@ static void dwcmshc_remove(struct
> platform_device *pdev)
> >  	struct sdhci_host *host = platform_get_drvdata(pdev);
> >  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	const struct dwcmshc_pltfm_data *pltfm_data = priv->dwcmshc_pdata;
> >
> >  	pm_runtime_get_sync(&pdev->dev);
> >  	pm_runtime_disable(&pdev->dev);
> > @@ -2534,6 +2689,9 @@ static void dwcmshc_remove(struct
> > platform_device *pdev)
> >
> >  	sdhci_remove_host(host, 0);
> >
> > +	if (pltfm_data->deinit)
> > +		pltfm_data->deinit(priv);
> > +
> >  	dwcmshc_disable_card_clk(host);
> >
> >  	clk_disable_unprepare(pltfm_host->clk);
> > --
> > 2.25.1

Best Regards,
Changhuang