Message-ID: <ebbbed14-ad93-4981-96f9-8cc344b63448@linaro.org>
Date:   Mon, 30 Oct 2023 08:44:28 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Jyan Chou <jyanchou@...ltek.com>, ulf.hansson@...aro.org,
        adrian.hunter@...el.com, jh80.chung@...sung.com,
        riteshh@...eaurora.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        asutoshd@...eaurora.org, p.zabel@...gutronix.de
Cc:     linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, arnd@...db.de,
        briannorris@...omium.org, doug@...morgal.com,
        tonyhuang.sunplus@...il.com, abel.vesa@...aro.org,
        william.qiu@...rfivetech.com
Subject: Re: [PATCH V4][3/4] mmc: Add dw mobile mmc cmdq rtk driver

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

Powered by blists - more mailing lists