RE: [PATCH v7][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver

["Jyan Chou [周芷安]" <jyanchou@realtek.com>]

Hi Philipp,

>> We implemented cmdq feature on Synopsys DesignWare mmc driver.
>> The difference between dw_mmc.c and dw_mmc_cqe.c were distinct 
>> register definitions, mmc user flow and the addition of cmdq.
>>
>> New version of User Guide had modify mmc driver's usage flow, we may 
>> need to renew code to precisely follow user guide.
>>
>> More over, We added a wait status function to satisfy synopsys user 
>> guide's description, since this flow might be specific in synopsys 
>> host driver only.
>>
>> Signed-off-by: Jyan Chou <jyanchou@realtek.com>
>>
>> —--
> [...]
>> diff --git a/drivers/mmc/host/dw_mmc_cqe.c 
>> b/drivers/mmc/host/dw_mmc_cqe.c new file mode 100644 index 
>> 000000000000..eb00d6a474b2
>> --- /dev/null
>> +++ b/drivers/mmc/host/dw_mmc_cqe.c
>> @@ -0,0 +1,1467 @@
> [...]
>> +#ifdef CONFIG_OF
>> +static struct dw_mci_board *dw_mci_cqe_parse_dt(struct dw_mci *host) 
>> +{
>> +     struct dw_mci_board *pdata;
>> +     struct device *dev = host->dev;
>> +     const struct dw_mci_drv_data *drv_data = host->drv_data;
>> +     int ret;
>> +
>> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     pdata->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
>> +     if (IS_ERR(pdata->rstc)) {
>> +             if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
>> +                     return ERR_PTR(-EPROBE_DEFER);

> This should

>                return ERR_CAST(pdata->rstc);

> instead.

> There is no reason to hide device tree parsing errors here, and I'd argue pdata should not be returned with rstc set to an error value.
> devm_reset_control_get_optional_exclusive() returns NULL if there are no errors and no reset is specified in the device tree.

> Then you can just use dev_err_probe() at the call site in dw_mci_cqe_probe().

Thanks for your advice, we will correct it in our next version.

> [...]
>> +int dw_mci_cqe_probe(struct dw_mci *host) {
> [...]
>> +     if (!IS_ERR(host->pdata->rstc)) {
>> +             reset_control_assert(host->pdata->rstc);
>> +             usleep_range(10, 50);
>> +             reset_control_deassert(host->pdata->rstc);
>> +     }

> This should be changed to

>        if (host->pdata->rstc) {
>                reset_control_assert(host->pdata->rstc);
>                usleep_range(10, 50);
>                reset_control_deassert(host->pdata->rstc);
>        }

[...]
> +     return 0;
> +
> +err_dmaunmap:
> +     if (!IS_ERR(host->pdata->rstc))
> +             reset_control_assert(host->pdata->rstc);

>This should be just

>        reset_control_assert(host->pdata->rstc);

> as reset_control_assert() is a no-op if host->pdata->rstc == NULL.

> [...]
>> +void dw_mci_cqe_remove(struct dw_mci *host) {
> [...]
>> +     if (!IS_ERR(host->pdata->rstc))
>> +             reset_control_assert(host->pdata->rstc);

>        reset_control_assert(host->pdata->rstc);

Also, we will correct this in our new version, thanks.

Best Regards, 
Jyan

-----Original Message-----
From: Philipp Zabel <p.zabel@pengutronix.de> 
Sent: Monday, November 27, 2023 8:51 PM
To: Jyan Chou [周芷安] <jyanchou@realtek.com>; ulf.hansson@linaro.org; adrian.hunter@intel.com; jh80.chung@samsung.com; riteshh@codeaurora.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org
Cc: conor+dt@kernel.org; asutoshd@codeaurora.org; linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; arnd@arndb.de; briannorris@chromium.org; doug@schmorgal.com; tonyhuang.sunplus@gmail.com; abel.vesa@linaro.org; william.qiu@starfivetech.com
Subject: Re: [PATCH v7][2/4] mmc: Add Synopsys DesignWare mmc cmdq host driver


External mail.



Hi,

On Di, 2023-11-21 at 17:10 +0800, Jyan Chou wrote:
> We implemented cmdq feature on Synopsys DesignWare mmc driver.
> The difference between dw_mmc.c and dw_mmc_cqe.c were distinct 
> register definitions, mmc user flow and the addition of cmdq.
>
> New version of User Guide had modify mmc driver's usage flow, we may 
> need to renew code to precisely follow user guide.
>
> More over, We added a wait status function to satisfy synopsys user 
> guide's description, since this flow might be specific in synopsys 
> host driver only.
>
> Signed-off-by: Jyan Chou <jyanchou@realtek.com>
>
> —--
[...]
> diff --git a/drivers/mmc/host/dw_mmc_cqe.c 
> b/drivers/mmc/host/dw_mmc_cqe.c new file mode 100644 index 
> 000000000000..eb00d6a474b2
> --- /dev/null
> +++ b/drivers/mmc/host/dw_mmc_cqe.c
> @@ -0,0 +1,1467 @@
[...]
> +#ifdef CONFIG_OF
> +static struct dw_mci_board *dw_mci_cqe_parse_dt(struct dw_mci *host) 
> +{
> +     struct dw_mci_board *pdata;
> +     struct device *dev = host->dev;
> +     const struct dw_mci_drv_data *drv_data = host->drv_data;
> +     int ret;
> +
> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +     if (!pdata)
> +             return ERR_PTR(-ENOMEM);
> +
> +     pdata->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> +     if (IS_ERR(pdata->rstc)) {
> +             if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> +                     return ERR_PTR(-EPROBE_DEFER);

This should

                return ERR_CAST(pdata->rstc);

instead.

There is no reason to hide device tree parsing errors here, and I'd argue pdata should not be returned with rstc set to an error value.
devm_reset_control_get_optional_exclusive() returns NULL if there are no errors and no reset is specified in the device tree.

Then you can just use dev_err_probe() at the call site in dw_mci_cqe_probe().


[...]
> +int dw_mci_cqe_probe(struct dw_mci *host) {
[...]
> +     if (!IS_ERR(host->pdata->rstc)) {
> +             reset_control_assert(host->pdata->rstc);
> +             usleep_range(10, 50);
> +             reset_control_deassert(host->pdata->rstc);
> +     }

This should be changed to

        if (host->pdata->rstc) {
                reset_control_assert(host->pdata->rstc);
                usleep_range(10, 50);
                reset_control_deassert(host->pdata->rstc);
        }

[...]
> +     return 0;
> +
> +err_dmaunmap:
> +     if (!IS_ERR(host->pdata->rstc))
> +             reset_control_assert(host->pdata->rstc);

This should be just

        reset_control_assert(host->pdata->rstc);

as reset_control_assert() is a no-op if host->pdata->rstc == NULL.

[...]
> +void dw_mci_cqe_remove(struct dw_mci *host) {
[...]
> +     if (!IS_ERR(host->pdata->rstc))
> +             reset_control_assert(host->pdata->rstc);

        reset_control_assert(host->pdata->rstc);


regards
Philipp