[PATCH v3 0/4] add slow clock support for SAM9X60

[PATCH v3 0/4] add slow clock support for SAM9X60

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Hi,

This series add slow clock support for SAM9X60. Apart from previous IPs, this
one uses different offsets in control register for different functionalities.
The series adapt current driver to work for all IPs using per IP
configurations initialized at probe.

Thank you,
Claudiu Beznea

Changes in v3:
- add patch 1/1 that remove bypass code in the code specific to SAMA5D4
  (there is no bypass support on SAMA5D4)
- adapt review comments
- register clock with of_clk_hw_onecell_get to emphasize that this IP has
  2 output clocks MD_SLKC and TD_SLCK (I considered not necessary to
  introduce new constants to be shared b/w driver and DT bindings; if
  you consider otherwise, let me know)
- adapt dt-binding patch with clock-cells changes (thus didn't introduced
  Reviewed-by tag)
- renamed struct clk_slow_offsets to struct clk_slow_bits and the
  corresponding instances of it

Changes in v2:
- split patch 1/1 from v1 in 2 patches: one adding register bit offsets
  support (patch 1/3 from this series), one adding support for SAM9X60
  (patch 2/3 from this series)
- fix compatible string from "microchip,at91sam9x60-sckc" to
  "microchip,sam9x60-sckc"

Claudiu Beznea (4):
  clk: at91: sckc: sama5d4 has no bypass support
  clk: at91: sckc: add support to specify registers bit offsets
  dt-bindings: clk: at91: add bindings for SAM9X60's slow clock
    controller
  clk: at91: sckc: add support for SAM9X60

 .../devicetree/bindings/clock/at91-clock.txt       |   7 +-
 drivers/clk/at91/sckc.c                            | 180 ++++++++++++++++-----
 2 files changed, 145 insertions(+), 42 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] clk: at91: sckc: sama5d4 has no bypass support

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

The slow clock of SAMA5D4 has no bypass support thus remove it.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/clk/at91/sckc.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
index e76b1d64e905..6c55a7a86f79 100644
--- a/drivers/clk/at91/sckc.c
+++ b/drivers/clk/at91/sckc.c
@@ -429,7 +429,6 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
 	struct clk_init_data init;
 	const char *xtal_name;
 	const char *parent_names[2] = { "slow_rc_osc", "slow_osc" };
-	bool bypass;
 	int ret;
 
 	if (!regbase)
@@ -443,8 +442,6 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
 
 	xtal_name = of_clk_get_parent_name(np, 0);
 
-	bypass = of_property_read_bool(np, "atmel,osc-bypass");
-
 	osc = kzalloc(sizeof(*osc), GFP_KERNEL);
 	if (!osc)
 		return;
@@ -459,9 +456,6 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
 	osc->sckcr = regbase;
 	osc->startup_usec = 1200000;
 
-	if (bypass)
-		writel((readl(regbase) | AT91_SCKC_OSC32BYP), regbase);
-
 	hw = &osc->hw;
 	ret = clk_hw_register(NULL, &osc->hw);
 	if (ret) {
-- 
2.7.4

[PATCH v3 2/4] clk: at91: sckc: add support to specify registers bit offsets

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Different IPs uses different bit offsets in registers for the same
functionality, thus adapt the driver to support this.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/clk/at91/sckc.c | 100 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
index 6c55a7a86f79..2a4ac548de80 100644
--- a/drivers/clk/at91/sckc.c
+++ b/drivers/clk/at91/sckc.c
@@ -22,15 +22,23 @@
 #define SLOWCK_SW_TIME_USEC	((SLOWCK_SW_CYCLES * USEC_PER_SEC) / \
 				 SLOW_CLOCK_FREQ)
 
-#define	AT91_SCKC_CR			0x00
-#define		AT91_SCKC_RCEN		(1 << 0)
-#define		AT91_SCKC_OSC32EN	(1 << 1)
-#define		AT91_SCKC_OSC32BYP	(1 << 2)
-#define		AT91_SCKC_OSCSEL	(1 << 3)
+#define	AT91_SCKC_CR		0x00
+#define		AT91_SCKC_RCEN(off)	((off)->cr_rcen)
+#define		AT91_SCKC_OSC32EN(off)	((off)->cr_osc32en)
+#define		AT91_SCKC_OSC32BYP(off)	((off)->cr_osc32byp)
+#define		AT91_SCKC_OSCSEL(off)	((off)->cr_oscsel)
+
+struct clk_slow_bits {
+	u32 cr_rcen;
+	u32 cr_osc32en;
+	u32 cr_osc32byp;
+	u32 cr_oscsel;
+};
 
 struct clk_slow_osc {
 	struct clk_hw hw;
 	void __iomem *sckcr;
+	const struct clk_slow_bits *bits;
 	unsigned long startup_usec;
 };
 
@@ -39,6 +47,7 @@ struct clk_slow_osc {
 struct clk_sama5d4_slow_osc {
 	struct clk_hw hw;
 	void __iomem *sckcr;
+	const struct clk_slow_bits *bits;
 	unsigned long startup_usec;
 	bool prepared;
 };
@@ -48,6 +57,7 @@ struct clk_sama5d4_slow_osc {
 struct clk_slow_rc_osc {
 	struct clk_hw hw;
 	void __iomem *sckcr;
+	const struct clk_slow_bits *bits;
 	unsigned long frequency;
 	unsigned long accuracy;
 	unsigned long startup_usec;
@@ -58,6 +68,7 @@ struct clk_slow_rc_osc {
 struct clk_sam9x5_slow {
 	struct clk_hw hw;
 	void __iomem *sckcr;
+	const struct clk_slow_bits *bits;
 	u8 parent;
 };
 
@@ -69,10 +80,11 @@ static int clk_slow_osc_prepare(struct clk_hw *hw)
 	void __iomem *sckcr = osc->sckcr;
 	u32 tmp = readl(sckcr);
 
-	if (tmp & (AT91_SCKC_OSC32BYP | AT91_SCKC_OSC32EN))
+	if (tmp & (AT91_SCKC_OSC32BYP(osc->bits) |
+		   AT91_SCKC_OSC32EN(osc->bits)))
 		return 0;
 
-	writel(tmp | AT91_SCKC_OSC32EN, sckcr);
+	writel(tmp | AT91_SCKC_OSC32EN(osc->bits), sckcr);
 
 	usleep_range(osc->startup_usec, osc->startup_usec + 1);
 
@@ -85,10 +97,10 @@ static void clk_slow_osc_unprepare(struct clk_hw *hw)
 	void __iomem *sckcr = osc->sckcr;
 	u32 tmp = readl(sckcr);
 
-	if (tmp & AT91_SCKC_OSC32BYP)
+	if (tmp & AT91_SCKC_OSC32BYP(osc->bits))
 		return;
 
-	writel(tmp & ~AT91_SCKC_OSC32EN, sckcr);
+	writel(tmp & ~AT91_SCKC_OSC32EN(osc->bits), sckcr);
 }
 
 static int clk_slow_osc_is_prepared(struct clk_hw *hw)
@@ -97,10 +109,10 @@ static int clk_slow_osc_is_prepared(struct clk_hw *hw)
 	void __iomem *sckcr = osc->sckcr;
 	u32 tmp = readl(sckcr);
 
-	if (tmp & AT91_SCKC_OSC32BYP)
+	if (tmp & AT91_SCKC_OSC32BYP(osc->bits))
 		return 1;
 
-	return !!(tmp & AT91_SCKC_OSC32EN);
+	return !!(tmp & AT91_SCKC_OSC32EN(osc->bits));
 }
 
 static const struct clk_ops slow_osc_ops = {
@@ -114,7 +126,8 @@ at91_clk_register_slow_osc(void __iomem *sckcr,
 			   const char *name,
 			   const char *parent_name,
 			   unsigned long startup,
-			   bool bypass)
+			   bool bypass,
+			   const struct clk_slow_bits *bits)
 {
 	struct clk_slow_osc *osc;
 	struct clk_hw *hw;
@@ -137,10 +150,11 @@ at91_clk_register_slow_osc(void __iomem *sckcr,
 	osc->hw.init = &init;
 	osc->sckcr = sckcr;
 	osc->startup_usec = startup;
+	osc->bits = bits;
 
 	if (bypass)
-		writel((readl(sckcr) & ~AT91_SCKC_OSC32EN) | AT91_SCKC_OSC32BYP,
-		       sckcr);
+		writel((readl(sckcr) & ~AT91_SCKC_OSC32EN(osc->bits)) |
+					AT91_SCKC_OSC32BYP(osc->bits), sckcr);
 
 	hw = &osc->hw;
 	ret = clk_hw_register(NULL, &osc->hw);
@@ -173,7 +187,7 @@ static int clk_slow_rc_osc_prepare(struct clk_hw *hw)
 	struct clk_slow_rc_osc *osc = to_clk_slow_rc_osc(hw);
 	void __iomem *sckcr = osc->sckcr;
 
-	writel(readl(sckcr) | AT91_SCKC_RCEN, sckcr);
+	writel(readl(sckcr) | AT91_SCKC_RCEN(osc->bits), sckcr);
 
 	usleep_range(osc->startup_usec, osc->startup_usec + 1);
 
@@ -185,14 +199,14 @@ static void clk_slow_rc_osc_unprepare(struct clk_hw *hw)
 	struct clk_slow_rc_osc *osc = to_clk_slow_rc_osc(hw);
 	void __iomem *sckcr = osc->sckcr;
 
-	writel(readl(sckcr) & ~AT91_SCKC_RCEN, sckcr);
+	writel(readl(sckcr) & ~AT91_SCKC_RCEN(osc->bits), sckcr);
 }
 
 static int clk_slow_rc_osc_is_prepared(struct clk_hw *hw)
 {
 	struct clk_slow_rc_osc *osc = to_clk_slow_rc_osc(hw);
 
-	return !!(readl(osc->sckcr) & AT91_SCKC_RCEN);
+	return !!(readl(osc->sckcr) & AT91_SCKC_RCEN(osc->bits));
 }
 
 static const struct clk_ops slow_rc_osc_ops = {
@@ -208,7 +222,8 @@ at91_clk_register_slow_rc_osc(void __iomem *sckcr,
 			      const char *name,
 			      unsigned long frequency,
 			      unsigned long accuracy,
-			      unsigned long startup)
+			      unsigned long startup,
+			      const struct clk_slow_bits *bits)
 {
 	struct clk_slow_rc_osc *osc;
 	struct clk_hw *hw;
@@ -230,6 +245,7 @@ at91_clk_register_slow_rc_osc(void __iomem *sckcr,
 
 	osc->hw.init = &init;
 	osc->sckcr = sckcr;
+	osc->bits = bits;
 	osc->frequency = frequency;
 	osc->accuracy = accuracy;
 	osc->startup_usec = startup;
@@ -255,14 +271,14 @@ static int clk_sam9x5_slow_set_parent(struct clk_hw *hw, u8 index)
 
 	tmp = readl(sckcr);
 
-	if ((!index && !(tmp & AT91_SCKC_OSCSEL)) ||
-	    (index && (tmp & AT91_SCKC_OSCSEL)))
+	if ((!index && !(tmp & AT91_SCKC_OSCSEL(slowck->bits))) ||
+	    (index && (tmp & AT91_SCKC_OSCSEL(slowck->bits))))
 		return 0;
 
 	if (index)
-		tmp |= AT91_SCKC_OSCSEL;
+		tmp |= AT91_SCKC_OSCSEL(slowck->bits);
 	else
-		tmp &= ~AT91_SCKC_OSCSEL;
+		tmp &= ~AT91_SCKC_OSCSEL(slowck->bits);
 
 	writel(tmp, sckcr);
 
@@ -275,7 +291,7 @@ static u8 clk_sam9x5_slow_get_parent(struct clk_hw *hw)
 {
 	struct clk_sam9x5_slow *slowck = to_clk_sam9x5_slow(hw);
 
-	return !!(readl(slowck->sckcr) & AT91_SCKC_OSCSEL);
+	return !!(readl(slowck->sckcr) & AT91_SCKC_OSCSEL(slowck->bits));
 }
 
 static const struct clk_ops sam9x5_slow_ops = {
@@ -287,7 +303,8 @@ static struct clk_hw * __init
 at91_clk_register_sam9x5_slow(void __iomem *sckcr,
 			      const char *name,
 			      const char **parent_names,
-			      int num_parents)
+			      int num_parents,
+			      const struct clk_slow_bits *bits)
 {
 	struct clk_sam9x5_slow *slowck;
 	struct clk_hw *hw;
@@ -309,7 +326,8 @@ at91_clk_register_sam9x5_slow(void __iomem *sckcr,
 
 	slowck->hw.init = &init;
 	slowck->sckcr = sckcr;
-	slowck->parent = !!(readl(sckcr) & AT91_SCKC_OSCSEL);
+	slowck->bits = bits;
+	slowck->parent = !!(readl(sckcr) & AT91_SCKC_OSCSEL(slowck->bits));
 
 	hw = &slowck->hw;
 	ret = clk_hw_register(NULL, &slowck->hw);
@@ -322,7 +340,8 @@ at91_clk_register_sam9x5_slow(void __iomem *sckcr,
 }
 
 static void __init at91sam9x5_sckc_register(struct device_node *np,
-					    unsigned int rc_osc_startup_us)
+					    unsigned int rc_osc_startup_us,
+					    const struct clk_slow_bits *bits)
 {
 	const char *parent_names[2] = { "slow_rc_osc", "slow_osc" };
 	void __iomem *regbase = of_iomap(np, 0);
@@ -335,7 +354,8 @@ static void __init at91sam9x5_sckc_register(struct device_node *np,
 		return;
 
 	hw = at91_clk_register_slow_rc_osc(regbase, parent_names[0], 32768,
-					   50000000, rc_osc_startup_us);
+					   50000000, rc_osc_startup_us,
+					   bits);
 	if (IS_ERR(hw))
 		return;
 
@@ -358,11 +378,12 @@ static void __init at91sam9x5_sckc_register(struct device_node *np,
 		return;
 
 	hw = at91_clk_register_slow_osc(regbase, parent_names[1], xtal_name,
-					1200000, bypass);
+					1200000, bypass, bits);
 	if (IS_ERR(hw))
 		return;
 
-	hw = at91_clk_register_sam9x5_slow(regbase, "slowck", parent_names, 2);
+	hw = at91_clk_register_sam9x5_slow(regbase, "slowck", parent_names, 2,
+					   bits);
 	if (IS_ERR(hw))
 		return;
 
@@ -373,16 +394,23 @@ static void __init at91sam9x5_sckc_register(struct device_node *np,
 		of_clk_add_hw_provider(child, of_clk_hw_simple_get, hw);
 }
 
+static const struct clk_slow_bits at91sam9x5_bits = {
+	.cr_rcen = BIT(0),
+	.cr_osc32en = BIT(1),
+	.cr_osc32byp = BIT(2),
+	.cr_oscsel = BIT(3),
+};
+
 static void __init of_at91sam9x5_sckc_setup(struct device_node *np)
 {
-	at91sam9x5_sckc_register(np, 75);
+	at91sam9x5_sckc_register(np, 75, &at91sam9x5_bits);
 }
 CLK_OF_DECLARE(at91sam9x5_clk_sckc, "atmel,at91sam9x5-sckc",
 	       of_at91sam9x5_sckc_setup);
 
 static void __init of_sama5d3_sckc_setup(struct device_node *np)
 {
-	at91sam9x5_sckc_register(np, 500);
+	at91sam9x5_sckc_register(np, 500, &at91sam9x5_bits);
 }
 CLK_OF_DECLARE(sama5d3_clk_sckc, "atmel,sama5d3-sckc",
 	       of_sama5d3_sckc_setup);
@@ -398,7 +426,7 @@ static int clk_sama5d4_slow_osc_prepare(struct clk_hw *hw)
 	 * Assume that if it has already been selected (for example by the
 	 * bootloader), enough time has aready passed.
 	 */
-	if ((readl(osc->sckcr) & AT91_SCKC_OSCSEL)) {
+	if ((readl(osc->sckcr) & AT91_SCKC_OSCSEL(osc->bits))) {
 		osc->prepared = true;
 		return 0;
 	}
@@ -421,6 +449,10 @@ static const struct clk_ops sama5d4_slow_osc_ops = {
 	.is_prepared = clk_sama5d4_slow_osc_is_prepared,
 };
 
+static const struct clk_slow_bits at91sama5d4_bits = {
+	.cr_oscsel = BIT(3),
+};
+
 static void __init of_sama5d4_sckc_setup(struct device_node *np)
 {
 	void __iomem *regbase = of_iomap(np, 0);
@@ -455,6 +487,7 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
 	osc->hw.init = &init;
 	osc->sckcr = regbase;
 	osc->startup_usec = 1200000;
+	osc->bits = &at91sama5d4_bits;
 
 	hw = &osc->hw;
 	ret = clk_hw_register(NULL, &osc->hw);
@@ -463,7 +496,8 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
 		return;
 	}
 
-	hw = at91_clk_register_sam9x5_slow(regbase, "slowck", parent_names, 2);
+	hw = at91_clk_register_sam9x5_slow(regbase, "slowck", parent_names, 2,
+					   &at91sama5d4_bits);
 	if (IS_ERR(hw))
 		return;
 
-- 
2.7.4

[PATCH v3 3/4] dt-bindings: clk: at91: add bindings for SAM9X60's slow clock controller

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add bindings for SAM9X60's slow clock controller.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

Hi Rob,

I didn't added your Reviewed-by tag to this version since I changed
the driver with regards to clock-cells DT binding (and I though you
may want to comment on this).

Thank you,
Claudiu Beznea

 Documentation/devicetree/bindings/clock/at91-clock.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/at91-clock.txt b/Documentation/devicetree/bindings/clock/at91-clock.txt
index b520280e33ff..13f45db3b66d 100644
--- a/Documentation/devicetree/bindings/clock/at91-clock.txt
+++ b/Documentation/devicetree/bindings/clock/at91-clock.txt
@@ -9,10 +9,11 @@ Slow Clock controller:
 Required properties:
 - compatible : shall be one of the following:
 	"atmel,at91sam9x5-sckc",
-	"atmel,sama5d3-sckc" or
-	"atmel,sama5d4-sckc":
+	"atmel,sama5d3-sckc",
+	"atmel,sama5d4-sckc" or
+	"microchip,sam9x60-sckc":
 		at91 SCKC (Slow Clock Controller)
-- #clock-cells : shall be 0.
+- #clock-cells : shall be 1 for "microchip,sam9x60-sckc" otherwise shall be 0.
 - clocks : shall be the input parent clock phandle for the clock.
 
 Optional properties:
-- 
2.7.4

[PATCH v3 4/4] clk: at91: sckc: add support for SAM9X60

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add support for SAM9X60's slow clock.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/clk/at91/sckc.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
index 2a4ac548de80..2c410f41b413 100644
--- a/drivers/clk/at91/sckc.c
+++ b/drivers/clk/at91/sckc.c
@@ -415,6 +415,80 @@ static void __init of_sama5d3_sckc_setup(struct device_node *np)
 CLK_OF_DECLARE(sama5d3_clk_sckc, "atmel,sama5d3-sckc",
 	       of_sama5d3_sckc_setup);
 
+static const struct clk_slow_bits at91sam9x60_bits = {
+	.cr_osc32en = BIT(1),
+	.cr_osc32byp = BIT(2),
+	.cr_oscsel = BIT(24),
+};
+
+static void __init of_sam9x60_sckc_setup(struct device_node *np)
+{
+	void __iomem *regbase = of_iomap(np, 0);
+	struct clk_hw_onecell_data *clk_data;
+	struct clk_hw *slow_rc, *slow_osc;
+	const char *xtal_name;
+	const char *parent_names[2] = { "slow_rc_osc", "slow_osc" };
+	bool bypass;
+	int ret;
+
+	if (!regbase)
+		return;
+
+	slow_rc = clk_hw_register_fixed_rate(NULL, parent_names[0], NULL, 0,
+					     32768);
+	if (IS_ERR(slow_rc))
+		return;
+
+	xtal_name = of_clk_get_parent_name(np, 0);
+	if (!xtal_name)
+		goto unregister_slow_rc;
+
+	bypass = of_property_read_bool(np, "atmel,osc-bypass");
+	slow_osc = at91_clk_register_slow_osc(regbase, parent_names[1],
+					      xtal_name, 5000000, bypass,
+					      &at91sam9x60_bits);
+	if (IS_ERR(slow_osc))
+		goto unregister_slow_rc;
+
+	clk_data = kzalloc(sizeof(*clk_data) + (2 * sizeof(struct clk_hw *)),
+			   GFP_KERNEL);
+	if (!clk_data)
+		goto unregister_slow_osc;
+
+	/* MD_SLCK and TD_SLCK. */
+	clk_data->num = 2;
+	clk_data->hws[0] = clk_hw_register_fixed_rate(NULL, "md_slck",
+						      parent_names[0],
+						      0, 32768);
+	if (IS_ERR(clk_data->hws[0]))
+		goto clk_data_free;
+
+	clk_data->hws[1] = at91_clk_register_sam9x5_slow(regbase, "td_slck",
+							 parent_names, 2,
+							 &at91sam9x60_bits);
+	if (IS_ERR(clk_data->hws[1]))
+		goto unregister_md_slck;
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data);
+	if (WARN_ON(ret))
+		goto unregister_td_slck;
+
+	return;
+
+unregister_td_slck:
+	clk_hw_unregister(clk_data->hws[1]);
+unregister_md_slck:
+	clk_hw_unregister(clk_data->hws[0]);
+clk_data_free:
+	kfree(clk_data);
+unregister_slow_osc:
+	clk_hw_unregister(slow_osc);
+unregister_slow_rc:
+	clk_hw_unregister(slow_rc);
+}
+CLK_OF_DECLARE(sam9x60_clk_sckc, "microchip,sam9x60-sckc",
+	       of_sam9x60_sckc_setup);
+
 static int clk_sama5d4_slow_osc_prepare(struct clk_hw *hw)
 {
 	struct clk_sama5d4_slow_osc *osc = to_clk_sama5d4_slow_osc(hw);
-- 
2.7.4

Re: [PATCH v3 1/4] clk: at91: sckc: sama5d4 has no bypass support

[Alexandre Belloni]

On 10/05/2019 11:23:27+0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> The slow clock of SAMA5D4 has no bypass support thus remove it.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/clk/at91/sckc.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
> index e76b1d64e905..6c55a7a86f79 100644
> --- a/drivers/clk/at91/sckc.c
> +++ b/drivers/clk/at91/sckc.c
> @@ -429,7 +429,6 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
>  	struct clk_init_data init;
>  	const char *xtal_name;
>  	const char *parent_names[2] = { "slow_rc_osc", "slow_osc" };
> -	bool bypass;
>  	int ret;
>  
>  	if (!regbase)
> @@ -443,8 +442,6 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
>  
>  	xtal_name = of_clk_get_parent_name(np, 0);
>  
> -	bypass = of_property_read_bool(np, "atmel,osc-bypass");
> -
>  	osc = kzalloc(sizeof(*osc), GFP_KERNEL);
>  	if (!osc)
>  		return;
> @@ -459,9 +456,6 @@ static void __init of_sama5d4_sckc_setup(struct device_node *np)
>  	osc->sckcr = regbase;
>  	osc->startup_usec = 1200000;
>  
> -	if (bypass)
> -		writel((readl(regbase) | AT91_SCKC_OSC32BYP), regbase);
> -
>  	hw = &osc->hw;
>  	ret = clk_hw_register(NULL, &osc->hw);
>  	if (ret) {
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/4] clk: at91: sckc: add support to specify registers bit offsets

[Alexandre Belloni]

On 10/05/2019 11:23:31+0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Different IPs uses different bit offsets in registers for the same
> functionality, thus adapt the driver to support this.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/clk/at91/sckc.c | 100 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 67 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
> index 6c55a7a86f79..2a4ac548de80 100644
> --- a/drivers/clk/at91/sckc.c
> +++ b/drivers/clk/at91/sckc.c
> @@ -22,15 +22,23 @@
>  #define SLOWCK_SW_TIME_USEC	((SLOWCK_SW_CYCLES * USEC_PER_SEC) / \
>  				 SLOW_CLOCK_FREQ)
>  
> -#define	AT91_SCKC_CR			0x00
> -#define		AT91_SCKC_RCEN		(1 << 0)
> -#define		AT91_SCKC_OSC32EN	(1 << 1)
> -#define		AT91_SCKC_OSC32BYP	(1 << 2)
> -#define		AT91_SCKC_OSCSEL	(1 << 3)
> +#define	AT91_SCKC_CR		0x00
> +#define		AT91_SCKC_RCEN(off)	((off)->cr_rcen)
> +#define		AT91_SCKC_OSC32EN(off)	((off)->cr_osc32en)
> +#define		AT91_SCKC_OSC32BYP(off)	((off)->cr_osc32byp)
> +#define		AT91_SCKC_OSCSEL(off)	((off)->cr_oscsel)
> +
> +struct clk_slow_bits {
> +	u32 cr_rcen;

This bit is only used on sam9x5 so I wouldn't bother having it in the
structure, especially since its use will always be quite separate from
the other ones as it is controlling a separate clock.

> +	u32 cr_osc32en;
> +	u32 cr_osc32byp;
> +	u32 cr_oscsel;
> +};
>  
>  struct clk_slow_osc {
>  	struct clk_hw hw;
>  	void __iomem *sckcr;
> +	const struct clk_slow_bits *bits;
>  	unsigned long startup_usec;
>  };
>  
> @@ -39,6 +47,7 @@ struct clk_slow_osc {
>  struct clk_sama5d4_slow_osc {
>  	struct clk_hw hw;
>  	void __iomem *sckcr;
> +	const struct clk_slow_bits *bits;
>  	unsigned long startup_usec;
>  	bool prepared;
>  };
> @@ -48,6 +57,7 @@ struct clk_sama5d4_slow_osc {
>  struct clk_slow_rc_osc {
>  	struct clk_hw hw;
>  	void __iomem *sckcr;
> +	const struct clk_slow_bits *bits;
>  	unsigned long frequency;
>  	unsigned long accuracy;
>  	unsigned long startup_usec;
> @@ -58,6 +68,7 @@ struct clk_slow_rc_osc {
>  struct clk_sam9x5_slow {
>  	struct clk_hw hw;
>  	void __iomem *sckcr;
> +	const struct clk_slow_bits *bits;
>  	u8 parent;
>  };
>  
> @@ -69,10 +80,11 @@ static int clk_slow_osc_prepare(struct clk_hw *hw)
>  	void __iomem *sckcr = osc->sckcr;
>  	u32 tmp = readl(sckcr);
>  
> -	if (tmp & (AT91_SCKC_OSC32BYP | AT91_SCKC_OSC32EN))
> +	if (tmp & (AT91_SCKC_OSC32BYP(osc->bits) |
> +		   AT91_SCKC_OSC32EN(osc->bits)))

I still find that:

	if (tmp & (osc->bits->cr_osc32byp | osc->bits->cr_osc32en))

would be shorter and easier to read and still fits on one line.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 3/4] dt-bindings: clk: at91: add bindings for SAM9X60's slow clock controller

[Alexandre Belloni]

On 10/05/2019 11:23:35+0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Add bindings for SAM9X60's slow clock controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
> 
> Hi Rob,
> 
> I didn't added your Reviewed-by tag to this version since I changed
> the driver with regards to clock-cells DT binding (and I though you
> may want to comment on this).
> 
> Thank you,
> Claudiu Beznea
> 
>  Documentation/devicetree/bindings/clock/at91-clock.txt | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/at91-clock.txt b/Documentation/devicetree/bindings/clock/at91-clock.txt
> index b520280e33ff..13f45db3b66d 100644
> --- a/Documentation/devicetree/bindings/clock/at91-clock.txt
> +++ b/Documentation/devicetree/bindings/clock/at91-clock.txt
> @@ -9,10 +9,11 @@ Slow Clock controller:
>  Required properties:
>  - compatible : shall be one of the following:
>  	"atmel,at91sam9x5-sckc",
> -	"atmel,sama5d3-sckc" or
> -	"atmel,sama5d4-sckc":
> +	"atmel,sama5d3-sckc",
> +	"atmel,sama5d4-sckc" or
> +	"microchip,sam9x60-sckc":
>  		at91 SCKC (Slow Clock Controller)
> -- #clock-cells : shall be 0.
> +- #clock-cells : shall be 1 for "microchip,sam9x60-sckc" otherwise shall be 0.
>  - clocks : shall be the input parent clock phandle for the clock.
>  
>  Optional properties:
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/4] clk: at91: sckc: add support for SAM9X60

[Alexandre Belloni]

On 10/05/2019 11:23:40+0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Add support for SAM9X60's slow clock.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/clk/at91/sckc.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
> index 2a4ac548de80..2c410f41b413 100644
> --- a/drivers/clk/at91/sckc.c
> +++ b/drivers/clk/at91/sckc.c
> @@ -415,6 +415,80 @@ static void __init of_sama5d3_sckc_setup(struct device_node *np)
>  CLK_OF_DECLARE(sama5d3_clk_sckc, "atmel,sama5d3-sckc",
>  	       of_sama5d3_sckc_setup);
>  
> +static const struct clk_slow_bits at91sam9x60_bits = {
> +	.cr_osc32en = BIT(1),
> +	.cr_osc32byp = BIT(2),
> +	.cr_oscsel = BIT(24),
> +};
> +
> +static void __init of_sam9x60_sckc_setup(struct device_node *np)
> +{
> +	void __iomem *regbase = of_iomap(np, 0);
> +	struct clk_hw_onecell_data *clk_data;
> +	struct clk_hw *slow_rc, *slow_osc;
> +	const char *xtal_name;
> +	const char *parent_names[2] = { "slow_rc_osc", "slow_osc" };
> +	bool bypass;
> +	int ret;
> +
> +	if (!regbase)
> +		return;
> +
> +	slow_rc = clk_hw_register_fixed_rate(NULL, parent_names[0], NULL, 0,
> +					     32768);
> +	if (IS_ERR(slow_rc))
> +		return;
> +
> +	xtal_name = of_clk_get_parent_name(np, 0);
> +	if (!xtal_name)
> +		goto unregister_slow_rc;
> +
> +	bypass = of_property_read_bool(np, "atmel,osc-bypass");
> +	slow_osc = at91_clk_register_slow_osc(regbase, parent_names[1],
> +					      xtal_name, 5000000, bypass,
> +					      &at91sam9x60_bits);
> +	if (IS_ERR(slow_osc))
> +		goto unregister_slow_rc;
> +
> +	clk_data = kzalloc(sizeof(*clk_data) + (2 * sizeof(struct clk_hw *)),
> +			   GFP_KERNEL);
> +	if (!clk_data)
> +		goto unregister_slow_osc;
> +
> +	/* MD_SLCK and TD_SLCK. */
> +	clk_data->num = 2;
> +	clk_data->hws[0] = clk_hw_register_fixed_rate(NULL, "md_slck",
> +						      parent_names[0],
> +						      0, 32768);
> +	if (IS_ERR(clk_data->hws[0]))
> +		goto clk_data_free;
> +
> +	clk_data->hws[1] = at91_clk_register_sam9x5_slow(regbase, "td_slck",
> +							 parent_names, 2,
> +							 &at91sam9x60_bits);
> +	if (IS_ERR(clk_data->hws[1]))
> +		goto unregister_md_slck;
> +
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data);
> +	if (WARN_ON(ret))
> +		goto unregister_td_slck;
> +
> +	return;
> +
> +unregister_td_slck:
> +	clk_hw_unregister(clk_data->hws[1]);
> +unregister_md_slck:
> +	clk_hw_unregister(clk_data->hws[0]);
> +clk_data_free:
> +	kfree(clk_data);
> +unregister_slow_osc:
> +	clk_hw_unregister(slow_osc);
> +unregister_slow_rc:
> +	clk_hw_unregister(slow_rc);
> +}
> +CLK_OF_DECLARE(sam9x60_clk_sckc, "microchip,sam9x60-sckc",
> +	       of_sam9x60_sckc_setup);
> +
>  static int clk_sama5d4_slow_osc_prepare(struct clk_hw *hw)
>  {
>  	struct clk_sama5d4_slow_osc *osc = to_clk_sama5d4_slow_osc(hw);
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 3/4] dt-bindings: clk: at91: add bindings for SAM9X60's slow clock controller

[Rob Herring]

On Fri, 10 May 2019 11:23:35 +0000, <Claudiu.Beznea@microchip.com> wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Add bindings for SAM9X60's slow clock controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
> 
> Hi Rob,
> 
> I didn't added your Reviewed-by tag to this version since I changed
> the driver with regards to clock-cells DT binding (and I though you
> may want to comment on this).
> 
> Thank you,
> Claudiu Beznea
> 
>  Documentation/devicetree/bindings/clock/at91-clock.txt | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/4] clk: at91: sckc: add support to specify registers bit offsets

[Claudiu.Beznea]

On 11.05.2019 00:32, Alexandre Belloni wrote:
> On 10/05/2019 11:23:31+0000, Claudiu.Beznea@microchip.com wrote:
>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>
>> Different IPs uses different bit offsets in registers for the same
>> functionality, thus adapt the driver to support this.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/clk/at91/sckc.c | 100 ++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 67 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
>> index 6c55a7a86f79..2a4ac548de80 100644
>> --- a/drivers/clk/at91/sckc.c
>> +++ b/drivers/clk/at91/sckc.c
>> @@ -22,15 +22,23 @@
>>  #define SLOWCK_SW_TIME_USEC	((SLOWCK_SW_CYCLES * USEC_PER_SEC) / \
>>  				 SLOW_CLOCK_FREQ)
>>  
>> -#define	AT91_SCKC_CR			0x00
>> -#define		AT91_SCKC_RCEN		(1 << 0)
>> -#define		AT91_SCKC_OSC32EN	(1 << 1)
>> -#define		AT91_SCKC_OSC32BYP	(1 << 2)
>> -#define		AT91_SCKC_OSCSEL	(1 << 3)
>> +#define	AT91_SCKC_CR		0x00
>> +#define		AT91_SCKC_RCEN(off)	((off)->cr_rcen)
>> +#define		AT91_SCKC_OSC32EN(off)	((off)->cr_osc32en)
>> +#define		AT91_SCKC_OSC32BYP(off)	((off)->cr_osc32byp)
>> +#define		AT91_SCKC_OSCSEL(off)	((off)->cr_oscsel)
>> +
>> +struct clk_slow_bits {
>> +	u32 cr_rcen;
> 
> This bit is only used on sam9x5 so I wouldn't bother having it in the
> structure, especially since its use will always be quite separate from
> the other ones as it is controlling a separate clock.
> 
>> +	u32 cr_osc32en;
>> +	u32 cr_osc32byp;
>> +	u32 cr_oscsel;
>> +};
>>  
>>  struct clk_slow_osc {
>>  	struct clk_hw hw;
>>  	void __iomem *sckcr;
>> +	const struct clk_slow_bits *bits;
>>  	unsigned long startup_usec;
>>  };
>>  
>> @@ -39,6 +47,7 @@ struct clk_slow_osc {
>>  struct clk_sama5d4_slow_osc {
>>  	struct clk_hw hw;
>>  	void __iomem *sckcr;
>> +	const struct clk_slow_bits *bits;
>>  	unsigned long startup_usec;
>>  	bool prepared;
>>  };
>> @@ -48,6 +57,7 @@ struct clk_sama5d4_slow_osc {
>>  struct clk_slow_rc_osc {
>>  	struct clk_hw hw;
>>  	void __iomem *sckcr;
>> +	const struct clk_slow_bits *bits;
>>  	unsigned long frequency;
>>  	unsigned long accuracy;
>>  	unsigned long startup_usec;
>> @@ -58,6 +68,7 @@ struct clk_slow_rc_osc {
>>  struct clk_sam9x5_slow {
>>  	struct clk_hw hw;
>>  	void __iomem *sckcr;
>> +	const struct clk_slow_bits *bits;
>>  	u8 parent;
>>  };
>>  
>> @@ -69,10 +80,11 @@ static int clk_slow_osc_prepare(struct clk_hw *hw)
>>  	void __iomem *sckcr = osc->sckcr;
>>  	u32 tmp = readl(sckcr);
>>  
>> -	if (tmp & (AT91_SCKC_OSC32BYP | AT91_SCKC_OSC32EN))
>> +	if (tmp & (AT91_SCKC_OSC32BYP(osc->bits) |
>> +		   AT91_SCKC_OSC32EN(osc->bits)))
> 
> I still find that:
> 
> 	if (tmp & (osc->bits->cr_osc32byp | osc->bits->cr_osc32en))
> 
> would be shorter and easier to read and still fits on one line.

Agree, but I thought to use the same interface everywhere. Anyway, tell me
if you want to resend with these changes.

> 

Re: [PATCH v3 2/4] clk: at91: sckc: add support to specify registers bit offsets

[Alexandre Belloni]

On 16/05/2019 08:10:34+0000, Claudiu.Beznea@microchip.com wrote:
> >> @@ -69,10 +80,11 @@ static int clk_slow_osc_prepare(struct clk_hw *hw)
> >>  	void __iomem *sckcr = osc->sckcr;
> >>  	u32 tmp = readl(sckcr);
> >>  
> >> -	if (tmp & (AT91_SCKC_OSC32BYP | AT91_SCKC_OSC32EN))
> >> +	if (tmp & (AT91_SCKC_OSC32BYP(osc->bits) |
> >> +		   AT91_SCKC_OSC32EN(osc->bits)))
> > 
> > I still find that:
> > 
> > 	if (tmp & (osc->bits->cr_osc32byp | osc->bits->cr_osc32en))
> > 
> > would be shorter and easier to read and still fits on one line.
> 
> Agree, but I thought to use the same interface everywhere. Anyway, tell me
> if you want to resend with these changes.
> 
My comment applies to all the AT91_SCKC_.*() macros. I don't feel that
the macros make the code clearer, accessing bits->cr_.* is self
documenting enough (and makes the code shorter).

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/4] clk: at91: sckc: add support to specify registers bit offsets

[Claudiu.Beznea]

On 18.05.2019 00:13, Alexandre Belloni wrote:
> External E-Mail
> 
> 
> On 16/05/2019 08:10:34+0000, Claudiu.Beznea@microchip.com wrote:
>>>> @@ -69,10 +80,11 @@ static int clk_slow_osc_prepare(struct clk_hw *hw)
>>>>  	void __iomem *sckcr = osc->sckcr;
>>>>  	u32 tmp = readl(sckcr);
>>>>  
>>>> -	if (tmp & (AT91_SCKC_OSC32BYP | AT91_SCKC_OSC32EN))
>>>> +	if (tmp & (AT91_SCKC_OSC32BYP(osc->bits) |
>>>> +		   AT91_SCKC_OSC32EN(osc->bits)))
>>>
>>> I still find that:
>>>
>>> 	if (tmp & (osc->bits->cr_osc32byp | osc->bits->cr_osc32en))
>>>
>>> would be shorter and easier to read and still fits on one line.
>>
>> Agree, but I thought to use the same interface everywhere. Anyway, tell me
>> if you want to resend with these changes.
>>
> My comment applies to all the AT91_SCKC_.*() macros. I don't feel that
> the macros make the code clearer, accessing bits->cr_.* is self
> documenting enough (and makes the code shorter).

OK, I'll send a new version taking this into consideration.

>