Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1295373 > unrolled thread

Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core

Started byRobert Jarzmik <robert.jarzmik@free.fr>
First post2015-12-19 13:50 +0100
Last post2015-12-22 20:30 +0100
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Robert Jarzmik <robert.jarzmik@free.fr> - 2015-12-19 13:50 +0100
    Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Robert Jarzmik <robert.jarzmik@free.fr> - 2015-12-19 14:30 +0100
      Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Sergei Ianovich <ynvich@gmail.com> - 2015-12-19 19:50 +0100
        Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Robert Jarzmik <robert.jarzmik@free.fr> - 2015-12-19 20:40 +0100
          Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Sergei Ianovich <ynvich@gmail.com> - 2015-12-19 21:20 +0100
            Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Robert Jarzmik <robert.jarzmik@free.fr> - 2015-12-20 00:20 +0100
              Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Sergei Ianovich <ynvich@gmail.com> - 2015-12-20 12:30 +0100
                Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core Robert Jarzmik <robert.jarzmik@free.fr> - 2015-12-22 20:30 +0100

#1295373 — Re: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2015-12-19 13:50 +0100
SubjectRe: [PATCH v4 1/2] serial: rewrite pxa2xx-uart to use 8250_core
Message-ID<qHp7r-3fu-3@gated-at.bofh.it>
Sergei Ianovich <ynvich@gmail.com> writes:

> pxa2xx-uart was a separate uart platform driver. It was declaring
> the same device names and numbers as 8250 driver. As a result,
> it was impossible to use 8250 driver on PXA SoCs.
>
> Upon closer examination pxa2xx-uart turned out to be a clone of
> 8250_core driver.
>
> Workaround for Erratum #19 according to Marvel(R) PXA270M Processor
> Specification Update (April 19, 2010) is dropped. 8250_core reads
> from FIFO immediately after checking DR bit in LSR.
>
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Reviewed-by: James Cameron <quozl@laptop.org>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>

I was wondering if this should go through pxa tree, or through serial tree ?

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1295377

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2015-12-19 14:30 +0100
Message-ID<qHpK9-3J2-11@gated-at.bofh.it>
In reply to#1295373
Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Sergei Ianovich <ynvich@gmail.com> writes:
>
>> pxa2xx-uart was a separate uart platform driver. It was declaring
>> the same device names and numbers as 8250 driver. As a result,
>> it was impossible to use 8250 driver on PXA SoCs.
>>
>> Upon closer examination pxa2xx-uart turned out to be a clone of
>> 8250_core driver.
>>
>> Workaround for Erratum #19 according to Marvel(R) PXA270M Processor
>> Specification Update (April 19, 2010) is dropped. 8250_core reads
>> from FIFO immediately after checking DR bit in LSR.
>>
>> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Reviewed-by: James Cameron <quozl@laptop.org>
>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>

Shrunk the list, the following is rather mach-pxa specific.

Actually there is a small glitch ...
Have a look at arch/arm/mach-pxa/viper.c, line 490:
	#ifndef CONFIG_SERIAL_PXA
	...

But that can be handled in an subsequent patch to keep your acks and reviews.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295432

FromSergei Ianovich <ynvich@gmail.com>
Date2015-12-19 19:50 +0100
Message-ID<qHuJP-6Om-3@gated-at.bofh.it>
In reply to#1295377
On Sat, 2015-12-19 at 14:26 +0100, Robert Jarzmik wrote:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
> 
> > Sergei Ianovich <ynvich@gmail.com> writes:
> > 
> > > pxa2xx-uart was a separate uart platform driver. It was declaring
> > > the same device names and numbers as 8250 driver. As a result,
> > > it was impossible to use 8250 driver on PXA SoCs.
> > > 
> > > Upon closer examination pxa2xx-uart turned out to be a clone of
> > > 8250_core driver.
> > > 
> > > Workaround for Erratum #19 according to Marvel(R) PXA270M
> > > Processor
> > > Specification Update (April 19, 2010) is dropped. 8250_core reads
> > > from FIFO immediately after checking DR bit in LSR.
> > > 
> > > Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Reviewed-by: James Cameron <quozl@laptop.org>
> > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> Shrunk the list, the following is rather mach-pxa specific.
> 
> Actually there is a small glitch ...
> Have a look at arch/arm/mach-pxa/viper.c, line 490:
> 	#ifndef CONFIG_SERIAL_PXA
> 	...

Thanks for spotting this. This is caused by a change in the latest
version of the patch (SERIAL_8250_PXA instead of SERIAL_PXA). This
change could be reverted.

> But that can be handled in an subsequent patch to keep your acks and
> reviews.
> 

I will respin the patch. Please comment on the acks and reviews. They
were made at an earlier version of the patch. That version no longer
applies. Can the updated version carry on the flags?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295436

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2015-12-19 20:40 +0100
Message-ID<qHvwd-7mb-5@gated-at.bofh.it>
In reply to#1295432
Sergei Ianovich <ynvich@gmail.com> writes:

> On Sat, 2015-12-19 at 14:26 +0100, Robert Jarzmik wrote:
>> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>> 
>> > Sergei Ianovich <ynvich@gmail.com> writes:
>> > 
>> > > pxa2xx-uart was a separate uart platform driver. It was declaring
>> > > the same device names and numbers as 8250 driver. As a result,
>> > > it was impossible to use 8250 driver on PXA SoCs.
>> > > 
>> > > Upon closer examination pxa2xx-uart turned out to be a clone of
>> > > 8250_core driver.
>> > > 
>> > > Workaround for Erratum #19 according to Marvel(R) PXA270M
>> > > Processor
>> > > Specification Update (April 19, 2010) is dropped. 8250_core reads
>> > > from FIFO immediately after checking DR bit in LSR.
>> > > 
>> > > Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
>> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> > > Reviewed-by: James Cameron <quozl@laptop.org>
>> > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
>> 
>> Shrunk the list, the following is rather mach-pxa specific.
>> 
>> Actually there is a small glitch ...
>> Have a look at arch/arm/mach-pxa/viper.c, line 490:
>> 	#ifndef CONFIG_SERIAL_PXA
>> 	...
>
> Thanks for spotting this. This is caused by a change in the latest
> version of the patch (SERIAL_8250_PXA instead of SERIAL_PXA). This
> change could be reverted.
Actually I'm against the revert.
The name change looks very good to me, please keep it.

>> But that can be handled in an subsequent patch to keep your acks and
>> reviews.
> I will respin the patch. Please comment on the acks and reviews. They
> were made at an earlier version of the patch. That version no longer
> applies. Can the updated version carry on the flags?
I don't get you. If you mean keeping CONFIG_SERIAL_8250_PXA, then yes, please
keep it.

Cheers.
-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295448

FromSergei Ianovich <ynvich@gmail.com>
Date2015-12-19 21:20 +0100
Message-ID<qHw8W-7P7-15@gated-at.bofh.it>
In reply to#1295436
On Sat, 2015-12-19 at 20:31 +0100, Robert Jarzmik wrote:
> Sergei Ianovich <ynvich@gmail.com> writes:
> Thanks for spotting this. This is caused by a change in the latest
> > version of the patch (SERIAL_8250_PXA instead of SERIAL_PXA). This
> > change could be reverted.
> Actually I'm against the revert.
> The name change looks very good to me, please keep it.

Is it worth adding an error if CONFIG_SERIAL_PXA is defined? Or is there
any other way of preventing this patch launching Linus' "flag days" as
Russel King named it?

I understand that people are afraid of taking this patch. If it starts
causing troubles at runtime, it will be difficult to diagnose. There
will be no console for most people. So it is probably good idea to fail
at boot time.

> > > But that can be handled in an subsequent patch to keep your acks
> > > and
> > > reviews.
> > I will respin the patch. Please comment on the acks and reviews.
> > They
> > were made at an earlier version of the patch. That version no longer
> > applies. Can the updated version carry on the flags?
> I don't get you. If you mean keeping CONFIG_SERIAL_8250_PXA, then yes,
> please
> keep it.

I mean should the patch be re-revied and re-acked?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295484

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2015-12-20 00:20 +0100
Message-ID<qHyX8-19a-5@gated-at.bofh.it>
In reply to#1295448
Sergei Ianovich <ynvich@gmail.com> writes:

> On Sat, 2015-12-19 at 20:31 +0100, Robert Jarzmik wrote:
>> Sergei Ianovich <ynvich@gmail.com> writes:
>> Thanks for spotting this. This is caused by a change in the latest
>> > version of the patch (SERIAL_8250_PXA instead of SERIAL_PXA). This
>> > change could be reverted.
>> Actually I'm against the revert.
>> The name change looks very good to me, please keep it.
>
> Is it worth adding an error if CONFIG_SERIAL_PXA is defined?
I don't think so.

> Or is there any other way of preventing this patch launching Linus' "flag
> days" as Russel King named it?
I must think about it, and test a bit more.
Ah and a small hint : if you include back Russell in the conversation, don't
forget the double "ll" if you wish him to answer.

> I understand that people are afraid of taking this patch. If it starts
> causing troubles at runtime, it will be difficult to diagnose. There
> will be no console for most people. So it is probably good idea to fail
> at boot time.
Who are "the people" ? If it's about something already written in a mailing
list, please point me to it so that it can help me think about it.

>> > > But that can be handled in an subsequent patch to keep your acks
>> > > and
>> > > reviews.
>> > I will respin the patch. Please comment on the acks and reviews.
>> > They
>> > were made at an earlier version of the patch. That version no longer
>> > applies. Can the updated version carry on the flags?
>> I don't get you. If you mean keeping CONFIG_SERIAL_8250_PXA, then yes,
>> please
>> keep it.
>
> I mean should the patch be re-revied and re-acked?
Well it depends on what was since then. If it's a trivial commit message fix or
a typo, I don't think it's necessary. If it's the algorithm, the code logic, or
even I think you should ask again.

For a name change in CONFIG_SERIAL_PXA into CONFIG_SERIAL_8250_PXA (change from
v3 to v4), I must admit I don't know. Maybe Kevin or Arnd might give a clue.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295568

FromSergei Ianovich <ynvich@gmail.com>
Date2015-12-20 12:30 +0100
Message-ID<qHKlB-8mk-27@gated-at.bofh.it>
In reply to#1295484
On Sun, 2015-12-20 at 00:12 +0100, Robert Jarzmik wrote:
> Sergei Ianovich <ynvich@gmail.com> writes:
> 
> > On Sat, 2015-12-19 at 20:31 +0100, Robert Jarzmik wrote:
> > > Sergei Ianovich <ynvich@gmail.com> writes:
> > > Thanks for spotting this. This is caused by a change in the latest
> > > > version of the patch (SERIAL_8250_PXA instead of SERIAL_PXA).
> > > > This
> > > > change could be reverted.
> > > Actually I'm against the revert.
> > > The name change looks very good to me, please keep it.
> > 
> > Is it worth adding an error if CONFIG_SERIAL_PXA is defined?
> I don't think so.

...

> 
> > I understand that people are afraid of taking this patch. If it
> > starts
> > causing troubles at runtime, it will be difficult to diagnose. There
> > will be no console for most people. So it is probably good idea to
> > fail
> > at boot time.
> Who are "the people" ? 

I think "the people" are at least Greg Kroah-Hartman and Russell King.

> If it's about something already written in a mailing
> list, please point me to it so that it can help me think about it.

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/2167
73.html

I can explain why I think so. Greg acked the patch, but hasn't merged it
since then. He has good reasons for this most probably. Russell's
comment pointed by the link seems to be the reason.

I think the problem raised by Russell could be addressed. My best guess
is compile time error, despite your comment above.

I have one more plan. For transition period, we can introduce a
temporary Kconfig option SERIAL_8250_PXA_OFF, and fail at build time if
neither SERIAL_8250_PXA nor SERIAL_8250_PXA_OFF is set. This way all
interested parties will be notified of this driver update.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297006

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2015-12-22 20:30 +0100
Message-ID<qIANc-81O-11@gated-at.bofh.it>
In reply to#1295568
Sergei Ianovich <ynvich@gmail.com> writes:

> On Sun, 2015-12-20 at 00:12 +0100, Robert Jarzmik wrote:
>> Sergei Ianovich <ynvich@gmail.com> writes:
>> 
>> > On Sat, 2015-12-19 at 20:31 +0100, Robert Jarzmik wrote:
>> > I understand that people are afraid of taking this patch. If it
>> > starts
>> > causing troubles at runtime, it will be difficult to diagnose. There
>> > will be no console for most people. So it is probably good idea to
>> > fail
>> > at boot time.
>> If it's about something already written in a mailing
>> list, please point me to it so that it can help me think about it.
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/2167
> 73.html
>
> I can explain why I think so. Greg acked the patch, but hasn't merged it
> since then. He has good reasons for this most probably. Russell's
> comment pointed by the link seems to be the reason.

> I think the problem raised by Russell could be addressed. My best guess
> is compile time error, despite your comment above.
I re-read carefully Russell's answer in [1].

What Russell asked is that for a period of time, the old pxa serial code and the
new will be in the kernel, so that maintainers have the option to switch over to
the old drivers/tty/serial/pxa.c if the new 8250 based version breaks their
userspace getty.

Then, once the transition is done, and that for a period (let's say 1 year) no
maintainer had complained while its defconfig was switched over to the new 8520
version, then and only then you will remove drivers/tty/serial/pxa.c.

> I have one more plan. For transition period, we can introduce a
> temporary Kconfig option SERIAL_8250_PXA_OFF, and fail at build time if
> neither SERIAL_8250_PXA nor SERIAL_8250_PXA_OFF is set. This way all
> interested parties will be notified of this driver update.
No, I'd like to stick with Russell's original plan :
 - phase 1: both SERIAL_8250_PXA and SERIAL_PXA exist in KConfig
            both are selectable
            This lasts one year or something like that
 - phase 2: remove SERIAL_PXA from KConfig and drivers/tty/serial/pxa.c

This means a different patch from the one acked by Greg, and a new serie of
acks. The diffstat will be way worse (as you won't have the -970 for pxa.c), but
in the end it will end up on that -970.

That sounds like a good transition plan to me.

Cheers.

-- 
Robert

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/216773.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web