[PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Greg Ungerer]

Configuring for a nommu classic m68k target and enabling the generic rtc
driver (CONFIG_RTC_DRV_GENERIC) will result in the following compile
error:

   m68k-linux-ld: arch/m68k/kernel/time.o: in function `rtc_ioctl':
   time.c:(.text+0x82): undefined reference to `mach_get_rtc_pll'
   m68k-linux-ld: time.c:(.text+0xbc): undefined reference to `mach_set_rtc_pll'
   m68k-linux-ld: time.c:(.text+0xf4): undefined reference to `mach_set_rtc_pll'

"mach_set_rtc_pll" and "mach_get_rtc_pll" are only defined in the common
MMU setup code, and are really only implemented in any meaningful way on
classic M68K MMU enabled machines. So conditionally limit their use to
MMU enabled classic M68K systems.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 arch/m68k/kernel/time.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/m68k/kernel/time.c b/arch/m68k/kernel/time.c
index 340ffeea0a9d..c8b70b425ada 100644
--- a/arch/m68k/kernel/time.c
+++ b/arch/m68k/kernel/time.c
@@ -62,7 +62,7 @@ void timer_heartbeat(void)
 }
 #endif /* CONFIG_HEARTBEAT */
 
-#ifdef CONFIG_M68KCLASSIC
+#if defined(CONFIG_M68KCLASSIC) && defined(CONFIG_MMU)
 #if !IS_BUILTIN(CONFIG_RTC_DRV_GENERIC)
 void read_persistent_clock64(struct timespec64 *ts)
 {
@@ -140,7 +140,7 @@ static int __init rtc_init(void)
 
 module_init(rtc_init);
 #endif /* CONFIG_RTC_DRV_GENERIC */
-#endif /* CONFIG M68KCLASSIC */
+#endif /* CONFIG M68KCLASSIC && CONFIG_MMU */
 
 void __init time_init(void)
 {
-- 
2.25.1

Re: [PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Geert Uytterhoeven]

Hi Greg,

On Fri, May 13, 2022 at 8:50 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
> Configuring for a nommu classic m68k target and enabling the generic rtc
> driver (CONFIG_RTC_DRV_GENERIC) will result in the following compile
> error:
>
>    m68k-linux-ld: arch/m68k/kernel/time.o: in function `rtc_ioctl':
>    time.c:(.text+0x82): undefined reference to `mach_get_rtc_pll'
>    m68k-linux-ld: time.c:(.text+0xbc): undefined reference to `mach_set_rtc_pll'
>    m68k-linux-ld: time.c:(.text+0xf4): undefined reference to `mach_set_rtc_pll'
>
> "mach_set_rtc_pll" and "mach_get_rtc_pll" are only defined in the common
> MMU setup code, and are really only implemented in any meaningful way on
> classic M68K MMU enabled machines. So conditionally limit their use to

And only on Q40/Q60.

> MMU enabled classic M68K systems.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>

Thanks for your patch!

> --- a/arch/m68k/kernel/time.c
> +++ b/arch/m68k/kernel/time.c
> @@ -62,7 +62,7 @@ void timer_heartbeat(void)
>  }
>  #endif /* CONFIG_HEARTBEAT */
>
> -#ifdef CONFIG_M68KCLASSIC
> +#if defined(CONFIG_M68KCLASSIC) && defined(CONFIG_MMU)
>  #if !IS_BUILTIN(CONFIG_RTC_DRV_GENERIC)
>  void read_persistent_clock64(struct timespec64 *ts)
>  {

read_persistent_clock64() uses mach_hwclk(), which is provided by
both setup_mm.c and setup_no.c, so it's always available?
Albeit not populated by coldfire or nommu platform code, so I see
the point in depending on MMU (no nommu Amiga support yet ;-).

Perhaps rtc_ioctl() should depend on CONFIG_Q40?

> @@ -140,7 +140,7 @@ static int __init rtc_init(void)
>
>  module_init(rtc_init);
>  #endif /* CONFIG_RTC_DRV_GENERIC */
> -#endif /* CONFIG M68KCLASSIC */
> +#endif /* CONFIG M68KCLASSIC && CONFIG_MMU */
>
>  void __init time_init(void)
>  {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Arnd Bergmann]

On Fri, May 13, 2022 at 9:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > -#ifdef CONFIG_M68KCLASSIC
> > +#if defined(CONFIG_M68KCLASSIC) && defined(CONFIG_MMU)
> >  #if !IS_BUILTIN(CONFIG_RTC_DRV_GENERIC)
> >  void read_persistent_clock64(struct timespec64 *ts)
> >  {
>
> read_persistent_clock64() uses mach_hwclk(), which is provided by
> both setup_mm.c and setup_no.c, so it's always available?
> Albeit not populated by coldfire or nommu platform code, so I see
> the point in depending on MMU (no nommu Amiga support yet ;-).
>
> Perhaps rtc_ioctl() should depend on CONFIG_Q40?

I think the best cleanup would be to turn the q40 rtc support into a
standalone driver using rtc_register_device(), and completely
detaching it from the rtc_generic support in arch/m68k/kernel/time.c.

        Arnd

Re: [PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Greg Ungerer]

Hi Geert,

On 13/5/22 17:12, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Fri, May 13, 2022 at 8:50 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
>> Configuring for a nommu classic m68k target and enabling the generic rtc
>> driver (CONFIG_RTC_DRV_GENERIC) will result in the following compile
>> error:
>>
>>     m68k-linux-ld: arch/m68k/kernel/time.o: in function `rtc_ioctl':
>>     time.c:(.text+0x82): undefined reference to `mach_get_rtc_pll'
>>     m68k-linux-ld: time.c:(.text+0xbc): undefined reference to `mach_set_rtc_pll'
>>     m68k-linux-ld: time.c:(.text+0xf4): undefined reference to `mach_set_rtc_pll'
>>
>> "mach_set_rtc_pll" and "mach_get_rtc_pll" are only defined in the common
>> MMU setup code, and are really only implemented in any meaningful way on
>> classic M68K MMU enabled machines. So conditionally limit their use to
> 
> And only on Q40/Q60.
> 
>> MMU enabled classic M68K systems.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
> 
> Thanks for your patch!
> 
>> --- a/arch/m68k/kernel/time.c
>> +++ b/arch/m68k/kernel/time.c
>> @@ -62,7 +62,7 @@ void timer_heartbeat(void)
>>   }
>>   #endif /* CONFIG_HEARTBEAT */
>>
>> -#ifdef CONFIG_M68KCLASSIC
>> +#if defined(CONFIG_M68KCLASSIC) && defined(CONFIG_MMU)
>>   #if !IS_BUILTIN(CONFIG_RTC_DRV_GENERIC)
>>   void read_persistent_clock64(struct timespec64 *ts)
>>   {
> 
> read_persistent_clock64() uses mach_hwclk(), which is provided by
> both setup_mm.c and setup_no.c, so it's always available?> Albeit not populated by coldfire or nommu platform code, so I see
> the point in depending on MMU (no nommu Amiga support yet ;-).

Yes, exactly. And even worse is that on ColdFire platforms it
is never set, so will be the startup value of NULL. It is called
without checking for NULL in both of rtc_generic_get_time() and
rtc_generic_set_time().

I figured if nommu mode is ever supported on any of the classic
MMU m68k then this could be revisited.


> Perhaps rtc_ioctl() should depend on CONFIG_Q40?

That is probably reasonable too. Using CONFIG_MMU was a good fit
for the way mach_set_rtc_pll and mach_get_rtc_pll are currently
defined - as in setup_mm.c which is CONFIG_MMU only, which is why
I went that way.

But if you prefer I can re-factor further based on CONFIG_Q40?

Regards
Greg


>> @@ -140,7 +140,7 @@ static int __init rtc_init(void)
>>
>>   module_init(rtc_init);
>>   #endif /* CONFIG_RTC_DRV_GENERIC */
>> -#endif /* CONFIG M68KCLASSIC */
>> +#endif /* CONFIG M68KCLASSIC && CONFIG_MMU */
>>
>>   void __init time_init(void)
>>   {
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

Re: [PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Arnd Bergmann]

On Fri, May 13, 2022 at 2:25 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
> On 13/5/22 17:12, Geert Uytterhoeven wrote:
> > read_persistent_clock64() uses mach_hwclk(), which is provided by
> > both setup_mm.c and setup_no.c, so it's always available?> Albeit not populated by coldfire or nommu platform code, so I see
> > the point in depending on MMU (no nommu Amiga support yet ;-).
>
> Yes, exactly. And even worse is that on ColdFire platforms it
> is never set, so will be the startup value of NULL. It is called
> without checking for NULL in both of rtc_generic_get_time() and
> rtc_generic_set_time().

I think that's ok because rtc_generic_{get,set}_time is only called
from the rtc_generic driver, but that is not registered when mach_hwclk()
is NULL.

With your patch to add the CONFIG_MMU check, you can actually
remove the mach_hwclk() symbol from setup_no.c, and move the
remaining RTC related symbols from setup_mm.c into the #ifdef.

       Arnd

Re: [PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Greg Ungerer]

Hi Arnd,

On 13/5/22 22:59, Arnd Bergmann wrote:
> On Fri, May 13, 2022 at 2:25 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
>> On 13/5/22 17:12, Geert Uytterhoeven wrote:
>>> read_persistent_clock64() uses mach_hwclk(), which is provided by
>>> both setup_mm.c and setup_no.c, so it's always available?> Albeit not populated by coldfire or nommu platform code, so I see
>>> the point in depending on MMU (no nommu Amiga support yet ;-).
>>
>> Yes, exactly. And even worse is that on ColdFire platforms it
>> is never set, so will be the startup value of NULL. It is called
>> without checking for NULL in both of rtc_generic_get_time() and
>> rtc_generic_set_time().
> 
> I think that's ok because rtc_generic_{get,set}_time is only called
> from the rtc_generic driver, but that is not registered when mach_hwclk()
> is NULL.
> 
> With your patch to add the CONFIG_MMU check, you can actually
> remove the mach_hwclk() symbol from setup_no.c, and move the
> remaining RTC related symbols from setup_mm.c into the #ifdef.

Yes, I think that would be a good idea.
Tidies thins up a little.

Regards
Greg

Re: [PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Geert Uytterhoeven]

On Mon, May 16, 2022 at 8:08 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
> On 13/5/22 22:59, Arnd Bergmann wrote:
> > On Fri, May 13, 2022 at 2:25 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
> >> On 13/5/22 17:12, Geert Uytterhoeven wrote:
> >>> read_persistent_clock64() uses mach_hwclk(), which is provided by
> >>> both setup_mm.c and setup_no.c, so it's always available?> Albeit not populated by coldfire or nommu platform code, so I see
> >>> the point in depending on MMU (no nommu Amiga support yet ;-).
> >>
> >> Yes, exactly. And even worse is that on ColdFire platforms it
> >> is never set, so will be the startup value of NULL. It is called
> >> without checking for NULL in both of rtc_generic_get_time() and
> >> rtc_generic_set_time().
> >
> > I think that's ok because rtc_generic_{get,set}_time is only called
> > from the rtc_generic driver, but that is not registered when mach_hwclk()
> > is NULL.
> >
> > With your patch to add the CONFIG_MMU check, you can actually
> > remove the mach_hwclk() symbol from setup_no.c, and move the
> > remaining RTC related symbols from setup_mm.c into the #ifdef.
>
> Yes, I think that would be a good idea.
> Tidies thins up a little.

Let's fix the build error first.

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] m68knommu: fix undefined reference to `mach_get_rtc_pll'

[Greg Ungerer]

Hi Geert,

On 16/5/22 17:15, Geert Uytterhoeven wrote:
> On Mon, May 16, 2022 at 8:08 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
>> On 13/5/22 22:59, Arnd Bergmann wrote:
>>> On Fri, May 13, 2022 at 2:25 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
>>>> On 13/5/22 17:12, Geert Uytterhoeven wrote:
>>>>> read_persistent_clock64() uses mach_hwclk(), which is provided by
>>>>> both setup_mm.c and setup_no.c, so it's always available?> Albeit not populated by coldfire or nommu platform code, so I see
>>>>> the point in depending on MMU (no nommu Amiga support yet ;-).
>>>>
>>>> Yes, exactly. And even worse is that on ColdFire platforms it
>>>> is never set, so will be the startup value of NULL. It is called
>>>> without checking for NULL in both of rtc_generic_get_time() and
>>>> rtc_generic_set_time().
>>>
>>> I think that's ok because rtc_generic_{get,set}_time is only called
>>> from the rtc_generic driver, but that is not registered when mach_hwclk()
>>> is NULL.
>>>
>>> With your patch to add the CONFIG_MMU check, you can actually
>>> remove the mach_hwclk() symbol from setup_no.c, and move the
>>> remaining RTC related symbols from setup_mm.c into the #ifdef.
>>
>> Yes, I think that would be a good idea.
>> Tidies thins up a little.
> 
> Let's fix the build error first.
> 
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

I am re-thinking this fix at the moment. I think I missed the fact that
the 68328 has underlying mach_hwclk support, and so can use be used by
rtc_generic_{get,set}_time. So in other words classic m68k nommu config
should be able to use this code too. CONFIG_MMU blocking it is probably
not right here.

New patch coming.

Regards
Greg


> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds