| Message ID | 1362049268-26822-1-git-send-email-apatel@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/02/13 11:01, Anup Patel wrote: Hi Anup, > Signed-off-by: Anup Patel <apatel@apm.com> A proper patch description would be nice. > --- > arch/arm64/kernel/early_printk.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/arch/arm64/kernel/early_printk.c b/arch/arm64/kernel/early_printk.c > index 7e320a2..62953ed 100644 > --- a/arch/arm64/kernel/early_printk.c > +++ b/arch/arm64/kernel/early_printk.c > @@ -29,6 +29,21 @@ static void __iomem *early_base; > static void (*printch)(char ch); > > /* > + * UART (8250/16550) single character TX. > + */ > +static void uart_printch(char ch) > +{ > +#define UART_LSR 0x14 > +#define UART_TX 0x0 Please use the constants defined in include/uapi/linux/serial_reg.h, together with the proper shifts. > + > + while (!(readl_relaxed(early_base + UART_LSR) & 0x20)) > + ; You may want to test both UART_LSR_TEMT and UART_LSR_THRE here. > + writeb_relaxed(ch, early_base + UART_TX); > + while (!(readl_relaxed(early_base + UART_LSR) & 0x20)) > + ; Why do you have to wait again here? It should be enough to do in only once when entering the function. > +} > + > +/* > * PL011 single character TX. > */ > static void pl011_printch(char ch) > @@ -47,6 +62,7 @@ struct earlycon_match { > > static const struct earlycon_match earlycon_match[] __initconst = { > { .name = "pl011", .printch = pl011_printch, }, > + { .name = "uart", .printch = uart_printch, }, "uart" is way too generic. pl011 is an UART too, and I suspect most of the backends that are going to be added here over time will be UARTs. "uart8250" would be a possibility (and actually consistent with the rest of the kernel, see drivers/tty/serial/8250/8250_early.c. Cheers, M.
On Thu, Feb 28, 2013 at 11:34:17AM +0000, Marc Zyngier wrote: > On 28/02/13 11:01, Anup Patel wrote: > > +} > > + > > +/* > > * PL011 single character TX. > > */ > > static void pl011_printch(char ch) > > @@ -47,6 +62,7 @@ struct earlycon_match { > > > > static const struct earlycon_match earlycon_match[] __initconst = { > > { .name = "pl011", .printch = pl011_printch, }, > > + { .name = "uart", .printch = uart_printch, }, > > "uart" is way too generic. pl011 is an UART too, and I suspect most of > the backends that are going to be added here over time will be UARTs. > > "uart8250" would be a possibility (and actually consistent with the rest > of the kernel, see drivers/tty/serial/8250/8250_early.c. I think it makes more sense to use the existing 8250_early.c driver. It has more features than the simple earlyprintk implementation in the 64-bit kernel (like parsing more parameters, initialising the UART). The only difference is that the early_param is called "earlycon".
On 28/02/13 12:10, Catalin Marinas wrote: > On Thu, Feb 28, 2013 at 11:34:17AM +0000, Marc Zyngier wrote: >> On 28/02/13 11:01, Anup Patel wrote: >>> +} >>> + >>> +/* >>> * PL011 single character TX. >>> */ >>> static void pl011_printch(char ch) >>> @@ -47,6 +62,7 @@ struct earlycon_match { >>> >>> static const struct earlycon_match earlycon_match[] __initconst = { >>> { .name = "pl011", .printch = pl011_printch, }, >>> + { .name = "uart", .printch = uart_printch, }, >> >> "uart" is way too generic. pl011 is an UART too, and I suspect most of >> the backends that are going to be added here over time will be UARTs. >> >> "uart8250" would be a possibility (and actually consistent with the rest >> of the kernel, see drivers/tty/serial/8250/8250_early.c. > > I think it makes more sense to use the existing 8250_early.c driver. It > has more features than the simple earlyprintk implementation in the > 64-bit kernel (like parsing more parameters, initialising the UART). The > only difference is that the early_param is called "earlycon". Indeed, this seems to be the best way, as it removes the need for this patch altogether. M.
On Thu, Feb 28, 2013 at 5:04 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 28/02/13 11:01, Anup Patel wrote: > > Hi Anup, > >> Signed-off-by: Anup Patel <apatel@apm.com> > > A proper patch description would be nice. Oke will do. > >> --- >> arch/arm64/kernel/early_printk.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/arch/arm64/kernel/early_printk.c b/arch/arm64/kernel/early_printk.c >> index 7e320a2..62953ed 100644 >> --- a/arch/arm64/kernel/early_printk.c >> +++ b/arch/arm64/kernel/early_printk.c >> @@ -29,6 +29,21 @@ static void __iomem *early_base; >> static void (*printch)(char ch); >> >> /* >> + * UART (8250/16550) single character TX. >> + */ >> +static void uart_printch(char ch) >> +{ >> +#define UART_LSR 0x14 >> +#define UART_TX 0x0 > > Please use the constants defined in include/uapi/linux/serial_reg.h, > together with the proper shifts. Yes, I am fine with reusing constants from serial_reg.h. Its just that I did not want to include entire header for 2 register accesses. > >> + >> + while (!(readl_relaxed(early_base + UART_LSR) & 0x20)) >> + ; > > You may want to test both UART_LSR_TEMT and UART_LSR_THRE here. We only need to check for empty space in Tx FIFO or Tx Holding Register hence a check on UART_LSR_THRE bit would suffice. (For more info, http://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming) > >> + writeb_relaxed(ch, early_base + UART_TX); >> + while (!(readl_relaxed(early_base + UART_LSR) & 0x20)) >> + ; > > Why do you have to wait again here? It should be enough to do in only > once when entering the function. Yes, the second loop seems to be redundant. Its there because I tried to implement uart_printch() similar to pl011_printch(). > >> +} >> + >> +/* >> * PL011 single character TX. >> */ >> static void pl011_printch(char ch) >> @@ -47,6 +62,7 @@ struct earlycon_match { >> >> static const struct earlycon_match earlycon_match[] __initconst = { >> { .name = "pl011", .printch = pl011_printch, }, >> + { .name = "uart", .printch = uart_printch, }, > > "uart" is way too generic. pl011 is an UART too, and I suspect most of > the backends that are going to be added here over time will be UARTs. > > "uart8250" would be a possibility (and actually consistent with the rest > of the kernel, see drivers/tty/serial/8250/8250_early.c. Actually, I was thinking of having earlyprintk=8250. What say ? > > Cheers, > > M. > -- > Jazz is not dead. It just smells funny... > Regards, Anup CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
On 02/28/2013 05:01 AM, Anup Patel wrote: > Signed-off-by: Anup Patel <apatel@apm.com> > --- > arch/arm64/kernel/early_printk.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, > is for the sole use of the intended recipient(s) and contains information > that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. > It is to be used solely for the purpose of furthering the parties' business relationship. > All unauthorized review, use, disclosure or distribution is prohibited. > If you are not the intended recipient, please contact the sender by reply e-mail > and destroy all copies of the original message. > Is this patch confidential or GPL? You can't submit patches with a notice like this. Rob
On Thu, Feb 28, 2013 at 6:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 28/02/13 12:10, Catalin Marinas wrote: >> On Thu, Feb 28, 2013 at 11:34:17AM +0000, Marc Zyngier wrote: >>> On 28/02/13 11:01, Anup Patel wrote: >>>> +} >>>> + >>>> +/* >>>> * PL011 single character TX. >>>> */ >>>> static void pl011_printch(char ch) >>>> @@ -47,6 +62,7 @@ struct earlycon_match { >>>> >>>> static const struct earlycon_match earlycon_match[] __initconst = { >>>> { .name = "pl011", .printch = pl011_printch, }, >>>> + { .name = "uart", .printch = uart_printch, }, >>> >>> "uart" is way too generic. pl011 is an UART too, and I suspect most of >>> the backends that are going to be added here over time will be UARTs. >>> >>> "uart8250" would be a possibility (and actually consistent with the rest >>> of the kernel, see drivers/tty/serial/8250/8250_early.c. >> >> I think it makes more sense to use the existing 8250_early.c driver. It >> has more features than the simple earlyprintk implementation in the >> 64-bit kernel (like parsing more parameters, initialising the UART). The >> only difference is that the early_param is called "earlycon". > > Indeed, this seems to be the best way, as it removes the need for this > patch altogether. The earlycon option does not seem to work on my ARM64 kernel because it uses ioremap_nocache() whereas ARM64 earlyprintk uses early_io_map() I do agree that earlycon is more feature rich compared to ARM64 earlyprintk but I doubt whether 8250_early.c is generic enough to be usable on ARM64 kernel The best part about ARM64 earlyprintk is that its very very simple and does not expect us to re-programme UART HW which is already programmed by the the bootloader. IMHO, adding 8250 support in ARM64 earlyprintk would be very ARM64 specific and not re-implementation of 8250_early.c > > M. > -- > Jazz is not dead. It just smells funny... > Regards, Anup CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
On Thu, Feb 28, 2013 at 7:33 PM, Rob Herring <robherring2@gmail.com> wrote: > On 02/28/2013 05:01 AM, Anup Patel wrote: >> Signed-off-by: Anup Patel <apatel@apm.com> >> --- >> arch/arm64/kernel/early_printk.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) > > >> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, >> is for the sole use of the intended recipient(s) and contains information >> that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. >> It is to be used solely for the purpose of furthering the parties' business relationship. >> All unauthorized review, use, disclosure or distribution is prohibited. >> If you are not the intended recipient, please contact the sender by reply e-mail >> and destroy all copies of the original message. >> > > Is this patch confidential or GPL? You can't submit patches with a > notice like this. > > Rob > No this patch is not confidential. I am sending this patch with appropriate permissions from APM. Actually, we at APM cannot disable this footer (which is a default setting) so please ignore it. Regards, Anup CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
On 02/28/2013 08:11 AM, Anup Patel wrote: > On Thu, Feb 28, 2013 at 7:33 PM, Rob Herring <robherring2@gmail.com> wrote: >> On 02/28/2013 05:01 AM, Anup Patel wrote: >>> Signed-off-by: Anup Patel <apatel@apm.com> >>> --- >>> arch/arm64/kernel/early_printk.c | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >> >> >>> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, >>> is for the sole use of the intended recipient(s) and contains information >>> that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. >>> It is to be used solely for the purpose of furthering the parties' business relationship. >>> All unauthorized review, use, disclosure or distribution is prohibited. >>> If you are not the intended recipient, please contact the sender by reply e-mail >>> and destroy all copies of the original message. >>> >> >> Is this patch confidential or GPL? You can't submit patches with a >> notice like this. >> >> Rob >> > > No this patch is not confidential. I am sending this patch with > appropriate permissions from APM. The signature doesn't tell anyone that. There are several examples of the same thing: http://www.mail-archive.com/linux-usb@vger.kernel.org/msg11135.html https://lkml.org/lkml/2011/4/22/147 Of course, there's probably many examples with patches accepted anyway by less attentive maintainers. > Actually, we at APM cannot disable this footer (which is a default > setting) so please ignore it. Which is one reason why people don't use their corporate email. Rob > > Regards, > Anup > CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, > is for the sole use of the intended recipient(s) and contains information > that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. > It is to be used solely for the purpose of furthering the parties' business relationship. > All unauthorized review, use, disclosure or distribution is prohibited. > If you are not the intended recipient, please contact the sender by reply e-mail > and destroy all copies of the original message. >
On Thu, Feb 28, 2013 at 02:06:27PM +0000, Anup Patel wrote: > On Thu, Feb 28, 2013 at 6:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On 28/02/13 12:10, Catalin Marinas wrote: > >> On Thu, Feb 28, 2013 at 11:34:17AM +0000, Marc Zyngier wrote: > >>> On 28/02/13 11:01, Anup Patel wrote: > >>>> +} > >>>> + > >>>> +/* > >>>> * PL011 single character TX. > >>>> */ > >>>> static void pl011_printch(char ch) > >>>> @@ -47,6 +62,7 @@ struct earlycon_match { > >>>> > >>>> static const struct earlycon_match earlycon_match[] __initconst = { > >>>> { .name = "pl011", .printch = pl011_printch, }, > >>>> + { .name = "uart", .printch = uart_printch, }, > >>> > >>> "uart" is way too generic. pl011 is an UART too, and I suspect most of > >>> the backends that are going to be added here over time will be UARTs. > >>> > >>> "uart8250" would be a possibility (and actually consistent with the rest > >>> of the kernel, see drivers/tty/serial/8250/8250_early.c. > >> > >> I think it makes more sense to use the existing 8250_early.c driver. It > >> has more features than the simple earlyprintk implementation in the > >> 64-bit kernel (like parsing more parameters, initialising the UART). The > >> only difference is that the early_param is called "earlycon". > > > > Indeed, this seems to be the best way, as it removes the need for this > > patch altogether. > > The earlycon option does not seem to work on my ARM64 kernel because it uses > ioremap_nocache() whereas ARM64 earlyprintk uses early_io_map() We try to get earlyprintk as early as possible and ioremap() isn't available yet, hence the arm64-specific early_io_map(). It looks like x86 has a CONFIG_FIX_EARLYCON_MEM to avoid the ioremap() issue but I wouldn't add yet another option for arm64. So we can go with your arm64 patch, once you implement the feedback from Marc.
On Thu, Feb 28, 2013 at 9:01 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Thu, Feb 28, 2013 at 02:06:27PM +0000, Anup Patel wrote: >> On Thu, Feb 28, 2013 at 6:22 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> > On 28/02/13 12:10, Catalin Marinas wrote: >> >> On Thu, Feb 28, 2013 at 11:34:17AM +0000, Marc Zyngier wrote: >> >>> On 28/02/13 11:01, Anup Patel wrote: >> >>>> +} >> >>>> + >> >>>> +/* >> >>>> * PL011 single character TX. >> >>>> */ >> >>>> static void pl011_printch(char ch) >> >>>> @@ -47,6 +62,7 @@ struct earlycon_match { >> >>>> >> >>>> static const struct earlycon_match earlycon_match[] __initconst = { >> >>>> { .name = "pl011", .printch = pl011_printch, }, >> >>>> + { .name = "uart", .printch = uart_printch, }, >> >>> >> >>> "uart" is way too generic. pl011 is an UART too, and I suspect most of >> >>> the backends that are going to be added here over time will be UARTs. >> >>> >> >>> "uart8250" would be a possibility (and actually consistent with the rest >> >>> of the kernel, see drivers/tty/serial/8250/8250_early.c. >> >> >> >> I think it makes more sense to use the existing 8250_early.c driver. It >> >> has more features than the simple earlyprintk implementation in the >> >> 64-bit kernel (like parsing more parameters, initialising the UART). The >> >> only difference is that the early_param is called "earlycon". >> > >> > Indeed, this seems to be the best way, as it removes the need for this >> > patch altogether. >> >> The earlycon option does not seem to work on my ARM64 kernel because it uses >> ioremap_nocache() whereas ARM64 earlyprintk uses early_io_map() > > We try to get earlyprintk as early as possible and ioremap() isn't > available yet, hence the arm64-specific early_io_map(). It looks like > x86 has a CONFIG_FIX_EARLYCON_MEM to avoid the ioremap() issue but I > wouldn't add yet another option for arm64. > > So we can go with your arm64 patch, once you implement the feedback from > Marc. > > -- > Catalin Sure, I will incorporate Marc's comments and resend the patch with more appropriate subject and description. Regards, Anup CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
On Thu, Feb 28, 2013 at 8:32 PM, Rob Herring <robherring2@gmail.com> wrote: > On 02/28/2013 08:11 AM, Anup Patel wrote: >> On Thu, Feb 28, 2013 at 7:33 PM, Rob Herring <robherring2@gmail.com> wrote: >>> On 02/28/2013 05:01 AM, Anup Patel wrote: >>>> Signed-off-by: Anup Patel <apatel@apm.com> >>>> --- >>>> arch/arm64/kernel/early_printk.c | 16 ++++++++++++++++ >>>> 1 file changed, 16 insertions(+) >>> >>> >>>> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, >>>> is for the sole use of the intended recipient(s) and contains information >>>> that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. >>>> It is to be used solely for the purpose of furthering the parties' business relationship. >>>> All unauthorized review, use, disclosure or distribution is prohibited. >>>> If you are not the intended recipient, please contact the sender by reply e-mail >>>> and destroy all copies of the original message. >>>> >>> >>> Is this patch confidential or GPL? You can't submit patches with a >>> notice like this. >>> >>> Rob >>> >> >> No this patch is not confidential. I am sending this patch with >> appropriate permissions from APM. > > The signature doesn't tell anyone that. There are several examples of > the same thing: > > http://www.mail-archive.com/linux-usb@vger.kernel.org/msg11135.html > https://lkml.org/lkml/2011/4/22/147 > > Of course, there's probably many examples with patches accepted anyway > by less attentive maintainers. > >> Actually, we at APM cannot disable this footer (which is a default >> setting) so please ignore it. > > Which is one reason why people don't use their corporate email. Let me try to use my personal email from next patch onwards if APM folks allow me to do that. > > Rob > >> >> Regards, >> Anup >> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, >> is for the sole use of the intended recipient(s) and contains information >> that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. >> It is to be used solely for the purpose of furthering the parties' business relationship. >> All unauthorized review, use, disclosure or distribution is prohibited. >> If you are not the intended recipient, please contact the sender by reply e-mail >> and destroy all copies of the original message. >> > Regards, Anup Patel CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
diff --git a/arch/arm64/kernel/early_printk.c b/arch/arm64/kernel/early_printk.c index 7e320a2..62953ed 100644 --- a/arch/arm64/kernel/early_printk.c +++ b/arch/arm64/kernel/early_printk.c @@ -29,6 +29,21 @@ static void __iomem *early_base; static void (*printch)(char ch); /* + * UART (8250/16550) single character TX. + */ +static void uart_printch(char ch) +{ +#define UART_LSR 0x14 +#define UART_TX 0x0 + + while (!(readl_relaxed(early_base + UART_LSR) & 0x20)) + ; + writeb_relaxed(ch, early_base + UART_TX); + while (!(readl_relaxed(early_base + UART_LSR) & 0x20)) + ; +} + +/* * PL011 single character TX. */ static void pl011_printch(char ch) @@ -47,6 +62,7 @@ struct earlycon_match { static const struct earlycon_match earlycon_match[] __initconst = { { .name = "pl011", .printch = pl011_printch, }, + { .name = "uart", .printch = uart_printch, }, {} };
Signed-off-by: Anup Patel <apatel@apm.com> --- arch/arm64/kernel/early_printk.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)