| Message ID | 1430117397-13517-1-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > When the driver has probed successfully the clk pointer is always valid, > so no need to test for it. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/tty/serial/8250/8250_mtk.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > Acked-by: Matthias Brugger <matthias.bgg@gmail.com> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > index 7a11fac..1297827 100644 > --- a/drivers/tty/serial/8250/8250_mtk.c > +++ b/drivers/tty/serial/8250/8250_mtk.c > @@ -214,10 +214,8 @@ static int mtk8250_remove(struct platform_device *pdev) > pm_runtime_get_sync(&pdev->dev); > > serial8250_unregister_port(data->line); > - if (!IS_ERR(data->uart_clk)) { > - clk_disable_unprepare(data->uart_clk); > - clk_put(data->uart_clk); > - } > + clk_disable_unprepare(data->uart_clk); > + clk_put(data->uart_clk); > > pm_runtime_disable(&pdev->dev); > pm_runtime_put_noidle(&pdev->dev); > @@ -249,8 +247,7 @@ static int mtk8250_runtime_suspend(struct device *dev) > { > struct mtk8250_data *data = dev_get_drvdata(dev); > > - if (!IS_ERR(data->uart_clk)) > - clk_disable_unprepare(data->uart_clk); > + clk_disable_unprepare(data->uart_clk); > > return 0; > } > @@ -259,8 +256,7 @@ static int mtk8250_runtime_resume(struct device *dev) > { > struct mtk8250_data *data = dev_get_drvdata(dev); > > - if (!IS_ERR(data->uart_clk)) > - clk_prepare_enable(data->uart_clk); > + clk_prepare_enable(data->uart_clk); > > return 0; > } > -- > 2.1.4 >
2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > When a struct device * is present clk_get should be used rather > than of_clk_get. Use the devm variant of this function to be able to > drop the clk_put in the error and remove pathes. While at it fix > a wrong error message. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/tty/serial/8250/8250_mtk.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) Acked-by: Matthias Brugger <matthias.bgg@gmail.com>
2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > The pm_runtime callbacks already enable and disable the device. > Use them in probe() and remove() instead of duplicating the > code. This allows us to concentrate more code for enabling/disabling > the UART in a single place. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/tty/serial/8250/8250_mtk.c | 69 ++++++++++++++++++++------------------ > 1 file changed, 36 insertions(+), 33 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > index bcfaa8dc..2f28bd0 100644 > --- a/drivers/tty/serial/8250/8250_mtk.c > +++ b/drivers/tty/serial/8250/8250_mtk.c > @@ -115,6 +115,29 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios, > tty_termios_encode_baud_rate(termios, baud, baud); > } > > +static int mtk8250_runtime_suspend(struct device *dev) > +{ > + struct mtk8250_data *data = dev_get_drvdata(dev); > + > + clk_disable_unprepare(data->uart_clk); > + > + return 0; > +} > + > +static int mtk8250_runtime_resume(struct device *dev) > +{ > + struct mtk8250_data *data = dev_get_drvdata(dev); > + int err; > + > + err = clk_prepare_enable(data->uart_clk); > + if (err) { > + dev_warn(dev, "Can't enable clock\n"); > + return err; > + } > + > + return 0; > +} > + > static void > mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) > { > @@ -130,19 +153,12 @@ mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) > static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, > struct mtk8250_data *data) > { > - int err; > - > data->uart_clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(data->uart_clk)) { > dev_warn(&pdev->dev, "Can't get uart clock\n"); > return PTR_ERR(data->uart_clk); > } > > - err = clk_prepare_enable(data->uart_clk); > - if (err) { > - dev_warn(&pdev->dev, "Can't prepare clock\n"); > - return err; > - } > p->uartclk = clk_get_rate(data->uart_clk); > > return 0; > @@ -193,14 +209,18 @@ static int mtk8250_probe(struct platform_device *pdev) > writel(0x0, uart.port.membase + > (MTK_UART_RATE_FIX << uart.port.regshift)); > > - data->line = serial8250_register_8250_port(&uart); > - if (data->line < 0) > - return data->line; > - > platform_set_drvdata(pdev, data); > > - pm_runtime_set_active(&pdev->dev); > pm_runtime_enable(&pdev->dev); > + if (!pm_runtime_enabled(&pdev->dev)) { > + err = mtk8250_runtime_resume(&pdev->dev); > + if (err) > + return err; > + } > + > + data->line = serial8250_register_8250_port(&uart); > + if (data->line < 0) > + return data->line; Why do you delete pm_runtime_set_active here?
On Mon, May 04, 2015 at 01:05:27PM +0200, Matthias Brugger wrote: > 2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > > The pm_runtime callbacks already enable and disable the device. > > Use them in probe() and remove() instead of duplicating the > > code. This allows us to concentrate more code for enabling/disabling > > the UART in a single place. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/tty/serial/8250/8250_mtk.c | 69 ++++++++++++++++++++------------------ > > 1 file changed, 36 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > > index bcfaa8dc..2f28bd0 100644 > > --- a/drivers/tty/serial/8250/8250_mtk.c > > +++ b/drivers/tty/serial/8250/8250_mtk.c > > @@ -115,6 +115,29 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios, > > tty_termios_encode_baud_rate(termios, baud, baud); > > } > > > > +static int mtk8250_runtime_suspend(struct device *dev) > > +{ > > + struct mtk8250_data *data = dev_get_drvdata(dev); > > + > > + clk_disable_unprepare(data->uart_clk); > > + > > + return 0; > > +} > > + > > +static int mtk8250_runtime_resume(struct device *dev) > > +{ > > + struct mtk8250_data *data = dev_get_drvdata(dev); > > + int err; > > + > > + err = clk_prepare_enable(data->uart_clk); > > + if (err) { > > + dev_warn(dev, "Can't enable clock\n"); > > + return err; > > + } > > + > > + return 0; > > +} > > + > > static void > > mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) > > { > > @@ -130,19 +153,12 @@ mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) > > static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, > > struct mtk8250_data *data) > > { > > - int err; > > - > > data->uart_clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(data->uart_clk)) { > > dev_warn(&pdev->dev, "Can't get uart clock\n"); > > return PTR_ERR(data->uart_clk); > > } > > > > - err = clk_prepare_enable(data->uart_clk); > > - if (err) { > > - dev_warn(&pdev->dev, "Can't prepare clock\n"); > > - return err; > > - } > > p->uartclk = clk_get_rate(data->uart_clk); > > > > return 0; > > @@ -193,14 +209,18 @@ static int mtk8250_probe(struct platform_device *pdev) > > writel(0x0, uart.port.membase + > > (MTK_UART_RATE_FIX << uart.port.regshift)); > > > > - data->line = serial8250_register_8250_port(&uart); > > - if (data->line < 0) > > - return data->line; > > - > > platform_set_drvdata(pdev, data); > > > > - pm_runtime_set_active(&pdev->dev); > > pm_runtime_enable(&pdev->dev); > > + if (!pm_runtime_enabled(&pdev->dev)) { > > + err = mtk8250_runtime_resume(&pdev->dev); > > + if (err) > > + return err; > > + } > > + > > + data->line = serial8250_register_8250_port(&uart); > > + if (data->line < 0) > > + return data->line; > > Why do you delete pm_runtime_set_active here? pm_runtime_set_active() communicates the initial state of the device to the pm core. Previously the driver activated the device manually by directly calling clk_prepare_enable(). Since this manual enabling of the clock is removed in this patch, the device is no longer active, hence pm_runtime_set_active() has to be removed. Sascha
2015-05-04 14:06 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > On Mon, May 04, 2015 at 01:05:27PM +0200, Matthias Brugger wrote: >> 2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: >> > The pm_runtime callbacks already enable and disable the device. >> > Use them in probe() and remove() instead of duplicating the >> > code. This allows us to concentrate more code for enabling/disabling >> > the UART in a single place. >> > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> > --- >> > drivers/tty/serial/8250/8250_mtk.c | 69 ++++++++++++++++++++------------------ >> > 1 file changed, 36 insertions(+), 33 deletions(-) >> > >> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c >> > index bcfaa8dc..2f28bd0 100644 >> > --- a/drivers/tty/serial/8250/8250_mtk.c >> > +++ b/drivers/tty/serial/8250/8250_mtk.c >> > @@ -115,6 +115,29 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios, >> > tty_termios_encode_baud_rate(termios, baud, baud); >> > } >> > >> > +static int mtk8250_runtime_suspend(struct device *dev) >> > +{ >> > + struct mtk8250_data *data = dev_get_drvdata(dev); >> > + >> > + clk_disable_unprepare(data->uart_clk); >> > + >> > + return 0; >> > +} >> > + >> > +static int mtk8250_runtime_resume(struct device *dev) >> > +{ >> > + struct mtk8250_data *data = dev_get_drvdata(dev); >> > + int err; >> > + >> > + err = clk_prepare_enable(data->uart_clk); >> > + if (err) { >> > + dev_warn(dev, "Can't enable clock\n"); >> > + return err; >> > + } >> > + >> > + return 0; >> > +} >> > + >> > static void >> > mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) >> > { >> > @@ -130,19 +153,12 @@ mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) >> > static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, >> > struct mtk8250_data *data) >> > { >> > - int err; >> > - >> > data->uart_clk = devm_clk_get(&pdev->dev, NULL); >> > if (IS_ERR(data->uart_clk)) { >> > dev_warn(&pdev->dev, "Can't get uart clock\n"); >> > return PTR_ERR(data->uart_clk); >> > } >> > >> > - err = clk_prepare_enable(data->uart_clk); >> > - if (err) { >> > - dev_warn(&pdev->dev, "Can't prepare clock\n"); >> > - return err; >> > - } >> > p->uartclk = clk_get_rate(data->uart_clk); >> > >> > return 0; >> > @@ -193,14 +209,18 @@ static int mtk8250_probe(struct platform_device *pdev) >> > writel(0x0, uart.port.membase + >> > (MTK_UART_RATE_FIX << uart.port.regshift)); >> > >> > - data->line = serial8250_register_8250_port(&uart); >> > - if (data->line < 0) >> > - return data->line; >> > - >> > platform_set_drvdata(pdev, data); >> > >> > - pm_runtime_set_active(&pdev->dev); >> > pm_runtime_enable(&pdev->dev); >> > + if (!pm_runtime_enabled(&pdev->dev)) { >> > + err = mtk8250_runtime_resume(&pdev->dev); >> > + if (err) >> > + return err; >> > + } >> > + >> > + data->line = serial8250_register_8250_port(&uart); >> > + if (data->line < 0) >> > + return data->line; >> >> Why do you delete pm_runtime_set_active here? > > pm_runtime_set_active() communicates the initial state of the device to > the pm core. Previously the driver activated the device manually by > directly calling clk_prepare_enable(). Since this manual enabling of > the clock is removed in this patch, the device is no longer active, > hence pm_runtime_set_active() has to be removed. Sounds resonable. Acked-by: Matthias Brugger <matthias.bgg@gmail.com>
2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > The mtk 8250 needs two clocks, one for providing the baudrate and > one that needs to be enabled for register accesses. The latter has > not been supported, this patch adds support for it. It is optional > for now since not all SoCs provide a bus clock. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > .../devicetree/bindings/serial/mtk-uart.txt | 12 ++++++++-- > drivers/tty/serial/8250/8250_mtk.c | 28 ++++++++++++++++++---- > 2 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/serial/mtk-uart.txt b/Documentation/devicetree/bindings/serial/mtk-uart.txt > index 4415226..8d63f1d 100644 > --- a/Documentation/devicetree/bindings/serial/mtk-uart.txt > +++ b/Documentation/devicetree/bindings/serial/mtk-uart.txt > @@ -14,7 +14,14 @@ Required properties: > > - interrupts: A single interrupt specifier. > > -- clocks: Clock driving the hardware. > +- clocks : Must contain an entry for each entry in clock-names. > + See ../clocks/clock-bindings.txt for details. > +- clock-names: > + - "baud": The clock the baudrate is derived from > + - "bus": The bus clock for register accesses (optional) > + > +For compatibility with older device trees an unnamed clock is used for the > +baud clock if the baudclk does not exist. Do not use this for new designs. > > Example: > > @@ -22,5 +29,6 @@ Example: > compatible = "mediatek,mt6589-uart", "mediatek,mt6577-uart"; > reg = <0x11006000 0x400>; > interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_LOW>; > - clocks = <&uart_clk>; > + clocks = <&uart_clk>, <&bus_clk>; > + clock-names = "baud", "bus"; > }; > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > index 2f28bd0..8eb3876 100644 > --- a/drivers/tty/serial/8250/8250_mtk.c > +++ b/drivers/tty/serial/8250/8250_mtk.c > @@ -34,6 +34,7 @@ > struct mtk8250_data { > int line; > struct clk *uart_clk; > + struct clk *bus_clk; > }; Not sure if it would be better to rename the uart_clk to baud_clk which reflects the new naming scheme. > > static void > @@ -120,6 +121,7 @@ static int mtk8250_runtime_suspend(struct device *dev) > struct mtk8250_data *data = dev_get_drvdata(dev); > > clk_disable_unprepare(data->uart_clk); > + clk_disable_unprepare(data->bus_clk); bus_clk can be NULL. > > return 0; > } > @@ -135,6 +137,12 @@ static int mtk8250_runtime_resume(struct device *dev) > return err; > } > > + err = clk_prepare_enable(data->bus_clk); > + if (err) { > + dev_warn(dev, "Can't enable bus clock\n"); > + return err; > + } > + Same here.
On Tue, May 05, 2015 at 05:41:16PM +0200, Matthias Brugger wrote: > 2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > > The mtk 8250 needs two clocks, one for providing the baudrate and > > one that needs to be enabled for register accesses. The latter has > > not been supported, this patch adds support for it. It is optional > > for now since not all SoCs provide a bus clock. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > .../devicetree/bindings/serial/mtk-uart.txt | 12 ++++++++-- > > drivers/tty/serial/8250/8250_mtk.c | 28 ++++++++++++++++++---- > > 2 files changed, 34 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/serial/mtk-uart.txt b/Documentation/devicetree/bindings/serial/mtk-uart.txt > > index 4415226..8d63f1d 100644 > > --- a/Documentation/devicetree/bindings/serial/mtk-uart.txt > > +++ b/Documentation/devicetree/bindings/serial/mtk-uart.txt > > @@ -14,7 +14,14 @@ Required properties: > > > > - interrupts: A single interrupt specifier. > > > > -- clocks: Clock driving the hardware. > > +- clocks : Must contain an entry for each entry in clock-names. > > + See ../clocks/clock-bindings.txt for details. > > +- clock-names: > > + - "baud": The clock the baudrate is derived from > > + - "bus": The bus clock for register accesses (optional) > > + > > +For compatibility with older device trees an unnamed clock is used for the > > +baud clock if the baudclk does not exist. Do not use this for new designs. > > > > Example: > > > > @@ -22,5 +29,6 @@ Example: > > compatible = "mediatek,mt6589-uart", "mediatek,mt6577-uart"; > > reg = <0x11006000 0x400>; > > interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_LOW>; > > - clocks = <&uart_clk>; > > + clocks = <&uart_clk>, <&bus_clk>; > > + clock-names = "baud", "bus"; > > }; > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > > index 2f28bd0..8eb3876 100644 > > --- a/drivers/tty/serial/8250/8250_mtk.c > > +++ b/drivers/tty/serial/8250/8250_mtk.c > > @@ -34,6 +34,7 @@ > > struct mtk8250_data { > > int line; > > struct clk *uart_clk; > > + struct clk *bus_clk; > > }; > > Not sure if it would be better to rename the uart_clk to baud_clk > which reflects the new naming scheme. Can do. > > > > > static void > > @@ -120,6 +121,7 @@ static int mtk8250_runtime_suspend(struct device *dev) > > struct mtk8250_data *data = dev_get_drvdata(dev); > > > > clk_disable_unprepare(data->uart_clk); > > + clk_disable_unprepare(data->bus_clk); > > bus_clk can be NULL. The clk core handles NULL clks just fine. They are considered dummy clks. You can prepare/enable them, clk_get_rate of course returns nothing useful. Sascha
2015-05-05 17:50 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > On Tue, May 05, 2015 at 05:41:16PM +0200, Matthias Brugger wrote: >> 2015-04-27 8:49 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: >> > The mtk 8250 needs two clocks, one for providing the baudrate and >> > one that needs to be enabled for register accesses. The latter has >> > not been supported, this patch adds support for it. It is optional >> > for now since not all SoCs provide a bus clock. >> > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> > --- >> > .../devicetree/bindings/serial/mtk-uart.txt | 12 ++++++++-- >> > drivers/tty/serial/8250/8250_mtk.c | 28 ++++++++++++++++++---- >> > 2 files changed, 34 insertions(+), 6 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/serial/mtk-uart.txt b/Documentation/devicetree/bindings/serial/mtk-uart.txt >> > index 4415226..8d63f1d 100644 >> > --- a/Documentation/devicetree/bindings/serial/mtk-uart.txt >> > +++ b/Documentation/devicetree/bindings/serial/mtk-uart.txt >> > @@ -14,7 +14,14 @@ Required properties: >> > >> > - interrupts: A single interrupt specifier. >> > >> > -- clocks: Clock driving the hardware. >> > +- clocks : Must contain an entry for each entry in clock-names. >> > + See ../clocks/clock-bindings.txt for details. >> > +- clock-names: >> > + - "baud": The clock the baudrate is derived from >> > + - "bus": The bus clock for register accesses (optional) >> > + >> > +For compatibility with older device trees an unnamed clock is used for the >> > +baud clock if the baudclk does not exist. Do not use this for new designs. >> > >> > Example: >> > >> > @@ -22,5 +29,6 @@ Example: >> > compatible = "mediatek,mt6589-uart", "mediatek,mt6577-uart"; >> > reg = <0x11006000 0x400>; >> > interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_LOW>; >> > - clocks = <&uart_clk>; >> > + clocks = <&uart_clk>, <&bus_clk>; >> > + clock-names = "baud", "bus"; >> > }; >> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c >> > index 2f28bd0..8eb3876 100644 >> > --- a/drivers/tty/serial/8250/8250_mtk.c >> > +++ b/drivers/tty/serial/8250/8250_mtk.c >> > @@ -34,6 +34,7 @@ >> > struct mtk8250_data { >> > int line; >> > struct clk *uart_clk; >> > + struct clk *bus_clk; >> > }; >> >> Not sure if it would be better to rename the uart_clk to baud_clk >> which reflects the new naming scheme. > > Can do. If you do so you can add: Acked-by: Matthias Brugger <matthias.bgg@gmail.com> Thanks, Matthias
The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031: Linux 4.1-rc1 (2015-04-26 17:59:10 -0700) are available in the git repository at: git://git.pengutronix.de/git/sha/linux-2.6.git tags/v4.1-mtk-uart-clk-v2 for you to fetch changes up to 02e3f747bea47f6c02dc3f52e40869ea8ce5342a: tty: serial: 8250_mtk: Add support for bus clock (2015-04-27 08:39:06 +0200) ---------------------------------------------------------------- This series adds support for the second clock on the Mediatek UART. This is necessary for SoCs needing a bus clock to be enabled before the UART can be accessed. Changes since v1: - keep support old binding with unnamed clk - hopefully clarify commit message - do not leak in if() with unnecessary braces. ---------------------------------------------------------------- Sascha Hauer (4): tty: serial: 8250_mtk: remove unnecessary test tty: serial: 8250_mtk: Use devm_clk_get tty: serial: 8250_mtk: use pm_runtime callbacks for enabling tty: serial: 8250_mtk: Add support for bus clock .../devicetree/bindings/serial/mtk-uart.txt | 12 ++- drivers/tty/serial/8250/8250_mtk.c | 104 ++++++++++++--------- 2 files changed, 70 insertions(+), 46 deletions(-)