Re: [PATCH v2 1/3] net: irda: pxaficp_ir: use sched_clock() for time management

[Robert Jarzmik]

David Miller  writes:

> From: Robert Jarzmik 
> Date: Fri, 18 Sep 2015 18:36:56 +0200
>
>> Which brings me to wonder which is the more correct :
>>  (a) replace to reproduce the same calculation
>>  Previously mtt was compared to a difference of 76ns steps (as 307ns / 4 
>> =
>>  76ns):
>>  while ((sched_clock() - si->last_clk) * 76 < mtt)
>> 
>>  (b) change the calculation assuming mtt is in microseconds :
>>  while ((sched_clock() - si->last_clk) * 1000 < mtt)
>> 
>> I have no IRDA protocol knowledge so unless someone points me to the correct
>> calculation I'll try my luck with (b).
>
> "a" would be "safer" and less likely to break anything, although as
> you say "b" might be more correct.
Indeed.

Well, let me try (b) then. I'll add in the commit message the timings change,
and if anybody complains a regression pops out, I'll provide a patch to fallback
to (a).

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] net: irda: pxaficp_ir: use sched_clock() for time management

[David Miller]

From: Robert Jarzmik 
Date: Fri, 18 Sep 2015 18:36:56 +0200

> Which brings me to wonder which is the more correct :
>  (a) replace to reproduce the same calculation
>  Previously mtt was compared to a difference of 76ns steps (as 307ns / 4 =
>  76ns):
>  while ((sched_clock() - si->last_clk) * 76 < mtt)
> 
>  (b) change the calculation assuming mtt is in microseconds :
>  while ((sched_clock() - si->last_clk) * 1000 < mtt)
> 
> I have no IRDA protocol knowledge so unless someone points me to the correct
> calculation I'll try my luck with (b).

"a" would be "safer" and less likely to break anything, although as
you say "b" might be more correct.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] net: irda: pxaficp_ir: use sched_clock() for time management

[Robert Jarzmik]

David Miller  writes:

>> My understanding is that the flow will be :
>>  sched_clock()
>>rd->read_sched_clock() (cyc_to_ns() transformed for return)
>>  pxa_read_sched_clock()
>>readl_relaxed(OSCR)
>> 
>> I didn't see any timings issue, as the flow looks equivalent to the 
>> readl(OSCR),
>> but I might have overlooked something.
>
> Of course it's different, because sched_clock() converts the value read
> from OSCR into nanoseconds, which is obviously different from using the
> OSCR register value directly.
>
> You're therefore feeding different values into this IRDA code.
Ah yes, I see it.
Which brings me to wonder which is the more correct :
 (a) replace to reproduce the same calculation
 Previously mtt was compared to a difference of 76ns steps (as 307ns / 4 =
 76ns):
 while ((sched_clock() - si->last_clk) * 76 < mtt)

 (b) change the calculation assuming mtt is in microseconds :
 while ((sched_clock() - si->last_clk) * 1000 < mtt)

I have no IRDA protocol knowledge so unless someone points me to the correct
calculation I'll try my luck with (b).

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] net: irda: pxaficp_ir: use sched_clock() for time management

[David Miller]

From: Robert Jarzmik 
Date: Wed, 16 Sep 2015 11:34:01 +0200

> David Miller  writes:
> 
>> From: Robert Jarzmik 
>> Date: Sat, 12 Sep 2015 13:45:22 +0200
>>
>>> Instead of using directly the OS timer through direct register access,
>>> use the standard sched_clock(), which will end up in OSCR reading
>>> anyway.
>>> 
>>> This is a first step for direct access register removal and machine
>>> specific code removal from this driver.
>>> 
>>> Signed-off-by: Robert Jarzmik 
>>
>> What is the granularity of the OSCR register?
> It's 307ns (ie. 3.25MHz clock).
> 
>> If it is not nanoseconds, then you need to adjust calculations
>> such as this one:
> Tell me if the 307ns requires something I should adjust.
> 
> My understanding is that the flow will be :
>  sched_clock()
>rd->read_sched_clock() (cyc_to_ns() transformed for return)
>  pxa_read_sched_clock()
>readl_relaxed(OSCR)
> 
> I didn't see any timings issue, as the flow looks equivalent to the 
> readl(OSCR),
> but I might have overlooked something.

Of course it's different, because sched_clock() converts the value read
from OSCR into nanoseconds, which is obviously different from using the
OSCR register value directly.

You're therefore feeding different values into this IRDA code.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] net: irda: pxaficp_ir: use sched_clock() for time management

[Robert Jarzmik]

David Miller  writes:

> From: Robert Jarzmik 
> Date: Sat, 12 Sep 2015 13:45:22 +0200
>
>> Instead of using directly the OS timer through direct register access,
>> use the standard sched_clock(), which will end up in OSCR reading
>> anyway.
>> 
>> This is a first step for direct access register removal and machine
>> specific code removal from this driver.
>> 
>> Signed-off-by: Robert Jarzmik 
>
> What is the granularity of the OSCR register?
It's 307ns (ie. 3.25MHz clock).

> If it is not nanoseconds, then you need to adjust calculations
> such as this one:
Tell me if the 307ns requires something I should adjust.

My understanding is that the flow will be :
 sched_clock()
   rd->read_sched_clock() (cyc_to_ns() transformed for return)
 pxa_read_sched_clock()
   readl_relaxed(OSCR)

I didn't see any timings issue, as the flow looks equivalent to the readl(OSCR),
but I might have overlooked something.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] net: irda: pxaficp_ir: use sched_clock() for time management

[David Miller]

From: Robert Jarzmik 
Date: Sat, 12 Sep 2015 13:45:22 +0200

> Instead of using directly the OS timer through direct register access,
> use the standard sched_clock(), which will end up in OSCR reading
> anyway.
> 
> This is a first step for direct access register removal and machine
> specific code removal from this driver.
> 
> Signed-off-by: Robert Jarzmik 

What is the granularity of the OSCR register?

If it is not nanoseconds, then you need to adjust calculations
such as this one:

> @@ -549,7 +548,7 @@ static int pxa_irda_hard_xmit(struct sk_buff *skb, struct 
> net_device *dev)
>   skb_copy_from_linear_data(skb, si->dma_tx_buff, skb->len);
>  
>   if (mtt)
> - while ((unsigned)(readl_relaxed(OSCR) - 
> si->last_oscr)/4 < mtt)
> + while ((sched_clock() - si->last_clk) / 4 < mtt)
>   cpu_relax();
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] net: irda: pxaficp_ir: use sched_clock() for time management

[Robert Jarzmik]

Instead of using directly the OS timer through direct register access,
use the standard sched_clock(), which will end up in OSCR reading
anyway.

This is a first step for direct access register removal and machine
specific code removal from this driver.

Signed-off-by: Robert Jarzmik 
---
 drivers/net/irda/pxaficp_ir.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c
index 100454662e4b..b1794998c68e 100644
--- a/drivers/net/irda/pxaficp_ir.c
+++ b/drivers/net/irda/pxaficp_ir.c
@@ -29,7 +29,6 @@
 
 #include 
 #include 
-#include 
 #include 
 
 #define FICP   __REG(0x4080)  /* Start of FICP area */
@@ -102,7 +101,7 @@
 struct pxa_irda {
int speed;
int newspeed;
-   unsigned long   last_oscr;
+   unsigned long long  last_clk;
 
unsigned char   *dma_rx_buff;
unsigned char   *dma_tx_buff;
@@ -292,7 +291,7 @@ static irqreturn_t pxa_irda_sir_irq(int irq, void *dev_id)
}
lsr = STLSR;
}
-   si->last_oscr = readl_relaxed(OSCR);
+   si->last_clk = sched_clock();
break;
 
case 0x04: /* Received Data Available */
@@ -303,7 +302,7 @@ static irqreturn_t pxa_irda_sir_irq(int irq, void *dev_id)
dev->stats.rx_bytes++;
async_unwrap_char(dev, >stats, >rx_buff, STRBR);
} while (STLSR & LSR_DR);
-   si->last_oscr = readl_relaxed(OSCR);
+   si->last_clk = sched_clock();
break;
 
case 0x02: /* Transmit FIFO Data Request */
@@ -319,7 +318,7 @@ static irqreturn_t pxa_irda_sir_irq(int irq, void *dev_id)
 /* We need to ensure that the transmitter has 
finished. */
while ((STLSR & LSR_TEMT) == 0)
cpu_relax();
-   si->last_oscr = readl_relaxed(OSCR);
+   si->last_clk = sched_clock();
 
/*
* Ok, we've finished transmitting.  Now enable
@@ -373,7 +372,7 @@ static void pxa_irda_fir_dma_tx_irq(int channel, void *data)
 
while (ICSR1 & ICSR1_TBY)
cpu_relax();
-   si->last_oscr = readl_relaxed(OSCR);
+   si->last_clk = sched_clock();
 
/*
 * HACK: It looks like the TBY bit is dropped too soon.
@@ -473,8 +472,8 @@ static irqreturn_t pxa_irda_fir_irq(int irq, void *dev_id)
 
/* stop RX DMA */
DCSR(si->rxdma) &= ~DCSR_RUN;
-   si->last_oscr = readl_relaxed(OSCR);
icsr0 = ICSR0;
+   si->last_clk = sched_clock();
 
if (icsr0 & (ICSR0_FRE | ICSR0_RAB)) {
if (icsr0 & ICSR0_FRE) {
@@ -549,7 +548,7 @@ static int pxa_irda_hard_xmit(struct sk_buff *skb, struct 
net_device *dev)
skb_copy_from_linear_data(skb, si->dma_tx_buff, skb->len);
 
if (mtt)
-   while ((unsigned)(readl_relaxed(OSCR) - 
si->last_oscr)/4 < mtt)
+   while ((sched_clock() - si->last_clk) / 4 < mtt)
cpu_relax();
 
/* stop RX DMA,  disable FICP */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html