[PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Rajendra Nayak]

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(-)

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;
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Matthias Kaehlcke]

Hi Rajendra,

On Wed, Apr 08, 2020 at 07:16:28PM +0530, Rajendra Nayak wrote:
> 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(-)
> 
> 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_set_clkname() can fail for multiple reasons, it seems an error
check would be warranted.

Is it actually necessary to save the OPP table in 'struct geni_se'? Both
the serial and the SPI driver save the table, but don't use it later (nor
does the SE driver).

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Akash Asthana]

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;
>   };

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Jun Nie]

> > @@ -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,

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.

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.

Jun

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Akash Asthana]

Hi Rajendra,

On 4/10/2020 12:26 PM, Akash Asthana wrote:
> 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.

oops Sorry, 1st patch is intended for backward compatibility. Which I 
missed earlier.

Regards,

Akash

>
>>       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;
>>   };
>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Rajendra Nayak]

Hi Matthias,

On 4/9/2020 11:15 PM, Matthias Kaehlcke wrote:
> Hi Rajendra,
> 
> On Wed, Apr 08, 2020 at 07:16:28PM +0530, Rajendra Nayak wrote:
>> 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(-)
>>
>> 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_set_clkname() can fail for multiple reasons, it seems an error
> check would be warranted.

right, looks like I should put some error check there

> Is it actually necessary to save the OPP table in 'struct geni_se'? Both
> the serial and the SPI driver save the table, but don't use it later (nor
> does the SE driver).

I think I did that initially because I wanted to use that to call into
dev_pm_opp_put_clkname during cleanup. That however never worked since
the way the clk_put is done in dev_pm_opp_put_clkname() and _opp_table_kref_release()
seems buggy. I kind of forgot about fixing it up, I will figure our whats the right
way to do it, and either not call dev_pm_opp_put_clkname() or not store the
opp table returned by it.

thanks for taking time to review.

- Rajendra
  

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Rajendra Nayak]

[]..

>> @@ -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.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Rajendra Nayak]

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.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 02/21] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Jun Nie]

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