From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 373FCC43214 for ; Wed, 1 Sep 2021 21:02:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1D9C86056C for ; Wed, 1 Sep 2021 21:02:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343497AbhIAVDC (ORCPT ); Wed, 1 Sep 2021 17:03:02 -0400 Received: from vps0.lunn.ch ([185.16.172.187]:52746 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231742AbhIAVCz (ORCPT ); Wed, 1 Sep 2021 17:02:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject: Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Content-Disposition:In-Reply-To:References; bh=YQOjoNe6FO7XqE8+nHNe6fKunB7mITXvGiju7UUdEhk=; b=C7ETk0ZtnnVZcelv/9ZcNf6vac Pzw7mTAHK73VdfgIkiu3tHaQxjoq7FVhN/b3LXzeJDRaKwuGi4i+OCxT3SJ88hWIhqOLhS6ShLzpc FKcS37lYErgXoz89zigpaKajBZTZ0ReOA7F65W1k2VIlZIXzUyYbp7l75l51KtCR46y4=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1mLXMn-004uRW-1L; Wed, 01 Sep 2021 23:01:53 +0200 Date: Wed, 1 Sep 2021 23:01:53 +0200 From: Andrew Lunn To: Gerhard Engleder Cc: David Miller , Jakub Kicinski , netdev , devicetree@vger.kernel.org Subject: Re: [PATCH net-next v2 3/3] tsnep: Add TSN endpoint Ethernet MAC driver Message-ID: References: <20210831193425.26193-1-gerhard@engleder-embedded.com> <20210831193425.26193-4-gerhard@engleder-embedded.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Sep 01, 2021 at 10:18:45PM +0200, Gerhard Engleder wrote: > > > +static int tsnep_ethtool_set_priv_flags(struct net_device *netdev, > > > + u32 priv_flags) > > > +{ > > > + struct tsnep_adapter *adapter = netdev_priv(netdev); > > > + int retval; > > > + > > > + if (priv_flags & ~TSNEP_PRIV_FLAGS) > > > + return -EINVAL; > > > + > > > + if ((priv_flags & TSNEP_PRIV_FLAGS_LOOPBACK_100) && > > > + (priv_flags & TSNEP_PRIV_FLAGS_LOOPBACK_1000)) > > > + return -EINVAL; > > > + > > > + if ((priv_flags & TSNEP_PRIV_FLAGS_LOOPBACK_100) && > > > + adapter->loopback != SPEED_100) { > > > + if (adapter->loopback != SPEED_UNKNOWN) > > > + retval = phy_loopback(adapter->phydev, false); > > > + else > > > + retval = 0; > > > + > > > + if (!retval) { > > > + adapter->phydev->speed = SPEED_100; > > > + adapter->phydev->duplex = DUPLEX_FULL; > > > + retval = phy_loopback(adapter->phydev, true); > > > > This is a pretty unusual use of private flags, changing loopback at > > runtime. ethtool --test generally does that. > > > > What is your use case which requires loopback in normal operation, not > > during testing? > > Yes it is unusual. I was searching for some user space interface for loopback > and found drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c which uses > private flags. Ah, that passed my by. I would of probably said something about it. > Use case is still testing and not normal operation. Testing is done mostly with > a user space application, because I don't want to overload the driver with test > code and test frameworks can be used in user space. With loopback it is > possible to execute a lot of tests like stressing the MAC with various frame > lengths and checking TX/RX time stamps. These tests are useful for every > integration of this IP core into an FPGA and not only for IP core development. I did a quick search. CAN has something interesting: https://wiki.rdu.im/_pages/Application-Notes/Software/can-bus-in-linux.html $ sudo ip link set can0 down $ sudo ip link set can0 type can loopback on $ sudo ip link set can0 up type can bitrate 1000000 Also https://www.kernel.org/doc/Documentation/networking/can.txt The semantics are maybe slightly different. It appears to loopback can messages, but also send out the wire. I think many can transcievers can do this in hardware, but this seems to be a software feature for when the hardware cannot do it? I have seen Ethernet PHYs which do send out the wire when in loopback, so it does seem like a reasonable model. Also i like that you need to down the interface before you can put it into loopback. Saves a lot of surprises. Maybe you can look at this, see if it can be made generic, and could be used here? > > > +static irqreturn_t tsnep_irq(int irq, void *arg) > > > +{ > > > + struct tsnep_adapter *adapter = arg; > > > + u32 active = ioread32(adapter->addr + ECM_INT_ACTIVE); > > > + > > > + /* acknowledge interrupt */ > > > + if (active != 0) > > > + iowrite32(active, adapter->addr + ECM_INT_ACKNOWLEDGE); > > > + > > > + /* handle management data interrupt */ > > > + if ((active & ECM_INT_MD) != 0) { > > > + adapter->md_active = false; > > > + wake_up_interruptible(&adapter->md_wait); > > > + } > > > + > > > + /* handle link interrupt */ > > > + if ((active & ECM_INT_LINK) != 0) { > > > + if (adapter->netdev->phydev) { > > > + struct phy_device *phydev = adapter->netdev->phydev; > > > + u32 status = ioread32(adapter->addr + ECM_STATUS); > > > + int link = (status & ECM_NO_LINK) ? 0 : 1; > > > + u32 speed = status & ECM_SPEED_MASK; > > > > How does PHY link and speed get into this MAC register? Is the MAC > > polling the PHY over the MDIO bus? Is the PHY internal to the MAC and > > it has backdoor access to the PHY status? > > PHY is external. The MAC expects additional signals for link status. These > signals can be derived from RGMII in band signaling of the link status or by > using PHY link and speed LED outputs. The MAC is using the link status for > a quick no link reaction to minimize the impact to real time applications. > EtherCAT for example also uses the link LED output for a no link reaction > within a few microseconds. O.K. This is not the normal Linux way. You normally have the PHY driver tell the PHY core, which then tells the MAC driver. That always works. RGMII in band signaling is not supported by all PHY devices, and the board design would require the LED output are correctly connected, and i guess you need a hacked PHY driver to use the correct LED meanings? Plus i guess you have additional changes in the PHY driver to do fast link down detection? I think this needs another DT property to enable using such short cuts, and you should use the Linux way by default. Also, don't you need a property which tells you to either use RGMII inband, or LED signals? > > > +static int tsnep_mdiobus_read(struct mii_bus *bus, int addr, int regnum) > > > +{ > > > + struct tsnep_adapter *adapter = bus->priv; > > > + u32 md; > > > + int retval; > > > + > > > + if (regnum & MII_ADDR_C45) > > > + return -EOPNOTSUPP; > > > + > > > + /* management data frame without preamble */ > > > + md = ECM_MD_READ; > > > > I know some PHYs are happy to work without a preamble. But as far as i > > know, 802.3 c22 does not say it is optional. So this needs to be an > > opt-in feature, for when you know all the devices on the bus support > > it. We have a standard DT property for this. See mdio.yaml, > > suppress-preamble. Please look for this in the DT blob, and only > > suppress the pre-amble if it is present. > > You are right, I will improve that. You might also be interested in clock-frequency, if you can control the bus frequency. I've run Marvell PHYs at i think 8Mhz, rather than the usual 2.5MHz. > > > +static int tsnep_phy_init(struct tsnep_adapter *adapter) > > > +{ > > > + struct device_node *dn; > > > + int retval; > > > + > > > + retval = of_get_phy_mode(adapter->pdev->dev.of_node, > > > + &adapter->phy_mode); > > > + if (retval) > > > + adapter->phy_mode = PHY_INTERFACE_MODE_GMII; > > > + > > > + dn = of_parse_phandle(adapter->pdev->dev.of_node, "phy-handle", 0); > > > + adapter->phydev = of_phy_find_device(dn); > > > + of_node_put(dn); > > > + if (!adapter->phydev && adapter->mdiobus) > > > + adapter->phydev = phy_find_first(adapter->mdiobus); > > > > Do you actually need phy_find_first()? It is better to have it in DT. > > I thought it is a reasonable fallback, because then PHY can be ommited in > DT (lazy developer, unknown PHY address during development, ...). It is a reasonable fallback, until it goes wrong, because you have two PHYs on the bus etc. > Driver > and IP core will be used also on x86 over PCIe without DT. In this case this > fallback also makes sense. But I must confess, the driver is not ready for > x86 use case yet. We recently added ACPI properties. See Documentation/firmware-guide/acpi/dsd/phy.rst. Also, watch out. It might not be ready, but it will get compiled for x86, mips, powerpc etc, by the build bots, if you don't prevent it. So it needs to at least be warning free. Andrew