RE: [PATCH V4][3/4] mmc: Add dw mobile mmc cmdq rtk driver

[Jyan Chou [周芷安]]

Hi Krzysztof,

Thanks for you to review our code and give some advices. We will check carefully and fix coding style of all,
also, we will drop useless function and message in our next push.
Since some advice we were not sure, we would like to discuss first and modified correctly in our next version.  

>> +     priv = devm_kzalloc(host->dev, sizeof(struct 
>> + dw_mci_rtkemmc_host), GFP_KERNEL);

>sizeof(*)

Thanks. I will correct it in our next version.

>> +     priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
>> +                                               PINCTRL_STATE_DEFAULT);
>> +     if (IS_ERR(priv->pins_default))
>> +             dev_warn(host->dev, "could not get default state\n");
>> +

> So this is required by the driver but not by the bindings.

Since our pinctrl driver supports different driver's usage, we may use bindings to get the correct pinctrl setting, so we add
different pinctrl-names in our bindings. It is correct, or am I misunderstand what you say?

> dev_err_probe. Everywhere where applicable.

Thanks. I will correct it in our next version.

> Read Linux coding style. Multiple times if needed.

Thanks. I will correct it in our next version.

>> +     host->dev = &pdev->dev;
>> +     host->irq_flags = 0;
>> +     host->pdata = pdev->dev.platform_data;
>> +
>> +     regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     host->regs = devm_ioremap_resource(&pdev->dev, regs);

> Use helper for this.

>Open existing, recent drivers and use the them as template or some set of patterns. Sorry, but upstreaming your vendor code will be painful, because you started from some old, buggier version.

Sorry for asking, but I would like to know what is the meaning of "Use helper for this." ? Does it mean we need to get rid of our code above or something else? Thanks a lot.  


Best regards,
Jyan

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 
Sent: Monday, October 30, 2023 3:44 PM
To: Jyan Chou [周芷安] <jyanchou@xxxxxxxxxxx>; ulf.hansson@xxxxxxxxxx; adrian.hunter@xxxxxxxxx; jh80.chung@xxxxxxxxxxx; riteshh@xxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; asutoshd@xxxxxxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx
Cc: linux-mmc@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; briannorris@xxxxxxxxxxxx; doug@xxxxxxxxxxxxx; tonyhuang.sunplus@xxxxxxxxx; abel.vesa@xxxxxxxxxx; william.qiu@xxxxxxxxxxxxxxxx
Subject: Re: [PATCH V4][3/4] mmc: Add dw mobile mmc cmdq rtk driver


External mail.



On 30/10/2023 07:27, Jyan Chou wrote:
> Add Realtek mmc driver to make good use Synopsys DesignWare mmc cmdq 
> host driver.
>
> Signed-off-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
>
> ---
> v3 -> v4:
> - Modify dma setting's code to fix linux coding style.

And coding style of all other parts were ignored. You must fix it everywhere in your code.

...

> +
> +static void dw_mci_rtk_init_card(struct mmc_host *host, struct 
> +mmc_card *card) {
> +     /* In Realtek Platform, we need to attach eMMC card onto mmc host
> +      * during eMMC initialization because of the following reason:
> +      * When system cannot run the hs400, we need to down speed to hs200
> +      * and call mmc_hw_reset and modify the mmc card attribute through mmc host.
> +      * At this moment, system will show errors if host->card = NULL.
> +      */
> +     host->card = card;
> +}
> +
> +static int dw_mci_rtk_parse_dt(struct dw_mci *host) {
> +     struct dw_mci_rtkemmc_host *priv;
> +     const u32 *prop;
> +     int size;
> +
> +     priv = devm_kzalloc(host->dev, sizeof(struct 
> + dw_mci_rtkemmc_host), GFP_KERNEL);

sizeof(*)

> +     if (!priv)
> +             return -ENOMEM;
> +
> +     priv->pinctrl = devm_pinctrl_get(host->dev);
> +     if (IS_ERR(priv->pinctrl))
> +             dev_dbg(host->dev, "no pinctrl\n");
> +
> +     priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
> +                                               PINCTRL_STATE_DEFAULT);
> +     if (IS_ERR(priv->pins_default))
> +             dev_warn(host->dev, "could not get default state\n");
> +

So this is required by the driver but not by the bindings.

> +     priv->pins_sdr50 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "sdr50");
> +     if (IS_ERR(priv->pins_sdr50))
> +             dev_warn(host->dev, "could not get sdr50 state\n");
> +
> +     priv->pins_hs200 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "hs200");
> +     if (IS_ERR(priv->pins_hs200))
> +             dev_warn(host->dev, "could not get hs200 state\n");
> +
> +     priv->pins_hs400 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "hs400");
> +     if (IS_ERR(priv->pins_hs400))
> +             dev_warn(host->dev, "could not get hs400 state\n");
> +
> +     priv->pins_tune0 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune0");
> +     if (IS_ERR(priv->pins_tune0))
> +             dev_warn(host->dev, "could not get tune0 state\n");
> +
> +     priv->pins_tune1 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune1");
> +     if (IS_ERR(priv->pins_tune1))
> +             dev_warn(host->dev, "could not get tune1 state\n");
> +
> +     priv->pins_tune2 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune2");
> +     if (IS_ERR(priv->pins_tune2))
> +             dev_warn(host->dev, "could not get tune2 state\n");
> +
> +     priv->pins_tune3 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune3");
> +     if (IS_ERR(priv->pins_tune3))
> +             dev_warn(host->dev, "could not get tune3 state\n");
> +
> +     priv->pins_tune4 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune4");
> +
> +     if (IS_ERR(priv->pins_tune4))
> +             dev_warn(host->dev, "could not get tune4 state\n");
> +
> +     priv->vp0 = devm_clk_get(host->dev, "vp0");
> +     if (IS_ERR(priv->vp0))
> +             dev_err(host->dev, "could not get vp0 clk\n");
> +
> +     priv->vp1 = devm_clk_get(host->dev, "vp1");
> +     if (IS_ERR(priv->vp1))
> +             dev_err(host->dev, "could not get vp1 clk\n");

dev_err_probe. Everywhere where applicable.

> +
> +     priv->emmc_mode = 0;
> +     prop = of_get_property(host->dev->of_node, "speed-step", &size);
> +     if (prop) {
> +             priv->emmc_mode = of_read_number(prop, 1);
> +             dev_info(host->dev, "emmc mode : %d\n", 
> + priv->emmc_mode);

Drop

> +     } else {
> +             dev_info(host->dev, "use default emmc sdr50 mode !\n");

Drop, why is this a problem?

> +     }
> +
> +     priv->is_cqe = 0;
> +     prop = of_get_property(host->dev->of_node, "cqe", &size);
> +     if (prop) {
> +             priv->is_cqe = of_read_number(prop, 1);
> +             dev_info(host->dev, "cmdq mode : %d\n", priv->is_cqe);

Drop


> +     } else {
> +             dev_info(host->dev, "use default eMMC legacy mode !\n");

Drop


> +     }
> +
> +     prop = of_get_property(host->dev->of_node, "rdq-ctrl", &size);
> +     if (prop) {
> +             priv->rdq_ctrl = of_read_number(prop, 1);
> +             dev_info(host->dev, "get rdq-ctrl : %u\n", 
> + priv->rdq_ctrl);

Drop


> +     } else {
> +             priv->rdq_ctrl = 0;
> +             dev_info(host->dev, "no dqs_dly_tape switch node, use 
> + default 0x0 !!\n");

Drop

> +     }
> +
> +     priv->m2tmx = syscon_regmap_lookup_by_phandle(host->dev->of_node, "realtek,m2tmx");
> +     if (IS_ERR_OR_NULL(priv->m2tmx))
> +             dev_err(host->dev, "can not get m2mtx node.\n");
> +
> +     host->priv = priv;
> +
> +     return 0;
> +}
> +
> +static int dw_mci_rtk_init(struct dw_mci *host) {
> +     struct dw_mci_rtkemmc_host *priv = host->priv;
> +
> +     host->pdata->caps2 = MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD;
> +
> +     if (priv->emmc_mode >= 2)
> +             host->pdata->caps2 |= MMC_CAP2_HS200_1_8V_SDR;
> +     if (priv->emmc_mode >= 3) {
> +             host->pdata->caps |= MMC_CAP_1_8V_DDR;
> +             host->pdata->caps2 |= MMC_CAP2_HS400_1_8V;
> +     }
> +
> +     if (priv->is_cqe > 0)
> +             host->pdata->caps2 |= (MMC_CAP2_CQE | 
> + MMC_CAP2_CQE_DCMD);
> +
> +     host->irq_flags = IRQF_SHARED;
> +
> +     mcq_writel(host, CP, 0x0);
> +
> +     /*Enable L4 gated*/

Read Linux coding style. Multiple times if needed.

> +     mcq_writel(host, OTHER1, mcq_readl(host, OTHER1) &
> +             ~(SDMMC_L4_GATED_DIS | SDMMC_L4_GATED_DIS1));
> +
> +     mcq_writel(host, OTHER1, mcq_readl(host, OTHER1) &
> +                (~(SDMMC_DQS_CTRL_GATE_DIS | 
> + SDMMC_DBUS_MAS_GATING_DIS)));
> +
> +     /*Set the eMMC wrapper little Endian*/
> +     mcq_writel(host, AHB, mcq_readl(host, AHB) | SDMMC_AHB_BIG);
> +
> +     mcq_writel(host, OTHER1,
> +                mcq_readl(host, OTHER1) | 
> + SDMMC_STARK_CARD_STOP_ENABLE);
> +
> +     /*set eMMC instead of nand*/
> +     regmap_update_bits_base(priv->m2tmx, SDMMC_NAND_DMA_SEL,
> +                             SDMMC_SRAM_DMA_SEL, SDMMC_SRAM_DMA_SEL, 
> + NULL, false, true);
> +
> +     /*Set the clk initial phase*/
> +     dw_mci_rtk_phase_tuning(host, 0, 0);
> +
> +     return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int dw_mci_rtk_suspend(struct device *dev) {
> +     struct dw_mci *host = dev_get_drvdata(dev);
> +     int ret = 0;
> +
> +     ret = dw_mci_cqe_runtime_suspend(dev);
> +     mcq_writel(host, AHB, 0);
> +
> +     return ret;
> +}
> +
> +static int dw_mci_rtk_resume(struct device *dev) {
> +     struct dw_mci *host = dev_get_drvdata(dev);
> +     int ret = 0;
> +
> +     mcq_writel(host, AHB, mcq_readl(host, AHB) | SDMMC_AHB_BIG);
> +     ret = dw_mci_cqe_runtime_resume(dev);
> +
> +     return ret;
> +}
> +#else
> +static int dw_mci_rtk_suspend(struct device *dev) {
> +     dev_info(dev, "User should enable CONFIG_PM kernel config\n");

NAK, come on. I asked to drop it. Did you just ignore the feedback? Yep...

> +
> +     return 0;
> +}
> +
> +static int dw_mci_rtk_resume(struct device *dev) {
> +     dev_info(dev, "User should enable CONFIG_PM kernel config\n");

NAK

> +
> +     return 0;
> +}
> +#endif /*CONFIG_PM*/
> +static const struct dev_pm_ops rtk_dev_pm_ops = {
> +     SET_SYSTEM_SLEEP_PM_OPS(dw_mci_rtk_suspend,
> +                             dw_mci_rtk_resume)
> +     SET_RUNTIME_PM_OPS(dw_mci_cqe_runtime_suspend,
> +                        dw_mci_cqe_runtime_resume,
> +                        NULL)
> +};
> +
> +static void dw_mci_rtk_shutdown(struct platform_device *pdev) {
> +     dev_info(&pdev->dev, "[eMMC] Shutdown\n");

NAK

> +     dw_mci_cqe_runtime_resume(&pdev->dev);
> +}
> +
> +static unsigned long dw_mci_rtk_dwmmc_caps[1] = {
> +     MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA |
> +     MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
> +     MMC_CAP_NONREMOVABLE | MMC_CAP_CMD23, };
> +
> +static const struct dw_mci_drv_data rtk_drv_data = {
> +     .caps                   = dw_mci_rtk_dwmmc_caps,
> +     .num_caps               = ARRAY_SIZE(dw_mci_rtk_dwmmc_caps),
> +     .set_ios                = dw_mci_rtk_set_ios,
> +     .execute_tuning         = dw_mci_rtk_execute_tuning,
> +     .parse_dt               = dw_mci_rtk_parse_dt,
> +     .init                   = dw_mci_rtk_init,
> +     .prepare_hs400_tuning   = dw_mci_rtk_prepare_hs400_tuning,
> +     .hs400_complete         = dw_mci_rtk_hs400_complete,
> +     .init_card              = dw_mci_rtk_init_card,
> +};
> +
> +static const struct of_device_id dw_mci_rtk_match[] = {
> +     { .compatible = "realtek,rtd1325-dw-cqe-emmc",
> +             .data = &rtk_drv_data },
> +     {},
> +};
> +MODULE_DEVICE_TABLE(of, dw_mci_rtk_match);
> +
> +int dw_mci_cqe_pltfm_register(struct platform_device *pdev,
> +                           const struct dw_mci_drv_data *drv_data) {
> +     struct dw_mci *host;
> +     struct resource *regs;
> +
> +     host = devm_kzalloc(&pdev->dev, sizeof(struct dw_mci), 
> + GFP_KERNEL);

sizeof(*)

> +     if (!host)
> +             return -ENOMEM;
> +
> +     host->irq = platform_get_irq(pdev, 0);
> +     if (host->irq < 0)
> +             return host->irq;
> +
> +     host->drv_data = drv_data;
> +     host->pdev = pdev;
> +     host->dev = &pdev->dev;
> +     host->irq_flags = 0;
> +     host->pdata = pdev->dev.platform_data;
> +
> +     regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +     host->regs = devm_ioremap_resource(&pdev->dev, regs);

Use helper for this.

Open existing, recent drivers and use the them as template or some set of patterns. Sorry, but upstreaming your vendor code will be painful, because you started from some old, buggier version.

> +     if (IS_ERR(host->regs))
> +             return PTR_ERR(host->regs);
> +
> +     /* Get registers' physical base address */
> +     host->phy_regs = regs->start;
> +
> +     platform_set_drvdata(pdev, host);
> +
> +     return dw_mci_cqe_probe(host);
> +}
> +
> +static int dw_mci_rtk_probe(struct platform_device *pdev) {
> +     const struct dw_mci_drv_data *drv_data;
> +     const struct of_device_id *match;
> +
> +     if (!pdev->dev.of_node)
> +             return -ENODEV;
> +
> +     match = of_match_node(dw_mci_rtk_match, pdev->dev.of_node);
> +     drv_data = match->data;
> +
> +     return dw_mci_cqe_pltfm_register(pdev, drv_data); }
> +
> +int dw_mci_rtk_remove(struct platform_device *pdev) {
> +     struct dw_mci *host = platform_get_drvdata(pdev);
> +
> +     dw_mci_cqe_remove(host);
> +     return 0;


Best regards,
Krzysztof