| Message ID | 1442058324-9760-1-git-send-email-robert.jarzmik@free.fr |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Robert Jarzmik <robert.jarzmik@free.fr> 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 <robert.jarzmik@free.fr> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller <davem@davemloft.net> writes: > From: Robert Jarzmik <robert.jarzmik@free.fr> > 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 <robert.jarzmik@free.fr> > > 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.
From: Robert Jarzmik <robert.jarzmik@free.fr> Date: Wed, 16 Sep 2015 11:34:01 +0200 > David Miller <davem@davemloft.net> writes: > >> From: Robert Jarzmik <robert.jarzmik@free.fr> >> 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 <robert.jarzmik@free.fr> >> >> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller <davem@davemloft.net> 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.
From: Robert Jarzmik <robert.jarzmik@free.fr> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller <davem@davemloft.net> writes: > From: Robert Jarzmik <robert.jarzmik@free.fr> > 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.
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 <mach/dma.h> #include <linux/platform_data/irda-pxaficp.h> -#include <mach/regs-ost.h> #include <mach/regs-uart.h> #define FICP __REG(0x40800000) /* 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, &dev->stats, &si->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 */
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 <robert.jarzmik@free.fr> --- drivers/net/irda/pxaficp_ir.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)