Re: [PATCH 01/28] serial: earlycon: allow MEM32 I/O for DT earlycon
On 11/30/2015 05:52 PM, Rob Herring wrote:
> On Mon, Nov 30, 2015 at 10:21 AM, Paul Burton wrote:
>> Read the reg-io-width property when earlycon is setup via device tree,
>> and set the I/O type to UPIO_MEM32 when 4 is read. This behaviour
>> matches that of the of_serial driver, and is needed for DT configured
>> earlycon on the MIPS Boston board.
>>
>> Note that this is only possible when CONFIG_LIBFDT is enabled, but
>> enabling it everywhere seems like overkill. Thus systems that need this
>> functionality should select CONFIG_LIBFDT for themselves.
>
> libfdt is enabled if you are booting from DT, so checking this
> property should not add anything.
>
>>
>> Signed-off-by: Paul Burton
>> ---
>>
>> drivers/of/fdt.c | 2 +-
>> drivers/tty/serial/Makefile | 1 +
>> drivers/tty/serial/earlycon.c | 15 ++-
>> include/linux/serial_core.h | 2 +-
>> 4 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index d243029..71c7f0d 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -833,7 +833,7 @@ static int __init early_init_dt_scan_chosen_serial(void)
>> if (addr == OF_BAD_ADDR)
>> return -ENXIO;
>>
>> - of_setup_earlycon(addr, match->data);
>> + of_setup_earlycon(fdt, offset, addr, match->data);
>> return 0;
>> }
>> return -ENODEV;
>> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
>> index 5ab4111..1d290d6 100644
>> --- a/drivers/tty/serial/Makefile
>> +++ b/drivers/tty/serial/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_SERIAL_21285) += 21285.o
>>
>> obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
>> obj-$(CONFIG_SERIAL_EARLYCON_ARM_SEMIHOST) += earlycon-arm-semihost.o
>> +CFLAGS_earlycon.o += -I$(srctree)/scripts/dtc/libfdt
>
> This is no longer necessary.
>
>>
>> # These Sparc drivers have to appear before others such as 8250
>> # which share ttySx minor node space. Otherwise console device
>> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
>> index f096360..2b936a7 100644
>> --- a/drivers/tty/serial/earlycon.c
>> +++ b/drivers/tty/serial/earlycon.c
>> @@ -17,6 +17,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -196,17 +197,29 @@ static int __init param_setup_earlycon(char *buf)
>> }
>> early_param("earlycon", param_setup_earlycon);
>>
>> -int __init of_setup_earlycon(unsigned long addr,
>> +int __init of_setup_earlycon(const void *fdt, int offset, unsigned long
>> addr,
>
> I would add iotype as a parameter instead, and then...
>
>> int (*setup)(struct earlycon_device *, const
>> char *))
>> {
>> int err;
>> struct uart_port *port = &early_console_dev.port;
>> + const __be32 *prop;
>>
>> port->iotype = UPIO_MEM;
>> port->mapbase = addr;
>> port->uartclk = BASE_BAUD * 16;
>> port->membase = earlycon_map(addr, SZ_4K);
>>
>> + if (config_enabled(CONFIG_LIBFDT)) {
>> + prop = fdt_getprop(fdt, offset, "reg-io-width", NULL);
>> + if (prop) {
>> + switch (be32_to_cpup(prop)) {
>> + case 4:
>> + port->iotype = UPIO_MEM32;
>> + break;
>> + }
>> + }
>
> ...move this parsing into fdt.c where we parse the address.
FWIW, all of of_setup_earlycon() should only be #ifdef CONFIG_OF_EARLY_FLATTREE
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Which is better to specify console, "console= " or "stdout-path" ?
Hi Masahiro,
On 10/22/2015 12:20 AM, Masahiro Yamada wrote:
> 2015-10-22 1:26 GMT+09:00 Peter Hurley :
>> On 10/21/2015 11:38 AM, Masahiro Yamada wrote:
>>> 2015-10-21 21:46 GMT+09:00 Peter Hurley :
>>>> On 10/21/2015 04:58 AM, Sudeep Holla wrote:
>>>>> On 21/10/15 06:09, Masahiro Yamada wrote:
>>>>>> I think there exist two ways to specify the console port and baudrate.
>>>>>>
>>>>>>
>>>>>> [1] Specify console in bootargs
>>>>>>
>>>>>>chosen {
>>>>>> bootargs = "console=ttyS0,115200";
>>>>>>};
>>>>>>
>>>>>>
>>>>>> [2] Specify stdout-path
>>>>>>
>>>>>> chosen {
>>>>>> stdout-path = "serial0:115200n8";
>>>>>
>>>>> This will work for even early/boot console, so this is better than
>>>>> option [1]
>>>>
>>>> Be aware that options specified via /chosen/stdout-path are
>>>> currently ignored by earlycon. There were some hiccups getting the
>>>> initial support upstream; when 4.4 hits mainline, I'll resubmit
>>>> my series that implements the of_serial i/o properties and
>>>> options passthrough to earlycon setup.
>>>
>>>
>>> As I said in another thread ("serial: earlycon: allow to specify
>>> uartclk in earlycon kernel-parameter"),
>>> stdout-path can pass dev->baud, but not port->uartclk.
>>
>> That's true but I'm not seeing in that thread where you wrote that?
>
> Sorry, I made you confused. I was talking about the kernel parameter
> (console=)
> in the thread.
>
>> My replies there were specific to uartclk on the kernel command line,
>> which isn't necessary if the bootloader has already initialized the
>> uart.
>>
>>> It is usually specified "clocks" phandle, but
>>> clk is not ready at the point of earlycon.
>>>
>>> It seems impossible to set up the baudrate even if the options are passed.
>>
>> It's difficult to understand what you're trying to do when I can't
>> see the code you're referring to. For example, I only recently
>> understood that you're talking about a earlycon implementation
>> that you're working on and not the 8250 earlycon itself.
>
> Sorry again for making you confused.
>
> I was talking both.
>
> Now I am tackling on some ARM board porting.
>
>
> The board has a pure 8250 family device (compatible = "ns16550a") on it.
>
> In addition, there exist 8250-variant IPs inside the SoC.
> (this is similar to 8250, but slightly different.
> drivers/tty/serial/8250/8250_uniphier.c)
>
>
> What I want to do is:
> [1] To use drivers/tty/serial/8250/8250_early.c for the on-board ns16550a.
> [2] To implement my own EARLYCON_DECLARE() in
> drivers/tty/serial/8250/8250_uniphier.c
>
>
>
>> If you look at other non-8250 earlycons, you'll see they all ignore
>> the baud rate, on the assumption the bootloader already set it up.
>
> OK, I will do so for [2].
>
>
>> The 8250 earlycon is a little different because legacy platforms
>> do not initialize the uart.
>
>
> Make sense.
>
> For legacy platforms, earlycon initializes the uart,
> assuming the hard-coded "port->uartclk = BASE_BAUD * 16"
> is the value.
>
>
> For embedded boards such as ARM, the boot-loader should initialize the UART
> and the earlycon should preserve it because port->uartclk varies from
> board to board.
> (For example, the ns16550a on my board expects BASE_BAUD is 1228000,
> but it does not match the one in include/asm-generic/serial.h )
I want to clarify one point: if you have a configuration for which the
earlycon uart is not initialized by the bootloader (or boot prom), then
we can add a way for the ASoC init to set BASE_BAUD. This was done for
the ARC arch, but I would like to use this as a last resort for the ARM
arch.
An example configuration I can envision that would require this solution
is u-boot's so-called 'falcon mode' without devicetree (or boot prom).
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of/fdt: fix error checking for earlycon address
On 10/27/2015 04:46 PM, Rob Herring wrote: > On Fri, Oct 23, 2015 at 8:15 AM, Peter Hurley > wrote: >> Hi Masahiro, >> >> On 10/23/2015 07:47 AM, Masahiro Yamada wrote: >>> fdt_translate_address() returns OF_BAD_ADDR on error. It is defined as >>> a u64 value, so the variable "addr" should be defined as u64 as well. >> >> Good catch. >> >> I would prefer if fdt_translate_address() returned resource_size_t (which >> is the proper type for handling addresses as integers) and that type >> was propagated through early_init_dt_scan_chosen_serial => of_setup_earlycon. > > That can be problematic in the DT code. The size of resource_size_t > can vary on 32-bit as it is based on phys_addr_t which in turn is > based on CONFIG_LPAE setting. However, the address sizes in DT are > fixed and may be 64-bit. So we stick with u64 in the DT code. I think I see your point about fdt_translate_address(); you want it to mirror of_translate_address(), yes? If so, then I have no objection to this patch and will mark it reviewed. But the u64 value from fdt_translate_address() should be preserved at least to the ioremap()/set_fixmap() in earlycon_map() . I could roll that change into my earlycon series that moves fdt_translate_address() into of_setup_earlycon(), thus preserving the u64 value until it's assigned to a phys_addr_t. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of/fdt: fix error checking for earlycon address
Hi Masahiro,
On 10/23/2015 07:47 AM, Masahiro Yamada wrote:
> fdt_translate_address() returns OF_BAD_ADDR on error. It is defined as
> a u64 value, so the variable "addr" should be defined as u64 as well.
Good catch.
I would prefer if fdt_translate_address() returned resource_size_t (which
is the proper type for handling addresses as integers) and that type
was propagated through early_init_dt_scan_chosen_serial => of_setup_earlycon.
Regards,
Peter Hurley
PS - It seems that there is a new user of fdt_translate_address() in the
nios arch:
commit e8bf5bc776edef44777b13b2eb4461d653519bae
Author: Ley Foon Tan
Date: Tue Feb 10 23:21:08 2015 +0800
nios2: add early printk support
Signed-off-by: Ley Foon Tan
I've copied the author as that should have been an earlycon in the
altera uart driver(s) rather than early_printk. Oh well.
> Fixes: fb11ffe74c79 ("of/fdt: add FDT serial scanning for earlycon")
> Signed-off-by: Masahiro Yamada
> ---
>
> drivers/of/fdt.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 9fc3568..196e449 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -822,14 +822,15 @@ static int __init early_init_dt_scan_chosen_serial(void)
> return -ENODEV;
>
> while (match->compatible[0]) {
> - unsigned long addr;
> + u64 addr;
> +
> if (fdt_node_check_compatible(fdt, offset, match->compatible)) {
> match++;
> continue;
> }
>
> addr = fdt_translate_address(fdt, offset);
> - if (!addr)
> + if (addr == OF_BAD_ADDR)
> return -ENXIO;
>
> of_setup_earlycon(addr, match->data);
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Which is better to specify console, "console= " or "stdout-path" ?
On 10/21/2015 11:38 AM, Masahiro Yamada wrote:
> Hi Peter.
>
> 2015-10-21 21:46 GMT+09:00 Peter Hurley :
>> On 10/21/2015 04:58 AM, Sudeep Holla wrote:
>>> On 21/10/15 06:09, Masahiro Yamada wrote:
>>>> I think there exist two ways to specify the console port and baudrate.
>>>>
>>>>
>>>> [1] Specify console in bootargs
>>>>
>>>>chosen {
>>>> bootargs = "console=ttyS0,115200";
>>>>};
>>>>
>>>>
>>>> [2] Specify stdout-path
>>>>
>>>> chosen {
>>>> stdout-path = "serial0:115200n8";
>>>
>>> This will work for even early/boot console, so this is better than
>>> option [1]
>>
>> Be aware that options specified via /chosen/stdout-path are
>> currently ignored by earlycon. There were some hiccups getting the
>> initial support upstream; when 4.4 hits mainline, I'll resubmit
>> my series that implements the of_serial i/o properties and
>> options passthrough to earlycon setup.
>
>
> As I said in another thread ("serial: earlycon: allow to specify
> uartclk in earlycon kernel-parameter"),
> stdout-path can pass dev->baud, but not port->uartclk.
That's true but I'm not seeing in that thread where you wrote that?
My replies there were specific to uartclk on the kernel command line,
which isn't necessary if the bootloader has already initialized the
uart.
> It is usually specified "clocks" phandle, but
> clk is not ready at the point of earlycon.
>
> It seems impossible to set up the baudrate even if the options are passed.
It's difficult to understand what you're trying to do when I can't
see the code you're referring to. For example, I only recently
understood that you're talking about a earlycon implementation
that you're working on and not the 8250 earlycon itself.
If you look at other non-8250 earlycons, you'll see they all ignore
the baud rate, on the assumption the bootloader already set it up.
The 8250 earlycon is a little different because legacy platforms
do not initialize the uart.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Which is better to specify console, "console= " or "stdout-path" ?
On 10/21/2015 08:54 AM, Sudeep Holla wrote:
>
>
> On 21/10/15 13:46, Peter Hurley wrote:
>> On 10/21/2015 04:58 AM, Sudeep Holla wrote:
>>> On 21/10/15 06:09, Masahiro Yamada wrote:
>>>> I think there exist two ways to specify the console port and baudrate.
>>>>
>>>>
>>>> [1] Specify console in bootargs
>>>>
>>>> chosen {
>>>> bootargs = "console=ttyS0,115200";
>>>> };
>>>>
>>>>
>>>> [2] Specify stdout-path
>>>>
>>>> chosen {
>>>>stdout-path = "serial0:115200n8";
>>>
>>> This will work for even early/boot console, so this is better than
>>> option [1]
>>
>> Be aware that options specified via /chosen/stdout-path are
>> currently ignored by earlycon. There were some hiccups getting the
>> initial support upstream; when 4.4 hits mainline, I'll resubmit
>> my series that implements the of_serial i/o properties and
>> options passthrough to earlycon setup.
>>
>
> Yes I am aware of that.
The others to which my email was addressed might not know that.
> IIUC it's in -next now and works fine for me.
Well, the fix in -next doesn't actually do anything wrt earlycon baud
rate; those changes were part of a larger series that depends on
the fix in -next.
Regards,
Peter Hurley
> But having stdout-path in the DTS won't hinder normal console in anyway.
> So I prefer it even without the patch you are referring(to support early
> console parsing stdout-path)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Which is better to specify console, "console= " or "stdout-path" ?
On 10/21/2015 04:58 AM, Sudeep Holla wrote:
> On 21/10/15 06:09, Masahiro Yamada wrote:
>> I think there exist two ways to specify the console port and baudrate.
>>
>>
>> [1] Specify console in bootargs
>>
>>chosen {
>> bootargs = "console=ttyS0,115200";
>>};
>>
>>
>> [2] Specify stdout-path
>>
>> chosen {
>> stdout-path = "serial0:115200n8";
>
> This will work for even early/boot console, so this is better than
> option [1]
Be aware that options specified via /chosen/stdout-path are
currently ignored by earlycon. There were some hiccups getting the
initial support upstream; when 4.4 hits mainline, I'll resubmit
my series that implements the of_serial i/o properties and
options passthrough to earlycon setup.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of/fdt: fix aliases with baudrate in earlycon
On 10/10/2015 04:29 AM, Stefan Agner wrote: > Many boards use an alias in the stdout-path specification along > with console options after a colon (e.g. serial0:115200n8). When > using earlycon, this specification currently does not work. While > fdt_path_offset supports alias resolution, it does not remove the > console options by itself. Use the fdt_path_offset_namelen variant > and provide the length of the alias to enable aliases with console > options in the stdout-path. > > Signed-off-by: Stefan Agner > --- > Hi, > > Stumbled upon this while testing 32-bit ARM earlycon support. It > seems that this once already came up on the list: > https://lkml.org/lkml/2015/3/13/562 Yeah, looks like my patch got lost somewhere. https://lkml.org/lkml/2015/4/8/604 Regards, Peter Hurley > > drivers/of/fdt.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index 6e82bc42..9fc3568 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -813,8 +813,11 @@ static int __init early_init_dt_scan_chosen_serial(void) > if (!p || !l) > return -ENOENT; > > + /* Remove console options if present */ > + l = strchrnul(p, ':') - p; > + > /* Get the node specified by stdout-path */ > - offset = fdt_path_offset(fdt, p); > + offset = fdt_path_offset_namelen(fdt, p, l); > if (offset < 0) > return -ENODEV; > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/7] serial: 8250_omap: workaround for IP errata
On 07/14/2015 04:02 AM, Sekhar Nori wrote: > This series works around "Advisory 21" as documented in > AM437x SoC errata[1]. This errata prevents UART module > from idling after DMA is used. AM335x and DRA7x also suffer > from the same errata and chip design team is in the process > of updating the errata documents of those devices as well. > > Patch 1/7 fixes a related bug but can be applied independently. Looks good. Thanks Sekhar! For series, Reviewed-by: Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA
On 07/09/2015 10:15 AM, Sekhar Nori wrote:
> On Thursday 09 July 2015 07:03 AM, Peter Hurley wrote:
[...]
>>> @@ -1307,6 +1320,36 @@ static int omap8250_runtime_suspend(struct device
>>> *dev)
>>> return -EBUSY;
>>> }
>>>
>>> + if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {
>>> + int sysc;
>>> + int syss;
>>> + int timeout = 100;
>>> +
>>> + sysc = serial_in(up, UART_OMAP_SYSC);
>>> +
>>> + /* softreset the UART */
>>> + sysc |= OMAP_UART_SYSC_SOFTRESET;
>>> + serial_out(up, UART_OMAP_SYSC, sysc);
>>> +
>>> + /* By experiments, 1us enough for reset complete on AM335x */
>>> + do {
>>> + udelay(1);
>>> + syss = serial_in(up, UART_OMAP_SYSS);
>>> + } while (--timeout && !(syss & OMAP_UART_SYSS_RESETDONE));
>>
>>
>> If there is not a omap helper function to reset modules, there should be.
>> Open-coding the module reset is not appropriate.
>
> There is work planned to have something implemented for OMAP as part of
> drivers/reset/ API. AFAIK, this depends on some PRCM code clean-up
> affecting multiple OMAP platforms and is couple of merge windows away
> from taking shape.
>
> Meanwhile, I think this is the best we can do. Other drivers like
> i2c-omap have done something similar (see omap_i2c_reset()).
Ok, then please make the reset a separate local function
(returning success/failure?). omap_8250_reset()?
A TODO or FIXME above it explaining
the temporary nature of the function would be appreciated :)
>>
>>> + if (!timeout) {
>>> + dev_err(dev, "timed out waiting for reset done\n");
>>> + return -ETIMEDOUT;
>>> + }
>>> +
>>> + /*
>>> +* For UART module wake-up to work, MDR1.MODESELECT should
>>> +* not be set to "Disable", so update it.
>>> +*/
>>> + if (device_may_wakeup(dev))
>>> + omap8250_update_mdr1(up, priv);
>>
>> Should this be unconditional?
>
> I do not see it doing any harm if done unconditionally. But it will be
> unnecessary. Unless the UART is being used for wakeup, we don't need
> MDR1 restored at this stage. And omap8250_restore_regs() will eventually
> restore the register anyway.
Yeah, I understand that, but special-casing it isn't adding any value;
it's not as if you actually want the module to not be in UART mode.
And the comment could be single-line:
/* Restore to UART mode after reset (for wakeup) */
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA
, there should be.
Open-coding the module reset is not appropriate.
> + if (!timeout) {
> + dev_err(dev, "timed out waiting for reset done\n");
> + return -ETIMEDOUT;
> + }
> +
> + /*
> + * For UART module wake-up to work, MDR1.MODESELECT should
> + * not be set to "Disable", so update it.
> + */
> + if (device_may_wakeup(dev))
> + omap8250_update_mdr1(up, priv);
Should this be unconditional?
Regards,
Peter Hurley
> + }
> +
> if (up->dma && up->dma->rxchan)
> omap_8250_rx_dma(up, UART_IIR_RX_TIMEOUT);
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] tty: 8250: omap: introduce function to update mdr1
Hi Sekhar,
On 07/06/2015 05:47 AM, Sekhar Nori wrote:
> updating mdr1 register on OMAP needs to take care of
> errata i202. Introduce a function to update mdr1.
>
> This will be useful later on when mdr1 needs to be
> written to from other places. No functional change.
This changelog is not clear. May I suggest:
serial: 8250_omap: Refactor MDR1 update
The errata [1] workaround implemented in follow-on patch,
"serial: 8250_omap: workaround errata on disabling UART after using DMA",
requires MDR1 register programming.
Extract MDR1 register update into helper function, omap8250_update_mdr1().
[1] Advisory 21 in http://www.ti.com/lit/er/sprz408b/sprz408b.pdf
Regards,
Peter Hurley
> Signed-off-by: Sekhar Nori
> ---
> drivers/tty/serial/8250/8250_omap.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index 20c5b9c4c288..d9c96b993a84 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -232,6 +232,15 @@ static void omap8250_update_scr(struct uart_8250_port
> *up,
> serial_out(up, UART_OMAP_SCR, priv->scr);
> }
>
> +static void omap8250_update_mdr1(struct uart_8250_port *up,
> + struct omap8250_priv *priv)
> +{
> + if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
> + omap_8250_mdr1_errataset(up, priv);
> + else
> + serial_out(up, UART_OMAP_MDR1, priv->mdr1);
> +}
> +
> static void omap8250_restore_regs(struct uart_8250_port *up)
> {
> struct omap8250_priv *priv = up->port.private_data;
> @@ -282,11 +291,9 @@ static void omap8250_restore_regs(struct uart_8250_port
> *up)
> serial_out(up, UART_XOFF1, priv->xoff);
>
> serial_out(up, UART_LCR, up->lcr);
> - /* need mode A for FCR */
> - if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
> - omap_8250_mdr1_errataset(up, priv);
> - else
> - serial_out(up, UART_OMAP_MDR1, priv->mdr1);
> +
> + omap8250_update_mdr1(up, priv);
> +
> up->port.ops->set_mctrl(&up->port, up->port.mctrl);
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] tty: 8250: omap eliminate use of of_machine_is_compatible()
Hi Sekhar,
On 07/06/2015 05:47 AM, Sekhar Nori wrote:
> Use of of_machine_is_compatible() for AM335x specific DMA
> quirk in 8250_omap driver makes it ugly to extend the
> quirk for other platforms. Instead use a new compatible.
>
> The new compatible will also make it easier to care of other
> quirks specific to AM335x and like SoCs.
>
> This patch does break backward DTB compatibility for users of
> 8250_omap driver on AM335x boards.
Not ok.
8250_omap was released with 3.19 and has been the default driver for
BeagleBone since 4.0.
Regards,
Peter Hurley
> However, the 8250_omap driver
> is new and omap_serial is still the default choice driver for UART
> and so choosing to break compatibility over keeping the code
> around forever.
>
> Signed-off-by: Sekhar Nori
> ---
> .../devicetree/bindings/serial/omap_serial.txt | 1 +
> arch/arm/boot/dts/am33xx.dtsi | 12
> drivers/tty/serial/8250/8250_omap.c| 35
> +-
> 3 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/omap_serial.txt
> b/Documentation/devicetree/bindings/serial/omap_serial.txt
> index d3bd2b1ec401..0ee88209b341 100644
> --- a/Documentation/devicetree/bindings/serial/omap_serial.txt
> +++ b/Documentation/devicetree/bindings/serial/omap_serial.txt
> @@ -5,6 +5,7 @@ Required properties:
> - compatible : should be "ti,omap3-uart" for OMAP3 controllers
> - compatible : should be "ti,omap4-uart" for OMAP4 controllers
> - compatible : should be "ti,am4372-uart" for AM437x controllers
> +- compatible : should be "ti,am3352-uart" for AM335x controllers
> - reg : address and length of the register space
> - interrupts or interrupts-extended : Should contain the uart interrupt
>specifier or both the interrupt
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 21fcc440fc1a..b76f9a2ce05d 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -210,7 +210,7 @@
> };
>
> uart0: serial@44e09000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart1";
> clock-frequency = <4800>;
> reg = <0x44e09000 0x2000>;
> @@ -221,7 +221,7 @@
> };
>
> uart1: serial@48022000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart2";
> clock-frequency = <4800>;
> reg = <0x48022000 0x2000>;
> @@ -232,7 +232,7 @@
> };
>
> uart2: serial@48024000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart3";
> clock-frequency = <4800>;
> reg = <0x48024000 0x2000>;
> @@ -243,7 +243,7 @@
> };
>
> uart3: serial@481a6000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart4";
> clock-frequency = <4800>;
> reg = <0x481a6000 0x2000>;
> @@ -252,7 +252,7 @@
> };
>
> uart4: serial@481a8000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart5";
> clock-frequency = <4800>;
> reg = <0x481a8000 0x2000>;
> @@ -261,7 +261,7 @@
> };
>
> uart5: serial@481aa000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart6";
> clock-frequency = <4800>;
> reg = <0x481aa000 0x2000>;
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index d9c96b993a84
Re: [PATCH RESEND 1/7] tty: serial: 8250: omap: fix kernel crash in suspend-to-ram
Hi Sekhar,
On 07/06/2015 05:47 AM, Sekhar Nori wrote:
> omap_device infrastructure has a suspend_noirq hook which
> runtime suspends all devices late in the suspend cycle (see
> _od_suspend_noirq() in arch/arm/mach-omap2/omap_device.c)
Why is omap2 the only arch/SoC that does this; ie., call the runtime
callbacks after the system pm callbacks?
Whatever positive it brings, it's a mess at the driver level.
For example, this driver has to hook prepare()/complete() so it can
set local state so that it can detect when the runtime suspend
is being called during system suspend.
Regards,
Peter Hurley
> This leads to a NULL pointer exception in 8250_omap driver
> since by the time omap8250_runtime_suspend() is called, 8250_dma
> driver has already set rxchan to NULL via serial8250_release_dma().
>
> Make an explicit check to see if rxchan is NULL in
> runtime_{suspend|resume} hooks to fix this.
>
> Signed-off-by: Sekhar Nori
> ---
> Previous version: http://www.spinics.net/lists/linux-omap/msg119459.html
> No change in this version except rebased to v4.2-rc1
>
> drivers/tty/serial/8250/8250_omap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index d75a66c72750..20c5b9c4c288 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1285,7 +1285,7 @@ static int omap8250_runtime_suspend(struct device *dev)
> return -EBUSY;
> }
>
> - if (up->dma)
> + if (up->dma && up->dma->rxchan)
> omap_8250_rx_dma(up, UART_IIR_RX_TIMEOUT);
>
> priv->latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
> @@ -1310,7 +1310,7 @@ static int omap8250_runtime_resume(struct device *dev)
> if (loss_cntx)
> omap8250_restore_regs(up);
>
> - if (up->dma)
> + if (up->dma && up->dma->rxchan)
> omap_8250_rx_dma(up, 0);
>
> priv->latency = priv->calc_latency;
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/15] of_serial: support for UARTs on I/O ports
[ + Arnd who has been reviewing/acking of_serial.c changes ]
On 05/22/2015 11:51 AM, Paul Burton wrote:
> If the address provided for the UART is of an I/O port rather than
> a regular memory address, then set the port iotype appropriately and
> write the address to iobase rather than mapbase.
>
> Signed-off-by: Paul Burton
> ---
>
> drivers/tty/serial/of_serial.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
> index 137381e..ccff9ba 100644
> --- a/drivers/tty/serial/of_serial.c
> +++ b/drivers/tty/serial/of_serial.c
> @@ -110,7 +110,12 @@ static int of_platform_serial_setup(struct
> platform_device *ofdev,
>
> port->irq = irq_of_parse_and_map(np, 0);
> port->iotype = UPIO_MEM;
> - if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> +
> + if (resource.flags & IORESOURCE_IO) {
> + port->iotype = UPIO_PORT;
> + port->iobase = port->mapbase;
> + port->mapbase = 0;
> + } else if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> switch (prop) {
> case 1:
> port->iotype = UPIO_MEM;
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Gta04-owner] [PATCH 0/3] tty slave device support - version 3.
Hi Nikolaus, On 05/06/2015 01:19 AM, Dr. H. Nikolaus Schaller wrote: > Am 05.05.2015 um 21:54 schrieb Peter Hurley : > >> Hi Neil, >> >> On 03/18/2015 01:58 AM, NeilBrown wrote: >>> here is version 3 of support for tty-slaves. >> >> Is there a v4 of this that I missed? > > We did have a lengthy discussion about [PATCH 3/3] how to best (1) > represent the slave device in the device tree but as far as I am concerned, > I do not see that we have a consensus (2) and the device tree maintainers > have no comments or clear guidelines so far. I understand there are some design considerations still apparently unresolved wrt devicetree representation. However, there were significant lifetime issues in v3 and I'd like some time to review v4 to understand how those were resolved, and study the implications. Maintainers are busy people and often don't provide design steering until the other pieces are ack'd. I'd like to see _a_ solution eventually, so let's move this process along. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [tty:tty-testing 9/30] drivers/of/fdt.c:807:2: error: implicit declaration of function 'fdt_path_offset_namelen'
[ +cc Rob H, Grant L, devicetree ]
On 04/28/2015 08:24 AM, kbuild test robot wrote:
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
> tty-testing
> head: 962a6a5ce6b8280d6ce7416ce57a36103d31a523
> commit: 5a95cfdfaf5cb960ea06f23dbe85ed269abf [9/30] of: earlycon: Fix
> 'stdout-path' with ':' path terminator
> config: microblaze-mmu_defconfig (attached as .config)
> reproduce:
> wget
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
> -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout 5a95cfdfaf5cb960ea06f23dbe85ed269abf
> # save the attached .config to linux build tree
> make.cross ARCH=microblaze
>
> All error/warnings:
>
>drivers/of/fdt.c: In function 'early_init_dt_scan_chosen_serial':
>>> drivers/of/fdt.c:807:2: error: implicit declaration of function
>>> 'fdt_path_offset_namelen' [-Werror=implicit-function-declaration]
> offset = fdt_path_offset_namelen(fdt, p, (int)(q - p));
> ^
>cc1: some warnings being treated as errors
Greg,
This error occurs because the patch "libfdt: Add fdt_path_offset_namelen()"
has not been upstreamed yet by the devicetree maintainers.
Please back out commits 5a95cfdfaf5c ("of: earlycon: Fix 'stdout-path' with ':'
path terminator")
through 865f9cc509db ("of: earlycon: Log more helpful message if earlycon not
found")
from tty-testing.
I'll just resend the series when the situation with the missing
commit has been sorted.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/4] misc: ti-st: use of_get_named_gpio_flags for nshutdown
Hi Eric, On 04/27/2015 04:18 PM, Eric Nelson wrote: > Hi Peter, > > On 04/27/2015 01:11 PM, Peter Hurley wrote: >> On 04/27/2015 03:27 PM, Eric Nelson wrote: >>> Use of_get_named_gpio_flags to retrieve the "nshutdown" gpio connected >>> to the BT_EN pin of the device when retrieving platform data from device >>> tree. >> >> This breaks all existing DTs wrt the 'nshutdown_gpio' key. >> I suggest using a different, optional key in the absence of 'nshutdown_gpio'. >> > > You mean all zero of them up-stream ;)? "nshutdown_gpio" DT key became ABI as of 4.0 > I did forward a patch set to the maintainers of the ti-linux-kernel on > git.ti.com: > > https://git.ti.com/ti-linux-kernel/ti-linux-kernel > > Regards, > > > Eric > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/4] misc: ti-st: use of_get_named_gpio_flags for nshutdown
On 04/27/2015 03:27 PM, Eric Nelson wrote:
> Use of_get_named_gpio_flags to retrieve the "nshutdown" gpio connected
> to the BT_EN pin of the device when retrieving platform data from device
> tree.
This breaks all existing DTs wrt the 'nshutdown_gpio' key.
I suggest using a different, optional key in the absence of 'nshutdown_gpio'.
Regards,
Peter Hurley
> This allows the polarity to be specified using GPIO_ACTIVE_HIGH/LOW
> in device tree.
>
> Signed-off-by: Eric Nelson
> ---
> V2 changes comments to use "assert" and "de-assert" to refer to
> the state of the nshutdown gpio instead of low/high/0/1.
>
> drivers/misc/ti-st/st_kim.c | 34 +-
> include/linux/ti_wilink_st.h | 6 +++---
> 2 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/misc/ti-st/st_kim.c b/drivers/misc/ti-st/st_kim.c
> index aaa17b0..54d5f50 100644
> --- a/drivers/misc/ti-st/st_kim.c
> +++ b/drivers/misc/ti-st/st_kim.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -481,10 +482,13 @@ long st_kim_start(void *kim_data)
> if (pdata->chip_enable)
> pdata->chip_enable(kim_gdata);
>
> - /* Configure BT nShutdown to HIGH state */
> - gpio_set_value(kim_gdata->nshutdown, GPIO_LOW);
> + /*
> + * De-assert nShutdown (enable bluetooth),
> + * but force a transition
> + */
> + gpio_set_value(kim_gdata->nshutdown, kim_gdata->shutdown_val);
> mdelay(5); /* FIXME: a proper toggle */
> - gpio_set_value(kim_gdata->nshutdown, GPIO_HIGH);
> + gpio_set_value(kim_gdata->nshutdown, !kim_gdata->shutdown_val);
> mdelay(100);
> /* re-initialize the completion */
> reinit_completion(&kim_gdata->ldisc_installed);
> @@ -527,7 +531,7 @@ long st_kim_start(void *kim_data)
> * (b) upon failure to either install ldisc or download firmware.
> * The function is responsible to (a) notify UIM about un-installation,
> * (b) flush UART if the ldisc was installed.
> - * (c) reset BT_EN - pull down nshutdown at the end.
> + * (c) reset BT_EN - assert nshutdown at the end.
> * (d) invoke platform's chip disabling routine.
> */
> long st_kim_stop(void *kim_data)
> @@ -565,12 +569,12 @@ long st_kim_stop(void *kim_data)
> err = -ETIMEDOUT;
> }
>
> - /* By default configure BT nShutdown to LOW state */
> - gpio_set_value(kim_gdata->nshutdown, GPIO_LOW);
> + /* Assert nShutdown (chip disabled), but force transitions */
> + gpio_set_value(kim_gdata->nshutdown, kim_gdata->shutdown_val);
> mdelay(1);
> - gpio_set_value(kim_gdata->nshutdown, GPIO_HIGH);
> + gpio_set_value(kim_gdata->nshutdown, !kim_gdata->shutdown_val);
> mdelay(1);
> - gpio_set_value(kim_gdata->nshutdown, GPIO_LOW);
> + gpio_set_value(kim_gdata->nshutdown, kim_gdata->shutdown_val);
>
> /* platform specific disable */
> if (pdata->chip_disable)
> @@ -749,6 +753,7 @@ static struct ti_st_plat_data *get_platform_data(struct
> device *dev)
> {
> struct device_node *np = dev->of_node;
> const u32 *dt_property;
> + enum of_gpio_flags flags;
> int len;
>
> dt_pdata = kzalloc(sizeof(*dt_pdata), GFP_KERNEL);
> @@ -759,8 +764,9 @@ static struct ti_st_plat_data *get_platform_data(struct
> device *dev)
> dt_property = of_get_property(np, "dev_name", &len);
> if (dt_property)
> memcpy(&dt_pdata->dev_name, dt_property, len);
> - of_property_read_u32(np, "nshutdown_gpio",
> - &dt_pdata->nshutdown_gpio);
> + dt_pdata->nshutdown_gpio = of_get_named_gpio_flags
> + (np, "nshutdown_gpio", 0, &flags);
> + dt_pdata->shutdown_val = !(flags & OF_GPIO_ACTIVE_LOW);
> of_property_read_u32(np, "flow_cntrl", &dt_pdata->flow_cntrl);
> of_property_read_u32(np, "baud_rate", &dt_pdata->baud_rate);
>
> @@ -808,16 +814,18 @@ static int kim_probe(struct platform_device *pdev)
> /* refer to itself */
> kim_gdata->core_data->kim_data = kim_gdata;
>
> - /* Claim the chip enable nShutdown gpio from the system */
> + /* Claim the nShutdown GPIO */
> kim_gdata->nshutdown = pdata->nshutdown_gpio;
> + kim_gdata->shutdown_val = pda
Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate
On 04/15/2015 10:35 AM, Rob Herring wrote: > On Wed, Apr 15, 2015 at 9:17 AM, Peter Hurley > wrote: >> 2. Should DT-specific drivers not be using irq_of_parse_and_map()? >>On probe failure irq_dispose_mapping() will be junking the mapping, >>thus invalidating the irq assignment in the platform resource table, >>which breaks platform drivers which might otherwise probe successfully. > > Generally no, they should not use irq_of_parse_and_map as we want > drivers to work with platform_data, DT, ACPI, or Bob's Firmware > Interface. I think most users are PPC drivers which don't have so much > of the probe ordering problems. Apologies for hijacking this thread for a moment. If of_device_alloc() creates the irq mapping, and no driver probes succeed, what is disposing the mapping? Similarly, if a platform driver fails its probe after platform_get_irq() what should dispose of that mapping? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] OF: probe order dependency aware of_platform_populate
On 04/08/2015 09:40 AM, Rob Herring wrote: > This doesn't sound right. It ignores failures because platform_get_irq > will parse the interrupts when called rather than just using the > resource struct and will return EPROBE_DEFER if the irq resource is > not ready. We left the of_device_alloc code in to be safe, but we > should be able to remove it. This brings up a couple of points which are plaguing the serial drivers: 1. Is platform_get_irq() now required to properly obtain the mapped irq for DT-aware drivers? IOW, is platform_get_resource(IORESOURCE_IRQ) broken? Will it be if the of_device_alloc() code is removed? 2. Should DT-specific drivers not be using irq_of_parse_and_map()? On probe failure irq_dispose_mapping() will be junking the mapping, thus invalidating the irq assignment in the platform resource table, which breaks platform drivers which might otherwise probe successfully. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
Hi Kevin, On 04/02/2015 11:36 AM, Peter Hurley wrote: > On 03/28/2015 03:28 PM, Kevin Cernekee wrote: >> Hi Peter, >> >> This is my latest work-in-progress, incorporating the feedback from >> you and Grant: >> >> https://github.com/cernekee/linux/commits/endian >> >> Not sure if this code plays nice with your recent cleanups? If we're >> touching the same files/functions we should probably coordinate. > > Ok, I'll look over your git tree and add whatever's required to > earlycon. Could you submit the first 5 patches from your 'endian' branch, so that big-endian support can be added to earlycon? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 10/15] serial: stm32-usart: Add STM32 USART Driver
Hi Andreas, On 04/07/2015 12:51 PM, Andreas Färber wrote: > Am 07.04.2015 um 18:30 schrieb Maxime Coquelin: >> This drivers adds support to the STM32 USART controller, which is a >> standard serial driver. >> >> Tested-by: Chanwoo Choi >> Reviewed-by: Peter Hurley >> Signed-off-by: Maxime Coquelin >> --- >> drivers/tty/serial/Kconfig | 17 + >> drivers/tty/serial/Makefile | 1 + >> drivers/tty/serial/stm32-usart.c | 735 >> +++ >> include/uapi/linux/serial_core.h | 3 + >> 4 files changed, 756 insertions(+) >> create mode 100644 drivers/tty/serial/stm32-usart.c > [...] >> diff --git a/drivers/tty/serial/stm32-usart.c >> b/drivers/tty/serial/stm32-usart.c >> new file mode 100644 >> index 000..4adc430 >> --- /dev/null >> +++ b/drivers/tty/serial/stm32-usart.c > [...] >> +#define DRIVER_NAME "stm32-usart" >> +#define STM32_SERIAL_NAME "ttyS" > > I'm surprised no one has complained about ttyS yet. Doesn't that need to > be unique, such as ttySTM (efm32 uses ttyefm), to avoid clashes between > serial drivers? ttyS was exclusive to the 8250 driver, I thought. As long as this platform doesn't support 8250 h/w, I don't care; and by the time this platform is ready for multiconfig, I'll have ttyS coexistence fixed. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
On 04/02/2015 11:35 AM, Peter Hurley wrote:
> On 04/02/2015 09:46 AM, Rob Herring wrote:
>> Sorry about that. I had thought about doing the same thing. At least
>> unifying the macros, but not necessarily the tables. If it is also
>> extendable to other firmware interfaces like ACPI perhaps that would
>> be good.
>
> No need to apologize; I'll make those changes and resubmit for your
> review.
What about something like the following?
This patch makes both __earlycon_table and __earlycon_of_table
arrays of struct earlycon_id, and a follow-on patch would use the
earlycon name to initialize the struct console fields.
The benefits of this approach are
1. diagnostics can readily identify the earlycon if there is some error
2. it would be trivial to enable both command line and devicetree
earlycon from the same earlycon declaration.
And a single table is doable from this point.
AFAICT there is no benefit to using actual OF tables, and I see no
other reasonable way to initialize the name of the struct console
for devicetree earlycons.
Regards,
Peter Hurley
--- >% ---
Subject: [PATCH] serial: earlycon: Use common framework for earlycon
declarations
Use common table definition and implementation macro to declare an
earlycon, but retain separate tables for command line and devicetree.
Add __EARLYCON_DECLARE() macro to instance a unique earlycon
declaration for the specified table.
This enables all earlycons to properly initialize the earlycon console
structure with name and index.
Signed-off-by: Peter Hurley
---
drivers/of/fdt.c | 6 +++---
drivers/tty/serial/earlycon.c | 3 +--
include/asm-generic/vmlinux.lds.h | 8 ++--
include/linux/serial_core.h | 30 +-
4 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 7cef9f9..f640efa 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -760,14 +760,14 @@ static inline void
early_init_dt_check_for_initrd(unsigned long node)
#endif /* CONFIG_BLK_DEV_INITRD */
#ifdef CONFIG_SERIAL_EARLYCON
-extern struct of_device_id __earlycon_of_table[];
+extern struct earlycon_id __earlycon_of_table[];
static int __init early_init_dt_scan_chosen_serial(void)
{
int offset;
const char *p, *q;
int l;
- const struct of_device_id *match = __earlycon_of_table;
+ const struct earlycon_id *match = __earlycon_of_table;
const void *fdt = initial_boot_params;
offset = fdt_path_offset(fdt, "/chosen");
@@ -800,7 +800,7 @@ static int __init early_init_dt_scan_chosen_serial(void)
if (!addr)
return -ENXIO;
- of_setup_earlycon(addr, match->data);
+ of_setup_earlycon(addr, match->setup);
return 0;
}
return -ENODEV;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 5fdc9f3..bf7eb76 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -19,7 +19,6 @@
#include
#include
#include
-#include
#ifdef CONFIG_FIX_EARLYCON_MEM
#include
@@ -41,7 +40,7 @@ extern struct earlycon_id __earlycon_table[];
static const struct earlycon_id __earlycon_table_sentinel
__used __section(__earlycon_table_end);
-static const struct of_device_id __earlycon_of_table_sentinel
+static const struct earlycon_id __earlycon_of_table_sentinel
__used __section(__earlycon_of_table_end);
static void __iomem * __init earlycon_map(unsigned long paddr, size_t size)
diff --git a/include/asm-generic/vmlinux.lds.h
b/include/asm-generic/vmlinux.lds.h
index 561daf4..7322c30 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -155,8 +155,13 @@
VMLINUX_SYMBOL(__earlycon_table) = .; \
*(__earlycon_table)\
*(__earlycon_table_end)
+#define EARLYCON_OF_TABLE()STRUCT_ALIGN(); \
+ VMLINUX_SYMBOL(__earlycon_of_table) = .; \
+ *(__earlycon_of_table) \
+ *(__earlycon_of_table_end)
#else
#define EARLYCON_TABLE()
+#define EARLYCON_OF_TABLE()
#endif
#define ___OF_TABLE(cfg, name) _OF_TABLE_##cfg(name)
@@ -175,7 +180,6 @@
#define IOMMU_OF_TABLES() OF_TABLE(CONFIG_OF_IOMMU, iommu)
#define RESERVEDMEM_OF_TABLES()OF_TABLE(CONFIG_OF_RESERVED_MEM,
reservedmem)
#define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
-#define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
#define KERNEL_DTB() \
STRUCT_ALIGN(); \
@@ -512,7 +516,7 @@
KERNEL_DTB()
Re: [PATCH] libfdt: Add fdt_path_offset_namelen()
On 03/13/2015 01:43 AM, David Gibson wrote:
> On Tue, Mar 10, 2015 at 09:47:44AM -0400, Peter Hurley wrote:
>> Hi David,
>>
>> On 03/09/2015 08:17 PM, David Gibson wrote:
>>> On Fri, Mar 06, 2015 at 10:12:38AM -0500, Peter Hurley wrote:
>>>> Properties may contain path names which are not NUL-terminated.
>>>> For example, the 'stdout-path' property allows the form 'path:options',
>>>> where the ':' character terminates the path specifier.
>>>>
>>>> Allow these path names to be used in-place for path descending;
>>>> add fdt_path_offset_namelen(), which limits the path name to 'namelen'
>>>> characters.
>>>>
>>>> Reimplement fdt_path_offset() as a trivial wrapper.
>>>>
>>>> Signed-off-by: Peter Hurley
>>>
>>> I think this function is a good idea, however I would like to see a
>>> testcase for it.
>>
>> Sure, I can do that.
>>
>> I assume you mean a path name with non-NUL termination because
>> the fdt_path_offset() tests are already exercising the
>> fdt_path_offset_name() implementation.
>
> Yes, I mean the non-\0-terminated case. Or more specifically still,
> making sure that if you call fdt_path_offset_namelen() on a portion of
> a longer path, it correctly gives you the offset for only the partial
> path.
>
> That said, there may be some other edge cases that could do with
> testing too, if you have time. In particular I'm thinking of paths
> where there are repeated '/' character, and paths ending with one or
> more '/' characters.
>
>> Is there a readme somewhere regarding the test matrix (ie.,
>> which dts files go with which tests)?
>
> I'm afraid not, apart from the test runner script itself. I'm not
> sure quite what information you're after here.
Ok, now that I have the test working, I see why you didn't understand
what I was after.
The purpose of fdt_path_offset_namelen() is to perform path-descending
from in-place property values; for example, finding the node offset from
the /chosen/stdout-path property of the form
/ {
chosen {
stdout-path = "/ocp/serial@44e04d00:115200";
};
}
I wrote the test to get string properties containing paths from the
fdt; for example, assuming the fdt contains
path1 = "/subnode@1/subsubnode:";
path2 = "/subnode@2/subsubnode@0:";
path3 = "/subnode@1subsubnode///:";
path4 = "/subnode@2///subsubnode///:/subnode@1";
the test did:
const char *path1;
path1 = fdt_getprop(fdt, 0, "path1", &len);
...
I had looked at the run_test.sh scripts and known that there were
multiple dtb files produced for the test_tree1 tests so I was asking
for a handy readme to tell me which dts(i) files to add the string
properties to.
When I got the test running for dtbs produced from test_tree1.body.dtsi,
I discovered there were some cross-referenced tests from other
sources, like sw_tree1.c; thus the changes to test_tree1.body.dtsi
cause other tests to fail.
So I've given up on testing fdt_path_offset_namelen() that way,
and instead, am just using static const paths defined in the test
itself, which I will submit shortly.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 10/15] serial: stm32-usart: Add STM32 USART Driver
On 04/03/2015 01:01 PM, Maxime Coquelin wrote: > This drivers adds support to the STM32 USART controller, which is a > standard serial driver. Reviewed-by: Peter Hurley PS - I saw Rob's comment about 'hw-flow-ctrl' vs 'auto-flow-control' and I'm ok with either so feel free to and my review comment to a v6 if that's the only change. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
On 03/28/2015 03:28 PM, Kevin Cernekee wrote: > Side note: > > AFAIK we still have a problem if somebody wants to build serial8250 + > (any other tty driver that occupies major 4 / minor 64) into the same > kernel, and use DT to pick the correct driver at runtime. Yep, exactly. > serial8250_init() starts registering ports before it knows whether the > system even has an 8250. This behavior is required to support hw configuration from userspace. > I talked to Rob about it earlier this week > and he suggested that you might have some thoughts on how to fix it. A big piece of that will land in 4.01 (once I get the actual 8250 split commit to Greg). The 8250 driver has been split into a legacy/universal 8250 driver and separate port ops module; one of the benefits of this is that the objectionable legacy behavior of the 8250 driver (especially limitations wrt ttyS sharing) can be left behind in that driver without breaking existing users. The idea is that an alternate 8250 driver(s) with none of the legacy baggage can be taught to share (4,64) ports with other drivers. Unfortunately, I haven't made further progress with because of other projects. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
Hi Kevin, On 03/28/2015 03:28 PM, Kevin Cernekee wrote: > On Sat, Mar 28, 2015 at 10:01 AM, Peter Hurley > wrote: >>>> I know these got ACKs already but as you point out in the commit log, >>>> earlycon _will_ need reg-io-width, reg-offset and reg-shift. Since the >>>> distinction between early_init_dt_scan_chosen_serial() and >>>> of_setup_earlycon() is arbitrary, I'd rather see of_setup_earlycon() >>>> taught to properly decode of_serial driver bindings instead of a >>>> stack of parameters to of_setup_earlycon(). >>>> >>>> In fact, this patch allows a mis-defined devicetree to bring up a >>>> functioning earlycon because the 'big-endian' property is directly >>>> associated with UPIO_MEM32BE, which will create incompatibility problems >>>> when DT earlycon is fixed to decode the of_serial DT bindings. >>> >>> That's a good point. This hasn't been merged yet, so there isn't any >>> impact on addressing this. I would propose that for consistency, the >>> earlycon code should always default to 8-bit access. if big-endian >>> accesses are required, then reg-io-width + big-endian must be specified. >>> >>> Something like the following would do it and would be future-proof. We >>> can add support for 16 or 64bit big or little endian access if it ever >>> became necessary. >> >> I was planning on adding MEM32BE support to OF earlycon on top of my >> patch series 'OF earlycon cleanup', which adds full support for the >> of_serial driver DT properties (among other things). > > Hi Peter, > > This is my latest work-in-progress, incorporating the feedback from > you and Grant: > > https://github.com/cernekee/linux/commits/endian > > Not sure if this code plays nice with your recent cleanups? If we're > touching the same files/functions we should probably coordinate. Ok, I'll look over your git tree and add whatever's required to earlycon. > Also, it is untested, as I do not currently have access to BE systems. > If I get desperate I can try it on an LE system, adding the big-endian > properties in DT and then hacking the 8250 driver to swap LE accesses > for BE accesses. Ok. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
On 04/02/2015 09:46 AM, Rob Herring wrote: > Sorry about that. I had thought about doing the same thing. At least > unifying the macros, but not necessarily the tables. If it is also > extendable to other firmware interfaces like ACPI perhaps that would > be good. No need to apologize; I'll make those changes and resubmit for your review. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
On 04/02/2015 09:32 AM, Grant Likely wrote:
> On Sat, 28 Mar 2015 13:01:24 -0400
> , Peter Hurley
> wrote:
>> Hi Grant,
>>
>> On 03/27/2015 09:36 PM, Grant Likely wrote:
>>> On Sun, 01 Mar 2015 17:23:11 -0500
>>> , Peter Hurley
>>> wrote:
>>>> Hi Kevin,
>>>>
>>>> On 11/24/2014 06:36 PM, Kevin Cernekee wrote:
>>>>> If an earlycon (stdout-path) node is being used, check for "big-endian"
>>>>> or "native-endian" properties and pass the appropriate iotype to the
>>>>> driver.
>>>>>
>>>>> Note that LE sets UPIO_MEM (8-bit) but BE sets UPIO_MEM32BE (32-bit). The
>>>>> big-endian property only really makes sense in the context of 32-bit
>>>>> registers, since 8-bit accesses never require data swapping.
>>>>>
>>>>> At some point, the of_earlycon code may want to pass in the reg-io-width,
>>>>> reg-offset, and reg-shift parameters too.
>>>>>
>>>>> Signed-off-by: Kevin Cernekee
>>>>> ---
>>>>> drivers/of/fdt.c | 7 ++-
>>>>> drivers/tty/serial/earlycon.c | 4 ++--
>>>>> include/linux/serial_core.h | 2 +-
>>>>> 3 files changed, 9 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>> index 658656f..9d21472 100644
>>>>> --- a/drivers/of/fdt.c
>>>>> +++ b/drivers/of/fdt.c
>>>>> @@ -794,6 +794,8 @@ int __init early_init_dt_scan_chosen_serial(void)
>>>>>
>>>>> while (match->compatible[0]) {
>>>>> unsigned long addr;
>>>>> + unsigned char iotype = UPIO_MEM;
>>>>> +
>>>>> if (fdt_node_check_compatible(fdt, offset, match->compatible)) {
>>>>> match++;
>>>>> continue;
>>>>> @@ -803,7 +805,10 @@ int __init early_init_dt_scan_chosen_serial(void)
>>>>> if (!addr)
>>>>> return -ENXIO;
>>>>>
>>>>> - of_setup_earlycon(addr, match->data);
>>>>> + if (of_fdt_is_big_endian(fdt, offset))
>>>>> + iotype = UPIO_MEM32BE;
>>>>> +
>>>>> + of_setup_earlycon(addr, iotype, match->data);
>>>>
>>>> I know these got ACKs already but as you point out in the commit log,
>>>> earlycon _will_ need reg-io-width, reg-offset and reg-shift. Since the
>>>> distinction between early_init_dt_scan_chosen_serial() and
>>>> of_setup_earlycon() is arbitrary, I'd rather see of_setup_earlycon()
>>>> taught to properly decode of_serial driver bindings instead of a
>>>> stack of parameters to of_setup_earlycon().
>>>>
>>>> In fact, this patch allows a mis-defined devicetree to bring up a
>>>> functioning earlycon because the 'big-endian' property is directly
>>>> associated with UPIO_MEM32BE, which will create incompatibility problems
>>>> when DT earlycon is fixed to decode the of_serial DT bindings.
>>>
>>> That's a good point. This hasn't been merged yet, so there isn't any
>>> impact on addressing this. I would propose that for consistency, the
>>> earlycon code should always default to 8-bit access. if big-endian
>>> accesses are required, then reg-io-width + big-endian must be specified.
>>>
>>> Something like the following would do it and would be future-proof. We
>>> can add support for 16 or 64bit big or little endian access if it ever
>>> became necessary.
>>
>> I was planning on adding MEM32BE support to OF earlycon on top of my
>> patch series 'OF earlycon cleanup', which adds full support for the
>> of_serial driver DT properties (among other things).
>>
>> Unfortunately, that series is waiting on two things:
>> 1. libfdt upstream patch, which I submitted but was referred back to me
>> to add test cases. That was 3 weeks ago and I simply haven't had a free
>> day to burn to figure out how their test matrix is organized. I don't
>> think that's going to change anytime soon; I might just abandon that patch
>> and do the string manipulation on the stack.
>>
>> ATM, earlycon is still broken if stdout-path options have been set.
>
> I don't seem to have that patch. Can you send it
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
Hi Grant,
On 03/27/2015 09:36 PM, Grant Likely wrote:
> On Sun, 01 Mar 2015 17:23:11 -0500
> , Peter Hurley
> wrote:
>> Hi Kevin,
>>
>> On 11/24/2014 06:36 PM, Kevin Cernekee wrote:
>>> If an earlycon (stdout-path) node is being used, check for "big-endian"
>>> or "native-endian" properties and pass the appropriate iotype to the
>>> driver.
>>>
>>> Note that LE sets UPIO_MEM (8-bit) but BE sets UPIO_MEM32BE (32-bit). The
>>> big-endian property only really makes sense in the context of 32-bit
>>> registers, since 8-bit accesses never require data swapping.
>>>
>>> At some point, the of_earlycon code may want to pass in the reg-io-width,
>>> reg-offset, and reg-shift parameters too.
>>>
>>> Signed-off-by: Kevin Cernekee
>>> ---
>>> drivers/of/fdt.c | 7 ++-
>>> drivers/tty/serial/earlycon.c | 4 ++--
>>> include/linux/serial_core.h | 2 +-
>>> 3 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 658656f..9d21472 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -794,6 +794,8 @@ int __init early_init_dt_scan_chosen_serial(void)
>>>
>>> while (match->compatible[0]) {
>>> unsigned long addr;
>>> + unsigned char iotype = UPIO_MEM;
>>> +
>>> if (fdt_node_check_compatible(fdt, offset, match->compatible)) {
>>> match++;
>>> continue;
>>> @@ -803,7 +805,10 @@ int __init early_init_dt_scan_chosen_serial(void)
>>> if (!addr)
>>> return -ENXIO;
>>>
>>> - of_setup_earlycon(addr, match->data);
>>> + if (of_fdt_is_big_endian(fdt, offset))
>>> + iotype = UPIO_MEM32BE;
>>> +
>>> + of_setup_earlycon(addr, iotype, match->data);
>>
>> I know these got ACKs already but as you point out in the commit log,
>> earlycon _will_ need reg-io-width, reg-offset and reg-shift. Since the
>> distinction between early_init_dt_scan_chosen_serial() and
>> of_setup_earlycon() is arbitrary, I'd rather see of_setup_earlycon()
>> taught to properly decode of_serial driver bindings instead of a
>> stack of parameters to of_setup_earlycon().
>>
>> In fact, this patch allows a mis-defined devicetree to bring up a
>> functioning earlycon because the 'big-endian' property is directly
>> associated with UPIO_MEM32BE, which will create incompatibility problems
>> when DT earlycon is fixed to decode the of_serial DT bindings.
>
> That's a good point. This hasn't been merged yet, so there isn't any
> impact on addressing this. I would propose that for consistency, the
> earlycon code should always default to 8-bit access. if big-endian
> accesses are required, then reg-io-width + big-endian must be specified.
>
> Something like the following would do it and would be future-proof. We
> can add support for 16 or 64bit big or little endian access if it ever
> became necessary.
I was planning on adding MEM32BE support to OF earlycon on top of my
patch series 'OF earlycon cleanup', which adds full support for the
of_serial driver DT properties (among other things).
Unfortunately, that series is waiting on two things:
1. libfdt upstream patch, which I submitted but was referred back to me
to add test cases. That was 3 weeks ago and I simply haven't had a free
day to burn to figure out how their test matrix is organized. I don't
think that's going to change anytime soon; I might just abandon that patch
and do the string manipulation on the stack.
ATM, earlycon is still broken if stdout-path options have been set.
2. Rob never got back to me on my query [1] to unify the OF_EARLYCON_DECLARE
macro with the EARLYCON_DECLARE macro so that all earlycon consoles
are named.
Regards,
Peter Hurley
[1] https://lkml.org/lkml/2015/3/6/571
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/15] serial: stm32-usart: Add STM32 USART Driver
On 03/26/2015 06:03 PM, Maxime Coquelin wrote:
>>> +static void stm32_set_termios(struct uart_port *port, struct ktermios
>>> *termios,
>>> + struct ktermios *old)
>>> +{
>>> + unsigned int baud;
>>> + u32 usardiv, mantissa, fraction;
>>> + tcflag_t cflag;
>>> + u32 cr1, cr2, cr3;
>>> + unsigned long flags;
>>> +
>>> + baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk / 16);
>>> + cflag = termios->c_cflag;
>>> +
>>> + spin_lock_irqsave(&port->lock, flags);
>>> +
>>> + /* Stop serial port and reset value */
>>> + writel_relaxed(0, port->membase + USART_CR1);
>>> +
>>> + cr1 = USART_CR1_TE | USART_CR1_RE | USART_CR1_UE | USART_CR1_RXNEIE;
>>> +
>>> + if (cflag & CSTOPB)
>>> + cr2 = USART_CR2_STOP_2B;
>>> +
>>> + if (cflag & PARENB) {
>>> + cr1 |= USART_CR1_PCE;
>>> + if ((cflag & CSIZE) == CS8)
>>> + cr1 |= USART_CR1_M;
>>> + }
>>> +
>>> + if (cflag & PARODD)
>>> + cr1 |= USART_CR1_PS;
>>> +
>>> + if (cflag & CRTSCTS)
>>> + cr3 = USART_CR3_RTSE | USART_CR3_CTSE;
>>
>> If this means autoflow control, then you need to define
>> throttle()/unthrottle() methods, otherwise the serial core won't
>> be able to throttle the remote when input buffers are about
>> to overflow.
>>
>> And you should only enable the autoCTS and let the serial
>> core enable autoRTS through set_mctrl(TIOCM_RTS).
>>
>> Just let me know if you need more info about how to do this.
>
> Ok, let's see if I have well understood.
>
> USART_CR3_RTSE should be set/cleared in set_mctrl(), depending on
> TIOCM_RTS value.
> The throttle callback should disable the rx interrupt, and the
> unthrottle enable it.
> For CTS, it should be enabled in set_termios() if CRTSCTS, as done here.
>
> Am I right?
Yeah, basically. You also have to indicate to the serial core that you
require throttle/unthrottle handling in this mode by setting port->status.
Your set_termios() method would look like:
port->status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS);
if (cflag & CRTSCTS) {
port->status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
cr3 = USART_CR3_CTSE;
}
and your set_mctrl() method would look like:
if ((mctrl & TIOCM_RTS) && (port->status & UPSTAT_AUTORTS))
stm32_set_bits(port, USART_CR3, USART_CR3_RTSE);
else
stm32_clear_bits(port, USART_CR3, USART_CR3_RTSE);
The UPSTAT_AUTOCTS doesn't really do anything right now but please
use it anyway to indicate this driver has that functionality.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On 03/25/2015 05:17 PM, NeilBrown wrote: > On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley > wrote: > >> On 03/18/2015 01:58 AM, NeilBrown wrote: >> >>> + * A "tty-slave" is a device permanently attached to a particularly >>> + * tty, typically wired to a UART. >> >> Why "permanently"? >> Is that a limitation of the implementation or design? >> > > The slave is described in devicetree - that only happens for permanently > attached devices, doesn't it? > > I guess that with device-tree overlays and 'capes' for boards you could have > a device attached to the uart "for this power session" rather than > "permanently", but I think it is a rather subtle distinction. > > Did you have something else in mind? My primary concern is that the abstraction match the scope. If the abstraction is at the tty layer, then the scope of the design should support tty devices, not just hard-wired, devicetree-defined uarts. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] TTY: add support for tty_slave devices.
On 03/18/2015 01:58 AM, NeilBrown wrote: > + * A "tty-slave" is a device permanently attached to a particularly > + * tty, typically wired to a UART. Why "permanently"? Is that a limitation of the implementation or design? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] TTY: use class_find_device to find port in uart_suspend/resume.
Hi Neil,
On 03/18/2015 01:58 AM, NeilBrown wrote:
> uart_{suspend,resume}_port seach the children of a uart device
> to find a particular tty device.
> This requires all the ttys to be direct children of the uart.
>
> A future patch will allow a 'tty_slave' to intervene between
> the port and the uart, voiding this requirement.
>
> So change to use class_find_device. This is made possibly by
> exporting a "tty_find_device" from tty_io.c
Comments below.
> Signed-off-by: NeilBrown
> ---
> drivers/tty/serial/serial_core.c | 21 -
> drivers/tty/tty_io.c |6 ++
> include/linux/tty.h |1 +
> 3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c
> b/drivers/tty/serial/serial_core.c
> index 6a1055ae3437..7abb7474870a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1960,26 +1960,19 @@ struct uart_match {
> struct uart_driver *driver;
> };
>
> -static int serial_match_port(struct device *dev, void *data)
> -{
> - struct uart_match *match = data;
> - struct tty_driver *tty_drv = match->driver->tty_driver;
> - dev_t devt = MKDEV(tty_drv->major, tty_drv->minor_start) +
> - match->port->line;
> -
> - return dev->devt == devt; /* Actually, only one tty per port */
> -}
>
> int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
> {
> struct uart_state *state = drv->state + uport->line;
> struct tty_port *port = &state->port;
> struct device *tty_dev;
> - struct uart_match match = {uport, drv};
> + dev_t devt = MKDEV(drv->tty_driver->major,
> +drv->tty_driver->minor_start) +
> + uport->line;
>
> mutex_lock(&port->mutex);
>
> - tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> + tty_dev = tty_find_device(devt);
> if (device_may_wakeup(tty_dev)) {
> if (!enable_irq_wake(uport->irq))
> uport->irq_wake = 1;
> @@ -2039,12 +2032,14 @@ int uart_resume_port(struct uart_driver *drv, struct
> uart_port *uport)
> struct uart_state *state = drv->state + uport->line;
> struct tty_port *port = &state->port;
> struct device *tty_dev;
> - struct uart_match match = {uport, drv};
> struct ktermios termios;
> + dev_t devt = MKDEV(drv->tty_driver->major,
> +drv->tty_driver->minor_start) +
> + uport->line;
>
> mutex_lock(&port->mutex);
>
> - tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> + tty_dev = tty_find_device(devt);
> if (!uport->suspended && device_may_wakeup(tty_dev)) {
> if (uport->irq_wake) {
> disable_irq_wake(uport->irq);
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 51f066aa375e..27632ad17d6f 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3077,6 +3077,12 @@ static struct device *tty_get_device(struct tty_struct
> *tty)
> return class_find_device(tty_class, NULL, &devt, dev_match_devt);
> }
>
> +struct device *tty_find_device(dev_t devt)
> +{
> + return class_find_device(tty_class, NULL, &devt, dev_match_devt);
> +}
> +EXPORT_SYMBOL(tty_find_device);
> +
Would you please replace tty_get_device() usage with tty_find_device()
(and keep the function comment from tty_get_device())?
Regards,
Peter Hurley
>
> /**
> * alloc_tty_struct
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 358a337af598..04d5f1213700 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -461,6 +461,7 @@ extern void tty_vhangup(struct tty_struct *tty);
> extern int tty_hung_up_p(struct file *filp);
> extern void do_SAK(struct tty_struct *tty);
> extern void __do_SAK(struct tty_struct *tty);
> +extern struct device *tty_find_device(dev_t devt);
> extern void no_tty(void);
> extern void tty_flush_to_ldisc(struct tty_struct *tty);
> extern void tty_buffer_free_all(struct tty_port *port);
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/15] serial: stm32-usart: Add STM32 USART Driver
gt; + * driver gets probed and the port should be mapped at that point.
> + */
> + if (stm32port->port.mapbase == 0 || stm32port->port.membase == NULL)
> + return -ENXIO;
> +
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(&stm32port->port, co, baud, parity, bits, flow);
> +}
> +
> +static struct console stm32_console = {
> + .name = STM32_SERIAL_NAME,
> + .device = uart_console_device,
> + .write = stm32_console_write,
> + .setup = stm32_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> + .data = &stm32_usart_driver,
> +};
> +
> +#define STM32_SERIAL_CONSOLE (&stm32_console)
> +
> +#else
> +#define STM32_SERIAL_CONSOLE NULL
> +#endif /* CONFIG_SERIAL_STM32_CONSOLE */
> +
> +static struct uart_driver stm32_usart_driver = {
> + .owner = THIS_MODULE,
> + .driver_name= DRIVER_NAME,
> + .dev_name = STM32_SERIAL_NAME,
> + .major = 0,
> + .minor = 0,
> + .nr = STM32_MAX_PORTS,
> + .cons = STM32_SERIAL_CONSOLE,
> +};
> +
> +static struct platform_driver stm32_serial_driver = {
> + .probe = stm32_serial_probe,
> + .remove = stm32_serial_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(stm32_match),
> + },
> +};
> +
> +static int __init usart_init(void)
> +{
> + int ret;
> + static char banner[] __initdata =
> + KERN_INFO "STM32 USART driver initialized\n";
> +
> + printk(banner);
> +
> + ret = uart_register_driver(&stm32_usart_driver);
> + if (ret)
> + return ret;
> +
> + ret = platform_driver_register(&stm32_serial_driver);
> + if (ret)
> + uart_unregister_driver(&stm32_usart_driver);
> +
> + return ret;
> +}
> +
> +static void __exit usart_exit(void)
> +{
> + platform_driver_unregister(&stm32_serial_driver);
> + uart_unregister_driver(&stm32_usart_driver);
> +}
> +
> +module_init(usart_init);
> +module_exit(usart_exit);
> +
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_DESCRIPTION("STMicroelectronics STM32 serial port driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/serial_core.h
> b/include/uapi/linux/serial_core.h
> index b212281..e22dee5 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -258,4 +258,7 @@
> /* Cris v10 / v32 SoC */
> #define PORT_CRIS112
>
> +/* STM32 USART */
> +#define PORT_STM32 110
Already taken.
You'll want to make sure v4 applies cleanly to Greg's tty-next branch.
Regards,
Peter Hurley
> +
> #endif /* _UAPILINUX_SERIAL_CORE_H */
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/15] serial: stm32-usart: Add STM32 USART Driver
Hi Maxime,
On 03/24/2015 01:21 PM, Maxime Coquelin wrote:
> Hi Peter,
>
> 2015-03-19 18:35 GMT+01:00 Maxime Coquelin :
>> 2015-03-19 15:58 GMT+01:00 Peter Hurley :
>>> On 03/19/2015 09:55 AM, Maxime Coquelin wrote:
>>>>>>>> +static void stm32_set_termios(struct uart_port *port, struct ktermios
>>>>>>>> *termios,
>>>>>>>> + struct ktermios *old)
>>> [...]
>>>>>>>> + usardiv = (port->uartclk * 25) / (baud * 4);
>>>>>>>> + mantissa = (usardiv / 100) << USART_BRR_DIV_M_SHIFT;
>>>>>>>> + fraction = DIV_ROUND_CLOSEST((usardiv % 100) * 16, 100);
>>>>>>>> + if (fraction & ~USART_BRR_DIV_F_MASK) {
>>>>>>>> + fraction = 0;
>>>>>>>> + mantissa += (1 << USART_BRR_DIV_M_SHIFT);
>>>>>>>> + }
>>> [...]
>>>> Really, I would prefer keeping this fractional divider as it is
>>>> implemented today.
>>>
>>> You have to admit that's basically an unintelligible mess;
>>> how would anyone ever be able to refactor and replace that with a
>>> common divider implementation?
>>>
>>> At the very least, please comment on the formula and format.
>>
>> Ok, I will refactor the implementation, and comment it.
>
> The implementation was indeed a mess.
> I found some time to refactor the code, and also added support for 8
> times oversampling (16 by default).
> It will allow to achieve higher speeds, with the side effect of being
> less tolerant to clock deviations.
>
> What do you think about the code below?
>
>
> usartdiv = DIV_ROUND_CLOSEST(port->uartclk, baud);
>
> /*
> * The USART supports 16 or 8 times oversampling.
> * By default we prefer 16 times oversampling, so that the receiver
> * has a better tolerance to clock deviations.
> * 8 times oversampling is only used to achieve higher speeds.
> */
> if (usartdiv < 16) {
> oversampling = 8;
> stm32_set_bits(port, USART_CR1, USART_CR1_OVER8);
> } else {
> oversampling = 16;
> stm32_clr_bits(port, USART_CR1, USART_CR1_OVER8);
> }
>
> mantissa = (usartdiv / oversampling) << USART_BRR_DIV_M_SHIFT;
> fraction = usartdiv % oversampling;
> writel_relaxed(mantissa | fraction, port->membase + USART_BRR)
Thanks! Way better :)
Much more obvious this is a fixed-point divisor.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/15] serial: stm32-usart: Add STM32 USART Driver
On 03/19/2015 09:55 AM, Maxime Coquelin wrote:
>>>>> +static void stm32_set_termios(struct uart_port *port, struct ktermios
>>>>> *termios,
>>>>> + struct ktermios *old)
[...]
>>>>> + usardiv = (port->uartclk * 25) / (baud * 4);
>>>>> + mantissa = (usardiv / 100) << USART_BRR_DIV_M_SHIFT;
>>>>> + fraction = DIV_ROUND_CLOSEST((usardiv % 100) * 16, 100);
>>>>> + if (fraction & ~USART_BRR_DIV_F_MASK) {
>>>>> + fraction = 0;
>>>>> + mantissa += (1 << USART_BRR_DIV_M_SHIFT);
>>>>> + }
[...]
> Really, I would prefer keeping this fractional divider as it is
> implemented today.
You have to admit that's basically an unintelligible mess;
how would anyone ever be able to refactor and replace that with a
common divider implementation?
At the very least, please comment on the formula and format.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/18/2015 06:46 PM, Jon Masters wrote: > Just a sidenote, that this is a data structure, not code. There are > plenty of other specifications and standards that the kernel uses that > are licensed under a variety of terms. The specific issue seems to be > the potential for conflict over the patent language in that document as > possibly pertaining to implementations, not the specific license of the > document per se. I'll ping a few and get a conversation going. Strictly speaking, only the patent claim(s) are relevant. Either, a. your code infringes on the claim(s), in which case, the specific license from Microsoft is required, or b. your code does not infringe, in which case, no license is necessary. In case (a), all of my previous email applies; ie., the terms of the license should be GPL v2 compatible. In case (b), there is no actual problem. Yes, the patent notice in the specification is a barrier, and it would be helpful to your cause if Microsoft cited the patent numbers, but you could also just explain these in your cover letter at submission time. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/17/2015 08:13 PM, Jon Masters wrote: > Hi Peter, > > On 03/17/2015 01:47 PM, Peter Hurley wrote: > >> On 03/17/2015 12:48 PM, Jon Masters wrote: >>> On 03/16/2015 03:46 PM, Peter Hurley wrote: >>>> On 03/16/2015 02:35 PM, Hans de Goede wrote: >>>>> To be clear about my aarch64 remark, that relates to the behavior of >>>>> aarch64 acpi using >>>>> machines, those will also output to both a serial tty and tty0 when the >>>>> acpi equivalent >>>>> of stdout-path is present and points to a serial tty. >>>> >>>> I already made comments addressing the unsuitability of the license for the >>>> aarch64 acpi console; >>> >>> Yes, you did. However, I believe you might have outdated information. >>> Have you read the SPCR in the past few months, or are you looking at a >>> version prior to update that was made in October of 2014? >> >> The version of the Serial Port Console Redirection Table specification >> I was referring to is downloadable here: >> >> https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132%28v=vs.85%29.aspx >> >> That page says Last Updated: October 21, 2014 > > Ok. You've got the most recent version :) Let me predicate the following > with the assertion that I am not a lawyer and I am not offering legal > advice. But I have worked with Microsoft (and many other large hardware > and software vendors) for some time on topics related to standardization > of ARM and I am offering to reach back out to them to resolve any > legitimate concerns arising here. The thing we need to ascertain is > whether there is a legitimate concern. > >>> Can you be specific about your concerns? The license has already been >>> changed once (I instigated the request that lead to that change to drop >>> several pages of terse terms that used to cover the first few pages). I >>> have found the Microsoft team extremely responsive and amenable to >>> resolving issues, so if you would do us the service of articulating what >>> the concern is, I'll reach back out and get that addressed. I have a >>> direct line into their server and legal teams to discuss this issue. >> >> Well, I'm deducing somewhat here because the code that would use the >> SPCR table format has not been submitted. So I don't _know_ what license(s) >> you intend to submit with. > > Fair enough. I wrote an initial quick and dirty implementation of SPCR > parsing which I handed off to a colleague to cleanup. They are indeed > planning such a submission. So such an implementation does in fact exist > and there is an intent to submit. I assumed that because I received no reply to my first email that this effort had been abandoned, so I didn't bother to raise the technical issues that plague the devicetree hack. > Ultimately, ACPI based ARM systems > (and even x86 ones) will be better off for supporting SPCR. While not > required, it does obviate the need for both "console=" and "earlycon=" > kernel command line parameters, and thus improves end user experience by > making console setup automatic. Sure; there are several platforms/arches that already do this from proms. > We decided to include SPCR in the SBBR > rather than writing a new table on the understanding that writing a new > table was otherwise the default, and a wasted effort when such a > specification already existed. The concern you raise was anticipated, > and the changes mentioned were already made. I'm sure we'll be able to > ask for additional clarification and changes also. > >> So if you and Microsoft have worked out some deal where Microsoft has >> licensed the SPCR spec to you under GPL v2 terms, then, great! there is no >> problem. > > I am willing to assist in brokering a discussion on this. But first we > should ascertain what is necessary here and articulate that succinctly. The kernel source is licensed under GPL v2 (see ./COPYING), so contributions are expected to be licensed under compatible terms. Patent retaliation provisions are not compatible with GPL v2, because those provisions assert limitations that don't exist in GPL v2. For example, the Apache 2.0 license and the original Eclipse 1.0 license both have patent retaliation provisions that make them incompatible with GPL v2. The Free Software Foundation maintains a list of free and non-free software licenses at http://www.gnu.org/philosophy/license-list.html with commentary on each regarding their compatibility and provisions. FSF also maintains a licensing and compliance lab which you can contact at licens...@fsf.org
[PATCH] Revert "of: Fix premature bootconsole disable with 'stdout-path'"
This reverts commit 2fa645cb2703d9b3786d850db815414dfeefa51d.
The assumption that at least 1 preferred console will be registered
when the stdout-path property is set is invalid, which can result
in _no_ consoles.
Signed-off-by: Peter Hurley
---
drivers/of/base.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index adb8764..8b904e5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1893,10 +1893,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
align))
name = of_get_property(of_chosen, "linux,stdout-path",
NULL);
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
- if (name) {
+ if (name)
of_stdout = of_find_node_opts_by_path(name,
&of_stdout_options);
- add_preferred_console("stdout-path", 0, NULL);
- }
}
if (!of_aliases)
--
2.3.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/17/2015 10:20 AM, Peter Hurley wrote: > On 03/17/2015 09:43 AM, Hans de Goede wrote: >> Hi, >> >> On 17-03-15 14:30, Rob Herring wrote: >>> On Tue, Mar 17, 2015 at 3:20 AM, Hans de Goede wrote: >> >> >> >>>> TBH I do not understand why we're even arguing here, AFAICT the behavior >>>> change >>>> is an unwanted side-effect of your patch, so the solution is to rewrite the >>>> patch >>>> so that we get the same end result (not turning off bootconsole-s too >>>> early) >>>> without >>>> the unwanted side-effect, and you agreed to work on that ? >>> >>> I intend to revert this if we don't have a fix soon. >>> >>> I think we just need a flag saying we've enabled the earlycon from >>> stdout-path or not and then add the preferred console based on that. I >>> assume with "earlycon" only on the command-line, getting console only >>> on stdout-path is okay. >> >> Yes, if a user explicitly specifies something like "earlycon" on the >> commandline then not automatically getting console output on tty0 is >> fine AFAICT. The use case important for me / distros is when no >> console= (or related) arguments are present on the cmdline at all, >> then the desired behavior is to have console output on tty0 as well >> as on any serial console specified with stdout-path. > > The issues raised by this patch have nothing to do with earlycon. > > 1. PowerPC boot crash - the report with the most troubleshooting info right > now >implicates some buffer overflow or console mismanagement triggered by > simply >having defined a preferred console. This needs to be figured out > regardless, >and this is what I'm working right now. > > 2. Hans' use-case was _already broken_ even before this patch; _any_ driver >that adds a preferred console before the vt console driver will cause >this problem. So again, this needs to be fixed regardless. Rob, You're right; this patch will need to be reverted. I'll send you a revert. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/17/2015 03:44 PM, Peter Hurley wrote: > On 03/17/2015 03:35 PM, Andreas Schwab wrote: >> Peter Hurley writes: >> >>> It doesn't boot? >> >> It boots right into user space, but the initrd doesn't like something >> (perhaps the missing console) and exits. Note that the framebuffer >> console does work, but all I see are the penguins. >> >> [6.235604] Warning: unable to open an initial console. > > Thanks, Andreas. I had figured out from your earlier email that your > specific breakage was the absence of any console. Would you share what the actual prom stdout string value is? (linux,stdout-path is equivalent) Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/17/2015 03:35 PM, Andreas Schwab wrote: > Peter Hurley writes: > >> It doesn't boot? > > It boots right into user space, but the initrd doesn't like something > (perhaps the missing console) and exits. Note that the framebuffer > console does work, but all I see are the penguins. > > [6.235604] Warning: unable to open an initial console. Thanks, Andreas. I had figured out from your earlier email that your specific breakage was the absence of any console. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
Hi Jon, On 03/17/2015 12:48 PM, Jon Masters wrote: > On 03/16/2015 03:46 PM, Peter Hurley wrote: >> On 03/16/2015 02:35 PM, Hans de Goede wrote: >>> To be clear about my aarch64 remark, that relates to the behavior of >>> aarch64 acpi using >>> machines, those will also output to both a serial tty and tty0 when the >>> acpi equivalent >>> of stdout-path is present and points to a serial tty. >> >> I already made comments addressing the unsuitability of the license for the >> aarch64 acpi console; > > Yes, you did. However, I believe you might have outdated information. > Have you read the SPCR in the past few months, or are you looking at a > version prior to update that was made in October of 2014? The version of the Serial Port Console Redirection Table specification I was referring to is downloadable here: https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132%28v=vs.85%29.aspx That page says Last Updated: October 21, 2014 The cover page of that downloaded specification has this text: " Patent Notice. Microsoft provides you certain patent rights for implementations of this specification under the terms of Microsoft’s Community Promise, available at http://www.microsoft.com/openspecifications/en/us/programs/community-promise/default.aspx. Version 1.02 — October 9, 2014 " >> the proposed SPCR table format is patented by Microsoft and >> licensed incompatibly with GPLv2. > > Can you be specific about your concerns? The license has already been > changed once (I instigated the request that lead to that change to drop > several pages of terse terms that used to cover the first few pages). I > have found the Microsoft team extremely responsive and amenable to > resolving issues, so if you would do us the service of articulating what > the concern is, I'll reach back out and get that addressed. I have a > direct line into their server and legal teams to discuss this issue. Well, I'm deducing somewhat here because the code that would use the SPCR table format has not been submitted. So I don't _know_ what license(s) you intend to submit with. But assuming you're using some part of the SPCR specification to implement the aarch64 acpi console, then my concern stems from the incompatibility between GPL v2 and the Microsoft Community Promise license. In order for you to submit code to mainline, you (or rather, the submitter) must certify that you have the legal right to do so. That's spelled out in Documentation/SubmittingPatches, under 'Developer's Certficate of Origin 1.1'. So if you and Microsoft have worked out some deal where Microsoft has licensed the SPCR spec to you under GPL v2 terms, then, great! there is no problem. However, if Microsoft is licensing your use of the SPCR specification under the Microsoft Community Promise and you intend license your submission with the same, then the problem is that the Microsoft Community Promise License asserts additional limitations which are not compatible with GPL v2; specifically, this text: [from https://msdn.microsoft.com/en-us/openspecifications/dn646766 ] "If you file, maintain, or voluntarily participate in a patent infringement lawsuit against a Microsoft implementation of any Covered Specification, then this personal promise does not apply with respect to any Covered Implementation made or used by you." IOW, if I sue Microsoft for patent infringement ste,ming from their use of Microsoft XML Document Object Model Level 1 Standards Support Document (which infringes on patents I own), then my embedded aarch64 kernel - which merely contains your implementation - is no longer covered under the terms of the license. Now, that's just my interpretation of it; maybe the Linux Foundation's lawyers would see it differently. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/17/2015 09:43 AM, Hans de Goede wrote: > Hi, > > On 17-03-15 14:30, Rob Herring wrote: >> On Tue, Mar 17, 2015 at 3:20 AM, Hans de Goede wrote: > > > >>> TBH I do not understand why we're even arguing here, AFAICT the behavior >>> change >>> is an unwanted side-effect of your patch, so the solution is to rewrite the >>> patch >>> so that we get the same end result (not turning off bootconsole-s too early) >>> without >>> the unwanted side-effect, and you agreed to work on that ? >> >> I intend to revert this if we don't have a fix soon. >> >> I think we just need a flag saying we've enabled the earlycon from >> stdout-path or not and then add the preferred console based on that. I >> assume with "earlycon" only on the command-line, getting console only >> on stdout-path is okay. > > Yes, if a user explicitly specifies something like "earlycon" on the > commandline then not automatically getting console output on tty0 is > fine AFAICT. The use case important for me / distros is when no > console= (or related) arguments are present on the cmdline at all, > then the desired behavior is to have console output on tty0 as well > as on any serial console specified with stdout-path. The issues raised by this patch have nothing to do with earlycon. 1. PowerPC boot crash - the report with the most troubleshooting info right now implicates some buffer overflow or console mismanagement triggered by simply having defined a preferred console. This needs to be figured out regardless, and this is what I'm working right now. 2. Hans' use-case was _already broken_ even before this patch; _any_ driver that adds a preferred console before the vt console driver will cause this problem. So again, this needs to be fixed regardless. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/16/2015 08:35 PM, Andreas Schwab wrote: > Peter Hurley writes: > >> On 03/16/2015 08:19 PM, Andreas Schwab wrote: >>> Peter Hurley writes: >>> >>>> I don't see this as a regression, but rather a misconfiguration. >>> >>> It breaks booting on PowerMac. >> >> It doesn't boot? > > Yes. Ok, thanks for the report. Can you attach 1) your .dts 2) your .config 3) complete dmesg from 4.0-rc4 boot with that patch reverted? Do you have a serial console hooked up? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/16/2015 08:19 PM, Andreas Schwab wrote: > Peter Hurley writes: > >> I don't see this as a regression, but rather a misconfiguration. > > It breaks booting on PowerMac. It doesn't boot? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/16/2015 02:35 PM, Hans de Goede wrote:
> Hi,
>
> On 16-03-15 19:23, Peter Hurley wrote:
>> On 03/16/2015 02:12 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 16-03-15 18:49, Peter Hurley wrote:
>>>> Hi Hans,
>>>>
>>>> On 03/16/2015 12:31 PM, Hans de Goede wrote:
>>>>> Hi All,
>>>>>
>>>>> While updating my local working tree to 4.0-rc4 this morning I noticed
>>>>> that I no longer
>>>>> get console / kernel messages output on the hdmi output of my ARM board /
>>>>> on tty0
>>>>>
>>>>> This is caused by:
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/of?id=2fa645cb2703d9b3786d850db815414dfeefa51d
>>>>>
>>>>> Reverting this commit fixes this for me.
>>>>>
>>>>> What is happening here is that the "add_preferred_console("stdout-path",
>>>>> 0, NULL);"
>>>>> happens before the tty0 registers stopping tty0 from becoming part of the
>>>>> console list
>>>>> since there already is a preferred console at that time.
>>>>>
>>>>> This is an undesirable behavior change caused by the commit in question,
>>>>> on boards
>>>>> where there is both video output, and a serial console configured through
>>>>> stdout-path
>>>>> we want to have console output on both as we do not know which of the 2
>>>>> will actually
>>>>> be hooked up by the user.
>>>>
>>>> I don't see this as a regression, but rather a misconfiguration.
>>>
>>> As said it is an undesirable behavior change, whether you want to call that
>>> a regression
>>> or not is not all that interesting. Fixing it however is important, as e.g.
>>> Fedora
>>> ARM images rely on this behavior, both "regular" arm as well as aarch64.
>>
>> What dts file is causing this problem?
>> Is it in mainline or distributed only in Fedora?
>
> This is on allwinner (sunxi) devices such as Allwinner A10 or A20 based
> boards, currently
> the setting of stdout-path on these boards is done by (an unmodified)
> upstream u-boot.
You forgot to mention my patch is not very likely to break anything
in the wild since _you_ upstreamed the dependency only 3 weeks ago [1].
And what "Fedora ARM images rely on this behavior"?
I don't appreciate the deception.
Regards,
Peter Hurley
[1] git log from u-boot repo
commit f3133962f469a8b6b9ba237ba670f0ca7c88a02e
Author: Hans de Goede
Date: Fri Feb 20 16:55:12 2015 +0100
sunxi: Set the /chosen/stdout-path fdt property for sunxi boards
While discussing with some people how to get the Linux kernel to do the
right thing wrt sending output to both the serial console and the
hdmi out / lcd screen when booting on ARM devices, Grant Likely pointed out
that there already is a solution for this.
All we need to do is set the /chosen/stdout-path fdt property, and if no
console= arguments were specified on the kernel commandline the kernel
will honor this and add this device as a console (next to the primary
video output on hdmi).
And u-boot already has support for setting this, all we need to do is
define OF_STDOUT_PATH and then everything will just work ootb, without
people needing to meddle with adding console= arguments in extlinux.conf .
Signed-off-by: Hans de Goede
Acked-by: Ian Campbell
Reviewed-by: Tom Rini
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index f5efebb..0b4f0a0 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -210,6 +210,20 @@ extern int soft_i2c_gpio_scl;
#define CONFIG_CONS_INDEX 1 /* UART0 */
#endif
+#if CONFIG_CONS_INDEX == 1
+#ifdef CONFIG_MACH_SUN9I
+#define OF_STDOUT_PATH "/soc/serial@0700:115200"
+#else
+#define OF_STDOUT_PATH "/soc@01c0/serial@01c28000:115200"
+#endif
+#elif CONFIG_CONS_INDEX == 2 && defined(CONFIG_MACH_SUN5I)
+#define OF_STDOUT_PATH "/soc@01c0/serial@01c28400:115200"
+#elif CONFIG_CONS_INDEX == 5 && defined(CONFIG_MACH_SUN8I)
+#define OF_STDOUT_PATH "/soc@01c0/serial@01f02800:115200"
+#else
+#error Unsupported console port nr. Please fix stdout-path in sunxi-common.h.
+#endif
+
/* GPIO */
#define CONFIG_SUNXI_GPIO
#define CONFIG_SPL_GPIO_SUPPORT
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/16/2015 02:35 PM, Hans de Goede wrote: > To be clear about my aarch64 remark, that relates to the behavior of aarch64 > acpi using > machines, those will also output to both a serial tty and tty0 when the acpi > equivalent > of stdout-path is present and points to a serial tty. I already made comments addressing the unsuitability of the license for the aarch64 acpi console; the proposed SPCR table format is patented by Microsoft and licensed incompatibly with GPLv2. That said, the aarch64 acpi console should be treated like any other prom that starts a console and so won't suffer from this breakage. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
On 03/16/2015 02:12 PM, Hans de Goede wrote:
> Hi,
>
> On 16-03-15 18:49, Peter Hurley wrote:
>> Hi Hans,
>>
>> On 03/16/2015 12:31 PM, Hans de Goede wrote:
>>> Hi All,
>>>
>>> While updating my local working tree to 4.0-rc4 this morning I noticed that
>>> I no longer
>>> get console / kernel messages output on the hdmi output of my ARM board /
>>> on tty0
>>>
>>> This is caused by:
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/of?id=2fa645cb2703d9b3786d850db815414dfeefa51d
>>>
>>> Reverting this commit fixes this for me.
>>>
>>> What is happening here is that the "add_preferred_console("stdout-path", 0,
>>> NULL);"
>>> happens before the tty0 registers stopping tty0 from becoming part of the
>>> console list
>>> since there already is a preferred console at that time.
>>>
>>> This is an undesirable behavior change caused by the commit in question, on
>>> boards
>>> where there is both video output, and a serial console configured through
>>> stdout-path
>>> we want to have console output on both as we do not know which of the 2
>>> will actually
>>> be hooked up by the user.
>>
>> I don't see this as a regression, but rather a misconfiguration.
>
> As said it is an undesirable behavior change, whether you want to call that a
> regression
> or not is not all that interesting. Fixing it however is important, as e.g.
> Fedora
> ARM images rely on this behavior, both "regular" arm as well as aarch64.
What dts file is causing this problem?
Is it in mainline or distributed only in Fedora?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REGRESSION] "of: Fix premature bootconsole disable with 'stdout-path'" breaks console on tty0
Hi Hans,
On 03/16/2015 12:31 PM, Hans de Goede wrote:
> Hi All,
>
> While updating my local working tree to 4.0-rc4 this morning I noticed that I
> no longer
> get console / kernel messages output on the hdmi output of my ARM board / on
> tty0
>
> This is caused by:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/of?id=2fa645cb2703d9b3786d850db815414dfeefa51d
>
> Reverting this commit fixes this for me.
>
> What is happening here is that the "add_preferred_console("stdout-path", 0,
> NULL);"
> happens before the tty0 registers stopping tty0 from becoming part of the
> console list
> since there already is a preferred console at that time.
>
> This is an undesirable behavior change caused by the commit in question, on
> boards
> where there is both video output, and a serial console configured through
> stdout-path
> we want to have console output on both as we do not know which of the 2 will
> actually
> be hooked up by the user.
I don't see this as a regression, but rather a misconfiguration.
1. Your DT indicates the stdout device is a serial console; that is
the expected outcome. Here's what ePAPR has to say on the chosen/stdout-path
property node:
"A string that specifies the full path to the node representing the device
to be
used for boot console output. If the character ":" is present in the value it
terminates the path. The value may be an alias."
If the serial console is not the stdout device then the DT should not
claim it is.
2. The tty0 console is not now, and has never been, always enabled.
The tty0 console is only enabled when either,
a) there is no other console
b) when specified on the command line (ie., "console=tty0") or
by prom/dt.
Your situation is akin to adding the serial console to the command line; if
"console=tty0" is not also explicitly added, there is no boot console output
to tty0.
3. This same breakage will happen if any other device registers a console
matching
the stdout-path at console_init() time (ie, with console_initcall() macro)
before
the dummy console. The order in which consoles are inited via
console_initcall()
is dependent on link order, so essentially not controllable across different
subsystems (or if there were consoles defined with the arch itself).
That said, I'll look into fixing your use-case automagically without requiring
configuration changes.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: chosen/stdout-path and userland (Re: Can't boot kernel v4.0-rc2 on Koelsch)
On 03/11/2015 06:08 AM, Geert Uytterhoeven wrote: > TL;DR > > After removing the "console=" parameter from chosen/bootargs, and relying > solely on chosen/stdout-path, Hiep-san's Linaro userland cannot find the > console device anymore. > > On Wed, Mar 11, 2015 at 7:45 AM, Laurent Pinchart > wrote: >> On Tuesday 10 March 2015 12:32:59 Geert Uytterhoeven wrote: >>> On Tue, Mar 10, 2015 at 10:03 AM, Cao Minh Hiep wrote: >>>> On 2015年03月10日 17:05, Nobuhiro Iwamatsu wrote: >>>>> This is user land issue. >>>>> Hiep, if you have not changed ttySCX in inittab, you will need to change. >>>>> Could you check your inittab? >>>>> >>>>> --- a/inittab2015-03-10 15:01:58.986609389 +0900 >>>>> +++ b/inittab2015-03-10 15:00:32.132094877 +0900 >>>>> @@ -28,7 +28,7 @@ >>>>> >>>>> l6:6:wait:/etc/init.d/rc 6 >>>>> # Normally not reached, but fallthrough in case of emergency. >>>>> z6:6:respawn:/sbin/sulogin >>>>> >>>>> -SC6:12345:respawn:/sbin/getty -L 38400 ttySC6 >>>>> +SC0:12345:respawn:/sbin/getty -L 38400 ttySC0 >>>>> >>>>> # /sbin/getty invocations for the runlevels. >>>>> # >>>>> # The "id" field MUST be the same as the last >>>> >>>> There is no inittab file in Linaro userland that I am using. >>> >>> I'm not familiar with the Linaro userland. >>> >>> Is there any other configuration file that contains the string "ttySC6"? >>> Or does the Linaro userland derive it from the kernel command line? >>> If yes, perhaps there's a newer version that does look at stdout-path >>> instead? >>> >>> The goal of the stdout-path support was to have a better description in DT >>> and automate things. So I don't think reverting the change is the proper way >>> forward. >>> >>> If you can't get it to work, I think we should bring it up with the DT >>> people first, some of which work for Linaro. >> >> I quite agree with that, but how should userspace know which device node in >> /dev corresponds to the console specified in stdout-path ? > > root@koelsch:~# hd /sys/firmware/devicetree/base/chosen/stdout-path > 2f 73 65 72 69 61 6c 40 65 36 65 36 30 30 30 30 |/serial@e6e6| > 0010 00|.| > 0011 > root@koelsch:~# ls -l /sys/devices/platform/e6e6.serial/tty > total 0 > drwxr-xr-x 3 root root 0 Mar 11 10:50 ttySC0 > root@koelsch:~# cat /sys/devices/platform/e6e6.serial/tty/ttySC0/dev > 204:8 > root@koelsch:~# ls -l /dev/ttySC0 > crw--- 1 root tty 204, 8 Mar 11 10:46 /dev/ttySC0 > > Hence all pieces of the puzzle are available to userspace... > Is there another way? > > BTW, we also had the "/dev/console" abstraction for quite a while. > I've just replaced "ttySC0" by "console" in my /etc/inittab, and everything > still works fine. That's Debian (without systemd) though, so it doesn't help > for the Linaro userland. The tty subsystem emits the list of consoles as a space-delimited, single text line in /sys/class/tty/console/active (alias: /sys/devices/virtual/tty/console/active). The first console listed is the primary console. tty0 is not resolved to its underlying tty device name. That said, is the original reporter of this problem even interested in upgrading their userspace, even if this is fixed in Linaro eventually? What about other Linaro users? I think the only reasonable course here is to continue to support "console=" in /chosen/bootargs. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: chosen/stdout-path and userland (Re: Can't boot kernel v4.0-rc2 on Koelsch)
Hi Geert, On 03/11/2015 09:23 AM, Geert Uytterhoeven wrote: > Hi Peter, > > On Wed, Mar 11, 2015 at 2:05 PM, Peter Hurley > wrote: >> The first console listed is the primary console. >> tty0 is not resolved to its underlying tty device name. > > Are you sure about that? Isn't the last one the primary console? > > root@koelsch:~# cat /sys/class/tty/console/active > tty0 ttySC0 > > If tty0 would be the primary console, getty on /dev/console wouldn't work for > my serial console, as only input on the primary console can be read through > /dev/console if my memory serves me well (or perhaps this has changed?). > > Ah, Documentation/ABI/testing/sysfs-tty agrees with me: > > What: /sys/class/tty/console/active > Date: Nov 2010 > Contact:Kay Sievers > Description: > Shows the list of currently configured > console devices, like 'tty1 ttyS0'. > The last entry in the file is the active > device connected to /dev/console. Did I write 'first'? Yeah, it is 'last', as the documentation states. >> That said, is the original reporter of this problem even interested in >> upgrading their userspace, even if this is fixed in Linaro eventually? >> What about other Linaro users? > > We'll see... > >> I think the only reasonable course here is to continue to support >> "console=" in /chosen/bootargs. > > Hmm... > If you really need it, you can always add it to the U-Boot bootargs variable. Is u-boot a given? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libfdt: Add fdt_path_offset_namelen()
Hi David, On 03/09/2015 08:17 PM, David Gibson wrote: > On Fri, Mar 06, 2015 at 10:12:38AM -0500, Peter Hurley wrote: >> Properties may contain path names which are not NUL-terminated. >> For example, the 'stdout-path' property allows the form 'path:options', >> where the ':' character terminates the path specifier. >> >> Allow these path names to be used in-place for path descending; >> add fdt_path_offset_namelen(), which limits the path name to 'namelen' >> characters. >> >> Reimplement fdt_path_offset() as a trivial wrapper. >> >> Signed-off-by: Peter Hurley > > I think this function is a good idea, however I would like to see a > testcase for it. Sure, I can do that. I assume you mean a path name with non-NUL termination because the fdt_path_offset() tests are already exercising the fdt_path_offset_name() implementation. Is there a readme somewhere regarding the test matrix (ie., which dts files go with which tests)? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()
On 03/06/2015 01:11 PM, Peter Hurley wrote:
> On 03/06/2015 11:52 AM, Leif Lindholm wrote:
[...]
>> Could you give the below a spin, and if it works for you, send me the
>> above tests as a full patch so that I can post both as a series?
>
> Will do as soon as I finish testing.
All passed with your patch; patch for testcases below.
Regards,
Peter Hurley
--- >% ---
From: Peter Hurley
Subject: [PATCH] of: unittest: Add options string testcase variants
Add testcase variants with '/' in the options string to test for
scan beyond end path name terminated by ':'.
Signed-off-by: Peter Hurley
---
drivers/of/unittest.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 0cf9a23..b2d7547 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -92,6 +92,11 @@ static void __init of_selftest_find_node_by_name(void)
"option path test failed\n");
of_node_put(np);
+ np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
+ selftest(np && !strcmp("test/option", options),
+"option path test, subcase #1 failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
selftest(np, "NULL option path test failed\n");
of_node_put(np);
@@ -102,6 +107,12 @@ static void __init of_selftest_find_node_by_name(void)
"option alias path test failed\n");
of_node_put(np);
+ np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
+ &options);
+ selftest(np && !strcmp("test/alias/option", options),
+"option alias path test, subcase #1 failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
selftest(np, "NULL option alias path test failed\n");
of_node_put(np);
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()
On 03/06/2015 11:52 AM, Leif Lindholm wrote:
> Hi Peter,
>
> On Wed, Mar 04, 2015 at 10:45:02AM -0500, Peter Hurley wrote:
>> The path parsing gets lost if the string after ':' contains '/'.
>
> Doh!
Hardly.
I only noticed because I had to implement the corresponding algorithm
for earlycon and FDT, where the string scanning is obvious.
> Thanks for reporting (and sorry for slow response).
No worries :)
Rather, I'd like to thank you for implementing the options string so
that bootloader -> earlycon -> console works so seamlessly now.
Can't wait to see 300Mb/s console from boot.
And thanks for the quick patch. I'm still testing because, while there weren't
those failures, there were some other messages. So I just need to go back
and see if those are regressions.
>> The selftests below fail with:
>> [1.365528] ### dt-test ### FAIL of_selftest_find_node_by_name():99
>> option path test failed
>> [1.365610] ### dt-test ### FAIL of_selftest_find_node_by_name():115
>> option alias path test failed
>>
>> Regards,
>> Peter Hurley
>>
>>
>> --- >% ---
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 41a4a13..07ba5aa 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -94,6 +94,11 @@ static void __init of_selftest_find_node_by_name(void)
>> "option path test failed\n");
>> of_node_put(np);
>>
>> +np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
>> +selftest(np && !strcmp("test/option", options),
>> + "option path test failed\n");
>> +of_node_put(np);
>> +
>> np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
>> selftest(np, "NULL option path test failed\n");
>> of_node_put(np);
>> @@ -104,6 +109,12 @@ static void __init of_selftest_find_node_by_name(void)
>> "option alias path test failed\n");
>> of_node_put(np);
>>
>> +np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
>> + &options);
>> +selftest(np && !strcmp("test/alias/option", options),
>> + "option alias path test failed\n");
>> +of_node_put(np);
>> +
>> np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
>> selftest(np, "NULL option alias path test failed\n");
>> of_node_put(np);
>
> Could you give the below a spin, and if it works for you, send me the
> above tests as a full patch so that I can post both as a series?
Will do as soon as I finish testing.
Regards,
Peter Hurley
> From bf4ab0b2e33902ba88809a3c4a2cdf07efd02dde Mon Sep 17 00:00:00 2001
> From: Leif Lindholm
> Date: Fri, 6 Mar 2015 16:38:54 +
> Subject: [PATCH] of: fix handling of '/' in options for of_find_node_by_path()
>
> Ensure proper handling of paths with appended options (after ':'),
> where those options may contain a '/'.
>
> Fixes: 7914a7c5651a ("of: support passing console options with stdout-path")
> Reported-by: Peter Hurley
> Signed-off-by: Leif Lindholm
> ---
> drivers/of/base.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 0a8aeb8..8b904e5 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -714,16 +714,17 @@ static struct device_node
> *__of_find_node_by_path(struct device_node *parent,
> const char *path)
> {
> struct device_node *child;
> - int len = strchrnul(path, '/') - path;
> - int term;
> + int len;
> + const char *end;
>
> + end = strchr(path, ':');
> + if (!end)
> + end = strchrnul(path, '/');
> +
> + len = end - path;
> if (!len)
> return NULL;
>
> - term = strchrnul(path, ':') - path;
> - if (term < len)
> - len = term;
> -
> __for_each_child_of_node(parent, child) {
> const char *name = strrchr(child->full_name, '/');
> if (WARN(!name, "malformed device_node %s\n", child->full_name))
> @@ -768,8 +769,12 @@ struct device_node *of_find_node_opts_by_path(const char
> *path, const char **opt
>
> /* The path could begin with an alias */
> if (*path != '/&#x
[PATCH] libfdt: Add fdt_path_offset_namelen()
Properties may contain path names which are not NUL-terminated.
For example, the 'stdout-path' property allows the form 'path:options',
where the ':' character terminates the path specifier.
Allow these path names to be used in-place for path descending;
add fdt_path_offset_namelen(), which limits the path name to 'namelen'
characters.
Reimplement fdt_path_offset() as a trivial wrapper.
Signed-off-by: Peter Hurley
---
libfdt/fdt_ro.c| 22 ++
libfdt/libfdt.h| 11 +++
libfdt/version.lds | 1 +
3 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 50007f6..a65e4b5 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -154,9 +154,9 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
return fdt_subnode_offset_namelen(fdt, parentoffset, name,
strlen(name));
}
-int fdt_path_offset(const void *fdt, const char *path)
+int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
{
- const char *end = path + strlen(path);
+ const char *end = path + namelen;
const char *p = path;
int offset = 0;
@@ -164,7 +164,7 @@ int fdt_path_offset(const void *fdt, const char *path)
/* see if we have an alias */
if (*path != '/') {
- const char *q = strchr(path, '/');
+ const char *q = memchr(path, '/', end - p);
if (!q)
q = end;
@@ -177,14 +177,15 @@ int fdt_path_offset(const void *fdt, const char *path)
p = q;
}
- while (*p) {
+ while (p < end) {
const char *q;
- while (*p == '/')
+ while (*p == '/') {
p++;
- if (! *p)
- return offset;
- q = strchr(p, '/');
+ if (p == end)
+ return offset;
+ }
+ q = memchr(p, '/', end - p);
if (! q)
q = end;
@@ -198,6 +199,11 @@ int fdt_path_offset(const void *fdt, const char *path)
return offset;
}
+int fdt_path_offset(const void *fdt, const char *path)
+{
+ return fdt_path_offset_namelen(fdt, path, strlen(path));
+}
+
const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
{
const struct fdt_node_header *nh = _fdt_offset_ptr(fdt, nodeoffset);
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 02baa84..50b13b4 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -318,6 +318,17 @@ int fdt_subnode_offset_namelen(const void *fdt, int
parentoffset,
int fdt_subnode_offset(const void *fdt, int parentoffset, const char *name);
/**
+ * fdt_path_offset_namelen - find a tree node by its full path
+ * @fdt: pointer to the device tree blob
+ * @path: full path of the node to locate
+ * @namelen: number of characters of path to consider
+ *
+ * Identical to fdt_path_offset(), but only consider the first namelen
+ * characters of path as the path name.
+ */
+int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen);
+
+/**
* fdt_path_offset - find a tree node by its full path
* @fdt: pointer to the device tree blob
* @path: full path of the node to locate
diff --git a/libfdt/version.lds b/libfdt/version.lds
index 80b322b..80e945e 100644
--- a/libfdt/version.lds
+++ b/libfdt/version.lds
@@ -8,6 +8,7 @@ LIBFDT_1.2 {
fdt_get_mem_rsv;
fdt_subnode_offset_namelen;
fdt_subnode_offset;
+ fdt_path_offset_namelen;
fdt_path_offset;
fdt_get_name;
fdt_get_property_namelen;
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libfdt: Teach fdt_path_offset() about ':' path separator
[ +cc Greg KH ]
On 03/05/2015 10:23 AM, Rob Herring wrote:
> On Wed, Mar 4, 2015 at 10:44 AM, Peter Hurley
> wrote:
>> stdout-path defines ':' as a path separator and commit 75c28c09af99a
>> ("of: add optional options parameter to of_find_node_by_path()") added
>> the necessary support to parse paths terminated with ':' path separator.
>> commit 7914a7c5651a5 ("of: support passing console options with
>> stdout-path") added options string support to the stdout-path property,
>> which broke earlycon.
>>
>> Add the same support to fdt_path_offset() so earlycon can parse and
>> process stdout-path properties containing an options string.
>>
>> Cc: Leif Lindholm
>> Cc: # 3.19+
>> Signed-off-by: Peter Hurley
>
> This needs to be first applied to upstream dtc[1]. Please do a patch
> against dtc and send to devicetree-compiler list.
Sure, I can do that, but then how is this patch going to end up in
3.19-stable?
Regards,
Peter Hurley
> Rob
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/jdl/dtc.git/
>
>> ---
>> scripts/dtc/libfdt/fdt_ro.c | 9 +
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/dtc/libfdt/fdt_ro.c b/scripts/dtc/libfdt/fdt_ro.c
>> index 02b6d68..a96e452 100644
>> --- a/scripts/dtc/libfdt/fdt_ro.c
>> +++ b/scripts/dtc/libfdt/fdt_ro.c
>> @@ -156,7 +156,8 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
>>
>> int fdt_path_offset(const void *fdt, const char *path)
>> {
>> - const char *end = path + strlen(path);
>> + const char *separator = strchr(path, ':');
>> + const char *end = separator ? separator : path + strlen(path);
>> const char *p = path;
>> int offset = 0;
>>
>> @@ -166,7 +167,7 @@ int fdt_path_offset(const void *fdt, const char *path)
>> if (*path != '/') {
>> const char *q = strchr(path, '/');
>>
>> - if (!q)
>> + if (!q || q > end)
>> q = end;
>>
>> p = fdt_get_alias_namelen(fdt, p, q - p);
>> @@ -177,7 +178,7 @@ int fdt_path_offset(const void *fdt, const char *path)
>> p = q;
>> }
>>
>> - while (*p) {
>> + while (p < end) {
>> const char *q;
>>
>> while (*p == '/')
>> @@ -185,7 +186,7 @@ int fdt_path_offset(const void *fdt, const char *path)
>> if (! *p)
>> return offset;
>> q = strchr(p, '/');
>> - if (! q)
>> + if (!q || q > end)
>> q = end;
>>
>> offset = fdt_subnode_offset_namelen(fdt, offset, p, q-p);
>> --
>> 2.3.1
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libfdt: Teach fdt_path_offset() about ':' path separator
stdout-path defines ':' as a path separator and commit 75c28c09af99a
("of: add optional options parameter to of_find_node_by_path()") added
the necessary support to parse paths terminated with ':' path separator.
commit 7914a7c5651a5 ("of: support passing console options with
stdout-path") added options string support to the stdout-path property,
which broke earlycon.
Add the same support to fdt_path_offset() so earlycon can parse and
process stdout-path properties containing an options string.
Cc: Leif Lindholm
Cc: # 3.19+
Signed-off-by: Peter Hurley
---
scripts/dtc/libfdt/fdt_ro.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/scripts/dtc/libfdt/fdt_ro.c b/scripts/dtc/libfdt/fdt_ro.c
index 02b6d68..a96e452 100644
--- a/scripts/dtc/libfdt/fdt_ro.c
+++ b/scripts/dtc/libfdt/fdt_ro.c
@@ -156,7 +156,8 @@ int fdt_subnode_offset(const void *fdt, int parentoffset,
int fdt_path_offset(const void *fdt, const char *path)
{
- const char *end = path + strlen(path);
+ const char *separator = strchr(path, ':');
+ const char *end = separator ? separator : path + strlen(path);
const char *p = path;
int offset = 0;
@@ -166,7 +167,7 @@ int fdt_path_offset(const void *fdt, const char *path)
if (*path != '/') {
const char *q = strchr(path, '/');
- if (!q)
+ if (!q || q > end)
q = end;
p = fdt_get_alias_namelen(fdt, p, q - p);
@@ -177,7 +178,7 @@ int fdt_path_offset(const void *fdt, const char *path)
p = q;
}
- while (*p) {
+ while (p < end) {
const char *q;
while (*p == '/')
@@ -185,7 +186,7 @@ int fdt_path_offset(const void *fdt, const char *path)
if (! *p)
return offset;
q = strchr(p, '/');
- if (! q)
+ if (!q || q > end)
q = end;
offset = fdt_subnode_offset_namelen(fdt, offset, p, q-p);
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] of: add optional options parameter to of_find_node_by_path()
options),
> + "option path test failed\n");
> + of_node_put(np);
> +
> + np = of_find_node_opts_by_path("testcase-alias:testaliasoption",
> +&options);
> + selftest(np && !strcmp("testaliasoption", options),
> + "option alias path test failed\n");
> + of_node_put(np);
> }
The path parsing gets lost if the string after ':' contains '/'.
The selftests below fail with:
[1.365528] ### dt-test ### FAIL of_selftest_find_node_by_name():99 option
path test failed
[1.365610] ### dt-test ### FAIL of_selftest_find_node_by_name():115 option
alias path test failed
Regards,
Peter Hurley
--- >% ---
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 41a4a13..07ba5aa 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -94,6 +94,11 @@ static void __init of_selftest_find_node_by_name(void)
"option path test failed\n");
of_node_put(np);
+ np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
+ selftest(np && !strcmp("test/option", options),
+"option path test failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
selftest(np, "NULL option path test failed\n");
of_node_put(np);
@@ -104,6 +109,12 @@ static void __init of_selftest_find_node_by_name(void)
"option alias path test failed\n");
of_node_put(np);
+ np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
+ &options);
+ selftest(np && !strcmp("test/alias/option", options),
+"option alias path test failed\n");
+ of_node_put(np);
+
np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
selftest(np, "NULL option alias path test failed\n");
of_node_put(np);
> static void __init of_selftest_dynamic(void)
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 29f0adc..a36be70 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -228,7 +228,13 @@ extern struct device_node
> *of_find_matching_node_and_match(
> const struct of_device_id *matches,
> const struct of_device_id **match);
>
> -extern struct device_node *of_find_node_by_path(const char *path);
> +extern struct device_node *of_find_node_opts_by_path(const char *path,
> + const char **opts);
> +static inline struct device_node *of_find_node_by_path(const char *path)
> +{
> + return of_find_node_opts_by_path(path, NULL);
> +}
> +
> extern struct device_node *of_find_node_by_phandle(phandle handle);
> extern struct device_node *of_get_parent(const struct device_node *node);
> extern struct device_node *of_get_next_parent(struct device_node *node);
> @@ -385,6 +391,12 @@ static inline struct device_node
> *of_find_node_by_path(const char *path)
> return NULL;
> }
>
> +static inline struct device_node *of_find_node_opts_by_path(const char *path,
> + const char **opts)
> +{
> + return NULL;
> +}
> +
> static inline struct device_node *of_get_parent(const struct device_node
> *node)
> {
> return NULL;
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/7] of: Document {little,big,native}-endian bindings
On 03/02/2015 11:28 AM, Kevin Cernekee wrote: > On Mon, Mar 2, 2015 at 8:08 AM, Peter Hurley wrote: >> On 03/02/2015 09:56 AM, Kevin Cernekee wrote: >>> On Mon, Mar 2, 2015 at 5:14 AM, Peter Hurley >>> wrote: >>>> On 11/24/2014 06:36 PM, Kevin Cernekee wrote: >>>>> These apply to newly converted drivers, like serial8250/libahci/... >>>>> The examples were adapted from the regmap bindings document. >>>>> >>>>> Signed-off-by: Kevin Cernekee >>>>> --- >>>>> .../devicetree/bindings/common-properties.txt | 60 >>>>> ++ >>>>> 1 file changed, 60 insertions(+) >>>>> create mode 100644 >>>>> Documentation/devicetree/bindings/common-properties.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/common-properties.txt >>>>> b/Documentation/devicetree/bindings/common-properties.txt >>>>> new file mode 100644 >>>>> index 000..21044a4 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/common-properties.txt >>>>> @@ -0,0 +1,60 @@ >>>>> +Common properties >>>>> + >>>>> +The ePAPR specification does not define any properties related to >>>>> hardware >>>>> +byteswapping, but endianness issues show up frequently in porting Linux >>>>> to >>>>> +different machine types. This document attempts to provide a consistent >>>>> +way of handling byteswapping across drivers. >>>>> + >>>>> +Optional properties: >>>>> + - big-endian: Boolean; force big endian register accesses >>>>> + unconditionally (e.g. ioread32be/iowrite32be). Use this if you >>>>> + know the peripheral always needs to be accessed in BE mode. >>>>> + - little-endian: Boolean; force little endian register accesses >>>>> + unconditionally (e.g. readl/writel). Use this if you know the >>>>> + peripheral always needs to be accessed in LE mode. This is the >>>>> + default. >>>> >>>> There is a fundamental problem with specifying the default in DT bindings. >>>> How can drivers which are currently native-endian support big-endian? >>>> >>>> If the driver is converted to support big-endian, every previous >>>> devicetree will be invalid with the new kernel (because those devicetrees >>>> don't specify 'native-endian'). >>>> >>>> IOW, consider if the default were 'native-endian'. How would the 8250 >>>> driver support existing devicetrees? >>> >>> Correct. This scheme is intended for drivers like 8250 and libahci >>> which currently default to little-endian by virtue of using >>> readl/writel for MMIO accesses. Drivers that default to native-endian >>> should specify that in their bindings documents, similar to >>> Documentation/devicetree/bindings/regmap/regmap.txt. >> >> Which effectively means if a user can't upgrade their devicetree, they >> can't upgrade their kernel. I don't think that flies. > > This doesn't change the behavior of pre-existing drivers that > implement the *-endian properties in a different way. There are not > many of these drivers and they can be documented as special cases. Yeah, ok, as long as there's no expectation that existing drivers meet this criteria when they add big-endian support. >> It's exactly this kind of stuff that prompted Jonathan Corbet's article, >> "Device trees as ABI" http://lwn.net/Articles/561462 >> >> Why not leave the default unspecified? > > The document aims to provide a consistent way of handling DT > endianness properties across (compliant) drivers. It is confusing if > one new driver defaults to little-endian, and another new driver > defaults to native-endian. Ok. How many 4.0 driver + DT submissions that are native-endian are declaring this binding? > And since most of the commonly used drivers already implement > little-endian MMIO accesses, that is the default. My personal > preference would have been native-endian since that seems more common > on the hardware side, but defaulting to little-endian prevents > breaking the device tree "ABI" on existing systems. That was basically my point; there's no way to meet these goals for existing, native-endian drivers without breakage (just as there would have been no way if native-endian had been the default). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/7] of: Document {little,big,native}-endian bindings
On 03/02/2015 09:56 AM, Kevin Cernekee wrote: > On Mon, Mar 2, 2015 at 5:14 AM, Peter Hurley wrote: >> On 11/24/2014 06:36 PM, Kevin Cernekee wrote: >>> These apply to newly converted drivers, like serial8250/libahci/... >>> The examples were adapted from the regmap bindings document. >>> >>> Signed-off-by: Kevin Cernekee >>> --- >>> .../devicetree/bindings/common-properties.txt | 60 >>> ++ >>> 1 file changed, 60 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/common-properties.txt >>> >>> diff --git a/Documentation/devicetree/bindings/common-properties.txt >>> b/Documentation/devicetree/bindings/common-properties.txt >>> new file mode 100644 >>> index 000..21044a4 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/common-properties.txt >>> @@ -0,0 +1,60 @@ >>> +Common properties >>> + >>> +The ePAPR specification does not define any properties related to hardware >>> +byteswapping, but endianness issues show up frequently in porting Linux to >>> +different machine types. This document attempts to provide a consistent >>> +way of handling byteswapping across drivers. >>> + >>> +Optional properties: >>> + - big-endian: Boolean; force big endian register accesses >>> + unconditionally (e.g. ioread32be/iowrite32be). Use this if you >>> + know the peripheral always needs to be accessed in BE mode. >>> + - little-endian: Boolean; force little endian register accesses >>> + unconditionally (e.g. readl/writel). Use this if you know the >>> + peripheral always needs to be accessed in LE mode. This is the >>> + default. >> >> There is a fundamental problem with specifying the default in DT bindings. >> How can drivers which are currently native-endian support big-endian? >> >> If the driver is converted to support big-endian, every previous >> devicetree will be invalid with the new kernel (because those devicetrees >> don't specify 'native-endian'). >> >> IOW, consider if the default were 'native-endian'. How would the 8250 >> driver support existing devicetrees? > > Correct. This scheme is intended for drivers like 8250 and libahci > which currently default to little-endian by virtue of using > readl/writel for MMIO accesses. Drivers that default to native-endian > should specify that in their bindings documents, similar to > Documentation/devicetree/bindings/regmap/regmap.txt. Which effectively means if a user can't upgrade their devicetree, they can't upgrade their kernel. I don't think that flies. It's exactly this kind of stuff that prompted Jonathan Corbet's article, "Device trees as ABI" http://lwn.net/Articles/561462 Why not leave the default unspecified? Regards, Peter Hurley > In practice we might not see too many cases of native-endian drivers > that need to be converted to work in forced big-endian mode anyway, > because most uses of the __raw_* accessors are found in SoC-specific > code. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/7] of: Document {little,big,native}-endian bindings
On 11/24/2014 06:36 PM, Kevin Cernekee wrote:
> These apply to newly converted drivers, like serial8250/libahci/...
> The examples were adapted from the regmap bindings document.
>
> Signed-off-by: Kevin Cernekee
> ---
> .../devicetree/bindings/common-properties.txt | 60
> ++
> 1 file changed, 60 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/common-properties.txt
>
> diff --git a/Documentation/devicetree/bindings/common-properties.txt
> b/Documentation/devicetree/bindings/common-properties.txt
> new file mode 100644
> index 000..21044a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/common-properties.txt
> @@ -0,0 +1,60 @@
> +Common properties
> +
> +The ePAPR specification does not define any properties related to hardware
> +byteswapping, but endianness issues show up frequently in porting Linux to
> +different machine types. This document attempts to provide a consistent
> +way of handling byteswapping across drivers.
> +
> +Optional properties:
> + - big-endian: Boolean; force big endian register accesses
> + unconditionally (e.g. ioread32be/iowrite32be). Use this if you
> + know the peripheral always needs to be accessed in BE mode.
> + - little-endian: Boolean; force little endian register accesses
> + unconditionally (e.g. readl/writel). Use this if you know the
> + peripheral always needs to be accessed in LE mode. This is the
> + default.
There is a fundamental problem with specifying the default in DT bindings.
How can drivers which are currently native-endian support big-endian?
If the driver is converted to support big-endian, every previous
devicetree will be invalid with the new kernel (because those devicetrees
don't specify 'native-endian').
IOW, consider if the default were 'native-endian'. How would the 8250
driver support existing devicetrees?
Regards,
Peter Hurley
> + - native-endian: Boolean; always use register accesses matched to the
> + endianness of the kernel binary (e.g. LE vmlinux -> readl/writel,
> + BE vmlinux -> ioread32be/iowrite32be). In this case no byteswaps
> + will ever be performed. Use this if the hardware "self-adjusts"
> + register endianness based on the CPU's configured endianness.
> +
> +Note that regmap, in contrast, defaults to native-endian. But this
> +document is targeted for existing drivers, most of which currently use
> +readl/writel because they expect to be accessing PCI/PCIe devices rather
> +than memory-mapped SoC peripherals. Since the readl/writel accessors
> +perform a byteswap on BE systems, this means that the drivers in question
> +are implicitly "little-endian".
> +
> +Examples:
> +Scenario 1 : CPU in LE mode & device in LE mode.
> +dev: dev@40031000 {
> + compatible = "name";
> + reg = <0x40031000 0x1000>;
> + ...
> + native-endian;
> +};
> +
> +Scenario 2 : CPU in LE mode & device in BE mode.
> +dev: dev@40031000 {
> + compatible = "name";
> + reg = <0x40031000 0x1000>;
> + ...
> + big-endian;
> +};
> +
> +Scenario 3 : CPU in BE mode & device in BE mode.
> +dev: dev@40031000 {
> + compatible = "name";
> + reg = <0x40031000 0x1000>;
> + ...
> + native-endian;
> +};
> +
> +Scenario 4 : CPU in BE mode & device in LE mode.
> +dev: dev@40031000 {
> + compatible = "name";
> + reg = <0x40031000 0x1000>;
> + ...
> + little-endian;
> +};
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 5/7] serial: earlycon: Set UPIO_MEM32BE based on DT properties
Hi Kevin,
On 11/24/2014 06:36 PM, Kevin Cernekee wrote:
> If an earlycon (stdout-path) node is being used, check for "big-endian"
> or "native-endian" properties and pass the appropriate iotype to the
> driver.
>
> Note that LE sets UPIO_MEM (8-bit) but BE sets UPIO_MEM32BE (32-bit). The
> big-endian property only really makes sense in the context of 32-bit
> registers, since 8-bit accesses never require data swapping.
>
> At some point, the of_earlycon code may want to pass in the reg-io-width,
> reg-offset, and reg-shift parameters too.
>
> Signed-off-by: Kevin Cernekee
> ---
> drivers/of/fdt.c | 7 ++-
> drivers/tty/serial/earlycon.c | 4 ++--
> include/linux/serial_core.h | 2 +-
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 658656f..9d21472 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -794,6 +794,8 @@ int __init early_init_dt_scan_chosen_serial(void)
>
> while (match->compatible[0]) {
> unsigned long addr;
> + unsigned char iotype = UPIO_MEM;
> +
> if (fdt_node_check_compatible(fdt, offset, match->compatible)) {
> match++;
> continue;
> @@ -803,7 +805,10 @@ int __init early_init_dt_scan_chosen_serial(void)
> if (!addr)
> return -ENXIO;
>
> - of_setup_earlycon(addr, match->data);
> + if (of_fdt_is_big_endian(fdt, offset))
> + iotype = UPIO_MEM32BE;
> +
> + of_setup_earlycon(addr, iotype, match->data);
I know these got ACKs already but as you point out in the commit log,
earlycon _will_ need reg-io-width, reg-offset and reg-shift. Since the
distinction between early_init_dt_scan_chosen_serial() and
of_setup_earlycon() is arbitrary, I'd rather see of_setup_earlycon()
taught to properly decode of_serial driver bindings instead of a
stack of parameters to of_setup_earlycon().
In fact, this patch allows a mis-defined devicetree to bring up a
functioning earlycon because the 'big-endian' property is directly
associated with UPIO_MEM32BE, which will create incompatibility problems
when DT earlycon is fixed to decode the of_serial DT bindings.
[rant]
In general, the ability to query devicetree from all over the
kernel creates all kinds of compatibility issues which eventually
will cause unresolvable breakage.
The same rigor applied to ioctls is the analysis required for how
DT bindings are used in the kernel.
I realize that since this particular case only applies to earlycon, it's
no big deal, but if this same mistake had been made in the of_serial
driver, the serial core would be permanently stuck with the
'big-endian' property == UPIO_MEM32BE which could impact future designs.
[/rant]
Regards,
Peter Hurley
> return 0;
> }
> return -ENODEV;
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index a514ee6..548f7d7 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -148,13 +148,13 @@ int __init setup_earlycon(char *buf, const char *match,
> return 0;
> }
>
> -int __init of_setup_earlycon(unsigned long addr,
> +int __init of_setup_earlycon(unsigned long addr, unsigned char iotype,
>int (*setup)(struct earlycon_device *, const char
> *))
> {
> int err;
> struct uart_port *port = &early_console_dev.port;
>
> - port->iotype = UPIO_MEM;
> + port->iotype = iotype;
> port->mapbase = addr;
> port->uartclk = BASE_BAUD * 16;
> port->membase = earlycon_map(addr, SZ_4K);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index d2d5bf6..0d60c64 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -310,7 +310,7 @@ struct earlycon_device {
> int setup_earlycon(char *buf, const char *match,
> int (*setup)(struct earlycon_device *, const char *));
>
> -extern int of_setup_earlycon(unsigned long addr,
> +extern int of_setup_earlycon(unsigned long addr, unsigned char iotype,
>int (*setup)(struct earlycon_device *, const char
> *));
>
> #define EARLYCON_DECLARE(name, func) \
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -next 00/13] Extensible console matching & direct earlycon
[ +cc devicetree, Mark Rutland, Grant Likely ] On 02/26/2015 09:58 AM, Rob Herring wrote: > On Thu, Feb 26, 2015 at 8:48 AM, Peter Hurley > wrote: >> Hi Rob, >> >> On 02/24/2015 03:20 PM, Rob Herring wrote: >>> On Tue, Feb 24, 2015 at 1:53 PM, Peter Hurley >>> wrote: >> >> [...] >> >>>>>> Direct earlycon >>>>>> >>>>>> This feature enables arches and proms to start an earlycon directly, >>>>>> rather than requiring an "earlycon=" command line parameter. >>>>>> Devicetree can already do this via the 'linux,stdout-path' property, >>>>>> but arch and prom code requires direct coupling to the serial driver. >>>>>> >>>>>> This support is implemented by judicious refactoring and the same >>>>>> construct that devicetree and early_param use: a link table containing >>>>>> the necessary information (name and setup() function) to find and >>>>>> bind the appropriate earlycon "driver". >>>>> >>>>> I've skimmed thru this and it looks like a great improvement. >>>>> >>>>> One problem we have currently with DT stdout-path and earlycon is a >>>>> preferred console does not get registered, so the console will get >>>>> switched to tty0 and you lose your console. The problem is DT does not >>>>> know the console name to register a preferred console. It looks like >>>>> this series may help this problem, but I'm not sure and wanted your >>>>> thoughts. >>>> >>>> I thought that of_alias_scan() + of_console_check() caused DT stdout-path >>>> to add_preferred_console() the driver console @ port registration time >>>> via uart_add_one_port() -> of_console_check(). >>>> >>>> Is that not how that works? >>> >>> Yes, I believe that is how it works with earlycon not enabled. This >>> doesn't work when earlycon is enabled with just "earlycon" on the >>> command line. The fix I have is here[1], but I don't like putting DT >>> specifics into the console code. >> >> After much gnashing of teeth and pulling of hair yesterday, I managed >> to mock up the situation you describe, but I need to study it in more >> detail. Some things I did learn: >> >> 1. The serial console _does_ come back up when using stdout-path but the >>line settings don't match, because the serial core sets them to the >>default of 9600n8 if unspecified. > > That may have been what I saw as I tested on QEMU which ignores the > baud rate. But it does stop between the time tty0 is enabled and the > "real" serial console which is a time period we really want the > console. The bug is that of_alias_scan() does not emit a preferred console, but rather defers it until of_console_check(). In many setups, this means there is no preferred console. So when the dummy vt console loads (which happens via console_init()), all the boot consoles are disabled. The trivial fix is to emit a preferred console from of_alias_scan(), even if that console is never matched. I've sent you a patch to do just that. >> 2. The line settings can now be set with stdout-path like, >> stdout-path = "serial0:115200n8" >>but this breaks DT earlycon (as I wrote in the other email you were >>cc'd on). > > Right. We should fix libfdt. Ok. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] of: Fix premature bootconsole disable with 'stdout-path'
Support for devicetree serial consoles via 'stdout-path' causes
bootconsoles to be disabled when the vt dummy console loads, since
there is no preferred console (the preferred console is not added
until the device is probed).
Ensure there is at least a preferred console, even if never matched.
Requires: "console: Fix console name size mismatch"
Cc: Andrew Morton
Signed-off-by: Peter Hurley
---
drivers/of/base.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 36536b6..83dc8a6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1885,8 +1885,10 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
align))
name = of_get_property(of_chosen, "linux,stdout-path",
NULL);
if (IS_ENABLED(CONFIG_PPC) && !name)
name = of_get_property(of_aliases, "stdout", NULL);
- if (name)
+ if (name) {
of_stdout = of_find_node_opts_by_path(name,
&of_stdout_options);
+ add_preferred_console("stdout-path", 0, NULL);
+ }
}
if (!of_aliases)
--
2.3.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] of: support passing console options with stdout-path
Hi Andrew, On 02/26/2015 08:46 AM, Andrew Lunn wrote: > On Thu, Feb 26, 2015 at 06:55:22AM -0500, Peter Hurley wrote: >> On 11/27/2014 12:56 PM, Leif Lindholm wrote: >>> Support specifying console options (like with console=ttyXN,) >>> by appending them to the stdout-path property after a separating ':'. >>> >>> Example: >>> stdout-path = "uart0:115200"; >> >> This format breaks DT earlycon because libfdt doesn't recognize ':' as a >> path terminator. >> >> It's simple enough to fix this directly in early_init_dt_scan_chosen_serial() >> but perhaps it would better to teach fdt_path_offset() to recognize ':'? > > Hi Peter > > ePAPR says for stdout-path in Chosen: > > A string that specifies the full path to the node representing the > device to be used for boot console output. If the character ":" is > present in the value it terminates the path. The value may be an > alias. > > So it is probably wise to implement this properly. Sorry if I was unclear. My question was not _whether_ to fix this for earlycon, but rather _how_. IOW, is the ':' character accepted as a path terminator for 1. all nodes, so fix this in fdt_path_offset(); 2. only chosen nodes; 3. unique to stdout-path, so fix this in early_init_dt_scan_chosen_serial()? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] of: support passing console options with stdout-path
On 11/27/2014 12:56 PM, Leif Lindholm wrote:
> Support specifying console options (like with console=ttyXN,)
> by appending them to the stdout-path property after a separating ':'.
>
> Example:
> stdout-path = "uart0:115200";
This format breaks DT earlycon because libfdt doesn't recognize ':' as a
path terminator.
It's simple enough to fix this directly in early_init_dt_scan_chosen_serial()
but perhaps it would better to teach fdt_path_offset() to recognize ':'?
Regards,
Peter Hurley
> Signed-off-by: Leif Lindholm
> ---
> drivers/of/base.c |9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 7f0e5f7..6d2d45e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -37,6 +37,7 @@ EXPORT_SYMBOL(of_allnodes);
> struct device_node *of_chosen;
> struct device_node *of_aliases;
> struct device_node *of_stdout;
> +static const char *of_stdout_options;
>
> struct kset *of_kset;
>
> @@ -1844,7 +1845,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64
> align))
> if (IS_ENABLED(CONFIG_PPC) && !name)
> name = of_get_property(of_aliases, "stdout", NULL);
> if (name)
> - of_stdout = of_find_node_by_path(name);
> + of_stdout = of_find_node_opts_by_path(name,
> &of_stdout_options);
> }
>
> if (!of_aliases)
> @@ -1968,9 +1969,13 @@ EXPORT_SYMBOL_GPL(of_prop_next_string);
> */
> bool of_console_check(struct device_node *dn, char *name, int index)
> {
> + char *console_options;
> +
> if (!dn || dn != of_stdout || console_set_on_cmdline)
> return false;
> - return !add_preferred_console(name, index, NULL);
> +
> + console_options = kstrdup(of_stdout_options, GFP_KERNEL);
> + return !add_preferred_console(name, index, console_options);
> }
> EXPORT_SYMBOL_GPL(of_console_check);
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants
Hi Pantelis, On 02/19/2015 01:44 PM, Pantelis Antoniou wrote: > Hi Tony, > >> On Feb 19, 2015, at 20:36 , Tony Lindgren wrote: >> >> * Pantelis Antoniou [150219 10:32]: >>>> On Feb 19, 2015, at 20:16 , Tony Lindgren wrote: >>>> >>>> Uhh I don't like the idea of duplicating the i2c-omap.c driver under >>>> arch/arm.. And in general we should initialize things later rather >>>> than earlier. >>>> >>>> What's stopping doing these quirk checks later on time with just >>>> a regular device driver, something like drivers/misc/bbone-quirks.c? >>>> >>> >>> We have no choice; we are way early in the boot process, right after >>> the device tree unflattening step. >> >> To me it seems the dt patching part should be done with minimal >> code before any driver like features.. >> > > The way it’s done right now is with minimal code. Reading the EEPROM > is required. > >>> I’ve toyed with the idea of using early platform devices but the omap-i2c >>> driver >>> would need some tender love and care to make it work, and I didn’t want to >>> get >>> bogged down with i2c driver details at this point. >> >> ..so how about just parse a kernel cmdline for the quirks to apply >> based on a version string or similar? That can be easily populated >> by u-boot or set manually with setenv. >> >> That leaves out the need for tinkering with i2c super early in >> the kernel for revision detection. >> > > You assume there’s going to be a bootloader… So does this patch. > diff --git a/arch/arm/mach-omap2/am33xx-dt-quirks.c > b/arch/arm/mach-omap2/am33xx-dt-quirks.c [...] > + * Note that we rely on the bootloader setting up the muxes > + * (which is the case for u-boot). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] of: DT quirks infrastructure
Hi Guenter, On 02/20/2015 11:47 AM, Guenter Roeck wrote: [...] > I am open to hearing your suggestions for our use case, where the CPU card > with > the eeprom is manufactured separately from its carier cards. I think your use case may be more compelling than two flavors of Beaglebone (one of which is pretty much a dead stick), but it's also less clear what your design constraints are (not that I really want to know, 'cause I don't). But the logical extension of this is an N-way dtb that supports unrelated SOCs, and I think most would agree that's not an acceptable outcome. My thought was that every design that can afford an EEPROM to probe can afford a bootloader to select the appropriate dtb, and can afford the extra space required for multiple dtbs. I'm not naysaying; I just want to elicit enough information so the community can make informed decisions. > I assume you might suggest that manufacturing should (re-)program the EEPROM > on the CPU card after it was inserted into the carrier. > > Problem is though that the CPU card may be inserted into ts carrier outside > manufacturing, at the final stages of assembly or in product repair. Those > groups would typically not even have the means to (re-)program the eeprom. > Besides, manufacturing would, quite understandably, go ballistic if we demand > that they start programming EEPROMs after insertion into carrier, and no > longer > use pre-programmed EEPROMs. I agree; that would be The Wrong Way. > Note that it is not feasible to put the necessary EEPROM onto the carrier > either. Maybe in a later design. Maybe that makes sense, and we will go along > that route at some point. However, forcing a specific hardware solution > due to software limitations, ie lack of ability by core software to handle > the different carries, seems to be not the right decision to make on an > OS level. Agreed; hardware is what it is. > In the PCI world it has long since been accepted that the world is not > perfect. > The argument here is pretty much equivalent to demanding that PCI drop its > quirks mechanism, to force the HW manufacturers to finally get it right from > the beginning. I somehow suspect that this won't happen. I was thinking back to the introductions of fast DEVSEL# and AGP :( > Instead of questioning the need for a mechanism such as the one proposed by > Pantelis, I think our time would be better spent arguing if it is the right > mechanism and, if not, how it can be improved. My thoughts exactly. Apologies if something I wrote came across as "You Shall Not Pass" :) One issue seems to be the moving target that is the compelling use case(s). The initial submission implied it was the Beaglebone, which comes with 4GB eMMC/microSD so naturally the argument for space-savings with DTBs doesn't fly. That has since been clarified so no need to rehash that. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] of: DT quirks infrastructure
On 02/20/2015 10:38 AM, Pantelis Antoniou wrote: > Hi Peter, > >> On Feb 20, 2015, at 17:24 , Peter Hurley wrote: >> >> On 02/20/2015 10:02 AM, Pantelis Antoniou wrote: >>> Hi Peter, >>> >>>> On Feb 20, 2015, at 17:00 , Peter Hurley wrote: >>>> >>>> On 02/20/2015 09:35 AM, Ludovic Desroches wrote: [...] >>>>> As you said, we can imagine many reasons to have a failure during the >>>>> production, having several DTB files will increase the risk. >>>> >>>> It's interesting that you don't see the added complexity of open-coding >>>> the i2c driver or mixing DTS fragments for different designs as increased >>>> risk >>>> (for us all). >>>> >>>> >>> >>> You don’t have to use it. >> >>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile >>> index 5d27dfd..02129e7 100644 >>> --- a/arch/arm/mach-omap2/Makefile >>> +++ b/arch/arm/mach-omap2/Makefile >>> @@ -259,6 +259,11 @@ obj-$(CONFIG_MACH_CRANEBOARD) += >>> board-am3517crane.o >>> >>> obj-$(CONFIG_MACH_SBC3530) += board-omap3stalker.o >>> >>> +# DT quirks >>> +ifneq ($(CONFIG_OF_DYNAMIC),) >>> +obj-$(CONFIG_SOC_AM33XX) += am33xx-dt-quirks.o >>> +endif >> >> Won't this automatically be included on my Black that supports DT overlays? >> > > Yes it will. It is a grand total of 498 lines of code, and the total size of > the code segment is about 2.2K. > > You do realize that you’re probably booting a multi-platform kernel on the > black right? Including things for all 2xxx/3xxx and 44xx platforms? > For instance: > >> ~/ti/kernels/linux-github.git $ wc -l arch/arm/mach-omap2/*44xx*.c >>443 arch/arm/mach-omap2/clockdomains44xx_data.c >>526 arch/arm/mach-omap2/cminst44xx.c >>251 arch/arm/mach-omap2/cpuidle44xx.c >>250 arch/arm/mach-omap2/dpll44xx.c >> 4864 arch/arm/mach-omap2/omap_hwmod_44xx_data.c >>295 arch/arm/mach-omap2/pm44xx.c >>358 arch/arm/mach-omap2/powerdomains44xx_data.c >> 62 arch/arm/mach-omap2/prcm_mpu44xx.c >>770 arch/arm/mach-omap2/prm44xx.c >>210 arch/arm/mach-omap2/prminst44xx.c >>117 arch/arm/mach-omap2/vc44xx_data.c >>130 arch/arm/mach-omap2/voltagedomains44xx_data.c >>104 arch/arm/mach-omap2/vp44xx_data.c >> 8380 total > > I bet those things are far larger than 2.2K. And I also bet that in the > tradeoff analysis that the board maintainer did things came down to > increasing complexity so that he can consolidate the kernels for all the > other platforms he has to support besides the black. Not that it really matters, but I'm not using any of that. >>> Some people really do though. As for increased risk >>> I expect to see arguments instead of a statement. >> >> No one is wasting your time with random arguments. Please keep your tone >> civil. >> > > A statement like 'increasing risk for all of us' is very open ended. What is > the risk? How much of it exists? My point was simply that this trades reduced complexity in one area with increased complexity in another area. For you, that trade-off is worth it, but for others, not so much. FWIW, I agree that some mechanism is required to support the other use cases. I just don't think ease of manufacturing, when the submit configuration is the BeagleBone, is where I would hang my hat. > If I offended you I’m really sorry though, I meant no such thing. In re-reading it, I realize I shouldn't have taken offense. Thanks anyway for the apology. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] of: DT quirks infrastructure
On 02/20/2015 10:02 AM, Pantelis Antoniou wrote: > Hi Peter, > >> On Feb 20, 2015, at 17:00 , Peter Hurley wrote: >> >> On 02/20/2015 09:35 AM, Ludovic Desroches wrote: >>> On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote: >>>> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote: >>>>> >>>>>> On Feb 19, 2015, at 19:30 , Frank Rowand wrote: >>>>>> >>>>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote: >>>>>>> Hi Frank, >>>>>>> >>>>>>>> On Feb 19, 2015, at 18:48 , Frank Rowand >>>>>>>> wrote: >>>>>>>> >>>>>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote: >>>>>>>>> Hi Mark, >>>>>>>>> >>>>>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>>>> +While this may in theory work, in practice it is very cumbersome >>>>>>>>>>>>> +for the following reasons: >>>>>>>>>>>>> + >>>>>>>>>>>>> +1. The act of selecting a different boot device tree blob >>>>>>>>>>>>> requires >>>>>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration >>>>>>>>>>>>> or >>>>>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, >>>>>>>>>>>>> the >>>>>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob. >>>>>>>>>>>> >>>>>>>>>>>> You can have several bootloader builds, or even a single build with >>>>>>>>>>>> something like appended DTB to get an appropriate DTB if the same >>>>>>>>>>>> binary >>>>>>>>>>>> will otherwise work across all variants of a board. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> No, the same DTB will not work across all the variants of a board. >>>>>>>>>> >>>>>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the >>>>>>>>>> case >>>>>>>>>> the FW/bootloader could be common even if the DTB couldn't. >>>>>>>>>> >>>>>>>>>> To some extent there must be a DTB that will work across all variants >>>>>>>>>> (albeit with limited utility) or the quirk approach wouldn't work… >>>>>>>>>> >>>>>>>>> >>>>>>>>> That’s not correct; the only part of the DTB that needs to be common >>>>>>>>> is the model property that would allow the quirk detection logic to >>>>>>>>> fire. >>>>>>>>> >>>>>>>>> So, there is a base DTB that will work on all variants, but that only >>>>>>>>> means >>>>>>>>> that it will work only up to the point that the quirk detector method >>>>>>>>> can work. So while in recommended practice there are common subsets >>>>>>>>> of the DTB that might work, they might be unsafe. >>>>>>>>> >>>>>>>>> For instance on the beaglebone the regulator configuration is >>>>>>>>> different >>>>>>>>> between white and black, it is imperative you get them right otherwise >>>>>>>>> you risk board damage. >>>>>>>>> >>>>>>>>>>>> So it's not necessarily true that you need a complex bootloader. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>>> +2. On many instances boot time is extremely critical; in some >>>>>>>>>>>>> cases >>>>>>>>>>>>> +there are hard requirements like having working video feeds in >>>>>>>>>>>>> under >>>>>>
Re: [PATCH 2/4] of: DT quirks infrastructure
On 02/20/2015 09:35 AM, Ludovic Desroches wrote: > On Fri, Feb 20, 2015 at 09:21:38AM -0500, Peter Hurley wrote: >> On 02/19/2015 12:38 PM, Pantelis Antoniou wrote: >>> >>>> On Feb 19, 2015, at 19:30 , Frank Rowand wrote: >>>> >>>> On 2/19/2015 9:00 AM, Pantelis Antoniou wrote: >>>>> Hi Frank, >>>>> >>>>>> On Feb 19, 2015, at 18:48 , Frank Rowand wrote: >>>>>> >>>>>> On 2/19/2015 6:29 AM, Pantelis Antoniou wrote: >>>>>>> Hi Mark, >>>>>>> >>>>>>>> On Feb 18, 2015, at 19:31 , Mark Rutland wrote: >>>>>>>> >>>>>>>>>>> +While this may in theory work, in practice it is very cumbersome >>>>>>>>>>> +for the following reasons: >>>>>>>>>>> + >>>>>>>>>>> +1. The act of selecting a different boot device tree blob requires >>>>>>>>>>> +a reasonably advanced bootloader with some kind of configuration or >>>>>>>>>>> +scripting capabilities. Sadly this is not the case many times, the >>>>>>>>>>> +bootloader is extremely dumb and can only use a single dt blob. >>>>>>>>>> >>>>>>>>>> You can have several bootloader builds, or even a single build with >>>>>>>>>> something like appended DTB to get an appropriate DTB if the same >>>>>>>>>> binary >>>>>>>>>> will otherwise work across all variants of a board. >>>>>>>>>> >>>>>>>>> >>>>>>>>> No, the same DTB will not work across all the variants of a board. >>>>>>>> >>>>>>>> I wasn't on about the DTB. I was on about the loader binary, in the >>>>>>>> case >>>>>>>> the FW/bootloader could be common even if the DTB couldn't. >>>>>>>> >>>>>>>> To some extent there must be a DTB that will work across all variants >>>>>>>> (albeit with limited utility) or the quirk approach wouldn't work… >>>>>>>> >>>>>>> >>>>>>> That’s not correct; the only part of the DTB that needs to be common >>>>>>> is the model property that would allow the quirk detection logic to >>>>>>> fire. >>>>>>> >>>>>>> So, there is a base DTB that will work on all variants, but that only >>>>>>> means >>>>>>> that it will work only up to the point that the quirk detector method >>>>>>> can work. So while in recommended practice there are common subsets >>>>>>> of the DTB that might work, they might be unsafe. >>>>>>> >>>>>>> For instance on the beaglebone the regulator configuration is different >>>>>>> between white and black, it is imperative you get them right otherwise >>>>>>> you risk board damage. >>>>>>> >>>>>>>>>> So it's not necessarily true that you need a complex bootloader. >>>>>>>>>> >>>>>>>>> >>>>>>>>>>> +2. On many instances boot time is extremely critical; in some cases >>>>>>>>>>> +there are hard requirements like having working video feeds in >>>>>>>>>>> under >>>>>>>>>>> +2 seconds from power-up. This leaves an extremely small time >>>>>>>>>>> budget for >>>>>>>>>>> +boot-up, as low as 500ms to kernel entry. The sanest way to get >>>>>>>>>>> there >>>>>>>>>>> +is by removing the standard bootloader from the normal boot >>>>>>>>>>> sequence >>>>>>>>>>> +altogether by having a very small boot shim that loads the kernel >>>>>>>>>>> and >>>>>>>>>>> +immediately jumps to kernel, like falcon-boot mode in u-boot does. >>>>>>>>>> >>>>>>>>>> Given my previous comments above I don't see why this is relevant. >>>>>>>>>> You're a
Re: [PATCH 2/4] of: DT quirks infrastructure
uirks. >>>>>> >>>>>> I understand that each variant is somewhat incompatible (and hence needs >>>>>> its own DTB). >>>>> >>>>> In theory it might work, in practice this does not. Ludovic mentioned >>>>> that they >>>>> have 27 different DTBs in use at the moment. At a relatively common 60k >>>>> per DTB >>>>> that’s 27x60k = 1.6MB of DTBs, that need to be installed. >>>> >>>> < snip > >>>> >>>> Or you can install the correct DTB on the board. You trust your >>>> manufacturing line >>>> to install the correct resistors. You trust your manufacturing line to >>>> install the >>>> correct kernel version (eg an updated version to resolve a security issue). >>>> >>>> I thought the DT blob was supposed to follow the same standard that other >>>> OS's or >>>> bootloaders understood. Are you willing to break that? (This is one of >>>> those >>>> ripples I mentioned in my other emails.) >>>> >>> >>> Trust no-one. >>> >>> This is one of those things that the kernel community doesn’t understand >>> which makes people >>> who push product quite mad. >>> >>> Engineering a product is not only about meeting customer spec, in order to >>> turn a profit >>> the whole endeavor must be engineered as well for manufacturability. >>> >>> Yes, you can always manually install files in the bootloader. For 1 board >>> no problem. >>> For 10 doable. For 100 I guess you can hire an extra guy. For 1 million? >>> Guess what, >>> instead of turning a profit you’re losing money if you only have a few >>> cents of profit >>> per unit. >> >> I'm not installing physical components manually. Why would I be installing >> software >> manually? (rhetorical question) >> > > Because on high volume product runs the flash comes preprogrammed and is > soldered as is. > > Having a single binary to flash to every revision of the board makes > logistics considerably > easier. > > Having to boot and tweak the bootloader settings to select the correct dtb > (even if it’s present > on the flash medium) takes time and is error-prone. > > Factory time == money, errors == money. > >>> >>> No knobs to tweak means no knobs to break. And a broken knob can have >>> pretty bad consequences >>> for a few million units. >> >> And you produce a few million units before testing that the first one off >> the line works? >> > > The first one off the line works. The rest will get some burn in and > functional testing if you’re > lucky. In many cases where the product is very cheap it might make financial > sense to just ship > as is and deal with recalls, if you’re reasonably happy after a little bit of > statistical sampling. > > Hardware is hard :) I'm failing to see how this series improves your manufacturing process at all. 1. Won't you have to provide the factory with different eeprom images for the White and Black? You _trust_ them to get that right, or more likely, you have process control procedures in place so that you don't get 1 million Blacks flashed with the White eeprom image. 2. The White and Black use different memory technology so it's not as if the eMMC from the Black will end up on the White SMT line (or vice versa). 3 For that matter, why wouldn't you worry that all the microSD cards intended for the White were accidentally assembled with the first 50,000 Blacks; at that point you're losing a lot more than a few cents of profit. And that has nothing to do with what image you provided. 3. The factory is just as likely to use some other customer's image by accident, so you're just as likely to have the same failure rate if you have no test process at the factory. 4. If you're using offline programming, the image has to be tested after reflow anyway. IOW, your QA process will not change at all == same cost. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] of: DT quirks infrastructure
On 02/19/2015 09:41 AM, Pantelis Antoniou wrote: > Hi Frank, > >> On Feb 19, 2015, at 04:08 , Frank Rowand wrote: >> >> On 2/18/2015 6:59 AM, Pantelis Antoniou wrote: >>> Implement a method of applying DT quirks early in the boot sequence. >>> >>> A DT quirk is a subtree of the boot DT that can be applied to >>> a target in the base DT resulting in a modification of the live >>> tree. The format of the quirk nodes is that of a device tree overlay. >> >> The use of the word "quirk" is a different mental model for me than what >> this patch series appears to be addressing. I would suggest totally >> removing the word "quirk" from this proposal to avoid confusing the >> mental models of future generations of kernel folks. >> > > Naming things is hard to do. Suggestions? variant -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] tty: serial: Add 8250 earlycon to support noinit option
On 02/02/2015 12:33 AM, Eddie Huang wrote: > Hi Peter, > > On Sun, 2015-02-01 at 21:24 -0800, Greg Kroah-Hartman wrote: >> On Mon, Feb 02, 2015 at 12:15:31AM -0500, Peter Hurley wrote: >>> On 02/01/2015 11:28 PM, Greg Kroah-Hartman wrote: >>>> On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote: >>>>> On 02/01/2015 10:16 PM, Eddie Huang wrote: >>> >>>>> I'll respin proper patches on top of Greg's tty-testing branch with >>>>> reverts >>>>> for the noinit options. I noticed that one of the noinit patches actually >>>>> has the linkage for the mtk earlycon, so I'll be sure to preserve that. >>>> >>>> I can just drop the patches in the tty-testing branch, that's what it is >>>> there for :) >>>> >>>> Just let me know the specific patches and I will do so, thanks. >>> >>> Well that pretty much means dropping the 3 patches that add earlycon to >>> 8250_mtk and then applying my patch (needs fixed to apply cleanly, which >>> I can do) and then applying a fixed-up replacement patch to add earlycon >>> to 8250_mtk (which I can also supply). >>> >>> Is that the way you want to go? >> >> Sounds good to me, send your patch, and I'll fix it all up tomorrow. >> >> thanks, >> >> greg k-h > > Actually, your patch is a little different from my original idea. > Although my use case only care about divisor now, but other hardware > setting is still hard-code, not from parameter. In init_port() > function: > serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */ > serial8250_early_out(port, UART_IER, 0);/* no interrupt */ > serial8250_early_out(port, UART_FCR, 0); /* no fifo */ > serial8250_early_out(port, UART_MCR, 0x3); /* DTR + RTS */ > > This is why I propose a new option "noinit". > > After checking further, in my case, I found that your patch should be > unnecessary because if skip baudrate, probe_baud() read DLL/DLM register > and init_port() write the same DLL/DLM value back, no touch any high > speed register, which means keep uart divisor setting as loader > > Since I don't take "console=uart,mmio32,,noinit" into > consideration, it is good to drop my patches in the tty-testing branch. > For my case, I can send another series without noinit, just 8250_mtk.c > and its linkage modification in 8250_early.c Ok. Greg, The patches to drop from tty-testing are: * 405017d Document: Modify 8250 earlycon kernel parameters * 0dff3a4 tty: serial: 8250_mtk: Add earlycon * b829735 tty: serial: Add 8250 earlycon to support noinit option Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] tty: serial: Add 8250 earlycon to support noinit option
On 02/01/2015 11:28 PM, Greg Kroah-Hartman wrote: > On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote: >> On 02/01/2015 10:16 PM, Eddie Huang wrote: >> I'll respin proper patches on top of Greg's tty-testing branch with reverts >> for the noinit options. I noticed that one of the noinit patches actually >> has the linkage for the mtk earlycon, so I'll be sure to preserve that. > > I can just drop the patches in the tty-testing branch, that's what it is > there for :) > > Just let me know the specific patches and I will do so, thanks. Well that pretty much means dropping the 3 patches that add earlycon to 8250_mtk and then applying my patch (needs fixed to apply cleanly, which I can do) and then applying a fixed-up replacement patch to add earlycon to 8250_mtk (which I can also supply). Is that the way you want to go? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] tty: serial: Add 8250 earlycon to support noinit option
On 02/01/2015 10:16 PM, Eddie Huang wrote:
> Hi Peter,
>
> On Sun, 2015-02-01 at 13:26 -0500, Peter Hurley wrote:
>> On 02/01/2015 11:27 AM, Peter Hurley wrote:
>>> Hi Eddie,
>>>
>>> On 01/12/2015 08:08 AM, Eddie Huang wrote:
>>>> Add earlycon support not only baudrate option, but also add noinit option.
>>>> If use noinit option, 8250 earlycon will not init serial hardware and use
>>>> loader setting.
>>>
>>> I see this went into Greg's tty-testing branch.
>>>
>>> The only point of this is to not program the divisor, right?
>>>
>>> I ask because early_serial8250_setup() could already handle this without
>>> extra options by simply not doing divisor programming if no baud option is
>>> present.
>>
>> Does the patch below work for your use-case?
>>
>> [ Note: the patch applies to 3.19-rcX. To test, your noinit patches need to
>> be
>> reverted first.
>> ]
>>
>> --- >% ---
>> From: Peter Hurley
>> Subject: [PATCH] serial: 8250_early: Assume uart already initialized if no
>> baud option
>>
>> The option string is not supplied if the earlycon
>> is started via devicetree and OF_EARLYCON_DECLARE(). The option string
>> is also not required if started via kernel command line parameters of
>> the form:
>> earlycon=uart,mmio,
>> console=uart,mmio,
>>
>> If earlycon_device->baud is 0, then an option string was not supplied.
>> In this case, assume the uart has already been initialized by the
>> bootloader or firmware.
>>
>> Signed-off-by: Peter Hurley
>> ---
>> drivers/tty/serial/8250/8250_early.c | 10 --
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_early.c
>> b/drivers/tty/serial/8250/8250_early.c
>> index d7b831b..1701d00 100644
>> --- a/drivers/tty/serial/8250/8250_early.c
>> +++ b/drivers/tty/serial/8250/8250_early.c
>> @@ -149,12 +149,18 @@ static int __init early_serial8250_setup(struct
>> earlycon_device *device,
>> return 0;
>>
>> if (!device->baud) {
>> +struct uart_port *port = &device->port;
>> +unsigned int ier;
>> +
>> device->baud = probe_baud(&device->port);
>> snprintf(device->options, sizeof(device->options), "%u",
>> device->baud);
>> -}
>>
>> -init_port(device);
>> +/* assume the device was initialized, only mask interrupts */
>> +ier = serial8250_early_in(port, UART_IER);
>> +serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
>> +} else
>> +init_port(device);
> Should add brace in else.
I don't do that unless I have to.
> Where is original line here.
> early_device = device;
Whoops :)
I wrote the patch from a private branch which implements extensible console
matching (so a console can define its own match function) and a bunch of
other console cleanup and code removal. In that series, early_device becomes
unnecessary and is removed.
I'll respin proper patches on top of Greg's tty-testing branch with reverts
for the noinit options. I noticed that one of the noinit patches actually
has the linkage for the mtk earlycon, so I'll be sure to preserve that.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] tty: serial: Add 8250 earlycon to support noinit option
On 02/01/2015 09:45 PM, Eddie Huang wrote: > Hi Peter, > > On Sun, 2015-02-01 at 11:27 -0500, Peter Hurley wrote: >> Hi Eddie, >> >> On 01/12/2015 08:08 AM, Eddie Huang wrote: >>> Add earlycon support not only baudrate option, but also add noinit option. >>> If use noinit option, 8250 earlycon will not init serial hardware and use >>> loader setting. >> >> I see this went into Greg's tty-testing branch. >> >> The only point of this is to not program the divisor, right? > In this case, yes. > >> I ask because early_serial8250_setup() could already handle this without >> extra options by simply not doing divisor programming if no baud option is >> present. > MTK support high speed UART, which means baudrate can be more than > 115200. You can reference mtk8250_set_termios() function in 8250_mtk.c. > Unfortunately, the early_serial8250_setup() can not handle high speed > case. This is why I add noinit parameter. Thanks, that's what I thought; I just wanted to verify. > Besides, I think "no baud option" is a little tricky, and maybe someday > someone not only care about divisor, but also flow. Legacy earlyprintk > and other uart drivers like msm_serial.c also don't init uart hardware. My point is the 8250 earlycon should only be doing hardware initialization if there is an option string (of the form ), because if there's no baud option, programming the divisor is pointless. And to specify any other line control option the baud must be specified. So to program any other setting would require the proper option string. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] tty: serial: Add 8250 earlycon to support noinit option
On 02/01/2015 11:27 AM, Peter Hurley wrote:
> Hi Eddie,
>
> On 01/12/2015 08:08 AM, Eddie Huang wrote:
>> Add earlycon support not only baudrate option, but also add noinit option.
>> If use noinit option, 8250 earlycon will not init serial hardware and use
>> loader setting.
>
> I see this went into Greg's tty-testing branch.
>
> The only point of this is to not program the divisor, right?
>
> I ask because early_serial8250_setup() could already handle this without
> extra options by simply not doing divisor programming if no baud option is
> present.
Does the patch below work for your use-case?
[ Note: the patch applies to 3.19-rcX. To test, your noinit patches need to be
reverted first.
]
--- >% ---
From: Peter Hurley
Subject: [PATCH] serial: 8250_early: Assume uart already initialized if no
baud option
The option string is not supplied if the earlycon
is started via devicetree and OF_EARLYCON_DECLARE(). The option string
is also not required if started via kernel command line parameters of
the form:
earlycon=uart,mmio,
console=uart,mmio,
If earlycon_device->baud is 0, then an option string was not supplied.
In this case, assume the uart has already been initialized by the
bootloader or firmware.
Signed-off-by: Peter Hurley
---
drivers/tty/serial/8250/8250_early.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_early.c
b/drivers/tty/serial/8250/8250_early.c
index d7b831b..1701d00 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -149,12 +149,18 @@ static int __init early_serial8250_setup(struct
earlycon_device *device,
return 0;
if (!device->baud) {
+ struct uart_port *port = &device->port;
+ unsigned int ier;
+
device->baud = probe_baud(&device->port);
snprintf(device->options, sizeof(device->options), "%u",
device->baud);
- }
- init_port(device);
+ /* assume the device was initialized, only mask interrupts */
+ ier = serial8250_early_in(port, UART_IER);
+ serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
+ } else
+ init_port(device);
device->con->write = early_serial8250_write;
return 0;
--
2.2.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] tty: serial: Add 8250 earlycon to support noinit option
Hi Eddie,
On 01/12/2015 08:08 AM, Eddie Huang wrote:
> Add earlycon support not only baudrate option, but also add noinit option.
> If use noinit option, 8250 earlycon will not init serial hardware and use
> loader setting.
I see this went into Greg's tty-testing branch.
The only point of this is to not program the divisor, right?
I ask because early_serial8250_setup() could already handle this without
extra options by simply not doing divisor programming if no baud option is
present.
And this blows up if the optional console= form is used:
console=uart,mmio32,,noinit
because the ttyS console will expect line settings for console match.
Regards,
Peter Hurley
> Signed-off-by: Eddie Huang
> ---
> drivers/tty/serial/8250/8250_early.c | 7 ---
> drivers/tty/serial/earlycon.c| 17 -
> include/linux/serial_8250.h | 2 ++
> include/linux/serial_core.h | 1 +
> 4 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_early.c
> b/drivers/tty/serial/8250/8250_early.c
> index 4858b8a..a13d757 100644
> --- a/drivers/tty/serial/8250/8250_early.c
> +++ b/drivers/tty/serial/8250/8250_early.c
> @@ -138,19 +138,20 @@ static void __init init_port(struct earlycon_device
> *device)
> serial8250_early_out(port, UART_LCR, c & ~UART_LCR_DLAB);
> }
>
> -static int __init early_serial8250_setup(struct earlycon_device *device,
> +int __init early_serial8250_setup(struct earlycon_device *device,
>const char *options)
> {
> if (!(device->port.membase || device->port.iobase))
> return 0;
>
> - if (!device->baud) {
> + if (!device->baud && !device->noinit) {
> device->baud = probe_baud(&device->port);
> snprintf(device->options, sizeof(device->options), "%u",
>device->baud);
> }
>
> - init_port(device);
> + if (!device->noinit)
> + init_port(device);
>
> early_device = device;
> device->con->write = early_serial8250_write;
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index 64fe25a..4891251 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -58,7 +58,7 @@ static int __init parse_options(struct earlycon_device
> *device,
> char *options)
> {
> struct uart_port *port = &device->port;
> - int mmio, mmio32, length;
> + int noinit, mmio, mmio32, length;
> unsigned long addr;
>
> if (!options)
> @@ -92,10 +92,17 @@ static int __init parse_options(struct earlycon_device
> *device,
> options = strchr(options, ',');
> if (options) {
> options++;
> - device->baud = simple_strtoul(options, NULL, 0);
> - length = min(strcspn(options, " ") + 1,
> - (size_t)(sizeof(device->options)));
> - strlcpy(device->options, options, length);
> + noinit = !strncmp(options, "noinit", 6);
> + if (noinit) {
> + device->noinit = noinit;
> + strlcpy(device->options, options, 6);
> + device->options[6] = '\0';
> + } else {
> + device->baud = simple_strtoul(options, NULL, 0);
> + length = min(strcspn(options, " ") + 1,
> + (size_t)(sizeof(device->options)));
> + strlcpy(device->options, options, length);
> + }
> }
>
> if (port->iotype == UPIO_MEM || port->iotype == UPIO_MEM32)
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index e02acf0..0e26eec 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -119,6 +119,8 @@ extern int serial8250_find_port(struct uart_port *p);
> extern int serial8250_find_port_for_earlycon(void);
> extern unsigned int serial8250_early_in(struct uart_port *port, int offset);
> extern void serial8250_early_out(struct uart_port *port, int offset, int
> value);
> +extern int early_serial8250_setup(struct earlycon_device *device,
> + const char *options);
> extern int setup_early_serial8250_console(char *cmdline);
> extern void serial8250_do_set_termios(struct uart_port *port,
> struct ktermios *termios, struct ktermios *old);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>
Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
On 01/30/2015 10:49 AM, Russell King - ARM Linux wrote: > On Fri, Jan 30, 2015 at 10:32:54AM -0500, Peter Hurley wrote: >> Before you say consistency, I think you should look at the stats below. >> IOW, if you want to change the error code return from probe() for >> consistency's sake, a tree-wide patch would be the appropriate way. > > Now look outside the serial driver sub-tree. > > There are 1234 instances of platform_get_resource(, IORESOURCE_MEM, ) in > the drivers/ sub-tree, with 700 instances of devm_ioremap_resource() > being used there. Of the devm_ioremap_resource() instances: > > - 555 use platform_get_resource() in the preceding two lines - which is > not enough to do anything but rely on the -EINVAL return value. > - 16 mention ENODEV in the preceding three lines. > > There are 132 which use platform_get_resource() and return ENODEV within > the following three lines (which may intersect with the above 16 number) > and 88 which use EINVAL. > > So, there are in total 643 instances where a missing resource returns > EINVAL, and between 132 and 148 instances which return ENODEV. > > Yes, 643 + 148 isn't 1234, but I'm not going to read through all 1234 > locations just for the sake of this thread. What's clear though is that > more than 50% of sites using platform_get_resource(, IORESOURCE_MEM, ) > return EINVAL for the lack of a resource. Sure, now that they're using devm_ioremap_resource(). What about before they were converted? For example, of the 10 serial drivers now using devm_ioremap_resource(), _not 1 returned EINVAL_ prior to using devm_ioremap_resource(). And of those 10 commits, only 1 mentions changing the return codes on purpose. In fact, all of them but one returned ENODEV. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
On 01/30/2015 09:08 AM, Russell King - ARM Linux wrote: > On Fri, Jan 30, 2015 at 07:03:03AM -0500, Peter Hurley wrote: >> On 01/30/2015 05:18 AM, Thierry Reding wrote: >>> -ENODEV is certainly not the correct return value if a resource is not >>> available. It translates to "no such device", but the device must >>> clearly be there, otherwise the ->probe() shouldn't have been called. >> >> -ENODEV is the appropriate return from the probe(); there is no device. > > No it is not. "no such device" means "the device is not present". If > the device is not present, we wouldn't have a struct device associated > with it. > > The missing resource is an error condition: what it's saying is that the > device is there, but something has failed in providing the IO regions > necessary to access it. That's really something very different from > "there is no device present". This is masking behavior changes behind what is essentially a refactor. For example, here's Felipe's changelog to omap-serial: commit d044d2356f8dd18c755e13f34318bc03ef9c6887 Author: Felipe Balbi Date: Wed Apr 23 09:58:33 2014 -0500 tty: serial: omap: switch over to devm_ioremap_resource just using helper function to remove some duplicated code a bit. While at that, also move allocation of struct uart_omap_port higher in the code so that we return much earlier in case of no memory. Signed-off-by: Felipe Balbi Signed-off-by: Greg Kroah-Hartman No mention of correcting an error return value here. Why change the error return from probe() now? Before you say consistency, I think you should look at the stats below. IOW, if you want to change the error code return from probe() for consistency's sake, a tree-wide patch would be the appropriate way. >>> Or if it really isn't there, then you'd at least need a memory region >>> to probe, otherwise you can't determine whether it's there or not. So >>> from the perspective of a device driver I think a missing, or NULL, >>> resource, is indeed an "invalid argument". >> >> Trying to argue that a missing host bus window is an "invalid argument" >> to probe() is just trying to make the shoe fit. > > As is arguing that -ENODEV is an appropriate return value for the missing > resource. > > Moreover, returning -ENODEV is actually *bad* in this case - the kernel's > generic probe handling does not report the failure of the driver to bind > given this, so a missing resource potentially becomes a silent failure of > a driver - leading users to wonder why their devices aren't found. > > If we /really/ have a problem with the error code, then why not invent > a new error code to cater for this condition - maybe, EBADRES (bad resource). > >>> I understand that people might see some ambiguity there, and -EINVAL is >>> certainly not a very accurate description, but such is the nature of >>> error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been >>> under discussion as well if I remember correctly, but they both equally >>> ambiguous. -ENODATA might have been a better fit, but that too has a >>> different meaning in other contexts. >>> >>> Besides, there's an error message that goes along with the error code >>> return, that should remove any ambiguity for people looking at dmesg. >>> >>> All of that said, the original assertion that the check is not required >>> is still valid. We can argue at length about the specific error code but >>> if we decide that it needs to change, then we should modify >>> devm_ioremap_resource() rather than requiring all callers to perform the >>> extra check again. >> >> None of the devm_ioremap_resource() changes have been submitted for >> serial drivers. > > $ grep devm_ioremap_resource drivers/tty/serial/ -r | wc -l > 10 Ok, not 'none' but hardly tree-wide. And of those 10 drivers now using devm_ioremap_resource(), 3 drivers still return ENODEV for a missing resource [1]. (FWIW, I wrote 'none' on the basis of a grep of devm_ioremap_resource and looking at the last one, serial-tegra.c, which has exactly the construct objected to in the Spreadtrum driver). Another 9 drivers still use plain devm_ioremap(), even those with trivial conversions like samsung.c [2] Of the serial drivers which use platform_get_resource(), 10 return ENODEV 5 return EINVAL (not including those converted to devm_ioremap_resource()) 4 return ENXIO 3 return ENOMEM 2 return ENOENT 1 returns EBUSY 1 returns EFAULT So to recap, I see no reason to respin this dri
Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
On 01/30/2015 05:18 AM, Thierry Reding wrote: > On Thu, Jan 29, 2015 at 04:05:53PM +, Russell King - ARM Linux wrote: >> On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote: >>> Hi Varka, >>> >>> On 01/29/2015 10:26 AM, Varka Bhadram wrote: >>>> This check is not required. It will be done by devm_ioremap_resource() >>> >>> I disagree. devm_ioremap_resource() interprets the NULL resource as >>> a bad parameter and returns -EINVAL which is then forwarded as the >>> return value from the probe. >>> >>> -ENODEV is the correct return value from the probe if the expected >>> resource is not available (either because it doesn't exist or was already >>> claimed by another driver). >> >> (Adding Thierry as the author of this function.) >> >> I believe devm_ioremap_resource() was explicitly designed to remove such >> error handling from drivers, and to give drivers a unified error response >> to such things as missing resources. >> >> See the comments for this function in lib/devres.c. > > Right. Before the introduction of this function drivers would copy the > same boilerplate but would use completely inconsistent return codes. > Well, to be correct devm_request_and_ioremap() was introduced first to > reduce the boilerplate, but one of the problems was that it returned a > NULL pointer on failure, so it was impossible to distinguish between > specific error conditions. It also had the negative side-effect of not > giving drivers any directions on what to do with the NULL return value > other than the example in the kerneldoc. But most people just didn't > seem to look at that, so instead of using -EADDRNOTAVAIL they'd again > go and do completely inconsistent things everywhere. > > When we introduced devm_ioremap_resource(), the idea was to remove any > of these inconsistencies once and for all. You call the function and if > it fails you simply propagate the error code, so you get consistent > behaviour everywhere. > > If I remember correctly the error codes for the various conditions were > discussed quite extensively, and what we currently have is what we got > by concensus. > > -ENODEV is certainly not the correct return value if a resource is not > available. It translates to "no such device", but the device must > clearly be there, otherwise the ->probe() shouldn't have been called. -ENODEV is the appropriate return from the probe(); there is no device. That returning that value doesn't make sense from devm_ioremap_resource is simply a reflection of the awkward construct. > Or if it really isn't there, then you'd at least need a memory region > to probe, otherwise you can't determine whether it's there or not. So > from the perspective of a device driver I think a missing, or NULL, > resource, is indeed an "invalid argument". Trying to argue that a missing host bus window is an "invalid argument" to probe() is just trying to make the shoe fit. > I understand that people might see some ambiguity there, and -EINVAL is > certainly not a very accurate description, but such is the nature of > error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been > under discussion as well if I remember correctly, but they both equally > ambiguous. -ENODATA might have been a better fit, but that too has a > different meaning in other contexts. > > Besides, there's an error message that goes along with the error code > return, that should remove any ambiguity for people looking at dmesg. > > All of that said, the original assertion that the check is not required > is still valid. We can argue at length about the specific error code but > if we decide that it needs to change, then we should modify > devm_ioremap_resource() rather than requiring all callers to perform the > extra check again. None of the devm_ioremap_resource() changes have been submitted for serial drivers. I see no problem with accepting the Spreadtrum driver as is, and if someone wants to do a massive changeset to rework the platform_get_resource()/devm_ioremap_resource() idiom for serial drivers for 3.21, then the Spreadtrum driver would be included then. That said, I'd prefer to see that idiom wrapped in a single function rather than what's being suggested. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
On 01/29/2015 11:05 AM, Russell King - ARM Linux wrote: > On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote: >> Hi Varka, >> >> On 01/29/2015 10:26 AM, Varka Bhadram wrote: >>> This check is not required. It will be done by devm_ioremap_resource() >> >> I disagree. devm_ioremap_resource() interprets the NULL resource as >> a bad parameter and returns -EINVAL which is then forwarded as the >> return value from the probe. >> >> -ENODEV is the correct return value from the probe if the expected >> resource is not available (either because it doesn't exist or was already >> claimed by another driver). > > (Adding Thierry as the author of this function.) > > I believe devm_ioremap_resource() was explicitly designed to remove such > error handling from drivers, and to give drivers a unified error response > to such things as missing resources. > > See the comments for this function in lib/devres.c. Maybe the purpose would be better served by wrapping the idiom in a single function. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
On 01/29/2015 11:04 AM, Varka Bhadram wrote:
> Hi Peter,
>
> On Thursday 29 January 2015 09:19 PM, Peter Hurley wrote:
>> Hi Varka,
>>
>> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
>>> Hi,
>>>
>>> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>>> spreadtrum sharkl64 platform.
>>>> This driver also support earlycon.
>>>>
>>>> Originally-by: Lanqing Liu
>>>> Signed-off-by: Orson Zhai
>>>> Signed-off-by: Chunyan Zhang
>>>> Acked-by: Arnd Bergmann
>>>> Reviewed-by: Peter Hurley
>>>> ---
>>>> drivers/tty/serial/Kconfig | 18 +
>>>> drivers/tty/serial/Makefile |1 +
>>>> drivers/tty/serial/sprd_serial.c | 793
>>>> ++
>>>> include/uapi/linux/serial_core.h |3 +
>>>> 4 files changed, 815 insertions(+)
>>>> create mode 100644 drivers/tty/serial/sprd_serial.c
>>>>
>>> (...)
>>>
>>>> +static int sprd_probe(struct platform_device *pdev)
>>>> +{
>>>> +struct resource *res;
>>>> +struct uart_port *up;
>>>> +struct clk *clk;
>>>> +int irq;
>>>> +int index;
>>>> +int ret;
>>>> +
>>>> +for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>>>> +if (sprd_port[index] == NULL)
>>>> +break;
>>>> +
>>>> +if (index == ARRAY_SIZE(sprd_port))
>>>> +return -EBUSY;
>>>> +
>>>> +index = sprd_probe_dt_alias(index, &pdev->dev);
>>>> +
>>>> +sprd_port[index] = devm_kzalloc(&pdev->dev,
>>>> +sizeof(*sprd_port[index]), GFP_KERNEL);
>>>> +if (!sprd_port[index])
>>>> +return -ENOMEM;
>>>> +
>>>> +up = &sprd_port[index]->port;
>>>> +up->dev = &pdev->dev;
>>>> +up->line = index;
>>>> +up->type = PORT_SPRD;
>>>> +up->iotype = SERIAL_IO_PORT;
>>>> +up->uartclk = SPRD_DEF_RATE;
>>>> +up->fifosize = SPRD_FIFO_SIZE;
>>>> +up->ops = &serial_sprd_ops;
>>>> +up->flags = UPF_BOOT_AUTOCONF;
>>>> +
>>>> +clk = devm_clk_get(&pdev->dev, NULL);
>>>> +if (!IS_ERR(clk))
>>>> +up->uartclk = clk_get_rate(clk);
>>>> +
>>>> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +if (!res) {
>>>> +dev_err(&pdev->dev, "not provide mem resource\n");
>>>> +return -ENODEV;
>>>> +}
>>> This check is not required. It will be done by devm_ioremap_resource()
>> I disagree. devm_ioremap_resource() interprets the NULL resource as
>> a bad parameter and returns -EINVAL which is then forwarded as the
>> return value from the probe.
>>
>> -ENODEV is the correct return value from the probe if the expected
>> resource is not available (either because it doesn't exist or was already
>> claimed by another driver).
>
> Check on the resource happening with evm_ioremap_resource.
>
> Not necessary to check multiple times.
>
> I did series for all the drivers. see [1]
>
> [1]: https://lkml.org/lkml/2014/11/3/986 <https://lkml.org/lkml/2014/11/3/986>
That's a usb series. Did you do a serial driver series I missed?
I don't see anything related in Greg's tty-next tree...
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
Hi Varka,
On 01/29/2015 10:26 AM, Varka Bhadram wrote:
> Hi,
>
> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>> spreadtrum sharkl64 platform.
>> This driver also support earlycon.
>>
>> Originally-by: Lanqing Liu
>> Signed-off-by: Orson Zhai
>> Signed-off-by: Chunyan Zhang
>> Acked-by: Arnd Bergmann
>> Reviewed-by: Peter Hurley
>> ---
>> drivers/tty/serial/Kconfig | 18 +
>> drivers/tty/serial/Makefile |1 +
>> drivers/tty/serial/sprd_serial.c | 793
>> ++
>> include/uapi/linux/serial_core.h |3 +
>> 4 files changed, 815 insertions(+)
>> create mode 100644 drivers/tty/serial/sprd_serial.c
>>
> (...)
>
>> +static int sprd_probe(struct platform_device *pdev)
>> +{
>> +struct resource *res;
>> +struct uart_port *up;
>> +struct clk *clk;
>> +int irq;
>> +int index;
>> +int ret;
>> +
>> +for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>> +if (sprd_port[index] == NULL)
>> +break;
>> +
>> +if (index == ARRAY_SIZE(sprd_port))
>> +return -EBUSY;
>> +
>> +index = sprd_probe_dt_alias(index, &pdev->dev);
>> +
>> +sprd_port[index] = devm_kzalloc(&pdev->dev,
>> +sizeof(*sprd_port[index]), GFP_KERNEL);
>> +if (!sprd_port[index])
>> +return -ENOMEM;
>> +
>> +up = &sprd_port[index]->port;
>> +up->dev = &pdev->dev;
>> +up->line = index;
>> +up->type = PORT_SPRD;
>> +up->iotype = SERIAL_IO_PORT;
>> +up->uartclk = SPRD_DEF_RATE;
>> +up->fifosize = SPRD_FIFO_SIZE;
>> +up->ops = &serial_sprd_ops;
>> +up->flags = UPF_BOOT_AUTOCONF;
>> +
>> +clk = devm_clk_get(&pdev->dev, NULL);
>> +if (!IS_ERR(clk))
>> +up->uartclk = clk_get_rate(clk);
>> +
>> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +if (!res) {
>> +dev_err(&pdev->dev, "not provide mem resource\n");
>> + return -ENODEV;
>> +}
>
> This check is not required. It will be done by devm_ioremap_resource()
I disagree. devm_ioremap_resource() interprets the NULL resource as
a bad parameter and returns -EINVAL which is then forwarded as the
return value from the probe.
-ENODEV is the correct return value from the probe if the expected
resource is not available (either because it doesn't exist or was already
claimed by another driver).
Regards,
Peter Hurley
>> +up->mapbase = res->start;
>> +up->membase = devm_ioremap_resource(&pdev->dev, res);
>> +if (IS_ERR(up->membase))
>> +return PTR_ERR(up->membase);
>> +
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
On 01/27/2015 09:47 PM, Chunyan Zhang wrote: > Add a full sc9836-uart driver for SC9836 SoC which is based on the > spreadtrum sharkl64 platform. > This driver also support earlycon. Reviewed-by: Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
Hi Chunyan,
Minor but important fixes below.
And for the v9 version, please only use "To:" for
"Greg Kroah-Hartman "
All other recipients should only be Cc:
Regards,
Peter Hurley
On 01/27/2015 02:56 AM, Chunyan Zhang wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
[...]
> +static int sprd_probe_dt_alias(int index, struct device *dev)
> +{
> + struct device_node *np;
> + static bool seen_dev_with_alias;
> + static bool seen_dev_without_alias;
^^
delete these two lines; these were used for the message deleted in a
previous patch version.
> + int ret = index;
> +
> + if (!IS_ENABLED(CONFIG_OF))
> + return ret;
> +
> + np = dev->of_node;
> + if (!np)
> + return ret;
> +
> + ret = of_alias_get_id(np, "serial");
> + if (IS_ERR_VALUE(ret)) {
> + seen_dev_without_alias = true;
^
delete this line.
> + ret = index;
> + } else {
> + seen_dev_with_alias = true;
^^^
delete this line.
> + if (ret >= ARRAY_SIZE(sprd_port) || sprd_port[ret] != NULL) {
> + dev_warn(dev, "requested serial port %d not
> available.\n", ret);
> + ret = index;
> + }
> + }
Simplify the entire "if (IS_ERR_VALUE(ret))" statement to:
if (IS_ERR_VALUE(ret))
ret = index;
else if (ret >= ..) {
dev_warn(.);
ret = index;
}
> +
> + return ret;
> +}
> +
> +static int sprd_remove(struct platform_device *dev)
> +{
> + struct sprd_uart_port *sup = platform_get_drvdata(dev);
> +
> + if (sup) {
> + uart_remove_one_port(&sprd_uart_driver, &sup->port);
> + sprd_port[sup->port.line] = NULL;
> + sprd_ports_num--;
> + }
> +
> + if (!sprd_ports_num)
> + uart_unregister_driver(&sprd_uart_driver);
> +
> + return 0;
> +}
> +
> +static int sprd_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct uart_port *up;
> + struct clk *clk;
> + int irq;
> + int index;
> + int ret;
> +
> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
> + if (sprd_port[index] == NULL)
> + break;
> +
> + if (index == ARRAY_SIZE(sprd_port))
> + return -EBUSY;
> +
> + index = sprd_probe_dt_alias(index, &pdev->dev);
> +
> + sprd_port[index] = devm_kzalloc(&pdev->dev,
> + sizeof(*sprd_port[index]), GFP_KERNEL);
> + if (!sprd_port[index])
> + return -ENOMEM;
> +
> + pdev->id = index;
delete this line.
The platform device id cannot be assigned by the driver.
(This was left over from trying to fix sprd_suspend/sprd_resume
but that's fixed correctly now.)
> +
> + up = &sprd_port[index]->port;
> + up->dev = &pdev->dev;
> + up->line = index;
> + up->type = PORT_SPRD;
> + up->iotype = SERIAL_IO_PORT;
> + up->uartclk = SPRD_DEF_RATE;
> + up->fifosize = SPRD_FIFO_SIZE;
> + up->ops = &serial_sprd_ops;
> + up->flags = UPF_BOOT_AUTOCONF;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(clk))
> + up->uartclk = clk_get_rate(clk);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "not provide mem resource\n");
> + return -ENODEV;
> + }
> + up->mapbase = res->start;
> + up->membase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(up->membase))
> + return PTR_ERR(up->membase);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "not provide irq resource\n");
> + return -ENODEV;
> + }
> + up->irq = irq;
> +
> + if (!sprd_ports_num) {
> + ret = uart_register_driver(&sprd_uart_driver);
> + if (ret < 0) {
> + pr_err("Failed to register SPRD-UART driver\n");
> + return ret;
> + }
> + }
> + sprd_ports_num++;
> +
> + ret = uart_add_one_port(&sprd_uart_driver, up);
> + if (ret) {
> + sprd_port[index] = NULL;
> + sprd_remove(pdev);
> + }
> +
> + platform_set_drvdata(pdev, up);
> +
> + return ret;
> +}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
Hi Chunyan,
Just the minor fix to zeroing the stack local in sprd_set_termios()
and using dev_get_drvdata() in sprd_suspend()/sprd_resume().
Regards,
Peter Hurley
On 01/23/2015 08:01 AM, Chunyan Zhang wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
[...]
> +static void sprd_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + struct ktermios *old)
> +{
> + unsigned int baud, quot;
> + unsigned int lcr, fc;
unsigned int lcr = 0, fc;
> + unsigned long flags;
> +
> + /* ask the core to calculate the divisor for us */
> + baud = uart_get_baud_rate(port, termios, old, 0, SPRD_BAUD_IO_LIMIT);
> +
> + quot = (unsigned int)((port->uartclk + baud / 2) / baud);
> +
> + /* set data length */
> + switch (termios->c_cflag & CSIZE) {
> + case CS5:
> + lcr |= SPRD_LCR_DATA_LEN5;
> + break;
> + case CS6:
> + lcr |= SPRD_LCR_DATA_LEN6;
> + break;
> + case CS7:
> + lcr |= SPRD_LCR_DATA_LEN7;
> + break;
> + case CS8:
> + default:
> + lcr |= SPRD_LCR_DATA_LEN8;
> + break;
> + }
> +
> + /* calculate stop bits */
> + lcr &= ~(SPRD_LCR_STOP_1BIT | SPRD_LCR_STOP_2BIT);
> + if (termios->c_cflag & CSTOPB)
> + lcr |= SPRD_LCR_STOP_2BIT;
> + else
> + lcr |= SPRD_LCR_STOP_1BIT;
> +
> + /* calculate parity */
> + lcr &= ~SPRD_LCR_PARITY;
> + termios->c_cflag &= ~CMSPAR;/* no support mark/space */
> + if (termios->c_cflag & PARENB) {
> + lcr |= SPRD_LCR_PARITY_EN;
> + if (termios->c_cflag & PARODD)
> + lcr |= SPRD_LCR_ODD_PAR;
> + else
> + lcr |= SPRD_LCR_EVEN_PAR;
> + }
[...]
> +static int sprd_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct uart_port *up;
> + struct clk *clk;
> + int irq;
> + int index;
> + int ret;
> +
> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
> + if (sprd_port[index] == NULL)
> + break;
> +
> + if (index == ARRAY_SIZE(sprd_port))
> + return -EBUSY;
> +
> + index = sprd_probe_dt_alias(index, &pdev->dev);
> +
> + sprd_port[index] = devm_kzalloc(&pdev->dev,
> + sizeof(*sprd_port[index]), GFP_KERNEL);
> + if (!sprd_port[index])
> + return -ENOMEM;
> +
> + pdev->id = index;
see my recent reply regarding this and sprd_suspend/sprd_resume.
> +
> + up = &sprd_port[index]->port;
> + up->dev = &pdev->dev;
> + up->line = index;
> + up->type = PORT_SPRD;
> + up->iotype = SERIAL_IO_PORT;
> + up->uartclk = SPRD_DEF_RATE;
> + up->fifosize = SPRD_FIFO_SIZE;
> + up->ops = &serial_sprd_ops;
> + up->flags = UPF_BOOT_AUTOCONF;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(clk))
> + up->uartclk = clk_get_rate(clk);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "not provide mem resource\n");
> + return -ENODEV;
> + }
> + up->mapbase = res->start;
> + up->membase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(up->membase))
> + return PTR_ERR(up->membase);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "not provide irq resource\n");
> + return -ENODEV;
> + }
> + up->irq = irq;
> +
> + if (!sprd_ports_num) {
> + ret = uart_register_driver(&sprd_uart_driver);
> + if (ret < 0) {
> + pr_err("Failed to register SPRD-UART driver\n");
> + return ret;
> + }
> + }
> + sprd_ports_num++;
> +
> + ret = uart_add_one_port(&sprd_uart_driver, up);
> + if (ret) {
> + sprd_port[index] = NULL;
> + sprd_remove(pdev);
> + }
> +
> + platform_set_drvdata(pdev, up);
> +
> + return ret;
> +}
> +
> +static int sprd_suspend(struct device *dev)
> +{
> + int id = to_platform_device(dev)->id;
> + struct uart_port *port = &sprd_port[id]->port;
> +
> + uart_suspend_port(&sprd_uart_driver, port);
same comment: see my recent reply using dev_get_drvdata().
> +
> + return 0;
> +}
> +
> +static int sprd_resume(struct device *dev)
> +{
> + int id = to_platform_device(dev)->id;
> + struct uart_port *port = &sprd_port[id]->port;
> +
> + uart_resume_port(&sprd_uart_driver, port);
> +
> + return 0;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
On 01/23/2015 02:23 AM, Lyra Zhang wrote:
> Hi, Peter
>
> Many thanks to you for reviewing so carefully and giving us so many
> suggestions and so clear explanations.
:)
> I'll address all of your comments and send an updated patch soon.
>
> On Fri, Jan 23, 2015 at 11:57 AM, Peter Hurley
> wrote:
[...]
>>> +static void sprd_set_termios(struct uart_port *port,
>>> + struct ktermios *termios,
>>> + struct ktermios *old)
>>> +{
>>> + unsigned int baud, quot;
>>> + unsigned int lcr, fc;
>>> + unsigned long flags;
>>> +
>>> + /* ask the core to calculate the divisor for us */
>>> + baud = uart_get_baud_rate(port, termios, old, 1200, 300);
>> ^^
>>mabye derive these from uartclk?
>
> I'm afraid I can't understand very clearly, Could you explain more
> details please?
Is the fixed clock divider == 8 and the uartclk == 2600 ?
If so,
baud = uartclk / 8 = 325
I see now this is clamping baud inside the maximum, so this is fine.
Please disregard my comment.
[...]
>>> +static int sprd_probe(struct platform_device *pdev)
>>> +{
>>> + struct resource *res;
>>> + struct uart_port *up;
>>> + struct clk *clk;
>>> + int irq;
>>> + int index;
>>> + int ret;
>>> +
>>> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>>> + if (sprd_port[index] == NULL)
>>> + break;
>>> +
>>> + if (index == ARRAY_SIZE(sprd_port))
>>> + return -EBUSY;
>>> +
>>> + index = sprd_probe_dt_alias(index, &pdev->dev);
>>> +
>>> + sprd_port[index] = devm_kzalloc(&pdev->dev,
>>> + sizeof(*sprd_port[index]), GFP_KERNEL);
>>> + if (!sprd_port[index])
>>> + return -ENOMEM;
>>> +
>>> + up = &sprd_port[index]->port;
>>> + up->dev = &pdev->dev;
>>> + up->line = index;
>>> + up->type = PORT_SPRD;
>>> + up->iotype = SERIAL_IO_PORT;
>>> + up->uartclk = SPRD_DEF_RATE;
>>> + up->fifosize = SPRD_FIFO_SIZE;
>>> + up->ops = &serial_sprd_ops;
>>> + up->flags = ASYNC_BOOT_AUTOCONF;
>> ^
>> UPF_BOOT_AUTOCONF
>>
>> sparse will catch errors like this. See Documentation/sparse.txt
>
> you mean we should use UPF_BOOT_AUTOCONF, right?
Yes. Only UPF_* flag definitions should be used with the uart_port.flags
field.
My comment regarding the sparse tool and documentation is because the
flags field and UPF_* definitions use a type mechanism to generate
warnings using the sparse tool if regular integer values are used
with the flags field.
The type mechanism was specifically introduced to catch using ASYNC_*
definitions with the uart_port.flags field.
[...]
>>> +static int sprd_suspend(struct device *dev)
>>> +{
>>> + int id = to_platform_device(dev)->id;
>>> + struct uart_port *port = &sprd_port[id]->port;
>>
>> I'm a little confused regarding the port indexing;
>> is platform_device->id == line ? Where did that happen?
>>
>
> Oh, I'll change to assign platform_device->id with port->line in probe()
I apologize; I should have made my comment clearer.
The ->id should not be assigned.
Replace
int id = to_platform_device(dev)->id;
struct uart_port *port = &sprd_port[id]->port;
uart_suspend_port(&sprd_uart_driver, port);
with
struct sprd_uart_port *sup = dev_get_drvdata(dev);
uart_suspend_port(&sprd_uart_driver, &sup->port);
I know it's not obvious but platform_get/set_drvdata() is really
dev_get/set_drvdata() using the embedded struct device dev.
>
>>
>>> +
>>> + uart_suspend_port(&sprd_uart_driver, port);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_resume(struct device *dev)
>>> +{
>>> + int id = to_platform_device(dev)->id;
>>> + struct uart_port *port = &sprd_port[id]->port;
>>> +
>>> + uart_resume_port(&sprd_uart_driver, port);
same here
>>> + return 0;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
Hi Chunyan, On 01/22/2015 08:35 AM, Chunyan Zhang wrote: > Add a full sc9836-uart driver for SC9836 SoC which is based on the > spreadtrum sharkl64 platform. > This driver also support earlycon. Looking good. Comments below. > This patch also replaced the spaces between the macros and their > values with the tabs in serial_core.h Notes about patch changes goes... > > Originally-by: Lanqing Liu > Signed-off-by: Orson Zhai > Signed-off-by: Chunyan Zhang > --- ...here. For example, * Replaced spaces with tab for PORT_SPRD define in serial_core.h > drivers/tty/serial/Kconfig | 18 + > drivers/tty/serial/Makefile |1 + > drivers/tty/serial/sprd_serial.c | 801 > ++ > include/uapi/linux/serial_core.h |3 + > 4 files changed, 823 insertions(+) > create mode 100644 drivers/tty/serial/sprd_serial.c > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index c79b43c..13211f7 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -1577,6 +1577,24 @@ config SERIAL_MEN_Z135 > This driver can also be build as a module. If so, the module will be > called > men_z135_uart.ko > > +config SERIAL_SPRD > + tristate "Support for Spreadtrum serial" > + depends on ARCH_SPRD > + select SERIAL_CORE > + help > + This enables the driver for the Spreadtrum's serial. > + > +config SERIAL_SPRD_CONSOLE > + bool "Spreadtrum UART console support" > + depends on SERIAL_SPRD=y > + select SERIAL_CORE_CONSOLE > + select SERIAL_EARLYCON > + help > + Support for early debug console using Spreadtrum's serial. This > enables > + the console before standard serial driver is probed. This is enabled > + with "earlycon" on the kernel command line. The console is > + enabled when early_param is processed. > + > endmenu > > config SERIAL_MCTRL_GPIO > diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile > index 9a548ac..4801aca 100644 > --- a/drivers/tty/serial/Makefile > +++ b/drivers/tty/serial/Makefile > @@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC)+= arc_uart.o > obj-$(CONFIG_SERIAL_RP2) += rp2.o > obj-$(CONFIG_SERIAL_FSL_LPUART) += fsl_lpuart.o > obj-$(CONFIG_SERIAL_MEN_Z135)+= men_z135_uart.o > +obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o > > # GPIOLIB helpers for modem control lines > obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o > diff --git a/drivers/tty/serial/sprd_serial.c > b/drivers/tty/serial/sprd_serial.c > new file mode 100644 > index 000..fd98541 > --- /dev/null > +++ b/drivers/tty/serial/sprd_serial.c > @@ -0,0 +1,801 @@ > +/* > + * Copyright (C) 2012-2015 Spreadtrum Communications Inc. > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* device name */ > +#define UART_NR_MAX 8 > +#define SPRD_TTY_NAME"ttyS" > +#define SPRD_FIFO_SIZE 128 > +#define SPRD_DEF_RATE2600 > +#define SPRD_TIMEOUT 2048 > + > +/* the offset of serial registers and BITs for them */ > +/* data registers */ > +#define SPRD_TXD 0x > +#define SPRD_RXD 0x0004 > + > +/* line status register and its BITs */ > +#define SPRD_LSR 0x0008 > +#define SPRD_LSR_OE BIT(4) > +#define SPRD_LSR_FE BIT(3) > +#define SPRD_LSR_PE BIT(2) > +#define SPRD_LSR_BI BIT(7) > +#define SPRD_LSR_TX_OVER BIT(15) > + > +/* data number in TX and RX fifo */ > +#define SPRD_STS10x000C > + > +/* interrupt enable register and its BITs */ > +#define SPRD_IEN 0x0010 > +#define SPRD_IEN_RX_FULL BIT(0) > +#define SPRD_IEN_TX_EMPTYBIT(1) > +#define SPRD_IEN_BREAK_DETECTBIT(7) > +#define SPRD_IEN_TIMEOUT BIT(13) > + > +/* interrupt clear register */ > +#define SPRD_ICLR0x0014 > + > +/* line control register */ > +#define SPRD_LCR 0x0018 > +#define SPRD_LCR_STOP_1BIT 0x10 > +#define SPRD_LCR_STOP_2BIT 0x30 > +#define SPRD_LCR_DATA_LEN(BIT(2) | BIT(3)) > +#define SPRD_LCR_DATA_LEN5 0x0 > +#define SPRD_LCR_DATA_LEN6 0x4 > +#define SPRD_LCR_DATA_LEN7 0x8 > +#define SPRD_LCR_DATA_LEN8 0xc > +#define SPRD_LCR_PARITY (BIT(0) | BIT(1)) > +#define SPRD_LCR_PARITY_EN 0x2 > +#define SP
Re: [PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support
p_tx,
> + .start_tx = sprd_start_tx,
> + .stop_rx = sprd_stop_rx,
> + .break_ctl = sprd_break_ctl,
> + .startup = sprd_startup,
> + .shutdown = sprd_shutdown,
> + .set_termios = sprd_set_termios,
> + .type = sprd_type,
> + .release_port = sprd_release_port,
> + .request_port = sprd_request_port,
> + .config_port = sprd_config_port,
> + .verify_port = sprd_verify_port,
> +};
> +
> +#ifdef CONFIG_SERIAL_SPRD_CONSOLE
> +static inline void wait_for_xmitr(struct uart_port *port)
> +{
> + unsigned int status, tmout = 1;
> +
> + /* wait up to 10ms for the character(s) to be sent */
> + do {
> + status = serial_in(port, SPRD_STS1);
> + if (--tmout == 0)
> + break;
> + udelay(1);
> + } while (status & 0xff00);
> +}
> +
> +static void sprd_console_putchar(struct uart_port *port, int ch)
> +{
> + wait_for_xmitr(port);
> + serial_out(port, SPRD_TXD, ch);
> +}
> +
> +static void sprd_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *port = &sprd_port[co->index]->port;
> + int ien;
> + int locked = 1;
> +
> + if (oops_in_progress)
> + locked = spin_trylock(&port->lock);
> + else
> + spin_lock(&port->lock);
If you do need to take the port->lock in your isr, then you need to
disable local irq here.
> + /* save the IEN then disable the interrupts */
> + ien = serial_in(port, SPRD_IEN);
> + serial_out(port, SPRD_IEN, 0x0);
> +
> + uart_console_write(port, s, count, sprd_console_putchar);
> +
> + /* wait for transmitter to become empty and restore the IEN */
> + wait_for_xmitr(port);
> + serial_out(port, SPRD_IEN, ien);
> + if (locked)
> + spin_unlock(&port->lock);
> +}
> +
> +static int __init sprd_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *port;
> + int baud = 115200;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> +
> + if (co->index >= UART_NR_MAX || co->index < 0)
> + co->index = 0;
> +
> + port = &sprd_port[co->index]->port;
> + if (port == NULL) {
> + pr_info("serial port %d not yet initialized\n", co->index);
> + return -ENODEV;
> + }
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(port, co, baud, parity, bits, flow);
> +}
> +
> +static struct uart_driver sprd_uart_driver;
> +static struct console sprd_console = {
> + .name = SPRD_TTY_NAME,
> + .write = sprd_console_write,
> + .device = uart_console_device,
> + .setup = sprd_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> + .data = &sprd_uart_driver,
> +};
> +
> +#define SPRD_CONSOLE (&sprd_console)
> +
> +/* Support for earlycon */
> +static void sprd_putc(struct uart_port *port, int c)
> +{
> + unsigned int timeout = SPRD_TIMEOUT;
> +
> + while (timeout-- &&
> +!(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
> + cpu_relax();
> +
> + writeb(c, port->membase + SPRD_TXD);
> +}
> +
> +static void sprd_early_write(struct console *con, const char *s,
> + unsigned n)
> +{
> + struct earlycon_device *dev = con->data;
> +
> + uart_console_write(&dev->port, s, n, sprd_putc);
> +}
> +
> +static int __init sprd_early_console_setup(
> + struct earlycon_device *device,
> + const char *opt)
> +{
> + if (!device->port.membase)
> + return -ENODEV;
> +
> + device->con->write = sprd_early_write;
> + return 0;
> +}
> +
> +EARLYCON_DECLARE(sprd_serial, sprd_early_console_setup);
> +OF_EARLYCON_DECLARE(sprd_serial, "sprd,sc9836-uart",
> + sprd_early_console_setup);
> +
> +#else /* !CONFIG_SERIAL_SPRD_CONSOLE */
> +#define SPRD_CONSOLE NULL
> +#endif
> +
> +static struct uart_driver sprd_uart_driver = {
> + .owner = THIS_MODULE,
> + .driver_name = "sprd_serial",
> + .dev_name = SPRD_TTY_NAME,
> + .major = 0,
> + .minor = 0,
> + .nr = UART_NR_MAX,
> + .cons = SPRD_CONSOLE,
> +};
> +
> +static int sprd_probe(struct platform_device *pdev)
> +{
> +
Re: [PATCH 1/3] TTY: add support for "tty slave" devices.
On 12/13/2014 09:23 AM, One Thousand Gnomes wrote: > On Fri, 12 Dec 2014 08:02:48 -0500 >> Which brings up another point: why not do this in the runtime power >> management; >> ie, turn on the slave device when the master device is turned on? Sebastian >> recently made the 8250 driver power-managed per access, which would enable >> significant power savings over just leaving the slave device on the whole >> time. > > That per access model only works on 8250 (in fact only on some 8250). On > most devices if the UART is in low power you lose bytes send to it rather > than then queuing up anywhere. > > It doesn't handle cases where you need to keep power on to different rules > (for example there are cases where you do things like power the uart down > after no bits have been received for 'n' millseconds). Right now if you > look into Androidspace you'll find people doing this in userspace by > having the sysfs node open and playing crazy games with sysfs, Android > glue hacks and gpio poking via the gpio sysfs/gpio lib routines. > > Good example is some of the GPS and serial based sensor chips. The > latency of tty open/close is unacceptable and risks losing bits so they > keep the tty open and whack the power management by hand right now. > > The kind of thing many of them want is stuff like > > "hold power on until we see receive data, then keep it on until it shuts > up for a bit" > > "assert gpio, send, receive, wait idle, drop gpio" > > "on GPIO interrupt wake uart, wait for data, when idle, power uart down" > >>>> 2) why is this tied to the tty core and not the serial core >>>>if this is only for UART? >>> >>> Because the knowledge of "the device is being opened" or "the device is >>> being >>> closed" seems to exist in the tty core but not in the serial core. >> >> uart_open() = device file is being opened >> uart_close() = device file is being released > > Only for a subset of uart type devices that use the uart layer. Quite a > few don't, including all the USB ones, and the sd ones. > >>> I'm happy to move it to serial_core.c if that is preferred. >>> I'll also move the open/close handling if you can point to where it should >>> go. >> >> Yes, please. > > NAK - I certainly object to it being moved to uart. That's the wrong > abstraction layer for this. It's a generic behaviour, it's a tty level > behaviour without any uart specific properties. It should work for uarts > that don't use the legacy serial_core code, uarts that do, uarts that use > USB, uarts that are directly connected to things like sdio etc. > > That means it has to be tty layer. I was reviewing the patch submitted, which does none of the things you outlined and does not provide the necessary infrastructure to build upon it. I'm all for the functionality you describe but I think that design will end up subsystem-agnostic. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] TTY: add support for "tty slave" devices.
On 12/12/2014 12:23 AM, NeilBrown wrote:
> On Thu, 11 Dec 2014 18:18:37 -0500 Peter Hurley
> wrote:
>
>> On 12/11/2014 04:59 PM, NeilBrown wrote:
>>> A "tty slave" is a device connected via UART.
>>> It may need a driver to, for example, power the device on
>>> when the tty is opened, and power it off when the tty
>>> is released.
>>>
>>> A "tty slave" is a platform device which is declared as a
>>> child of the uart in device-tree:
>>>
>>> &uart1 {
>>> bluetooth {
>>> compatible = "tty,regulator";
>>> vdd-supply = <&vaux4>;
>>> };
>>> };
>>>
>>> runtime power management is used to power-up the device
>>> on tty_open() and power-down on tty_release().
>>
>> I have a couple of issues with this:
>> 1) why does a child device imply power management and a platform
>>device? iow, what happens when someone else wants to have a
>>child device that does something different?
>
> Why does it imply power management?
> Because it seems to make obvious sense to turn something on when the tty is
> open. If the device has other reason to remain on when the tty is closed,
> it can arrange for extra references to be taken of course.
> Is it conceivable that you would want the device to remain off when the
> tty device is open? In that case just make it a regular platform device.
>
> Why a platform device?
> Things on an i2c bus are i2c devices. Things on a usb bus are usb devices.
> Ideally a thing on a uart 'bus' would be a 'uart device', but no such thing
> exists. I did contemplate the possibility of creating an explicit "uart" or
> "tty" bus type, but I could find no value in that.
> The door is certainly still open to that possibility if a meaning for the
> idea becomes apparent. As far as device tree is concerned it is just a
> child device node. The fact that it is implemented as a "platform" device
> could easily be changed later if needed without device tree noticing.
>
> What could conceivably want to be different? The only (purely hypothetical)
> concept I can come up with which wouldn't fit is a device with both an UART
> port and a USB port, or similar. However device tree, and the device model
> in general, just isn't geared towards devices being on multiple buses so
> if that were real it would have much greater implications that just here.
>
> But maybe I misunderstand...
Yeah, misunderstanding.
This patch is using a very generic abstraction (parent-child device association)
for a really specific purpose (power management for platform devices).
What about PCI, PNP, etc.?
Also, what happens for existing child device associations like bluetooth &
serio?
Why not just provide a serial core interface for associating power-managed
child devices?
Which brings up another point: why not do this in the runtime power management;
ie, turn on the slave device when the master device is turned on? Sebastian
recently made the 8250 driver power-managed per access, which would enable
significant power savings over just leaving the slave device on the whole time.
>> 2) why is this tied to the tty core and not the serial core
>>if this is only for UART?
>
> Because the knowledge of "the device is being opened" or "the device is being
> closed" seems to exist in the tty core but not in the serial core.
uart_open() = device file is being opened
uart_close() = device file is being released
> The "of_platform_populate()" call could certainly go in serial_core.c, in
> uart_add_one_port() I think. I did have it there originally. I moved it for
> a reason that I think is no longer relevant. As the on/off code is (and I
> think has to be) in tty_io.c, I left all of it there.
>
> I'm happy to move it to serial_core.c if that is preferred.
> I'll also move the open/close handling if you can point to where it should go.
Yes, please.
The reverse dependency (tty core => of) is undesirable.
Regards,
Peter Hurley
>>> Signed-off-by: NeilBrown
>>> ---
>>> .../devicetree/bindings/serial/of-serial.txt |4
>>> drivers/tty/tty_io.c | 22
>>>
>>> 2 files changed, 26 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt
>>> b/Documentation/devicetree/bindings/serial/of-serial.txt
>>> index 8c4fd0332028..b59501ee2f21 100644
>>> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
>>&
Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.
On 12/12/2014 12:27 AM, NeilBrown wrote: > On Thu, 11 Dec 2014 18:32:52 -0500 Peter Hurley > wrote: > >> On 12/11/2014 04:59 PM, NeilBrown wrote: >>> The regulator is identified in devicetree as 'vdd-supply' >>> >>> Signed-off-by: NeilBrown >> >> tty_* is only for tty core functions/data. > > I think you are just objecting to the function and type names - is that > correct? Yeah, exactly. > I suspect I can come up with something different. Thanks. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] TTY: add slave driver to power-on device via a regulator.
On 12/11/2014 04:59 PM, NeilBrown wrote:
> The regulator is identified in devicetree as 'vdd-supply'
>
> Signed-off-by: NeilBrown
tty_* is only for tty core functions/data.
Regards,
Peter Hurley
> ---
> .../devicetree/bindings/serial/slave-reg.txt | 18
> drivers/tty/Kconfig|2
> drivers/tty/Makefile |1
> drivers/tty/slaves/Kconfig | 12 ++
> drivers/tty/slaves/Makefile|2
> drivers/tty/slaves/tty-reg.c | 102
>
> 6 files changed, 137 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/serial/slave-reg.txt
> create mode 100644 drivers/tty/slaves/Kconfig
> create mode 100644 drivers/tty/slaves/Makefile
> create mode 100644 drivers/tty/slaves/tty-reg.c
>
> diff --git a/Documentation/devicetree/bindings/serial/slave-reg.txt
> b/Documentation/devicetree/bindings/serial/slave-reg.txt
> new file mode 100644
> index ..7896bce8dfe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-reg.txt
> @@ -0,0 +1,18 @@
> +Regulator powered UART-attached device
> +
> +Required properties:
> +- compatible: "tty,regulator"
> +- vdd-supply: regulator to power the device
> +
> +
> +This is listed as a child node of a UART. When the
> +UART is opened, the device is powered.
> +
> +Example:
> +
> +&uart1 {
> + bluetooth {
> + compatible = "tty,regulator";
> + vdd-supply = <&vaux4>;
> + };
> +};
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index b24aa010f68c..ea3383c71701 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -419,4 +419,6 @@ config DA_CONSOLE
> help
> This enables a console on a Dash channel.
>
> +source drivers/tty/slaves/Kconfig
> +
> endif # TTY
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 58ad1c05b7f8..45957d671e33 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_R3964) += n_r3964.o
> obj-y+= vt/
> obj-$(CONFIG_HVC_DRIVER) += hvc/
> obj-y+= serial/
> +obj-$(CONFIG_TTY_SLAVE) += slaves/
>
> # tty drivers
> obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
> diff --git a/drivers/tty/slaves/Kconfig b/drivers/tty/slaves/Kconfig
> new file mode 100644
> index ..2dd1acf80f8c
> --- /dev/null
> +++ b/drivers/tty/slaves/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig TTY_SLAVE
> + bool "TTY slave devices"
> + help
> + Devices which attach via a uart, but need extra
> + driver support for power management etc.
> +
> +if TTY_SLAVE
> +
> +config TTY_SLAVE_REGULATOR
> + tristate "TTY slave device powered by regulator"
> +
> +endif
> diff --git a/drivers/tty/slaves/Makefile b/drivers/tty/slaves/Makefile
> new file mode 100644
> index ..e636bf49f1cc
> --- /dev/null
> +++ b/drivers/tty/slaves/Makefile
> @@ -0,0 +1,2 @@
> +
> +obj-$(CONFIG_TTY_SLAVE_REGULATOR) += tty-reg.o
> diff --git a/drivers/tty/slaves/tty-reg.c b/drivers/tty/slaves/tty-reg.c
> new file mode 100644
> index ..613f8b10967c
> --- /dev/null
> +++ b/drivers/tty/slaves/tty-reg.c
> @@ -0,0 +1,102 @@
> +/*
> + * tty-reg:
> + * Support for any device which needs a regulator turned on
> + * when a tty is opened.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct tty_reg_data {
> + struct regulator *reg;
> + boolreg_enabled;
> +};
> +
> +static int tty_reg_runtime_resume(struct device *slave)
> +{
> + struct tty_reg_data *data = dev_get_drvdata(slave);
> + if (!data->reg_enabled &&
> + regulator_enable(data->reg) == 0) {
> + dev_dbg(slave, "power on\n");
> + data->reg_enabled = true;
> + }
> + return 0;
> +}
> +
> +static int tty_reg_runtime_suspend(struct device *slave)
> +{
> + struct tty_reg_data *data = dev_get_drvdata(slave);
> +
> + if (data->reg_enabled &&
> + regulator_disable(data->reg) == 0) {
> + dev_dbg(slave, "power off\n");
> + data->reg_enabled = false;
> + }
> + return 0;
> +}
> +
> +static int tty_reg_probe(struct platform_device *pdev)
> +{
> + struct tty_reg_data *data;
> + struct regulator *re
Re: [PATCH 1/3] TTY: add support for "tty slave" devices.
On 12/11/2014 04:59 PM, NeilBrown wrote:
> A "tty slave" is a device connected via UART.
> It may need a driver to, for example, power the device on
> when the tty is opened, and power it off when the tty
> is released.
>
> A "tty slave" is a platform device which is declared as a
> child of the uart in device-tree:
>
> &uart1 {
> bluetooth {
> compatible = "tty,regulator";
> vdd-supply = <&vaux4>;
> };
> };
>
> runtime power management is used to power-up the device
> on tty_open() and power-down on tty_release().
I have a couple of issues with this:
1) why does a child device imply power management and a platform
device? iow, what happens when someone else wants to have a
child device that does something different?
2) why is this tied to the tty core and not the serial core
if this is only for UART?
Regards,
Peter Hurley
> Signed-off-by: NeilBrown
> ---
> .../devicetree/bindings/serial/of-serial.txt |4
> drivers/tty/tty_io.c | 22
>
> 2 files changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt
> b/Documentation/devicetree/bindings/serial/of-serial.txt
> index 8c4fd0332028..b59501ee2f21 100644
> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt
> @@ -39,6 +39,10 @@ Optional properties:
>driver is allowed to detect support for the capability even without this
>property.
>
> +Optional child node:
> +- a platform device listed as a child node will be probed and
> + powered-on whenever the tty is in use (open).
> +
> Example:
>
> uart@8023 {
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 0508a1d8e4cd..7acdc6f093f4 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -95,6 +95,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include
>
> @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty,
> struct tty_struct *o_tty,
> return 0;
> }
>
> +static int open_child(struct device *dev, void *data)
> +{
> + pm_runtime_get_sync(dev);
> + return 0;
> +}
> +static int release_child(struct device *dev, void *data)
> +{
> + pm_runtime_put_autosuspend(dev);
> + return 0;
> +}
> +
> /**
> * tty_release - vfs callback for close
> * @inode: inode of tty
> @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp)
> longtimeout = 0;
> int once = 1;
>
> + if (tty->dev)
> + device_for_each_child(tty->dev, NULL, release_child);
> if (tty_paranoia_check(tty, inode, __func__))
> return 0;
>
> @@ -2118,6 +2133,8 @@ retry_open:
> __proc_set_tty(current, tty);
> spin_unlock_irq(¤t->sighand->siglock);
> tty_unlock(tty);
> + if (tty->dev)
> + device_for_each_child(tty->dev, NULL, open_child);
> mutex_unlock(&tty_mutex);
> return 0;
> err_unlock:
> @@ -3207,6 +3224,11 @@ struct device *tty_register_device_attr(struct
> tty_driver *driver,
> retval = device_register(dev);
> if (retval)
> goto error;
> + if (device && device->of_node)
> + /* Children are platform devices and will be
> + * runtime_pm managed by this tty.
> + */
> + of_platform_populate(device->of_node, NULL, NULL, dev);
>
> return dev;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 01/10] tty: Fallback to use dynamic major number
On 11/26/2014 08:33 AM, Grant Likely wrote: > On Tue, 25 Nov 2014 15:37:16 -0800 > , Kevin Cernekee > wrote: >> On Tue, Nov 25, 2014 at 12:34 PM, Greg KH wrote: >>> On Wed, Nov 12, 2014 at 12:53:58PM -0800, Kevin Cernekee wrote: From: Tushar Behera >>> >>> This email bounces, so I'm going to have to reject this patch. I can't >>> accept a patch from a "fake" person, let alone something that touches >>> core code like this. >>> >>> Sorry, I can't accept anything in this series then. >> >> Oops, guess I probably should have updated his address after the V1 >> emails bounced... >> >> Before I send a new version, what do you think about the overall >> approach? Should we try to make serial8250 coexist with the other >> "ttyS / major 4 / minor 64" drivers (possibly at the expense of >> compatibility) or is it better to start with a simpler, cleaner driver >> like serial/pxa? > > Co-existing really needs to be fixed. What are the requirements for co-existence? Is it sufficient to provide 1st come-1st served minor allocation? Anything done should be designed to solve this name problem forever, not some expeditious band-aid. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
