[U-Boot] How does Driver Model UART work?

[Simon Glass]

Hi Masahiro,

On 4 October 2014 06:57, Masahiro YAMADA <yamada.m at jp.panasonic.com> wrote:
>
> Hi Simon,
>
>
>
> 2014-10-03 22:51 GMT+09:00 Simon Glass <sjg at chromium.org>:
> > Hi Masahiro,
> >
> > On 3 October 2014 07:04, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> >> Simon,
> >>
> >>
> >>
> >> I am totally being confused.
> >>
> >>
> >>
> >> As far as I looked at the dm code,
> >> the private data is calloc'ed in device_probe() function
> >>
> >>         if (drv->priv_auto_alloc_size) {
> >>                 dev->priv = calloc(1, drv->priv_auto_alloc_size);
> >>                 if (!dev->priv) {
> >>                         ret = -ENOMEM;
> >>                         goto fail;
> >>                 }
> >>         }
> >>
> >>
> >>
> >> So,  dev->priv is storing the address of the allocated memory.
> >>
> >> Am I understanding correctly?
> >
> > Yes, it always does in driver model. This driver is no different.
> >
> > BTW the ns16550 driver is probably the oddest in U-Boot - it looks
> > like UniPhier has its own UART driver so it would be better to convert
> > that I think. See below for ideas on other UART drivers to look at
> > which are much more normal.
> >
> >>
> >>
> >>
> >> If so, I can't understand the following code:
> >>
> >>
> >> static int ns16550_serial_getc(struct udevice *dev)
> >> {
> >>         struct NS16550 *const com_port = dev_get_priv(dev);
> >>
> >>         if (!serial_in(&com_port->lsr) & UART_LSR_DR)
> >>                 return -EAGAIN;
> >>
> >>
> >>
> >> "com_port" is dev->priv, so it is pointing to the allocated area on RAM,
> >> I guess.
> >>
> >>
> >> It looks like
> >> serial_in(&com_port->lsr)  is trying to read from the hardware register ?
> >>
> >> Or reading from malloc area RAM ??
> >
> > If you go one level deeper you will see that serial_in() is defined at
> > the top in about 5 ways, one of which is used for driver model:
> >
> > #define serial_in(addr) \
> >      ns16550_readb(com_port, addr - (unsigned char *)com_port)
> >
> > So it used com_port which is not an parameter. The parameter addr is
> > only used to specify the register. This is actually the same as the
> > non-DM code except in that case the parameter specifies the register
> > and hardware address at the same time.
>
> Without your help, I never could have understood this code.
> Now it is clearer, thanks! (although this code is really unfortunate...)

It felt like laparoscopic surgery - there is a nicely-healed wound but
inside is another story.

>
>
>
> > This is done since the internal NS16550_t type is unfortunately
> > exported all over U-Boot. This is annoying because it is actually an
> > internal register format for the UART. Even worse is the fact that the
> > structure changes depending on CONFIG_SYS_NS16550_REG_SIZE. We can't
> > have this sort of thing in driver model - we need to be able to cope
> > with the device tree specifying all the information that the UART
> > needs. So for driver model:
>
> Agreed.
> We should pass "reg-shift" from a device tree or a board-file.
>
> If all the board switched to the driver model,
> the driver code would become much clean.

Yes, I'm looking forward to that day.

>
> >
> > /*
> >  * For driver model we always use one byte per register, and sort out the
> >  * differences in the driver
> >  */
> > #define CONFIG_SYS_NS16550_REG_SIZE (-1)
> >
> > (see ns16550.h header for where this is used)
> >
> > I would like to avoid having that type exported and use a different
> > method to pass the UART information around. But it is used in about 30
> > places . So this approach allows us to move forward bit by bit,
> > without duplicating the UART driver and creating a maintenance
> > headache.
> >
> > In terns of implementing a new UART driver, are you using device tree?
>
> UniPhier SoCs have not supported the device tree control yet.
>
> I am planning to do the dm conversion with a board file.
>
>
> > If so then you should just be able to follow along with Tegra - it's
> > really easy - just copy that serial_tegra.c and adjust the device tree
> > compat string and input clock. Other examples are serial_omap and
> > serial_dw. (See u-boot-dm/working)
>
> These are unfortunate.
> They are borrowing ns16550 code.

Tegra and many other SOCs have used this code for a while, just with
some defines to make it work.  I didn't want to copy it.

>
>
> > If you are using platform data, then the examples are serial_mxc and
> > serial_pl01x.
>
> I am doing this way.
>
> Anyway, the register map of UniPhier is not 8250-compatible.
> It cannot borrow ns16550 code.

That's probably for the best.

Regards,
Simon