| Message ID | 20200213211427.33004-2-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rtc: sun6i: Make external oscillator optional | expand |
Hi Jernej, Thanks for taking care of this On Thu, Feb 13, 2020 at 10:14:26PM +0100, Jernej Skrabec wrote: > Some boards, like OrangePi PC2 (H5), OrangePi Plus 2E (H3) and Tanix TX6 > (H6) don't have external 32kHz oscillator. Till H6, it didn't really > matter if external oscillator was enabled because HW detected error and > fall back to internal one. H6 has same functionality but it's the first > SoC which have "auto switch bypass" bit documented and always enabled in > driver. This prevents RTC to work correctly if external crystal is not > present on board. There are other side effects - all peripherals which > depends on this clock also don't work (HDMI CEC for example). > > Make clocks property optional. If it is present, select external > oscillator. If not, stay on internal. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/rtc/rtc-sun6i.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c > index 852f5f3b3592..538cf7e19034 100644 > --- a/drivers/rtc/rtc-sun6i.c > +++ b/drivers/rtc/rtc-sun6i.c > @@ -250,19 +250,17 @@ static void __init sun6i_rtc_clk_init(struct device_node *node, > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > } > > - /* Switch to the external, more precise, oscillator */ > - reg |= SUN6I_LOSC_CTRL_EXT_OSC; > - if (rtc->data->has_losc_en) > - reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > + /* Switch to the external, more precise, oscillator, if present */ > + if (of_get_property(node, "clocks", NULL)) { > + reg |= SUN6I_LOSC_CTRL_EXT_OSC; > + if (rtc->data->has_losc_en) > + reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > + } > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > > /* Yes, I know, this is ugly. */ > sun6i_rtc = rtc; > > - /* Deal with old DTs */ > - if (!of_get_property(node, "clocks", NULL)) > - goto err; > - Doesn't that prevent the parents to be properly set if there's an external crystal? Maxime
Hi Maxime, Dne petek, 14. februar 2020 ob 09:14:43 CET je Maxime Ripard napisal(a): > Hi Jernej, > > Thanks for taking care of this > > On Thu, Feb 13, 2020 at 10:14:26PM +0100, Jernej Skrabec wrote: > > Some boards, like OrangePi PC2 (H5), OrangePi Plus 2E (H3) and Tanix TX6 > > (H6) don't have external 32kHz oscillator. Till H6, it didn't really > > matter if external oscillator was enabled because HW detected error and > > fall back to internal one. H6 has same functionality but it's the first > > SoC which have "auto switch bypass" bit documented and always enabled in > > driver. This prevents RTC to work correctly if external crystal is not > > present on board. There are other side effects - all peripherals which > > depends on this clock also don't work (HDMI CEC for example). > > > > Make clocks property optional. If it is present, select external > > oscillator. If not, stay on internal. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/rtc/rtc-sun6i.c | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c > > index 852f5f3b3592..538cf7e19034 100644 > > --- a/drivers/rtc/rtc-sun6i.c > > +++ b/drivers/rtc/rtc-sun6i.c > > @@ -250,19 +250,17 @@ static void __init sun6i_rtc_clk_init(struct > > device_node *node,> > > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > > > > } > > > > - /* Switch to the external, more precise, oscillator */ > > - reg |= SUN6I_LOSC_CTRL_EXT_OSC; > > - if (rtc->data->has_losc_en) > > - reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > > + /* Switch to the external, more precise, oscillator, if present */ > > + if (of_get_property(node, "clocks", NULL)) { > > + reg |= SUN6I_LOSC_CTRL_EXT_OSC; > > + if (rtc->data->has_losc_en) > > + reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > > + } > > > > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > > > > /* Yes, I know, this is ugly. */ > > sun6i_rtc = rtc; > > > > - /* Deal with old DTs */ > > - if (!of_get_property(node, "clocks", NULL)) > > - goto err; > > - > > Doesn't that prevent the parents to be properly set if there's an > external crystal? No, why? Check these two clk_summary: http://ix.io/2bHY Tanix TX6 (no external crystal) http://ix.io/2bI2 OrangePi 3 (external crystal present) Please disregard ac200_clk in first case, it's part of another work. Best regards, Jernej
On Fri, Feb 14, 2020 at 05:42:13PM +0100, Jernej Škrabec wrote: > Hi Maxime, > > Dne petek, 14. februar 2020 ob 09:14:43 CET je Maxime Ripard napisal(a): > > Hi Jernej, > > > > Thanks for taking care of this > > > > On Thu, Feb 13, 2020 at 10:14:26PM +0100, Jernej Skrabec wrote: > > > Some boards, like OrangePi PC2 (H5), OrangePi Plus 2E (H3) and Tanix TX6 > > > (H6) don't have external 32kHz oscillator. Till H6, it didn't really > > > matter if external oscillator was enabled because HW detected error and > > > fall back to internal one. H6 has same functionality but it's the first > > > SoC which have "auto switch bypass" bit documented and always enabled in > > > driver. This prevents RTC to work correctly if external crystal is not > > > present on board. There are other side effects - all peripherals which > > > depends on this clock also don't work (HDMI CEC for example). > > > > > > Make clocks property optional. If it is present, select external > > > oscillator. If not, stay on internal. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/rtc/rtc-sun6i.c | 14 ++++++-------- > > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c > > > index 852f5f3b3592..538cf7e19034 100644 > > > --- a/drivers/rtc/rtc-sun6i.c > > > +++ b/drivers/rtc/rtc-sun6i.c > > > @@ -250,19 +250,17 @@ static void __init sun6i_rtc_clk_init(struct > > > device_node *node,> > > > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > > > > > > } > > > > > > - /* Switch to the external, more precise, oscillator */ > > > - reg |= SUN6I_LOSC_CTRL_EXT_OSC; > > > - if (rtc->data->has_losc_en) > > > - reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > > > + /* Switch to the external, more precise, oscillator, if present */ > > > + if (of_get_property(node, "clocks", NULL)) { > > > + reg |= SUN6I_LOSC_CTRL_EXT_OSC; > > > + if (rtc->data->has_losc_en) > > > + reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > > > + } > > > > > > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > > > > > > /* Yes, I know, this is ugly. */ > > > sun6i_rtc = rtc; > > > > > > - /* Deal with old DTs */ > > > - if (!of_get_property(node, "clocks", NULL)) > > > - goto err; > > > - > > > > Doesn't that prevent the parents to be properly set if there's an > > external crystal? > > No, why? > > Check these two clk_summary: > http://ix.io/2bHY Tanix TX6 (no external crystal) > http://ix.io/2bI2 OrangePi 3 (external crystal present) I was concerned about the "other" parent. In the case where you don't have a clocks property (so the check that you are removing), the code then registers a clock with two parents: the one that we create (the internal oscillator) and the one coming from the clocks property. clk_summary only shows the current parent, which is going to be right with your patch, but in the case where you have no clocks property, you still (attempts to) register two parents, the second one being non-functional. Further looking at it, we might be good because we allocate an array of two clocks, but only register of_clk_get_parent_count(node) + 1 clocks, so 1 if clocks is missing. Still, I think this should be more obvious, through a comment or shuffling a bit the parent registration maybe? Maxime
Dne četrtek, 20. februar 2020 ob 18:47:49 CET je Maxime Ripard napisal(a): > On Fri, Feb 14, 2020 at 05:42:13PM +0100, Jernej Škrabec wrote: > > Hi Maxime, > > > > Dne petek, 14. februar 2020 ob 09:14:43 CET je Maxime Ripard napisal(a): > > > Hi Jernej, > > > > > > Thanks for taking care of this > > > > > > On Thu, Feb 13, 2020 at 10:14:26PM +0100, Jernej Skrabec wrote: > > > > Some boards, like OrangePi PC2 (H5), OrangePi Plus 2E (H3) and Tanix > > > > TX6 > > > > (H6) don't have external 32kHz oscillator. Till H6, it didn't really > > > > matter if external oscillator was enabled because HW detected error > > > > and > > > > fall back to internal one. H6 has same functionality but it's the > > > > first > > > > SoC which have "auto switch bypass" bit documented and always enabled > > > > in > > > > driver. This prevents RTC to work correctly if external crystal is not > > > > present on board. There are other side effects - all peripherals which > > > > depends on this clock also don't work (HDMI CEC for example). > > > > > > > > Make clocks property optional. If it is present, select external > > > > oscillator. If not, stay on internal. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > --- > > > > > > > > drivers/rtc/rtc-sun6i.c | 14 ++++++-------- > > > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c > > > > index 852f5f3b3592..538cf7e19034 100644 > > > > --- a/drivers/rtc/rtc-sun6i.c > > > > +++ b/drivers/rtc/rtc-sun6i.c > > > > @@ -250,19 +250,17 @@ static void __init sun6i_rtc_clk_init(struct > > > > device_node *node,> > > > > > > > > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > > > > > > > > } > > > > > > > > - /* Switch to the external, more precise, oscillator */ > > > > - reg |= SUN6I_LOSC_CTRL_EXT_OSC; > > > > - if (rtc->data->has_losc_en) > > > > - reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > > > > + /* Switch to the external, more precise, oscillator, if present */ > > > > + if (of_get_property(node, "clocks", NULL)) { > > > > + reg |= SUN6I_LOSC_CTRL_EXT_OSC; > > > > + if (rtc->data->has_losc_en) > > > > + reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; > > > > + } > > > > > > > > writel(reg, rtc->base + SUN6I_LOSC_CTRL); > > > > > > > > /* Yes, I know, this is ugly. */ > > > > sun6i_rtc = rtc; > > > > > > > > - /* Deal with old DTs */ > > > > - if (!of_get_property(node, "clocks", NULL)) > > > > - goto err; > > > > - > > > > > > Doesn't that prevent the parents to be properly set if there's an > > > external crystal? > > > > No, why? > > > > Check these two clk_summary: > > http://ix.io/2bHY Tanix TX6 (no external crystal) > > http://ix.io/2bI2 OrangePi 3 (external crystal present) > > I was concerned about the "other" parent. In the case where you don't > have a clocks property (so the check that you are removing), the code > then registers a clock with two parents: the one that we create (the > internal oscillator) and the one coming from the clocks property. > > clk_summary only shows the current parent, which is going to be right > with your patch, but in the case where you have no clocks property, > you still (attempts to) register two parents, the second one being > non-functional. > > Further looking at it, we might be good because we allocate an array > of two clocks, but only register of_clk_get_parent_count(node) + 1 > clocks, so 1 if clocks is missing. Yes, my patch rely on "of_clk_get_parent_count(node) + 1". If there is no property, it will return 1 thus only first parent (internal RC oscilator) will be registered. Anyway, following line: parents[1] = of_clk_get_parent_name(node, 0); should evaluate to null. I didn't research further what clk framework does with null parent because number of parents will be set to 1 and this null value will be ignored anyway. > > Still, I think this should be more obvious, through a comment or > shuffling a bit the parent registration maybe? I think code is in correct order, just maybe a bit more explanation in form of comment(s) to make it more obvious how it works for either case. Best regards, Jernej
diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c index 852f5f3b3592..538cf7e19034 100644 --- a/drivers/rtc/rtc-sun6i.c +++ b/drivers/rtc/rtc-sun6i.c @@ -250,19 +250,17 @@ static void __init sun6i_rtc_clk_init(struct device_node *node, writel(reg, rtc->base + SUN6I_LOSC_CTRL); } - /* Switch to the external, more precise, oscillator */ - reg |= SUN6I_LOSC_CTRL_EXT_OSC; - if (rtc->data->has_losc_en) - reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; + /* Switch to the external, more precise, oscillator, if present */ + if (of_get_property(node, "clocks", NULL)) { + reg |= SUN6I_LOSC_CTRL_EXT_OSC; + if (rtc->data->has_losc_en) + reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN; + } writel(reg, rtc->base + SUN6I_LOSC_CTRL); /* Yes, I know, this is ugly. */ sun6i_rtc = rtc; - /* Deal with old DTs */ - if (!of_get_property(node, "clocks", NULL)) - goto err; - /* Only read IOSC name from device tree if it is exported */ if (rtc->data->export_iosc) of_property_read_string_index(node, "clock-output-names", 2,
Some boards, like OrangePi PC2 (H5), OrangePi Plus 2E (H3) and Tanix TX6 (H6) don't have external 32kHz oscillator. Till H6, it didn't really matter if external oscillator was enabled because HW detected error and fall back to internal one. H6 has same functionality but it's the first SoC which have "auto switch bypass" bit documented and always enabled in driver. This prevents RTC to work correctly if external crystal is not present on board. There are other side effects - all peripherals which depends on this clock also don't work (HDMI CEC for example). Make clocks property optional. If it is present, select external oscillator. If not, stay on internal. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/rtc/rtc-sun6i.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)