Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Christian Marangi <ansuelsmth@gmail.com>]

On Thu, Jan 16, 2025 at 07:01:13AM +0000, Andy-ld Lu (卢东) wrote:
> On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > Add support for AN7581 MMC Host. The MMC Host controller is based on
> > mt7622 with the difference of not having regulator supply and
> > state_uhs
> > pins and hclk clock.
> > 
> > Some minor fixes are applied to check if the state_uhs pins are
> > defined
> > and make hclk optional for the new airoha compatible.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-----
> > --
> >  1 file changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index efb0d2d5716b..9d6868883c91 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > mt8196_compat = {
> >  	.support_new_rx = true,
> >  };
> >  
> > +static const struct mtk_mmc_compatible an7581_compat = {
> > +	.clk_div_bits = 12,
> > +	.recheck_sdio_irq = true,
> > +	.hs400_tune = false,
> > +	.pad_tune_reg = MSDC_PAD_TUNE0,
> > +	.async_fifo = true,
> > +	.data_tune = true,
> > +	.busy_check = true,
> > +	.stop_clk_fix = true,
> > +	.stop_dly_sel = 3,
> > +	.enhance_rx = true,
> > +	.support_64g = false,
> > +};
> > +
> >  static const struct of_device_id msdc_of_ids[] = {
> >  	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
> >  	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
> > @@ -680,7 +694,7 @@ static const struct of_device_id msdc_of_ids[] =
> > {
> >  	{ .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
> >  	{ .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
> >  	{ .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
> > -
> > +	{ .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > mmc_host *mmc, struct mmc_ios *ios)
> >  	struct msdc_host *host = mmc_priv(mmc);
> >  	int ret;
> >  
> > +	/* Skip setting supply if not supported */
> > +	if (!mmc->supply.vqmmc)
> > +		return 0;
> > +
> Hi Christian,
> 
> I think here is no need. If you have not 'vqmmc' in the
> dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the corresponding
> flow would not be executed.
> 
> And another question, host->pins_default is just selected here, that
> would be lost.
>  
> >  	if (!IS_ERR(mmc->supply.vqmmc)) {
> >  		if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
> >  		    ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > mmc_host *mmc, int enb)
> >  				dev_dbg(host->dev, "SDIO eint irq:
> > %d!\n", host->eint_irq);
> >  			}
> >  
> > -			pinctrl_select_state(host->pinctrl, host-
> > >pins_uhs);
> > +			/* Skip setting uhs pins if not supported */
> > +			if (host->pins_uhs)
> > +				pinctrl_select_state(host->pinctrl,
> > host->pins_uhs);
> >  		} else {
> >  			dev_pm_clear_wake_irq(host->dev);
> >  		}
> > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >  
> >  	msdc_set_buswidth(host, ios->bus_width);
> >  
> > +	/* Skip regulator if not supported */
> > +	if (!mmc->supply.vmmc)
> > +		goto skip_regulator;
> > +
> No need too.
> 
> >  	/* Suspend/Resume will do power off/on */
> >  	switch (ios->power_mode) {
> >  	case MMC_POWER_UP:
> > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >  		break;
> >  	}
> >  
> > +skip_regulator:
> >  	if (host->mclk != ios->clock || host->timing != ios->timing)
> >  		msdc_set_mclk(host, ios->timing, ios->clock);
> >  }
> > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > platform_device *pdev,
> >  	if (IS_ERR(host->src_clk))
> >  		return PTR_ERR(host->src_clk);
> >  
> > -	host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > -	if (IS_ERR(host->h_clk))
> > -		return PTR_ERR(host->h_clk);
> > +	/* AN7581 SoC doesn't have hclk */
> > +	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > +		host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > +		if (IS_ERR(host->h_clk))
> > +			return PTR_ERR(host->h_clk);
> > +	}
> devm_clk_get_optional could be used to instead here, no need to use
> compatible to distinguish.
>

I can make the hclk optional but I think this would affect also every
other compatible by hiding broken clock configuration.

> >  	host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
> >  	if (IS_ERR(host->bus_clk))
> > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >  		return PTR_ERR(host->pins_default);
> >  	}
> >  
> > -	host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > "state_uhs");
> > -	if (IS_ERR(host->pins_uhs)) {
> > -		dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> > -		return PTR_ERR(host->pins_uhs);
> > +	/* AN7581 doesn't have state_uhs pins */
> > +	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > +		host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > "state_uhs");
> > +		if (IS_ERR(host->pins_uhs)) {
> > +			dev_err(&pdev->dev, "Cannot find pinctrl
> > uhs!\n");
> > +			return PTR_ERR(host->pins_uhs);
> > +		}
> >  	}
> Could you consider to set a dummy 'state_uhs' same as 'state_default'
> in the dts, that you could not use compatible to distinguish here.
> 

This is problematic, correct me if I'm wrong, you are suggesting to
assign the emmc pins to both default and uhs? This is problematic as the
pinctrl driver would complain that such pins are already assigned to
something. Also I don't think it's possible to assign these pins to a
dummy pin.

> >  
> >  	/* Support for SDIO eint irq ? */
> > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >  		dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> >  		goto release_clk;
> >  	}
> > +
> > +	/* AN7581 without regulator require tune to OCR values */
> > +	if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
> > +	    !mmc->ocr_avail)
> > +		mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > +
> Maybe you could use regulator-fixed in the dts and configure min/max
> voltage to get ocr_avail, no need to set hard code here. 
> 

Also suggested by Wenbin Mei (梅文彬) and I just tried this.

This can't be done, fixed-regulator needs to have the same min and max
voltage or they fail to probe sooo fixed-regulator saddly can't be used
:(

I will send a new version of this with the other point corrected but I
think a compatible and these additional if is a must :(

> >  	msdc_init_hw(host);
> >  
> >  	if (mmc->caps2 & MMC_CAP2_CQE) {

-- 
	Ansuel