How does Driver Model UART work?

U-Boot Masahiro Yamada 2 participants 5 messages Oct 3, 2014 – Oct 4, 2014

lore

Share & tools

lore

Masahiro Yamada

author

2014-10-03 12:45 UTC #0 lore raw

Simon,

Now I am seeing driver/serial/serial_tegra.c

U_BOOT_DRIVER(serial_ns16550) = {
	.name	= "serial_tegra20",
	.id	= UCLASS_SERIAL,
	.of_match = tegra_serial_ids,
	.ofdata_to_platdata = tegra_serial_ofdata_to_platdata,
	.platdata_auto_alloc_size = sizeof(struct ns16550_platdata),
	.priv_auto_alloc_size = sizeof(struct NS16550),
	.probe = ns16550_serial_probe,
	.ops	= &ns16550_serial_ops,

You require sizeof(struct NS16550) (= about 30~ 40 bytes) for .priv_auto_alloc_size.

Seems strange.

It looks like the private date is used as the base address of the hardware

Here,

	struct NS16550 *const com_port = dev_get_priv(dev);

If the hardware register has 0x10000 width,
do we need to allocation 0x10000 memory for .priv data?

I think 4 byte (or 8 byte on 64bit architectures) is enough to store the base address.

It looks like the memory is allocated for .priv in device_probe() function, but I could not find where .priv is set for pointing to the base address.

Perhaps I am not understanding well yet, but
I'm still not convinced how it is working.

Best Regards
Masahiro Yamada

Masahiro Yamada

author

2014-10-03 13:04 UTC #1 lore raw

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?

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 ??

Best Regards
Masahiro Yamada

Simon Glass

2014-10-03 13:51 UTC #2 lore raw

Hi Masahiro,

Quoted 21 lines · Masahiro Yamada

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;
                }
        }

Quoted 3 lines

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.

Quoted 15 lines

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;

Quoted 8 lines

"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.

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:

/*
* 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?

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)

If you are using platform data, then the examples are serial_mxc and serial_pl01x.

Regards,
Simon

Masahiro YAMADA

author

2014-10-04 12:57 UTC #3 lore raw

Hi Simon,

2014-10-03 22:51 GMT+09:00 Simon Glass <[email protected]>:

Quoted 62 lines

Hi Masahiro,
Quoted 21 lines · Masahiro Yamada
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;
                }
        }
Quoted 3 lines
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.
Quoted 15 lines
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;
Quoted 8 lines
"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:

Quoted 7 lines

#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...)

Quoted 7 lines

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.

Quoted 9 lines

/*
* 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)

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Signature · 2 lines

Best Regards
Masahiro Yamada

Simon Glass

2014-10-04 17:08 UTC #4 lore raw

Hi Masahiro,

Quoted 71 lines · Masahiro YAMADA

Hi Simon,
2014-10-03 22:51 GMT+09:00 Simon Glass <[email protected]>:
Quoted 65 lines
Hi Masahiro,
Quoted 21 lines · Masahiro Yamada
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;
                }
        }
Quoted 3 lines
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.
Quoted 15 lines
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;
Quoted 8 lines
"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)

Quoted 7 lines

Quoted 4 lines
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.

Quoted 11 lines

Quoted 10 lines
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:

Quoted 5 lines

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.

Quoted 23 lines

Quoted 10 lines
/*
* 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)
Quoted 7 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 9 lines

Quoted 4 lines
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