Re: [PATCH v1 2/3] mmc: starfive: Add sdio/emmc driver support

[William Qiu]

On 2022/12/9 5:09, Linus Walleij wrote:
> Hi William,
> 
> thanks for your patch!
> 
> On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@xxxxxxxxxxxxxxxx> wrote:
> 
>> Add sdio/emmc driver support for StarFive JH7110 soc.
>>
>> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> 
> (...)
>> +#include <linux/gpio.h>
> 
> Never include this legacy header in new code. Also: you don't use it.
> 

Will fix.

>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
> 
> You're not using this include either.
> 

Will fix.

>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
> 
> Or this.
> 

Will fix.

>> +#define ALL_INT_CLR            0x1ffff
>> +#define MAX_DELAY_CHAIN                32
>> +
>> +struct starfive_priv {
>> +       struct device *dev;
>> +       struct regmap *reg_syscon;
>> +       u32 syscon_offset;
>> +       u32 syscon_shift;
>> +       u32 syscon_mask;
>> +};
>> +
>> +static unsigned long dw_mci_starfive_caps[] = {
>> +       MMC_CAP_CMD23,
>> +       MMC_CAP_CMD23,
>> +       MMC_CAP_CMD23
>> +};
>> +
>> +static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>> +{
>> +       int ret;
>> +       unsigned int clock;
>> +
>> +       if (ios->timing == MMC_TIMING_MMC_DDR52 || ios->timing == MMC_TIMING_UHS_DDR50) {
>> +               clock = (ios->clock > 50000000 && ios->clock <= 52000000) ? 100000000 : ios->clock;
>> +               ret = clk_set_rate(host->ciu_clk, clock);
>> +               if (ret)
>> +                       dev_dbg(host->dev, "Use an external frequency divider %uHz\n", ios->clock);
>> +               host->bus_hz = clk_get_rate(host->ciu_clk);
>> +       } else {
>> +               dev_dbg(host->dev, "Using the internal divider\n");
>> +       }
>> +}
>> +
>> +static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
>> +                                            u32 opcode)
>> +{
>> +       static const int grade  = MAX_DELAY_CHAIN;
>> +       struct dw_mci *host = slot->host;
>> +       struct starfive_priv *priv = host->priv;
>> +       int raise_point = -1, fall_point = -1;
>> +       int err, prev_err = -1;
> 
> I don't like these default-init to -1. Can you just skip it and assign it
> where it makes most sense instead?
> 

Will fix.

>> +       int found = 0;
> 
> This looks like a bool.
> 

Will update.

>> +       int i;
>> +       u32 regval;
>> +
>> +       for (i = 0; i < grade; i++) {
>> +               regval = i << priv->syscon_shift;
>> +               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
>> +                                               priv->syscon_mask, regval);
>> +               if (err)
>> +                       return err;
>> +               mci_writel(host, RINTSTS, ALL_INT_CLR);
>> +
>> +               err = mmc_send_tuning(slot->mmc, opcode, NULL);
>> +               if (!err)
>> +                       found = 1;
>> +
>> +               if (i > 0) {
>> +                       if (err && !prev_err)
>> +                               fall_point = i - 1;
>> +                       if (!err && prev_err)
>> +                               raise_point = i;
>> +               }
>> +
>> +               if (raise_point != -1 && fall_point != -1)
>> +                       goto tuning_out;
> 
> There are just these raise point (shouldn't this be "rise_point" in proper
> english?) and fall point, this misses some comments explaining what is
> going on, the code is not intuitively eviden. Rise and fall of *what* for
> example.
> 

I'll update it in next version.

>> +
>> +               prev_err = err;
>> +               err = 0;
>> +       }
>> +
>> +tuning_out:
>> +       if (found) {
>> +               if (raise_point == -1)
>> +                       raise_point = 0;
>> +               if (fall_point == -1)
>> +                       fall_point = grade - 1;
>> +               if (fall_point < raise_point) {
>> +                       if ((raise_point + fall_point) >
>> +                           (grade - 1))
>> +                               i = fall_point / 2;
>> +                       else
>> +                               i = (raise_point + grade - 1) / 2;
>> +               } else {
>> +                       i = (raise_point + fall_point) / 2;
>> +               }
> 
> Likewise here, explain what grade is, refer to the eMMC spec if necessary.
> 

Will update.

> (...)
>> +       ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
>> +                                               "starfive,sys-syscon", 3, 0, &args);
>> +       if (ret) {
>> +               dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       priv->reg_syscon = syscon_node_to_regmap(args.np);
>> +       of_node_put(args.np);
>> +       if (IS_ERR(priv->reg_syscon))
>> +               return PTR_ERR(priv->reg_syscon);
>> +
>> +       priv->syscon_offset = args.args[0];
>> +       priv->syscon_shift  = args.args[1];
>> +       priv->syscon_mask   = args.args[2];
> 
> Why should these three things be in the device tree instead of being derived
> from the compatible-string or just plain hard-coded as #defines?
> I don't get it.
> 

Will update.

>> +static int dw_mci_starfive_probe(struct platform_device *pdev)
>> +{
>> +       return dw_mci_pltfm_register(pdev, &starfive_data);
>> +}
>> +
>> +static int dw_mci_starfive_remove(struct platform_device *pdev)
>> +{
>> +       return dw_mci_pltfm_remove(pdev);
>> +}
> 
> Can't you just assign dw_mci_pltfm_remove() to .remove?
> 

Will fix.

> Other than these things, the driver looks good!
> 

Hi Linus,

Thank you for taking time to review and provide helpful comments for this patch.
I will take all of your suggestions and update this driver in next version.

Best Regards
William Qiu
> Yours,
> Linus Walleij