| Message ID | 7bbf871f73f2beed898a4cb9de9e00d1e24c2ea1.1659581119.git.weijie.gao@mediatek.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add support for MediaTek MT7981/MT7986 SoCs | expand |
Hi Weijie, On Wed, 3 Aug 2022 at 21:36, Weijie Gao <weijie.gao@mediatek.com> wrote: > > The baud clock on some platform may change due to assigned-clock-parent > set in DT. In current flow the baud clock is only retrieved during probe > stage. If the parent of the source clock changes after probe stage, the > setbrg will set wrong baudrate. > > To get the right clock rate, this patch records the baud clk struct to the > driver's priv, and changes the driver's flow to get the clock rate before > calling _mtk_serial_setbrg(). > > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com> > --- > drivers/serial/serial_mtk.c | 72 ++++++++++++++++++++----------------- > 1 file changed, 39 insertions(+), 33 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> please see below > > diff --git a/drivers/serial/serial_mtk.c b/drivers/serial/serial_mtk.c > index a84f39b3fa..99cf62b8d9 100644 > --- a/drivers/serial/serial_mtk.c > +++ b/drivers/serial/serial_mtk.c > @@ -10,6 +10,7 @@ > #include <common.h> > #include <div64.h> > #include <dm.h> > +#include <dm/device_compat.h> > #include <errno.h> > #include <log.h> > #include <serial.h> > @@ -72,25 +73,27 @@ struct mtk_serial_regs { > > struct mtk_serial_priv { please add a full comment for this struct > struct mtk_serial_regs __iomem *regs; > - u32 clock; > + struct clk clk; > + u32 fixed_clk_rate; > bool force_highspeed; > }; > > -static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int baud) > +static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int baud, > + u32 clk_rate) Why u32? Can you use uint? Generally it is better for parameters to use natural types unless there is a good reason. [..] Regards, Simon
On Thu, 2022-08-04 at 07:56 -0600, Simon Glass wrote: > Hi Weijie, > > On Wed, 3 Aug 2022 at 21:36, Weijie Gao <weijie.gao@mediatek.com> > wrote: > > > > The baud clock on some platform may change due to assigned-clock- > > parent > > set in DT. In current flow the baud clock is only retrieved during > > probe > > stage. If the parent of the source clock changes after probe stage, > > the > > setbrg will set wrong baudrate. > > > > To get the right clock rate, this patch records the baud clk struct > > to the > > driver's priv, and changes the driver's flow to get the clock rate > > before > > calling _mtk_serial_setbrg(). > > > > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com> > > --- > > drivers/serial/serial_mtk.c | 72 ++++++++++++++++++++------------- > > ---- > > 1 file changed, 39 insertions(+), 33 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > please see below > > > > > diff --git a/drivers/serial/serial_mtk.c > > b/drivers/serial/serial_mtk.c > > index a84f39b3fa..99cf62b8d9 100644 > > --- a/drivers/serial/serial_mtk.c > > +++ b/drivers/serial/serial_mtk.c > > @@ -10,6 +10,7 @@ > > #include <common.h> > > #include <div64.h> > > #include <dm.h> > > +#include <dm/device_compat.h> > > #include <errno.h> > > #include <log.h> > > #include <serial.h> > > @@ -72,25 +73,27 @@ struct mtk_serial_regs { > > > > struct mtk_serial_priv { > > please add a full comment for this struct OK. > > > struct mtk_serial_regs __iomem *regs; > > - u32 clock; > > + struct clk clk; > > + u32 fixed_clk_rate; > > bool force_highspeed; > > }; > > > > -static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int > > baud) > > +static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int > > baud, > > + u32 clk_rate) > > Why u32? Can you use uint? Generally it is better for parameters to > use natural types unless there is a good reason. In fact u32 is identical to uint. <asm-generic/int-ll64.h>: typedef __u32 u32; <asm-generic/int-ll64.h>: typedef unsigned int __u32; <linux/types.h>: typedef unsigned int uint; > > [..] > > Regards, > Simon
Hi Weijie, On Sun, 7 Aug 2022 at 20:36, Weijie Gao <weijie.gao@mediatek.com> wrote: > > On Thu, 2022-08-04 at 07:56 -0600, Simon Glass wrote: > > Hi Weijie, > > > > On Wed, 3 Aug 2022 at 21:36, Weijie Gao <weijie.gao@mediatek.com> > > wrote: > > > > > > The baud clock on some platform may change due to assigned-clock- > > > parent > > > set in DT. In current flow the baud clock is only retrieved during > > > probe > > > stage. If the parent of the source clock changes after probe stage, > > > the > > > setbrg will set wrong baudrate. > > > > > > To get the right clock rate, this patch records the baud clk struct > > > to the > > > driver's priv, and changes the driver's flow to get the clock rate > > > before > > > calling _mtk_serial_setbrg(). > > > > > > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com> > > > --- > > > drivers/serial/serial_mtk.c | 72 ++++++++++++++++++++------------- > > > ---- > > > 1 file changed, 39 insertions(+), 33 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > please see below > > > > > > > > diff --git a/drivers/serial/serial_mtk.c > > > b/drivers/serial/serial_mtk.c > > > index a84f39b3fa..99cf62b8d9 100644 > > > --- a/drivers/serial/serial_mtk.c > > > +++ b/drivers/serial/serial_mtk.c > > > @@ -10,6 +10,7 @@ > > > #include <common.h> > > > #include <div64.h> > > > #include <dm.h> > > > +#include <dm/device_compat.h> > > > #include <errno.h> > > > #include <log.h> > > > #include <serial.h> > > > @@ -72,25 +73,27 @@ struct mtk_serial_regs { > > > > > > struct mtk_serial_priv { > > > > please add a full comment for this struct > > OK. > > > > > > struct mtk_serial_regs __iomem *regs; > > > - u32 clock; > > > + struct clk clk; > > > + u32 fixed_clk_rate; > > > bool force_highspeed; > > > }; > > > > > > -static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int > > > baud) > > > +static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int > > > baud, > > > + u32 clk_rate) > > > > Why u32? Can you use uint? Generally it is better for parameters to > > use natural types unless there is a good reason. > > In fact u32 is identical to uint. > > <asm-generic/int-ll64.h>: typedef __u32 u32; > <asm-generic/int-ll64.h>: typedef unsigned int __u32; > > <linux/types.h>: typedef unsigned int uint; It's not about the eventual type...if u32 is the same as uint, why not just use uint? It is simpler and does the right thing on 64-bit, where u32 is different. The clock interface uses long. In some cases the compiler must mask the values so it adds to code size. Regards, SImon
diff --git a/drivers/serial/serial_mtk.c b/drivers/serial/serial_mtk.c index a84f39b3fa..99cf62b8d9 100644 --- a/drivers/serial/serial_mtk.c +++ b/drivers/serial/serial_mtk.c @@ -10,6 +10,7 @@ #include <common.h> #include <div64.h> #include <dm.h> +#include <dm/device_compat.h> #include <errno.h> #include <log.h> #include <serial.h> @@ -72,25 +73,27 @@ struct mtk_serial_regs { struct mtk_serial_priv { struct mtk_serial_regs __iomem *regs; - u32 clock; + struct clk clk; + u32 fixed_clk_rate; bool force_highspeed; }; -static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int baud) +static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int baud, + u32 clk_rate) { u32 quot, realbaud, samplecount = 1; /* Special case for low baud clock */ - if (baud <= 115200 && priv->clock <= 12000000) { + if (baud <= 115200 && clk_rate == 12000000) { writel(3, &priv->regs->highspeed); - quot = DIV_ROUND_CLOSEST(priv->clock, 256 * baud); + quot = DIV_ROUND_CLOSEST(clk_rate, 256 * baud); if (quot == 0) quot = 1; - samplecount = DIV_ROUND_CLOSEST(priv->clock, quot * baud); + samplecount = DIV_ROUND_CLOSEST(clk_rate, quot * baud); - realbaud = priv->clock / samplecount / quot; + realbaud = clk_rate / samplecount / quot; if (realbaud > BAUD_ALLOW_MAX(baud) || realbaud < BAUD_ALLOW_MIX(baud)) { pr_info("baud %d can't be handled\n", baud); @@ -104,7 +107,7 @@ static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int baud) if (baud <= 115200) { writel(0, &priv->regs->highspeed); - quot = DIV_ROUND_CLOSEST(priv->clock, 16 * baud); + quot = DIV_ROUND_CLOSEST(clk_rate, 16 * baud); } else if (baud <= 576000) { writel(2, &priv->regs->highspeed);
The baud clock on some platform may change due to assigned-clock-parent set in DT. In current flow the baud clock is only retrieved during probe stage. If the parent of the source clock changes after probe stage, the setbrg will set wrong baudrate. To get the right clock rate, this patch records the baud clk struct to the driver's priv, and changes the driver's flow to get the clock rate before calling _mtk_serial_setbrg(). Signed-off-by: Weijie Gao <weijie.gao@mediatek.com> --- drivers/serial/serial_mtk.c | 72 ++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 33 deletions(-) @@ -112,13 +115,13 @@ static void _mtk_serial_setbrg(struct mtk_serial_priv *priv, int baud) if ((baud == 500000) || (baud == 576000)) baud = 460800; - quot = DIV_ROUND_UP(priv->clock, 4 * baud); + quot = DIV_ROUND_UP(clk_rate, 4 * baud); } else { use_hs3: writel(3, &priv->regs->highspeed); - quot = DIV_ROUND_UP(priv->clock, 256 * baud); - samplecount = DIV_ROUND_CLOSEST(priv->clock, quot * baud); + quot = DIV_ROUND_UP(clk_rate, 256 * baud); + samplecount = DIV_ROUND_CLOSEST(clk_rate, quot * baud); } set_baud: @@ -167,8 +170,13 @@ static int _mtk_serial_pending(struct mtk_serial_priv *priv, bool input) static int mtk_serial_setbrg(struct udevice *dev, int baudrate) { struct mtk_serial_priv *priv = dev_get_priv(dev); + u32 clk_rate; + + clk_rate = clk_get_rate(&priv->clk); + if (IS_ERR_VALUE(clk_rate) || clk_rate == 0) + clk_rate = priv->fixed_clk_rate; - _mtk_serial_setbrg(priv, baudrate); + _mtk_serial_setbrg(priv, baudrate, clk_rate); return 0; } @@ -211,7 +219,6 @@ static int mtk_serial_of_to_plat(struct udevice *dev) { struct mtk_serial_priv *priv = dev_get_priv(dev); fdt_addr_t addr; - struct clk clk; int err; addr = dev_read_addr(dev); @@ -220,22 +227,19 @@ static int mtk_serial_of_to_plat(struct udevice *dev) priv->regs = map_physmem(addr, 0, MAP_NOCACHE); - err = clk_get_by_index(dev, 0, &clk); - if (!err) { - err = clk_get_rate(&clk); - if (!IS_ERR_VALUE(err)) - priv->clock = err; - } else if (err != -ENOENT && err != -ENODEV && err != -ENOSYS) { - debug("mtk_serial: failed to get clock\n"); - return err; - } - - if (!priv->clock) - priv->clock = dev_read_u32_default(dev, "clock-frequency", 0); - - if (!priv->clock) { - debug("mtk_serial: clock not defined\n"); - return -EINVAL; + err = clk_get_by_index(dev, 0, &priv->clk); + if (err) { + err = dev_read_u32(dev, "clock-frequency", &priv->fixed_clk_rate); + if (err) { + dev_err(dev, "baud clock not defined\n"); + return -EINVAL; + } + } else { + err = clk_get_rate(&priv->clk); + if (IS_ERR_VALUE(err)) { + dev_err(dev, "invalid baud clock\n"); + return -EINVAL; + } } priv->force_highspeed = dev_read_bool(dev, "mediatek,force-highspeed"); @@ -273,7 +277,7 @@ DECLARE_GLOBAL_DATA_PTR; #define DECLARE_HSUART_PRIV(port) \ static struct mtk_serial_priv mtk_hsuart##port = { \ .regs = (struct mtk_serial_regs *)CONFIG_SYS_NS16550_COM##port, \ - .clock = CONFIG_SYS_NS16550_CLK \ + .fixed_clk_rate = CONFIG_SYS_NS16550_CLK \ }; #define DECLARE_HSUART_FUNCTIONS(port) \ @@ -282,12 +286,14 @@ DECLARE_GLOBAL_DATA_PTR; writel(0, &mtk_hsuart##port.regs->ier); \ writel(UART_MCRVAL, &mtk_hsuart##port.regs->mcr); \ writel(UART_FCRVAL, &mtk_hsuart##port.regs->fcr); \ - _mtk_serial_setbrg(&mtk_hsuart##port, gd->baudrate); \ + _mtk_serial_setbrg(&mtk_hsuart##port, gd->baudrate, \ + mtk_hsuart##port.fixed_clk_rate); \ return 0 ; \ } \ static void mtk_serial##port##_setbrg(void) \ { \ - _mtk_serial_setbrg(&mtk_hsuart##port, gd->baudrate); \ + _mtk_serial_setbrg(&mtk_hsuart##port, gd->baudrate, \ + mtk_hsuart##port.fixed_clk_rate); \ } \ static int mtk_serial##port##_getc(void) \ { \ @@ -427,13 +433,13 @@ static inline void _debug_uart_init(void) struct mtk_serial_priv priv; priv.regs = (void *) CONFIG_VAL(DEBUG_UART_BASE); - priv.clock = CONFIG_DEBUG_UART_CLOCK; + priv.fixed_clk_rate = CONFIG_DEBUG_UART_CLOCK; writel(0, &priv.regs->ier); writel(UART_MCRVAL, &priv.regs->mcr); writel(UART_FCRVAL, &priv.regs->fcr); - _mtk_serial_setbrg(&priv, CONFIG_BAUDRATE); + _mtk_serial_setbrg(&priv, CONFIG_BAUDRATE, priv.fixed_clk_rate); } static inline void _debug_uart_putc(int ch)