Hi Simon,

On Thu, Feb 4, 2021 at 5:42 AM Simon Glass sjg@chromium.org wrote:

Hi Bin,

On Wed, 3 Feb 2021 at 07:42, Bin Meng bmeng.cn@gmail.com wrote:

...

A working device tree node of ns16550 should never be populated with value zero for the <clock-frequency> property. Unfortunately this is the case for the QEMU ppce500 target.

Let's try to assign plat->clock to CONFIG_SYS_NS16550_CLK as the last resort to handle such case.

This commit should be reverted when:

• The following QEMU patch [1] is merged, and
• U-Boot CI has upgraded its QEMU version that contains the fix

[1] http://patchwork.ozlabs.org/project/qemu-devel/patch/1612362288-22216-2-git-...

Signed-off-by: Bin Meng bmeng.cn@gmail.com

drivers/serial/ns16550.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index da903c1..a1593a2 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -563,6 +563,8 @@ int ns16550_serial_of_to_plat(struct udevice *dev) if (!plat->clock) plat->clock = dev_read_u32_default(dev, "clock-frequency", CONFIG_SYS_NS16550_CLK);

•   if (!plat->clock)
  

•           plat->clock = CONFIG_SYS_NS16550_CLK;
  

This is already done in the line above...does that not work?

The line above only works if there is no <clock-frequency> in the device tree. Since the device tree does provide a <clock-frequency>, the default one was never used.

...

    if (!plat->clock) {
            debug("ns16550 clock not defined\n");
            return -EINVAL;

Regards, Bin

Hi Simon,

On Thu, Feb 4, 2021 at 8:33 AM Simon Glass sjg@chromium.org wrote:

On Wed, 3 Feb 2021 at 17:20, Bin Meng bmeng.cn@gmail.com wrote:

...

Hi Simon,

On Thu, Feb 4, 2021 at 5:42 AM Simon Glass sjg@chromium.org wrote:

...

Hi Bin,

On Wed, 3 Feb 2021 at 07:42, Bin Meng bmeng.cn@gmail.com wrote:

...

A working device tree node of ns16550 should never be populated with value zero for the <clock-frequency> property. Unfortunately this is the case for the QEMU ppce500 target.

Let's try to assign plat->clock to CONFIG_SYS_NS16550_CLK as the last resort to handle such case.

This commit should be reverted when:

• The following QEMU patch [1] is merged, and
• U-Boot CI has upgraded its QEMU version that contains the fix

[1] http://patchwork.ozlabs.org/project/qemu-devel/patch/1612362288-22216-2-git-...

Signed-off-by: Bin Meng bmeng.cn@gmail.com

drivers/serial/ns16550.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index da903c1..a1593a2 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -563,6 +563,8 @@ int ns16550_serial_of_to_plat(struct udevice *dev) if (!plat->clock) plat->clock = dev_read_u32_default(dev, "clock-frequency", CONFIG_SYS_NS16550_CLK);

•   if (!plat->clock)
  

•           plat->clock = CONFIG_SYS_NS16550_CLK;
  

This is already done in the line above...does that not work?

The line above only works if there is no <clock-frequency> in the device tree. Since the device tree does provide a <clock-frequency>, the default one was never used.

Yes you said that in the commit message, but there was 15 minutes between be reading that and the patch :-(

I really don't like adding this sort of thing globally though. Do you think we could make it happen only for qemu?

Correct, that's why I said we should revert this patch once the 2 conditions are met.

I am working on migrating QEMU ppce500 to driver model. Without this patch the U-Boot does not boot.

Reviewed-by: Simon Glass sjg@chromium.org

Regards, Bin