Hello.
On 02/07/2016 07:50 PM, Yoshihiro Kaneko wrote:
I apologize for not responding to you earlier.
Absolutely no problem, these reviews/tests take time from my main tasks
anyway. :-)
From: Kazuya Mizuguchi <kazuya.mizuguchi...@renesas.com>
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...@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0...@gmail.com>
---
This patch is based on the master branch of David Miller's next networking
tree.
v4 [Yoshihiro Kaneko]
* compile tested only
* As suggested by Sergei Shtylyov
drivers/net/ethernet/renesas/ravb.h:
- make two lines of comment into one line.
- remove unused definition of xxx_ALL.
drivers/net/ethernet/renesas/ravb_main.c:
- remove unrelated change (fix indentation).
- output warning messages when napi_schedule_prep() fails in
ravb_dmaq_
interrupt() like ravb_interrupt().
- change the function name from req_irq to hook_irq.
- fix programming error in hook_irq().
- do free_irq() for rx_irqs[] and tx_irqs[] for only gen3 in
out_free_
irq label in ravb_open().
v3 [Yoshihiro Kaneko]
* compile tested only
* As suggested by Sergei Shtylyov
- update changelog
drivers/net/ethernet/renesas/ravb.h:
- add comments to the additional registers like CIE
drivers/net/ethernet/renesas/ravb_main.c:
- fix the initialization of the interrupt in ravb_dmac_init()
- revert ravb_error_interrupt() because gen3 code is wrong
- change the comment "Management" in ravb_multi_interrupt()
- add a helper function for request_irq() in ravb_open()
- revert ravb_close() because atomicity is not necessary here
drivers/net/ethernet/renesas/ravb_ptp.c:
- revert ravb_ptp_stop() because atomicity is not necessary here
v2 [Yoshihiro Kaneko]
* compile tested only
* As suggested by Sergei Shtylyov
- add comment to CIE
- remove comments from CIE bits
- fix value of TIx_ALL
- define each bits for CIE, GIE, GID, RIE0, RID0, RIE2, RID2, TIE, TID
- reversed Christmas tree declaration ordered
- rename _ravb_emac_interrupt() to ravb_emac_interrupt_unlocked()
- remove unnecessary clearing of CIE
- use a bit name corresponding to the target register, RIE0, RIE2, TIE,
TID, RID2, GID, GIE
As I already noted, the changes made to the original patch are supposed
to be documented above --- (no need to separate diff versions there though).
Either that, or just say that it's your patch, based on Mizuguchi-san's
work (the amount of changes makes that possible, I think).
I will record that I made a change to this patch in the commit log of
the next version.
I don't think that I changed the essence of this patch. I changed
various trivial things, or fixed bugs you pointed out.
OK, as you wish. But in case this gets too tedious, I'll understand if you
change the authorship.
[...]
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...
[...]
@@ -1215,29 +1332,64 @@ static const struct ethtool_ops ravb_ethtool_ops =
{
.get_ts_info = ravb_get_ts_info,
};
+static inline int hook_irq(unsigned int irq, irq_handler_t handler,
Namespacing this function with 'ravb_' prefix would be preferable, I did
that for all functions, even those that didn't have this prefix in sh_eth...
Didn't have 'sh_eth_' prefix, I meant.
Got it.
OK.
+ 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.
[...]
[...]
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... :-(
Thanks,
kaneko
MBR, Sergei
