Hi Sergei,
2016-01-24 7:17 GMT+09:00 Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>:
> Hello.
>
> On 01/17/2016 01:55 PM, Yoshihiro Kaneko wrote:
>
>> 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.
>
>
> Not sure I got the last sentence right... Did you mean that we save on
> the register reads?
Yes, I meant that.
> Yet another reason could be that we don't want to depend on the boot
> loader's whim any more...
>
>> 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.
>>
>> v3 [Yoshihiro Kaneko]
>> * compile tested only
>
>
> Doesn't sound very encouraging... couldn't you manage to re-test the
> patch before Dave merges it?
I will talk to Simon Horman.
>
> [...]
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>> b/drivers/net/ethernet/renesas/ravb.h
>> index 9fbe92a..385f06b 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>
> [...]
>>
>> @@ -556,6 +566,18 @@ enum ISS_BIT {
>> ISS_DPS15 = 0x80000000,
>> };
>>
>> +/* CIE
>> + * R-Car Gen3 only
>> + */
>
>
> I'd be more happy with one line comment like:
>
> /* CIE (R-Car Gen3 only) */
Agreed.
>
> [...]
>>
>> @@ -592,6 +614,212 @@ enum GIS_BIT {
>> GIS_PTMF = 0x00000004,
>> };
>>
>> +/* GIE
>> + * R-Car Gen3 only
>> + */
>> +enum GIE_BIT {
>
> [...]
>>
>> + GIE_ALL = 0xffff03ff,
>
>
> GIE_ALL isn't used, no need to define it.
Agreed.
>
>> +};
>> +
>> +/* GID
>> + * R-Car Gen3 only
>> + */
>> +enum GID_BIT {
>
> [...]
>>
>> + GID_ALL = 0xffff03ff,
>
>
> Not used any more.
>
>> +};
>> +
>> +/* RIE0
>> + * R-Car Gen3 only
>> + */
>> +enum RIE0_BIT {
>
> [...]
>>
>> + RIE0_ALL = 0x0003ffff,
>
>
> Likewise.
>
>> +};
>> +
>> +/* RID0
>> + * R-Car Gen3 only
>> + */
>> +enum RID0_BIT {
>
> [...]
>>
>> + RID0_ALL = 0x0003ffff,
>
>
> Likewise.
>
>> +};
>> +
>> +/* RIE2
>> + * R-Car Gen3 only
>> + */
>> +enum RIE2_BIT {
>
> [...]
>>
>> + RIE2_ALL = 0x8003ffff,
>
>
> Likewise.
>
>> +};
>> +
>> +/* RID2
>> + * R-Car Gen3 only
>> + */
>> +enum RID2_BIT {
>
>
>> + RID2_ALL = 0x8003ffff,
>
>
> Likewise.
>
>> +};
>> +
>> +/* TIE
>> + * R-Car Gen3 only
>> + */
>> +enum TIE_BIT {
>
> [...]
>>
>> + TIE_ALL = 0x000f0f0f,
>
>
> Likewise.
>
>> +};
>> +
>> +/* TID
>> + * R-Car Gen3 only
>> + */
>> +enum TID_BIT {
>
> [...]
>>
>> + TID_ALL = 0x000f0f0f,
>
>
> Likewise.
>
> [...]
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index ac43ed9..4c4912e0 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -42,6 +42,16 @@
>> NETIF_MSG_RX_ERR | \
>> NETIF_MSG_TX_ERR)
>>
>> +static const char *ravb_rx_irqs[NUM_RX_QUEUE] = {
>> + "ch0", /* RAVB_BE */
>> + "ch1", /* RAVB_NC */
>> +};
>> +
>> +static const char *ravb_tx_irqs[NUM_TX_QUEUE] = {
>> + "ch18", /* RAVB_BE */
>> + "ch19", /* RAVB_NC */
>> +};
>> +
>
>
> Do what you wish but I don't like these...
I think there is no problem.
>
> [...]
>>
>> @@ -693,7 +722,7 @@ static void ravb_error_interrupt(struct net_device
>> *ndev)
>> if (ris2 & RIS2_QFF0)
>> priv->stats[RAVB_BE].rx_over_errors++;
>>
>> - /* Receive Descriptor Empty int */
>> + /* Receive Descriptor Empty int */
>
>
> It's not even a "drove-by" change, please don't do this here, submit a
> separate patch (or I can do it).
Got it.
I remove this change from the patch.
>
> [...]
>
>> @@ -773,6 +829,55 @@ static irqreturn_t ravb_interrupt(int irq, void
>> *dev_id)
>> return result;
>> }
>>
>> +static irqreturn_t ravb_dmaq_interrupt(int irq, void *dev_id, int
>> ravb_queue)
>> +{
>> + struct net_device *ndev = dev_id;
>> + struct ravb_private *priv = netdev_priv(ndev);
>> + irqreturn_t result = IRQ_NONE;
>> + u32 ris0, ric0, tis, tic;
>> + int q = ravb_queue;
>> +
>> + 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);
>> + ravb_get_tx_tstamp(ndev);
>> + result = IRQ_HANDLED;
>> + }
>> +
>> + /* Best effort queue RX/TX */
>> + if (((ris0 & ric0) & BIT(q)) ||
>> + ((tis & tic) & BIT(q))) {
>> + if (napi_schedule_prep(&priv->napi[q])) {
>> + /* Mask RX and TX interrupts */
>> + ravb_write(ndev, BIT(q), RID0);
>> + ravb_write(ndev, BIT(q), TID);
>> + __napi_schedule(&priv->napi[q]);
>> + }
>
>
> There was an *else* branch originally (napi_schedule_prep()'s failure is
> a serious issue deserving an explanation), not sure why you dropped it
> here...
Agreed.
>
> [...]
>>
>> @@ -1215,29 +1325,63 @@ static const struct ethtool_ops ravb_ethtool_ops =
>> {
>> .get_ts_info = ravb_get_ts_info,
>> };
>>
>> +static inline int req_irq(unsigned int irq, irq_handler_t handler,
>
>
> I'd suggest hook_irq() instead.
>
>> + 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);
>
>
> Looks like you missed a line here. Compile testing is not enough,
> apparently... :-(
Uh, this is terrible.
Anyway, I will fix it.
>
>> + netdev_err(ndev, "cannot request IRQ %s\n", name);
>> +
>> + return error;
>> +}
>> +
>
> [...]
>>
>> @@ -1268,6 +1412,10 @@ out_free_irq2:
>> free_irq(priv->emac_irq, ndev);
>> out_free_irq:
>> free_irq(ndev->irq, ndev);
>> + for (i = 0; i < NUM_RX_QUEUE; i++)
>> + free_irq(priv->rx_irqs[i], ndev);
>> + for (i = 0; i < NUM_TX_QUEUE; i++)
>> + free_irq(priv->tx_irqs[i], ndev);
>
>
> If these are left uninitialized (as done for gen2), __free_irq() will
> curse loudly. Please only do this for gen3 like was done in my fix just
> above this code.
Got it.
>
> [...]
>
> MBR, Sergei
>
Thanks,
kaneko
