[PATCH] serial: pmac_zilog: don't init if zilog is not available

[PATCH] serial: pmac_zilog: don't init if zilog is not available

[Laurent Vivier]

We can avoid to probe for the Zilog device (and generate ugly kernel warning)
if kernel is built for Mac but not on a Mac.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index 063484b22523..d1d2e55983c3 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
 static int __init init_pmz(void)
 {
 	int rc, i;
+
+#ifdef CONFIG_MAC
+	if (!MACH_IS_MAC)
+		return -ENODEV;
+#endif
+
 	printk(KERN_INFO "%s\n", version);
 
 	/* 
@@ -2034,6 +2040,11 @@ static int __init pmz_console_setup(struct console *co, char *options)
 
 static int __init pmz_console_init(void)
 {
+#ifdef CONFIG_MAC
+	if (!MACH_IS_MAC)
+		return -ENODEV;
+#endif
+
 	/* Probe ports */
 	pmz_probe();
 
-- 
2.26.2

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Greg KH]

On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
> if kernel is built for Mac but not on a Mac.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> index 063484b22523..d1d2e55983c3 100644
> --- a/drivers/tty/serial/pmac_zilog.c
> +++ b/drivers/tty/serial/pmac_zilog.c
> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
>  static int __init init_pmz(void)
>  {
>  	int rc, i;
> +
> +#ifdef CONFIG_MAC
> +	if (!MACH_IS_MAC)
> +		return -ENODEV;
> +#endif

Why is the #ifdef needed?

We don't like putting #ifdef in .c files for good reasons.  Can you make
the api check for this work with and without that #ifdef needed?

thanks,

greg k-h

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Laurent Vivier]

Le 20/10/2020 à 18:28, Greg KH a écrit :
> On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
>> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
>> if kernel is built for Mac but not on a Mac.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
>> index 063484b22523..d1d2e55983c3 100644
>> --- a/drivers/tty/serial/pmac_zilog.c
>> +++ b/drivers/tty/serial/pmac_zilog.c
>> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
>>  static int __init init_pmz(void)
>>  {
>>  	int rc, i;
>> +
>> +#ifdef CONFIG_MAC
>> +	if (!MACH_IS_MAC)
>> +		return -ENODEV;
>> +#endif
> 
> Why is the #ifdef needed?
> 
> We don't like putting #ifdef in .c files for good reasons.  Can you make
> the api check for this work with and without that #ifdef needed?

The #ifdef is needed because this file can be compiled for PowerMac and
m68k Mac. For PowerMac, the MACH_IS_MAC is not defined, so we need the
#ifdef.

We need the MAC_IS_MAC because the same kernel can be used with several
m68k machines, so the init_pmz can be called on a m68k machine without
the zilog device (it's a multi-targets kernel).

You can check it's the good way to do by looking inside:

    drivers/video/fbdev/valkyriefb.c +317
    drivers/macintosh/adb.c +316

That are two files used by both, mac and pmac.

Thanks,
Laurent

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Greg KH]

On Tue, Oct 20, 2020 at 06:37:41PM +0200, Laurent Vivier wrote:
> Le 20/10/2020 à 18:28, Greg KH a écrit :
> > On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
> >> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
> >> if kernel is built for Mac but not on a Mac.
> >>
> >> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >> ---
> >>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> >> index 063484b22523..d1d2e55983c3 100644
> >> --- a/drivers/tty/serial/pmac_zilog.c
> >> +++ b/drivers/tty/serial/pmac_zilog.c
> >> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
> >>  static int __init init_pmz(void)
> >>  {
> >>  	int rc, i;
> >> +
> >> +#ifdef CONFIG_MAC
> >> +	if (!MACH_IS_MAC)
> >> +		return -ENODEV;
> >> +#endif
> > 
> > Why is the #ifdef needed?
> > 
> > We don't like putting #ifdef in .c files for good reasons.  Can you make
> > the api check for this work with and without that #ifdef needed?
> 
> The #ifdef is needed because this file can be compiled for PowerMac and
> m68k Mac. For PowerMac, the MACH_IS_MAC is not defined, so we need the
> #ifdef.
> 
> We need the MAC_IS_MAC because the same kernel can be used with several
> m68k machines, so the init_pmz can be called on a m68k machine without
> the zilog device (it's a multi-targets kernel).
> 
> You can check it's the good way to do by looking inside:
> 
>     drivers/video/fbdev/valkyriefb.c +317
>     drivers/macintosh/adb.c +316
> 
> That are two files used by both, mac and pmac.

Why not fix it to work properly like other arch checks are done?

Put it in a .h file and do the #ifdef there.  Why is this "special"?

thanks,

greg k-h

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Laurent Vivier]

Le 20/10/2020 à 19:37, Greg KH a écrit :
> On Tue, Oct 20, 2020 at 06:37:41PM +0200, Laurent Vivier wrote:
>> Le 20/10/2020 à 18:28, Greg KH a écrit :
>>> On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
>>>> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
>>>> if kernel is built for Mac but not on a Mac.
>>>>
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
>>>> index 063484b22523..d1d2e55983c3 100644
>>>> --- a/drivers/tty/serial/pmac_zilog.c
>>>> +++ b/drivers/tty/serial/pmac_zilog.c
>>>> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
>>>>  static int __init init_pmz(void)
>>>>  {
>>>>  	int rc, i;
>>>> +
>>>> +#ifdef CONFIG_MAC
>>>> +	if (!MACH_IS_MAC)
>>>> +		return -ENODEV;
>>>> +#endif
>>>
>>> Why is the #ifdef needed?
>>>
>>> We don't like putting #ifdef in .c files for good reasons.  Can you make
>>> the api check for this work with and without that #ifdef needed?
>>
>> The #ifdef is needed because this file can be compiled for PowerMac and
>> m68k Mac. For PowerMac, the MACH_IS_MAC is not defined, so we need the
>> #ifdef.
>>
>> We need the MAC_IS_MAC because the same kernel can be used with several
>> m68k machines, so the init_pmz can be called on a m68k machine without
>> the zilog device (it's a multi-targets kernel).
>>
>> You can check it's the good way to do by looking inside:
>>
>>     drivers/video/fbdev/valkyriefb.c +317
>>     drivers/macintosh/adb.c +316
>>
>> That are two files used by both, mac and pmac.
> 
> Why not fix it to work properly like other arch checks are done
I would be happy to do the same.

> Put it in a .h file and do the #ifdef there.  Why is this "special"?

I don't know.

Do you mean something like:

drivers/tty/serial/pmac_zilog.h
...
#ifndef MACH_IS_MAC
#define MACH_IS_MAC (0)
#endif
...

drivers/tty/serial/pmac_zilog.c
...
static int __init pmz_console_init(void)
{
        if (!MACH_IS_MAC)
                return -ENODEV;
...

Thanks,
Laurent

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Greg KH]

On Tue, Oct 20, 2020 at 08:19:26PM +0200, Laurent Vivier wrote:
> Le 20/10/2020 à 19:37, Greg KH a écrit :
> > On Tue, Oct 20, 2020 at 06:37:41PM +0200, Laurent Vivier wrote:
> >> Le 20/10/2020 à 18:28, Greg KH a écrit :
> >>> On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
> >>>> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
> >>>> if kernel is built for Mac but not on a Mac.
> >>>>
> >>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >>>> ---
> >>>>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
> >>>>  1 file changed, 11 insertions(+)
> >>>>
> >>>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> >>>> index 063484b22523..d1d2e55983c3 100644
> >>>> --- a/drivers/tty/serial/pmac_zilog.c
> >>>> +++ b/drivers/tty/serial/pmac_zilog.c
> >>>> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
> >>>>  static int __init init_pmz(void)
> >>>>  {
> >>>>  	int rc, i;
> >>>> +
> >>>> +#ifdef CONFIG_MAC
> >>>> +	if (!MACH_IS_MAC)
> >>>> +		return -ENODEV;
> >>>> +#endif
> >>>
> >>> Why is the #ifdef needed?
> >>>
> >>> We don't like putting #ifdef in .c files for good reasons.  Can you make
> >>> the api check for this work with and without that #ifdef needed?
> >>
> >> The #ifdef is needed because this file can be compiled for PowerMac and
> >> m68k Mac. For PowerMac, the MACH_IS_MAC is not defined, so we need the
> >> #ifdef.
> >>
> >> We need the MAC_IS_MAC because the same kernel can be used with several
> >> m68k machines, so the init_pmz can be called on a m68k machine without
> >> the zilog device (it's a multi-targets kernel).
> >>
> >> You can check it's the good way to do by looking inside:
> >>
> >>     drivers/video/fbdev/valkyriefb.c +317
> >>     drivers/macintosh/adb.c +316
> >>
> >> That are two files used by both, mac and pmac.
> > 
> > Why not fix it to work properly like other arch checks are done
> I would be happy to do the same.
> 
> > Put it in a .h file and do the #ifdef there.  Why is this "special"?
> 
> I don't know.
> 
> Do you mean something like:
> 
> drivers/tty/serial/pmac_zilog.h
> ...
> #ifndef MACH_IS_MAC
> #define MACH_IS_MAC (0)
> #endif
> ...
> 
> drivers/tty/serial/pmac_zilog.c
> ...
> static int __init pmz_console_init(void)
> {
>         if (!MACH_IS_MAC)
>                 return -ENODEV;
> ...

Yup, that would be a good start, but why is the pmac_zilog.h file
responsible for this?  Shouldn't this be in some arch-specific file
somewhere?

thanks,

greg k-h

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Laurent Vivier]

Le 20/10/2020 à 20:32, Greg KH a écrit :
> On Tue, Oct 20, 2020 at 08:19:26PM +0200, Laurent Vivier wrote:
>> Le 20/10/2020 à 19:37, Greg KH a écrit :
>>> On Tue, Oct 20, 2020 at 06:37:41PM +0200, Laurent Vivier wrote:
>>>> Le 20/10/2020 à 18:28, Greg KH a écrit :
>>>>> On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
>>>>>> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
>>>>>> if kernel is built for Mac but not on a Mac.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>>> ---
>>>>>>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
>>>>>>  1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
>>>>>> index 063484b22523..d1d2e55983c3 100644
>>>>>> --- a/drivers/tty/serial/pmac_zilog.c
>>>>>> +++ b/drivers/tty/serial/pmac_zilog.c
>>>>>> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
>>>>>>  static int __init init_pmz(void)
>>>>>>  {
>>>>>>  	int rc, i;
>>>>>> +
>>>>>> +#ifdef CONFIG_MAC
>>>>>> +	if (!MACH_IS_MAC)
>>>>>> +		return -ENODEV;
>>>>>> +#endif
>>>>>
>>>>> Why is the #ifdef needed?
>>>>>
>>>>> We don't like putting #ifdef in .c files for good reasons.  Can you make
>>>>> the api check for this work with and without that #ifdef needed?
>>>>
>>>> The #ifdef is needed because this file can be compiled for PowerMac and
>>>> m68k Mac. For PowerMac, the MACH_IS_MAC is not defined, so we need the
>>>> #ifdef.
>>>>
>>>> We need the MAC_IS_MAC because the same kernel can be used with several
>>>> m68k machines, so the init_pmz can be called on a m68k machine without
>>>> the zilog device (it's a multi-targets kernel).
>>>>
>>>> You can check it's the good way to do by looking inside:
>>>>
>>>>     drivers/video/fbdev/valkyriefb.c +317
>>>>     drivers/macintosh/adb.c +316
>>>>
>>>> That are two files used by both, mac and pmac.
>>>
>>> Why not fix it to work properly like other arch checks are done
>> I would be happy to do the same.
>>
>>> Put it in a .h file and do the #ifdef there.  Why is this "special"?
>>
>> I don't know.
>>
>> Do you mean something like:
>>
>> drivers/tty/serial/pmac_zilog.h
>> ...
>> #ifndef MACH_IS_MAC
>> #define MACH_IS_MAC (0)
>> #endif
>> ...
>>
>> drivers/tty/serial/pmac_zilog.c
>> ...
>> static int __init pmz_console_init(void)
>> {
>>         if (!MACH_IS_MAC)
>>                 return -ENODEV;
>> ...
> 
> Yup, that would be a good start, but why is the pmac_zilog.h file
> responsible for this?  Shouldn't this be in some arch-specific file
> somewhere?

For m68k, MACH_IS_MAC is defined in arch/m68k/include/asm/setup.h

If I want to define it for any other archs I don't know in which file we
can put it.

But as m68k mac is only sharing drivers with pmac perhaps we can put
this in arch/powerpc/include/asm/setup.h?

Thanks,
Laurent

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Brad Boyer]

On Tue, Oct 20, 2020 at 08:42:53PM +0200, Laurent Vivier wrote:
> Le 20/10/2020 ?? 20:32, Greg KH a ??crit??:
> > On Tue, Oct 20, 2020 at 08:19:26PM +0200, Laurent Vivier wrote:
> >> Le 20/10/2020 ?? 19:37, Greg KH a ??crit??:
> >>> Why not fix it to work properly like other arch checks are done
> >> I would be happy to do the same.
> >>
> >>> Put it in a .h file and do the #ifdef there.  Why is this "special"?
> >>
> >> I don't know.
> >>
> > 
> > Yup, that would be a good start, but why is the pmac_zilog.h file
> > responsible for this?  Shouldn't this be in some arch-specific file
> > somewhere?
> 
> For m68k, MACH_IS_MAC is defined in arch/m68k/include/asm/setup.h
> 
> If I want to define it for any other archs I don't know in which file we
> can put it.
> 
> But as m68k mac is only sharing drivers with pmac perhaps we can put
> this in arch/powerpc/include/asm/setup.h?

Wouldn't it be better to rearrange this code to only run if the devices
are present? This is a macio driver on pmac and a platform driver on mac,
so shouldn't it be possible to only run this code when the appropriate
entries are present in the right data structures?

I didn't look at a lot of the other serial drivers, but some other mac
drivers have recently been updated to no longer have MACH_IS_MAC checks
due to being converted to platform drivers.

	Brad Boyer
	brad@allandria.com

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Finn Thain]

On Tue, 20 Oct 2020, Brad Boyer wrote:

> 
> Wouldn't it be better to rearrange this code to only run if the devices 
> are present? This is a macio driver on pmac and a platform driver on 
> mac, so shouldn't it be possible to only run this code when the 
> appropriate entries are present in the right data structures?
> 
> I didn't look at a lot of the other serial drivers, but some other mac 
> drivers have recently been updated to no longer have MACH_IS_MAC checks 
> due to being converted to platform drivers.
> 

Actually, it's not simply a platform driver or macio driver. I think the 
console is supposed to be registered before the normal bus matching takes 
place. Hence this comment in pmac_zilog.c,

        /* 
         * First, we need to do a direct OF-based probe pass. We
         * do that because we want serial console up before the
         * macio stuffs calls us back, and since that makes it
         * easier to pass the proper number of channels to
         * uart_register_driver()
         */

Laurent, can we avoid the irq == 0 warning splat like this?

diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index 96e7aa479961..7db600cd8cc7 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -1701,8 +1701,10 @@ static int __init pmz_init_port(struct uart_pmac_port *uap)
 	int irq;
 
 	r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
+	if (!r_ports)
+		return -ENODEV;
 	irq = platform_get_irq(uap->pdev, 0);
-	if (!r_ports || irq <= 0)
+	if (irq <= 0)
 		return -ENODEV;
 
 	uap->port.mapbase  = r_ports->start;

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Laurent Vivier]

Le 21/10/2020 à 01:43, Finn Thain a écrit :
> On Tue, 20 Oct 2020, Brad Boyer wrote:
> 
>>
>> Wouldn't it be better to rearrange this code to only run if the devices 
>> are present? This is a macio driver on pmac and a platform driver on 
>> mac, so shouldn't it be possible to only run this code when the 
>> appropriate entries are present in the right data structures?
>>
>> I didn't look at a lot of the other serial drivers, but some other mac 
>> drivers have recently been updated to no longer have MACH_IS_MAC checks 
>> due to being converted to platform drivers.
>>
> 
> Actually, it's not simply a platform driver or macio driver. I think the 
> console is supposed to be registered before the normal bus matching takes 
> place. Hence this comment in pmac_zilog.c,
> 
>         /* 
>          * First, we need to do a direct OF-based probe pass. We
>          * do that because we want serial console up before the
>          * macio stuffs calls us back, and since that makes it
>          * easier to pass the proper number of channels to
>          * uart_register_driver()
>          */
> 
> Laurent, can we avoid the irq == 0 warning splat like this?
> 
> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> index 96e7aa479961..7db600cd8cc7 100644
> --- a/drivers/tty/serial/pmac_zilog.c
> +++ b/drivers/tty/serial/pmac_zilog.c
> @@ -1701,8 +1701,10 @@ static int __init pmz_init_port(struct uart_pmac_port *uap)
>  	int irq;
>  
>  	r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
> +	if (!r_ports)
> +		return -ENODEV;
>  	irq = platform_get_irq(uap->pdev, 0);
> -	if (!r_ports || irq <= 0)
> +	if (irq <= 0)
>  		return -ENODEV;
>  
>  	uap->port.mapbase  = r_ports->start;
> 

No, this doesn't fix the problem.

The message is still:

[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at drivers/base/platform.c:224
platform_get_irq_optional+0x7a/0x80
[    0.000000] 0 is an invalid IRQ number
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.9.0+ #324
[    0.000000] Stack from 004e7f24:
                       004e7f24 0046c1d3 0046c1d3 0002cb26 004985fb
000000e0 00000009 00000000
                       0002cb6a 004985fb 000000e0 002a5b86 00000009
00000000 004e7f70 00553cc4
                       00000000 00000000 00000000 004985df 004e7f90
004e7ff8 002a5b86 004985fb
                       000000e0 00000009 004985df 004eb290 002a5bd2
004eb290 00000000 00553cc4
                       0057bb66 00553cc4 00573d6e 004eb290 00000000
00573d38 0021c42c 00573e06
                       00553cc4 0046df15 00583a7c 00573e58 00564b74
0005299a 0055ce34 00000000
[    0.000000] Call Trace: [<0002cb26>] __warn+0xb2/0xb4
[    0.000000]  [<0002cb6a>] warn_slowpath_fmt+0x42/0x64
[    0.000000]  [<002a5b86>] platform_get_irq_optional+0x7a/0x80
[    0.000000]  [<002a5b86>] platform_get_irq_optional+0x7a/0x80
[    0.000000]  [<002a5bd2>] platform_get_irq+0x16/0x42
[    0.000000]  [<00573d6e>] pmz_init_port+0x36/0x9e
[    0.000000]  [<00573d38>] pmz_init_port+0x0/0x9e
[    0.000000]  [<0021c42c>] strlen+0x0/0x14
[    0.000000]  [<00573e06>] pmz_probe+0x30/0x7e
[    0.000000]  [<00573e58>] pmz_console_init+0x4/0x22
[    0.000000]  [<00564b74>] console_init+0x1e/0x20
[    0.000000]  [<0005299a>] printk+0x0/0x18
[    0.000000]  [<0055ce34>] start_kernel+0x332/0x4c4
[    0.000000]  [<0055b8c6>] _sinittext+0x8c6/0x1268
[    0.000000] ---[ end trace 32d780b8cd50b829 ]---

Thanks,
Laurent

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Michael Ellerman]

Laurent Vivier <laurent@vivier.eu> writes:
> Le 20/10/2020 à 20:32, Greg KH a écrit :
>> On Tue, Oct 20, 2020 at 08:19:26PM +0200, Laurent Vivier wrote:
>>> Le 20/10/2020 à 19:37, Greg KH a écrit :
>>>> On Tue, Oct 20, 2020 at 06:37:41PM +0200, Laurent Vivier wrote:
>>>>> Le 20/10/2020 à 18:28, Greg KH a écrit :
>>>>>> On Tue, Oct 20, 2020 at 06:23:03PM +0200, Laurent Vivier wrote:
>>>>>>> We can avoid to probe for the Zilog device (and generate ugly kernel warning)
>>>>>>> if kernel is built for Mac but not on a Mac.
>>>>>>>
>>>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>>>> ---
>>>>>>>  drivers/tty/serial/pmac_zilog.c | 11 +++++++++++
>>>>>>>  1 file changed, 11 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
>>>>>>> index 063484b22523..d1d2e55983c3 100644
>>>>>>> --- a/drivers/tty/serial/pmac_zilog.c
>>>>>>> +++ b/drivers/tty/serial/pmac_zilog.c
>>>>>>> @@ -1867,6 +1867,12 @@ static struct platform_driver pmz_driver = {
>>>>>>>  static int __init init_pmz(void)
>>>>>>>  {
>>>>>>>  	int rc, i;
>>>>>>> +
>>>>>>> +#ifdef CONFIG_MAC
>>>>>>> +	if (!MACH_IS_MAC)
>>>>>>> +		return -ENODEV;
>>>>>>> +#endif
>>>>>>
>>>>>> Why is the #ifdef needed?
>>>>>>
>>>>>> We don't like putting #ifdef in .c files for good reasons.  Can you make
>>>>>> the api check for this work with and without that #ifdef needed?
>>>>>
>>>>> The #ifdef is needed because this file can be compiled for PowerMac and
>>>>> m68k Mac. For PowerMac, the MACH_IS_MAC is not defined, so we need the
>>>>> #ifdef.
>>>>>
>>>>> We need the MAC_IS_MAC because the same kernel can be used with several
>>>>> m68k machines, so the init_pmz can be called on a m68k machine without
>>>>> the zilog device (it's a multi-targets kernel).
>>>>>
>>>>> You can check it's the good way to do by looking inside:
>>>>>
>>>>>     drivers/video/fbdev/valkyriefb.c +317
>>>>>     drivers/macintosh/adb.c +316
>>>>>
>>>>> That are two files used by both, mac and pmac.
>>>>
>>>> Why not fix it to work properly like other arch checks are done
>>> I would be happy to do the same.
>>>
>>>> Put it in a .h file and do the #ifdef there.  Why is this "special"?
>>>
>>> I don't know.
>>>
>>> Do you mean something like:
>>>
>>> drivers/tty/serial/pmac_zilog.h
>>> ...
>>> #ifndef MACH_IS_MAC
>>> #define MACH_IS_MAC (0)
>>> #endif
>>> ...
>>>
>>> drivers/tty/serial/pmac_zilog.c
>>> ...
>>> static int __init pmz_console_init(void)
>>> {
>>>         if (!MACH_IS_MAC)
>>>                 return -ENODEV;
>>> ...
>> 
>> Yup, that would be a good start, but why is the pmac_zilog.h file
>> responsible for this?  Shouldn't this be in some arch-specific file
>> somewhere?
>
> For m68k, MACH_IS_MAC is defined in arch/m68k/include/asm/setup.h
>
> If I want to define it for any other archs I don't know in which file we
> can put it.
>
> But as m68k mac is only sharing drivers with pmac perhaps we can put
> this in arch/powerpc/include/asm/setup.h?

It doesn't really belong in there.

I'd accept a patch to create arch/powerpc/include/asm/macintosh.h, with
MACH_IS_MAC defined in there.

cheers

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]

On Wed, 21 Oct 2020, Laurent Vivier wrote:

> Le 21/10/2020 à 01:43, Finn Thain a écrit :
> 
> > Laurent, can we avoid the irq == 0 warning splat like this?
> > 
> > diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> > index 96e7aa479961..7db600cd8cc7 100644
> > --- a/drivers/tty/serial/pmac_zilog.c
> > +++ b/drivers/tty/serial/pmac_zilog.c
> > @@ -1701,8 +1701,10 @@ static int __init pmz_init_port(struct uart_pmac_port *uap)
> >  	int irq;
> >  
> >  	r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
> > +	if (!r_ports)
> > +		return -ENODEV;
> >  	irq = platform_get_irq(uap->pdev, 0);
> > -	if (!r_ports || irq <= 0)
> > +	if (irq <= 0)
> >  		return -ENODEV;
> >  
> >  	uap->port.mapbase  = r_ports->start;
> > 
> 
> No, this doesn't fix the problem.
> 

Then I had better stop guessing and start up Aranym...

The patch below seems to fix the problem for me. Does it work on your 
system(s)?

diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index a621fcc1a576..4e802f70333d 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -776,16 +776,12 @@ static struct resource scc_b_rsrcs[] = {
 struct platform_device scc_a_pdev = {
 	.name           = "scc",
 	.id             = 0,
-	.num_resources  = ARRAY_SIZE(scc_a_rsrcs),
-	.resource       = scc_a_rsrcs,
 };
 EXPORT_SYMBOL(scc_a_pdev);
 
 struct platform_device scc_b_pdev = {
 	.name           = "scc",
 	.id             = 1,
-	.num_resources  = ARRAY_SIZE(scc_b_rsrcs),
-	.resource       = scc_b_rsrcs,
 };
 EXPORT_SYMBOL(scc_b_pdev);
 
@@ -812,10 +808,15 @@ static void __init mac_identify(void)
 
 	/* Set up serial port resources for the console initcall. */
 
-	scc_a_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase + 2;
-	scc_a_rsrcs[0].end   = scc_a_rsrcs[0].start;
-	scc_b_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase;
-	scc_b_rsrcs[0].end   = scc_b_rsrcs[0].start;
+	scc_a_rsrcs[0].start     = (resource_size_t)mac_bi_data.sccbase + 2;
+	scc_a_rsrcs[0].end       = scc_a_rsrcs[0].start;
+	scc_a_pdev.num_resources = ARRAY_SIZE(scc_a_rsrcs);
+	scc_a_pdev.resource      = scc_a_rsrcs;
+
+	scc_b_rsrcs[0].start     = (resource_size_t)mac_bi_data.sccbase;
+	scc_b_rsrcs[0].end       = scc_b_rsrcs[0].start;
+	scc_b_pdev.num_resources = ARRAY_SIZE(scc_b_rsrcs);
+	scc_b_pdev.resource      = scc_b_rsrcs;
 
 	switch (macintosh_config->scc_type) {
 	case MAC_SCC_PSC:
diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index 96e7aa479961..95abdb305d67 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -1697,18 +1697,17 @@ extern struct platform_device scc_a_pdev, scc_b_pdev;
 
 static int __init pmz_init_port(struct uart_pmac_port *uap)
 {
-	struct resource *r_ports;
-	int irq;
+	struct resource *r_ports, *r_irq;
 
 	r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
-	irq = platform_get_irq(uap->pdev, 0);
-	if (!r_ports || irq <= 0)
+	r_irq = platform_get_resource(uap->pdev, IORESOURCE_IRQ, 0);
+	if (!r_ports || !r_irq)
 		return -ENODEV;
 
 	uap->port.mapbase  = r_ports->start;
 	uap->port.membase  = (unsigned char __iomem *) r_ports->start;
 	uap->port.iotype   = UPIO_MEM;
-	uap->port.irq      = irq;
+	uap->port.irq      = r_irq->start;
 	uap->port.uartclk  = ZS_CLOCK;
 	uap->port.fifosize = 1;
 	uap->port.ops      = &pmz_pops;

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Laurent Vivier]

Le 22/10/2020 à 05:23, Finn Thain a écrit :
> On Wed, 21 Oct 2020, Laurent Vivier wrote:
> 
>> Le 21/10/2020 à 01:43, Finn Thain a écrit :
>>
>>> Laurent, can we avoid the irq == 0 warning splat like this?
>>>
>>> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
>>> index 96e7aa479961..7db600cd8cc7 100644
>>> --- a/drivers/tty/serial/pmac_zilog.c
>>> +++ b/drivers/tty/serial/pmac_zilog.c
>>> @@ -1701,8 +1701,10 @@ static int __init pmz_init_port(struct uart_pmac_port *uap)
>>>  	int irq;
>>>  
>>>  	r_ports = platform_get_resource(uap->pdev, IORESOURCE_MEM, 0);
>>> +	if (!r_ports)
>>> +		return -ENODEV;
>>>  	irq = platform_get_irq(uap->pdev, 0);
>>> -	if (!r_ports || irq <= 0)
>>> +	if (irq <= 0)
>>>  		return -ENODEV;
>>>  
>>>  	uap->port.mapbase  = r_ports->start;
>>>
>>
>> No, this doesn't fix the problem.
>>
> 
> Then I had better stop guessing and start up Aranym...
> 
> The patch below seems to fix the problem for me. Does it work on your 
> system(s)?

It works like a charm.

Thank you,
Laurent

Re: [PATCH] serial: pmac_zilog: don't init if zilog is not available

[Geert Uytterhoeven]

Hi Finn,

On Thu, Oct 22, 2020 at 5:23 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> The patch below seems to fix the problem for me. Does it work on your
> system(s)?

Thanks for your patch!

> --- a/arch/m68k/mac/config.c
> +++ b/arch/m68k/mac/config.c
> @@ -776,16 +776,12 @@ static struct resource scc_b_rsrcs[] = {
>  struct platform_device scc_a_pdev = {
>         .name           = "scc",
>         .id             = 0,
> -       .num_resources  = ARRAY_SIZE(scc_a_rsrcs),
> -       .resource       = scc_a_rsrcs,
>  };
>  EXPORT_SYMBOL(scc_a_pdev);
>
>  struct platform_device scc_b_pdev = {
>         .name           = "scc",
>         .id             = 1,
> -       .num_resources  = ARRAY_SIZE(scc_b_rsrcs),
> -       .resource       = scc_b_rsrcs,
>  };
>  EXPORT_SYMBOL(scc_b_pdev);
>
> @@ -812,10 +808,15 @@ static void __init mac_identify(void)
>
>         /* Set up serial port resources for the console initcall. */
>
> -       scc_a_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase + 2;
> -       scc_a_rsrcs[0].end   = scc_a_rsrcs[0].start;
> -       scc_b_rsrcs[0].start = (resource_size_t) mac_bi_data.sccbase;
> -       scc_b_rsrcs[0].end   = scc_b_rsrcs[0].start;
> +       scc_a_rsrcs[0].start     = (resource_size_t)mac_bi_data.sccbase + 2;
> +       scc_a_rsrcs[0].end       = scc_a_rsrcs[0].start;
> +       scc_a_pdev.num_resources = ARRAY_SIZE(scc_a_rsrcs);
> +       scc_a_pdev.resource      = scc_a_rsrcs;
> +
> +       scc_b_rsrcs[0].start     = (resource_size_t)mac_bi_data.sccbase;
> +       scc_b_rsrcs[0].end       = scc_b_rsrcs[0].start;
> +       scc_b_pdev.num_resources = ARRAY_SIZE(scc_b_rsrcs);
> +       scc_b_pdev.resource      = scc_b_rsrcs;

I can't say I'm a fan of this...

>
>         switch (macintosh_config->scc_type) {
>         case MAC_SCC_PSC:
> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> index 96e7aa479961..95abdb305d67 100644
> --- a/drivers/tty/serial/pmac_zilog.c
> +++ b/drivers/tty/serial/pmac_zilog.c
> @@ -1697,18 +1697,17 @@ extern struct platform_device scc_a_pdev, scc_b_pdev;

The real issue is this "extern struct platform_device scc_a_pdev, scc_b_pdev",
circumventing the driver framework.

Can we get rid of that?

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] serial: pmac_zilog: don't init if zilog is not available

[Finn Thain]

On Thu, 22 Oct 2020, Geert Uytterhoeven wrote:

> 
> Thanks for your patch...
> 

You're welcome.

> I can't say I'm a fan of this...
> 

Sorry.

> 
> The real issue is this "extern struct platform_device scc_a_pdev, 
> scc_b_pdev", circumventing the driver framework.
> 
> Can we get rid of that?
> 

Is there a better alternative?

pmz_probe() is called by console_initcall(pmz_console_init) when 
CONFIG_SERIAL_PMACZILOG_CONSOLE=y because this has to happen earlier than 
the normal platform bus probing which takes place later as a typical 
module_initcall.