[PATCH 1/2] soc: qcom: geni: Don't ignore clk_round_rate() errors in geni_se_clk_tbl_get()

[PATCH 1/2] soc: qcom: geni: Don't ignore clk_round_rate() errors in geni_se_clk_tbl_get()

[Douglas Anderson]

The function clk_round_rate() is defined to return a "long", not an
"unsigned long".  That's because it might return a negative error
code.  Change the call in geni_se_clk_tbl_get() to check for errors.

NOTE: overall the idea that we should iterate over clk_round_rate() to
try to reconstruct a table already present in the clock driver is
questionable.  Specifically:
- This method relies on "clk_round_rate()" rounding up.
- This method only works if the table is sorted and has no duplicates.
...this patch doesn't try to fix those problems, it just makes the
error handling more correct.

Fixes: eddac5af0654 ("soc: qcom: Add GENI based QUP Wrapper driver")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/soc/qcom/qcom-geni-se.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index feed3db21c10..1b19b8428c4a 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -513,7 +513,7 @@ EXPORT_SYMBOL(geni_se_resources_on);
  */
 int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
 {
-	unsigned long freq = 0;
+	long freq = 0;
 	int i;
 
 	if (se->clk_perf_tbl) {
@@ -529,7 +529,7 @@ int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
 
 	for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) {
 		freq = clk_round_rate(se->clk, freq + 1);
-		if (!freq || freq == se->clk_perf_tbl[i - 1])
+		if (freq <= 0 || freq == se->clk_perf_tbl[i - 1])
 			break;
 		se->clk_perf_tbl[i] = freq;
 	}
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog

[PATCH 2/2] soc: qcom: geni: geni_se_clk_freq_match() should always accept multiples

[Douglas Anderson]

The geni_se_clk_freq_match() has some strange semantics.  Specifically
it is defined with two modes:
1. It can find a clock that's an exact multiple of the requested rate
2. If can find a non-exact match but it can't handle multiples then

...but callers should always be able to handle a clock that is a
multiple of the requested clock so mode #2 doesn't really make sense.
Let's change the semantics so that the non-exact match can also accept
multiples and then change the code to handle that.

The only caller of this code is the unlanded SPI driver [1] which
currently passes "exact = True", thus it should be safe to change the
semantics in this way.  ...and, in fact, the SPI driver should likely
be modified to pass "exact = False" (with the new semantics) since
that will allow it to work with SPI devices that request a clock rate
that doesn't exactly match a rate we can make.

[1] https://lkml.kernel.org/r/1535107336-2214-1-git-send-email-dkota@codeaurora.org

Fixes: eddac5af0654 ("soc: qcom: Add GENI based QUP Wrapper driver")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/soc/qcom/qcom-geni-se.c | 37 ++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 1b19b8428c4a..092b32a315c3 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -544,16 +544,17 @@ EXPORT_SYMBOL(geni_se_clk_tbl_get);
  * @se:		Pointer to the concerned serial engine.
  * @req_freq:	Requested clock frequency.
  * @index:	Index of the resultant frequency in the table.
- * @res_freq:	Resultant frequency which matches or is closer to the
- *		requested frequency.
+ * @res_freq:	Resultant frequency of the source clock.
  * @exact:	Flag to indicate exact multiple requirement of the requested
  *		frequency.
  *
- * This function is called by the protocol drivers to determine the matching
- * or exact multiple of the requested frequency, as provided by the serial
- * engine clock in order to meet the performance requirements. If there is
- * no matching or exact multiple of the requested frequency found, then it
- * selects the closest floor frequency, if exact flag is not set.
+ * This function is called by the protocol drivers to determine the best match
+ * of the requested frequency as provided by the serial engine clock in order
+ * to meet the performance requirements.
+ *
+ * If we return success:
+ * - if @exact is true  then @res_freq / <an_integer> == @req_freq
+ * - if @exact is false then @res_freq / <an_integer> <= @req_freq
  *
  * Return: 0 on success, standard Linux error codes on failure.
  */
@@ -564,6 +565,9 @@ int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
 	unsigned long *tbl;
 	int num_clk_levels;
 	int i;
+	unsigned long best_delta;
+	unsigned long new_delta;
+	unsigned int divider;
 
 	num_clk_levels = geni_se_clk_tbl_get(se, &tbl);
 	if (num_clk_levels < 0)
@@ -572,18 +576,21 @@ int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
 	if (num_clk_levels == 0)
 		return -EINVAL;
 
-	*res_freq = 0;
+	best_delta = 0;
 	for (i = 0; i < num_clk_levels; i++) {
-		if (!(tbl[i] % req_freq)) {
+		divider = DIV_ROUND_UP(tbl[i], req_freq);
+		new_delta = req_freq - tbl[i] / divider;
+		if (!best_delta || new_delta < best_delta) {
+			/* We have a new best! */
 			*index = i;
 			*res_freq = tbl[i];
-			return 0;
-		}
 
-		if (!(*res_freq) || ((tbl[i] > *res_freq) &&
-				     (tbl[i] < req_freq))) {
-			*index = i;
-			*res_freq = tbl[i];
+			/* If the new best is exact then we're done */
+			if (new_delta == 0)
+				return 0;
+
+			/* Record how close we got */
+			best_delta = new_delta;
 		}
 	}
 
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog

Re: [PATCH 2/2] soc: qcom: geni: geni_se_clk_freq_match() should always accept multiples

[Matthias Kaehlcke]

On Thu, Aug 30, 2018 at 11:36:12AM -0700, Douglas Anderson wrote:
> The geni_se_clk_freq_match() has some strange semantics.  Specifically
> it is defined with two modes:
> 1. It can find a clock that's an exact multiple of the requested rate
> 2. If can find a non-exact match but it can't handle multiples then

nit: s/If/It/

> 
> ...but callers should always be able to handle a clock that is a
> multiple of the requested clock so mode #2 doesn't really make sense.
> Let's change the semantics so that the non-exact match can also accept
> multiples and then change the code to handle that.
> 
> The only caller of this code is the unlanded SPI driver [1] which
> currently passes "exact = True", thus it should be safe to change the
> semantics in this way.  ...and, in fact, the SPI driver should likely
> be modified to pass "exact = False" (with the new semantics) since
> that will allow it to work with SPI devices that request a clock rate
> that doesn't exactly match a rate we can make.
> 
> [1] https://lkml.kernel.org/r/1535107336-2214-1-git-send-email-dkota@codeaurora.org
> 
> Fixes: eddac5af0654 ("soc: qcom: Add GENI based QUP Wrapper driver")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/soc/qcom/qcom-geni-se.c | 37 ++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 1b19b8428c4a..092b32a315c3 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -544,16 +544,17 @@ EXPORT_SYMBOL(geni_se_clk_tbl_get);
>   * @se:		Pointer to the concerned serial engine.
>   * @req_freq:	Requested clock frequency.
>   * @index:	Index of the resultant frequency in the table.
> - * @res_freq:	Resultant frequency which matches or is closer to the
> - *		requested frequency.
> + * @res_freq:	Resultant frequency of the source clock.
>   * @exact:	Flag to indicate exact multiple requirement of the requested
>   *		frequency.
>   *
> - * This function is called by the protocol drivers to determine the matching
> - * or exact multiple of the requested frequency, as provided by the serial
> - * engine clock in order to meet the performance requirements. If there is
> - * no matching or exact multiple of the requested frequency found, then it
> - * selects the closest floor frequency, if exact flag is not set.
> + * This function is called by the protocol drivers to determine the best match
> + * of the requested frequency as provided by the serial engine clock in order
> + * to meet the performance requirements.
> + *
> + * If we return success:
> + * - if @exact is true  then @res_freq / <an_integer> == @req_freq
> + * - if @exact is false then @res_freq / <an_integer> <= @req_freq
>   *
>   * Return: 0 on success, standard Linux error codes on failure.
>   */
> @@ -564,6 +565,9 @@ int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
>  	unsigned long *tbl;
>  	int num_clk_levels;
>  	int i;
> +	unsigned long best_delta;
> +	unsigned long new_delta;
> +	unsigned int divider;
>  
>  	num_clk_levels = geni_se_clk_tbl_get(se, &tbl);
>  	if (num_clk_levels < 0)
> @@ -572,18 +576,21 @@ int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
>  	if (num_clk_levels == 0)
>  		return -EINVAL;
>  
> -	*res_freq = 0;
> +	best_delta = 0;
>  	for (i = 0; i < num_clk_levels; i++) {
> -		if (!(tbl[i] % req_freq)) {
> +		divider = DIV_ROUND_UP(tbl[i], req_freq);
> +		new_delta = req_freq - tbl[i] / divider;
> +		if (!best_delta || new_delta < best_delta) {

nit: if you assigned best_delta to ULONG_MAX above you could omit the
check for !best_delta here, better to read IMO.

> +			/* We have a new best! */
>  			*index = i;
>  			*res_freq = tbl[i];
> -			return 0;
> -		}
>  
> -		if (!(*res_freq) || ((tbl[i] > *res_freq) &&
> -				     (tbl[i] < req_freq))) {
> -			*index = i;
> -			*res_freq = tbl[i];
> +			/* If the new best is exact then we're done */
> +			if (new_delta == 0)
> +				return 0;
> +
> +			/* Record how close we got */
> +			best_delta = new_delta;
>  		}
>  	}
>  

Looks good to me assuming that the statement "callers should always be
able to handle a clock that is a multiple of the requested clock" is
correct.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 2/2] soc: qcom: geni: geni_se_clk_freq_match() should always accept multiples

[Doug Anderson]

Hi,

On Wed, Sep 5, 2018 at 5:20 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> On Thu, Aug 30, 2018 at 11:36:12AM -0700, Douglas Anderson wrote:
>> The geni_se_clk_freq_match() has some strange semantics.  Specifically
>> it is defined with two modes:
>> 1. It can find a clock that's an exact multiple of the requested rate
>> 2. If can find a non-exact match but it can't handle multiples then
>
> nit: s/If/It/

Splleing was nvevr my strong suit.  Done.


>> +     best_delta = 0;
>>       for (i = 0; i < num_clk_levels; i++) {
>> -             if (!(tbl[i] % req_freq)) {
>> +             divider = DIV_ROUND_UP(tbl[i], req_freq);
>> +             new_delta = req_freq - tbl[i] / divider;
>> +             if (!best_delta || new_delta < best_delta) {
>
> nit: if you assigned best_delta to ULONG_MAX above you could omit the
> check for !best_delta here, better to read IMO.

Done.


> Looks good to me assuming that the statement "callers should always be
> able to handle a clock that is a multiple of the requested clock" is
> correct.

I think we should be OK given that currently there's no real callers.
If we really need to add another parameter to disallow multiples we
can always do it later.


> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Thanks for the reviews!

-Doug

Re: [PATCH 1/2] soc: qcom: geni: Don't ignore clk_round_rate() errors in geni_se_clk_tbl_get()

[Matthias Kaehlcke]

On Thu, Aug 30, 2018 at 11:36:11AM -0700, Douglas Anderson wrote:
> The function clk_round_rate() is defined to return a "long", not an
> "unsigned long".  That's because it might return a negative error
> code.  Change the call in geni_se_clk_tbl_get() to check for errors.
> 
> NOTE: overall the idea that we should iterate over clk_round_rate() to
> try to reconstruct a table already present in the clock driver is
> questionable.  Specifically:
> - This method relies on "clk_round_rate()" rounding up.
> - This method only works if the table is sorted and has no duplicates.
> ...this patch doesn't try to fix those problems, it just makes the
> error handling more correct.
> 
> Fixes: eddac5af0654 ("soc: qcom: Add GENI based QUP Wrapper driver")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/soc/qcom/qcom-geni-se.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index feed3db21c10..1b19b8428c4a 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -513,7 +513,7 @@ EXPORT_SYMBOL(geni_se_resources_on);
>   */
>  int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
>  {
> -	unsigned long freq = 0;
> +	long freq = 0;

nit: Since you are already touching this you could also remove the
pointless initialization, 'freq' is always assigned before it is used.

>  	int i;
>  
>  	if (se->clk_perf_tbl) {
> @@ -529,7 +529,7 @@ int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
>  
>  	for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) {
>  		freq = clk_round_rate(se->clk, freq + 1);
> -		if (!freq || freq == se->clk_perf_tbl[i - 1])
> +		if (freq <= 0 || freq == se->clk_perf_tbl[i - 1])
>  			break;
>  		se->clk_perf_tbl[i] = freq;
>  	}

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 1/2] soc: qcom: geni: Don't ignore clk_round_rate() errors in geni_se_clk_tbl_get()

[Doug Anderson]

Hi,

On Wed, Sep 5, 2018 at 4:37 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>>  int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
>>  {
>> -     unsigned long freq = 0;
>> +     long freq = 0;
>
> nit: Since you are already touching this you could also remove the
> pointless initialization, 'freq' is always assigned before it is used.

Done.

-Doug