| Message ID | 1587132279-27659-2-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | DVFS for IO devices on sdm845 and sc7180 | expand |
Hi Rajendra, On Fri, Apr 17, 2020 at 07:34:23PM +0530, 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> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Akash Asthana <akashast@codeaurora.org> > Cc: linux-serial@vger.kernel.org > --- > drivers/tty/serial/qcom_geni_serial.c | 30 +++++++++++++++++++++++++----- > include/linux/qcom-geni-se.h | 2 ++ > 2 files changed, 27 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 6119090..151012c 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> > @@ -128,6 +129,7 @@ struct qcom_geni_serial_port { > int wakeup_irq; > bool rx_tx_swap; > bool cts_rts_swap; > + bool opp_table; The name of the variable suggests that it holds a OPP table, something like 'has_opp_table' would be clearer. > }; > > static const struct uart_ops qcom_geni_console_pops; > @@ -961,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; > > @@ -1198,8 +1200,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 = { > @@ -1318,13 +1323,20 @@ 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 = dev_pm_opp_set_clkname(&pdev->dev, "se"); > + if (IS_ERR(port->se.opp)) > + return PTR_ERR(port->se.opp); > + /* OPP table is optional */ > + if (!dev_pm_opp_of_add_table(&pdev->dev)) Even if the OPP table is optional you probably want to fail if the error is anything other than -ENODEV ("'operating-points' property is not found or is invalid data in device node."). > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h > index dd46494..737e713 100644 > --- a/include/linux/qcom-geni-se.h > +++ b/include/linux/qcom-geni-se.h > @@ -24,6 +24,7 @@ enum geni_se_protocol_type { > > struct geni_wrapper; > struct clk; > +struct opp_table; > > /** > * struct geni_se - GENI Serial Engine > @@ -39,6 +40,7 @@ struct geni_se { > struct device *dev; > struct geni_wrapper *wrapper; > struct clk *clk; > + struct opp_table *opp; This name suggests that the variable holds a single OPP ('struct dev_pm_opp'). Most other code uses the name 'opp_table', which also seems a good candidate here.
On 4/17/2020 11:30 PM, Matthias Kaehlcke wrote: > Hi Rajendra, Hey Matthias, > > On Fri, Apr 17, 2020 at 07:34:23PM +0530, 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> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Akash Asthana <akashast@codeaurora.org> >> Cc: linux-serial@vger.kernel.org >> --- >> drivers/tty/serial/qcom_geni_serial.c | 30 +++++++++++++++++++++++++----- >> include/linux/qcom-geni-se.h | 2 ++ >> 2 files changed, 27 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >> index 6119090..151012c 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> >> @@ -128,6 +129,7 @@ struct qcom_geni_serial_port { >> int wakeup_irq; >> bool rx_tx_swap; >> bool cts_rts_swap; >> + bool opp_table; > > The name of the variable suggests that it holds a OPP table, something > like 'has_opp_table' would be clearer. I agree with your suggestions, I will update the variable names when I respin. > >> }; >> >> static const struct uart_ops qcom_geni_console_pops; >> @@ -961,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; >> >> @@ -1198,8 +1200,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 = { >> @@ -1318,13 +1323,20 @@ 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 = dev_pm_opp_set_clkname(&pdev->dev, "se"); >> + if (IS_ERR(port->se.opp)) >> + return PTR_ERR(port->se.opp); >> + /* OPP table is optional */ >> + if (!dev_pm_opp_of_add_table(&pdev->dev)) > > Even if the OPP table is optional you probably want to fail if the error > is anything other than -ENODEV ("'operating-points' property is not found > or is invalid data in device node."). sounds valid. I will fix it up and respin. Will just wait a few days to see if I get any more reviews and feedback. > >> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h >> index dd46494..737e713 100644 >> --- a/include/linux/qcom-geni-se.h >> +++ b/include/linux/qcom-geni-se.h >> @@ -24,6 +24,7 @@ enum geni_se_protocol_type { >> >> struct geni_wrapper; >> struct clk; >> +struct opp_table; >> >> /** >> * struct geni_se - GENI Serial Engine >> @@ -39,6 +40,7 @@ struct geni_se { >> struct device *dev; >> struct geni_wrapper *wrapper; >> struct clk *clk; >> + struct opp_table *opp; > > This name suggests that the variable holds a single OPP ('struct > dev_pm_opp'). Most other code uses the name 'opp_table', which > also seems a good candidate here. Agree, thanks again for taking time to review.
Hey Bjorn, > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h > index dd46494..737e713 100644 > --- a/include/linux/qcom-geni-se.h > +++ b/include/linux/qcom-geni-se.h > @@ -24,6 +24,7 @@ enum geni_se_protocol_type { > > struct geni_wrapper; > struct clk; > +struct opp_table; > > /** > * struct geni_se - GENI Serial Engine > @@ -39,6 +40,7 @@ struct geni_se { > struct device *dev; > struct geni_wrapper *wrapper; > struct clk *clk; > + struct opp_table *opp; I just realized this is going to cause merge issues across geni serial and geni spi driver (PATCH 02/17 in this series) unless all of this goes via your tree. I see this is also an issue with the ongoing ICC patch series [1] Do you or Greg have any thoughts on how this common header across various drivers issue should be resolved to avoid conflicts while merging? - Rajendra [1] https://patchwork.kernel.org/patch/11491015/
On Wed, Apr 22, 2020 at 02:52:21PM +0530, Rajendra Nayak wrote: > Hey Bjorn, > > > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h > > index dd46494..737e713 100644 > > --- a/include/linux/qcom-geni-se.h > > +++ b/include/linux/qcom-geni-se.h > > @@ -24,6 +24,7 @@ enum geni_se_protocol_type { > > struct geni_wrapper; > > struct clk; > > +struct opp_table; > > /** > > * struct geni_se - GENI Serial Engine > > @@ -39,6 +40,7 @@ struct geni_se { > > struct device *dev; > > struct geni_wrapper *wrapper; > > struct clk *clk; > > + struct opp_table *opp; > > I just realized this is going to cause merge issues across geni serial and geni spi > driver (PATCH 02/17 in this series) unless all of this goes via your tree. > I see this is also an issue with the ongoing ICC patch series [1] > > Do you or Greg have any thoughts on how this common header across various drivers > issue should be resolved to avoid conflicts while merging? I can ack it and you can take it through a different tree as part of the whole series to prevent that. thanks, greg k-h
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 6119090..151012c 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> @@ -128,6 +129,7 @@ struct qcom_geni_serial_port { int wakeup_irq; bool rx_tx_swap; bool cts_rts_swap; + bool opp_table; }; static const struct uart_ops qcom_geni_console_pops; @@ -961,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; @@ -1198,8 +1200,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 = { @@ -1318,13 +1323,20 @@ 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 = dev_pm_opp_set_clkname(&pdev->dev, "se"); + if (IS_ERR(port->se.opp)) + return PTR_ERR(port->se.opp); + /* OPP table is optional */ + if (!dev_pm_opp_of_add_table(&pdev->dev)) + port->opp_table = true; + 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, @@ -1332,7 +1344,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; } /* @@ -1349,11 +1361,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->opp_table) + dev_pm_opp_of_remove_table(&pdev->dev); + dev_pm_opp_put_clkname(port->se.opp); + return ret; } static int qcom_geni_serial_remove(struct platform_device *pdev) @@ -1361,6 +1378,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->opp_table) + dev_pm_opp_of_remove_table(&pdev->dev); + dev_pm_opp_put_clkname(port->se.opp); 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..737e713 100644 --- a/include/linux/qcom-geni-se.h +++ b/include/linux/qcom-geni-se.h @@ -24,6 +24,7 @@ enum geni_se_protocol_type { struct geni_wrapper; struct clk; +struct opp_table; /** * struct geni_se - GENI Serial Engine @@ -39,6 +40,7 @@ struct geni_se { struct device *dev; struct geni_wrapper *wrapper; struct clk *clk; + struct opp_table *opp; unsigned int num_clk_levels; unsigned long *clk_perf_tbl; };
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> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Akash Asthana <akashast@codeaurora.org> Cc: linux-serial@vger.kernel.org --- drivers/tty/serial/qcom_geni_serial.c | 30 +++++++++++++++++++++++++----- include/linux/qcom-geni-se.h | 2 ++ 2 files changed, 27 insertions(+), 5 deletions(-)