Hi Andrew,
Apologies for replying late.
On Wednesday 25 March 2015 04:29 AM, Andrew Morton wrote:
> On Tue, 3 Mar 2015 15:12:02 +0530 Keerthy <[email protected]> wrote:
>
>> Add external 32k clock feature. The internal clock will be gated during suspend.
>> Hence make use of the external 32k clock so that rtc is functional accross
>> suspend/resume.
>>
>> ...
>>
>> @@ -446,6 +449,7 @@ static const struct omap_rtc_device_type omap_rtc_default_type = {
>>
>> static const struct omap_rtc_device_type omap_rtc_am3352_type = {
>> .has_32kclk_en = true,
>> + .has_osc_ext_32k = true,
>> .has_kicker = true,
>> .has_irqwakeen = true,
>> .has_pmic_mode = true,
>> @@ -543,7 +547,16 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
>> if (rtc->type->has_32kclk_en) {
>> reg = rtc_read(rtc, OMAP_RTC_OSC_REG);
>> rtc_writel(rtc, OMAP_RTC_OSC_REG,
>> - reg | OMAP_RTC_OSC_32KCLK_EN);
>> + reg | OMAP_RTC_OSC_32KCLK_EN);
>> + }
>> +
>> + /* Enable External clock as the source */
>> +
>> + if (rtc->type->has_osc_ext_32k) {
>> + rtc_writel(rtc, OMAP_RTC_OSC_REG,
>> + (OMAP_RTC_OSC_EXT_32K |
>> + rtc_read(rtc, OMAP_RTC_OSC_REG)) &
>> + (~OMAP_RTC_OSC_OSC32K_GZ));
>> }
>
> How do we know that all systems have this external clock and that it
> works OK?
>
AM335 and AM43X have the external clock feature which we choose using
RTC_OSC_REG. I verified it works OK by seeing the RTC seconds ticking
even after switching the source to the external 32k Clock.
Regards,
Keerthy
Hi,
On 04/07/15 06:29, Keerthy wrote:
> Hi Andrew,
>
> Apologies for replying late.
>
> On Wednesday 25 March 2015 04:29 AM, Andrew Morton wrote:
>> On Tue, 3 Mar 2015 15:12:02 +0530 Keerthy <[email protected]> wrote:
>>
>>> Add external 32k clock feature. The internal clock will be gated during suspend.
>>> Hence make use of the external 32k clock so that rtc is functional accross
>>> suspend/resume.
>>>
>>> ...
>>>
>>> @@ -446,6 +449,7 @@ static const struct omap_rtc_device_type omap_rtc_default_type = {
>>>
>>> static const struct omap_rtc_device_type omap_rtc_am3352_type = {
>>> .has_32kclk_en = true,
>>> + .has_osc_ext_32k = true,
>>> .has_kicker = true,
>>> .has_irqwakeen = true,
>>> .has_pmic_mode = true,
>>> @@ -543,7 +547,16 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
>>> if (rtc->type->has_32kclk_en) {
>>> reg = rtc_read(rtc, OMAP_RTC_OSC_REG);
>>> rtc_writel(rtc, OMAP_RTC_OSC_REG,
>>> - reg | OMAP_RTC_OSC_32KCLK_EN);
>>> + reg | OMAP_RTC_OSC_32KCLK_EN);
>>> + }
>>> +
>>> + /* Enable External clock as the source */
>>> +
>>> + if (rtc->type->has_osc_ext_32k) {
>>> + rtc_writel(rtc, OMAP_RTC_OSC_REG,
>>> + (OMAP_RTC_OSC_EXT_32K |
>>> + rtc_read(rtc, OMAP_RTC_OSC_REG)) &
>>> + (~OMAP_RTC_OSC_OSC32K_GZ));
>>> }
>>
>> How do we know that all systems have this external clock and that it
>> works OK?
>>
>
> AM335 and AM43X have the external clock feature which we choose using
> RTC_OSC_REG. I verified it works OK by seeing the RTC seconds ticking
> even after switching the source to the external 32k Clock.
AFAIU, this is related to the external (outside of SoC) oscillator, right?
If so, there is a possibility to not assemble it on the board (at least
on AM335) and use the internal clock source instead.
There are dozens of boards out there that do not have the external
oscillator assembled.
Am I missing something?
--
Regards,
Igor.
Hi
On Tuesday 07 April 2015 12:36 PM, Igor Grinberg wrote:
> Hi,
>
> On 04/07/15 06:29, Keerthy wrote:
>> Hi Andrew,
>>
>> Apologies for replying late.
>>
>> On Wednesday 25 March 2015 04:29 AM, Andrew Morton wrote:
>>> On Tue, 3 Mar 2015 15:12:02 +0530 Keerthy <[email protected]> wrote:
>>>
>>>> Add external 32k clock feature. The internal clock will be gated during suspend.
>>>> Hence make use of the external 32k clock so that rtc is functional accross
>>>> suspend/resume.
>>>>
>>>> ...
>>>>
>>>> @@ -446,6 +449,7 @@ static const struct omap_rtc_device_type omap_rtc_default_type = {
>>>>
>>>> static const struct omap_rtc_device_type omap_rtc_am3352_type = {
>>>> .has_32kclk_en = true,
>>>> + .has_osc_ext_32k = true,
>>>> .has_kicker = true,
>>>> .has_irqwakeen = true,
>>>> .has_pmic_mode = true,
>>>> @@ -543,7 +547,16 @@ static int __init omap_rtc_probe(struct platform_device *pdev)
>>>> if (rtc->type->has_32kclk_en) {
>>>> reg = rtc_read(rtc, OMAP_RTC_OSC_REG);
>>>> rtc_writel(rtc, OMAP_RTC_OSC_REG,
>>>> - reg | OMAP_RTC_OSC_32KCLK_EN);
>>>> + reg | OMAP_RTC_OSC_32KCLK_EN);
>>>> + }
>>>> +
>>>> + /* Enable External clock as the source */
>>>> +
>>>> + if (rtc->type->has_osc_ext_32k) {
>>>> + rtc_writel(rtc, OMAP_RTC_OSC_REG,
>>>> + (OMAP_RTC_OSC_EXT_32K |
>>>> + rtc_read(rtc, OMAP_RTC_OSC_REG)) &
>>>> + (~OMAP_RTC_OSC_OSC32K_GZ));
>>>> }
>>>
>>> How do we know that all systems have this external clock and that it
>>> works OK?
>>>
>>
>> AM335 and AM43X have the external clock feature which we choose using
>> RTC_OSC_REG. I verified it works OK by seeing the RTC seconds ticking
>> even after switching the source to the external 32k Clock.
>
> AFAIU, this is related to the external (outside of SoC) oscillator, right?
> If so, there is a possibility to not assemble it on the board (at least
> on AM335) and use the internal clock source instead.
Yes. The register 'OMAP_RTC_OSC_REG' is part of the RTC IP register set
so i assumed that it would be with the RTCs of those particular SoCs.
> There are dozens of boards out there that do not have the external
> oscillator assembled.
>
> Am I missing something?
>
Regards,
Keerthy
On Fri, 10 Apr 2015 13:56:54 +0530 Keerthy <[email protected]> wrote:
> >>> How do we know that all systems have this external clock and that it
> >>> works OK?
> >>>
> >>
> >> AM335 and AM43X have the external clock feature which we choose using
> >> RTC_OSC_REG. I verified it works OK by seeing the RTC seconds ticking
> >> even after switching the source to the external 32k Clock.
> >
> > AFAIU, this is related to the external (outside of SoC) oscillator, right?
> > If so, there is a possibility to not assemble it on the board (at least
> > on AM335) and use the internal clock source instead.
>
> Yes. The register 'OMAP_RTC_OSC_REG' is part of the RTC IP register set
> so i assumed that it would be with the RTCs of those particular SoCs.
I'm a bit lost here. AFAICT there's a risk that some manufacturers
have not wired up the external oscillator, so this patch will break
those systems. Do we know for sure that this cannot be the case?
Is there any way in which we can get all systems working properly? If
there's no way of auto-detecting an external oscillator then perhaps a
module parameter is needed. If so, it would be better if the driver
were to default to internal oscillator (ie: current behaviour), and the
module parameter selects the external oscillator. This way, existing
systems are unaffected by the kernel upgrade.
On 10/04/2015 at 13:37:30 -0700, Andrew Morton wrote :
> Is there any way in which we can get all systems working properly? If
> there's no way of auto-detecting an external oscillator then perhaps a
> module parameter is needed. If so, it would be better if the driver
> were to default to internal oscillator (ie: current behaviour), and the
> module parameter selects the external oscillator. This way, existing
> systems are unaffected by the kernel upgrade.
>
Actually, I would use the common clock framework, allowing to chose
between the internal and an external clock source by using the "clocks"
property. This would also allow to have the correct reference count on
that 32k internal clock. It may not matter now but for example, not
doing so became a problem on at91.
As a fallback, if no clocks property is available, I would use the
internal 32k to keep DT ABI backward compatibility.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com