Re: [PATCH/RFC v4 net-next] ravb: Add dma queue interrupt support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

Sorry for the late reply -- I was hoping to get AVB working on another gen2 board...

On 02/08/2016 08:19 PM, Yoshihiro Kaneko wrote:

[...]
From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@xxxxxxxxxxx>

This patch supports the following interrupts.

- One interrupt for multiple (descriptor, error, management)
- One interrupt for emac
- Four interrupts for dma queue (best effort rx/tx, network control
rx/tx)

This patch improve efficiency of the interrupt handler by adding the
interrupt handler corresponding to each interrupt source described
above. Additionally, it reduces the number of times of the access to
EthernetAVB IF.

Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@xxxxxxxxxxx>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>

[...]

diff --git a/drivers/net/ethernet/renesas/ravb_main.c
b/drivers/net/ethernet/renesas/ravb_main.c
index ac43ed9..076f25f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
+
+       spin_lock(&priv->lock);
+
+       ris0 = ravb_read(ndev, RIS0);
+       ric0 = ravb_read(ndev, RIC0);
+       tis  = ravb_read(ndev, TIS);
+       tic  = ravb_read(ndev, TIC);
+
+       /* Timestamp updated */
+       if (tis & TIS_TFUF) {
+               ravb_write(ndev, TID_TFUD, TID);

     Wait, you're supposed to clear the TFUF interrupt, not to disable!

Thanks for finding this bug.

And with that fixed, this interrupt's handler could get factored out into
a
function...

Is this not too small to make a function?

    I wouldn't say so. But need to count the summary LoCs, of course...
perhaps indeed not worth it...

I don't feel need for making it a function.
It only clears the interrupt and calls ravb_get_tx_tstamp().

   I meant that the enclosing *if* statement should be a part of function too...

[...]

+                          struct net_device *ndev, struct device *dev,
+                          const char *ch)
+{
+       char *name;
+       int error;
+
+       name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
+       error = request_irq(irq, handler, IRQF_SHARED, name, ndev);

     Not sure if we need IRQF_SHARED on those IRQs, they're not really
shareable...

I don't know whether this causes something bad.
I think this controller is supporting a shared IRQ.

    Based on the high-level trigger, I'd rather suspect not. Anyway, all the
SoC IRQs are dedicated to a certain (single) source.

I don't want to change that if there is no fatal issue in the use of
IRQF_SHARED.
However, I will remove the flag if it is simple waste...

   It's not a waste but it just shouldn't be needed.

[...]

     OK, I'll now proceed to sanity-testing this patch on the gen2
hardware.

    I'm afraid this will have to wait until I have a gen2 board with fully
working AVB... :-(

Does it take long time?

   Unfortunately. :-(
I wasn't able to get the AVB driver to work on the modified Porter board and now I've ascertained that it doesn't work on the re-jumpred Alt board too, seemingly for the same reason -- the AVB_MDIO pin gets stuck at 1 during the MDIO bus scan, so the device opening fails due to the driver not able to connect to PHY. LTSI 3.10 based kernels do work, however...

Thanks,
kaneko

MBR, Sergei




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux