2020-10-11 18:03:22

by Wenbin Mei (梅文彬)

[permalink] [raw]
Subject: [PATCH v4 0/4] Add mmc support for MT8192 SoC

Change in v4:
1)drop "vmmc" and "vqmmc" desciption in mtk-sd.yaml
2)add vmmq/vqmmc supplies and the pinctrls to required properties
3)change dbg level and exit this function
4)use devm_clk_get_optional instead of devm_clk_get function
5)remove else branch for sys_clk_cg

Change in v3:
1)change maintainers name in mtk-sd.yaml
2)change "compatible" properties to enum type and sort it
3)drop these properties: "reg" and "interrupts"
4)add "maxItems" constraints on these properties: "vmmc-supply", "vqmmc-supply",
"assigned-clocks", "assigned-clock-parents"
5)add "minimum" and "maximum" constraints on these properties: "mediatek,hs400-cmd-int-delay",
"mediatek,latch-ck", "hs400-ds-delay", "mediatek,hs200-cmd-int-delay"

Change in v2:
Convert mtk-sd to json-schema

Wenbin Mei (4):
dt-bindings: mmc: Convert mtk-sd to json-schema
mmc: dt-bindings: add support for MT8192 SoC
arm64: dts: mt8192: add mmc device node
mmc: mediatek: Add subsys clock control for MT8192 msdc
---
This patch depends on
[v4,1/3] arm64: dts: Add Mediatek SoC MT8192 and evaluation board dts and Makefile
[v3,1/9] dt-bindings: ARM: Mediatek: Document bindings for MT8192 BSP
[v3,6/9] clk: mediatek: Add dt-bindings for MT8192 clocks
[v3,9/9] clk: mediatek: Add MT8192 clock support
[v3,1/3] dt-bindings: pinctrl: mt8192: add pinctrl file
[v3,2/3] dt-bindings: pinctrl: mt8192: add binding document
[v3,3/3] pinctrl: add pinctrl driver on mt8192
[v2,1/4] soc: mediatek: pwrap: use BIT() macro
[v2,2/4] soc: mediatek: pwrap: add arbiter capability
[v2,3/4] dt-bindings: mediatek: add compatible for MT6873/8192 pwrap
[v2,4/4] soc: mediatek: pwrap: add pwrap driver for MT6873/8192 SoCs
[2/8] dt-bindings: mfd: Add compatible for the MediaTek MT6359 PMIC
[3/8] dt-bindings: regulator: Add document for MT6359 regulator
[4/8] mfd: Add support for the MediaTek MT6359 PMIC
[5/8] regulator: mt6359: Add support for MT6359 regulator
[7/8] regulator: mt6359: Add support for MT6359P regulator
[8/8] arm64: dts: mt6359: add PMIC MT6359 related nodes

Please also accept this patch together with [1][2][3][4][5]
to avoid build and dt binding check error.
[1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=332621
[2] https://patchwork.kernel.org/project/linux-mediatek/list/?series=342593
[3] https://patchwork.kernel.org/project/linux-mediatek/list/?series=330017
[4] https://patchwork.kernel.org/project/linux-mediatek/list/?series=322937
[5] https://patchwork.kernel.org/project/linux-mediatek/list/?series=323171
---
.../devicetree/bindings/mmc/mtk-sd.txt | 75 --------
.../devicetree/bindings/mmc/mtk-sd.yaml | 174 ++++++++++++++++++
arch/arm64/boot/dts/mediatek/mt8192-evb.dts | 89 +++++++++
arch/arm64/boot/dts/mediatek/mt8192.dtsi | 34 ++++
drivers/mmc/host/mtk-sd.c | 80 ++++++--
5 files changed, 359 insertions(+), 93 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mmc/mtk-sd.txt
create mode 100644 Documentation/devicetree/bindings/mmc/mtk-sd.yaml

--
2.18.0


2020-10-11 18:03:27

by Wenbin Mei (梅文彬)

[permalink] [raw]
Subject: [PATCH v4 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc

MT8192 msdc is an independent sub system, we need control more bus
clocks for it.
Add support for the additional subsys clocks to allow it to be
configured appropriately.

Signed-off-by: Wenbin Mei <[email protected]>
Reviewed-by: Nicolas Boichat <[email protected]>
---
drivers/mmc/host/mtk-sd.c | 80 ++++++++++++++++++++++++++++++---------
1 file changed, 62 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index a704745e5882..350e45432e21 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -425,6 +425,8 @@ struct msdc_host {
struct clk *h_clk; /* msdc h_clk */
struct clk *bus_clk; /* bus clock which used to access register */
struct clk *src_clk_cg; /* msdc source clock control gate */
+ struct clk *sys_clk_cg; /* msdc subsys clock control gate */
+ struct clk_bulk_data bulk_clks[3]; /* pclk, axi, ahb clock control gate */
u32 mclk; /* mmc subsystem clock frequency */
u32 src_clk_freq; /* source clock frequency */
unsigned char timing;
@@ -784,6 +786,8 @@ static void msdc_set_busy_timeout(struct msdc_host *host, u64 ns, u64 clks)

static void msdc_gate_clock(struct msdc_host *host)
{
+ clk_bulk_disable_unprepare(ARRAY_SIZE(host->bulk_clks),
+ host->bulk_clks);
clk_disable_unprepare(host->src_clk_cg);
clk_disable_unprepare(host->src_clk);
clk_disable_unprepare(host->bus_clk);
@@ -792,10 +796,19 @@ static void msdc_gate_clock(struct msdc_host *host)

static void msdc_ungate_clock(struct msdc_host *host)
{
+ int ret;
+
clk_prepare_enable(host->h_clk);
clk_prepare_enable(host->bus_clk);
clk_prepare_enable(host->src_clk);
clk_prepare_enable(host->src_clk_cg);
+ ret = clk_bulk_prepare_enable(ARRAY_SIZE(host->bulk_clks),
+ host->bulk_clks);
+ if (ret) {
+ dev_err(host->dev, "enable clks failed!\n");
+ return;
+ }
+
while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
cpu_relax();
}
@@ -2366,6 +2379,53 @@ static void msdc_of_property_parse(struct platform_device *pdev,
host->cqhci = false;
}

+static int msdc_of_clock_parse(struct platform_device *pdev,
+ struct msdc_host *host)
+{
+ struct clk *clk;
+
+ host->src_clk = devm_clk_get_optional(&pdev->dev, "source");
+ if (IS_ERR(host->src_clk))
+ return PTR_ERR(host->src_clk);
+
+ host->h_clk = devm_clk_get_optional(&pdev->dev, "hclk");
+ if (IS_ERR(host->h_clk))
+ return PTR_ERR(host->h_clk);
+
+ host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
+ if (IS_ERR(host->bus_clk))
+ host->bus_clk = NULL;
+
+ /*source clock control gate is optional clock*/
+ host->src_clk_cg = devm_clk_get_optional(&pdev->dev, "source_cg");
+ if (IS_ERR(host->src_clk_cg))
+ host->src_clk_cg = NULL;
+
+ host->sys_clk_cg = devm_clk_get_optional(&pdev->dev, "sys_cg");
+ if (IS_ERR(host->sys_clk_cg))
+ host->sys_clk_cg = NULL;
+
+ /* If present, always enable for this clock gate */
+ clk_prepare_enable(host->sys_clk_cg);
+
+ clk = devm_clk_get_optional(&pdev->dev, "pclk_cg");
+ if (IS_ERR(clk))
+ clk = NULL;
+ host->bulk_clks[0].clk = clk;
+
+ clk = devm_clk_get_optional(&pdev->dev, "axi_cg");
+ if (IS_ERR(clk))
+ clk = NULL;
+ host->bulk_clks[1].clk = clk;
+
+ clk = devm_clk_get_optional(&pdev->dev, "ahb_cg");
+ if (IS_ERR(clk))
+ clk = NULL;
+ host->bulk_clks[2].clk = clk;
+
+ return 0;
+}
+
static int msdc_drv_probe(struct platform_device *pdev)
{
struct mmc_host *mmc;
@@ -2405,25 +2465,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
if (ret)
goto host_free;

- host->src_clk = devm_clk_get(&pdev->dev, "source");
- if (IS_ERR(host->src_clk)) {
- ret = PTR_ERR(host->src_clk);
- goto host_free;
- }
-
- host->h_clk = devm_clk_get(&pdev->dev, "hclk");
- if (IS_ERR(host->h_clk)) {
- ret = PTR_ERR(host->h_clk);
+ ret = msdc_of_clock_parse(pdev, host);
+ if (ret)
goto host_free;
- }
-
- host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
- if (IS_ERR(host->bus_clk))
- host->bus_clk = NULL;
- /*source clock control gate is optional clock*/
- host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
- if (IS_ERR(host->src_clk_cg))
- host->src_clk_cg = NULL;

host->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
"hrst");
--
2.18.0

2020-10-12 02:22:26

by Nicolas Boichat

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] mmc: mediatek: Add subsys clock control for MT8192 msdc

On Sun, Oct 11, 2020 at 5:10 PM Wenbin Mei <[email protected]> wrote:
>
> MT8192 msdc is an independent sub system, we need control more bus
> clocks for it.
> Add support for the additional subsys clocks to allow it to be
> configured appropriately.
>
> Signed-off-by: Wenbin Mei <[email protected]>
> Reviewed-by: Nicolas Boichat <[email protected]>

Err, you must not add R-by tag unless I explicitly say so (yes, I
reviewed v3, but I didn't add my R-by tag).

> ---
> drivers/mmc/host/mtk-sd.c | 80 ++++++++++++++++++++++++++++++---------
> 1 file changed, 62 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index a704745e5882..350e45432e21 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -425,6 +425,8 @@ struct msdc_host {
> struct clk *h_clk; /* msdc h_clk */
> struct clk *bus_clk; /* bus clock which used to access register */
> struct clk *src_clk_cg; /* msdc source clock control gate */
> + struct clk *sys_clk_cg; /* msdc subsys clock control gate */
> + struct clk_bulk_data bulk_clks[3]; /* pclk, axi, ahb clock control gate */
> u32 mclk; /* mmc subsystem clock frequency */
> u32 src_clk_freq; /* source clock frequency */
> unsigned char timing;
> @@ -784,6 +786,8 @@ static void msdc_set_busy_timeout(struct msdc_host *host, u64 ns, u64 clks)
>
> static void msdc_gate_clock(struct msdc_host *host)
> {
> + clk_bulk_disable_unprepare(ARRAY_SIZE(host->bulk_clks),
> + host->bulk_clks);
> clk_disable_unprepare(host->src_clk_cg);
> clk_disable_unprepare(host->src_clk);
> clk_disable_unprepare(host->bus_clk);
> @@ -792,10 +796,19 @@ static void msdc_gate_clock(struct msdc_host *host)
>
> static void msdc_ungate_clock(struct msdc_host *host)
> {
> + int ret;
> +
> clk_prepare_enable(host->h_clk);
> clk_prepare_enable(host->bus_clk);
> clk_prepare_enable(host->src_clk);
> clk_prepare_enable(host->src_clk_cg);
> + ret = clk_bulk_prepare_enable(ARRAY_SIZE(host->bulk_clks),
> + host->bulk_clks);
> + if (ret) {
> + dev_err(host->dev, "enable clks failed!\n");
> + return;
> + }
> +
> while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
> cpu_relax();
> }
> @@ -2366,6 +2379,53 @@ static void msdc_of_property_parse(struct platform_device *pdev,
> host->cqhci = false;
> }
>
> +static int msdc_of_clock_parse(struct platform_device *pdev,
> + struct msdc_host *host)
> +{
> + struct clk *clk;
> +
> + host->src_clk = devm_clk_get_optional(&pdev->dev, "source");
> + if (IS_ERR(host->src_clk))
> + return PTR_ERR(host->src_clk);
> +
> + host->h_clk = devm_clk_get_optional(&pdev->dev, "hclk");
> + if (IS_ERR(host->h_clk))
> + return PTR_ERR(host->h_clk);
> +
> + host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
> + if (IS_ERR(host->bus_clk))
> + host->bus_clk = NULL;
> +
> + /*source clock control gate is optional clock*/
> + host->src_clk_cg = devm_clk_get_optional(&pdev->dev, "source_cg");
> + if (IS_ERR(host->src_clk_cg))
> + host->src_clk_cg = NULL;
> +
> + host->sys_clk_cg = devm_clk_get_optional(&pdev->dev, "sys_cg");
> + if (IS_ERR(host->sys_clk_cg))
> + host->sys_clk_cg = NULL;
> +
> + /* If present, always enable for this clock gate */
> + clk_prepare_enable(host->sys_clk_cg);

Understand your reply on v3 (this clock gate cannot be disabled --
https://patchwork.kernel.org/patch/11808433/#23678227), but I'm still
unsure if this is the right thing to do to enable it at probe time.
I'll let others comment.

> +
> + clk = devm_clk_get_optional(&pdev->dev, "pclk_cg");
> + if (IS_ERR(clk))
> + clk = NULL;
> + host->bulk_clks[0].clk = clk;
> +
> + clk = devm_clk_get_optional(&pdev->dev, "axi_cg");
> + if (IS_ERR(clk))
> + clk = NULL;
> + host->bulk_clks[1].clk = clk;
> +
> + clk = devm_clk_get_optional(&pdev->dev, "ahb_cg");
> + if (IS_ERR(clk))
> + clk = NULL;
> + host->bulk_clks[2].clk = clk;

Put the clock names in host->bulk_clks[x].id, then call
devm_clk_bulk_get_optional.

Example here: https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpio-dwapb.c#L675

> +
> + return 0;
> +}
> +
> static int msdc_drv_probe(struct platform_device *pdev)
> {
> struct mmc_host *mmc;
> @@ -2405,25 +2465,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
> if (ret)
> goto host_free;
>
> - host->src_clk = devm_clk_get(&pdev->dev, "source");
> - if (IS_ERR(host->src_clk)) {
> - ret = PTR_ERR(host->src_clk);
> - goto host_free;
> - }
> -
> - host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> - if (IS_ERR(host->h_clk)) {
> - ret = PTR_ERR(host->h_clk);
> + ret = msdc_of_clock_parse(pdev, host);
> + if (ret)
> goto host_free;
> - }
> -
> - host->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
> - if (IS_ERR(host->bus_clk))
> - host->bus_clk = NULL;
> - /*source clock control gate is optional clock*/
> - host->src_clk_cg = devm_clk_get(&pdev->dev, "source_cg");
> - if (IS_ERR(host->src_clk_cg))
> - host->src_clk_cg = NULL;
>
> host->reset = devm_reset_control_get_optional_exclusive(&pdev->dev,
> "hrst");
> --
> 2.18.0