Hi, Jaehoon
Thanks for your review!
чт, 5 дек. 2024 г. в 02:37, Jaehoon Chung <jh80.ch...@samsung.com>:
...[snip]...
> > +
> > +static void sdhci_phy_1_8v_init(struct sdhci_host *host)
> > +{
> > + struct snps_sdhci_plat *plat = dev_get_plat(host->mmc->dev);
> > + u32 val;
> > +
> > + /* deassert phy reset & set tx drive strength */
> > + val = PHY_CNFG_RSTN_DEASSERT;
> > + val |= FIELD_PREP(PHY_CNFG_PAD_SP_MASK, PHY_CNFG_PAD_SP);
> > + val |= FIELD_PREP(PHY_CNFG_PAD_SN_MASK, PHY_CNFG_PAD_SN);
> > + sdhci_writel(host, val, PHY_CNFG_R);
> > +
> > + /* disable delay line */
> > + sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE, PHY_SDCLKDL_CNFG_R);
> > +
> > + /* set delay line */
> > + sdhci_writeb(host, plat->delay_line, PHY_SDCLKDL_DC_R);
> > + sdhci_writeb(host, PHY_DLL_CNFG2_JUMPSTEP, PHY_DLL_CNFG2_R);
> > +
> > + /* enable delay lane */
> > + val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > + val &= ~(PHY_SDCLKDL_CNFG_UPDATE);
> > + sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > + /* configure phy pads */
> > + val = PHY_PAD_RXSEL_1V8;
> > + val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLUP);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > + sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > + sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > + sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > +
> > + val = FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > + sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > +
> > + val = PHY_PAD_RXSEL_1V8;
> > + val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLDOWN);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > + sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > +
> > + /* enable data strobe mode */
> > + sdhci_writeb(host, FIELD_PREP(PHY_DLLDL_CNFG_SLV_INPSEL_MASK,
> > PHY_DLLDL_CNFG_SLV_INPSEL),
> > + PHY_DLLDL_CNFG_R);
> > +
> > + /* enable phy dll */
> > + sdhci_writeb(host, PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void sdhci_phy_3_3v_init(struct sdhci_host *host)
> > +{
> > + struct snps_sdhci_plat *plat = dev_get_plat(host->mmc->dev);
> > + u32 val;
> > +
> > + /* deassert phy reset & set tx drive strength */
> > + val = PHY_CNFG_RSTN_DEASSERT;
> > + val |= FIELD_PREP(PHY_CNFG_PAD_SP_MASK, PHY_CNFG_PAD_SP);
> > + val |= FIELD_PREP(PHY_CNFG_PAD_SN_MASK, PHY_CNFG_PAD_SN);
> > + sdhci_writel(host, val, PHY_CNFG_R);
> > +
> > + /* disable delay line */
> > + sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE, PHY_SDCLKDL_CNFG_R);
> > +
> > + /* set delay line */
> > + sdhci_writeb(host, plat->delay_line, PHY_SDCLKDL_DC_R);
> > + sdhci_writeb(host, PHY_DLL_CNFG2_JUMPSTEP, PHY_DLL_CNFG2_R);
> > +
> > + /* enable delay lane */
> > + val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > + val &= ~(PHY_SDCLKDL_CNFG_UPDATE);
> > + sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > + /* configure phy pads */
> > + val = PHY_PAD_RXSEL_3V3;
> > + val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLUP);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > + sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > + sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > + sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > +
> > + val = FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > + sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > +
> > + val = PHY_PAD_RXSEL_3V3;
> > + val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLDOWN);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P);
> > + val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N);
> > + sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > +
> > + /* enable phy dll */
> > + sdhci_writeb(host, PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void snps_sdhci_set_phy(struct sdhci_host *host)
> > +{
> > + struct snps_sdhci_plat *plat = dev_get_plat(host->mmc->dev);
> > + struct mmc *mmc = host->mmc;
> > +
> > + /* Before power on, set PHY configs */
> > + if ((plat->flags & FLAG_IO_FIXED_1V8) ||
> > + mmc->signal_voltage == MMC_SIGNAL_VOLTAGE_180)
> > + sdhci_phy_1_8v_init(host);
> > + else
> > + sdhci_phy_3_3v_init(host);
>
>
> Well, if my reading is right, sdhci_phy_1_8v_init and sdhci_3_3v_init are
> different PHY_PAD_RXSEL_1V8 and PHY_PAD_RXSEL_3V3.
> Also enable data strobe mode part.
>
> I'm not sure why mainline kernel code doesn't re-use some code.
> It can be reducing code.
> Even though There are no objection because of mainline kernel codes, frankly,
> it's not my preference.
>
> I have posted the kernel patch to reuse code. (I'm not sure if can be
> accepted.)
> https://patchwork.kernel.org/project/linux-mmc/patch/20241204100507.330025-1-jh80.ch...@samsung.com/
>
>
Good catch, I'll wait when your linux patch will be accepted.
And I will make the same changes here.
...[snip]...
> > +static const struct sdhci_ops snps_sdhci_ops = {
> > + .set_ios_post = snps_sdhci_set_ios_post,
> > + .platform_execute_tuning = snps_sdhci_execute_tuning,
> > + .set_enhanced_strobe = snps_sdhci_set_enhanced_strobe,
> > +#if CONFIG_IS_ENABLED(CONFIG_MMC_SDHCI_ADMA_HELPERS)
>
> CONFIG_IS_ENABLED(MMC_SDHCI_ADMA_HELPERS) ?
>
I'll fix it in v2.
> > + .adma_write_desc = snps_sdhci_adma_write_desc,
> > +#endif
> > +};
> > +
> > +static int snps_sdhci_probe(struct udevice *dev)
> > +{
> > + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> > + struct snps_sdhci_plat *plat = dev_get_plat(dev);
> > + struct mmc_config *cfg = &plat->cfg;
> > + struct sdhci_host *host = dev_get_priv(dev);
> > + struct clk clk;
> > + int ret;
> > +
> > + plat->delay_line = PHY_SDCLKDL_DC_DEFAULT;
> > +
> > + host->max_clk = cfg->f_max;
> > + ret = clk_get_by_index(dev, 0, &clk);
> > + if (!ret) {
> > + ret = clk_set_rate(&clk, host->max_clk);
> > + if (IS_ERR_VALUE(ret))
> > + printf("%s clk set rate fail!\n", __func__);
>
> Even though clock set rate is failed, it doesn't matter to be still going?
>
Well, I took this part from the rockchip sdhci driver as is...
But mmc clk actually is fixed on TH1520 and we can't change it.
So I'll drop this in v2.
> > + } else {
> > + printf("%s fail to get clk\n", __func__);
>
> Ditto?
>
> > + }
> > +
> > + host->ops = &snps_sdhci_ops;
> > +
> > + host->mmc = &plat->mmc;
> > + host->mmc->priv = host;
> > + host->mmc->dev = dev;
> > + upriv->mmc = host->mmc;
> > +
> > + ret = sdhci_setup_cfg(cfg, host, cfg->f_max, EMMC_MIN_FREQ);
> > + if (ret)
> > + return ret;
> > +
> > + if ((dev_read_bool(dev, "mmc-ddr-1_8v")) ||
> > + (dev_read_bool(dev, "mmc-hs200-1_8v")) ||
> > + (dev_read_bool(dev, "mmc-hs400-1_8v")))
> > + plat->flags |= FLAG_IO_FIXED_1V8;
> > + else
> > + plat->flags &= ~FLAG_IO_FIXED_1V8;
> > +
> > + return sdhci_probe(dev);
> > +}
> > +
> > +static int snps_sdhci_of_to_plat(struct udevice *dev)
> > +{
> > + struct snps_sdhci_plat *plat = dev_get_plat(dev);
> > + struct mmc_config *cfg = &plat->cfg;
> > + struct sdhci_host *host = dev_get_priv(dev);
> > + int ret;
> > +
> > + host->name = dev->name;
> > + host->ioaddr = dev_read_addr_ptr(dev);
> > +
> > + ret = mmc_of_parse(dev, cfg);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> Is it possible to use "return mmc_of_parse();"?
Yes, it is. Thanks, will be fixed in v2.
>
> Best Regards,
> Jaehoon Chung
>
Best wishes,
Maksim
> > +}
> > +
> > +static int snps_sdhci_bind(struct udevice *dev)
> > +{
> > + struct snps_sdhci_plat *plat = dev_get_plat(dev);
> > +
> > + return sdhci_bind(dev, &plat->mmc, &plat->cfg);
> > +}
> > +
> > +static const struct udevice_id snps_sdhci_ids[] = {
> > + { .compatible = "thead,th1520-dwcmshc" }
> > +};
> > +
> > +U_BOOT_DRIVER(snps_sdhci_drv) = {
> > + .name = "snps_sdhci",
> > + .id = UCLASS_MMC,
> > + .of_match = snps_sdhci_ids,
> > + .of_to_plat = snps_sdhci_of_to_plat,
> > + .ops = &sdhci_ops,
> > + .bind = snps_sdhci_bind,
> > + .probe = snps_sdhci_probe,
> > + .priv_auto = sizeof(struct sdhci_host),
> > + .plat_auto = sizeof(struct snps_sdhci_plat),
> > +};
> > --
> > 2.45.2
