2022-01-13 11:57:56

by Liang Yang

[permalink] [raw]
Subject: [PATCH v9 0/4] clk: meson: add a sub EMMC clock controller support

Changes since v8 [9]
- use MESON_SCLK_ONE_BASED instead of CLK_DIVIDER_ONE_BASED
- use struct_size to caculate onecell_data
- add clk-phase-delay.h
- define CLK_DELAY_STEP_PS_GX and CLK_DELAY_STEP_PS_AXG

Changes since v7 [8]
- move meson_clk_get_phase_delay_data() from header to driver
- CONFIG sclk-div with COMMON_CLK_AMLOGIC instead of COMMON_CLK_AMLOGIC_AUDIO
- remove onecell date and ID for internal MUX clk
- use helper for functions for ONE_BASED in sclk-div
- add ONE_BASED support for duty cycle

Changes since v6 [7]:
- add one based support for sclk divier
- alloc sclk in probe for multiple instance
- fix coding styles

Changes since v5 [6]:
- remove divider ops with .init and use sclk_div instead
- drop CLK_DIVIDER_ROUND_CLOSEST in mux and div
- drop the useless type cast

Changes since v4 [5]:
- use struct parm in phase delay driver
- remove 0 delay releted part in phase delay driver
- don't rebuild the parent name once again
- add divider ops with .init

Changes since v3 [4]:
- separate clk-phase-delay driver
- replace clk_get_rate() with clk_hw_get_rate()
- collect Rob's R-Y
- drop 'meson-' prefix from compatible string

Changes since v2 [3]:
- squash dt-binding clock-id patch
- update license
- fix alignment
- construct a clk register helper() function

Changes since v1 [2]:
- implement phase clock
- update compatible name
- adjust file name
- divider probe() into small functions, and re-use them

[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]
[3] https://lkml.kernel.org/r/[email protected]
[4] https://lkml.kernel.org/r/[email protected]
[5] https://lkml.kernel.org/r/[email protected]
[6] https://lkml.kernel.org/r/[email protected]
[7] https://lkml.kernel.org/r/[email protected]
[8] https://lkml.kernel.org/r/[email protected]
[9] https://lkml.kernel.org/r/[email protected]
Liang Yang (4):
clk: meson: add one based divider support for sclk
clk: meson: add emmc sub clock phase delay driver
clk: meson: add DT documentation for emmc clock controller
clk: meson: add sub MMC clock controller driver

.../bindings/clock/amlogic,mmc-clkc.yaml | 64 ++++
drivers/clk/meson/Kconfig | 18 ++
drivers/clk/meson/Makefile | 2 +
drivers/clk/meson/clk-phase-delay.c | 69 ++++
drivers/clk/meson/clk-phase-delay.h | 20 ++
drivers/clk/meson/mmc-clkc.c | 300 ++++++++++++++++++
drivers/clk/meson/sclk-div.c | 61 ++--
drivers/clk/meson/sclk-div.h | 3 +
include/dt-bindings/clock/amlogic,mmc-clkc.h | 14 +
9 files changed, 529 insertions(+), 22 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
create mode 100644 drivers/clk/meson/clk-phase-delay.c
create mode 100644 drivers/clk/meson/clk-phase-delay.h
create mode 100644 drivers/clk/meson/mmc-clkc.c
create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h

--
2.34.1



2022-01-13 11:58:00

by Liang Yang

[permalink] [raw]
Subject: [PATCH v9 1/4] clk: meson: add one based divider support for sclk

When MESON_SCLK_ONE_BASED flag is set, the sclk divider will be:
one based divider (div = val), and zero value gates the clock

Signed-off-by: Liang Yang <[email protected]>
---
drivers/clk/meson/sclk-div.c | 61 +++++++++++++++++++++++-------------
drivers/clk/meson/sclk-div.h | 3 ++
2 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
index 76d31c0a3342..79c9efd28115 100644
--- a/drivers/clk/meson/sclk-div.c
+++ b/drivers/clk/meson/sclk-div.c
@@ -4,16 +4,17 @@
* Author: Jerome Brunet <[email protected]>
*
* Sample clock generator divider:
- * This HW divider gates with value 0 but is otherwise a zero based divider:
+ * This HW divider gates with value 0:
*
* val >= 1
- * divider = val + 1
+ * divider = val + 1 if ONE_BASED is not set, otherwise divider = val.
*
* The duty cycle may also be set for the LR clock variant. The duty cycle
* ratio is:
*
* hi = [0 - val]
- * duty_cycle = (1 + hi) / (1 + val)
+ * duty_cycle = (1 + hi) / (1 + val) if ONE_BASED is not set, otherwise:
+ * duty_cycle = hi / (1 + val)
*/

#include <linux/clk-provider.h>
@@ -28,22 +29,39 @@ meson_sclk_div_data(struct clk_regmap *clk)
return (struct meson_sclk_div_data *)clk->data;
}

-static int sclk_div_maxval(struct meson_sclk_div_data *sclk)
+static inline int sclk_get_reg(int val, unsigned char flag)
{
- return (1 << sclk->div.width) - 1;
+ if ((flag & MESON_SCLK_ONE_BASED) || !val)
+ return val;
+ else
+ return val - 1;
+}
+
+static inline int sclk_get_divider(int reg, unsigned char flag)
+{
+ if (flag & MESON_SCLK_ONE_BASED)
+ return reg;
+ else
+ return reg + 1;
}

static int sclk_div_maxdiv(struct meson_sclk_div_data *sclk)
{
- return sclk_div_maxval(sclk) + 1;
+ unsigned int reg = (1 << sclk->div.width) - 1;
+
+ return sclk_get_divider(reg, sclk->flags);
}

static int sclk_div_getdiv(struct clk_hw *hw, unsigned long rate,
- unsigned long prate, int maxdiv)
+ unsigned long prate)
{
int div = DIV_ROUND_CLOSEST_ULL((u64)prate, rate);
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
+ int mindiv = sclk_get_divider(1, sclk->flags);
+ int maxdiv = sclk_div_maxdiv(sclk);

- return clamp(div, 2, maxdiv);
+ return clamp(div, mindiv, maxdiv);
}

static int sclk_div_bestdiv(struct clk_hw *hw, unsigned long rate,
@@ -51,25 +69,25 @@ static int sclk_div_bestdiv(struct clk_hw *hw, unsigned long rate,
struct meson_sclk_div_data *sclk)
{
struct clk_hw *parent = clk_hw_get_parent(hw);
- int bestdiv = 0, i;
+ int bestdiv = 0, i, mindiv;
unsigned long maxdiv, now, parent_now;
unsigned long best = 0, best_parent = 0;

if (!rate)
rate = 1;

- maxdiv = sclk_div_maxdiv(sclk);
-
if (!(clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT))
- return sclk_div_getdiv(hw, rate, *prate, maxdiv);
+ return sclk_div_getdiv(hw, rate, *prate);

/*
* The maximum divider we can use without overflowing
* unsigned long in rate * i below
*/
+ maxdiv = sclk_div_maxdiv(sclk);
maxdiv = min(ULONG_MAX / rate, maxdiv);
+ mindiv = sclk_get_divider(1, sclk->flags);

- for (i = 2; i <= maxdiv; i++) {
+ for (i = mindiv; i <= maxdiv; i++) {
/*
* It's the most ideal case if the requested rate can be
* divided from parent clock without needing to change
@@ -115,10 +133,7 @@ static void sclk_apply_ratio(struct clk_regmap *clk,
sclk->cached_duty.num,
sclk->cached_duty.den);

- if (hi)
- hi -= 1;
-
- meson_parm_write(clk->map, &sclk->hi, hi);
+ meson_parm_write(clk->map, &sclk->hi, sclk_get_reg(hi, sclk->flags));
}

static int sclk_div_set_duty_cycle(struct clk_hw *hw,
@@ -149,7 +164,7 @@ static int sclk_div_get_duty_cycle(struct clk_hw *hw,
}

hi = meson_parm_read(clk->map, &sclk->hi);
- duty->num = hi + 1;
+ duty->num = sclk_get_divider(hi, sclk->flags);
duty->den = sclk->cached_div;
return 0;
}
@@ -157,10 +172,13 @@ static int sclk_div_get_duty_cycle(struct clk_hw *hw,
static void sclk_apply_divider(struct clk_regmap *clk,
struct meson_sclk_div_data *sclk)
{
+ unsigned int div;
+
if (MESON_PARM_APPLICABLE(&sclk->hi))
sclk_apply_ratio(clk, sclk);

- meson_parm_write(clk->map, &sclk->div, sclk->cached_div - 1);
+ div = sclk_get_reg(sclk->cached_div, sclk->flags);
+ meson_parm_write(clk->map, &sclk->div, div);
}

static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -168,9 +186,8 @@ static int sclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
{
struct clk_regmap *clk = to_clk_regmap(hw);
struct meson_sclk_div_data *sclk = meson_sclk_div_data(clk);
- unsigned long maxdiv = sclk_div_maxdiv(sclk);

- sclk->cached_div = sclk_div_getdiv(hw, rate, prate, maxdiv);
+ sclk->cached_div = sclk_div_getdiv(hw, rate, prate);

if (clk_hw_is_enabled(hw))
sclk_apply_divider(clk, sclk);
@@ -228,7 +245,7 @@ static int sclk_div_init(struct clk_hw *hw)
if (!val)
sclk->cached_div = sclk_div_maxdiv(sclk);
else
- sclk->cached_div = val + 1;
+ sclk->cached_div = sclk_get_divider(val, sclk->flags);

sclk_div_get_duty_cycle(hw, &sclk->cached_duty);

diff --git a/drivers/clk/meson/sclk-div.h b/drivers/clk/meson/sclk-div.h
index b64b2a32005f..944dab5ec0cf 100644
--- a/drivers/clk/meson/sclk-div.h
+++ b/drivers/clk/meson/sclk-div.h
@@ -10,11 +10,14 @@
#include <linux/clk-provider.h>
#include "parm.h"

+#define MESON_SCLK_ONE_BASED BIT(0)
+
struct meson_sclk_div_data {
struct parm div;
struct parm hi;
unsigned int cached_div;
struct clk_duty cached_duty;
+ u8 flags;
};

extern const struct clk_ops meson_sclk_div_ops;
--
2.34.1


2022-01-13 11:58:02

by Liang Yang

[permalink] [raw]
Subject: [PATCH v9 2/4] clk: meson: add emmc sub clock phase delay driver

Export the emmc sub clock phase delay ops which will be used
by the emmc sub clock driver itself.

Signed-off-by: Liang Yang <[email protected]>
---
drivers/clk/meson/Kconfig | 4 ++
drivers/clk/meson/Makefile | 1 +
drivers/clk/meson/clk-phase-delay.c | 69 +++++++++++++++++++++++++++++
drivers/clk/meson/clk-phase-delay.h | 20 +++++++++
4 files changed, 94 insertions(+)
create mode 100644 drivers/clk/meson/clk-phase-delay.c
create mode 100644 drivers/clk/meson/clk-phase-delay.h

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index 3014e2f1fbb4..bb0f59eea366 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -18,6 +18,10 @@ config COMMON_CLK_MESON_PHASE
tristate
select COMMON_CLK_MESON_REGMAP

+config COMMON_CLK_MESON_PHASE_DELAY
+ tristate
+ select COMMON_CLK_MESON_REGMAP
+
config COMMON_CLK_MESON_PLL
tristate
select COMMON_CLK_MESON_REGMAP
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index b3ef5f67820f..c450f38d3801 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
+obj-$(CONFIG_COMMON_CLK_MESON_PHASE_DELAY) += clk-phase-delay.o

# Amlogic Clock controllers

diff --git a/drivers/clk/meson/clk-phase-delay.c b/drivers/clk/meson/clk-phase-delay.c
new file mode 100644
index 000000000000..3c1ae0ee2a24
--- /dev/null
+++ b/drivers/clk/meson/clk-phase-delay.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+
+#include "clk-regmap.h"
+#include "clk-phase-delay.h"
+
+static inline struct meson_clk_phase_delay_data *
+meson_clk_get_phase_delay_data(struct clk_regmap *clk)
+{
+ return clk->data;
+}
+
+static int meson_clk_phase_delay_get_phase(struct clk_hw *hw)
+{
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct meson_clk_phase_delay_data *ph;
+ unsigned long period_ps, p, d;
+ int degrees;
+
+ ph = meson_clk_get_phase_delay_data(clk);
+ p = meson_parm_read(clk->map, &ph->phase);
+ degrees = p * 360 / (1 << (ph->phase.width));
+
+ period_ps = DIV_ROUND_UP_ULL(NSEC_PER_SEC * 1000ull,
+ clk_hw_get_rate(hw));
+
+ d = meson_parm_read(clk->map, &ph->delay);
+ degrees += d * ph->delay_step_ps * 360 / period_ps;
+ degrees %= 360;
+
+ return degrees;
+}
+
+static int meson_clk_phase_delay_set_phase(struct clk_hw *hw, int degrees)
+{
+ struct clk_regmap *clk = to_clk_regmap(hw);
+ struct meson_clk_phase_delay_data *ph;
+ unsigned long period_ps, d = 0;
+ unsigned int p;
+
+ ph = meson_clk_get_phase_delay_data(clk);
+ period_ps = DIV_ROUND_UP_ULL(NSEC_PER_SEC * 1000ull,
+ clk_hw_get_rate(hw));
+
+ /*
+ * First compute the phase index (p), the remainder (r) is the
+ * part we'll try to acheive using the delays (d).
+ */
+ p = 360 / 1 << (ph->phase.width);
+ degrees = degrees / p;
+ d = DIV_ROUND_CLOSEST((degrees % p) * period_ps,
+ 360 * ph->delay_step_ps);
+ d = min(d, PMASK(ph->delay.width));
+
+ meson_parm_write(clk->map, &ph->phase, degrees);
+ meson_parm_write(clk->map, &ph->delay, d);
+ return 0;
+}
+
+const struct clk_ops meson_clk_phase_delay_ops = {
+ .get_phase = meson_clk_phase_delay_get_phase,
+ .set_phase = meson_clk_phase_delay_set_phase,
+};
+EXPORT_SYMBOL_GPL(meson_clk_phase_delay_ops);
diff --git a/drivers/clk/meson/clk-phase-delay.h b/drivers/clk/meson/clk-phase-delay.h
new file mode 100644
index 000000000000..b4f211d02c84
--- /dev/null
+++ b/drivers/clk/meson/clk-phase-delay.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ */
+
+#ifndef __MESON_CLK_PHASE_DELAY_H
+#define __MESON_CLK_PHASE_DELAY_H
+
+#include <linux/clk-provider.h>
+#include "parm.h"
+
+struct meson_clk_phase_delay_data {
+ struct parm phase;
+ struct parm delay;
+ unsigned int delay_step_ps;
+};
+
+extern const struct clk_ops meson_clk_phase_delay_ops;
+
+#endif /* __MESON_CLK_PHASE_DELAY_H */
--
2.34.1


2022-01-13 11:58:05

by Liang Yang

[permalink] [raw]
Subject: [PATCH v9 3/4] clk: meson: add DT documentation for emmc clock controller

Document the MMC sub clock controller driver, the potential consumer
of this driver is MMC or NAND. Also add four clock bindings IDs which
provided by this driver.

Signed-off-by: Liang Yang <[email protected]>
---
.../bindings/clock/amlogic,mmc-clkc.yaml | 64 +++++++++++++++++++
include/dt-bindings/clock/amlogic,mmc-clkc.h | 14 ++++
2 files changed, 78 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h

diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
new file mode 100644
index 000000000000..a274c3d5fc2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic MMC Sub Clock Controller Driver Device Tree Bindings
+
+maintainers:
+ - [email protected]
+ - [email protected]
+
+properties:
+ compatible:
+ enum:
+ - "amlogic,axg-mmc-clkc", "syscon"
+ - "amlogic,gx-mmc-clkc", "syscon"
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: "clkin0", "clkin1"
+
+ "#clock-cells":
+ const: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ sd_mmc_c_clkc: [email protected] {
+ compatible = "amlogic,axg-mmc-clkc", "syscon";
+ reg = <0x0 0x7000 0x0 0x4>;
+ #clock-cells = <1>;
+
+ clock-names = "clkin0", "clkin1";
+ clocks = <&clkc CLKID_SD_MMC_C_CLK0>,
+ <&clkc CLKID_FCLK_DIV2>;
+ };
+
+ - |
+ sd_emmc_b_clkc: [email protected] {
+ compatible = "amlogic,axg-mmc-clkc", "syscon";
+ reg = <0x0 0x5000 0x0 0x4>;
+
+ #clock-cells = <1>;
+ clock-names = "clkin0", "clkin1";
+ clocks = <&clkc CLKID_SD_EMMC_B_CLK0>,
+ <&clkc CLKID_FCLK_DIV2>;
+ };
+
+...
\ No newline at end of file
diff --git a/include/dt-bindings/clock/amlogic,mmc-clkc.h b/include/dt-bindings/clock/amlogic,mmc-clkc.h
new file mode 100644
index 000000000000..71301517b183
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,mmc-clkc.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ */
+
+#ifndef __MMC_CLKC_H
+#define __MMC_CLKC_H
+
+#define CLKID_MMC_DIV 0
+#define CLKID_MMC_PHASE_CORE 1
+#define CLKID_MMC_PHASE_TX 2
+#define CLKID_MMC_PHASE_RX 3
+
+#endif
--
2.34.1


2022-01-13 11:58:10

by Liang Yang

[permalink] [raw]
Subject: [PATCH v9 4/4] clk: meson: add sub MMC clock controller driver

The patch will add a MMC clock controller driver which used by MMC or NAND,
It provide a mux and divider clock, and three phase clocks - core, tx, tx.

Two clocks are provided as the parent of MMC clock controller from
upper layer clock controller - eg "amlogic,axg-clkc" in AXG platform.

To specify which clock the MMC or NAND driver may consume,
the preprocessor macros in the dt-bindings/clock/amlogic,mmc-clkc.h header
can be used in the device tree sources.

Signed-off-by: Liang Yang <[email protected]>
---
drivers/clk/meson/Kconfig | 14 ++
drivers/clk/meson/Makefile | 1 +
drivers/clk/meson/mmc-clkc.c | 300 +++++++++++++++++++++++++++++++++++
3 files changed, 315 insertions(+)
create mode 100644 drivers/clk/meson/mmc-clkc.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index bb0f59eea366..3de6f3b24461 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -39,6 +39,20 @@ config COMMON_CLK_MESON_AO_CLKC
select COMMON_CLK_MESON_REGMAP
select RESET_CONTROLLER

+config COMMON_CLK_MMC_MESON
+ tristate "Meson MMC Sub Clock Controller Driver"
+ depends on ARCH_MESON || COMPILE_TEST
+ select MFD_SYSCON
+ select COMMON_CLK_AMLOGIC
+ select COMMON_CLK_MESON_PHASE
+ select COMMON_CLK_MESON_PHASE_DELAY
+ select COMMON_CLK_MESON_SCLK_DIV
+ help
+ Support for the MMC sub clock controller on
+ Amlogic Meson Platform, which includes S905 (GXBB, GXL),
+ A113D/X (AXG) devices . Say Y if you want this
+ clock enabled.
+
config COMMON_CLK_MESON_EE_CLKC
tristate
select COMMON_CLK_MESON_REGMAP
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index c450f38d3801..f7a94a33e789 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PHASE_DELAY) += clk-phase-delay.o

obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
+obj-$(CONFIG_COMMON_CLK_MMC_MESON) += mmc-clkc.o
obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
new file mode 100644
index 000000000000..f53977f61390
--- /dev/null
+++ b/drivers/clk/meson/mmc-clkc.c
@@ -0,0 +1,300 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/clock/amlogic,mmc-clkc.h>
+
+#include "sclk-div.h"
+#include "clk-phase-delay.h"
+#include "clk-regmap.h"
+#include "clk-phase.h"
+
+/* clock ID used by internal driver */
+
+#define SD_EMMC_CLOCK 0
+#define CLK_DELAY_STEP_PS_GX 200
+#define CLK_DELAY_STEP_PS_AXG 78
+#define MUX_CLK_NUM_PARENTS 2
+#define MMC_MAX_CLKS 4
+
+struct mmc_clkc_data {
+ struct meson_clk_phase_delay_data tx;
+ struct meson_clk_phase_delay_data rx;
+};
+
+static struct clk_regmap_mux_data mmc_clkc_mux_data = {
+ .offset = SD_EMMC_CLOCK,
+ .mask = 0x3,
+ .shift = 6,
+};
+
+static const struct meson_sclk_div_data mmc_clkc_div_data = {
+ .div = {
+ .reg_off = SD_EMMC_CLOCK,
+ .width = 6,
+ },
+ .flags = MESON_SCLK_ONE_BASED,
+};
+
+static struct meson_clk_phase_data mmc_clkc_core_phase = {
+ .ph = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 8,
+ .width = 2,
+ }
+};
+
+static const struct mmc_clkc_data mmc_clkc_gx_data = {
+ .tx = {
+ .phase = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 10,
+ .width = 2,
+ },
+ .delay = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 16,
+ .width = 4,
+ },
+ .delay_step_ps = CLK_DELAY_STEP_PS_GX,
+ },
+ .rx = {
+ .phase = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 12,
+ .width = 2,
+ },
+ .delay = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 20,
+ .width = 4,
+ },
+ .delay_step_ps = CLK_DELAY_STEP_PS_GX,
+ },
+};
+
+static const struct mmc_clkc_data mmc_clkc_axg_data = {
+ .tx = {
+ .phase = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 10,
+ .width = 2,
+ },
+ .delay = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 16,
+ .width = 6,
+ },
+ .delay_step_ps = CLK_DELAY_STEP_PS_AXG,
+ },
+ .rx = {
+ .phase = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 12,
+ .width = 2,
+ },
+ .delay = {
+ .reg_off = SD_EMMC_CLOCK,
+ .shift = 22,
+ .width = 6,
+ },
+ .delay_step_ps = CLK_DELAY_STEP_PS_AXG,
+ },
+};
+
+static const struct of_device_id mmc_clkc_match_table[] = {
+ {
+ .compatible = "amlogic,gx-mmc-clkc",
+ .data = &mmc_clkc_gx_data
+ },
+ {
+ .compatible = "amlogic,axg-mmc-clkc",
+ .data = &mmc_clkc_axg_data
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
+
+static struct clk_regmap *
+mmc_clkc_register_clk(struct device *dev, struct regmap *map,
+ struct clk_init_data *init,
+ const char *suffix, void *data)
+{
+ struct clk_regmap *clk;
+ char *name;
+ int ret;
+
+ clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
+ if (!clk)
+ return ERR_PTR(-ENOMEM);
+
+ name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
+ if (!name)
+ return ERR_PTR(-ENOMEM);
+
+ init->name = name;
+ clk->map = map;
+ clk->data = data;
+ clk->hw.init = init;
+ ret = devm_clk_hw_register(dev, &clk->hw);
+ if (ret)
+ clk = ERR_PTR(ret);
+
+ kfree(name);
+ return clk;
+}
+
+static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
+ struct regmap *map)
+{
+ const char *parent_names[MUX_CLK_NUM_PARENTS];
+ struct clk_init_data init;
+ struct clk_regmap *mux;
+ struct clk *clk;
+ int i;
+
+ for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
+ char name[8];
+
+ snprintf(name, sizeof(name), "clkin%d", i);
+ clk = devm_clk_get(dev, name);
+ if (IS_ERR(clk)) {
+ if (clk != ERR_PTR(-EPROBE_DEFER))
+ dev_err(dev, "Missing clock %s\n", name);
+ return ERR_CAST(clk);
+ }
+
+ parent_names[i] = __clk_get_name(clk);
+ }
+
+ init.ops = &clk_regmap_mux_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ init.parent_names = parent_names;
+ init.num_parents = MUX_CLK_NUM_PARENTS;
+
+ mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
+ if (IS_ERR(mux))
+ dev_err(dev, "Mux clock registration failed\n");
+
+ return mux;
+}
+
+static struct clk_regmap *
+mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
+ char *suffix, const struct clk_hw *hw,
+ unsigned long flags,
+ const struct clk_ops *ops, void *data)
+{
+ struct clk_init_data init;
+ struct clk_regmap *clk;
+ const char *parent_name = clk_hw_get_name(hw);
+
+ init.ops = ops;
+ init.flags = flags;
+ init.parent_names = &parent_name;
+ init.num_parents = 1;
+
+ clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
+ if (IS_ERR(clk))
+ dev_err(dev, "%s clock registration failed\n", suffix);
+
+ return clk;
+}
+
+static int mmc_clkc_probe(struct platform_device *pdev)
+{
+ struct clk_hw_onecell_data *onecell_data;
+ struct device *dev = &pdev->dev;
+ struct mmc_clkc_data *data;
+ struct regmap *map;
+ struct clk_regmap *clk, *core;
+ struct meson_sclk_div_data *div_data;
+
+ /*cast to drop the const in match->data*/
+ data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
+ if (!data)
+ return -ENODEV;
+
+ map = syscon_node_to_regmap(dev->of_node);
+ if (IS_ERR(map)) {
+ dev_err(dev, "could not find mmc clock controller\n");
+ return PTR_ERR(map);
+ }
+
+ onecell_data = devm_kzalloc(dev,
+ struct_size(onecell_data, hws,
+ MMC_MAX_CLKS),
+ GFP_KERNEL);
+ if (!onecell_data)
+ return -ENOMEM;
+
+ clk = mmc_clkc_register_mux(dev, map);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ div_data = devm_kzalloc(dev, sizeof(*div_data), GFP_KERNEL);
+ if (!div_data)
+ return -ENOMEM;
+
+ memcpy(div_data, &mmc_clkc_div_data, sizeof(*div_data));
+ clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
+ &clk->hw,
+ CLK_SET_RATE_PARENT,
+ &meson_sclk_div_ops,
+ div_data);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ onecell_data->hws[CLKID_MMC_DIV] = &clk->hw;
+ core = mmc_clkc_register_clk_with_parent(dev, map, "core",
+ &clk->hw,
+ CLK_SET_RATE_PARENT,
+ &meson_clk_phase_ops,
+ &mmc_clkc_core_phase);
+ if (IS_ERR(core))
+ return PTR_ERR(core);
+
+ onecell_data->hws[CLKID_MMC_PHASE_CORE] = &core->hw;
+ clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
+ &core->hw, 0,
+ &meson_clk_phase_delay_ops,
+ &data->rx);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ onecell_data->hws[CLKID_MMC_PHASE_RX] = &clk->hw;
+ clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
+ &core->hw, 0,
+ &meson_clk_phase_delay_ops,
+ &data->tx);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ onecell_data->hws[CLKID_MMC_PHASE_TX] = &clk->hw;
+ onecell_data->num = MMC_MAX_CLKS;
+ return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+ onecell_data);
+}
+
+static struct platform_driver mmc_clkc_driver = {
+ .probe = mmc_clkc_probe,
+ .driver = {
+ .name = "meson-mmc-clkc",
+ .of_match_table = of_match_ptr(mmc_clkc_match_table),
+ },
+};
+
+module_platform_driver(mmc_clkc_driver);
+
+MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
+MODULE_AUTHOR("Jianxin Pan <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.34.1


2022-01-13 21:28:06

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v9 1/4] clk: meson: add one based divider support for sclk

Quoting Liang Yang (2022-01-13 03:57:42)
> diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
> index 76d31c0a3342..79c9efd28115 100644
> --- a/drivers/clk/meson/sclk-div.c
> +++ b/drivers/clk/meson/sclk-div.c
> @@ -28,22 +29,39 @@ meson_sclk_div_data(struct clk_regmap *clk)
> return (struct meson_sclk_div_data *)clk->data;
> }
>
> -static int sclk_div_maxval(struct meson_sclk_div_data *sclk)
> +static inline int sclk_get_reg(int val, unsigned char flag)
> {
> - return (1 << sclk->div.width) - 1;
> + if ((flag & MESON_SCLK_ONE_BASED) || !val)
> + return val;
> + else
> + return val - 1;

Please drop the else

> +}
> +
> +static inline int sclk_get_divider(int reg, unsigned char flag)
> +{
> + if (flag & MESON_SCLK_ONE_BASED)
> + return reg;
> + else

Please drop the else

> + return reg + 1;
> }
>
> static int sclk_div_maxdiv(struct meson_sclk_div_data *sclk)
> {
> - return sclk_div_maxval(sclk) + 1;

2022-01-13 21:29:31

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v9 2/4] clk: meson: add emmc sub clock phase delay driver

Quoting Liang Yang (2022-01-13 03:57:43)
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index b3ef5f67820f..c450f38d3801 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
> +obj-$(CONFIG_COMMON_CLK_MESON_PHASE_DELAY) += clk-phase-delay.o

Sort by Kconfig symbol?

>
> # Amlogic Clock controllers
>

2022-01-13 21:30:02

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v9 3/4] clk: meson: add DT documentation for emmc clock controller

Quoting Liang Yang (2022-01-13 03:57:44)
> Document the MMC sub clock controller driver, the potential consumer
> of this driver is MMC or NAND. Also add four clock bindings IDs which
> provided by this driver.
>
> Signed-off-by: Liang Yang <[email protected]>
> ---
> .../bindings/clock/amlogic,mmc-clkc.yaml | 64 +++++++++++++++++++
> include/dt-bindings/clock/amlogic,mmc-clkc.h | 14 ++++
> 2 files changed, 78 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
> create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h
>
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
> new file mode 100644
> index 000000000000..a274c3d5fc2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic MMC Sub Clock Controller Driver Device Tree Bindings
> +
> +maintainers:
> + - [email protected]
> + - [email protected]
> +
> +properties:
> + compatible:
> + enum:
> + - "amlogic,axg-mmc-clkc", "syscon"

Why is it a syscon?

2022-01-13 21:35:18

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v9 4/4] clk: meson: add sub MMC clock controller driver

Quoting Liang Yang (2022-01-13 03:57:45)
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index bb0f59eea366..3de6f3b24461 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -39,6 +39,20 @@ config COMMON_CLK_MESON_AO_CLKC
> select COMMON_CLK_MESON_REGMAP
> select RESET_CONTROLLER
>
> +config COMMON_CLK_MMC_MESON
> + tristate "Meson MMC Sub Clock Controller Driver"
> + depends on ARCH_MESON || COMPILE_TEST
> + select MFD_SYSCON
> + select COMMON_CLK_AMLOGIC
> + select COMMON_CLK_MESON_PHASE
> + select COMMON_CLK_MESON_PHASE_DELAY
> + select COMMON_CLK_MESON_SCLK_DIV
> + help
> + Support for the MMC sub clock controller on
> + Amlogic Meson Platform, which includes S905 (GXBB, GXL),
> + A113D/X (AXG) devices . Say Y if you want this

s/devices /devices/

> + clock enabled.
> +
> config COMMON_CLK_MESON_EE_CLKC
> tristate
> select COMMON_CLK_MESON_REGMAP
> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> new file mode 100644
> index 000000000000..f53977f61390
> --- /dev/null
> +++ b/drivers/clk/meson/mmc-clkc.c
> @@ -0,0 +1,300 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
> +
> +#include "sclk-div.h"
> +#include "clk-phase-delay.h"
> +#include "clk-regmap.h"
> +#include "clk-phase.h"
> +
> +/* clock ID used by internal driver */
> +
> +#define SD_EMMC_CLOCK 0
> +#define CLK_DELAY_STEP_PS_GX 200
> +#define CLK_DELAY_STEP_PS_AXG 78
> +#define MUX_CLK_NUM_PARENTS 2
> +#define MMC_MAX_CLKS 4
> +
> +struct mmc_clkc_data {
> + struct meson_clk_phase_delay_data tx;
> + struct meson_clk_phase_delay_data rx;
> +};
> +
> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
> + .offset = SD_EMMC_CLOCK,
> + .mask = 0x3,
> + .shift = 6,
> +};
> +
> +static const struct meson_sclk_div_data mmc_clkc_div_data = {
> + .div = {
> + .reg_off = SD_EMMC_CLOCK,
> + .width = 6,
> + },
> + .flags = MESON_SCLK_ONE_BASED,
> +};
> +
> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
> + .ph = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 8,
> + .width = 2,
> + }
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
> + .tx = {
> + .phase = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 10,
> + .width = 2,
> + },
> + .delay = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 16,
> + .width = 4,
> + },
> + .delay_step_ps = CLK_DELAY_STEP_PS_GX,
> + },
> + .rx = {
> + .phase = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 12,
> + .width = 2,
> + },
> + .delay = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 20,
> + .width = 4,
> + },
> + .delay_step_ps = CLK_DELAY_STEP_PS_GX,
> + },
> +};
> +
> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
> + .tx = {
> + .phase = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 10,
> + .width = 2,
> + },
> + .delay = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 16,
> + .width = 6,
> + },
> + .delay_step_ps = CLK_DELAY_STEP_PS_AXG,
> + },
> + .rx = {
> + .phase = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 12,
> + .width = 2,
> + },
> + .delay = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = 22,
> + .width = 6,
> + },
> + .delay_step_ps = CLK_DELAY_STEP_PS_AXG,
> + },
> +};
> +
> +static const struct of_device_id mmc_clkc_match_table[] = {
> + {
> + .compatible = "amlogic,gx-mmc-clkc",
> + .data = &mmc_clkc_gx_data
> + },
> + {
> + .compatible = "amlogic,axg-mmc-clkc",
> + .data = &mmc_clkc_axg_data
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
> +
> +static struct clk_regmap *
> +mmc_clkc_register_clk(struct device *dev, struct regmap *map,
> + struct clk_init_data *init,
> + const char *suffix, void *data)
> +{
> + struct clk_regmap *clk;
> + char *name;
> + int ret;
> +
> + clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
> + if (!clk)
> + return ERR_PTR(-ENOMEM);
> +
> + name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
> + if (!name)
> + return ERR_PTR(-ENOMEM);
> +
> + init->name = name;
> + clk->map = map;
> + clk->data = data;
> + clk->hw.init = init;
> + ret = devm_clk_hw_register(dev, &clk->hw);
> + if (ret)
> + clk = ERR_PTR(ret);
> +
> + kfree(name);
> + return clk;
> +}
> +
> +static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
> + struct regmap *map)
> +{
> + const char *parent_names[MUX_CLK_NUM_PARENTS];
> + struct clk_init_data init;
> + struct clk_regmap *mux;
> + struct clk *clk;
> + int i;
> +
> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> + char name[8];
> +
> + snprintf(name, sizeof(name), "clkin%d", i);
> + clk = devm_clk_get(dev, name);
> + if (IS_ERR(clk)) {
> + if (clk != ERR_PTR(-EPROBE_DEFER))
> + dev_err(dev, "Missing clock %s\n", name);

Use dev_err_probe()?

> + return ERR_CAST(clk);
> + }
> +
> + parent_names[i] = __clk_get_name(clk);

Why can't we use clk_parent_data?

> + }
> +
> + init.ops = &clk_regmap_mux_ops;
> + init.flags = CLK_SET_RATE_PARENT;
> + init.parent_names = parent_names;
> + init.num_parents = MUX_CLK_NUM_PARENTS;
> +
> + mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
> + if (IS_ERR(mux))
> + dev_err(dev, "Mux clock registration failed\n");
> +
> + return mux;
> +}
> +
> +static struct clk_regmap *
> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
> + char *suffix, const struct clk_hw *hw,
> + unsigned long flags,
> + const struct clk_ops *ops, void *data)
> +{
> + struct clk_init_data init;
> + struct clk_regmap *clk;
> + const char *parent_name = clk_hw_get_name(hw);
> +
> + init.ops = ops;
> + init.flags = flags;
> + init.parent_names = &parent_name;
> + init.num_parents = 1;
> +
> + clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
> + if (IS_ERR(clk))
> + dev_err(dev, "%s clock registration failed\n", suffix);
> +
> + return clk;
> +}
> +
> +static int mmc_clkc_probe(struct platform_device *pdev)
> +{
> + struct clk_hw_onecell_data *onecell_data;
> + struct device *dev = &pdev->dev;
> + struct mmc_clkc_data *data;
> + struct regmap *map;
> + struct clk_regmap *clk, *core;
> + struct meson_sclk_div_data *div_data;
> +
> + /*cast to drop the const in match->data*/

Space after *, also why do we need to cast away const? The user of this
pointer passes it all the way down to mmc_clkc_register_clk() which
could take the data as const void pointer and decide to cast away const
there.

> + data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
> + if (!data)
> + return -ENODEV;
> +
> + map = syscon_node_to_regmap(dev->of_node);
> + if (IS_ERR(map)) {
> + dev_err(dev, "could not find mmc clock controller\n");
> + return PTR_ERR(map);
> + }
> +
> + onecell_data = devm_kzalloc(dev,
> + struct_size(onecell_data, hws,
> + MMC_MAX_CLKS),
> + GFP_KERNEL);
> + if (!onecell_data)
> + return -ENOMEM;
> +
> + clk = mmc_clkc_register_mux(dev, map);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + div_data = devm_kzalloc(dev, sizeof(*div_data), GFP_KERNEL);
> + if (!div_data)
> + return -ENOMEM;
> +
> + memcpy(div_data, &mmc_clkc_div_data, sizeof(*div_data));
> + clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
> + &clk->hw,
> + CLK_SET_RATE_PARENT,
> + &meson_sclk_div_ops,
> + div_data);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + onecell_data->hws[CLKID_MMC_DIV] = &clk->hw;
> + core = mmc_clkc_register_clk_with_parent(dev, map, "core",
> + &clk->hw,
> + CLK_SET_RATE_PARENT,
> + &meson_clk_phase_ops,
> + &mmc_clkc_core_phase);
> + if (IS_ERR(core))
> + return PTR_ERR(core);
> +
> + onecell_data->hws[CLKID_MMC_PHASE_CORE] = &core->hw;
> + clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
> + &core->hw, 0,
> + &meson_clk_phase_delay_ops,
> + &data->rx);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + onecell_data->hws[CLKID_MMC_PHASE_RX] = &clk->hw;
> + clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
> + &core->hw, 0,
> + &meson_clk_phase_delay_ops,
> + &data->tx);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + onecell_data->hws[CLKID_MMC_PHASE_TX] = &clk->hw;
> + onecell_data->num = MMC_MAX_CLKS;
> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> + onecell_data);
> +}
> +
> +static struct platform_driver mmc_clkc_driver = {
> + .probe = mmc_clkc_probe,
> + .driver = {
> + .name = "meson-mmc-clkc",
> + .of_match_table = of_match_ptr(mmc_clkc_match_table),
> + },
> +};
> +
> +module_platform_driver(mmc_clkc_driver);
> +
> +MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
> +MODULE_AUTHOR("Jianxin Pan <[email protected]>");
> +MODULE_LICENSE("GPL v2");

2022-01-14 03:06:12

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 3/4] clk: meson: add DT documentation for emmc clock controller

Hi Stephen,

Thanks for your quick response.

On 2022/1/14 5:29, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Liang Yang (2022-01-13 03:57:44)
>> Document the MMC sub clock controller driver, the potential consumer
>> of this driver is MMC or NAND. Also add four clock bindings IDs which
>> provided by this driver.
>>
>> Signed-off-by: Liang Yang <[email protected]>
>> ---
>> .../bindings/clock/amlogic,mmc-clkc.yaml | 64 +++++++++++++++++++
>> include/dt-bindings/clock/amlogic,mmc-clkc.h | 14 ++++
>> 2 files changed, 78 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
>> create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
>> new file mode 100644
>> index 000000000000..a274c3d5fc2e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
>> @@ -0,0 +1,64 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic MMC Sub Clock Controller Driver Device Tree Bindings
>> +
>> +maintainers:
>> + - [email protected]
>> + - [email protected]
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - "amlogic,axg-mmc-clkc", "syscon"
>
> Why is it a syscon?

The register documented by reg is shared with SD/eMMC controller port C,
and it need to be ops on NFC driver.

>
> .

2022-01-14 03:07:22

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 1/4] clk: meson: add one based divider support for sclk

Hi Stephen,

On 2022/1/14 5:28, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Liang Yang (2022-01-13 03:57:42)
>> diff --git a/drivers/clk/meson/sclk-div.c b/drivers/clk/meson/sclk-div.c
>> index 76d31c0a3342..79c9efd28115 100644
>> --- a/drivers/clk/meson/sclk-div.c
>> +++ b/drivers/clk/meson/sclk-div.c
>> @@ -28,22 +29,39 @@ meson_sclk_div_data(struct clk_regmap *clk)
>> return (struct meson_sclk_div_data *)clk->data;
>> }
>>
>> -static int sclk_div_maxval(struct meson_sclk_div_data *sclk)
>> +static inline int sclk_get_reg(int val, unsigned char flag)
>> {
>> - return (1 << sclk->div.width) - 1;
>> + if ((flag & MESON_SCLK_ONE_BASED) || !val)
>> + return val;
>> + else
>> + return val - 1;
>
> Please drop the else
ok
>
>> +}
>> +
>> +static inline int sclk_get_divider(int reg, unsigned char flag)
>> +{
>> + if (flag & MESON_SCLK_ONE_BASED)
>> + return reg;
>> + else
>
> Please drop the else
ok
>
>> + return reg + 1;
>> }
>>
>> static int sclk_div_maxdiv(struct meson_sclk_div_data *sclk)
>> {
>> - return sclk_div_maxval(sclk) + 1;
>
> .

2022-01-14 03:08:08

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 2/4] clk: meson: add emmc sub clock phase delay driver



On 2022/1/14 5:29, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Liang Yang (2022-01-13 03:57:43)
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index b3ef5f67820f..c450f38d3801 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
>> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
>> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
>> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
>> +obj-$(CONFIG_COMMON_CLK_MESON_PHASE_DELAY) += clk-phase-delay.o
>
> Sort by Kconfig symbol?
ok, it will fix it.
>
>>
>> # Amlogic Clock controllers
>>
>
> .

2022-01-14 05:14:52

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 4/4] clk: meson: add sub MMC clock controller driver

Hi Stephen,

On 2022/1/14 5:35, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Liang Yang (2022-01-13 03:57:45)
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index bb0f59eea366..3de6f3b24461 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -39,6 +39,20 @@ config COMMON_CLK_MESON_AO_CLKC
>> select COMMON_CLK_MESON_REGMAP
>> select RESET_CONTROLLER
>>
>> +config COMMON_CLK_MMC_MESON
>> + tristate "Meson MMC Sub Clock Controller Driver"
>> + depends on ARCH_MESON || COMPILE_TEST
>> + select MFD_SYSCON
>> + select COMMON_CLK_AMLOGIC
>> + select COMMON_CLK_MESON_PHASE
>> + select COMMON_CLK_MESON_PHASE_DELAY
>> + select COMMON_CLK_MESON_SCLK_DIV
>> + help
>> + Support for the MMC sub clock controller on
>> + Amlogic Meson Platform, which includes S905 (GXBB, GXL),
>> + A113D/X (AXG) devices . Say Y if you want this
>
> s/devices /devices/
ok, i will fix it
>
>> + clock enabled.
>> +
>> config COMMON_CLK_MESON_EE_CLKC
>> tristate
>> select COMMON_CLK_MESON_REGMAP
>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>> new file mode 100644
>> index 000000000000..f53977f61390
>> --- /dev/null
>> +++ b/drivers/clk/meson/mmc-clkc.c
>> @@ -0,0 +1,300 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <dt-bindings/clock/amlogic,mmc-clkc.h>
>> +
>> +#include "sclk-div.h"
>> +#include "clk-phase-delay.h"
>> +#include "clk-regmap.h"
>> +#include "clk-phase.h"
>> +
>> +/* clock ID used by internal driver */
>> +
>> +#define SD_EMMC_CLOCK 0
>> +#define CLK_DELAY_STEP_PS_GX 200
>> +#define CLK_DELAY_STEP_PS_AXG 78
>> +#define MUX_CLK_NUM_PARENTS 2
>> +#define MMC_MAX_CLKS 4
>> +
>> +struct mmc_clkc_data {
>> + struct meson_clk_phase_delay_data tx;
>> + struct meson_clk_phase_delay_data rx;
>> +};
>> +
>> +static struct clk_regmap_mux_data mmc_clkc_mux_data = {
>> + .offset = SD_EMMC_CLOCK,
>> + .mask = 0x3,
>> + .shift = 6,
>> +};
>> +
>> +static const struct meson_sclk_div_data mmc_clkc_div_data = {
>> + .div = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .width = 6,
>> + },
>> + .flags = MESON_SCLK_ONE_BASED,
>> +};
>> +
>> +static struct meson_clk_phase_data mmc_clkc_core_phase = {
>> + .ph = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 8,
>> + .width = 2,
>> + }
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_gx_data = {
>> + .tx = {
>> + .phase = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 10,
>> + .width = 2,
>> + },
>> + .delay = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 16,
>> + .width = 4,
>> + },
>> + .delay_step_ps = CLK_DELAY_STEP_PS_GX,
>> + },
>> + .rx = {
>> + .phase = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 12,
>> + .width = 2,
>> + },
>> + .delay = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 20,
>> + .width = 4,
>> + },
>> + .delay_step_ps = CLK_DELAY_STEP_PS_GX,
>> + },
>> +};
>> +
>> +static const struct mmc_clkc_data mmc_clkc_axg_data = {
>> + .tx = {
>> + .phase = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 10,
>> + .width = 2,
>> + },
>> + .delay = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 16,
>> + .width = 6,
>> + },
>> + .delay_step_ps = CLK_DELAY_STEP_PS_AXG,
>> + },
>> + .rx = {
>> + .phase = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 12,
>> + .width = 2,
>> + },
>> + .delay = {
>> + .reg_off = SD_EMMC_CLOCK,
>> + .shift = 22,
>> + .width = 6,
>> + },
>> + .delay_step_ps = CLK_DELAY_STEP_PS_AXG,
>> + },
>> +};
>> +
>> +static const struct of_device_id mmc_clkc_match_table[] = {
>> + {
>> + .compatible = "amlogic,gx-mmc-clkc",
>> + .data = &mmc_clkc_gx_data
>> + },
>> + {
>> + .compatible = "amlogic,axg-mmc-clkc",
>> + .data = &mmc_clkc_axg_data
>> + },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, mmc_clkc_match_table);
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_clk(struct device *dev, struct regmap *map,
>> + struct clk_init_data *init,
>> + const char *suffix, void *data)
>> +{
>> + struct clk_regmap *clk;
>> + char *name;
>> + int ret;
>> +
>> + clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
>> + if (!clk)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + name = kasprintf(GFP_KERNEL, "%s#%s", dev_name(dev), suffix);
>> + if (!name)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init->name = name;
>> + clk->map = map;
>> + clk->data = data;
>> + clk->hw.init = init;
>> + ret = devm_clk_hw_register(dev, &clk->hw);
>> + if (ret)
>> + clk = ERR_PTR(ret);
>> +
>> + kfree(name);
>> + return clk;
>> +}
>> +
>> +static struct clk_regmap *mmc_clkc_register_mux(struct device *dev,
>> + struct regmap *map)
>> +{
>> + const char *parent_names[MUX_CLK_NUM_PARENTS];
>> + struct clk_init_data init;
>> + struct clk_regmap *mux;
>> + struct clk *clk;
>> + int i;
>> +
>> + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> + char name[8];
>> +
>> + snprintf(name, sizeof(name), "clkin%d", i);
>> + clk = devm_clk_get(dev, name);
>> + if (IS_ERR(clk)) {
>> + if (clk != ERR_PTR(-EPROBE_DEFER))
>> + dev_err(dev, "Missing clock %s\n", name);
>
> Use dev_err_probe()?
ok
>
>> + return ERR_CAST(clk);
>> + }
>> +
>> + parent_names[i] = __clk_get_name(clk);
>
> Why can't we use clk_parent_data?

ok, i will try clk_parent_data.

>
>> + }
>> +
>> + init.ops = &clk_regmap_mux_ops;
>> + init.flags = CLK_SET_RATE_PARENT;
>> + init.parent_names = parent_names;
>> + init.num_parents = MUX_CLK_NUM_PARENTS;
>> +
>> + mux = mmc_clkc_register_clk(dev, map, &init, "mux", &mmc_clkc_mux_data);
>> + if (IS_ERR(mux))
>> + dev_err(dev, "Mux clock registration failed\n");
>> +
>> + return mux;
>> +}
>> +
>> +static struct clk_regmap *
>> +mmc_clkc_register_clk_with_parent(struct device *dev, struct regmap *map,
>> + char *suffix, const struct clk_hw *hw,
>> + unsigned long flags,
>> + const struct clk_ops *ops, void *data)
>> +{
>> + struct clk_init_data init;
>> + struct clk_regmap *clk;
>> + const char *parent_name = clk_hw_get_name(hw);
>> +
>> + init.ops = ops;
>> + init.flags = flags;
>> + init.parent_names = &parent_name;
>> + init.num_parents = 1;
>> +
>> + clk = mmc_clkc_register_clk(dev, map, &init, suffix, data);
>> + if (IS_ERR(clk))
>> + dev_err(dev, "%s clock registration failed\n", suffix);
>> +
>> + return clk;
>> +}
>> +
>> +static int mmc_clkc_probe(struct platform_device *pdev)
>> +{
>> + struct clk_hw_onecell_data *onecell_data;
>> + struct device *dev = &pdev->dev;
>> + struct mmc_clkc_data *data;
>> + struct regmap *map;
>> + struct clk_regmap *clk, *core;
>> + struct meson_sclk_div_data *div_data;
>> +
>> + /*cast to drop the const in match->data*/
>
> Space after *, also why do we need to cast away const? The user of this
> pointer passes it all the way down to mmc_clkc_register_clk() which
> could take the data as const void pointer and decide to cast away const
> there.

if use 'const' here, it will report a warning:
drivers/clk/meson/mmc-clkc.c:224:7: error: assignment discards ‘const’
qualifier from pointer targe
t type [-Werror=discarded-qualifiers]

data = (const struct mmc_clkc_data *)of_device_get_match_data(dev);

>
>> + data = (struct mmc_clkc_data *)of_device_get_match_data(dev);
>> + if (!data)
>> + return -ENODEV;
>> +
>> + map = syscon_node_to_regmap(dev->of_node);
>> + if (IS_ERR(map)) {
>> + dev_err(dev, "could not find mmc clock controller\n");
>> + return PTR_ERR(map);
>> + }
>> +
>> + onecell_data = devm_kzalloc(dev,
>> + struct_size(onecell_data, hws,
>> + MMC_MAX_CLKS),
>> + GFP_KERNEL);
>> + if (!onecell_data)
>> + return -ENOMEM;
>> +
>> + clk = mmc_clkc_register_mux(dev, map);
>> + if (IS_ERR(clk))
>> + return PTR_ERR(clk);
>> +
>> + div_data = devm_kzalloc(dev, sizeof(*div_data), GFP_KERNEL);
>> + if (!div_data)
>> + return -ENOMEM;
>> +
>> + memcpy(div_data, &mmc_clkc_div_data, sizeof(*div_data));
>> + clk = mmc_clkc_register_clk_with_parent(dev, map, "div",
>> + &clk->hw,
>> + CLK_SET_RATE_PARENT,
>> + &meson_sclk_div_ops,
>> + div_data);
>> + if (IS_ERR(clk))
>> + return PTR_ERR(clk);
>> +
>> + onecell_data->hws[CLKID_MMC_DIV] = &clk->hw;
>> + core = mmc_clkc_register_clk_with_parent(dev, map, "core",
>> + &clk->hw,
>> + CLK_SET_RATE_PARENT,
>> + &meson_clk_phase_ops,
>> + &mmc_clkc_core_phase);
>> + if (IS_ERR(core))
>> + return PTR_ERR(core);
>> +
>> + onecell_data->hws[CLKID_MMC_PHASE_CORE] = &core->hw;
>> + clk = mmc_clkc_register_clk_with_parent(dev, map, "rx",
>> + &core->hw, 0,
>> + &meson_clk_phase_delay_ops,
>> + &data->rx);
>> + if (IS_ERR(clk))
>> + return PTR_ERR(clk);
>> +
>> + onecell_data->hws[CLKID_MMC_PHASE_RX] = &clk->hw;
>> + clk = mmc_clkc_register_clk_with_parent(dev, map, "tx",
>> + &core->hw, 0,
>> + &meson_clk_phase_delay_ops,
>> + &data->tx);
>> + if (IS_ERR(clk))
>> + return PTR_ERR(clk);
>> +
>> + onecell_data->hws[CLKID_MMC_PHASE_TX] = &clk->hw;
>> + onecell_data->num = MMC_MAX_CLKS;
>> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> + onecell_data);
>> +}
>> +
>> +static struct platform_driver mmc_clkc_driver = {
>> + .probe = mmc_clkc_probe,
>> + .driver = {
>> + .name = "meson-mmc-clkc",
>> + .of_match_table = of_match_ptr(mmc_clkc_match_table),
>> + },
>> +};
>> +
>> +module_platform_driver(mmc_clkc_driver);
>> +
>> +MODULE_DESCRIPTION("Amlogic AXG MMC clock driver");
>> +MODULE_AUTHOR("Jianxin Pan <[email protected]>");
>> +MODULE_LICENSE("GPL v2");
>
> .

2022-01-15 00:57:27

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v9 3/4] clk: meson: add DT documentation for emmc clock controller

Quoting Liang Yang (2022-01-13 19:06:07)
> Hi Stephen,
>
> Thanks for your quick response.
>
> On 2022/1/14 5:29, Stephen Boyd wrote:
> > [ EXTERNAL EMAIL ]
> >
> > Quoting Liang Yang (2022-01-13 03:57:44)
> >> Document the MMC sub clock controller driver, the potential consumer
> >> of this driver is MMC or NAND. Also add four clock bindings IDs which
> >> provided by this driver.
> >>
> >> Signed-off-by: Liang Yang <[email protected]>
> >> ---
> >> .../bindings/clock/amlogic,mmc-clkc.yaml | 64 +++++++++++++++++++
> >> include/dt-bindings/clock/amlogic,mmc-clkc.h | 14 ++++
> >> 2 files changed, 78 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
> >> create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
> >> new file mode 100644
> >> index 000000000000..a274c3d5fc2e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
> >> @@ -0,0 +1,64 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Amlogic MMC Sub Clock Controller Driver Device Tree Bindings
> >> +
> >> +maintainers:
> >> + - [email protected]
> >> + - [email protected]
> >> +
> >> +properties:
> >> + compatible:
> >> + enum:
> >> + - "amlogic,axg-mmc-clkc", "syscon"
> >
> > Why is it a syscon?
>
> The register documented by reg is shared with SD/eMMC controller port C,
> and it need to be ops on NFC driver.
>

Is this the case where the clk is inside the SD/eMMC controller? Can the
mmc driver register the clk controller from there and pass it an iomem
pointer to poke clks?

2022-01-15 00:57:52

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v9 4/4] clk: meson: add sub MMC clock controller driver

Quoting Liang Yang (2022-01-13 21:14:46)
> On 2022/1/14 5:35, Stephen Boyd wrote:
> > Quoting Liang Yang (2022-01-13 03:57:45)
> >> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> >> new file mode 100644
> >> index 000000000000..f53977f61390
> >> --- /dev/null
> >> +++ b/drivers/clk/meson/mmc-clkc.c
> >> @@ -0,0 +1,300 @@
[..]
> >> +
> >> +static int mmc_clkc_probe(struct platform_device *pdev)
> >> +{
> >> + struct clk_hw_onecell_data *onecell_data;
> >> + struct device *dev = &pdev->dev;
> >> + struct mmc_clkc_data *data;
> >> + struct regmap *map;
> >> + struct clk_regmap *clk, *core;
> >> + struct meson_sclk_div_data *div_data;
> >> +
> >> + /*cast to drop the const in match->data*/
> >
> > Space after *, also why do we need to cast away const? The user of this
> > pointer passes it all the way down to mmc_clkc_register_clk() which
> > could take the data as const void pointer and decide to cast away const
> > there.
>
> if use 'const' here, it will report a warning:
> drivers/clk/meson/mmc-clkc.c:224:7: error: assignment discards ‘const’
> qualifier from pointer targe
> t type [-Werror=discarded-qualifiers]
>
> data = (const struct mmc_clkc_data *)of_device_get_match_data(dev);

Of course. The type declaration up above needs const added to it.

2022-01-15 07:38:46

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCH v9 3/4] clk: meson: add DT documentation for emmc clock controller

Hi Liang,

On Fri, Jan 14, 2022 at 4:06 AM Liang Yang <[email protected]> wrote:
[...]
> >> +properties:
> >> + compatible:
> >> + enum:
> >> + - "amlogic,axg-mmc-clkc", "syscon"
> >
> > Why is it a syscon?
>
> The register documented by reg is shared with SD/eMMC controller port C,
> and it need to be ops on NFC driver.
Can you please share an example how the .dts would look like in the
end for two example cases:
1) using the sd_emmc_c MMC interface
2) not using the MMC interface of sd_emmc_c but only using it's clocks
for the NFC

Initially I wanted to suggest the usage of a sub-node with compatible
= "mmc-slot" (see
Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt for an
example) to decide whether the MMC controller is registered or whether
just the clock controller is registered.
However, the mmc-slot compatible string is not documented in
Documentation/devicetree/bindings/mmc/mmc-controller.yaml currently.
Maybe the MMC maintainers have any suggestions if other IPs have
similar requirements (for IPs with shared parts).


Best regards,
Martin

2022-01-17 11:57:18

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 3/4] clk: meson: add DT documentation for emmc clock controller



On 2022/1/15 6:59, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Liang Yang (2022-01-13 19:06:07)
>> Hi Stephen,
>>
>> Thanks for your quick response.
>>
>> On 2022/1/14 5:29, Stephen Boyd wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Quoting Liang Yang (2022-01-13 03:57:44)
>>>> Document the MMC sub clock controller driver, the potential consumer
>>>> of this driver is MMC or NAND. Also add four clock bindings IDs which
>>>> provided by this driver.
>>>>
>>>> Signed-off-by: Liang Yang <[email protected]>
>>>> ---
>>>> .../bindings/clock/amlogic,mmc-clkc.yaml | 64 +++++++++++++++++++
>>>> include/dt-bindings/clock/amlogic,mmc-clkc.h | 14 ++++
>>>> 2 files changed, 78 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
>>>> create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
>>>> new file mode 100644
>>>> index 000000000000..a274c3d5fc2e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
>>>> @@ -0,0 +1,64 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/clock/amlogic,mmc-clkc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Amlogic MMC Sub Clock Controller Driver Device Tree Bindings
>>>> +
>>>> +maintainers:
>>>> + - [email protected]
>>>> + - [email protected]
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + enum:
>>>> + - "amlogic,axg-mmc-clkc", "syscon"
>>>
>>> Why is it a syscon?
>>
>> The register documented by reg is shared with SD/eMMC controller port C,
>> and it need to be ops on NFC driver.
>>
>
> Is this the case where the clk is inside the SD/eMMC controller? Can the
yes.
> mmc driver register the clk controller from there and pass it an iomem
> pointer to poke clks?
we can't do that since EMMC and NAND is mutually exclusivem. both of
them share the same data pins.
>
> .

2022-01-17 14:31:55

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 4/4] clk: meson: add sub MMC clock controller driver



On 2022/1/15 7:01, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Liang Yang (2022-01-13 21:14:46)
>> On 2022/1/14 5:35, Stephen Boyd wrote:
>>> Quoting Liang Yang (2022-01-13 03:57:45)
>>>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>>>> new file mode 100644
>>>> index 000000000000..f53977f61390
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/mmc-clkc.c
>>>> @@ -0,0 +1,300 @@
> [..]
>>>> +
>>>> +static int mmc_clkc_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct clk_hw_onecell_data *onecell_data;
>>>> + struct device *dev = &pdev->dev;
>>>> + struct mmc_clkc_data *data;
>>>> + struct regmap *map;
>>>> + struct clk_regmap *clk, *core;
>>>> + struct meson_sclk_div_data *div_data;
>>>> +
>>>> + /*cast to drop the const in match->data*/
>>>
>>> Space after *, also why do we need to cast away const? The user of this
>>> pointer passes it all the way down to mmc_clkc_register_clk() which
>>> could take the data as const void pointer and decide to cast away const
>>> there.
>>
>> if use 'const' here, it will report a warning:
>> drivers/clk/meson/mmc-clkc.c:224:7: error: assignment discards ‘const’
>> qualifier from pointer targe
>> t type [-Werror=discarded-qualifiers]
>>
>> data = (const struct mmc_clkc_data *)of_device_get_match_data(dev);
>
> Of course. The type declaration up above needs const added to it.The parm of mmc_clkc_register_clk_with_parent(...., void *data) does not
have 'const', so make the type declaration cause a further 'const' cast
warning. Could i copy these infos just like below:

struct mmc_clkc_data *data_clkc;
......
data = of_device_get_match_data(dev);
data_clkc = devm_kmalloc(dev, sizeof(*data_clkc), GFP_KERNEL);
memcpy(data_clkc, data, sizeof(*data_clkc));
......
clk = mmc_clkc_register_clk_with_parent(...., &data_clkc->rx);

>
> .

2022-01-17 15:01:08

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 3/4] clk: meson: add DT documentation for emmc clock controller

Hi Martin,

On 2022/1/15 8:09, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Liang,
>
> On Fri, Jan 14, 2022 at 4:06 AM Liang Yang <[email protected]> wrote:
> [...]
>>>> +properties:
>>>> + compatible:
>>>> + enum:
>>>> + - "amlogic,axg-mmc-clkc", "syscon"
>>>
>>> Why is it a syscon?
>>
>> The register documented by reg is shared with SD/eMMC controller port C,
>> and it need to be ops on NFC driver.
> Can you please share an example how the .dts would look like in the
> end for two example cases:
> 1) using the sd_emmc_c MMC interface > 2) not using the MMC interface of sd_emmc_c but only using it's clocks
> for the NFC
I think both EMMC(although it is not implemented by 'axg-mmc-clkc' yet)
and NAND clock have the same way in dts:
......
clocks = <&clkc CLKID_SD_EMMC_C>,
<&sd_emmc_c_clkc CLKID_MMC_DIV>,
<&sd_emmc_c_clkc CLKID_MMC_PHASE_RX>,
<&sd_emmc_c_clkc CLKID_MMC_PHASE_TX>;
clock-names = "core", "device", "rx", "tx";
amlogic,mmc-syscon = <&sd_emmc_c_clkc>;
......

>
> Initially I wanted to suggest the usage of a sub-node with compatible
> = "mmc-slot" (see
> Documentation/devicetree/bindings/mmc/amlogic,meson-mx-sdio.txt for an
> example) to decide whether the MMC controller is registered or whether
> just the clock controller is registered.
> However, the mmc-slot compatible string is not documented in
> Documentation/devicetree/bindings/mmc/mmc-controller.yaml currently.
> Maybe the MMC maintainers have any suggestions if other IPs have
> similar requirements (for IPs with shared parts).
>
>
> Best regards,
> Martin
>
> .

2022-01-21 14:05:32

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v9 4/4] clk: meson: add sub MMC clock controller driver

Quoting Liang Yang (2022-01-16 22:24:28)
>
>
> On 2022/1/15 7:01, Stephen Boyd wrote:
> > [ EXTERNAL EMAIL ]
> >
> > Quoting Liang Yang (2022-01-13 21:14:46)
> >> On 2022/1/14 5:35, Stephen Boyd wrote:
> >>> Quoting Liang Yang (2022-01-13 03:57:45)
> >>>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
> >>>> new file mode 100644
> >>>> index 000000000000..f53977f61390
> >>>> --- /dev/null
> >>>> +++ b/drivers/clk/meson/mmc-clkc.c
> >>>> @@ -0,0 +1,300 @@
> > [..]
> >>>> +
> >>>> +static int mmc_clkc_probe(struct platform_device *pdev)
> >>>> +{
> >>>> + struct clk_hw_onecell_data *onecell_data;
> >>>> + struct device *dev = &pdev->dev;
> >>>> + struct mmc_clkc_data *data;
> >>>> + struct regmap *map;
> >>>> + struct clk_regmap *clk, *core;
> >>>> + struct meson_sclk_div_data *div_data;
> >>>> +
> >>>> + /*cast to drop the const in match->data*/
> >>>
> >>> Space after *, also why do we need to cast away const? The user of this
> >>> pointer passes it all the way down to mmc_clkc_register_clk() which
> >>> could take the data as const void pointer and decide to cast away const
> >>> there.
> >>
> >> if use 'const' here, it will report a warning:
> >> drivers/clk/meson/mmc-clkc.c:224:7: error: assignment discards ‘const’
> >> qualifier from pointer targe
> >> t type [-Werror=discarded-qualifiers]
> >>
> >> data = (const struct mmc_clkc_data *)of_device_get_match_data(dev);
> >
> > Of course. The type declaration up above needs const added to it.The parm of mmc_clkc_register_clk_with_parent(...., void *data) does not
> have 'const', so make the type declaration cause a further 'const' cast
> warning. Could i copy these infos just like below:

Why can't you push const down to the function that really cares to
remove const?

2022-01-21 14:30:32

by Liang Yang

[permalink] [raw]
Subject: Re: [PATCH v9 4/4] clk: meson: add sub MMC clock controller driver

Hi Stephen,

On 2022/1/19 10:22, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Liang Yang (2022-01-16 22:24:28)
>>
>>
>> On 2022/1/15 7:01, Stephen Boyd wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Quoting Liang Yang (2022-01-13 21:14:46)
>>>> On 2022/1/14 5:35, Stephen Boyd wrote:
>>>>> Quoting Liang Yang (2022-01-13 03:57:45)
>>>>>> diff --git a/drivers/clk/meson/mmc-clkc.c b/drivers/clk/meson/mmc-clkc.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..f53977f61390
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/clk/meson/mmc-clkc.c
>>>>>> @@ -0,0 +1,300 @@
>>> [..]
>>>>>> +
>>>>>> +static int mmc_clkc_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> + struct clk_hw_onecell_data *onecell_data;
>>>>>> + struct device *dev = &pdev->dev;
>>>>>> + struct mmc_clkc_data *data;
>>>>>> + struct regmap *map;
>>>>>> + struct clk_regmap *clk, *core;
>>>>>> + struct meson_sclk_div_data *div_data;
>>>>>> +
>>>>>> + /*cast to drop the const in match->data*/
>>>>>
>>>>> Space after *, also why do we need to cast away const? The user of this
>>>>> pointer passes it all the way down to mmc_clkc_register_clk() which
>>>>> could take the data as const void pointer and decide to cast away const
>>>>> there.
>>>>
>>>> if use 'const' here, it will report a warning:
>>>> drivers/clk/meson/mmc-clkc.c:224:7: error: assignment discards ‘const’
>>>> qualifier from pointer targe
>>>> t type [-Werror=discarded-qualifiers]
>>>>
>>>> data = (const struct mmc_clkc_data *)of_device_get_match_data(dev);
>>>
>>> Of course. The type declaration up above needs const added to it.The parm of mmc_clkc_register_clk_with_parent(...., void *data) does not
>> have 'const', so make the type declaration cause a further 'const' cast
>> warning. Could i copy these infos just like below:
>
> Why can't you push const down to the function that really cares to
> remove const?
em, it is really ok to me. i will fix it next version.
>
> .