Hi,
This patchset adds initial rudimentary support for the StarFive
designware mobile storage host controller driver. And this driver will
be used in StarFive's visionfive-v2 board. The main purpose of adding
this driver is to accommodate the ultra-high speed mode of eMMC.
The patch series is based on v6.1-rc5.
-- William
William Qiu (3):
dt-bindings: mmc: Add bindings for StarFive
mmc: starfive: Add sdio/emmc driver support
riscv: dts: starfive: Add mmc node
.../bindings/mmc/starfive,jh7110-sdio.yaml | 71 +++++++
MAINTAINERS | 6 +
.../jh7110-starfive-visionfive-v2.dts | 25 +++
arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 ++++
drivers/mmc/host/Kconfig | 10 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/dw_mmc-starfive.c | 197 ++++++++++++++++++
7 files changed, 348 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
--
2.34.1
Add sdio/emmc driver support for StarFive JH7110 soc.
Signed-off-by: William Qiu <[email protected]>
---
MAINTAINERS | 6 +
drivers/mmc/host/Kconfig | 10 ++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/dw_mmc-starfive.c | 197 +++++++++++++++++++++++++++++
4 files changed, 214 insertions(+)
create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a70c1d0f303e..2b46ef07f5dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19623,6 +19623,12 @@ F: Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
F: drivers/reset/starfive/
F: include/dt-bindings/reset/starfive*
+STARFIVE JH7110 MMC/SD/SDIO DRIVER
+M: William Qiu <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/mmc/starfive*
+F: drivers/mmc/dw_mmc-starfive.c
+
STATIC BRANCH/CALL
M: Peter Zijlstra <[email protected]>
M: Josh Poimboeuf <[email protected]>
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index fb1062a6394c..b87262503403 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -871,6 +871,16 @@ config MMC_DW_ROCKCHIP
Synopsys DesignWare Memory Card Interface driver. Select this option
for platforms based on RK3066, RK3188 and RK3288 SoC's.
+config MMC_DW_STARFIVE
+ tristate "StarFive specific extensions for Synopsys DW Memory Card Interface"
+ depends on SOC_STARFIVE
+ depends on MMC_DW
+ select MMC_DW_PLTFM
+ help
+ This selects support for StarFive JH7110 SoC specific extensions to the
+ Synopsys DesignWare Memory Card Interface driver. Select this option
+ for platforms based on StarFive JH7110 SoC.
+
config MMC_SH_MMCIF
tristate "SuperH Internal MMCIF support"
depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 4e4ceb32c4b4..32c0e5564b9a 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_MMC_DW_HI3798CV200) += dw_mmc-hi3798cv200.o
obj-$(CONFIG_MMC_DW_K3) += dw_mmc-k3.o
obj-$(CONFIG_MMC_DW_PCI) += dw_mmc-pci.o
obj-$(CONFIG_MMC_DW_ROCKCHIP) += dw_mmc-rockchip.o
+obj-$(CONFIG_MMC_DW_STARFIVE) += dw_mmc-starfive.o
obj-$(CONFIG_MMC_SH_MMCIF) += sh_mmcif.o
obj-$(CONFIG_MMC_JZ4740) += jz4740_mmc.o
obj-$(CONFIG_MMC_VUB300) += vub300.o
diff --git a/drivers/mmc/host/dw_mmc-starfive.c b/drivers/mmc/host/dw_mmc-starfive.c
new file mode 100644
index 000000000000..b2e6a0b6abf9
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-starfive.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * StarFive Designware Mobile Storage Host Controller Driver
+ *
+ * Copyright (c) 2022 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#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>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+#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;
+ int found = 0;
+ 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;
+
+ 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;
+ }
+
+ 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);
+
+ dev_dbg(host->dev, "Found valid delay chain! use it [delay=%d]\n", i);
+ } else {
+ dev_err(host->dev, "No valid delay chain! use default\n");
+ err = -EINVAL;
+ }
+
+ mci_writel(host, RINTSTS, ALL_INT_CLR);
+ return err;
+}
+
+static int dw_mci_starfive_parse_dt(struct dw_mci *host)
+{
+ struct of_phandle_args args;
+ struct starfive_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ 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];
+
+ host->priv = priv;
+
+ return 0;
+}
+
+static const struct dw_mci_drv_data starfive_data = {
+ .caps = dw_mci_starfive_caps,
+ .num_caps = ARRAY_SIZE(dw_mci_starfive_caps),
+ .set_ios = dw_mci_starfive_set_ios,
+ .parse_dt = dw_mci_starfive_parse_dt,
+ .execute_tuning = dw_mci_starfive_execute_tuning,
+};
+
+static const struct of_device_id dw_mci_starfive_match[] = {
+ { .compatible = "starfive,jh7110-sdio",
+ .data = &starfive_data },
+ {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_starfive_match);
+
+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);
+}
+
+static struct platform_driver dw_mci_starfive_driver = {
+ .probe = dw_mci_starfive_probe,
+ .remove = dw_mci_starfive_remove,
+ .driver = {
+ .name = "dwmmc_starfive",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ .of_match_table = dw_mci_starfive_match,
+ },
+};
+module_platform_driver(dw_mci_starfive_driver);
+
+MODULE_DESCRIPTION("StarFive JH7110 Specific DW-MSHC Driver Extension");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:dwmmc_starfive");
--
2.34.1
Hi William,
thanks for your patch!
On Wed, Dec 7, 2022 at 2:17 PM William Qiu <[email protected]> wrote:
> Add sdio/emmc driver support for StarFive JH7110 soc.
>
> Signed-off-by: William Qiu <[email protected]>
(...)
> +#include <linux/gpio.h>
Never include this legacy header in new code. Also: you don't use it.
> +#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.
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
Or this.
> +#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?
> + int found = 0;
This looks like a bool.
> + 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.
> +
> + 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.
(...)
> + 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.
> +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?
Other than these things, the driver looks good!
Yours,
Linus Walleij
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 <[email protected]> wrote:
>
>> Add sdio/emmc driver support for StarFive JH7110 soc.
>>
>> Signed-off-by: William Qiu <[email protected]>
>
> (...)
>> +#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
Hi
On 2022/12/7 21:17, William Qiu wrote:
> Add sdio/emmc driver support for StarFive JH7110 soc.
>
> Signed-off-by: William Qiu <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/mmc/host/Kconfig | 10 ++
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/dw_mmc-starfive.c | 197 +++++++++++++++++++++++++++++
> 4 files changed, 214 insertions(+)
> create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
>
...
> +
> +static unsigned long dw_mci_starfive_caps[] = {
> + MMC_CAP_CMD23,
> + MMC_CAP_CMD23,
> + MMC_CAP_CMD23
> +};
> +
....
> + host->priv = priv;
> +
> + return 0;
> +}
> +
> +static const struct dw_mci_drv_data starfive_data = {
> + .caps = dw_mci_starfive_caps,
> + .num_caps = ARRAY_SIZE(dw_mci_starfive_caps),
use ".common_caps = MMC_CAP_CMD23" instead.
> + .set_ios = dw_mci_starfive_set_ios,
> + .parse_dt = dw_mci_starfive_parse_dt,
> + .execute_tuning = dw_mci_starfive_execute_tuning,
> +};
> +
> +static const struct of_device_id dw_mci_starfive_match[] = {
On 2022/12/13 10:24, Shawn Lin wrote:
> Hi
>
> On 2022/12/7 21:17, William Qiu wrote:
>> Add sdio/emmc driver support for StarFive JH7110 soc.
>>
>> Signed-off-by: William Qiu <[email protected]>
>> ---
>> MAINTAINERS | 6 +
>> drivers/mmc/host/Kconfig | 10 ++
>> drivers/mmc/host/Makefile | 1 +
>> drivers/mmc/host/dw_mmc-starfive.c | 197 +++++++++++++++++++++++++++++
>> 4 files changed, 214 insertions(+)
>> create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
>>
>
> ...
>
>> +
>> +static unsigned long dw_mci_starfive_caps[] = {
>> + MMC_CAP_CMD23,
>> + MMC_CAP_CMD23,
>> + MMC_CAP_CMD23
>> +};
>> +
>
> ....
>
>> + host->priv = priv;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dw_mci_drv_data starfive_data = {
>> + .caps = dw_mci_starfive_caps,
>> + .num_caps = ARRAY_SIZE(dw_mci_starfive_caps),
>
> use ".common_caps = MMC_CAP_CMD23" instead.
>
Hi Shawn,
Thank you for taking time to review.
The .common_caps is not defined in dw_mci_drv_data.
And .num_caps is also used in dw_mci-rockchip.c.
Best regards,
William Qiu
>> + .set_ios = dw_mci_starfive_set_ios,
>> + .parse_dt = dw_mci_starfive_parse_dt,
>> + .execute_tuning = dw_mci_starfive_execute_tuning,
>> +};
>> +
>> +static const struct of_device_id dw_mci_starfive_match[] = {
>
Hi
On 2022/12/13 15:21, William Qiu wrote:
>
>
> On 2022/12/13 10:24, Shawn Lin wrote:
>> Hi
>
>> use ".common_caps = MMC_CAP_CMD23" instead.
>>
>
> Hi Shawn,
>
> Thank you for taking time to review.
> The .common_caps is not defined in dw_mci_drv_data.
> And .num_caps is also used in dw_mci-rockchip.c.
>
That means your patch is based on old upstream kernel which hasn't
been updated for at least one year.:)
> Best regards,
> William Qiu
>>> + .set_ios = dw_mci_starfive_set_ios,
>>> + .parse_dt = dw_mci_starfive_parse_dt,
>>> + .execute_tuning = dw_mci_starfive_execute_tuning,
>>> +};
>>> +
>>> +static const struct of_device_id dw_mci_starfive_match[] = {
>>
On 2022/12/13 15:33, Shawn Lin wrote:
> Hi
>
> On 2022/12/13 15:21, William Qiu wrote:
>>
>>
>> On 2022/12/13 10:24, Shawn Lin wrote:
>>> Hi
>>
>>> use ".common_caps = MMC_CAP_CMD23" instead.
>>>
>>
>> Hi Shawn,
>>
>> Thank you for taking time to review.
>> The .common_caps is not defined in dw_mci_drv_data.
>> And .num_caps is also used in dw_mci-rockchip.c.
>>
>
> That means your patch is based on old upstream kernel which hasn't
> been updated for at least one year.:)
>
Sorry about that. I just check the code on 5.15.
I will update it in next version.
Thank you any way.
>> Best regards,
>> William Qiu
>>>> + .set_ios = dw_mci_starfive_set_ios,
>>>> + .parse_dt = dw_mci_starfive_parse_dt,
>>>> + .execute_tuning = dw_mci_starfive_execute_tuning,
>>>> +};
>>>> +
>>>> +static const struct of_device_id dw_mci_starfive_match[] = {
>>>
On 2022/12/7 21:17, William Qiu wrote:
> Hi,
>
> This patchset adds initial rudimentary support for the StarFive
> designware mobile storage host controller driver. And this driver will
> be used in StarFive's visionfive-v2 board. The main purpose of adding
> this driver is to accommodate the ultra-high speed mode of eMMC.
>
> The patch series is based on v6.1-rc5.
>
> -- William
>
> William Qiu (3):
> dt-bindings: mmc: Add bindings for StarFive
> mmc: starfive: Add sdio/emmc driver support
> riscv: dts: starfive: Add mmc node
>
> .../bindings/mmc/starfive,jh7110-sdio.yaml | 71 +++++++
> MAINTAINERS | 6 +
> .../jh7110-starfive-visionfive-v2.dts | 25 +++
> arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 ++++
> drivers/mmc/host/Kconfig | 10 +
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/dw_mmc-starfive.c | 197 ++++++++++++++++++
> 7 files changed, 348 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
>
> --
> 2.34.1
>
Hi Jaehoon/Ulf,
Could you please help to review and provide comments on this patch series?
Thank you in advance.
Best regards,
William Qiu
On Fri, 16 Dec 2022 at 03:02, William Qiu <[email protected]> wrote:
>
>
>
> On 2022/12/7 21:17, William Qiu wrote:
> > Hi,
> >
> > This patchset adds initial rudimentary support for the StarFive
> > designware mobile storage host controller driver. And this driver will
> > be used in StarFive's visionfive-v2 board. The main purpose of adding
> > this driver is to accommodate the ultra-high speed mode of eMMC.
> >
> > The patch series is based on v6.1-rc5.
> >
> > -- William
> >
> > William Qiu (3):
> > dt-bindings: mmc: Add bindings for StarFive
> > mmc: starfive: Add sdio/emmc driver support
> > riscv: dts: starfive: Add mmc node
> >
> > .../bindings/mmc/starfive,jh7110-sdio.yaml | 71 +++++++
> > MAINTAINERS | 6 +
> > .../jh7110-starfive-visionfive-v2.dts | 25 +++
> > arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 ++++
> > drivers/mmc/host/Kconfig | 10 +
> > drivers/mmc/host/Makefile | 1 +
> > drivers/mmc/host/dw_mmc-starfive.c | 197 ++++++++++++++++++
> > 7 files changed, 348 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> > create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
> >
> > --
> > 2.34.1
> >
>
> Hi Jaehoon/Ulf,
>
> Could you please help to review and provide comments on this patch series?
> Thank you in advance.
Hi William,
Looks like you have received plenty of good comments already and there
are a lot of things for you to update. That said, I think it makes
better sense for me to look at the next version instead.
>
> Best regards,
> William Qiu
Kind regards
Uffe
On 2022/12/16 17:22, Ulf Hansson wrote:
> On Fri, 16 Dec 2022 at 03:02, William Qiu <[email protected]> wrote:
>>
>>
>>
>> On 2022/12/7 21:17, William Qiu wrote:
>> > Hi,
>> >
>> > This patchset adds initial rudimentary support for the StarFive
>> > designware mobile storage host controller driver. And this driver will
>> > be used in StarFive's visionfive-v2 board. The main purpose of adding
>> > this driver is to accommodate the ultra-high speed mode of eMMC.
>> >
>> > The patch series is based on v6.1-rc5.
>> >
>> > -- William
>> >
>> > William Qiu (3):
>> > dt-bindings: mmc: Add bindings for StarFive
>> > mmc: starfive: Add sdio/emmc driver support
>> > riscv: dts: starfive: Add mmc node
>> >
>> > .../bindings/mmc/starfive,jh7110-sdio.yaml | 71 +++++++
>> > MAINTAINERS | 6 +
>> > .../jh7110-starfive-visionfive-v2.dts | 25 +++
>> > arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 ++++
>> > drivers/mmc/host/Kconfig | 10 +
>> > drivers/mmc/host/Makefile | 1 +
>> > drivers/mmc/host/dw_mmc-starfive.c | 197 ++++++++++++++++++
>> > 7 files changed, 348 insertions(+)
>> > create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
>> > create mode 100644 drivers/mmc/host/dw_mmc-starfive.c
>> >
>> > --
>> > 2.34.1
>> >
>>
>> Hi Jaehoon/Ulf,
>>
>> Could you please help to review and provide comments on this patch series?
>> Thank you in advance.
>
> Hi William,
>
> Looks like you have received plenty of good comments already and there
> are a lot of things for you to update. That said, I think it makes
> better sense for me to look at the next version instead.
>
Fine, I'll do that then.
Thanks
William Qiu
>>
>> Best regards,
>> William Qiu
>
> Kind regards
> Uffe
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 <[email protected]> wrote:
>
>> Add sdio/emmc driver support for StarFive JH7110 soc.
>>
>> Signed-off-by: William Qiu <[email protected]>
>
> (...)
>> +#include <linux/gpio.h>
>
> Never include this legacy header in new code. Also: you don't use it.
>
>> +#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.
>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>
> Or this.
>
>> +#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?
>
>> + int found = 0;
>
> This looks like a bool.
>
>> + 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.
>
>> +
>> + 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.
>
> (...)
>> + 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.
>
Hi Linus,
I'm sorry to bother you, but as for the definition of syscon, after discussing with
my colleagues, we think it is easier to distinguish SDIO0 and SDIO1 by defining it in
the device tree, and the code compatibility is better.
Best Regards
William Qiu
>> +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?
>
> Other than these things, the driver looks good!
>
> Yours,
> Linus Walleij
On Thu, Feb 2, 2023 at 12:10 PM William Qiu
<[email protected]> wrote:
> On 2022/12/9 5:09, Linus Walleij wrote:
> >> + 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.
> >
> Hi Linus,
>
> I'm sorry to bother you, but as for the definition of syscon, after discussing with
> my colleagues, we think it is easier to distinguish SDIO0 and SDIO1 by defining it in
> the device tree, and the code compatibility is better.
OK sounds good looking forward to seeing the result :)
Yours,
Linus Walleij