[PATCH] spi: sh-msiof: Update calculation of frequency dividing

[PATCH] spi: sh-msiof: Update calculation of frequency dividing

[Nobuhiro Iwamatsu]

sh-msiof of frequency dividing does not perform the calculation, driver have
to manage setting value in the table. It is not possible to set frequency
dividing value close to the actual data in this way. This changes from
frequency dividing of table management to setting by calculation.
This driver is able to set a value close to the actual data.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
---
 drivers/spi/spi-sh-msiof.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 96a5fc0..58b1bfe 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -241,42 +241,37 @@ static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
 
 static struct {
 	unsigned short div;
-	unsigned short scr;
-} const sh_msiof_spi_clk_table[] = {
-	{ 1,	SCR_BRPS( 1) | SCR_BRDV_DIV_1 },
-	{ 2,	SCR_BRPS( 1) | SCR_BRDV_DIV_2 },
-	{ 4,	SCR_BRPS( 1) | SCR_BRDV_DIV_4 },
-	{ 8,	SCR_BRPS( 1) | SCR_BRDV_DIV_8 },
-	{ 16,	SCR_BRPS( 1) | SCR_BRDV_DIV_16 },
-	{ 32,	SCR_BRPS( 1) | SCR_BRDV_DIV_32 },
-	{ 64,	SCR_BRPS(32) | SCR_BRDV_DIV_2 },
-	{ 128,	SCR_BRPS(32) | SCR_BRDV_DIV_4 },
-	{ 256,	SCR_BRPS(32) | SCR_BRDV_DIV_8 },
-	{ 512,	SCR_BRPS(32) | SCR_BRDV_DIV_16 },
-	{ 1024,	SCR_BRPS(32) | SCR_BRDV_DIV_32 },
+	unsigned short brdv;
+} const sh_msiof_spi_div_table[] = {
+	{ 1,	SCR_BRDV_DIV_1 },
+	{ 2,	SCR_BRDV_DIV_2 },
+	{ 4,	SCR_BRDV_DIV_4 },
+	{ 8,	SCR_BRDV_DIV_8 },
+	{ 16,	SCR_BRDV_DIV_16 },
+	{ 32,	SCR_BRDV_DIV_32 },
 };
 
 static void sh_msiof_spi_set_clk_regs(struct sh_msiof_spi_priv *p,
 				      unsigned long parent_rate, u32 spi_hz)
 {
 	unsigned long div = 1024;
+	unsigned long brps, scr;
 	size_t k;
 
 	if (!WARN_ON(!spi_hz || !parent_rate))
 		div = DIV_ROUND_UP(parent_rate, spi_hz);
 
-	/* TODO: make more fine grained */
-
-	for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_clk_table); k++) {
-		if (sh_msiof_spi_clk_table[k].div >= div)
-			break;
+	for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_div_table); k++) {
+		brps = DIV_ROUND_UP(div, sh_msiof_spi_div_table[k].div);
+		if (brps > 32) /* max of brsv is 32 */
+			continue;
+		break;
 	}
 
-	k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
-
-	sh_msiof_write(p, TSCR, sh_msiof_spi_clk_table[k].scr);
+	scr = sh_msiof_spi_div_table[k].brdv | (brps -1) << 8;
+	sh_msiof_write(p, TSCR, scr);
 	if (!(p->chipdata->master_flags & SPI_MASTER_MUST_TX))
-		sh_msiof_write(p, RSCR, sh_msiof_spi_clk_table[k].scr);
+		sh_msiof_write(p, RSCR, scr);
 }
 
 static void sh_msiof_spi_set_pin_regs(struct sh_msiof_spi_priv *p,
-- 
2.1.3

Re: [PATCH] spi: sh-msiof: Update calculation of frequency dividing

[Geert Uytterhoeven]

Hi Iwamatsu-san,

On Wed, Jan 14, 2015 at 8:25 AM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.yj@renesas.com> wrote:
> sh-msiof of frequency dividing does not perform the calculation, driver have
> to manage setting value in the table. It is not possible to set frequency
> dividing value close to the actual data in this way. This changes from
> frequency dividing of table management to setting by calculation.
> This driver is able to set a value close to the actual data.

Thanks for your patch!

> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 96a5fc0..58b1bfe 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -241,42 +241,37 @@ static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
>
>  static struct {
>         unsigned short div;
> -       unsigned short scr;
> -} const sh_msiof_spi_clk_table[] = {
> -       { 1,    SCR_BRPS( 1) | SCR_BRDV_DIV_1 },
> -       { 2,    SCR_BRPS( 1) | SCR_BRDV_DIV_2 },
> -       { 4,    SCR_BRPS( 1) | SCR_BRDV_DIV_4 },
> -       { 8,    SCR_BRPS( 1) | SCR_BRDV_DIV_8 },
> -       { 16,   SCR_BRPS( 1) | SCR_BRDV_DIV_16 },
> -       { 32,   SCR_BRPS( 1) | SCR_BRDV_DIV_32 },
> -       { 64,   SCR_BRPS(32) | SCR_BRDV_DIV_2 },
> -       { 128,  SCR_BRPS(32) | SCR_BRDV_DIV_4 },
> -       { 256,  SCR_BRPS(32) | SCR_BRDV_DIV_8 },
> -       { 512,  SCR_BRPS(32) | SCR_BRDV_DIV_16 },
> -       { 1024, SCR_BRPS(32) | SCR_BRDV_DIV_32 },
> +       unsigned short brdv;
> +} const sh_msiof_spi_div_table[] = {
> +       { 1,    SCR_BRDV_DIV_1 },
> +       { 2,    SCR_BRDV_DIV_2 },
> +       { 4,    SCR_BRDV_DIV_4 },
> +       { 8,    SCR_BRDV_DIV_8 },
> +       { 16,   SCR_BRDV_DIV_16 },
> +       { 32,   SCR_BRDV_DIV_32 },
>  };
>
>  static void sh_msiof_spi_set_clk_regs(struct sh_msiof_spi_priv *p,
>                                       unsigned long parent_rate, u32 spi_hz)
>  {
>         unsigned long div = 1024;
> +       unsigned long brps, scr;

"u32", as these are (parts of) 32-bit register values.

>         size_t k;
>
>         if (!WARN_ON(!spi_hz || !parent_rate))
>                 div = DIV_ROUND_UP(parent_rate, spi_hz);
>
> -       /* TODO: make more fine grained */
> -
> -       for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_clk_table); k++) {
> -               if (sh_msiof_spi_clk_table[k].div >= div)
> -                       break;
> +       for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_div_table); k++) {
> +               brps = DIV_ROUND_UP(div, sh_msiof_spi_div_table[k].div);
> +               if (brps > 32) /* max of brsv is 32 */

max of brdv

> +                       continue;
> +               break;

Having a conditional "continue" followed by a "break" looks strange.

        if (brps <= 32) /* max of brdv is 32 */
                break

>         }

"brps" may be larger than 32 after the loop.
In the old code, the min_t() below handled that case.

> -       k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
> -
> -       sh_msiof_write(p, TSCR, sh_msiof_spi_clk_table[k].scr);
> +       scr = sh_msiof_spi_div_table[k].brdv | (brps -1) << 8;

"scr = sh_msiof_spi_div_table[k].brdv | SCR_BRPS(i);" would avoid the need
for "- 1" and the hardcoded shift.

> +       sh_msiof_write(p, TSCR, scr);
>         if (!(p->chipdata->master_flags & SPI_MASTER_MUST_TX))
> -               sh_msiof_write(p, RSCR, sh_msiof_spi_clk_table[k].scr);
> +               sh_msiof_write(p, RSCR, scr);
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: sh-msiof: Update calculation of frequency dividing

[Nobuhiro Iwamatsu]

Hi,

Thanks for your review.

(2015/01/14 17:36), Geert Uytterhoeven wrote:
> Hi Iwamatsu-san,
>
> On Wed, Jan 14, 2015 at 8:25 AM, Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.yj@renesas.com>  wrote:
>> sh-msiof of frequency dividing does not perform the calculation, driver have
>> to manage setting value in the table. It is not possible to set frequency
>> dividing value close to the actual data in this way. This changes from
>> frequency dividing of table management to setting by calculation.
>> This driver is able to set a value close to the actual data.
>
> Thanks for your patch!
>
>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>> index 96a5fc0..58b1bfe 100644
>> --- a/drivers/spi/spi-sh-msiof.c
>> +++ b/drivers/spi/spi-sh-msiof.c
>> @@ -241,42 +241,37 @@ static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
>>
>>   static struct {
>>          unsigned short div;
>> -       unsigned short scr;
>> -} const sh_msiof_spi_clk_table[] = {
>> -       { 1,    SCR_BRPS( 1) | SCR_BRDV_DIV_1 },
>> -       { 2,    SCR_BRPS( 1) | SCR_BRDV_DIV_2 },
>> -       { 4,    SCR_BRPS( 1) | SCR_BRDV_DIV_4 },
>> -       { 8,    SCR_BRPS( 1) | SCR_BRDV_DIV_8 },
>> -       { 16,   SCR_BRPS( 1) | SCR_BRDV_DIV_16 },
>> -       { 32,   SCR_BRPS( 1) | SCR_BRDV_DIV_32 },
>> -       { 64,   SCR_BRPS(32) | SCR_BRDV_DIV_2 },
>> -       { 128,  SCR_BRPS(32) | SCR_BRDV_DIV_4 },
>> -       { 256,  SCR_BRPS(32) | SCR_BRDV_DIV_8 },
>> -       { 512,  SCR_BRPS(32) | SCR_BRDV_DIV_16 },
>> -       { 1024, SCR_BRPS(32) | SCR_BRDV_DIV_32 },
>> +       unsigned short brdv;
>> +} const sh_msiof_spi_div_table[] = {
>> +       { 1,    SCR_BRDV_DIV_1 },
>> +       { 2,    SCR_BRDV_DIV_2 },
>> +       { 4,    SCR_BRDV_DIV_4 },
>> +       { 8,    SCR_BRDV_DIV_8 },
>> +       { 16,   SCR_BRDV_DIV_16 },
>> +       { 32,   SCR_BRDV_DIV_32 },
>>   };
>>
>>   static void sh_msiof_spi_set_clk_regs(struct sh_msiof_spi_priv *p,
>>                                        unsigned long parent_rate, u32 spi_hz)
>>   {
>>          unsigned long div = 1024;
>> +       unsigned long brps, scr;
>
> "u32", as these are (parts of) 32-bit register values.
>

I see. I will fix.

>>          size_t k;
>>
>>          if (!WARN_ON(!spi_hz || !parent_rate))
>>                  div = DIV_ROUND_UP(parent_rate, spi_hz);
>>
>> -       /* TODO: make more fine grained */
>> -
>> -       for (k = 0; k<  ARRAY_SIZE(sh_msiof_spi_clk_table); k++) {
>> -               if (sh_msiof_spi_clk_table[k].div>= div)
>> -                       break;
>> +       for (k = 0; k<  ARRAY_SIZE(sh_msiof_spi_div_table); k++) {
>> +               brps = DIV_ROUND_UP(div, sh_msiof_spi_div_table[k].div);
>> +               if (brps>  32) /* max of brsv is 32 */
>
> max of brdv

Thanks, I will fix.

>
>> +                       continue;
>> +               break;
>
> Having a conditional "continue" followed by a "break" looks strange.
>
>          if (brps<= 32) /* max of brdv is 32 */
>                  break
>

OK, I will fix this.
>>          }
>
> "brps" may be larger than 32 after the loop.
> In the old code, the min_t() below handled that case.
>

Yes, and div is equal to or higher than 1024, this calculation does not work correctly.
This is the specifications of the device. If div is more than 1024, to add the process
of this function returns error.

>> -       k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
>> -
>> -       sh_msiof_write(p, TSCR, sh_msiof_spi_clk_table[k].scr);
>> +       scr = sh_msiof_spi_div_table[k].brdv | (brps -1)<<  8;
>
> "scr = sh_msiof_spi_div_table[k].brdv | SCR_BRPS(i);" would avoid the need
> for "- 1" and the hardcoded shift.
>

I see. I change to using SCR_BRPS(i).

>> +       sh_msiof_write(p, TSCR, scr);
>>          if (!(p->chipdata->master_flags&  SPI_MASTER_MUST_TX))
>> -               sh_msiof_write(p, RSCR, sh_msiof_spi_clk_table[k].scr);
>> +               sh_msiof_write(p, RSCR, scr);
>>   }
>
> Gr{oetje,eeting}s,
>
>                          Geert
>

Best regards,
   Nobuhiro

Re: [PATCH] spi: sh-msiof: Update calculation of frequency dividing

[Sergei Shtylyov]

Hello.

On 1/14/2015 10:25 AM, Nobuhiro Iwamatsu wrote:

> sh-msiof of frequency dividing does not perform the calculation, driver have
> to manage setting value in the table. It is not possible to set frequency
> dividing value close to the actual data in this way. This changes from
> frequency dividing of table management to setting by calculation.
> This driver is able to set a value close to the actual data.

> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> ---
>   drivers/spi/spi-sh-msiof.c | 39 +++++++++++++++++----------------------
>   1 file changed, 17 insertions(+), 22 deletions(-)

> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 96a5fc0..58b1bfe 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -241,42 +241,37 @@ static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
[...]
> -	k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
> -
> -	sh_msiof_write(p, TSCR, sh_msiof_spi_clk_table[k].scr);
> +	scr = sh_msiof_spi_div_table[k].brdv | (brps -1) << 8;

    You forgot a space after '-'.

[...]

WBR, Sergei

Re: [PATCH] spi: sh-msiof: Update calculation of frequency dividing

[Nobuhiro Iwamatsu]

Hi,

Thanks for your review.

(2015/01/14 20:22), Sergei Shtylyov wrote:
> Hello.
>
> On 1/14/2015 10:25 AM, Nobuhiro Iwamatsu wrote:
>
>> sh-msiof of frequency dividing does not perform the calculation, driver have
>> to manage setting value in the table. It is not possible to set frequency
>> dividing value close to the actual data in this way. This changes from
>> frequency dividing of table management to setting by calculation.
>> This driver is able to set a value close to the actual data.
>
>> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
>> ---
>> drivers/spi/spi-sh-msiof.c | 39 +++++++++++++++++----------------------
>> 1 file changed, 17 insertions(+), 22 deletions(-)
>
>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>> index 96a5fc0..58b1bfe 100644
>> --- a/drivers/spi/spi-sh-msiof.c
>> +++ b/drivers/spi/spi-sh-msiof.c
>> @@ -241,42 +241,37 @@ static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
> [...]
>> - k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
>> -
>> - sh_msiof_write(p, TSCR, sh_msiof_spi_clk_table[k].scr);
>> + scr = sh_msiof_spi_div_table[k].brdv | (brps -1) << 8;
>
> You forgot a space after '-'.

Thanks.
I will use SCR_BRPS instead of -1 as comment from Geert.

>
> [...]
>
> WBR, Sergei
>

Best regards,
   Nobuhiro

[PATCH] spi: sh-msiof: Update calculation of frequency dividing

[Nobuhiro Iwamatsu]

sh-msiof of frequency dividing does not perform the calculation, driver have
to manage setting value in the table. It is not possible to set frequency
dividing value close to the actual data in this way. This changes from
frequency dividing of table management to setting by calculation.
This driver is able to set a value close to the actual data.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
---
v3:
 - Remove check of div maximum value.
 - Fix count of overrun by min_t.
v2:
 - use u32 instead of unsigned long.
 - Fix loop.
 - Use SCR_BRPS instead of hoard code shift.
 - Add check of div maximum value.

 drivers/spi/spi-sh-msiof.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 96a5fc0..bcc961f 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -241,42 +241,38 @@ static irqreturn_t sh_msiof_spi_irq(int irq, void *data)
 
 static struct {
 	unsigned short div;
-	unsigned short scr;
-} const sh_msiof_spi_clk_table[] = {
-	{ 1,	SCR_BRPS( 1) | SCR_BRDV_DIV_1 },
-	{ 2,	SCR_BRPS( 1) | SCR_BRDV_DIV_2 },
-	{ 4,	SCR_BRPS( 1) | SCR_BRDV_DIV_4 },
-	{ 8,	SCR_BRPS( 1) | SCR_BRDV_DIV_8 },
-	{ 16,	SCR_BRPS( 1) | SCR_BRDV_DIV_16 },
-	{ 32,	SCR_BRPS( 1) | SCR_BRDV_DIV_32 },
-	{ 64,	SCR_BRPS(32) | SCR_BRDV_DIV_2 },
-	{ 128,	SCR_BRPS(32) | SCR_BRDV_DIV_4 },
-	{ 256,	SCR_BRPS(32) | SCR_BRDV_DIV_8 },
-	{ 512,	SCR_BRPS(32) | SCR_BRDV_DIV_16 },
-	{ 1024,	SCR_BRPS(32) | SCR_BRDV_DIV_32 },
+	unsigned short brdv;
+} const sh_msiof_spi_div_table[] = {
+	{ 1,	SCR_BRDV_DIV_1 },
+	{ 2,	SCR_BRDV_DIV_2 },
+	{ 4,	SCR_BRDV_DIV_4 },
+	{ 8,	SCR_BRDV_DIV_8 },
+	{ 16,	SCR_BRDV_DIV_16 },
+	{ 32,	SCR_BRDV_DIV_32 },
 };
 
 static void sh_msiof_spi_set_clk_regs(struct sh_msiof_spi_priv *p,
 				      unsigned long parent_rate, u32 spi_hz)
 {
 	unsigned long div = 1024;
+	u32 brps, scr;
 	size_t k;
 
 	if (!WARN_ON(!spi_hz || !parent_rate))
 		div = DIV_ROUND_UP(parent_rate, spi_hz);
 
-	/* TODO: make more fine grained */
-
-	for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_clk_table); k++) {
-		if (sh_msiof_spi_clk_table[k].div >= div)
+	for (k = 0; k < ARRAY_SIZE(sh_msiof_spi_div_table); k++) {
+		brps = DIV_ROUND_UP(div, sh_msiof_spi_div_table[k].div);
+		if (brps <= 32) /* max of brdv is 32 */
 			break;
 	}
 
-	k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_clk_table) - 1);
+	k = min_t(int, k, ARRAY_SIZE(sh_msiof_spi_div_table) - 1);
 
-	sh_msiof_write(p, TSCR, sh_msiof_spi_clk_table[k].scr);
+	scr = sh_msiof_spi_div_table[k].brdv | SCR_BRPS(brps);
+	sh_msiof_write(p, TSCR, scr);
 	if (!(p->chipdata->master_flags & SPI_MASTER_MUST_TX))
-		sh_msiof_write(p, RSCR, sh_msiof_spi_clk_table[k].scr);
+		sh_msiof_write(p, RSCR, scr);
 }
 
 static void sh_msiof_spi_set_pin_regs(struct sh_msiof_spi_priv *p,
-- 
2.1.3

Re: [PATCH] spi: sh-msiof: Update calculation of frequency dividing

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

On Fri, Jan 30, 2015 at 03:11:54PM +0900, Nobuhiro Iwamatsu wrote:
> sh-msiof of frequency dividing does not perform the calculation, driver have
> to manage setting value in the table. It is not possible to set frequency
> dividing value close to the actual data in this way. This changes from
> frequency dividing of table management to setting by calculation.
> This driver is able to set a value close to the actual data.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]