Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1266826 > unrolled thread

Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-10 23:40 +0100
    Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-10 23:50 +0100
      Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 00:10 +0100
        Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800  Ethernet controller Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-11 01:40 +0100
          Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Måns Rullgård <mans@mansr.com> - 2015-11-11 01:50 +0100

#1266826 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>> +                               u32 mask, u32 val)
>> +{
>> +       u32 old = nb8800_readb(priv, reg);
>> +       u32 new = (old & ~mask) | val;
>
> Shoudn't be "… | (val & mask);" ?

No, it's meant to replace the bits in mask with the corresponding bits
from val.

> + empty line?
>
>> +       if (new != old)
>> +               nb8800_writeb(priv, reg, new);
>> +}
>> +

[...]

>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>
> unsigned int i ?
> len as well?

Does it matter?  The values are nowhere near overflowing signed int.


[...]

>> +               /* If a packet arrived after we last checked but
>> +                * before writing RX_ITR, the interrupt will be
>> +                * delayed, so we retrieve it now. */
>
> Block comments usually
> /*
>  * text
>  */

Documentation/CodingStyle says net/ and drivers/net/ are special, though
currently a mix of styles can be found.  Personally, I don't
particularly care.

> Can be longer lines?

Still won't fit on two lines.

>> +               if (priv->rx_descs[next].report)
>> +                       goto again;
>> +
>> +               napi_complete_done(napi, work);
>> +       }
>> +
>> +       return work;
>> +}
>> +
>> +static void nb8800_tx_dma_start(struct net_device *dev)
>> +{
>> +       struct nb8800_priv *priv = netdev_priv(dev);
>> +       struct nb8800_tx_buf *txb;
>> +       u32 txc_cr;
>> +
>> +       txb = &priv->tx_bufs[priv->tx_queue];
>> +       if (!txb->ready)
>> +               return;
>> +
>> +       txc_cr = nb8800_readl(priv, NB8800_TXC_CR);
>> +       if (txc_cr & TCR_EN)
>> +               return;
>> +
>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>> +       wmb();          /* ensure desc addr is written before starting DMA */
>
> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?

Possibly.

>> +       nb8800_writel(priv, NB8800_TXC_CR, txc_cr | TCR_EN);
>> +
>> +       priv->tx_queue = (priv->tx_queue + txb->chain_len) % TX_DESC_COUNT;
>> +}

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1266832 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård <mans@mansr.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
>>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>>> +                               u32 mask, u32 val)
>>> +{
>>> +       u32 old = nb8800_readb(priv, reg);
>>> +       u32 new = (old & ~mask) | val;
>>
>> Shoudn't be "… | (val & mask);" ?
>
> No, it's meant to replace the bits in mask with the corresponding bits
> from val.

But you unconditionally use entire val value which might bring bits
outside of mask.

>> Block comments usually
>> /*
>>  * text
>>  */
>
> Documentation/CodingStyle says net/ and drivers/net/ are special, though
> currently a mix of styles can be found.  Personally, I don't
> particularly care.

OK.

>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>
>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>
> Possibly.

Standalone wmb() doesn't make sense.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266841

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård <mans@mansr.com> wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>>
>>>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>>>> +                               u32 mask, u32 val)
>>>> +{
>>>> +       u32 old = nb8800_readb(priv, reg);
>>>> +       u32 new = (old & ~mask) | val;
>>>
>>> Shoudn't be "… | (val & mask);" ?
>>
>> No, it's meant to replace the bits in mask with the corresponding bits
>> from val.
>
> But you unconditionally use entire val value which might bring bits
> outside of mask.

Very well, I'll apply the mask to both then.

>>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>>
>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>>
>> Possibly.
>
> Standalone wmb() doesn't make sense.

It does if you need to enforce ordering between normal and I/O memory.
In fact, since the descriptor is filled in using normal memory accesses,
my understanding is that mmiowb() would be insufficient here.  The
comment could be improved, however.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266894 — Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

On Wed, Nov 11, 2015 at 1:07 AM, Måns Rullgård <mans@mansr.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:

>>>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>>>
>>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>>>
>>> Possibly.
>>
>> Standalone wmb() doesn't make sense.
>
> It does if you need to enforce ordering between normal and I/O memory.
> In fact, since the descriptor is filled in using normal memory accesses,
> my understanding is that mmiowb() would be insufficient here.  The
> comment could be improved, however.

Can you then explain what exactly you are assured against in all cases
where you are using wmb()s? It seems I don't recognize this part in
some excerpts.


-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266907

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Wed, Nov 11, 2015 at 1:07 AM, Måns Rullgård <mans@mansr.com> wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
>>>>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>>>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>>>>
>>>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>>>>
>>>> Possibly.
>>>
>>> Standalone wmb() doesn't make sense.
>>
>> It does if you need to enforce ordering between normal and I/O memory.
>> In fact, since the descriptor is filled in using normal memory accesses,
>> my understanding is that mmiowb() would be insufficient here.  The
>> comment could be improved, however.
>
> Can you then explain what exactly you are assured against in all cases
> where you are using wmb()s? It seems I don't recognize this part in
> some excerpts.

Certainly.  I'll re-read memory-barriers.txt to make sure I'm doing the
right thing, then write a better comment.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web