Message ID | 1612363345-24335-1-git-send-email-bmeng.cn@gmail.com |
---|---|
State | Accepted |
Commit | 384b62c073f3aaccba0c917a8da7701a82441aec |
Delegated to: | Simon Glass |
Headers | show |
Series | serial: ns16550: Handle zero <clock-frequency> value | expand |
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-send-email-bmeng.cn@gmail.com/ > > 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? > if (!plat->clock) { > debug("ns16550 clock not defined\n"); > return -EINVAL; > -- > 2.7.4 > Regards, Simon
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-send-email-bmeng.cn@gmail.com/ > > > > 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
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-send-email-bmeng.cn@gmail.com/ > > > > > > 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? Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > 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-send-email-bmeng.cn@gmail.com/ > > > > > > > > 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
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-send-email-bmeng.cn@gmail.com/ > > > > > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > > --- > > > > > > > > drivers/serial/ns16550.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > Applied to u-boot-dm, thanks!
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; if (!plat->clock) { debug("ns16550 clock not defined\n"); return -EINVAL;
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-send-email-bmeng.cn@gmail.com/ Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- drivers/serial/ns16550.c | 2 ++ 1 file changed, 2 insertions(+)