Re: [PATCH 11/20] dm: serial: Update binding for PL01x serial UART

Re: [PATCH 11/20] dm: serial: Update binding for PL01x serial UART

[Linus Walleij]

On Wed, Jul 8, 2015 at 4:53 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:

> This binding differs from that of Linux. Update it and change existing
> users.
>
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

I'm confused by this. Isn't devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org the place to discuss
device tree bindings? This is sent only to the U-Boot list, does this mean
that there will now be *two* ontologies for DT bindings? That sounds like
a recepie for ... something not good.

> +- clock-frequency:
> +          Input clock frequency for UART.

This is the real trick is it not?

Is the real commit blurb for this patch something like:

"we want a simple way to look up the clock frequency for our serial
ports during early boot, and following clocks = <&...> phandles
is too much trouble and might imply having to implement a whole
clock framework in U-Boot so we just see a lot of trouble, instead
provide a default frequency here"

I suggest in that case, instead do this:

- boot-frequency:
    input clock frequency at boot time, to be used by boot loaders
    and other early access.

This would be good for early Linux stuff as well I believe and I
do not think it's very controversial.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/20] dm: serial: Update binding for PL01x serial UART

[Arnd Bergmann]

On Wednesday 15 July 2015 11:00:59 Linus Walleij wrote:
> On Wed, Jul 8, 2015 at 4:53 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> 
> > This binding differs from that of Linux. Update it and change existing
> > users.
> >
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> I'm confused by this. Isn't devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org the place to discuss
> device tree bindings? This is sent only to the U-Boot list, does this mean
> that there will now be *two* ontologies for DT bindings? That sounds like
> a recepie for ... something not good.
> 
> > +- clock-frequency:
> > +          Input clock frequency for UART.
> 
> This is the real trick is it not?
> 
> Is the real commit blurb for this patch something like:
> 
> "we want a simple way to look up the clock frequency for our serial
> ports during early boot, and following clocks = <&...> phandles
> is too much trouble and might imply having to implement a whole
> clock framework in U-Boot so we just see a lot of trouble, instead
> provide a default frequency here"
> 
> I suggest in that case, instead do this:
> 
> - boot-frequency:
>     input clock frequency at boot time, to be used by boot loaders
>     and other early access.
> 
> This would be good for early Linux stuff as well I believe and I
> do not think it's very controversial.

The CHRP ISA binding defines that a 8250 compatible UART must have this
property:

  "clock-frequency" S

  Standard property, encoded as with encode-int, that shall be the baud-rate
  generator's clock input frequency (in hertz).
  Typically, the clock frequency is nominally 1,843,200 Hz. Some devices
  generate the baud rate input clock by dividing a higher-frequency clock 
  source that is not an exact multiple of 1,843,200 Hz, resulting in a
  small but acceptable error in the nominal clock frequency. In such cases,
  the "clock-frequency" shall report the actual nominal frequency. For
  example, if the baud rate input clock is generated by dividing a 24
  MHz clock by 13, the value of the "clock-frequency" property shall be 1846153.
	
	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/20] dm: serial: Update binding for PL01x serial UART

[Linus Walleij]

On Wed, Jul 15, 2015 at 11:38 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

> The CHRP ISA binding defines that a 8250 compatible UART must have this
> property:
>
>   "clock-frequency" S
>
>   Standard property, encoded as with encode-int, that shall be the baud-rate
>   generator's clock input frequency (in hertz).
>   Typically, the clock frequency is nominally 1,843,200 Hz. Some devices
>   generate the baud rate input clock by dividing a higher-frequency clock
>   source that is not an exact multiple of 1,843,200 Hz, resulting in a
>   small but acceptable error in the nominal clock frequency. In such cases,
>   the "clock-frequency" shall report the actual nominal frequency. For
>   example, if the baud rate input clock is generated by dividing a 24
>   MHz clock by 13, the value of the "clock-frequency" property shall be 1846153.

Isn't that for the case where the clock frequency will never change at runtime?

The problem (I think) with many systems using PL011 is that they can
actually change the serial port clock at runtime (software controlled clock),
so providing a "clock-frequency" would imply that they cannot, and the clock
frequency is fixed to that value.

I think it's better to use boot-clock-frequency or so to indicate that a richer
OS will provide more refined clocks. For example that frequency can
typically change if the SoC changes operating point or so, though it would
be awkward to handle in the driver, admittedly and I haven't seen a
piece of code that actually go and change the UART input frequency.

Still for completion it is better for the UART to tie into the clk framework
as the OS gets up, and just providing clock-frequency seems to imply
that it need not do that or something. The semantical effect of providing both
clock-frequency = and clocks =< &..> must be clarified in any case.
Like the latter overrides the former if clock phandles can be handled by
the environment. (And this should be stated in the binding.)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/20] dm: serial: Update binding for PL01x serial UART

[Arnd Bergmann]

On Wednesday 15 July 2015 12:08:05 Linus Walleij wrote:
> On Wed, Jul 15, 2015 at 11:38 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> 
> > The CHRP ISA binding defines that a 8250 compatible UART must have this
> > property:
> >
> >   "clock-frequency" S
> >
> >   Standard property, encoded as with encode-int, that shall be the baud-rate
> >   generator's clock input frequency (in hertz).
> >   Typically, the clock frequency is nominally 1,843,200 Hz. Some devices
> >   generate the baud rate input clock by dividing a higher-frequency clock
> >   source that is not an exact multiple of 1,843,200 Hz, resulting in a
> >   small but acceptable error in the nominal clock frequency. In such cases,
> >   the "clock-frequency" shall report the actual nominal frequency. For
> >   example, if the baud rate input clock is generated by dividing a 24
> >   MHz clock by 13, the value of the "clock-frequency" property shall be 1846153.
> 
> Isn't that for the case where the clock frequency will never change at runtime?
> 
> The problem (I think) with many systems using PL011 is that they can
> actually change the serial port clock at runtime (software controlled clock),
> so providing a "clock-frequency" would imply that they cannot, and the clock
> frequency is fixed to that value.
> 
> I think it's better to use boot-clock-frequency or so to indicate that a richer
> OS will provide more refined clocks. For example that frequency can
> typically change if the SoC changes operating point or so, though it would
> be awkward to handle in the driver, admittedly and I haven't seen a
> piece of code that actually go and change the UART input frequency.
> 
> Still for completion it is better for the UART to tie into the clk framework
> as the OS gets up, and just providing clock-frequency seems to imply
> that it need not do that or something. The semantical effect of providing both
> clock-frequency = and clocks =< &..> must be clarified in any case.
> Like the latter overrides the former if clock phandles can be handled by
> the environment. (And this should be stated in the binding.)

Ah right. I don't think that naming is super critical though, as a lot 
of properties in the DT just describe how things are at boot time.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/20] dm: serial: Update binding for PL01x serial UART

[Geert Uytterhoeven]

Hi Arnd,

On Wed, Jul 15, 2015 at 12:17 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 15 July 2015 12:08:05 Linus Walleij wrote:
>> On Wed, Jul 15, 2015 at 11:38 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>
>> > The CHRP ISA binding defines that a 8250 compatible UART must have this
>> > property:
>> >
>> >   "clock-frequency" S
>> >
>> >   Standard property, encoded as with encode-int, that shall be the baud-rate
>> >   generator's clock input frequency (in hertz).
>> >   Typically, the clock frequency is nominally 1,843,200 Hz. Some devices
>> >   generate the baud rate input clock by dividing a higher-frequency clock
>> >   source that is not an exact multiple of 1,843,200 Hz, resulting in a
>> >   small but acceptable error in the nominal clock frequency. In such cases,
>> >   the "clock-frequency" shall report the actual nominal frequency. For
>> >   example, if the baud rate input clock is generated by dividing a 24
>> >   MHz clock by 13, the value of the "clock-frequency" property shall be 1846153.
>>
>> Isn't that for the case where the clock frequency will never change at runtime?
>>
>> The problem (I think) with many systems using PL011 is that they can
>> actually change the serial port clock at runtime (software controlled clock),
>> so providing a "clock-frequency" would imply that they cannot, and the clock
>> frequency is fixed to that value.
>>
>> I think it's better to use boot-clock-frequency or so to indicate that a richer
>> OS will provide more refined clocks. For example that frequency can
>> typically change if the SoC changes operating point or so, though it would
>> be awkward to handle in the driver, admittedly and I haven't seen a
>> piece of code that actually go and change the UART input frequency.
>>
>> Still for completion it is better for the UART to tie into the clk framework
>> as the OS gets up, and just providing clock-frequency seems to imply
>> that it need not do that or something. The semantical effect of providing both
>> clock-frequency = and clocks =< &..> must be clarified in any case.
>> Like the latter overrides the former if clock phandles can be handled by
>> the environment. (And this should be stated in the binding.)
>
> Ah right. I don't think that naming is super critical though, as a lot
> of properties in the DT just describe how things are at boot time.

Hence those don't describe the hardware...
Shouldn't they be in /chosen instead?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/20] dm: serial: Update binding for PL01x serial UART

[Linus Walleij]

On Thu, Jul 16, 2015 at 9:41 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

>> Ah right. I don't think that naming is super critical though, as a lot
>> of properties in the DT just describe how things are at boot time.
>
> Hence those don't describe the hardware...
> Shouldn't they be in /chosen instead?

/chosen is used for system-wide things mostly, as alias.

I think this can go in the hardware node.

Simon can you please make a patch to
Documentation/devicetree/bindings/serial/pl011.txt in the kernel
and post to devicetree@vger.kernel.org *and*
linux-doc@vger.kernel.org and Jonathan Corbet <corbet@lwn.net>
so he can merge it if we get some ACKs on it.

Yours,
Linus Walleij