Hi Simon,
2014-10-03 22:51 GMT+09:00 Simon Glass <s...@chromium.org>: > Hi Masahiro, > > On 3 October 2014 07:04, Masahiro Yamada <yamad...@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...) > 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. > > /* > * 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. > 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. -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot