* [PATCH v2 0/2] Add support for Allwinner A64 mmc controller
@ 2016-07-31 11:02 ` Icenowy Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 11:02 UTC (permalink / raw)
To: Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson, Hans de Goede
Cc: Mark Rutland, devicetree, Michal Suchanek, linux-mmc,
linux-kernel, Jaehoon Chung, linux-arm-kernel
Allwinner A64 has a slightly modified version of the Allwinner MMC controller.
It do not have "output" or "sample" clock input, instead, it can calibrate the
delay internally.
This series of patch added the calibrate feature to the sunxi-mmc driver. It
is based on works by Andre Przywara <andre.przywara@arm.com>, and now it
depends on the patch set sent recently by Hans de Geode <hdegoede@redhat.com>,
which disabled "output" and "sample" clock on A10/13.
( http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/446187.html )
As the clock and device tree patch for Allwinner A64 is not yet merged, the
patch set contains no patch to enable it.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 0/2] Add support for Allwinner A64 mmc controller
@ 2016-07-31 11:02 ` Icenowy Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 11:02 UTC (permalink / raw)
To: linux-arm-kernel
Allwinner A64 has a slightly modified version of the Allwinner MMC controller.
It do not have "output" or "sample" clock input, instead, it can calibrate the
delay internally.
This series of patch added the calibrate feature to the sunxi-mmc driver. It
is based on works by Andre Przywara <andre.przywara@arm.com>, and now it
depends on the patch set sent recently by Hans de Geode <hdegoede@redhat.com>,
which disabled "output" and "sample" clock on A10/13.
( http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/446187.html )
As the clock and device tree patch for Allwinner A64 is not yet merged, the
patch set contains no patch to enable it.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] Documentation: dt: Add new compatible to sunxi mmc driver bindings
2016-07-31 11:02 ` Icenowy Zheng
@ 2016-07-31 11:02 ` Icenowy Zheng
-1 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 11:02 UTC (permalink / raw)
To: Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson, Hans de Goede
Cc: Mark Rutland, devicetree, Michal Suchanek, linux-mmc,
linux-kernel, Jaehoon Chung, Icenowy Zheng, linux-arm-kernel
Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
index 904ff9f..55cdd80 100644
--- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
@@ -13,6 +13,7 @@ Required properties:
* "allwinner,sun5i-a13-mmc"
* "allwinner,sun7i-a20-mmc"
* "allwinner,sun9i-a80-mmc"
+ * "allwinner,sun50i-a64-mmc"
- reg : mmc controller base registers
- clocks : a list with 4 phandle + clock specifier pairs
- clock-names : must contain "ahb", "mmc", "output" and "sample"
--
2.9.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] Documentation: dt: Add new compatible to sunxi mmc driver bindings
@ 2016-07-31 11:02 ` Icenowy Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 11:02 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
index 904ff9f..55cdd80 100644
--- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
@@ -13,6 +13,7 @@ Required properties:
* "allwinner,sun5i-a13-mmc"
* "allwinner,sun7i-a20-mmc"
* "allwinner,sun9i-a80-mmc"
+ * "allwinner,sun50i-a64-mmc"
- reg : mmc controller base registers
- clocks : a list with 4 phandle + clock specifier pairs
- clock-names : must contain "ahb", "mmc", "output" and "sample"
--
2.9.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
2016-07-31 11:02 ` Icenowy Zheng
@ 2016-07-31 11:02 ` Icenowy Zheng
-1 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 11:02 UTC (permalink / raw)
To: Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson, Hans de Goede
Cc: Mark Rutland, devicetree, Michal Suchanek, linux-mmc,
linux-kernel, Jaehoon Chung, Icenowy Zheng, linux-arm-kernel
A64 SoC features a MMC controller which need only the mod clock, and can
calibrate delay by itself. This patch adds support for the new MMC
controller IP core.
Based on work by Andre Przywara <andre.przywara@arm.com>.
Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 90 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 2ec91ce..aa2abf3 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -72,6 +72,14 @@
#define SDXC_REG_CHDA (0x90)
#define SDXC_REG_CBDA (0x94)
+/* New registers introduced in A64 */
+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
+#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
+
+
#define mmc_readl(host, reg) \
readl((host)->reg_base + SDXC_##reg)
#define mmc_writel(host, reg, value) \
@@ -217,6 +225,15 @@
#define SDXC_CLK_50M_DDR 3
#define SDXC_CLK_50M_DDR_8BIT 4
+#define SDXC_2X_TIMING_MODE BIT(31)
+
+#define SDXC_CAL_START BIT(15)
+#define SDXC_CAL_DONE BIT(14)
+#define SDXC_CAL_DL_SHIFT 8
+#define SDXC_CAL_DL_SW_EN BIT(7)
+#define SDXC_CAL_DL_SW_SHIFT 0
+#define SDXC_CAL_DL_MASK 0x3f
+
struct sunxi_mmc_clk_delay {
u32 output;
u32 sample;
@@ -232,6 +249,9 @@ struct sunxi_idma_des {
struct sunxi_mmc_cfg {
u32 idma_des_size_bits;
const struct sunxi_mmc_clk_delay *clk_delays;
+
+ /* does the IP block support autocalibration? */
+ bool can_calibrate;
};
struct sunxi_mmc_host {
@@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
return 0;
}
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int reg_off)
+{
+ u32 reg = readl(host->reg_base + reg_off);
+ u32 delay;
+
+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
+ reg &= ~SDXC_CAL_DL_SW_EN;
+
+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
+
+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
+ cpu_relax();
+
+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+
+ reg &= ~SDXC_CAL_START;
+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
+
+ writel(reg, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
+
+ return 0;
+}
+
static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
struct mmc_ios *ios, u32 rate)
{
@@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
}
mmc_writel(host, REG_CLKCR, rval);
- ret = sunxi_mmc_clk_set_phase(host, ios, rate);
- if (ret)
- return ret;
+ if (host->cfg->can_calibrate) {
+ ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
+ if (ret)
+ return ret;
+
+ /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
+ } else {
+ ret = sunxi_mmc_clk_set_phase(host, ios, rate);
+ if (ret)
+ return ret;
+ }
return sunxi_mmc_oclk_onoff(host, 1);
}
@@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
.idma_des_size_bits = 13,
.clk_delays = NULL,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
.idma_des_size_bits = 16,
.clk_delays = NULL,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
.idma_des_size_bits = 16,
.clk_delays = sunxi_mmc_clk_delays,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
.idma_des_size_bits = 16,
.clk_delays = sun9i_mmc_clk_delays,
+ .can_calibrate = false,
+};
+
+static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
+ .idma_des_size_bits = 16,
+ .clk_delays = NULL,
+ .can_calibrate = true,
};
static const struct of_device_id sunxi_mmc_of_match[] = {
@@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
{ .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
+ { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
@@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
goto error_disable_clk_ahb;
}
- ret = clk_prepare_enable(host->clk_output);
- if (ret) {
- dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
- goto error_disable_clk_mmc;
- }
+ if (host->cfg->clk_delays) {
+ ret = clk_prepare_enable(host->clk_output);
+ if (ret) {
+ dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
+ goto error_disable_clk_mmc;
+ }
- ret = clk_prepare_enable(host->clk_sample);
- if (ret) {
- dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
- goto error_disable_clk_output;
+ ret = clk_prepare_enable(host->clk_sample);
+ if (ret) {
+ dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
+ goto error_disable_clk_output;
+ }
}
if (!IS_ERR(host->reset)) {
@@ -1107,9 +1176,11 @@ error_assert_reset:
if (!IS_ERR(host->reset))
reset_control_assert(host->reset);
error_disable_clk_sample:
- clk_disable_unprepare(host->clk_sample);
+ if (host->cfg->clk_delays)
+ clk_disable_unprepare(host->clk_sample);
error_disable_clk_output:
- clk_disable_unprepare(host->clk_output);
+ if (host->cfg->clk_delays)
+ clk_disable_unprepare(host->clk_output);
error_disable_clk_mmc:
clk_disable_unprepare(host->clk_mmc);
error_disable_clk_ahb:
@@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
if (!IS_ERR(host->reset))
reset_control_assert(host->reset);
- clk_disable_unprepare(host->clk_sample);
- clk_disable_unprepare(host->clk_output);
+ if (host->cfg->clk_delays) {
+ clk_disable_unprepare(host->clk_sample);
+ clk_disable_unprepare(host->clk_output);
+ }
+
clk_disable_unprepare(host->clk_mmc);
clk_disable_unprepare(host->clk_ahb);
--
2.9.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
@ 2016-07-31 11:02 ` Icenowy Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 11:02 UTC (permalink / raw)
To: linux-arm-kernel
A64 SoC features a MMC controller which need only the mod clock, and can
calibrate delay by itself. This patch adds support for the new MMC
controller IP core.
Based on work by Andre Przywara <andre.przywara@arm.com>.
Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 90 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 2ec91ce..aa2abf3 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -72,6 +72,14 @@
#define SDXC_REG_CHDA (0x90)
#define SDXC_REG_CBDA (0x94)
+/* New registers introduced in A64 */
+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
+#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
+
+
#define mmc_readl(host, reg) \
readl((host)->reg_base + SDXC_##reg)
#define mmc_writel(host, reg, value) \
@@ -217,6 +225,15 @@
#define SDXC_CLK_50M_DDR 3
#define SDXC_CLK_50M_DDR_8BIT 4
+#define SDXC_2X_TIMING_MODE BIT(31)
+
+#define SDXC_CAL_START BIT(15)
+#define SDXC_CAL_DONE BIT(14)
+#define SDXC_CAL_DL_SHIFT 8
+#define SDXC_CAL_DL_SW_EN BIT(7)
+#define SDXC_CAL_DL_SW_SHIFT 0
+#define SDXC_CAL_DL_MASK 0x3f
+
struct sunxi_mmc_clk_delay {
u32 output;
u32 sample;
@@ -232,6 +249,9 @@ struct sunxi_idma_des {
struct sunxi_mmc_cfg {
u32 idma_des_size_bits;
const struct sunxi_mmc_clk_delay *clk_delays;
+
+ /* does the IP block support autocalibration? */
+ bool can_calibrate;
};
struct sunxi_mmc_host {
@@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
return 0;
}
+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
+ struct mmc_ios *ios, int reg_off)
+{
+ u32 reg = readl(host->reg_base + reg_off);
+ u32 delay;
+
+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
+ reg &= ~SDXC_CAL_DL_SW_EN;
+
+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
+
+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
+ cpu_relax();
+
+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
+
+ reg &= ~SDXC_CAL_START;
+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
+
+ writel(reg, host->reg_base + reg_off);
+
+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
+
+ return 0;
+}
+
static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
struct mmc_ios *ios, u32 rate)
{
@@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
}
mmc_writel(host, REG_CLKCR, rval);
- ret = sunxi_mmc_clk_set_phase(host, ios, rate);
- if (ret)
- return ret;
+ if (host->cfg->can_calibrate) {
+ ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
+ if (ret)
+ return ret;
+
+ /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
+ } else {
+ ret = sunxi_mmc_clk_set_phase(host, ios, rate);
+ if (ret)
+ return ret;
+ }
return sunxi_mmc_oclk_onoff(host, 1);
}
@@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
.idma_des_size_bits = 13,
.clk_delays = NULL,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
.idma_des_size_bits = 16,
.clk_delays = NULL,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
.idma_des_size_bits = 16,
.clk_delays = sunxi_mmc_clk_delays,
+ .can_calibrate = false,
};
static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
.idma_des_size_bits = 16,
.clk_delays = sun9i_mmc_clk_delays,
+ .can_calibrate = false,
+};
+
+static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
+ .idma_des_size_bits = 16,
+ .clk_delays = NULL,
+ .can_calibrate = true,
};
static const struct of_device_id sunxi_mmc_of_match[] = {
@@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
{ .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
+ { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
@@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
goto error_disable_clk_ahb;
}
- ret = clk_prepare_enable(host->clk_output);
- if (ret) {
- dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
- goto error_disable_clk_mmc;
- }
+ if (host->cfg->clk_delays) {
+ ret = clk_prepare_enable(host->clk_output);
+ if (ret) {
+ dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
+ goto error_disable_clk_mmc;
+ }
- ret = clk_prepare_enable(host->clk_sample);
- if (ret) {
- dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
- goto error_disable_clk_output;
+ ret = clk_prepare_enable(host->clk_sample);
+ if (ret) {
+ dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
+ goto error_disable_clk_output;
+ }
}
if (!IS_ERR(host->reset)) {
@@ -1107,9 +1176,11 @@ error_assert_reset:
if (!IS_ERR(host->reset))
reset_control_assert(host->reset);
error_disable_clk_sample:
- clk_disable_unprepare(host->clk_sample);
+ if (host->cfg->clk_delays)
+ clk_disable_unprepare(host->clk_sample);
error_disable_clk_output:
- clk_disable_unprepare(host->clk_output);
+ if (host->cfg->clk_delays)
+ clk_disable_unprepare(host->clk_output);
error_disable_clk_mmc:
clk_disable_unprepare(host->clk_mmc);
error_disable_clk_ahb:
@@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
if (!IS_ERR(host->reset))
reset_control_assert(host->reset);
- clk_disable_unprepare(host->clk_sample);
- clk_disable_unprepare(host->clk_output);
+ if (host->cfg->clk_delays) {
+ clk_disable_unprepare(host->clk_sample);
+ clk_disable_unprepare(host->clk_output);
+ }
+
clk_disable_unprepare(host->clk_mmc);
clk_disable_unprepare(host->clk_ahb);
--
2.9.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
2016-07-31 11:02 ` Icenowy Zheng
(?)
@ 2016-07-31 14:30 ` Hans de Goede
-1 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2016-07-31 14:30 UTC (permalink / raw)
To: Icenowy Zheng, Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson
Cc: Mark Rutland, Jaehoon Chung, Michal Suchanek, devicetree,
linux-arm-kernel, linux-kernel, linux-mmc
Hi,
On 31-07-16 13:02, Icenowy Zheng wrote:
> A64 SoC features a MMC controller which need only the mod clock, and can
> calibrate delay by itself. This patch adds support for the new MMC
> controller IP core.
>
> Based on work by Andre Przywara <andre.przywara@arm.com>.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
Looks good, some minor remarks (see comments inline) after those
are addressed this is ready to be merged IMHO.
> ---
> drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 90 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 2ec91ce..aa2abf3 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -72,6 +72,14 @@
> #define SDXC_REG_CHDA (0x90)
> #define SDXC_REG_CBDA (0x94)
>
> +/* New registers introduced in A64 */
> +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
> +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
> +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
> +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
> +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
> +
> +
Please drop 1 empty line here.
> #define mmc_readl(host, reg) \
> readl((host)->reg_base + SDXC_##reg)
> #define mmc_writel(host, reg, value) \
> @@ -217,6 +225,15 @@
> #define SDXC_CLK_50M_DDR 3
> #define SDXC_CLK_50M_DDR_8BIT 4
>
> +#define SDXC_2X_TIMING_MODE BIT(31)
> +
> +#define SDXC_CAL_START BIT(15)
> +#define SDXC_CAL_DONE BIT(14)
> +#define SDXC_CAL_DL_SHIFT 8
> +#define SDXC_CAL_DL_SW_EN BIT(7)
> +#define SDXC_CAL_DL_SW_SHIFT 0
> +#define SDXC_CAL_DL_MASK 0x3f
> +
> struct sunxi_mmc_clk_delay {
> u32 output;
> u32 sample;
> @@ -232,6 +249,9 @@ struct sunxi_idma_des {
> struct sunxi_mmc_cfg {
> u32 idma_des_size_bits;
> const struct sunxi_mmc_clk_delay *clk_delays;
> +
I would not insert an empty line here.
> + /* does the IP block support autocalibration? */
> + bool can_calibrate;
> };
>
> struct sunxi_mmc_host {
> @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
> return 0;
> }
>
> +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
> + struct mmc_ios *ios, int reg_off)
> +{
> + u32 reg = readl(host->reg_base + reg_off);
> + u32 delay;
> +
I would add:
if (!host->cfg->can_calibrate)
return 0;
Here; and ...
> + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
> + reg &= ~SDXC_CAL_DL_SW_EN;
> +
> + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
> +
> + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
> +
> + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
> + cpu_relax();
> +
> + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
> +
> + reg &= ~SDXC_CAL_START;
> + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
> +
> + writel(reg, host->reg_base + reg_off);
> +
> + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
Add:
/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
here; and ...
> +
> + return 0;
> +}
> +
> static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
> struct mmc_ios *ios, u32 rate)
> {
> @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
> }
> mmc_writel(host, REG_CLKCR, rval);
>
> - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
> - if (ret)
> - return ret;
Keep this as is; and add:
ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
if (ret)
return ret;
Instead of:
> + if (host->cfg->can_calibrate) {
> + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
> + if (ret)
> + return ret;
> +
> + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
> + } else {
> + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
> + if (ret)
> + return ret;
> + }
>
> return sunxi_mmc_oclk_onoff(host, 1);
> }
> @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
> static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
> .idma_des_size_bits = 13,
> .clk_delays = NULL,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = NULL,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = sunxi_mmc_clk_delays,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = sun9i_mmc_clk_delays,
> + .can_calibrate = false,
> +};
> +
> +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
> + .idma_des_size_bits = 16,
> + .clk_delays = NULL,
> + .can_calibrate = true,
> };
>
> static const struct of_device_id sunxi_mmc_of_match[] = {
> @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
> { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
> { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
> { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
> + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
Not sure why all the below hunks are here, they certainly do not belong
in this patch; and they are not necessary. As I mentioned in the
commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
"Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
calls to the sample clks as-is, without adding checks for them being
NULL. All the clk_foo calls accept a NULL clk and will return success when
called with a NULL clk."
So please drop these.
Thanks & Regards,
Hans
> @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> goto error_disable_clk_ahb;
> }
>
> - ret = clk_prepare_enable(host->clk_output);
> - if (ret) {
> - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
> - goto error_disable_clk_mmc;
> - }
> + if (host->cfg->clk_delays) {
> + ret = clk_prepare_enable(host->clk_output);
> + if (ret) {
> + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
> + goto error_disable_clk_mmc;
> + }
>
> - ret = clk_prepare_enable(host->clk_sample);
> - if (ret) {
> - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
> - goto error_disable_clk_output;
> + ret = clk_prepare_enable(host->clk_sample);
> + if (ret) {
> + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
> + goto error_disable_clk_output;
> + }
> }
>
> if (!IS_ERR(host->reset)) {
> @@ -1107,9 +1176,11 @@ error_assert_reset:
> if (!IS_ERR(host->reset))
> reset_control_assert(host->reset);
> error_disable_clk_sample:
> - clk_disable_unprepare(host->clk_sample);
> + if (host->cfg->clk_delays)
> + clk_disable_unprepare(host->clk_sample);
> error_disable_clk_output:
> - clk_disable_unprepare(host->clk_output);
> + if (host->cfg->clk_delays)
> + clk_disable_unprepare(host->clk_output);
> error_disable_clk_mmc:
> clk_disable_unprepare(host->clk_mmc);
> error_disable_clk_ahb:
> @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
> if (!IS_ERR(host->reset))
> reset_control_assert(host->reset);
>
> - clk_disable_unprepare(host->clk_sample);
> - clk_disable_unprepare(host->clk_output);
> + if (host->cfg->clk_delays) {
> + clk_disable_unprepare(host->clk_sample);
> + clk_disable_unprepare(host->clk_output);
> + }
> +
> clk_disable_unprepare(host->clk_mmc);
> clk_disable_unprepare(host->clk_ahb);
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
@ 2016-07-31 14:30 ` Hans de Goede
0 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2016-07-31 14:30 UTC (permalink / raw)
To: Icenowy Zheng, Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson
Cc: Mark Rutland, devicetree, Michal Suchanek, linux-mmc,
linux-kernel, Jaehoon Chung, linux-arm-kernel
Hi,
On 31-07-16 13:02, Icenowy Zheng wrote:
> A64 SoC features a MMC controller which need only the mod clock, and can
> calibrate delay by itself. This patch adds support for the new MMC
> controller IP core.
>
> Based on work by Andre Przywara <andre.przywara@arm.com>.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
Looks good, some minor remarks (see comments inline) after those
are addressed this is ready to be merged IMHO.
> ---
> drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 90 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 2ec91ce..aa2abf3 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -72,6 +72,14 @@
> #define SDXC_REG_CHDA (0x90)
> #define SDXC_REG_CBDA (0x94)
>
> +/* New registers introduced in A64 */
> +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
> +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
> +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
> +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
> +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
> +
> +
Please drop 1 empty line here.
> #define mmc_readl(host, reg) \
> readl((host)->reg_base + SDXC_##reg)
> #define mmc_writel(host, reg, value) \
> @@ -217,6 +225,15 @@
> #define SDXC_CLK_50M_DDR 3
> #define SDXC_CLK_50M_DDR_8BIT 4
>
> +#define SDXC_2X_TIMING_MODE BIT(31)
> +
> +#define SDXC_CAL_START BIT(15)
> +#define SDXC_CAL_DONE BIT(14)
> +#define SDXC_CAL_DL_SHIFT 8
> +#define SDXC_CAL_DL_SW_EN BIT(7)
> +#define SDXC_CAL_DL_SW_SHIFT 0
> +#define SDXC_CAL_DL_MASK 0x3f
> +
> struct sunxi_mmc_clk_delay {
> u32 output;
> u32 sample;
> @@ -232,6 +249,9 @@ struct sunxi_idma_des {
> struct sunxi_mmc_cfg {
> u32 idma_des_size_bits;
> const struct sunxi_mmc_clk_delay *clk_delays;
> +
I would not insert an empty line here.
> + /* does the IP block support autocalibration? */
> + bool can_calibrate;
> };
>
> struct sunxi_mmc_host {
> @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
> return 0;
> }
>
> +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
> + struct mmc_ios *ios, int reg_off)
> +{
> + u32 reg = readl(host->reg_base + reg_off);
> + u32 delay;
> +
I would add:
if (!host->cfg->can_calibrate)
return 0;
Here; and ...
> + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
> + reg &= ~SDXC_CAL_DL_SW_EN;
> +
> + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
> +
> + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
> +
> + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
> + cpu_relax();
> +
> + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
> +
> + reg &= ~SDXC_CAL_START;
> + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
> +
> + writel(reg, host->reg_base + reg_off);
> +
> + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
Add:
/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
here; and ...
> +
> + return 0;
> +}
> +
> static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
> struct mmc_ios *ios, u32 rate)
> {
> @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
> }
> mmc_writel(host, REG_CLKCR, rval);
>
> - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
> - if (ret)
> - return ret;
Keep this as is; and add:
ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
if (ret)
return ret;
Instead of:
> + if (host->cfg->can_calibrate) {
> + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
> + if (ret)
> + return ret;
> +
> + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
> + } else {
> + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
> + if (ret)
> + return ret;
> + }
>
> return sunxi_mmc_oclk_onoff(host, 1);
> }
> @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
> static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
> .idma_des_size_bits = 13,
> .clk_delays = NULL,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = NULL,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = sunxi_mmc_clk_delays,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = sun9i_mmc_clk_delays,
> + .can_calibrate = false,
> +};
> +
> +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
> + .idma_des_size_bits = 16,
> + .clk_delays = NULL,
> + .can_calibrate = true,
> };
>
> static const struct of_device_id sunxi_mmc_of_match[] = {
> @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
> { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
> { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
> { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
> + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
Not sure why all the below hunks are here, they certainly do not belong
in this patch; and they are not necessary. As I mentioned in the
commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
"Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
calls to the sample clks as-is, without adding checks for them being
NULL. All the clk_foo calls accept a NULL clk and will return success when
called with a NULL clk."
So please drop these.
Thanks & Regards,
Hans
> @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> goto error_disable_clk_ahb;
> }
>
> - ret = clk_prepare_enable(host->clk_output);
> - if (ret) {
> - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
> - goto error_disable_clk_mmc;
> - }
> + if (host->cfg->clk_delays) {
> + ret = clk_prepare_enable(host->clk_output);
> + if (ret) {
> + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
> + goto error_disable_clk_mmc;
> + }
>
> - ret = clk_prepare_enable(host->clk_sample);
> - if (ret) {
> - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
> - goto error_disable_clk_output;
> + ret = clk_prepare_enable(host->clk_sample);
> + if (ret) {
> + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
> + goto error_disable_clk_output;
> + }
> }
>
> if (!IS_ERR(host->reset)) {
> @@ -1107,9 +1176,11 @@ error_assert_reset:
> if (!IS_ERR(host->reset))
> reset_control_assert(host->reset);
> error_disable_clk_sample:
> - clk_disable_unprepare(host->clk_sample);
> + if (host->cfg->clk_delays)
> + clk_disable_unprepare(host->clk_sample);
> error_disable_clk_output:
> - clk_disable_unprepare(host->clk_output);
> + if (host->cfg->clk_delays)
> + clk_disable_unprepare(host->clk_output);
> error_disable_clk_mmc:
> clk_disable_unprepare(host->clk_mmc);
> error_disable_clk_ahb:
> @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
> if (!IS_ERR(host->reset))
> reset_control_assert(host->reset);
>
> - clk_disable_unprepare(host->clk_sample);
> - clk_disable_unprepare(host->clk_output);
> + if (host->cfg->clk_delays) {
> + clk_disable_unprepare(host->clk_sample);
> + clk_disable_unprepare(host->clk_output);
> + }
> +
> clk_disable_unprepare(host->clk_mmc);
> clk_disable_unprepare(host->clk_ahb);
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
@ 2016-07-31 14:30 ` Hans de Goede
0 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2016-07-31 14:30 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 31-07-16 13:02, Icenowy Zheng wrote:
> A64 SoC features a MMC controller which need only the mod clock, and can
> calibrate delay by itself. This patch adds support for the new MMC
> controller IP core.
>
> Based on work by Andre Przywara <andre.przywara@arm.com>.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
Looks good, some minor remarks (see comments inline) after those
are addressed this is ready to be merged IMHO.
> ---
> drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 90 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 2ec91ce..aa2abf3 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -72,6 +72,14 @@
> #define SDXC_REG_CHDA (0x90)
> #define SDXC_REG_CBDA (0x94)
>
> +/* New registers introduced in A64 */
> +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
> +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
> +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
> +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
> +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
> +
> +
Please drop 1 empty line here.
> #define mmc_readl(host, reg) \
> readl((host)->reg_base + SDXC_##reg)
> #define mmc_writel(host, reg, value) \
> @@ -217,6 +225,15 @@
> #define SDXC_CLK_50M_DDR 3
> #define SDXC_CLK_50M_DDR_8BIT 4
>
> +#define SDXC_2X_TIMING_MODE BIT(31)
> +
> +#define SDXC_CAL_START BIT(15)
> +#define SDXC_CAL_DONE BIT(14)
> +#define SDXC_CAL_DL_SHIFT 8
> +#define SDXC_CAL_DL_SW_EN BIT(7)
> +#define SDXC_CAL_DL_SW_SHIFT 0
> +#define SDXC_CAL_DL_MASK 0x3f
> +
> struct sunxi_mmc_clk_delay {
> u32 output;
> u32 sample;
> @@ -232,6 +249,9 @@ struct sunxi_idma_des {
> struct sunxi_mmc_cfg {
> u32 idma_des_size_bits;
> const struct sunxi_mmc_clk_delay *clk_delays;
> +
I would not insert an empty line here.
> + /* does the IP block support autocalibration? */
> + bool can_calibrate;
> };
>
> struct sunxi_mmc_host {
> @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
> return 0;
> }
>
> +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
> + struct mmc_ios *ios, int reg_off)
> +{
> + u32 reg = readl(host->reg_base + reg_off);
> + u32 delay;
> +
I would add:
if (!host->cfg->can_calibrate)
return 0;
Here; and ...
> + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
> + reg &= ~SDXC_CAL_DL_SW_EN;
> +
> + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
> +
> + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
> +
> + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
> + cpu_relax();
> +
> + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
> +
> + reg &= ~SDXC_CAL_START;
> + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
> +
> + writel(reg, host->reg_base + reg_off);
> +
> + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
Add:
/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
here; and ...
> +
> + return 0;
> +}
> +
> static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
> struct mmc_ios *ios, u32 rate)
> {
> @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
> }
> mmc_writel(host, REG_CLKCR, rval);
>
> - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
> - if (ret)
> - return ret;
Keep this as is; and add:
ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
if (ret)
return ret;
Instead of:
> + if (host->cfg->can_calibrate) {
> + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
> + if (ret)
> + return ret;
> +
> + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
> + } else {
> + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
> + if (ret)
> + return ret;
> + }
>
> return sunxi_mmc_oclk_onoff(host, 1);
> }
> @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
> static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
> .idma_des_size_bits = 13,
> .clk_delays = NULL,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = NULL,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = sunxi_mmc_clk_delays,
> + .can_calibrate = false,
> };
>
> static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
> .idma_des_size_bits = 16,
> .clk_delays = sun9i_mmc_clk_delays,
> + .can_calibrate = false,
> +};
> +
> +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
> + .idma_des_size_bits = 16,
> + .clk_delays = NULL,
> + .can_calibrate = true,
> };
>
> static const struct of_device_id sunxi_mmc_of_match[] = {
> @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
> { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
> { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
> { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
> + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
Not sure why all the below hunks are here, they certainly do not belong
in this patch; and they are not necessary. As I mentioned in the
commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
"Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
calls to the sample clks as-is, without adding checks for them being
NULL. All the clk_foo calls accept a NULL clk and will return success when
called with a NULL clk."
So please drop these.
Thanks & Regards,
Hans
> @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> goto error_disable_clk_ahb;
> }
>
> - ret = clk_prepare_enable(host->clk_output);
> - if (ret) {
> - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
> - goto error_disable_clk_mmc;
> - }
> + if (host->cfg->clk_delays) {
> + ret = clk_prepare_enable(host->clk_output);
> + if (ret) {
> + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
> + goto error_disable_clk_mmc;
> + }
>
> - ret = clk_prepare_enable(host->clk_sample);
> - if (ret) {
> - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
> - goto error_disable_clk_output;
> + ret = clk_prepare_enable(host->clk_sample);
> + if (ret) {
> + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
> + goto error_disable_clk_output;
> + }
> }
>
> if (!IS_ERR(host->reset)) {
> @@ -1107,9 +1176,11 @@ error_assert_reset:
> if (!IS_ERR(host->reset))
> reset_control_assert(host->reset);
> error_disable_clk_sample:
> - clk_disable_unprepare(host->clk_sample);
> + if (host->cfg->clk_delays)
> + clk_disable_unprepare(host->clk_sample);
> error_disable_clk_output:
> - clk_disable_unprepare(host->clk_output);
> + if (host->cfg->clk_delays)
> + clk_disable_unprepare(host->clk_output);
> error_disable_clk_mmc:
> clk_disable_unprepare(host->clk_mmc);
> error_disable_clk_ahb:
> @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
> if (!IS_ERR(host->reset))
> reset_control_assert(host->reset);
>
> - clk_disable_unprepare(host->clk_sample);
> - clk_disable_unprepare(host->clk_output);
> + if (host->cfg->clk_delays) {
> + clk_disable_unprepare(host->clk_sample);
> + clk_disable_unprepare(host->clk_output);
> + }
> +
> clk_disable_unprepare(host->clk_mmc);
> clk_disable_unprepare(host->clk_ahb);
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
2016-07-31 14:30 ` Hans de Goede
@ 2016-07-31 23:48 ` Icenowy Zheng
-1 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 23:48 UTC (permalink / raw)
To: Hans de Goede, Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson
Cc: Mark Rutland, devicetree, Michal Suchanek, linux-mmc,
linux-kernel, Jaehoon Chung, linux-arm-kernel
Hi,
31.07.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
> Hi,
>
> On 31-07-16 13:02, Icenowy Zheng wrote:
>> A64 SoC features a MMC controller which need only the mod clock, and can
>> calibrate delay by itself. This patch adds support for the new MMC
>> controller IP core.
>>
>> Based on work by Andre Przywara <andre.przywara@arm.com>.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>
> Looks good, some minor remarks (see comments inline) after those
> are addressed this is ready to be merged IMHO.
>
>> ---
>> drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 90 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index 2ec91ce..aa2abf3 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -72,6 +72,14 @@
>> #define SDXC_REG_CHDA (0x90)
>> #define SDXC_REG_CBDA (0x94)
>>
>> +/* New registers introduced in A64 */
>> +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
>> +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
>> +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
>> +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
>> +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
>> +
>> +
>
> Please drop 1 empty line here.
Thanks for you notice... I forgot to checkout the empty line...
>
>> #define mmc_readl(host, reg) \
>> readl((host)->reg_base + SDXC_##reg)
>> #define mmc_writel(host, reg, value) \
>> @@ -217,6 +225,15 @@
>> #define SDXC_CLK_50M_DDR 3
>> #define SDXC_CLK_50M_DDR_8BIT 4
>>
>> +#define SDXC_2X_TIMING_MODE BIT(31)
>> +
>> +#define SDXC_CAL_START BIT(15)
>> +#define SDXC_CAL_DONE BIT(14)
>> +#define SDXC_CAL_DL_SHIFT 8
>> +#define SDXC_CAL_DL_SW_EN BIT(7)
>> +#define SDXC_CAL_DL_SW_SHIFT 0
>> +#define SDXC_CAL_DL_MASK 0x3f
>> +
>> struct sunxi_mmc_clk_delay {
>> u32 output;
>> u32 sample;
>> @@ -232,6 +249,9 @@ struct sunxi_idma_des {
>> struct sunxi_mmc_cfg {
>> u32 idma_des_size_bits;
>> const struct sunxi_mmc_clk_delay *clk_delays;
>> +
>
> I would not insert an empty line here.
I'm only feeling that a empty before such a comment line is a
kind of separator...
(Refer to struct sunxi_mmc_host)
>
>> + /* does the IP block support autocalibration? */
>> + bool can_calibrate;
>> };
>>
>> struct sunxi_mmc_host {
>> @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>> return 0;
>> }
>>
>> +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
>> + struct mmc_ios *ios, int reg_off)
>> +{
>> + u32 reg = readl(host->reg_base + reg_off);
>> + u32 delay;
>> +
>
> I would add:
>
> if (!host->cfg->can_calibrate)
> return 0;
>
> Here; and ...
I agree with it. It's some kind of error checking.
>
>> + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
>> + reg &= ~SDXC_CAL_DL_SW_EN;
>> +
>> + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
>> +
>> + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
>> +
>> + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
>> + cpu_relax();
>> +
>> + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
>> +
>> + reg &= ~SDXC_CAL_START;
>> + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
>> +
>> + writel(reg, host->reg_base + reg_off);
>> +
>> + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
>
> Add:
>
> /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>
> here; and ...
The function only received a register address as a parameter...
It do not know the difference between different regs to calibrate.
>
>> +
>> + return 0;
>> +}
>> +
>> static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
>> struct mmc_ios *ios, u32 rate)
>> {
>> @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>> }
>> mmc_writel(host, REG_CLKCR, rval);
>>
>> - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>> - if (ret)
>> - return ret;
>
> Keep this as is; and add:
>
> ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
> if (ret)
> return ret;
>
It's a good idea :-)
> Instead of:
>
>> + if (host->cfg->can_calibrate) {
>> + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>> + if (ret)
>> + return ret;
>> +
>> + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>> + } else {
>> + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>> + if (ret)
>> + return ret;
>> + }
>>
>> return sunxi_mmc_oclk_onoff(host, 1);
>> }
>> @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
>> static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
>> .idma_des_size_bits = 13,
>> .clk_delays = NULL,
>> + .can_calibrate = false,
>> };
>>
>> static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
>> .idma_des_size_bits = 16,
>> .clk_delays = NULL,
>> + .can_calibrate = false,
>> };
>>
>> static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
>> .idma_des_size_bits = 16,
>> .clk_delays = sunxi_mmc_clk_delays,
>> + .can_calibrate = false,
>> };
>>
>> static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
>> .idma_des_size_bits = 16,
>> .clk_delays = sun9i_mmc_clk_delays,
>> + .can_calibrate = false,
>> +};
>> +
>> +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
>> + .idma_des_size_bits = 16,
>> + .clk_delays = NULL,
>> + .can_calibrate = true,
>> };
>>
>> static const struct of_device_id sunxi_mmc_of_match[] = {
>> @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>> { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
>> { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
>> { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
>> + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>
> Not sure why all the below hunks are here, they certainly do not belong
> in this patch; and they are not necessary. As I mentioned in the
> commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
>
> "Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
> calls to the sample clks as-is, without adding checks for them being
> NULL. All the clk_foo calls accept a NULL clk and will return success when
> called with a NULL clk."
>
> So please drop these.
>
Thanks! I will drop these!
(I don't know that the clk functions can accept NULL...)
> Thanks & Regards,
>
> Hans
>
>> @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>> goto error_disable_clk_ahb;
>> }
>>
>> - ret = clk_prepare_enable(host->clk_output);
>> - if (ret) {
>> - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>> - goto error_disable_clk_mmc;
>> - }
>> + if (host->cfg->clk_delays) {
>> + ret = clk_prepare_enable(host->clk_output);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>> + goto error_disable_clk_mmc;
>> + }
>>
>> - ret = clk_prepare_enable(host->clk_sample);
>> - if (ret) {
>> - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>> - goto error_disable_clk_output;
>> + ret = clk_prepare_enable(host->clk_sample);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>> + goto error_disable_clk_output;
>> + }
>> }
>>
>> if (!IS_ERR(host->reset)) {
>> @@ -1107,9 +1176,11 @@ error_assert_reset:
>> if (!IS_ERR(host->reset))
>> reset_control_assert(host->reset);
>> error_disable_clk_sample:
>> - clk_disable_unprepare(host->clk_sample);
>> + if (host->cfg->clk_delays)
>> + clk_disable_unprepare(host->clk_sample);
>> error_disable_clk_output:
>> - clk_disable_unprepare(host->clk_output);
>> + if (host->cfg->clk_delays)
>> + clk_disable_unprepare(host->clk_output);
>> error_disable_clk_mmc:
>> clk_disable_unprepare(host->clk_mmc);
>> error_disable_clk_ahb:
>> @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>> if (!IS_ERR(host->reset))
>> reset_control_assert(host->reset);
>>
>> - clk_disable_unprepare(host->clk_sample);
>> - clk_disable_unprepare(host->clk_output);
>> + if (host->cfg->clk_delays) {
>> + clk_disable_unprepare(host->clk_sample);
>> + clk_disable_unprepare(host->clk_output);
>> + }
>> +
>> clk_disable_unprepare(host->clk_mmc);
>> clk_disable_unprepare(host->clk_ahb);
Thanks,
Icenowy
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
@ 2016-07-31 23:48 ` Icenowy Zheng
0 siblings, 0 replies; 14+ messages in thread
From: Icenowy Zheng @ 2016-07-31 23:48 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
31.07.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
> Hi,
>
> On 31-07-16 13:02, Icenowy Zheng wrote:
>> ?A64 SoC features a MMC controller which need only the mod clock, and can
>> ?calibrate delay by itself. This patch adds support for the new MMC
>> ?controller IP core.
>>
>> ?Based on work by Andre Przywara <andre.przywara@arm.com>.
>>
>> ?Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>
> Looks good, some minor remarks (see comments inline) after those
> are addressed this is ready to be merged IMHO.
>
>> ?---
>> ??drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
>> ??1 file changed, 90 insertions(+), 16 deletions(-)
>>
>> ?diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> ?index 2ec91ce..aa2abf3 100644
>> ?--- a/drivers/mmc/host/sunxi-mmc.c
>> ?+++ b/drivers/mmc/host/sunxi-mmc.c
>> ?@@ -72,6 +72,14 @@
>> ??#define SDXC_REG_CHDA (0x90)
>> ??#define SDXC_REG_CBDA (0x94)
>>
>> ?+/* New registers introduced in A64 */
>> ?+#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
>> ?+#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
>> ?+#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
>> ?+#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
>> ?+#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
>> ?+
>> ?+
>
> Please drop 1 empty line here.
Thanks for you notice... I forgot to checkout the empty line...
>
>> ??#define mmc_readl(host, reg) \
>> ??????????readl((host)->reg_base + SDXC_##reg)
>> ??#define mmc_writel(host, reg, value) \
>> ?@@ -217,6 +225,15 @@
>> ??#define SDXC_CLK_50M_DDR 3
>> ??#define SDXC_CLK_50M_DDR_8BIT 4
>>
>> ?+#define SDXC_2X_TIMING_MODE BIT(31)
>> ?+
>> ?+#define SDXC_CAL_START BIT(15)
>> ?+#define SDXC_CAL_DONE BIT(14)
>> ?+#define SDXC_CAL_DL_SHIFT 8
>> ?+#define SDXC_CAL_DL_SW_EN BIT(7)
>> ?+#define SDXC_CAL_DL_SW_SHIFT 0
>> ?+#define SDXC_CAL_DL_MASK 0x3f
>> ?+
>> ??struct sunxi_mmc_clk_delay {
>> ??????????u32 output;
>> ??????????u32 sample;
>> ?@@ -232,6 +249,9 @@ struct sunxi_idma_des {
>> ??struct sunxi_mmc_cfg {
>> ??????????u32 idma_des_size_bits;
>> ??????????const struct sunxi_mmc_clk_delay *clk_delays;
>> ?+
>
> I would not insert an empty line here.
I'm only feeling that a empty before such a comment line is a
kind of separator...
(Refer to struct sunxi_mmc_host)
>
>> ?+ /* does the IP block support autocalibration? */
>> ?+ bool can_calibrate;
>> ??};
>>
>> ??struct sunxi_mmc_host {
>> ?@@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>> ??????????return 0;
>> ??}
>>
>> ?+static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
>> ?+ struct mmc_ios *ios, int reg_off)
>> ?+{
>> ?+ u32 reg = readl(host->reg_base + reg_off);
>> ?+ u32 delay;
>> ?+
>
> I would add:
>
> ????????if (!host->cfg->can_calibrate)
> ????????????????return 0;
>
> Here; and ...
I agree with it. It's some kind of error checking.
>
>> ?+ reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
>> ?+ reg &= ~SDXC_CAL_DL_SW_EN;
>> ?+
>> ?+ writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
>> ?+
>> ?+ dev_dbg(mmc_dev(host->mmc), "calibration started\n");
>> ?+
>> ?+ while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
>> ?+ cpu_relax();
>> ?+
>> ?+ delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
>> ?+
>> ?+ reg &= ~SDXC_CAL_START;
>> ?+ reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
>> ?+
>> ?+ writel(reg, host->reg_base + reg_off);
>> ?+
>> ?+ dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
>
> Add:
>
> ????????/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>
> here; and ...
The function only received a register address as a parameter...
It do not know the difference between different regs to calibrate.
>
>> ?+
>> ?+ return 0;
>> ?+}
>> ?+
>> ??static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
>> ?????????????????????????????????????struct mmc_ios *ios, u32 rate)
>> ??{
>> ?@@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>> ??????????}
>> ??????????mmc_writel(host, REG_CLKCR, rval);
>>
>> ?- ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>> ?- if (ret)
>> ?- return ret;
>
> Keep this as is; and add:
>
> ????????????????ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
> ????????????????if (ret)
> ????????????????????????return ret;
>
It's a good idea :-)
> Instead of:
>
>> ?+ if (host->cfg->can_calibrate) {
>> ?+ ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>> ?+ if (ret)
>> ?+ return ret;
>> ?+
>> ?+ /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>> ?+ } else {
>> ?+ ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>> ?+ if (ret)
>> ?+ return ret;
>> ?+ }
>>
>> ??????????return sunxi_mmc_oclk_onoff(host, 1);
>> ??}
>> ?@@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
>> ??static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
>> ??????????.idma_des_size_bits = 13,
>> ??????????.clk_delays = NULL,
>> ?+ .can_calibrate = false,
>> ??};
>>
>> ??static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
>> ??????????.idma_des_size_bits = 16,
>> ??????????.clk_delays = NULL,
>> ?+ .can_calibrate = false,
>> ??};
>>
>> ??static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
>> ??????????.idma_des_size_bits = 16,
>> ??????????.clk_delays = sunxi_mmc_clk_delays,
>> ?+ .can_calibrate = false,
>> ??};
>>
>> ??static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
>> ??????????.idma_des_size_bits = 16,
>> ??????????.clk_delays = sun9i_mmc_clk_delays,
>> ?+ .can_calibrate = false,
>> ?+};
>> ?+
>> ?+static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
>> ?+ .idma_des_size_bits = 16,
>> ?+ .clk_delays = NULL,
>> ?+ .can_calibrate = true,
>> ??};
>>
>> ??static const struct of_device_id sunxi_mmc_of_match[] = {
>> ?@@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>> ??????????{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
>> ??????????{ .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
>> ??????????{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
>> ?+ { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
>> ??????????{ /* sentinel */ }
>> ??};
>> ??MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>
> Not sure why all the below hunks are here, they certainly do not belong
> in this patch; and they are not necessary. As I mentioned in the
> commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
>
> ????"Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
> ?????calls to the sample clks as-is, without adding checks for them being
> ?????NULL. All the clk_foo calls accept a NULL clk and will return success when
> ?????called with a NULL clk."
>
> So please drop these.
>
Thanks! I will drop these!
(I don't know that the clk functions can accept NULL...)
> Thanks & Regards,
>
> Hans
>
>> ?@@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>> ??????????????????goto error_disable_clk_ahb;
>> ??????????}
>>
>> ?- ret = clk_prepare_enable(host->clk_output);
>> ?- if (ret) {
>> ?- dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>> ?- goto error_disable_clk_mmc;
>> ?- }
>> ?+ if (host->cfg->clk_delays) {
>> ?+ ret = clk_prepare_enable(host->clk_output);
>> ?+ if (ret) {
>> ?+ dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>> ?+ goto error_disable_clk_mmc;
>> ?+ }
>>
>> ?- ret = clk_prepare_enable(host->clk_sample);
>> ?- if (ret) {
>> ?- dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>> ?- goto error_disable_clk_output;
>> ?+ ret = clk_prepare_enable(host->clk_sample);
>> ?+ if (ret) {
>> ?+ dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>> ?+ goto error_disable_clk_output;
>> ?+ }
>> ??????????}
>>
>> ??????????if (!IS_ERR(host->reset)) {
>> ?@@ -1107,9 +1176,11 @@ error_assert_reset:
>> ??????????if (!IS_ERR(host->reset))
>> ??????????????????reset_control_assert(host->reset);
>> ??error_disable_clk_sample:
>> ?- clk_disable_unprepare(host->clk_sample);
>> ?+ if (host->cfg->clk_delays)
>> ?+ clk_disable_unprepare(host->clk_sample);
>> ??error_disable_clk_output:
>> ?- clk_disable_unprepare(host->clk_output);
>> ?+ if (host->cfg->clk_delays)
>> ?+ clk_disable_unprepare(host->clk_output);
>> ??error_disable_clk_mmc:
>> ??????????clk_disable_unprepare(host->clk_mmc);
>> ??error_disable_clk_ahb:
>> ?@@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>> ??????????if (!IS_ERR(host->reset))
>> ??????????????????reset_control_assert(host->reset);
>>
>> ?- clk_disable_unprepare(host->clk_sample);
>> ?- clk_disable_unprepare(host->clk_output);
>> ?+ if (host->cfg->clk_delays) {
>> ?+ clk_disable_unprepare(host->clk_sample);
>> ?+ clk_disable_unprepare(host->clk_output);
>> ?+ }
>> ?+
>> ??????????clk_disable_unprepare(host->clk_mmc);
>> ??????????clk_disable_unprepare(host->clk_ahb);
Thanks,
Icenowy
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
2016-07-31 23:48 ` Icenowy Zheng
(?)
@ 2016-08-01 7:29 ` Hans de Goede
-1 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2016-08-01 7:29 UTC (permalink / raw)
To: Icenowy Zheng, Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson
Cc: Mark Rutland, Jaehoon Chung, Michal Suchanek, devicetree,
linux-arm-kernel, linux-kernel, linux-mmc
Hi,
On 01-08-16 01:48, Icenowy Zheng wrote:
>
> Hi,
> 31.07.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
>> Hi,
>>
>> On 31-07-16 13:02, Icenowy Zheng wrote:
>>> A64 SoC features a MMC controller which need only the mod clock, and can
>>> calibrate delay by itself. This patch adds support for the new MMC
>>> controller IP core.
>>>
>>> Based on work by Andre Przywara <andre.przywara@arm.com>.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>
>> Looks good, some minor remarks (see comments inline) after those
>> are addressed this is ready to be merged IMHO.
>>
>>> ---
>>> drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 90 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>>> index 2ec91ce..aa2abf3 100644
>>> --- a/drivers/mmc/host/sunxi-mmc.c
>>> +++ b/drivers/mmc/host/sunxi-mmc.c
>>> @@ -72,6 +72,14 @@
>>> #define SDXC_REG_CHDA (0x90)
>>> #define SDXC_REG_CBDA (0x94)
>>>
>>> +/* New registers introduced in A64 */
>>> +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
>>> +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
>>> +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
>>> +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
>>> +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
>>> +
>>> +
>>
>> Please drop 1 empty line here.
> Thanks for you notice... I forgot to checkout the empty line...
>>
>>> #define mmc_readl(host, reg) \
>>> readl((host)->reg_base + SDXC_##reg)
>>> #define mmc_writel(host, reg, value) \
>>> @@ -217,6 +225,15 @@
>>> #define SDXC_CLK_50M_DDR 3
>>> #define SDXC_CLK_50M_DDR_8BIT 4
>>>
>>> +#define SDXC_2X_TIMING_MODE BIT(31)
>>> +
>>> +#define SDXC_CAL_START BIT(15)
>>> +#define SDXC_CAL_DONE BIT(14)
>>> +#define SDXC_CAL_DL_SHIFT 8
>>> +#define SDXC_CAL_DL_SW_EN BIT(7)
>>> +#define SDXC_CAL_DL_SW_SHIFT 0
>>> +#define SDXC_CAL_DL_MASK 0x3f
>>> +
>>> struct sunxi_mmc_clk_delay {
>>> u32 output;
>>> u32 sample;
>>> @@ -232,6 +249,9 @@ struct sunxi_idma_des {
>>> struct sunxi_mmc_cfg {
>>> u32 idma_des_size_bits;
>>> const struct sunxi_mmc_clk_delay *clk_delays;
>>> +
>>
>> I would not insert an empty line here.
> I'm only feeling that a empty before such a comment line is a
> kind of separator...
> (Refer to struct sunxi_mmc_host)
Ok.
>>
>>> + /* does the IP block support autocalibration? */
>>> + bool can_calibrate;
>>> };
>>>
>>> struct sunxi_mmc_host {
>>> @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>>> return 0;
>>> }
>>>
>>> +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
>>> + struct mmc_ios *ios, int reg_off)
>>> +{
>>> + u32 reg = readl(host->reg_base + reg_off);
>>> + u32 delay;
>>> +
>>
>> I would add:
>>
>> if (!host->cfg->can_calibrate)
>> return 0;
>>
>> Here; and ...
> I agree with it. It's some kind of error checking.
>>
>>> + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
>>> + reg &= ~SDXC_CAL_DL_SW_EN;
>>> +
>>> + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
>>> +
>>> + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
>>> +
>>> + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
>>> + cpu_relax();
>>> +
>>> + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
>>> +
>>> + reg &= ~SDXC_CAL_START;
>>> + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
>>> +
>>> + writel(reg, host->reg_base + reg_off);
>>> +
>>> + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
>>
>> Add:
>>
>> /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>
>> here; and ...
> The function only received a register address as a parameter...
> It do not know the difference between different regs to calibrate.
We can change the parameters the function gets, or even introduce
a new function once we fix the TODO.
For now we just need a place to put the TODO and this seems like
the best place.
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
>>> struct mmc_ios *ios, u32 rate)
>>> {
>>> @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>>> }
>>> mmc_writel(host, REG_CLKCR, rval);
>>>
>>> - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>> - if (ret)
>>> - return ret;
>>
>> Keep this as is; and add:
>>
>> ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>> if (ret)
>> return ret;
>>
> It's a good idea :-)
>> Instead of:
>>
>>> + if (host->cfg->can_calibrate) {
>>> + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>> + } else {
>>> + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>> + if (ret)
>>> + return ret;
>>> + }
>>>
>>> return sunxi_mmc_oclk_onoff(host, 1);
>>> }
>>> @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
>>> static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
>>> .idma_des_size_bits = 13,
>>> .clk_delays = NULL,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = NULL,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = sunxi_mmc_clk_delays,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = sun9i_mmc_clk_delays,
>>> + .can_calibrate = false,
>>> +};
>>> +
>>> +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
>>> + .idma_des_size_bits = 16,
>>> + .clk_delays = NULL,
>>> + .can_calibrate = true,
>>> };
>>>
>>> static const struct of_device_id sunxi_mmc_of_match[] = {
>>> @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>>> { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
>>> { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
>>> { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
>>> + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
>>> { /* sentinel */ }
>>> };
>>> MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>>
>> Not sure why all the below hunks are here, they certainly do not belong
>> in this patch; and they are not necessary. As I mentioned in the
>> commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
>>
>> "Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
>> calls to the sample clks as-is, without adding checks for them being
>> NULL. All the clk_foo calls accept a NULL clk and will return success when
>> called with a NULL clk."
>>
>> So please drop these.
>>
> Thanks! I will drop these!
> (I don't know that the clk functions can accept NULL...)
Great.
Regards,
Hans
>
>> Thanks & Regards,
>>
>> Hans
>>
>>> @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>>> goto error_disable_clk_ahb;
>>> }
>>>
>>> - ret = clk_prepare_enable(host->clk_output);
>>> - if (ret) {
>>> - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>> - goto error_disable_clk_mmc;
>>> - }
>>> + if (host->cfg->clk_delays) {
>>> + ret = clk_prepare_enable(host->clk_output);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>> + goto error_disable_clk_mmc;
>>> + }
>>>
>>> - ret = clk_prepare_enable(host->clk_sample);
>>> - if (ret) {
>>> - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>> - goto error_disable_clk_output;
>>> + ret = clk_prepare_enable(host->clk_sample);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>> + goto error_disable_clk_output;
>>> + }
>>> }
>>>
>>> if (!IS_ERR(host->reset)) {
>>> @@ -1107,9 +1176,11 @@ error_assert_reset:
>>> if (!IS_ERR(host->reset))
>>> reset_control_assert(host->reset);
>>> error_disable_clk_sample:
>>> - clk_disable_unprepare(host->clk_sample);
>>> + if (host->cfg->clk_delays)
>>> + clk_disable_unprepare(host->clk_sample);
>>> error_disable_clk_output:
>>> - clk_disable_unprepare(host->clk_output);
>>> + if (host->cfg->clk_delays)
>>> + clk_disable_unprepare(host->clk_output);
>>> error_disable_clk_mmc:
>>> clk_disable_unprepare(host->clk_mmc);
>>> error_disable_clk_ahb:
>>> @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>>> if (!IS_ERR(host->reset))
>>> reset_control_assert(host->reset);
>>>
>>> - clk_disable_unprepare(host->clk_sample);
>>> - clk_disable_unprepare(host->clk_output);
>>> + if (host->cfg->clk_delays) {
>>> + clk_disable_unprepare(host->clk_sample);
>>> + clk_disable_unprepare(host->clk_output);
>>> + }
>>> +
>>> clk_disable_unprepare(host->clk_mmc);
>>> clk_disable_unprepare(host->clk_ahb);
>
> Thanks,
> Icenowy
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
@ 2016-08-01 7:29 ` Hans de Goede
0 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2016-08-01 7:29 UTC (permalink / raw)
To: Icenowy Zheng, Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson
Cc: Mark Rutland, devicetree, Michal Suchanek, linux-mmc,
linux-kernel, Jaehoon Chung, linux-arm-kernel
Hi,
On 01-08-16 01:48, Icenowy Zheng wrote:
>
> Hi,
> 31.07.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
>> Hi,
>>
>> On 31-07-16 13:02, Icenowy Zheng wrote:
>>> A64 SoC features a MMC controller which need only the mod clock, and can
>>> calibrate delay by itself. This patch adds support for the new MMC
>>> controller IP core.
>>>
>>> Based on work by Andre Przywara <andre.przywara@arm.com>.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>
>> Looks good, some minor remarks (see comments inline) after those
>> are addressed this is ready to be merged IMHO.
>>
>>> ---
>>> drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 90 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>>> index 2ec91ce..aa2abf3 100644
>>> --- a/drivers/mmc/host/sunxi-mmc.c
>>> +++ b/drivers/mmc/host/sunxi-mmc.c
>>> @@ -72,6 +72,14 @@
>>> #define SDXC_REG_CHDA (0x90)
>>> #define SDXC_REG_CBDA (0x94)
>>>
>>> +/* New registers introduced in A64 */
>>> +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
>>> +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
>>> +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
>>> +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
>>> +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
>>> +
>>> +
>>
>> Please drop 1 empty line here.
> Thanks for you notice... I forgot to checkout the empty line...
>>
>>> #define mmc_readl(host, reg) \
>>> readl((host)->reg_base + SDXC_##reg)
>>> #define mmc_writel(host, reg, value) \
>>> @@ -217,6 +225,15 @@
>>> #define SDXC_CLK_50M_DDR 3
>>> #define SDXC_CLK_50M_DDR_8BIT 4
>>>
>>> +#define SDXC_2X_TIMING_MODE BIT(31)
>>> +
>>> +#define SDXC_CAL_START BIT(15)
>>> +#define SDXC_CAL_DONE BIT(14)
>>> +#define SDXC_CAL_DL_SHIFT 8
>>> +#define SDXC_CAL_DL_SW_EN BIT(7)
>>> +#define SDXC_CAL_DL_SW_SHIFT 0
>>> +#define SDXC_CAL_DL_MASK 0x3f
>>> +
>>> struct sunxi_mmc_clk_delay {
>>> u32 output;
>>> u32 sample;
>>> @@ -232,6 +249,9 @@ struct sunxi_idma_des {
>>> struct sunxi_mmc_cfg {
>>> u32 idma_des_size_bits;
>>> const struct sunxi_mmc_clk_delay *clk_delays;
>>> +
>>
>> I would not insert an empty line here.
> I'm only feeling that a empty before such a comment line is a
> kind of separator...
> (Refer to struct sunxi_mmc_host)
Ok.
>>
>>> + /* does the IP block support autocalibration? */
>>> + bool can_calibrate;
>>> };
>>>
>>> struct sunxi_mmc_host {
>>> @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>>> return 0;
>>> }
>>>
>>> +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
>>> + struct mmc_ios *ios, int reg_off)
>>> +{
>>> + u32 reg = readl(host->reg_base + reg_off);
>>> + u32 delay;
>>> +
>>
>> I would add:
>>
>> if (!host->cfg->can_calibrate)
>> return 0;
>>
>> Here; and ...
> I agree with it. It's some kind of error checking.
>>
>>> + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
>>> + reg &= ~SDXC_CAL_DL_SW_EN;
>>> +
>>> + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
>>> +
>>> + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
>>> +
>>> + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
>>> + cpu_relax();
>>> +
>>> + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
>>> +
>>> + reg &= ~SDXC_CAL_START;
>>> + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
>>> +
>>> + writel(reg, host->reg_base + reg_off);
>>> +
>>> + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
>>
>> Add:
>>
>> /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>
>> here; and ...
> The function only received a register address as a parameter...
> It do not know the difference between different regs to calibrate.
We can change the parameters the function gets, or even introduce
a new function once we fix the TODO.
For now we just need a place to put the TODO and this seems like
the best place.
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
>>> struct mmc_ios *ios, u32 rate)
>>> {
>>> @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>>> }
>>> mmc_writel(host, REG_CLKCR, rval);
>>>
>>> - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>> - if (ret)
>>> - return ret;
>>
>> Keep this as is; and add:
>>
>> ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>> if (ret)
>> return ret;
>>
> It's a good idea :-)
>> Instead of:
>>
>>> + if (host->cfg->can_calibrate) {
>>> + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>> + } else {
>>> + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>> + if (ret)
>>> + return ret;
>>> + }
>>>
>>> return sunxi_mmc_oclk_onoff(host, 1);
>>> }
>>> @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
>>> static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
>>> .idma_des_size_bits = 13,
>>> .clk_delays = NULL,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = NULL,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = sunxi_mmc_clk_delays,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = sun9i_mmc_clk_delays,
>>> + .can_calibrate = false,
>>> +};
>>> +
>>> +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
>>> + .idma_des_size_bits = 16,
>>> + .clk_delays = NULL,
>>> + .can_calibrate = true,
>>> };
>>>
>>> static const struct of_device_id sunxi_mmc_of_match[] = {
>>> @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>>> { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
>>> { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
>>> { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
>>> + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
>>> { /* sentinel */ }
>>> };
>>> MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>>
>> Not sure why all the below hunks are here, they certainly do not belong
>> in this patch; and they are not necessary. As I mentioned in the
>> commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
>>
>> "Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
>> calls to the sample clks as-is, without adding checks for them being
>> NULL. All the clk_foo calls accept a NULL clk and will return success when
>> called with a NULL clk."
>>
>> So please drop these.
>>
> Thanks! I will drop these!
> (I don't know that the clk functions can accept NULL...)
Great.
Regards,
Hans
>
>> Thanks & Regards,
>>
>> Hans
>>
>>> @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>>> goto error_disable_clk_ahb;
>>> }
>>>
>>> - ret = clk_prepare_enable(host->clk_output);
>>> - if (ret) {
>>> - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>> - goto error_disable_clk_mmc;
>>> - }
>>> + if (host->cfg->clk_delays) {
>>> + ret = clk_prepare_enable(host->clk_output);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>> + goto error_disable_clk_mmc;
>>> + }
>>>
>>> - ret = clk_prepare_enable(host->clk_sample);
>>> - if (ret) {
>>> - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>> - goto error_disable_clk_output;
>>> + ret = clk_prepare_enable(host->clk_sample);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>> + goto error_disable_clk_output;
>>> + }
>>> }
>>>
>>> if (!IS_ERR(host->reset)) {
>>> @@ -1107,9 +1176,11 @@ error_assert_reset:
>>> if (!IS_ERR(host->reset))
>>> reset_control_assert(host->reset);
>>> error_disable_clk_sample:
>>> - clk_disable_unprepare(host->clk_sample);
>>> + if (host->cfg->clk_delays)
>>> + clk_disable_unprepare(host->clk_sample);
>>> error_disable_clk_output:
>>> - clk_disable_unprepare(host->clk_output);
>>> + if (host->cfg->clk_delays)
>>> + clk_disable_unprepare(host->clk_output);
>>> error_disable_clk_mmc:
>>> clk_disable_unprepare(host->clk_mmc);
>>> error_disable_clk_ahb:
>>> @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>>> if (!IS_ERR(host->reset))
>>> reset_control_assert(host->reset);
>>>
>>> - clk_disable_unprepare(host->clk_sample);
>>> - clk_disable_unprepare(host->clk_output);
>>> + if (host->cfg->clk_delays) {
>>> + clk_disable_unprepare(host->clk_sample);
>>> + clk_disable_unprepare(host->clk_output);
>>> + }
>>> +
>>> clk_disable_unprepare(host->clk_mmc);
>>> clk_disable_unprepare(host->clk_ahb);
>
> Thanks,
> Icenowy
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
@ 2016-08-01 7:29 ` Hans de Goede
0 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2016-08-01 7:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 01-08-16 01:48, Icenowy Zheng wrote:
>
> Hi,
> 31.07.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
>> Hi,
>>
>> On 31-07-16 13:02, Icenowy Zheng wrote:
>>> A64 SoC features a MMC controller which need only the mod clock, and can
>>> calibrate delay by itself. This patch adds support for the new MMC
>>> controller IP core.
>>>
>>> Based on work by Andre Przywara <andre.przywara@arm.com>.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>
>> Looks good, some minor remarks (see comments inline) after those
>> are addressed this is ready to be merged IMHO.
>>
>>> ---
>>> drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 90 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>>> index 2ec91ce..aa2abf3 100644
>>> --- a/drivers/mmc/host/sunxi-mmc.c
>>> +++ b/drivers/mmc/host/sunxi-mmc.c
>>> @@ -72,6 +72,14 @@
>>> #define SDXC_REG_CHDA (0x90)
>>> #define SDXC_REG_CBDA (0x94)
>>>
>>> +/* New registers introduced in A64 */
>>> +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
>>> +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
>>> +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
>>> +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
>>> +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
>>> +
>>> +
>>
>> Please drop 1 empty line here.
> Thanks for you notice... I forgot to checkout the empty line...
>>
>>> #define mmc_readl(host, reg) \
>>> readl((host)->reg_base + SDXC_##reg)
>>> #define mmc_writel(host, reg, value) \
>>> @@ -217,6 +225,15 @@
>>> #define SDXC_CLK_50M_DDR 3
>>> #define SDXC_CLK_50M_DDR_8BIT 4
>>>
>>> +#define SDXC_2X_TIMING_MODE BIT(31)
>>> +
>>> +#define SDXC_CAL_START BIT(15)
>>> +#define SDXC_CAL_DONE BIT(14)
>>> +#define SDXC_CAL_DL_SHIFT 8
>>> +#define SDXC_CAL_DL_SW_EN BIT(7)
>>> +#define SDXC_CAL_DL_SW_SHIFT 0
>>> +#define SDXC_CAL_DL_MASK 0x3f
>>> +
>>> struct sunxi_mmc_clk_delay {
>>> u32 output;
>>> u32 sample;
>>> @@ -232,6 +249,9 @@ struct sunxi_idma_des {
>>> struct sunxi_mmc_cfg {
>>> u32 idma_des_size_bits;
>>> const struct sunxi_mmc_clk_delay *clk_delays;
>>> +
>>
>> I would not insert an empty line here.
> I'm only feeling that a empty before such a comment line is a
> kind of separator...
> (Refer to struct sunxi_mmc_host)
Ok.
>>
>>> + /* does the IP block support autocalibration? */
>>> + bool can_calibrate;
>>> };
>>>
>>> struct sunxi_mmc_host {
>>> @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>>> return 0;
>>> }
>>>
>>> +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
>>> + struct mmc_ios *ios, int reg_off)
>>> +{
>>> + u32 reg = readl(host->reg_base + reg_off);
>>> + u32 delay;
>>> +
>>
>> I would add:
>>
>> if (!host->cfg->can_calibrate)
>> return 0;
>>
>> Here; and ...
> I agree with it. It's some kind of error checking.
>>
>>> + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
>>> + reg &= ~SDXC_CAL_DL_SW_EN;
>>> +
>>> + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
>>> +
>>> + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
>>> +
>>> + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
>>> + cpu_relax();
>>> +
>>> + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
>>> +
>>> + reg &= ~SDXC_CAL_START;
>>> + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
>>> +
>>> + writel(reg, host->reg_base + reg_off);
>>> +
>>> + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
>>
>> Add:
>>
>> /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>
>> here; and ...
> The function only received a register address as a parameter...
> It do not know the difference between different regs to calibrate.
We can change the parameters the function gets, or even introduce
a new function once we fix the TODO.
For now we just need a place to put the TODO and this seems like
the best place.
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
>>> struct mmc_ios *ios, u32 rate)
>>> {
>>> @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>>> }
>>> mmc_writel(host, REG_CLKCR, rval);
>>>
>>> - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>> - if (ret)
>>> - return ret;
>>
>> Keep this as is; and add:
>>
>> ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>> if (ret)
>> return ret;
>>
> It's a good idea :-)
>> Instead of:
>>
>>> + if (host->cfg->can_calibrate) {
>>> + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>> + } else {
>>> + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>> + if (ret)
>>> + return ret;
>>> + }
>>>
>>> return sunxi_mmc_oclk_onoff(host, 1);
>>> }
>>> @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
>>> static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
>>> .idma_des_size_bits = 13,
>>> .clk_delays = NULL,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = NULL,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = sunxi_mmc_clk_delays,
>>> + .can_calibrate = false,
>>> };
>>>
>>> static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
>>> .idma_des_size_bits = 16,
>>> .clk_delays = sun9i_mmc_clk_delays,
>>> + .can_calibrate = false,
>>> +};
>>> +
>>> +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
>>> + .idma_des_size_bits = 16,
>>> + .clk_delays = NULL,
>>> + .can_calibrate = true,
>>> };
>>>
>>> static const struct of_device_id sunxi_mmc_of_match[] = {
>>> @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>>> { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
>>> { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
>>> { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
>>> + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
>>> { /* sentinel */ }
>>> };
>>> MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>>
>> Not sure why all the below hunks are here, they certainly do not belong
>> in this patch; and they are not necessary. As I mentioned in the
>> commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
>>
>> "Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
>> calls to the sample clks as-is, without adding checks for them being
>> NULL. All the clk_foo calls accept a NULL clk and will return success when
>> called with a NULL clk."
>>
>> So please drop these.
>>
> Thanks! I will drop these!
> (I don't know that the clk functions can accept NULL...)
Great.
Regards,
Hans
>
>> Thanks & Regards,
>>
>> Hans
>>
>>> @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>>> goto error_disable_clk_ahb;
>>> }
>>>
>>> - ret = clk_prepare_enable(host->clk_output);
>>> - if (ret) {
>>> - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>> - goto error_disable_clk_mmc;
>>> - }
>>> + if (host->cfg->clk_delays) {
>>> + ret = clk_prepare_enable(host->clk_output);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>> + goto error_disable_clk_mmc;
>>> + }
>>>
>>> - ret = clk_prepare_enable(host->clk_sample);
>>> - if (ret) {
>>> - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>> - goto error_disable_clk_output;
>>> + ret = clk_prepare_enable(host->clk_sample);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>> + goto error_disable_clk_output;
>>> + }
>>> }
>>>
>>> if (!IS_ERR(host->reset)) {
>>> @@ -1107,9 +1176,11 @@ error_assert_reset:
>>> if (!IS_ERR(host->reset))
>>> reset_control_assert(host->reset);
>>> error_disable_clk_sample:
>>> - clk_disable_unprepare(host->clk_sample);
>>> + if (host->cfg->clk_delays)
>>> + clk_disable_unprepare(host->clk_sample);
>>> error_disable_clk_output:
>>> - clk_disable_unprepare(host->clk_output);
>>> + if (host->cfg->clk_delays)
>>> + clk_disable_unprepare(host->clk_output);
>>> error_disable_clk_mmc:
>>> clk_disable_unprepare(host->clk_mmc);
>>> error_disable_clk_ahb:
>>> @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>>> if (!IS_ERR(host->reset))
>>> reset_control_assert(host->reset);
>>>
>>> - clk_disable_unprepare(host->clk_sample);
>>> - clk_disable_unprepare(host->clk_output);
>>> + if (host->cfg->clk_delays) {
>>> + clk_disable_unprepare(host->clk_sample);
>>> + clk_disable_unprepare(host->clk_output);
>>> + }
>>> +
>>> clk_disable_unprepare(host->clk_mmc);
>>> clk_disable_unprepare(host->clk_ahb);
>
> Thanks,
> Icenowy
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-08-01 7:30 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-31 11:02 [PATCH v2 0/2] Add support for Allwinner A64 mmc controller Icenowy Zheng
2016-07-31 11:02 ` Icenowy Zheng
2016-07-31 11:02 ` [PATCH v2 1/2] Documentation: dt: Add new compatible to sunxi mmc driver bindings Icenowy Zheng
2016-07-31 11:02 ` Icenowy Zheng
2016-07-31 11:02 ` [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller Icenowy Zheng
2016-07-31 11:02 ` Icenowy Zheng
2016-07-31 14:30 ` Hans de Goede
2016-07-31 14:30 ` Hans de Goede
2016-07-31 14:30 ` Hans de Goede
2016-07-31 23:48 ` Icenowy Zheng
2016-07-31 23:48 ` Icenowy Zheng
2016-08-01 7:29 ` Hans de Goede
2016-08-01 7:29 ` Hans de Goede
2016-08-01 7:29 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.