| Message ID | 1586353607-32222-3-git-send-email-rnayak@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | DVFS for IO devices on sdm845 and sc7180 | expand |
Hi Rajendra, On 4/8/2020 7:16 PM, Rajendra Nayak wrote: > geni serial needs to express a perforamnce state requirement on CX *performance > 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: Akash Asthana <akashast@codeaurora.org> > Cc: linux-serial@vger.kernel.org > --- > drivers/tty/serial/qcom_geni_serial.c | 20 +++++++++++++++----- > include/linux/qcom-geni-se.h | 2 ++ > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 6119090..754eaf6 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> > @@ -961,7 +962,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); Is this change not intended for backward compatibility? If I don't pick DT change for Geni drivers, dev_pm_opp_set_rate is failing and causing functionality issues. > ser_clk_cfg = SER_CLK_EN; > ser_clk_cfg |= clk_div << CLK_DIV_SHFT; > > @@ -1198,8 +1199,10 @@ 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) { > + 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 +1321,16 @@ 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"); > + dev_pm_opp_of_add_table(&pdev->dev); > + > 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 +1338,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 +1355,14 @@ 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: > + dev_pm_opp_of_remove_table(&pdev->dev); do we need to call "dev_pm_opp_put_clkname" here and in remove to release clk resource grabbed by dev_pm_opp_set_clkname(&pdev->dev, "se");? Regards, Akash > + return ret; > } > > static int qcom_geni_serial_remove(struct platform_device *pdev) > @@ -1361,6 +1370,7 @@ 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; > > + dev_pm_opp_of_remove_table(&pdev->dev); > 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; > };
[].. >> @@ -1318,13 +1321,16 @@ 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"); >> + dev_pm_opp_of_add_table(&pdev->dev); >> + >> 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 +1338,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 +1355,14 @@ 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: >> + dev_pm_opp_of_remove_table(&pdev->dev); > do we need to call "dev_pm_opp_put_clkname" here and in remove to release clk resource grabbed by > > dev_pm_opp_set_clkname(&pdev->dev, "se");? Thanks for catching this, I did indeed try to call dev_pm_opp_put_clkname() but the way clk_put is handled in it seems buggy. I need to go back and fix it. Besides I realized dev_pm_opp_of_remove_table() does go ahead and do a clk_put on the clock. Viresh, whats the right way to clean up >> + port->se.opp = dev_pm_opp_set_clkname(&pdev->dev, "se"); >> + dev_pm_opp_of_add_table(&pdev->dev); is it 1. dev_pm_opp_of_remove_table() dev_pm_opp_put_clkname() or 2. dev_pm_opp_put_clkname() dev_pm_opp_of_remove_table() or, what this patch is currently doing, which is just calling dev_pm_opp_of_remove_table()? Note that both 1. and 2. today result in a crash, since they don't handle clk_put very well. I can send in a fix if you think dev_pm_opp_put_clkname is needed and in a certain order.
Rajendra Nayak <rnayak@codeaurora.org> 于2020年4月13日周一 下午10:22写道： > > > > On 4/10/2020 2:06 PM, Jun Nie wrote: > >>> @@ -961,7 +962,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); > > > > Hi Rajendra, > > Hi Jun, > > > I see lowest rpmhpd_opp_low_svs opp is for 75MHz. It is a bit higher > > for a serial. > > I am just curious about this. > > Well these OPP tables are technically what we call as fmax tables, which means > you can get the clock to a max of 75MHz at that perf level. You need to go > to the next perf level if you want to go higher. > That however does not mean that serial cannot run at clocks lower than 75Mhz. > > > I also want to confirm that the rpmhpd_opp_low_svs voltage restriction > > is for serial > > controller, not for clock controller? Because I see there is similar > > restriction to clock > > controller on another platform, the restriction is for branch clock, > > not leaf clock that > > consumer device will get. > > yes, its a serial controller restriction and not of the clock provider. > On your note on the branch clock vs leaf clock I am not sure I understand > the point you are making. For the leaf clock, I mean the clock that consumer get with devm_clk_get(). The branch clock means it is not for consumer directly, and its child clock or grandchild clock is for consumer. In that case, the restriction has to be done in clock driver, not in clock consumer driver. Sorry for confusing you. I just want to know more about what function this patch set provide. Because I am working on the clock controller restriction of fmax/voltage. Thanks! Jun > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 6119090..754eaf6 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> @@ -961,7 +962,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 +1199,10 @@ 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) { + 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 +1321,16 @@ 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"); + dev_pm_opp_of_add_table(&pdev->dev); + 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 +1338,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 +1355,14 @@ 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: + dev_pm_opp_of_remove_table(&pdev->dev); + return ret; } static int qcom_geni_serial_remove(struct platform_device *pdev) @@ -1361,6 +1370,7 @@ 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; + dev_pm_opp_of_remove_table(&pdev->dev); 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 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: Akash Asthana <akashast@codeaurora.org> Cc: linux-serial@vger.kernel.org --- drivers/tty/serial/qcom_geni_serial.c | 20 +++++++++++++++----- include/linux/qcom-geni-se.h | 2 ++ 2 files changed, 17 insertions(+), 5 deletions(-)