[PATCH 0/6] rtc: sun6i: Fix the RTC accuracy

[PATCH 0/6] rtc: sun6i: Fix the RTC accuracy

[Maxime Ripard]

Hi,

The RTC used in the A31 and later SoC has an accuracy issue, which is
already significant even after a couple of hours.

This is due to the fact that the oscillator used by default is an internal
and very inaccurate one.

A first attempt at fixing that by switching to the external oscillator was
done in the patch "rtc: sun6i: Switch to the external oscillator". However,
it turned out to be problematic since it was tracked properly in the clock
framework, which might lead to some clocks being disabled, even though
their devices were not notified.

This is a second attempt, this time by making it part of the CCF. It
turned out to be a bit more complicated than one would expect since the mux
found inside the RTC also controls one of the input of the main clock unit.
Therefore, it needs to be probed before the main clock unit driver.

Let me know what you think,
Maxime

Maxime Ripard (6):
  rtc: sun6i: Expose the 32kHz oscillator
  rtc: sun6i: Add some locking
  rtc: sun6i: Disable the build as a module
  rtc: sun6i: Force the mux to the external oscillator
  ARM: sun8i: a23/a33: Enable the real LOSC and use it
  ARM: sun8i: a23/a33: Add the oscillators accuracy

 Documentation/devicetree/bindings/rtc/sun6i-rtc.txt |   8 +-
 arch/arm/boot/dts/sun8i-a23-a33.dtsi                |  15 +-
 drivers/rtc/Kconfig                                 |   2 +-
 drivers/rtc/rtc-sun6i.c                             | 189 +++++++++++--
 4 files changed, 181 insertions(+), 33 deletions(-)

base-commit: 99cef370ac9939df2aeb16c96d07e842b2fa8201
-- 
git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 1/6] rtc: sun6i: Expose the 32kHz oscillator

[Maxime Ripard]

The RTC controls the input source of the main 32kHz oscillator in the
system, feeding it to the clock unit too.

By default, this is using an internal, very inaccurate (+/- 30%)
oscillator with a divider to make it roughly around 32kHz. This is however
quite impractical for the RTC, since our time will not be tracked properly.

Since this oscillator is an input of the main clock unit, and since that
clock unit will be probed using CLK_OF_DECLARE, we have to use it as well,
leading to a two stage probe: one to enable the clock, the other one to
enable the RTC.

There is also a slight change in the binding that is required (and should
have been from the beginning), since we'll need a phandle to the external
oscillator used on that board. We support the old binding by not allowing
to switch to the external oscillator and only using the internal one (which
was the previous behaviour) in the case where we're missing that phandle.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/rtc/sun6i-rtc.txt |   8 +-
 drivers/rtc/rtc-sun6i.c                             | 140 ++++++++++++-
 2 files changed, 139 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt b/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
index f007e428a1ab..44d788240162 100644
--- a/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
@@ -8,10 +8,18 @@ Required properties:
 		  memory mapped region.
 - interrupts	: IRQ lines for the RTC alarm 0 and alarm 1, in that order.
 
+Required properties for new device trees
+- clocks	: phandle to the 32kHz external oscillator
+- clock-output-names : name of the LOSC clock created
+- #clock-cells  : must be equals to 0
+
 Example:
 
 rtc: rtc@01f00000 {
 	compatible = "allwinner,sun6i-a31-rtc";
 	reg = <0x01f00000 0x54>;
 	interrupts = <0 40 4>, <0 41 4>;
+	clock-output-names = "osc32k";
+	clocks = <&ext_osc32k>;
+	#clock-cells = <0>;
 };
diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index c169a2cd4727..408dd512a6ac 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -20,6 +20,8 @@
  * more details.
  */
 
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/fs.h>
@@ -33,15 +35,20 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/rtc.h>
+#include <linux/slab.h>
 #include <linux/types.h>
 
 /* Control register */
 #define SUN6I_LOSC_CTRL				0x0000
+#define SUN6I_LOSC_CTRL_KEY			(0x16aa << 16)
 #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
 #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
 #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
+#define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
 #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
 
+#define SUN6I_LOSC_CLK_PRESCAL			0x0008
+
 /* RTC */
 #define SUN6I_RTC_YMD				0x0010
 #define SUN6I_RTC_HMS				0x0014
@@ -114,8 +121,128 @@ struct sun6i_rtc_dev {
 	void __iomem *base;
 	int irq;
 	unsigned long alarm;
+
+	struct clk_hw hw;
+	struct clk_hw *int_osc;
+	struct clk *losc;
 };
 
+static struct sun6i_rtc_dev *sun6i_rtc;
+
+static unsigned long sun6i_rtc_osc_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
+	u32 val;
+
+	val = readl(rtc->base + SUN6I_LOSC_CTRL);
+	if (val & SUN6I_LOSC_CTRL_EXT_OSC)
+		return parent_rate;
+
+	val = readl(rtc->base + SUN6I_LOSC_CLK_PRESCAL);
+	val &= GENMASK(4, 0);
+
+	return parent_rate / (val + 1);
+}
+
+static u8 sun6i_rtc_osc_get_parent(struct clk_hw *hw)
+{
+	struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
+
+	return readl(rtc->base + SUN6I_LOSC_CTRL) & SUN6I_LOSC_CTRL_EXT_OSC;
+}
+
+static int sun6i_rtc_osc_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
+	u32 val;
+
+	if (index > 1)
+		return -EINVAL;
+
+	val = readl(rtc->base + SUN6I_LOSC_CTRL);
+	val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
+	val |= SUN6I_LOSC_CTRL_KEY;
+	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
+	writel(val, rtc->base + SUN6I_LOSC_CTRL);
+
+	return 0;
+}
+
+static const struct clk_ops sun6i_rtc_osc_ops = {
+	.recalc_rate	= sun6i_rtc_osc_recalc_rate,
+
+	.get_parent	= sun6i_rtc_osc_get_parent,
+	.set_parent	= sun6i_rtc_osc_set_parent,
+};
+
+static void __init sun6i_rtc_clk_init(struct device_node *node)
+{
+	struct sun6i_rtc_dev *rtc;
+	struct clk_init_data init = {
+		.ops		= &sun6i_rtc_osc_ops,
+	};
+	const char *parents[2];
+
+	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		pr_crit("Can't allocate RTC structure\n");
+
+	rtc->base = of_io_request_and_map(node, 0, of_node_full_name(node));
+	if (!rtc->base) {
+		pr_crit("Can't map RTC registers");
+		return;
+	}
+
+	rtc->int_osc = clk_hw_register_fixed_rate_with_accuracy(NULL,
+								"rtc-int-osc",
+								NULL, 0,
+								667000,
+								300000000);
+	if (IS_ERR(rtc->int_osc)) {
+		pr_crit("Couldn't register the internal oscillator\n");
+		return;
+	}
+
+	/*
+	 * Due to the missing clocks property we had to express the
+	 * parenthood with the external oscillator that cannot fix (or
+	 * at least expect to have) in the old DTs, we have to be a
+	 * bit smart here.
+	 *
+	 * We deal with that by simply registering the internal
+	 * oscillator as our parent in all cases, and try to get the
+	 * external oscillator from the DT.
+	 *
+	 * In the case where we don't have it, of_clk_get_parent_name
+	 * will return NULL, which is ok, and of_clk_get_parent_count
+	 * will return 0, which is fine too. We will just register a
+	 * single parent, everything works.
+	 */
+	parents[0] = clk_hw_get_name(rtc->int_osc);
+	parents[1] = of_clk_get_parent_name(node, 0);
+
+	rtc->hw.init = &init;
+
+	init.parent_names = parents;
+	init.num_parents = of_clk_get_parent_count(node) + 1;
+	init.name = "rtc-osc";
+	of_property_read_string(node, "clock-output-names", &init.name);
+
+	rtc->losc = clk_register(NULL, &rtc->hw);
+	if (IS_ERR(rtc->losc)) {
+		pr_crit("Couldn't register the LOSC clock\n");
+		return;
+	}
+
+	of_clk_add_hw_provider(node, of_clk_hw_simple_get, &rtc->hw);
+
+	/* Yes, I know, this is ugly. */
+	sun6i_rtc = rtc;
+}
+CLK_OF_DECLARE_DRIVER(sun6i_rtc_clk, "allwinner,sun6i-a31-rtc",
+		      sun6i_rtc_clk_init);
+
 static irqreturn_t sun6i_rtc_alarmirq(int irq, void *id)
 {
 	struct sun6i_rtc_dev *chip = (struct sun6i_rtc_dev *) id;
@@ -349,22 +476,15 @@ static const struct rtc_class_ops sun6i_rtc_ops = {
 
 static int sun6i_rtc_probe(struct platform_device *pdev)
 {
-	struct sun6i_rtc_dev *chip;
-	struct resource *res;
+	struct sun6i_rtc_dev *chip = sun6i_rtc;
 	int ret;
 
-	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
-		return -ENOMEM;
+		return -ENODEV;
 
 	platform_set_drvdata(pdev, chip);
 	chip->dev = &pdev->dev;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	chip->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(chip->base))
-		return PTR_ERR(chip->base);
-
 	chip->irq = platform_get_irq(pdev, 0);
 	if (chip->irq < 0) {
 		dev_err(&pdev->dev, "No IRQ resource\n");
@@ -404,6 +524,8 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
 	/* disable alarm wakeup */
 	writel(0, chip->base + SUN6I_ALARM_CONFIG);
 
+	clk_prepare_enable(chip->losc);
+
 	chip->rtc = rtc_device_register("rtc-sun6i", &pdev->dev,
 					&sun6i_rtc_ops, THIS_MODULE);
 	if (IS_ERR(chip->rtc)) {
-- 
git-series 0.8.11
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] rtc: sun6i: Expose the 32kHz oscillator

[Chen-Yu Tsai]

On Fri, Jan 20, 2017 at 11:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The RTC controls the input source of the main 32kHz oscillator in the
> system, feeding it to the clock unit too.
>
> By default, this is using an internal, very inaccurate (+/- 30%)
> oscillator with a divider to make it roughly around 32kHz. This is however
> quite impractical for the RTC, since our time will not be tracked properly.
>
> Since this oscillator is an input of the main clock unit, and since that
> clock unit will be probed using CLK_OF_DECLARE, we have to use it as well,
> leading to a two stage probe: one to enable the clock, the other one to
> enable the RTC.
>
> There is also a slight change in the binding that is required (and should
> have been from the beginning), since we'll need a phandle to the external
> oscillator used on that board. We support the old binding by not allowing
> to switch to the external oscillator and only using the internal one (which
> was the previous behaviour) in the case where we're missing that phandle.

I'm not sure about this. The original behavior was wrong to begin with.
We were claiming 32768 Hz in the device tree already, and any users
would already be seeing the correct rate if we force the mux to the
external oscillator.

Furthermore, I don't think it's possible to turn the external oscillator
off, unlike the 24 MHz main oscillator. I do agree that exposing the
internal oscillator is the right thing to do. On some later SoCs it
is actually directly a source for the CPUS mux.

The code changes below look good.

Regards
ChenYu

> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/rtc/sun6i-rtc.txt |   8 +-
>  drivers/rtc/rtc-sun6i.c                             | 140 ++++++++++++-
>  2 files changed, 139 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt b/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
> index f007e428a1ab..44d788240162 100644
> --- a/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
> +++ b/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
> @@ -8,10 +8,18 @@ Required properties:
>                   memory mapped region.
>  - interrupts   : IRQ lines for the RTC alarm 0 and alarm 1, in that order.
>
> +Required properties for new device trees
> +- clocks       : phandle to the 32kHz external oscillator
> +- clock-output-names : name of the LOSC clock created
> +- #clock-cells  : must be equals to 0
> +
>  Example:
>
>  rtc: rtc@01f00000 {
>         compatible = "allwinner,sun6i-a31-rtc";
>         reg = <0x01f00000 0x54>;
>         interrupts = <0 40 4>, <0 41 4>;
> +       clock-output-names = "osc32k";
> +       clocks = <&ext_osc32k>;
> +       #clock-cells = <0>;
>  };
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index c169a2cd4727..408dd512a6ac 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -20,6 +20,8 @@
>   * more details.
>   */
>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/fs.h>
> @@ -33,15 +35,20 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/rtc.h>
> +#include <linux/slab.h>
>  #include <linux/types.h>
>
>  /* Control register */
>  #define SUN6I_LOSC_CTRL                                0x0000
> +#define SUN6I_LOSC_CTRL_KEY                    (0x16aa << 16)
>  #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC           BIT(9)
>  #define SUN6I_LOSC_CTRL_RTC_HMS_ACC            BIT(8)
>  #define SUN6I_LOSC_CTRL_RTC_YMD_ACC            BIT(7)
> +#define SUN6I_LOSC_CTRL_EXT_OSC                        BIT(0)
>  #define SUN6I_LOSC_CTRL_ACC_MASK               GENMASK(9, 7)
>
> +#define SUN6I_LOSC_CLK_PRESCAL                 0x0008
> +
>  /* RTC */
>  #define SUN6I_RTC_YMD                          0x0010
>  #define SUN6I_RTC_HMS                          0x0014
> @@ -114,8 +121,128 @@ struct sun6i_rtc_dev {
>         void __iomem *base;
>         int irq;
>         unsigned long alarm;
> +
> +       struct clk_hw hw;
> +       struct clk_hw *int_osc;
> +       struct clk *losc;
>  };
>
> +static struct sun6i_rtc_dev *sun6i_rtc;
> +
> +static unsigned long sun6i_rtc_osc_recalc_rate(struct clk_hw *hw,
> +                                              unsigned long parent_rate)
> +{
> +       struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
> +       u32 val;
> +
> +       val = readl(rtc->base + SUN6I_LOSC_CTRL);
> +       if (val & SUN6I_LOSC_CTRL_EXT_OSC)
> +               return parent_rate;
> +
> +       val = readl(rtc->base + SUN6I_LOSC_CLK_PRESCAL);
> +       val &= GENMASK(4, 0);
> +
> +       return parent_rate / (val + 1);
> +}
> +
> +static u8 sun6i_rtc_osc_get_parent(struct clk_hw *hw)
> +{
> +       struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
> +
> +       return readl(rtc->base + SUN6I_LOSC_CTRL) & SUN6I_LOSC_CTRL_EXT_OSC;
> +}
> +
> +static int sun6i_rtc_osc_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
> +       u32 val;
> +
> +       if (index > 1)
> +               return -EINVAL;
> +
> +       val = readl(rtc->base + SUN6I_LOSC_CTRL);
> +       val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
> +       val |= SUN6I_LOSC_CTRL_KEY;
> +       val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
> +       writel(val, rtc->base + SUN6I_LOSC_CTRL);
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops sun6i_rtc_osc_ops = {
> +       .recalc_rate    = sun6i_rtc_osc_recalc_rate,
> +
> +       .get_parent     = sun6i_rtc_osc_get_parent,
> +       .set_parent     = sun6i_rtc_osc_set_parent,
> +};
> +
> +static void __init sun6i_rtc_clk_init(struct device_node *node)
> +{
> +       struct sun6i_rtc_dev *rtc;
> +       struct clk_init_data init = {
> +               .ops            = &sun6i_rtc_osc_ops,
> +       };
> +       const char *parents[2];
> +
> +       rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> +       if (!rtc)
> +               pr_crit("Can't allocate RTC structure\n");
> +
> +       rtc->base = of_io_request_and_map(node, 0, of_node_full_name(node));
> +       if (!rtc->base) {
> +               pr_crit("Can't map RTC registers");
> +               return;
> +       }
> +
> +       rtc->int_osc = clk_hw_register_fixed_rate_with_accuracy(NULL,
> +                                                               "rtc-int-osc",
> +                                                               NULL, 0,
> +                                                               667000,
> +                                                               300000000);
> +       if (IS_ERR(rtc->int_osc)) {
> +               pr_crit("Couldn't register the internal oscillator\n");
> +               return;
> +       }
> +
> +       /*
> +        * Due to the missing clocks property we had to express the
> +        * parenthood with the external oscillator that cannot fix (or
> +        * at least expect to have) in the old DTs, we have to be a
> +        * bit smart here.
> +        *
> +        * We deal with that by simply registering the internal
> +        * oscillator as our parent in all cases, and try to get the
> +        * external oscillator from the DT.
> +        *
> +        * In the case where we don't have it, of_clk_get_parent_name
> +        * will return NULL, which is ok, and of_clk_get_parent_count
> +        * will return 0, which is fine too. We will just register a
> +        * single parent, everything works.
> +        */
> +       parents[0] = clk_hw_get_name(rtc->int_osc);
> +       parents[1] = of_clk_get_parent_name(node, 0);
> +
> +       rtc->hw.init = &init;
> +
> +       init.parent_names = parents;
> +       init.num_parents = of_clk_get_parent_count(node) + 1;
> +       init.name = "rtc-osc";
> +       of_property_read_string(node, "clock-output-names", &init.name);
> +
> +       rtc->losc = clk_register(NULL, &rtc->hw);
> +       if (IS_ERR(rtc->losc)) {
> +               pr_crit("Couldn't register the LOSC clock\n");
> +               return;
> +       }
> +
> +       of_clk_add_hw_provider(node, of_clk_hw_simple_get, &rtc->hw);
> +
> +       /* Yes, I know, this is ugly. */
> +       sun6i_rtc = rtc;
> +}
> +CLK_OF_DECLARE_DRIVER(sun6i_rtc_clk, "allwinner,sun6i-a31-rtc",
> +                     sun6i_rtc_clk_init);
> +
>  static irqreturn_t sun6i_rtc_alarmirq(int irq, void *id)
>  {
>         struct sun6i_rtc_dev *chip = (struct sun6i_rtc_dev *) id;
> @@ -349,22 +476,15 @@ static const struct rtc_class_ops sun6i_rtc_ops = {
>
>  static int sun6i_rtc_probe(struct platform_device *pdev)
>  {
> -       struct sun6i_rtc_dev *chip;
> -       struct resource *res;
> +       struct sun6i_rtc_dev *chip = sun6i_rtc;
>         int ret;
>
> -       chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>         if (!chip)
> -               return -ENOMEM;
> +               return -ENODEV;
>
>         platform_set_drvdata(pdev, chip);
>         chip->dev = &pdev->dev;
>
> -       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -       chip->base = devm_ioremap_resource(&pdev->dev, res);
> -       if (IS_ERR(chip->base))
> -               return PTR_ERR(chip->base);
> -
>         chip->irq = platform_get_irq(pdev, 0);
>         if (chip->irq < 0) {
>                 dev_err(&pdev->dev, "No IRQ resource\n");
> @@ -404,6 +524,8 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
>         /* disable alarm wakeup */
>         writel(0, chip->base + SUN6I_ALARM_CONFIG);
>
> +       clk_prepare_enable(chip->losc);
> +
>         chip->rtc = rtc_device_register("rtc-sun6i", &pdev->dev,
>                                         &sun6i_rtc_ops, THIS_MODULE);
>         if (IS_ERR(chip->rtc)) {
> --
> git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/6] rtc: sun6i: Expose the 32kHz oscillator

[Alexandre Belloni]

Hi,

On 20/01/2017 at 16:56:38 +0100, Maxime Ripard wrote :
> +
> +	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> +	if (!rtc)
> +		pr_crit("Can't allocate RTC structure\n");
> +

The message is unnecessary but you probably want to stop there and
return.

> +	rtc->base = of_io_request_and_map(node, 0, of_node_full_name(node));
> +	if (!rtc->base) {
> +		pr_crit("Can't map RTC registers");
> +		return;
> +	}
> +
> +	rtc->int_osc = clk_hw_register_fixed_rate_with_accuracy(NULL,
> +								"rtc-int-osc",
> +								NULL, 0,
> +								667000,
> +								300000000);
> +	if (IS_ERR(rtc->int_osc)) {
> +		pr_crit("Couldn't register the internal oscillator\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Due to the missing clocks property we had to express the
> +	 * parenthood with the external oscillator that cannot fix (or
> +	 * at least expect to have) in the old DTs, we have to be a
> +	 * bit smart here.
> +	 *
> +	 * We deal with that by simply registering the internal
> +	 * oscillator as our parent in all cases, and try to get the
> +	 * external oscillator from the DT.
> +	 *
> +	 * In the case where we don't have it, of_clk_get_parent_name
> +	 * will return NULL, which is ok, and of_clk_get_parent_count
> +	 * will return 0, which is fine too. We will just register a
> +	 * single parent, everything works.
> +	 */
> +	parents[0] = clk_hw_get_name(rtc->int_osc);
> +	parents[1] = of_clk_get_parent_name(node, 0);
> +
> +	rtc->hw.init = &init;
> +
> +	init.parent_names = parents;
> +	init.num_parents = of_clk_get_parent_count(node) + 1;
> +	init.name = "rtc-osc";
> +	of_property_read_string(node, "clock-output-names", &init.name);
> +
> +	rtc->losc = clk_register(NULL, &rtc->hw);
> +	if (IS_ERR(rtc->losc)) {
> +		pr_crit("Couldn't register the LOSC clock\n");
> +		return;
> +	}
> +
> +	of_clk_add_hw_provider(node, of_clk_hw_simple_get, &rtc->hw);
> +
> +	/* Yes, I know, this is ugly. */
> +	sun6i_rtc = rtc;
> +}
> +CLK_OF_DECLARE_DRIVER(sun6i_rtc_clk, "allwinner,sun6i-a31-rtc",
> +		      sun6i_rtc_clk_init);
> +
>  static irqreturn_t sun6i_rtc_alarmirq(int irq, void *id)
>  {
>  	struct sun6i_rtc_dev *chip = (struct sun6i_rtc_dev *) id;
> @@ -349,22 +476,15 @@ static const struct rtc_class_ops sun6i_rtc_ops = {
>  
>  static int sun6i_rtc_probe(struct platform_device *pdev)
>  {
> -	struct sun6i_rtc_dev *chip;
> -	struct resource *res;
> +	struct sun6i_rtc_dev *chip = sun6i_rtc;
>  	int ret;
>  
> -	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
> -		return -ENOMEM;
> +		return -ENODEV;
>  
>  	platform_set_drvdata(pdev, chip);
>  	chip->dev = &pdev->dev;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	chip->base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(chip->base))
> -		return PTR_ERR(chip->base);
> -
>  	chip->irq = platform_get_irq(pdev, 0);
>  	if (chip->irq < 0) {
>  		dev_err(&pdev->dev, "No IRQ resource\n");
> @@ -404,6 +524,8 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
>  	/* disable alarm wakeup */
>  	writel(0, chip->base + SUN6I_ALARM_CONFIG);
>  
> +	clk_prepare_enable(chip->losc);
> +
>  	chip->rtc = rtc_device_register("rtc-sun6i", &pdev->dev,
>  					&sun6i_rtc_ops, THIS_MODULE);
>  	if (IS_ERR(chip->rtc)) {
> -- 
> git-series 0.8.11

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 2/6] rtc: sun6i: Add some locking

[Maxime Ripard]

Some registers have a read-modify-write access pattern that are not atomic.

Add some locking to prevent from concurrent accesses.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/rtc/rtc-sun6i.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 408dd512a6ac..872d18609183 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -125,6 +125,8 @@ struct sun6i_rtc_dev {
 	struct clk_hw hw;
 	struct clk_hw *int_osc;
 	struct clk *losc;
+
+	spinlock_t lock;
 };
 
 static struct sun6i_rtc_dev *sun6i_rtc;
@@ -155,16 +157,19 @@ static u8 sun6i_rtc_osc_get_parent(struct clk_hw *hw)
 static int sun6i_rtc_osc_set_parent(struct clk_hw *hw, u8 index)
 {
 	struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
+	unsigned long flags;
 	u32 val;
 
 	if (index > 1)
 		return -EINVAL;
 
+	spin_lock_irqsave(&rtc->lock, flags);
 	val = readl(rtc->base + SUN6I_LOSC_CTRL);
 	val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
 	val |= SUN6I_LOSC_CTRL_KEY;
 	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
 	writel(val, rtc->base + SUN6I_LOSC_CTRL);
+	spin_unlock_irqrestore(&rtc->lock, flags);
 
 	return 0;
 }
@@ -187,6 +192,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
 	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
 	if (!rtc)
 		pr_crit("Can't allocate RTC structure\n");
+	spin_lock_init(&rtc->lock);
 
 	rtc->base = of_io_request_and_map(node, 0, of_node_full_name(node));
 	if (!rtc->base) {
@@ -246,8 +252,10 @@ CLK_OF_DECLARE_DRIVER(sun6i_rtc_clk, "allwinner,sun6i-a31-rtc",
 static irqreturn_t sun6i_rtc_alarmirq(int irq, void *id)
 {
 	struct sun6i_rtc_dev *chip = (struct sun6i_rtc_dev *) id;
+	irqreturn_t ret = IRQ_NONE;
 	u32 val;
 
+	spin_lock(&chip->lock);
 	val = readl(chip->base + SUN6I_ALRM_IRQ_STA);
 
 	if (val & SUN6I_ALRM_IRQ_STA_CNT_IRQ_PEND) {
@@ -256,10 +264,11 @@ static irqreturn_t sun6i_rtc_alarmirq(int irq, void *id)
 
 		rtc_update_irq(chip->rtc, 1, RTC_AF | RTC_IRQF);
 
-		return IRQ_HANDLED;
+		ret = IRQ_HANDLED;
 	}
+	spin_unlock(&chip->lock);
 
-	return IRQ_NONE;
+	return ret;
 }
 
 static void sun6i_rtc_setaie(int to, struct sun6i_rtc_dev *chip)
@@ -267,6 +276,7 @@ static void sun6i_rtc_setaie(int to, struct sun6i_rtc_dev *chip)
 	u32 alrm_val = 0;
 	u32 alrm_irq_val = 0;
 	u32 alrm_wake_val = 0;
+	unsigned long flags;
 
 	if (to) {
 		alrm_val = SUN6I_ALRM_EN_CNT_EN;
@@ -277,9 +287,11 @@ static void sun6i_rtc_setaie(int to, struct sun6i_rtc_dev *chip)
 		       chip->base + SUN6I_ALRM_IRQ_STA);
 	}
 
+	spin_lock_irqsave(&chip->lock, flags);
 	writel(alrm_val, chip->base + SUN6I_ALRM_EN);
 	writel(alrm_irq_val, chip->base + SUN6I_ALRM_IRQ_EN);
 	writel(alrm_wake_val, chip->base + SUN6I_ALARM_CONFIG);
+	spin_unlock_irqrestore(&chip->lock, flags);
 }
 
 static int sun6i_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
@@ -318,11 +330,15 @@ static int sun6i_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 static int sun6i_rtc_getalarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 {
 	struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
+	unsigned long flags;
 	u32 alrm_st;
 	u32 alrm_en;
 
+	spin_lock_irqsave(&chip->lock, flags);
 	alrm_en = readl(chip->base + SUN6I_ALRM_IRQ_EN);
 	alrm_st = readl(chip->base + SUN6I_ALRM_IRQ_STA);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
 	wkalrm->enabled = !!(alrm_en & SUN6I_ALRM_EN_CNT_EN);
 	wkalrm->pending = !!(alrm_st & SUN6I_ALRM_EN_CNT_EN);
 	rtc_time_to_tm(chip->alarm, &wkalrm->time);
-- 
git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/6] rtc: sun6i: Add some locking

[Chen-Yu Tsai]

On Fri, Jan 20, 2017 at 11:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Some registers have a read-modify-write access pattern that are not atomic.
>
> Add some locking to prevent from concurrent accesses.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

Though I think it would be better to add locking first, then add
clock support, if the original code already had issues?

> ---
>  drivers/rtc/rtc-sun6i.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index 408dd512a6ac..872d18609183 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -125,6 +125,8 @@ struct sun6i_rtc_dev {
>         struct clk_hw hw;
>         struct clk_hw *int_osc;
>         struct clk *losc;
> +
> +       spinlock_t lock;
>  };
>
>  static struct sun6i_rtc_dev *sun6i_rtc;
> @@ -155,16 +157,19 @@ static u8 sun6i_rtc_osc_get_parent(struct clk_hw *hw)
>  static int sun6i_rtc_osc_set_parent(struct clk_hw *hw, u8 index)
>  {
>         struct sun6i_rtc_dev *rtc = container_of(hw, struct sun6i_rtc_dev, hw);
> +       unsigned long flags;
>         u32 val;
>
>         if (index > 1)
>                 return -EINVAL;
>
> +       spin_lock_irqsave(&rtc->lock, flags);
>         val = readl(rtc->base + SUN6I_LOSC_CTRL);
>         val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
>         val |= SUN6I_LOSC_CTRL_KEY;
>         val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
>         writel(val, rtc->base + SUN6I_LOSC_CTRL);
> +       spin_unlock_irqrestore(&rtc->lock, flags);
>
>         return 0;
>  }
> @@ -187,6 +192,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
>         rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
>         if (!rtc)
>                 pr_crit("Can't allocate RTC structure\n");
> +       spin_lock_init(&rtc->lock);
>
>         rtc->base = of_io_request_and_map(node, 0, of_node_full_name(node));
>         if (!rtc->base) {
> @@ -246,8 +252,10 @@ CLK_OF_DECLARE_DRIVER(sun6i_rtc_clk, "allwinner,sun6i-a31-rtc",
>  static irqreturn_t sun6i_rtc_alarmirq(int irq, void *id)
>  {
>         struct sun6i_rtc_dev *chip = (struct sun6i_rtc_dev *) id;
> +       irqreturn_t ret = IRQ_NONE;
>         u32 val;
>
> +       spin_lock(&chip->lock);
>         val = readl(chip->base + SUN6I_ALRM_IRQ_STA);
>
>         if (val & SUN6I_ALRM_IRQ_STA_CNT_IRQ_PEND) {
> @@ -256,10 +264,11 @@ static irqreturn_t sun6i_rtc_alarmirq(int irq, void *id)
>
>                 rtc_update_irq(chip->rtc, 1, RTC_AF | RTC_IRQF);
>
> -               return IRQ_HANDLED;
> +               ret = IRQ_HANDLED;
>         }
> +       spin_unlock(&chip->lock);
>
> -       return IRQ_NONE;
> +       return ret;
>  }
>
>  static void sun6i_rtc_setaie(int to, struct sun6i_rtc_dev *chip)
> @@ -267,6 +276,7 @@ static void sun6i_rtc_setaie(int to, struct sun6i_rtc_dev *chip)
>         u32 alrm_val = 0;
>         u32 alrm_irq_val = 0;
>         u32 alrm_wake_val = 0;
> +       unsigned long flags;
>
>         if (to) {
>                 alrm_val = SUN6I_ALRM_EN_CNT_EN;
> @@ -277,9 +287,11 @@ static void sun6i_rtc_setaie(int to, struct sun6i_rtc_dev *chip)
>                        chip->base + SUN6I_ALRM_IRQ_STA);
>         }
>
> +       spin_lock_irqsave(&chip->lock, flags);
>         writel(alrm_val, chip->base + SUN6I_ALRM_EN);
>         writel(alrm_irq_val, chip->base + SUN6I_ALRM_IRQ_EN);
>         writel(alrm_wake_val, chip->base + SUN6I_ALARM_CONFIG);
> +       spin_unlock_irqrestore(&chip->lock, flags);
>  }
>
>  static int sun6i_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
> @@ -318,11 +330,15 @@ static int sun6i_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
>  static int sun6i_rtc_getalarm(struct device *dev, struct rtc_wkalrm *wkalrm)
>  {
>         struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
> +       unsigned long flags;
>         u32 alrm_st;
>         u32 alrm_en;
>
> +       spin_lock_irqsave(&chip->lock, flags);
>         alrm_en = readl(chip->base + SUN6I_ALRM_IRQ_EN);
>         alrm_st = readl(chip->base + SUN6I_ALRM_IRQ_STA);
> +       spin_unlock_irqrestore(&chip->lock, flags);
> +
>         wkalrm->enabled = !!(alrm_en & SUN6I_ALRM_EN_CNT_EN);
>         wkalrm->pending = !!(alrm_st & SUN6I_ALRM_EN_CNT_EN);
>         rtc_time_to_tm(chip->alarm, &wkalrm->time);
> --
> git-series 0.8.11
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] rtc: sun6i: Disable the build as a module

[Maxime Ripard]

Since we have to provide the clock very early on, the RTC driver cannot be
built as a module. Make sure that won't happen.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/rtc/Kconfig     |  2 +-
 drivers/rtc/rtc-sun6i.c | 17 +----------------
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index c93c5a8fba32..53e35c138ff3 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1434,7 +1434,7 @@ config RTC_DRV_SUN4V
 	  based RTC on SUN4V systems.
 
 config RTC_DRV_SUN6I
-	tristate "Allwinner A31 RTC"
+	bool "Allwinner A31 RTC"
 	default MACH_SUN6I || MACH_SUN8I || COMPILE_TEST
 	depends on ARCH_SUNXI
 	help
diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 872d18609183..edd5627da10f 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -554,15 +554,6 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int sun6i_rtc_remove(struct platform_device *pdev)
-{
-	struct sun6i_rtc_dev *chip = platform_get_drvdata(pdev);
-
-	rtc_device_unregister(chip->rtc);
-
-	return 0;
-}
-
 static const struct of_device_id sun6i_rtc_dt_ids[] = {
 	{ .compatible = "allwinner,sun6i-a31-rtc" },
 	{ /* sentinel */ },
@@ -571,15 +562,9 @@ MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
 
 static struct platform_driver sun6i_rtc_driver = {
 	.probe		= sun6i_rtc_probe,
-	.remove		= sun6i_rtc_remove,
 	.driver		= {
 		.name		= "sun6i-rtc",
 		.of_match_table = sun6i_rtc_dt_ids,
 	},
 };
-
-module_platform_driver(sun6i_rtc_driver);
-
-MODULE_DESCRIPTION("sun6i RTC driver");
-MODULE_AUTHOR("Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>");
-MODULE_LICENSE("GPL");
+builtin_platform_driver(sun6i_rtc_driver);
-- 
git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 3/6] rtc: sun6i: Disable the build as a module

[Chen-Yu Tsai]

Hi,

On Fri, Jan 20, 2017 at 11:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Since we have to provide the clock very early on, the RTC driver cannot be
> built as a module. Make sure that won't happen.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/rtc/Kconfig     |  2 +-
>  drivers/rtc/rtc-sun6i.c | 17 +----------------
>  2 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index c93c5a8fba32..53e35c138ff3 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1434,7 +1434,7 @@ config RTC_DRV_SUN4V
>           based RTC on SUN4V systems.
>
>  config RTC_DRV_SUN6I
> -       tristate "Allwinner A31 RTC"
> +       bool "Allwinner A31 RTC"
>         default MACH_SUN6I || MACH_SUN8I || COMPILE_TEST
>         depends on ARCH_SUNXI
>         help
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index 872d18609183..edd5627da10f 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -554,15 +554,6 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
>         return 0;
>  }
>
> -static int sun6i_rtc_remove(struct platform_device *pdev)
> -{
> -       struct sun6i_rtc_dev *chip = platform_get_drvdata(pdev);
> -
> -       rtc_device_unregister(chip->rtc);
> -
> -       return 0;
> -}
> -
>  static const struct of_device_id sun6i_rtc_dt_ids[] = {
>         { .compatible = "allwinner,sun6i-a31-rtc" },
>         { /* sentinel */ },
> @@ -571,15 +562,9 @@ MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
>
>  static struct platform_driver sun6i_rtc_driver = {
>         .probe          = sun6i_rtc_probe,
> -       .remove         = sun6i_rtc_remove,

I might have gotten this wrong in the past. It seems that the
.remove callback can still be called, even if the driver is
builtin, by unbinding the driver from the device using the
sysfs interface. So we still need it regardless.

We can get rid of the .remove callback by switching to
devm_rtc_device_register, but that is a separate issue.

Also, a personal preference, is that this change should precede
the clk change you mentioned in the commit message, since you
are making this change in anticipation of possible breakage
by that one.

Regards
ChenYu

>         .driver         = {
>                 .name           = "sun6i-rtc",
>                 .of_match_table = sun6i_rtc_dt_ids,
>         },
>  };
> -
> -module_platform_driver(sun6i_rtc_driver);
> -
> -MODULE_DESCRIPTION("sun6i RTC driver");
> -MODULE_AUTHOR("Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>");
> -MODULE_LICENSE("GPL");
> +builtin_platform_driver(sun6i_rtc_driver);
> --
> git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 4/6] rtc: sun6i: Force the mux to the external oscillator

[Maxime Ripard]

The internal oscillator is way too inaccurate to do something useful with
it. Switch to the external oscillator if it is available.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/rtc/rtc-sun6i.c | 12 ++++++++++++
 1 file changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index edd5627da10f..1695fae24cd5 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -493,6 +493,7 @@ static const struct rtc_class_ops sun6i_rtc_ops = {
 static int sun6i_rtc_probe(struct platform_device *pdev)
 {
 	struct sun6i_rtc_dev *chip = sun6i_rtc;
+	struct clk *parent;
 	int ret;
 
 	if (!chip)
@@ -540,6 +541,17 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
 	/* disable alarm wakeup */
 	writel(0, chip->base + SUN6I_ALARM_CONFIG);
 
+	parent = clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(parent)) {
+		ret = clk_set_parent(chip->losc, parent);
+		clk_put(parent);
+
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed to reparent the RTC to the external oscillator\n");
+			return ret;
+		}
+	}
 	clk_prepare_enable(chip->losc);
 
 	chip->rtc = rtc_device_register("rtc-sun6i", &pdev->dev,
-- 
git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 4/6] rtc: sun6i: Force the mux to the external oscillator

[Chen-Yu Tsai]

Hi,

On Fri, Jan 20, 2017 at 11:56 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The internal oscillator is way too inaccurate to do something useful with
> it. Switch to the external oscillator if it is available.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/rtc/rtc-sun6i.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index edd5627da10f..1695fae24cd5 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -493,6 +493,7 @@ static const struct rtc_class_ops sun6i_rtc_ops = {
>  static int sun6i_rtc_probe(struct platform_device *pdev)
>  {
>         struct sun6i_rtc_dev *chip = sun6i_rtc;
> +       struct clk *parent;
>         int ret;
>
>         if (!chip)
> @@ -540,6 +541,17 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
>         /* disable alarm wakeup */
>         writel(0, chip->base + SUN6I_ALARM_CONFIG);
>
> +       parent = clk_get(&pdev->dev, NULL);
> +       if (!IS_ERR(parent)) {
> +               ret = clk_set_parent(chip->losc, parent);
> +               clk_put(parent);
> +
> +               if (ret) {
> +                       dev_err(&pdev->dev,
> +                               "Failed to reparent the RTC to the external oscillator\n");
> +                       return ret;
> +               }
> +       }

Following what I mentioned in patch 1, maybe it is easier to force
the mux before the clocks are registered by writing directly to
the registers? We could also backport the changes to stable?

ChenYu

>         clk_prepare_enable(chip->losc);
>
>         chip->rtc = rtc_device_register("rtc-sun6i", &pdev->dev,
> --
> git-series 0.8.11
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] rtc: sun6i: Force the mux to the external oscillator

[Alexandre Belloni]

On 21/01/2017 at 11:53:08 +0800, Chen-Yu Tsai wrote :
> Hi,
> 
> On Fri, Jan 20, 2017 at 11:56 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > The internal oscillator is way too inaccurate to do something useful with
> > it. Switch to the external oscillator if it is available.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/rtc/rtc-sun6i.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index edd5627da10f..1695fae24cd5 100644
> > --- a/drivers/rtc/rtc-sun6i.c
> > +++ b/drivers/rtc/rtc-sun6i.c
> > @@ -493,6 +493,7 @@ static const struct rtc_class_ops sun6i_rtc_ops = {
> >  static int sun6i_rtc_probe(struct platform_device *pdev)
> >  {
> >         struct sun6i_rtc_dev *chip = sun6i_rtc;
> > +       struct clk *parent;
> >         int ret;
> >
> >         if (!chip)
> > @@ -540,6 +541,17 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
> >         /* disable alarm wakeup */
> >         writel(0, chip->base + SUN6I_ALARM_CONFIG);
> >
> > +       parent = clk_get(&pdev->dev, NULL);
> > +       if (!IS_ERR(parent)) {
> > +               ret = clk_set_parent(chip->losc, parent);
> > +               clk_put(parent);
> > +
> > +               if (ret) {
> > +                       dev_err(&pdev->dev,
> > +                               "Failed to reparent the RTC to the external oscillator\n");
> > +                       return ret;
> > +               }
> > +       }
> 
> Following what I mentioned in patch 1, maybe it is easier to force
> the mux before the clocks are registered by writing directly to
> the registers? We could also backport the changes to stable?
> 

I'd say that the risk to break existing platforms is low enough to
backport that patch to stable.
Maxime as you are certainly the one that will handle the potential
breakage, I'll let you choose what you want to do.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 5/6] ARM: sun8i: a23/a33: Enable the real LOSC and use it

[Maxime Ripard]

So far, the LOSC was generated through the RTC internal oscillator, which
was a pretty poor and inaccurate choice.

Now that the RTC properly exposes its internal mux between its oscillator
and the external oscillator, we can use it were relevant.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a23-a33.dtsi | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
index 597f2fce6c31..e9d98d803dc6 100644
--- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
@@ -109,11 +109,11 @@
 			clock-output-names = "osc24M";
 		};
 
-		osc32k: osc32k_clk {
+		ext_osc32k: ext_osc32k_clk {
 			#clock-cells = <0>;
 			compatible = "fixed-clock";
 			clock-frequency = <32768>;
-			clock-output-names = "osc32k";
+			clock-output-names = "ext-osc32k";
 		};
 	};
 
@@ -256,7 +256,7 @@
 
 		ccu: clock@01c20000 {
 			reg = <0x01c20000 0x400>;
-			clocks = <&osc24M>, <&osc32k>;
+			clocks = <&osc24M>, <&rtc>;
 			clock-names = "hosc", "losc";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
@@ -266,7 +266,7 @@
 			/* compatible gets set in SoC specific dtsi file */
 			reg = <0x01c20800 0x400>;
 			/* interrupts get set in SoC specific dtsi file */
-			clocks = <&ccu CLK_BUS_PIO>, <&osc24M>, <&osc32k>;
+			clocks = <&ccu CLK_BUS_PIO>, <&osc24M>, <&rtc>;
 			clock-names = "apb", "hosc", "losc";
 			gpio-controller;
 			interrupt-controller;
@@ -486,6 +486,9 @@
 			reg = <0x01f00000 0x54>;
 			interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+			clock-output-names = "osc32k";
+			clocks = <&ext_osc32k>;
+			#clock-cells = <0>;
 		};
 
 		nmi_intc: interrupt-controller@01f00c0c {
@@ -564,7 +567,7 @@
 			compatible = "allwinner,sun8i-a23-r-pinctrl";
 			reg = <0x01f02c00 0x400>;
 			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&apb0_gates 0>, <&osc24M>, <&osc32k>;
+			clocks = <&apb0_gates 0>, <&osc24M>, <&rtc>;
 			clock-names = "apb", "hosc", "losc";
 			resets = <&apb0_rst 0>;
 			gpio-controller;
-- 
git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 6/6] ARM: sun8i: a23/a33: Add the oscillators accuracy

[Maxime Ripard]

The datasheet provided by Allwinner requires oscillators with an accuracy
of 50ppm. Add it to our fixed clocks so that we can properly track the
accuracy chain.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a23-a33.dtsi | 2 ++
 1 file changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
index e9d98d803dc6..f5925a18031c 100644
--- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
@@ -106,6 +106,7 @@
 			#clock-cells = <0>;
 			compatible = "fixed-clock";
 			clock-frequency = <24000000>;
+			clock-accuracy = <50000>;
 			clock-output-names = "osc24M";
 		};
 
@@ -113,6 +114,7 @@
 			#clock-cells = <0>;
 			compatible = "fixed-clock";
 			clock-frequency = <32768>;
+			clock-accuracy = <50000>;
 			clock-output-names = "ext-osc32k";
 		};
 	};
-- 
git-series 0.8.11

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 0/6] rtc: sun6i: Fix the RTC accuracy

[Alexandre Belloni]

On 20/01/2017 at 16:56:37 +0100, Maxime Ripard wrote :
> Hi,
> 
> The RTC used in the A31 and later SoC has an accuracy issue, which is
> already significant even after a couple of hours.
> 
> This is due to the fact that the oscillator used by default is an internal
> and very inaccurate one.
> 
> A first attempt at fixing that by switching to the external oscillator was
> done in the patch "rtc: sun6i: Switch to the external oscillator". However,
> it turned out to be problematic since it was tracked properly in the clock
> framework, which might lead to some clocks being disabled, even though
> their devices were not notified.
> 
> This is a second attempt, this time by making it part of the CCF. It
> turned out to be a bit more complicated than one would expect since the mux
> found inside the RTC also controls one of the input of the main clock unit.
> Therefore, it needs to be probed before the main clock unit driver.
> 
> Let me know what you think,
> Maxime
> 
> Maxime Ripard (6):
>   rtc: sun6i: Expose the 32kHz oscillator
>   rtc: sun6i: Add some locking
>   rtc: sun6i: Disable the build as a module
>   rtc: sun6i: Force the mux to the external oscillator
>   ARM: sun8i: a23/a33: Enable the real LOSC and use it
>   ARM: sun8i: a23/a33: Add the oscillators accuracy
> 

As Chen-Yu mentioned, maybe you can reorder some patches so they can go
in stable.

>  Documentation/devicetree/bindings/rtc/sun6i-rtc.txt |   8 +-
>  arch/arm/boot/dts/sun8i-a23-a33.dtsi                |  15 +-
>  drivers/rtc/Kconfig                                 |   2 +-
>  drivers/rtc/rtc-sun6i.c                             | 189 +++++++++++--
>  4 files changed, 181 insertions(+), 33 deletions(-)
> 
> base-commit: 99cef370ac9939df2aeb16c96d07e842b2fa8201
> -- 
> git-series 0.8.11

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.