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

Groups > linux.kernel > #1447066 > unrolled thread

[PATCH v2 0/5] net-next: ethernet: add sun8i-emac driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/5] net-next: ethernet: add sun8i-emac driver LABBE Corentin <clabbe.montjoie@gmail.com> - 2016-07-20 10:10 +0200
    [PATCH v2 2/5] MAINTAINERS: Add myself as maintainers of sun8i-emac LABBE Corentin <clabbe.montjoie@gmail.com> - 2016-07-20 10:10 +0200
    Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Arnd Bergmann <arnd@arndb.de> - 2016-07-20 12:00 +0200
      Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver LABBE Corentin <clabbe.montjoie@gmail.com> - 2016-07-28 15:20 +0200
        Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Arnd Bergmann <arnd@arndb.de> - 2016-07-29 11:30 +0200
    Re: [PATCH v2 0/5] net-next: ethernet: add sun8i-emac driver paulo@inutilfutil.com - 2016-07-25 18:20 +0200
    Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Andre Przywara <andre.przywara@arm.com> - 2016-07-29 12:20 +0200
    Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-29 19:30 +0200
      Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Chen-Yu Tsai <wens@csie.org> - 2016-07-30 03:40 +0200
        Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-30 09:40 +0200

#1447066 — [PATCH v2 0/5] net-next: ethernet: add sun8i-emac driver

Hello

This patch series add the driver for sun8i-emac which handle the Ethernet MAC
present on Allwinner H3/A83T/A64 SoCs.

It supports 10/100/1000 Mbit/s speed with half/full duplex.
It can use an internal PHY (MII 10/100) or an external PHY
via RGMII/RMII.

This patch series enable the driver only for the H3 SoC since A83T and A64
doesn't have the necessary clocks present in mainline.

This patch series enable the driver only for the OrangePiPC board since other
board with H3 use external PHY which need optional regulators that will be
supported later.

The driver have been tested on the following boards:
- H3 Orange PI PC, Orange PI Plus, BananaPI-M2+
- A64 Pine64
- A83T BananaPI-M3

I would like to thanks Chen-Yu Tsai for his help on developing this driver.

Regards

Changes since v1
- Implement NAPI
- Sorted and reworded all define
- Reworked ethtools stats strings
- Removed all unneeded __packked and __aligned
- Added tuning of RX/TX ring size via ethtool
- Corrected use of sk/skb naming
- Added some wmb when needed
- Moved irq claim/free to emac_open/close
- Lots of code refactoring

LABBE Corentin (5):
  ethernet: add sun8i-emac driver
  MAINTAINERS: Add myself as maintainers of sun8i-emac
  ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
  ARM: dts: sun8i-h3: add sun8i-emac ethernet driver
  ARM: dts: sun8i: Enable sun8i-emac on the Orange PI PC

 .../bindings/net/allwinner,sun8i-emac.txt          |   65 +
 MAINTAINERS                                        |    6 +
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts         |   11 +
 arch/arm/boot/dts/sun8i-h3.dtsi                    |   14 +
 drivers/net/ethernet/allwinner/Kconfig             |   13 +
 drivers/net/ethernet/allwinner/Makefile            |    1 +
 drivers/net/ethernet/allwinner/sun8i-emac.c        | 2127 ++++++++++++++++++++
 7 files changed, 2237 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
 create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c

-- 
2.7.3

[toc] | [next] | [standalone]

#1447069 — [PATCH v2 2/5] MAINTAINERS: Add myself as maintainers of sun8i-emac

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1d74837..daefb19 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -581,6 +581,12 @@ S:	Maintained
 F:	Documentation/i2c/busses/i2c-ali1563
 F:	drivers/i2c/busses/i2c-ali1563.c
 
+ALLWINNER SUN8I-EMAC ETHERNET DRIVER
+M:	Corentin Labbe <clabbe.montjoie@gmail.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/ethernet/allwinner/sun8i-emac.c
+
 ALLWINNER SECURITY SYSTEM
 M:	Corentin Labbe <clabbe.montjoie@gmail.com>
 L:	linux-crypto@vger.kernel.org
-- 
2.7.3

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

#1447138 — Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

On Wednesday, July 20, 2016 10:03:16 AM CEST LABBE Corentin wrote:
> +
> +       /* Benched on OPIPC with 100M, setting more than 256 does not give any
> +        * perf boost
> +        */
> +       priv->nbdesc_rx = 128;
> +       priv->nbdesc_tx = 256;
> +
> 

256 tx descriptors can introduce a significant latency. Can you add
support for BQL (netdev_sent_queue/netdev_completed_queue) to limit
the queue size to the minimum?

I also noticed that your tx_lock() prevents you from concurrently
running sun8i_emac_complete_xmit() and sun8i_emac_xmit(). Is that
necessary? I'd think that you can find a way to make them work
concurrently.

	Arnd

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

#1451924 — Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

On Wed, Jul 20, 2016 at 11:56:12AM +0200, Arnd Bergmann wrote:
> On Wednesday, July 20, 2016 10:03:16 AM CEST LABBE Corentin wrote:
> > +
> > +       /* Benched on OPIPC with 100M, setting more than 256 does not give any
> > +        * perf boost
> > +        */
> > +       priv->nbdesc_rx = 128;
> > +       priv->nbdesc_tx = 256;
> > +
> > 
> 
> 256 tx descriptors can introduce a significant latency. Can you add
> support for BQL (netdev_sent_queue/netdev_completed_queue) to limit
> the queue size to the minimum?

Done, since setting below 256 give lower performance with iperf.

> 
> I also noticed that your tx_lock() prevents you from concurrently
> running sun8i_emac_complete_xmit() and sun8i_emac_xmit(). Is that
> necessary? I'd think that you can find a way to make them work
> concurrently.
> 
> 	Arnd

I will reworked locking and it seems that no locking is necessary.
I have added the following comment about the locking strategy:

/* Locking strategy:
 * RX queue does not need any lock since only sun8i_emac_poll() access it.
 * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and so sun8i_emac_poll())
 * TX queue is handled by sun8i_emac_xmit(), sun8i_emac_complete_xmit() and sun8i_emac_tx_timeout()
 * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and stop queue)
 *
 * sun8i_emac_xmit() could fire only once (netif_tx_lock)
 * sun8i_emac_complete_xmit() could fire only once (called from NAPI)
 * sun8i_emac_tx_timeout() could fire only once (netif_tx_lock) and couldnt
 * race with sun8i_emac_xmit (due to netif_tx_lock) and with sun8i_emac_complete_xmit which disable NAPI.
 *
 * So only sun8i_emac_xmit and sun8i_emac_complete_xmit could fire at the same time.
 * But they never could modify the same descriptors:
 * - sun8i_emac_complete_xmit() will modify only descriptors with empty status
 * - sun8i_emac_xmit() will modify only descriptors set to DCLEAN
 * Proper memory barriers ensure that descriptor set to DCLEAN could not be
 * modified latter by sun8i_emac_complete_xmit().
 * */

Does I am right ?

Thanks for your review.

Regards

LABBE Corentin

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

#1452349 — Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

On Thursday, July 28, 2016 3:18:26 PM CEST LABBE Corentin wrote:
> 
> I will reworked locking and it seems that no locking is necessary.
> I have added the following comment about the locking strategy:
> 
> /* Locking strategy:
>  * RX queue does not need any lock since only sun8i_emac_poll() access it.
>  * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and so sun8i_emac_poll())
>  * TX queue is handled by sun8i_emac_xmit(), sun8i_emac_complete_xmit() and sun8i_emac_tx_timeout()
>  * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and stop queue)
>  *
>  * sun8i_emac_xmit() could fire only once (netif_tx_lock)
>  * sun8i_emac_complete_xmit() could fire only once (called from NAPI)
>  * sun8i_emac_tx_timeout() could fire only once (netif_tx_lock) and couldnt
>  * race with sun8i_emac_xmit (due to netif_tx_lock) and with sun8i_emac_complete_xmit which disable NAPI.
>  *
>  * So only sun8i_emac_xmit and sun8i_emac_complete_xmit could fire at the same time.
>  * But they never could modify the same descriptors:
>  * - sun8i_emac_complete_xmit() will modify only descriptors with empty status
>  * - sun8i_emac_xmit() will modify only descriptors set to DCLEAN
>  * Proper memory barriers ensure that descriptor set to DCLEAN could not be
>  * modified latter by sun8i_emac_complete_xmit().
>  * */

Sounds good, the comment is certainly very helpful here.

	Arnd

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

#1449634

[Multipart message — attachments visible in raw view] — view raw

On Wednesday, July 20, 2016 at 5:03:41 AM UTC-3, clabbe.montjoie wrote:
> Hello
> 
> This patch series add the driver for sun8i-emac which handle the Ethernet MAC
> present on Allwinner H3/A83T/A64 SoCs.
> 
> It supports 10/100/1000 Mbit/s speed with half/full duplex.
> It can use an internal PHY (MII 10/100) or an external PHY
> via RGMII/RMII.
> 
> This patch series enable the driver only for the H3 SoC since A83T and A64
> doesn't have the necessary clocks present in mainline.
> 
> This patch series enable the driver only for the OrangePiPC board since other
> board with H3 use external PHY which need optional regulators that will be
> supported later.
> 
> The driver have been tested on the following boards:
> - H3 Orange PI PC, Orange PI Plus, BananaPI-M2+
> - A64 Pine64
> - A83T BananaPI-M3
> 
> I would like to thanks Chen-Yu Tsai for his help on developing this driver.
> 
> Regards
> 
> Changes since v1
> - Implement NAPI
> - Sorted and reworded all define
> - Reworked ethtools stats strings
> - Removed all unneeded __packked and __aligned
> - Added tuning of RX/TX ring size via ethtool
> - Corrected use of sk/skb naming
> - Added some wmb when needed
> - Moved irq claim/free to emac_open/close
> - Lots of code refactoring
> 
> LABBE Corentin (5):
>   ethernet: add sun8i-emac driver
>   MAINTAINERS: Add myself as maintainers of sun8i-emac
>   ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac
>   ARM: dts: sun8i-h3: add sun8i-emac ethernet driver
>   ARM: dts: sun8i: Enable sun8i-emac on the Orange PI PC
> 
>  .../bindings/net/allwinner,sun8i-emac.txt          |   65 +
>  MAINTAINERS                                        |    6 +
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts         |   11 +
>  arch/arm/boot/dts/sun8i-h3.dtsi                    |   14 +
>  drivers/net/ethernet/allwinner/Kconfig             |   13 +
>  drivers/net/ethernet/allwinner/Makefile            |    1 +
>  drivers/net/ethernet/allwinner/sun8i-emac.c        | 2127 ++++++++++++++++++++
>  7 files changed, 2237 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
>  create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c
> 
> -- 
> 2.7.3

Works on my Orange Pi PC. Thank you!

IMHO, we should enable this module on sunxi_defconfig.

Paulo

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

#1452370 — Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

Hi,

On 25/07/16 20:54, Maxime Ripard wrote:
> On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote:
>> This patch add support for sun8i-emac ethernet MAC hardware.
>> It could be found in Allwinner H3/A83T/A64 SoCs.
>>
>> It supports 10/100/1000 Mbit/s speed with half/full duplex.
>> It can use an internal PHY (MII 10/100) or an external PHY
>> via RGMII/RMII.
>>
>> Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
>> ---
>>  drivers/net/ethernet/allwinner/Kconfig      |   13 +
>>  drivers/net/ethernet/allwinner/Makefile     |    1 +
>>  drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 +++++++++++++++++++++++++++
>>  3 files changed, 2143 insertions(+)
>>  create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c b/drivers/net/ethernet/allwinner/sun8i-emac.c
>> new file mode 100644
>> index 0000000..fc0c1dd
>> --- /dev/null
>> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> +
>> +/* struct dma_desc - Structure of DMA descriptor used by the hardware
>> + * @status: Status of the frame written by HW, so RO for the
>> + *	driver (except for BIT(31) which is R/W)
>> + * @ctl: Information on the frame written by the driver (INT, len,...)
>> + * @buf_addr: physical address of the frame data
>> + * @next: physical address of next dma_desc
>> + */
>> +struct dma_desc {
>> +	u32 status;
>> +	u32 ctl;
>> +	u32 buf_addr;
>> +	u32 next;
>> +};
> 
> You should use the endian-aware variants here.

For the records: just doing the sparse annotation with __le32 here will
of course not be sufficient to make it work on BE kernels. I added
proper endianness conversion to all accesses to the descriptors and got
it to work with an arm64 big-endian kernel on the Pine64.

I put a patch here:
https://gist.github.com/apritzel/bc792c4dbbd8789f5f18aef538e8c440

This particular version is untested (though it compiles), since I just
adapted the working patch against the newer driver code and couldn't
test it yet.
I am not really an endianness expert, so don't know if there are smarter
ways to tackle this, if we should for instance provide access wrappers
to the DMA descriptor fields.

I will try to test this later today, if that works, feel free to merge
those changes into your driver.

Cheers,
Andre.

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

#1452489 — Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 28, 2016 at 04:57:34PM +0200, LABBE Corentin wrote:
> > > +static int sun8i_mdio_write(struct mii_bus *bus, int phy_addr, int phy_reg,
> > > +			    u16 data)
> > > +{
> > > +	struct net_device *ndev = bus->priv;
> > > +	struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > +	u32 reg;
> > > +	int err;
> > > +
> > > +	err = readl_poll_timeout(priv->base + SUN8I_EMAC_MDIO_CMD, reg,
> > > +				 !(reg & MDIO_CMD_MII_BUSY), 100, 10000);
> > > +	if (err) {
> > > +		dev_err(priv->dev, "%s timeout %x\n", __func__, reg);
> > > +		return err;
> > > +	}
> > 
> > Why the poll_timeout variant?
> > 
> Because, in case of bad clock/reset/regulator setting, the value
> expected to come could never be set.

Ah, I missed that it was for a busy bit, my bad. However, you seem to
be using that on several occasions, maybe you could turn that into a
function?

> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev)
> > > +{
> > > +	struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > +	u32 reg = 0;
> > > +
> > > +	if (priv->variant == H3_EMAC)
> > > +		reg = H3_EPHY_DEFAULT_VALUE;
> > 
> > Why do you need that?
> > 
> For resetting the syscon to the factory default.

Yes, but does it matter? Does it have any side effect? Is that
register shared with another device?

Otherwise, either it won't be used anymore, and you don't care, or you
will reload the driver later, and the driver should work whatever
state is programmed in there. In both cases, you don't need to reset
that value.

> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id)
> > > +{
> > > +	struct net_device *ndev = dev_id;
> > > +	struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > +	u32 v, u;
> > > +
> > > +	v = readl(priv->base + SUN8I_EMAC_INT_STA);
> > > +
> > > +	/* When this bit is asserted, a frame transmission is completed. */
> > > +	if (v & BIT(0)) {
> > > +		priv->estats.tx_int++;
> > > +		writel(0, priv->base + SUN8I_EMAC_INT_EN);
> > > +		napi_schedule(&priv->napi);
> > > +	}
> > > +
> > > +	/* When this bit is asserted, the TX DMA FSM is stopped. */
> > > +	if (v & BIT(1))
> > > +		priv->estats.tx_dma_stop++;
> > > +
> > > +	/* When this asserted, the TX DMA can not acquire next TX descriptor
> > > +	 * and TX DMA FSM is suspended.
> > > +	*/
> > > +	if (v & BIT(2))
> > > +		priv->estats.tx_dma_ua++;
> > > +
> > > +	if (v & BIT(3))
> > > +		netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n");
> > 
> > Why do you enable that interrupt if you can't handle it?
>
> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT)

So the bits 9 and 2, respectively, in the interrupt enable register
are useless?

> > And printing in the interrupt handler is a very bad idea.
> 
> There are printed only when DEBUG is set, so not a problem ?

It's always a problem, this adds a very significant latency and will
fill the kernel log buffer at an insane rate, flushing out actual
important messages, for no particular reason.
> > > +
> > > +	return IRQ_HANDLED;
> > 
> > The lack of spinlocks in there is quite worrying.
> > 
> 
> The interrupt handler just do nothing harmfull if it race with itself.
> Just stats, enabling NAPI etc..
> Anyway, It miss a comment for that non-locking strategy

The interrupt handler cannot race with itself. The interrupts will be
masked on the local CPU and the interrupt can only be delivered to a
single CPU (so, the one that the handler is currently running from).

> > > +}
> > > +
> > > +static int sun8i_emac_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device_node *node = pdev->dev.of_node;
> > > +	struct sun8i_emac_priv *priv;
> > > +	struct net_device *ndev;
> > > +	struct resource *res;
> > > +	int ret;
> > > +
> > > +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "No suitable DMA available\n");
> > > +		return ret;
> > > +	}
> > 
> > Isn't that the default?
> > 
> No, it is necessary on arm64 as apritzel requested.

http://lxr.free-electrons.com/source/drivers/of/device.c#L93

It seems to be shared between the two.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

#1452627 — Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

On Sat, Jul 30, 2016 at 1:25 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Jul 28, 2016 at 04:57:34PM +0200, LABBE Corentin wrote:
>> > > +static int sun8i_mdio_write(struct mii_bus *bus, int phy_addr, int phy_reg,
>> > > +                     u16 data)
>> > > +{
>> > > + struct net_device *ndev = bus->priv;
>> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
>> > > + u32 reg;
>> > > + int err;
>> > > +
>> > > + err = readl_poll_timeout(priv->base + SUN8I_EMAC_MDIO_CMD, reg,
>> > > +                          !(reg & MDIO_CMD_MII_BUSY), 100, 10000);
>> > > + if (err) {
>> > > +         dev_err(priv->dev, "%s timeout %x\n", __func__, reg);
>> > > +         return err;
>> > > + }
>> >
>> > Why the poll_timeout variant?
>> >
>> Because, in case of bad clock/reset/regulator setting, the value
>> expected to come could never be set.
>
> Ah, I missed that it was for a busy bit, my bad. However, you seem to
> be using that on several occasions, maybe you could turn that into a
> function?
>
>> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev)
>> > > +{
>> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
>> > > + u32 reg = 0;
>> > > +
>> > > + if (priv->variant == H3_EMAC)
>> > > +         reg = H3_EPHY_DEFAULT_VALUE;
>> >
>> > Why do you need that?
>> >
>> For resetting the syscon to the factory default.
>
> Yes, but does it matter? Does it have any side effect? Is that
> register shared with another device?
>
> Otherwise, either it won't be used anymore, and you don't care, or you
> will reload the driver later, and the driver should work whatever
> state is programmed in there. In both cases, you don't need to reset
> that value.

The "default" setting also disables and powers down the internal PHY.
I think that's a good thing? The naming could be better.

>> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id)
>> > > +{
>> > > + struct net_device *ndev = dev_id;
>> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
>> > > + u32 v, u;
>> > > +
>> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA);
>> > > +
>> > > + /* When this bit is asserted, a frame transmission is completed. */
>> > > + if (v & BIT(0)) {
>> > > +         priv->estats.tx_int++;
>> > > +         writel(0, priv->base + SUN8I_EMAC_INT_EN);
>> > > +         napi_schedule(&priv->napi);
>> > > + }
>> > > +
>> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */
>> > > + if (v & BIT(1))
>> > > +         priv->estats.tx_dma_stop++;
>> > > +
>> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor
>> > > +  * and TX DMA FSM is suspended.
>> > > + */
>> > > + if (v & BIT(2))
>> > > +         priv->estats.tx_dma_ua++;
>> > > +
>> > > + if (v & BIT(3))
>> > > +         netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n");
>> >
>> > Why do you enable that interrupt if you can't handle it?
>>
>> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT)
>
> So the bits 9 and 2, respectively, in the interrupt enable register
> are useless?

Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just
the interrupt state showing an event? IIRC some other hardware blocks have this
behavior, such as the timer.

ChenYu

>> > And printing in the interrupt handler is a very bad idea.
>>
>> There are printed only when DEBUG is set, so not a problem ?
>
> It's always a problem, this adds a very significant latency and will
> fill the kernel log buffer at an insane rate, flushing out actual
> important messages, for no particular reason.
>> > > +
>> > > + return IRQ_HANDLED;
>> >
>> > The lack of spinlocks in there is quite worrying.
>> >
>>
>> The interrupt handler just do nothing harmfull if it race with itself.
>> Just stats, enabling NAPI etc..
>> Anyway, It miss a comment for that non-locking strategy
>
> The interrupt handler cannot race with itself. The interrupts will be
> masked on the local CPU and the interrupt can only be delivered to a
> single CPU (so, the one that the handler is currently running from).
>
>> > > +}
>> > > +
>> > > +static int sun8i_emac_probe(struct platform_device *pdev)
>> > > +{
>> > > + struct device_node *node = pdev->dev.of_node;
>> > > + struct sun8i_emac_priv *priv;
>> > > + struct net_device *ndev;
>> > > + struct resource *res;
>> > > + int ret;
>> > > +
>> > > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> > > + if (ret) {
>> > > +         dev_err(&pdev->dev, "No suitable DMA available\n");
>> > > +         return ret;
>> > > + }
>> >
>> > Isn't that the default?
>> >
>> No, it is necessary on arm64 as apritzel requested.
>
> http://lxr.free-electrons.com/source/drivers/of/device.c#L93
>
> It seems to be shared between the two.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

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

#1452658 — Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Multipart message — attachments visible in raw view] — view raw

On Sat, Jul 30, 2016 at 09:30:01AM +0800, Chen-Yu Tsai wrote:
> >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev)
> >> > > +{
> >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> >> > > + u32 reg = 0;
> >> > > +
> >> > > + if (priv->variant == H3_EMAC)
> >> > > +         reg = H3_EPHY_DEFAULT_VALUE;
> >> >
> >> > Why do you need that?
> >> >
> >> For resetting the syscon to the factory default.
> >
> > Yes, but does it matter? Does it have any side effect? Is that
> > register shared with another device?
> >
> > Otherwise, either it won't be used anymore, and you don't care, or you
> > will reload the driver later, and the driver should work whatever
> > state is programmed in there. In both cases, you don't need to reset
> > that value.
> 
> The "default" setting also disables and powers down the internal PHY.
> I think that's a good thing? The naming could be better.

Ah, it might, and that would obviously be the right thing to do. Using
a define for those enable and power down bits would be better though.

> >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id)
> >> > > +{
> >> > > + struct net_device *ndev = dev_id;
> >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> >> > > + u32 v, u;
> >> > > +
> >> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA);
> >> > > +
> >> > > + /* When this bit is asserted, a frame transmission is completed. */
> >> > > + if (v & BIT(0)) {
> >> > > +         priv->estats.tx_int++;
> >> > > +         writel(0, priv->base + SUN8I_EMAC_INT_EN);
> >> > > +         napi_schedule(&priv->napi);
> >> > > + }
> >> > > +
> >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */
> >> > > + if (v & BIT(1))
> >> > > +         priv->estats.tx_dma_stop++;
> >> > > +
> >> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor
> >> > > +  * and TX DMA FSM is suspended.
> >> > > + */
> >> > > + if (v & BIT(2))
> >> > > +         priv->estats.tx_dma_ua++;
> >> > > +
> >> > > + if (v & BIT(3))
> >> > > +         netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n");
> >> >
> >> > Why do you enable that interrupt if you can't handle it?
> >>
> >> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT)
> >
> > So the bits 9 and 2, respectively, in the interrupt enable register
> > are useless?
> 
> Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just
> the interrupt state showing an event? IIRC some other hardware blocks have this
> behavior, such as the timer.

That's quite easy to implement, you can do a bitwise and on the status
and enable registers.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web