| Message ID | 1592222564-13556-2-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DVFS for IO devices on sdm845 and sc7180 | expand |
On Mon 15 Jun 05:02 PDT 2020, Rajendra Nayak wrote: > geni serial needs to express a perforamnce state requirement on CX > powerdomain depending on the frequency of the clock rates. > Use OPP table from DT to register with OPP framework and use > dev_pm_opp_set_rate() to set the clk/perf state. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Akash Asthana <akashast@codeaurora.org> > Cc: linux-serial@vger.kernel.org Picked up patch 1 and 2 through the qcom tree. As Mark requested, please don't lump together patches that doesn't actually depend on each other in the same series - it's quite confusing to know what to pick and not. Regards, Bjorn > --- > This patch needs to land via the msm tree. Greg had this already pulled in, > but later dropped it on my request. > No change in v6, just resposting it here so Bjorn/Andy can pull it in. > > drivers/tty/serial/qcom_geni_serial.c | 34 +++++++++++++++++++++++++++++----- > include/linux/qcom-geni-se.h | 4 ++++ > 2 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 457c0bf..a90f8ec 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -9,6 +9,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/pm_opp.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/pm_wakeirq.h> > @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, > goto out_restart_rx; > > uport->uartclk = clk_rate; > - clk_set_rate(port->se.clk, clk_rate); > + dev_pm_opp_set_rate(uport->dev, clk_rate); > ser_clk_cfg = SER_CLK_EN; > ser_clk_cfg |= clk_div << CLK_DIV_SHFT; > > @@ -1231,8 +1232,11 @@ static void qcom_geni_serial_pm(struct uart_port *uport, > if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) > geni_se_resources_on(&port->se); > else if (new_state == UART_PM_STATE_OFF && > - old_state == UART_PM_STATE_ON) > + old_state == UART_PM_STATE_ON) { > + /* Drop the performance state vote */ > + dev_pm_opp_set_rate(uport->dev, 0); > geni_se_resources_off(&port->se); > + } > } > > static const struct uart_ops qcom_geni_console_pops = { > @@ -1351,13 +1355,25 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) > if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap")) > port->cts_rts_swap = true; > > + port->se.opp_table = dev_pm_opp_set_clkname(&pdev->dev, "se"); > + if (IS_ERR(port->se.opp_table)) > + return PTR_ERR(port->se.opp_table); > + /* OPP table is optional */ > + ret = dev_pm_opp_of_add_table(&pdev->dev); > + if (!ret) { > + port->se.has_opp_table = true; > + } else if (ret != -ENODEV) { > + dev_err(&pdev->dev, "invalid OPP table in device tree\n"); > + return ret; > + } > + > uport->private_data = drv; > platform_set_drvdata(pdev, port); > port->handle_rx = console ? handle_rx_console : handle_rx_uart; > > ret = uart_add_one_port(drv, uport); > if (ret) > - return ret; > + goto err; > > irq_set_status_flags(uport->irq, IRQ_NOAUTOEN); > ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr, > @@ -1365,7 +1381,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) > if (ret) { > dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret); > uart_remove_one_port(drv, uport); > - return ret; > + goto err; > } > > /* > @@ -1382,11 +1398,16 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) > if (ret) { > device_init_wakeup(&pdev->dev, false); > uart_remove_one_port(drv, uport); > - return ret; > + goto err; > } > } > > return 0; > +err: > + if (port->se.has_opp_table) > + dev_pm_opp_of_remove_table(&pdev->dev); > + dev_pm_opp_put_clkname(port->se.opp_table); > + return ret; > } > > static int qcom_geni_serial_remove(struct platform_device *pdev) > @@ -1394,6 +1415,9 @@ static int qcom_geni_serial_remove(struct platform_device *pdev) > struct qcom_geni_serial_port *port = platform_get_drvdata(pdev); > struct uart_driver *drv = port->uport.private_data; > > + if (port->se.has_opp_table) > + dev_pm_opp_of_remove_table(&pdev->dev); > + dev_pm_opp_put_clkname(port->se.opp_table); > dev_pm_clear_wake_irq(&pdev->dev); > device_init_wakeup(&pdev->dev, false); > uart_remove_one_port(drv, &port->uport); > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h > index dd46494..6b78094 100644 > --- a/include/linux/qcom-geni-se.h > +++ b/include/linux/qcom-geni-se.h > @@ -33,6 +33,8 @@ struct clk; > * @clk: Handle to the core serial engine clock > * @num_clk_levels: Number of valid clock levels in clk_perf_tbl > * @clk_perf_tbl: Table of clock frequency input to serial engine clock > + * @opp_table: Pointer to the OPP table > + * @has_opp_table: Specifies if the SE has an OPP table > */ > struct geni_se { > void __iomem *base; > @@ -41,6 +43,8 @@ struct geni_se { > struct clk *clk; > unsigned int num_clk_levels; > unsigned long *clk_perf_tbl; > + struct opp_table *opp_table; > + bool has_opp_table; > }; > > /* Common SE registers */ > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
Quoting Rajendra Nayak (2020-06-15 05:02:39) > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 457c0bf..a90f8ec 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -9,6 +9,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/pm_opp.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <linux/pm_wakeirq.h> > @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, > goto out_restart_rx; > > uport->uartclk = clk_rate; > - clk_set_rate(port->se.clk, clk_rate); > + dev_pm_opp_set_rate(uport->dev, clk_rate); If there isn't an OPP table for the device because it is optional then how can we unconditionally call dev_pm_opp_set_rate()? > ser_clk_cfg = SER_CLK_EN; > ser_clk_cfg |= clk_div << CLK_DIV_SHFT; > > @@ -1231,8 +1232,11 @@ static void qcom_geni_serial_pm(struct uart_port *uport, > if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) > geni_se_resources_on(&port->se); > else if (new_state == UART_PM_STATE_OFF && > - old_state == UART_PM_STATE_ON) > + old_state == UART_PM_STATE_ON) { > + /* Drop the performance state vote */ > + dev_pm_opp_set_rate(uport->dev, 0); > geni_se_resources_off(&port->se); > + } > } > > static const struct uart_ops qcom_geni_console_pops = { > @@ -1351,13 +1355,25 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) > if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap")) > port->cts_rts_swap = true; > > + port->se.opp_table = dev_pm_opp_set_clkname(&pdev->dev, "se"); > + if (IS_ERR(port->se.opp_table)) > + return PTR_ERR(port->se.opp_table); > + /* OPP table is optional */ > + ret = dev_pm_opp_of_add_table(&pdev->dev); > + if (!ret) { > + port->se.has_opp_table = true; > + } else if (ret != -ENODEV) { > + dev_err(&pdev->dev, "invalid OPP table in device tree\n"); > + return ret; > + } At least it looks optional here.
On 6/30/2020 4:47 AM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2020-06-15 05:02:39) >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >> index 457c0bf..a90f8ec 100644 >> --- a/drivers/tty/serial/qcom_geni_serial.c >> +++ b/drivers/tty/serial/qcom_geni_serial.c >> @@ -9,6 +9,7 @@ >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/of_device.h> >> +#include <linux/pm_opp.h> >> #include <linux/platform_device.h> >> #include <linux/pm_runtime.h> >> #include <linux/pm_wakeirq.h> >> @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, >> goto out_restart_rx; >> >> uport->uartclk = clk_rate; >> - clk_set_rate(port->se.clk, clk_rate); >> + dev_pm_opp_set_rate(uport->dev, clk_rate); > > If there isn't an OPP table for the device because it is optional then > how can we unconditionally call dev_pm_opp_set_rate()? because we have 'aca48b6 opp: Manage empty OPP tables with clk handle' to handle this.
On 30-06-20, 08:31, Rajendra Nayak wrote: > > > On 6/30/2020 4:47 AM, Stephen Boyd wrote: > > Quoting Rajendra Nayak (2020-06-15 05:02:39) > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > > > index 457c0bf..a90f8ec 100644 > > > --- a/drivers/tty/serial/qcom_geni_serial.c > > > +++ b/drivers/tty/serial/qcom_geni_serial.c > > > @@ -9,6 +9,7 @@ > > > #include <linux/module.h> > > > #include <linux/of.h> > > > #include <linux/of_device.h> > > > +#include <linux/pm_opp.h> > > > #include <linux/platform_device.h> > > > #include <linux/pm_runtime.h> > > > #include <linux/pm_wakeirq.h> > > > @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, > > > goto out_restart_rx; > > > uport->uartclk = clk_rate; > > > - clk_set_rate(port->se.clk, clk_rate); > > > + dev_pm_opp_set_rate(uport->dev, clk_rate); > > > > If there isn't an OPP table for the device because it is optional then > > how can we unconditionally call dev_pm_opp_set_rate()? Looks like some *Maintainers* aren't paying enough attention lately ;) Just kidding. > because we have 'aca48b6 opp: Manage empty OPP tables with clk handle' to handle this.
On 6/25/2020 10:38 AM, Bjorn Andersson wrote: > On Mon 15 Jun 05:02 PDT 2020, Rajendra Nayak wrote: > >> geni serial needs to express a perforamnce state requirement on CX >> powerdomain depending on the frequency of the clock rates. >> Use OPP table from DT to register with OPP framework and use >> dev_pm_opp_set_rate() to set the clk/perf state. >> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Akash Asthana <akashast@codeaurora.org> >> Cc: linux-serial@vger.kernel.org > > Picked up patch 1 and 2 through the qcom tree. thanks Bjorn > As Mark requested, please don't lump together patches that doesn't > actually depend on each other in the same series - it's quite confusing > to know what to pick and not. Sorry for the confusion, I will try and split them into driver specific patches henceforth. For this series though, I see that you have merged the entire ICC series for QUP and QSPI into for-next of the qcom tree. In which case it perhaps makes sense for you to pull in the QSPI change from this series as well? i.e PATCH 6/6. If so, I will rebase the patch on your for-next and resend (Its already Ack'ed by Mark) thanks, Rajendra
Quoting Viresh Kumar (2020-06-29 20:05:52) > On 30-06-20, 08:31, Rajendra Nayak wrote: > > > > > > On 6/30/2020 4:47 AM, Stephen Boyd wrote: > > > Quoting Rajendra Nayak (2020-06-15 05:02:39) > > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > > > > index 457c0bf..a90f8ec 100644 > > > > --- a/drivers/tty/serial/qcom_geni_serial.c > > > > +++ b/drivers/tty/serial/qcom_geni_serial.c > > > > @@ -9,6 +9,7 @@ > > > > #include <linux/module.h> > > > > #include <linux/of.h> > > > > #include <linux/of_device.h> > > > > +#include <linux/pm_opp.h> > > > > #include <linux/platform_device.h> > > > > #include <linux/pm_runtime.h> > > > > #include <linux/pm_wakeirq.h> > > > > @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, > > > > goto out_restart_rx; > > > > uport->uartclk = clk_rate; > > > > - clk_set_rate(port->se.clk, clk_rate); > > > > + dev_pm_opp_set_rate(uport->dev, clk_rate); > > > > > > If there isn't an OPP table for the device because it is optional then > > > how can we unconditionally call dev_pm_opp_set_rate()? > > Looks like some *Maintainers* aren't paying enough attention lately ;) > > Just kidding. > It seems that dev_pm_opp_set_rate() calls _find_opp_table() and finds something that isn't an error pointer but then dev_pm_opp_of_add_table() returns an error value because there isn't an operating-points property in DT. We're getting saved because this driver also happens to call dev_pm_opp_set_clkname() which allocates the OPP table a second time (because the first time it got freed when dev_pm_opp_of_add_table() return -ENODEV because the property was missing). Why do we need 'has_opp_table' logic? It seems that we have to keep track of the fact that dev_pm_opp_of_add_table() failed so that we don't put the table again, but then dev_pm_opp_set_clkname() can be called to allocate the table regardless. This maintainer is paying very close attention to super confusing code like this: if (drv->has_opp_table) dev_pm_opp_of_remove_table(dev); dev_pm_opp_put_clkname(drv->opp_table); which reads as "if I have an opp table remove it and oh by the way remove the clk name for this opp table pointer I also happen to always have". Maybe I would be happier if dev_pm_opp_of_table() went away and we just had dev_pm_opp_add_table(const struct opp_config *config) that did all the things for us like set a clk name, set the supported hw, set the prop name, etc. based on the single config struct pointer and also parsed out the OPP table from DT or just ignored that if there isn't any operating-points property. Then the caller wouldn't need to keep track of 'if has_opp_table' because it doesn't seem to actually care and the core is happy to allocate a table for the device anyway so long as it sets a clk name.
On 21-07-20, 01:43, Stephen Boyd wrote: > It seems that dev_pm_opp_set_rate() calls _find_opp_table() and finds > something that isn't an error pointer but then dev_pm_opp_of_add_table() > returns an error value because there isn't an operating-points property > in DT. We're getting saved because this driver also happens to call > dev_pm_opp_set_clkname() which allocates the OPP table a second time > (because the first time it got freed when dev_pm_opp_of_add_table() > return -ENODEV because the property was missing). > > Why do we need 'has_opp_table' logic? It seems that we have to keep > track of the fact that dev_pm_opp_of_add_table() failed so that we don't > put the table again, but then dev_pm_opp_set_clkname() can be called > to allocate the table regardless. > > This maintainer is paying very close attention :) > to super confusing code like > this: > > if (drv->has_opp_table) > dev_pm_opp_of_remove_table(dev); > dev_pm_opp_put_clkname(drv->opp_table); > > which reads as "if I have an opp table remove it and oh by the way > remove the clk name for this opp table pointer I also happen to always > have". > > Maybe I would be happier if dev_pm_opp_of_table() went away and we just > had dev_pm_opp_add_table(const struct opp_config *config) that did all > the things for us like set a clk name, set the supported hw, set the > prop name, etc. based on the single config struct pointer and also > parsed out the OPP table from DT or just ignored that if there isn't any > operating-points property. Then the caller wouldn't need to keep track > of 'if has_opp_table' because it doesn't seem to actually care and the > core is happy to allocate a table for the device anyway so long as it > sets a clk name. The config style wouldn't work as well as we don't really want to allocate an OPP table if the property isn't found in DT. All the mess is coming from the fact that I wanted to make it easy for the generic drivers to have code which can do opp-set-rate or clk-set-rate depending on how the platform is configured. While the intention was fine, the end result is still not great as you figured out. Because we need to keep a flag to make the right decision anyway, I wonder if doing this is the best solution we have at hand. if (opp-table-present) opp_set_rate(); else clk_set_rate(); Or maybe stop printing errors from dev_pm_opp_of_remove_table() if the OPP table isn't found. And so we can get rid of the flag.
On 22-07-20, 10:54, Viresh Kumar wrote: > On 21-07-20, 01:43, Stephen Boyd wrote: > > It seems that dev_pm_opp_set_rate() calls _find_opp_table() and finds > > something that isn't an error pointer but then dev_pm_opp_of_add_table() > > returns an error value because there isn't an operating-points property > > in DT. We're getting saved because this driver also happens to call > > dev_pm_opp_set_clkname() which allocates the OPP table a second time > > (because the first time it got freed when dev_pm_opp_of_add_table() > > return -ENODEV because the property was missing). > > > > Why do we need 'has_opp_table' logic? It seems that we have to keep > > track of the fact that dev_pm_opp_of_add_table() failed so that we don't > > put the table again, but then dev_pm_opp_set_clkname() can be called > > to allocate the table regardless. I have sent a patchset to clean this stuff up a bit now.
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 457c0bf..a90f8ec 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -9,6 +9,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/pm_opp.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/pm_wakeirq.h> @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, goto out_restart_rx; uport->uartclk = clk_rate; - clk_set_rate(port->se.clk, clk_rate); + dev_pm_opp_set_rate(uport->dev, clk_rate); ser_clk_cfg = SER_CLK_EN; ser_clk_cfg |= clk_div << CLK_DIV_SHFT; @@ -1231,8 +1232,11 @@ static void qcom_geni_serial_pm(struct uart_port *uport, if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) geni_se_resources_on(&port->se); else if (new_state == UART_PM_STATE_OFF && - old_state == UART_PM_STATE_ON) + old_state == UART_PM_STATE_ON) { + /* Drop the performance state vote */ + dev_pm_opp_set_rate(uport->dev, 0); geni_se_resources_off(&port->se); + } } static const struct uart_ops qcom_geni_console_pops = { @@ -1351,13 +1355,25 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap")) port->cts_rts_swap = true; + port->se.opp_table = dev_pm_opp_set_clkname(&pdev->dev, "se"); + if (IS_ERR(port->se.opp_table)) + return PTR_ERR(port->se.opp_table); + /* OPP table is optional */ + ret = dev_pm_opp_of_add_table(&pdev->dev); + if (!ret) { + port->se.has_opp_table = true; + } else if (ret != -ENODEV) { + dev_err(&pdev->dev, "invalid OPP table in device tree\n"); + return ret; + } + uport->private_data = drv; platform_set_drvdata(pdev, port); port->handle_rx = console ? handle_rx_console : handle_rx_uart; ret = uart_add_one_port(drv, uport); if (ret) - return ret; + goto err; irq_set_status_flags(uport->irq, IRQ_NOAUTOEN); ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr, @@ -1365,7 +1381,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) if (ret) { dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret); uart_remove_one_port(drv, uport); - return ret; + goto err; } /* @@ -1382,11 +1398,16 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) if (ret) { device_init_wakeup(&pdev->dev, false); uart_remove_one_port(drv, uport); - return ret; + goto err; } } return 0; +err: + if (port->se.has_opp_table) + dev_pm_opp_of_remove_table(&pdev->dev); + dev_pm_opp_put_clkname(port->se.opp_table); + return ret; } static int qcom_geni_serial_remove(struct platform_device *pdev) @@ -1394,6 +1415,9 @@ static int qcom_geni_serial_remove(struct platform_device *pdev) struct qcom_geni_serial_port *port = platform_get_drvdata(pdev); struct uart_driver *drv = port->uport.private_data; + if (port->se.has_opp_table) + dev_pm_opp_of_remove_table(&pdev->dev); + dev_pm_opp_put_clkname(port->se.opp_table); dev_pm_clear_wake_irq(&pdev->dev); device_init_wakeup(&pdev->dev, false); uart_remove_one_port(drv, &port->uport); diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h index dd46494..6b78094 100644 --- a/include/linux/qcom-geni-se.h +++ b/include/linux/qcom-geni-se.h @@ -33,6 +33,8 @@ struct clk; * @clk: Handle to the core serial engine clock * @num_clk_levels: Number of valid clock levels in clk_perf_tbl * @clk_perf_tbl: Table of clock frequency input to serial engine clock + * @opp_table: Pointer to the OPP table + * @has_opp_table: Specifies if the SE has an OPP table */ struct geni_se { void __iomem *base; @@ -41,6 +43,8 @@ struct geni_se { struct clk *clk; unsigned int num_clk_levels; unsigned long *clk_perf_tbl; + struct opp_table *opp_table; + bool has_opp_table; }; /* Common SE registers */