Date:	Mon, 3 Mar 2014 16:11:02 -0600
From:	Vince Bridgers <vbridgers2013@...il.com>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	netdev <netdev@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Rob Landley <rob@...dley.net>
Subject: Re: [PATCH RFC 3/3] Altera TSE: Add Altera Triple Speed Ethernet
 (TSE) Driver

On Mon, Mar 3, 2014 at 12:38 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
> 2014-03-03 10:21 GMT-08:00 Vince Bridgers <vbridgers2013@...il.com>:
>> Hello Florian, thank you for taking the time to comments. My responses inline.
>>
>> On Sun, Mar 2, 2014 at 6:59 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
>>> Hello Vince,
>>>
>>> It might help reviewing the patches by breaking the patches into:
>>>
>>> - the SGDMA bits
>>> - the MSGDMA bits
>>> - the Ethernet MAC driver per-se
>>
>> I'll break down the next submission.
>>
>>>
>>> BTW, it does look like the SGDMA code could/should be a dmaengine driver?
>>
>> I did consider this, but after studying the dmaengine api I found the
>> API definitions and semantics were not a match to the way the SGDMA
>> and MSGDMA behave collectively. Moreover, I could not find an example
>> of an Ethernet driver that made use of the dmaengine API - only the
>> Micrel driver seems to use it. When studying what components actually
>> used the dmaengine API I concluded the dmaengine API was defined for
>> use cases different than Ethernet.
>
> To tell you the truth, I have myself been toying with the idea of
> using a dmaengine driver for some Ethernet drivers out there (mainly
> bcm63xx_enet), but so far have not had any chance to take a stab at
> doing a real implementation. My concern was more about the fact that
> maybe the SGDMA/MSGDMA code could be reusable for other peripherals in
> your system, such as USB device, PCM etc... This is fine anyway.
>
> [snip]
>
>>>
>>> Is this the software number of descriptors or hardware number of
>>> descriptors?
>>
>> This number applies to the number of descriptors as limited by the
>> MSGDMA capabilities. The SGDMA has different limitations and issues,
>> but the maximum number of descriptors for either DMA engine that can
>> be used is represented as shown above. This is important since an
>> unusual hardware configuration could support both SGDMA and MSGDMA
>> simultaneously for more than one TSE instance. I used a design that
>> supported a single SGDMA with TSE and a single MSGDMA with TSE for
>> testing purposes (among other designs). So this a hardware defined
>> number of descriptors and is fixed.
>
> Ok, so this describes a hardware configuration, and as such should be
> part of some Device Tree properties, something that could easily be
> changed by someone wanting to slightly modify the RTL parameters.
>
>>
>>>
>>> [snip]
>>>
>>>
>>>> +
>>>> +static int altera_tse_mdio_create(struct net_device *dev, unsigned int
>>>> id)
>>>> +{
>>>> +       struct altera_tse_private *priv = netdev_priv(dev);
>>>> +       int ret;
>>>> +       int i;
>>>> +       struct device_node *mdio_node;
>>>> +       struct mii_bus *mdio;
>>>> +
>>>> +       mdio_node = of_find_compatible_node(priv->device->of_node, NULL,
>>>> +               "altr,tse-mdio");
>>>> +
>>>> +       if (mdio_node) {
>>>> +               dev_warn(priv->device, "FOUND MDIO subnode\n");
>>>> +       } else {
>>>> +               dev_warn(priv->device, "NO MDIO subnode\n");
>>>> +               return 0;
>>>> +       }
>>>> +
>>>> +       mdio = mdiobus_alloc();
>>>> +       if (mdio == NULL) {
>>>> +               dev_err(priv->device, "Error allocating MDIO bus\n");
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       mdio->name = ALTERA_TSE_RESOURCE_NAME;
>>>> +       mdio->read = &altera_tse_mdio_read;
>>>> +       mdio->write = &altera_tse_mdio_write;
>>>> +       snprintf(mdio->id, MII_BUS_ID_SIZE, "%s-%u", mdio->name, id);
>>>
>>>
>>> You could use something more user-friendly such as mdio_node->full_name.
>>
>> The full name will exceed the characters available in that particular
>> structure data member. MII_BUS_ID_SIZE is defined as (20-3) in
>> include/linux/phy.h. The full name would exceed the allocated space in
>> that structure. That's why this method was chosen.
>
> Ok, that works too.
>
>>
>>>
>>>
>>>> +
>>>> +       mdio->irq = kcalloc(PHY_MAX_ADDR, sizeof(int), GFP_KERNEL);
>>>> +       if (mdio->irq == NULL) {
>>>> +               dev_err(priv->device, "%s: Cannot allocate memory\n",
>>>> __func__);
>>>> +               ret = -ENOMEM;
>>>> +               goto out_free_mdio;
>>>> +       }
>>>> +       for (i = 0; i < PHY_MAX_ADDR; i++)
>>>> +               mdio->irq[i] = PHY_POLL;
>>>> +
>>>> +       mdio->priv = (void *)priv->mac_dev;
>>>
>>>
>>> No need for the cast here, this is already a void *.
>>
>> Noted.
>>
>>>
>>>
>>>> +       mdio->parent = priv->device;
>>>
>>>
>>> [snip]
>>>
>>>
>>>> +               /* make cache consistent with receive packet buffer */
>>>> +               dma_sync_single_for_cpu(priv->device,
>>>> +                                       priv->rx_ring[entry].dma_addr,
>>>> +                                       priv->rx_ring[entry].len,
>>>> +                                       DMA_FROM_DEVICE);
>>>> +
>>>> +               dma_unmap_single(priv->device,
>>>> priv->rx_ring[entry].dma_addr,
>>>> +                                priv->rx_ring[entry].len,
>>>> DMA_FROM_DEVICE);
>>>> +
>>>> +               /* make sure all pending memory updates are complete */
>>>> +               rmb();
>>>
>>>
>>> Are you sure this does something in your case? I am fairly sure that the
>>> dma_unmap_single() call would have done that implicitely for you here.
>>
>> I wrote the code this way intentionally to be explicit. I'll check the
>> API for behavior as you describe for both ARM and NIOS and if not
>> handled this barrier should probably remain.
>>
>
> [snip]
>
>>>
>>> I am not sure this will even be triggered if you want do not advertise
>>> NETIF_F_SG, so you might want to drop that entirely.
>>
>> The intent was to add Scatter Gather capabilities at some point in the
>> future, so this was a form of documenting. I'll just drop the code and
>> add a comment instead if you agree.
>
> Since you are not advertising the NETIF_F_SG capability bit, you
> should probably just drop this code and add it back again under a
> different form when SG support is added.

Noted, will do.

>
> [snip]
>
>>>
>>> You might rather do this during your driver probe function rather than in
>>> the ndo_open() callback.
>>
>> This seems to be the normal place to probe and initialize the phy from
>> examination of existing code. Perhaps I missed something, could you
>> provide an example of where this is done differently?
>
> Most drivers I can think about: tg3, bcmgenet, bcm63xx_enet, r6040 do
> probe for the PHY in their driver's probe routine.

Thank you for the pointers.

>
>>
>>>
>>> [snip]
>>>
>>>
>>>> +       /* Stop and disconnect the PHY */
>>>> +       if (priv->phydev) {
>>>> +               phy_stop(priv->phydev);
>>>> +               phy_disconnect(priv->phydev);
>>>> +               priv->phydev = NULL;
>>>> +       }
>>>> +
>>>> +       netif_stop_queue(dev);
>>>> +       napi_disable(&priv->napi);
>>>> +
>>>> +       /* Disable DMA interrupts */
>>>> +       spin_lock_irqsave(&priv->rxdma_irq_lock, flags);
>>>> +       priv->disable_rxirq(priv);
>>>> +       priv->disable_txirq(priv);
>>>> +       spin_unlock_irqrestore(&priv->rxdma_irq_lock, flags);
>>>> +
>>>> +       /* Free the IRQ lines */
>>>> +       free_irq(priv->rx_irq, dev);
>>>> +       free_irq(priv->tx_irq, dev);
>>>> +
>>>> +       /* disable and reset the MAC, empties fifo */
>>>> +       spin_lock(&priv->mac_cfg_lock);
>>>> +       spin_lock(&priv->tx_lock);
>>>> +
>>>> +       ret = reset_mac(priv);
>>>> +       if (ret)
>>>> +               netdev_err(dev, "Cannot reset MAC core (error: %d)\n",
>>>> ret);
>>>> +       priv->reset_dma(priv);
>>>> +       free_skbufs(dev);
>>>> +
>>>> +       spin_unlock(&priv->tx_lock);
>>>> +       spin_unlock(&priv->mac_cfg_lock);
>>>> +
>>>> +       priv->uninit_dma(priv);
>>>> +
>>>> +       netif_carrier_off(dev);
>>>
>>>
>>> phy_stop() does that already.
>>
>> If you mean phy_stop calls netif_carrier_off, I don't see it in that
>> function (phy/phy.c). Common usage in the intree drivers seems to be
>> calling netif_carrier_off in this context, but perhaps the drivers I
>> examined have not been updated. Is there some more specific feedback
>> or comments that you could provide here? Do you mean that
>> netif_carrier_off is unnecessary here?
>
> by calling phy_stop() the PHY state machine will move to the state
> PHY_HALTED, if the link state was valid at this point, it will call
> netif_carrier_off(). If the link was not valid before, the PHY state
> machine would have already called netif_carrier_off().
>
>>
>>>
>>>
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static struct net_device_ops altera_tse_netdev_ops = {
>>>> +       .ndo_open               = tse_open,
>>>> +       .ndo_stop               = tse_shutdown,
>>>> +       .ndo_start_xmit         = tse_start_xmit,
>>>> +       .ndo_set_mac_address    = eth_mac_addr,
>>>> +       .ndo_set_rx_mode        = tse_set_rx_mode,
>>>> +       .ndo_change_mtu         = tse_change_mtu,
>>>> +       .ndo_validate_addr      = eth_validate_addr,
>>>> +};
>>>> +
>>>> +static int altera_tse_get_of_prop(struct platform_device *pdev,
>>>> +                                 const char *name, unsigned int *val)
>>>> +{
>>>> +       const __be32 *tmp;
>>>> +       int len;
>>>> +       char buf[strlen(name)+1];
>>>> +
>>>> +       tmp = of_get_property(pdev->dev.of_node, name, &len);
>>>> +       if (!tmp && !strncmp(name, "altr,", 5)) {
>>>> +               strcpy(buf, name);
>>>> +               strncpy(buf, "ALTR,", 5);
>>>> +               tmp = of_get_property(pdev->dev.of_node, buf, &len);
>>>> +       }
>>>> +       if (!tmp || (len < sizeof(__be32)))
>>>> +               return -ENODEV;
>>>> +
>>>> +       *val = be32_to_cpup(tmp);
>>>> +       return 0;
>>>> +}
>>>
>>>
>>> Do we really need that abstration?
>>
>> The intent here is to support legacy device trees that were created
>> with upper case "ALTR".
>
> Oh Device Tree fun, welcome to the club.

Yes, thanks for the warm welcome :) There's certainly no shortage of
oddball corner cases to take care of.

>
>>
>>>
>>>
>>>> +
>>>> +static int altera_tse_get_phy_iface_prop(struct platform_device *pdev,
>>>> +                                        phy_interface_t *iface)
>>>> +{
>>>> +       const void *prop;
>>>> +       int len;
>>>> +
>>>> +       prop = of_get_property(pdev->dev.of_node, "phy-mode", &len);
>>>> +       if (!prop)
>>>> +               return -ENOENT;
>>>> +       if (len < 4)
>>>> +               return -EINVAL;
>>>> +
>>>> +       if (!strncmp((char *)prop, "mii", 3)) {
>>>> +               *iface = PHY_INTERFACE_MODE_MII;
>>>> +               return 0;
>>>> +       } else if (!strncmp((char *)prop, "gmii", 4)) {
>>>> +               *iface = PHY_INTERFACE_MODE_GMII;
>>>> +               return 0;
>>>> +       } else if (!strncmp((char *)prop, "rgmii-id", 8)) {
>>>> +               *iface = PHY_INTERFACE_MODE_RGMII_ID;
>>>> +               return 0;
>>>> +       } else if (!strncmp((char *)prop, "rgmii", 5)) {
>>>> +               *iface = PHY_INTERFACE_MODE_RGMII;
>>>> +               return 0;
>>>> +       } else if (!strncmp((char *)prop, "sgmii", 5)) {
>>>> +               *iface = PHY_INTERFACE_MODE_SGMII;
>>>> +               return 0;
>>>> +       }
>>>
>>>
>>> of_get_phy_mode() does that for you.
>>
>> Will address this. Thank you.
>>
>>>
>>>
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +
>>>> +static int request_and_map(struct platform_device *pdev, const char
>>>> *name,
>>>> +                          struct resource **res, void __iomem **ptr)
>>>> +{
>>>> +       struct resource *region;
>>>> +       struct device *device = &pdev->dev;
>>>> +
>>>> +       *res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>>>> +       if (*res == NULL) {
>>>> +               dev_err(device, "resource %s not defined\n", name);
>>>> +               return -ENODEV;
>>>> +       }
>>>> +
>>>> +       region = devm_request_mem_region(device, (*res)->start,
>>>> +                                        resource_size(*res),
>>>> dev_name(device));
>>>> +       if (region == NULL) {
>>>> +               dev_err(device, "unable to request %s\n", name);
>>>> +               return -EBUSY;
>>>> +       }
>>>> +
>>>> +       *ptr = devm_ioremap_nocache(device, region->start,
>>>> +                                   resource_size(region));
>>>> +       if (*ptr == NULL) {
>>>> +               dev_err(device, "ioremap_nocache of %s failed!", name);
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +/* Probe Altera TSE MAC device
>>>> + */
>>>> +static int altera_tse_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct net_device *ndev;
>>>> +       int ret = -ENODEV;
>>>> +       struct resource *control_port;
>>>> +       struct resource *dma_res;
>>>> +       struct altera_tse_private *priv;
>>>> +       int len;
>>>> +       const unsigned char *macaddr;
>>>> +       struct device_node *np = pdev->dev.of_node;
>>>> +       unsigned int descmap;
>>>> +
>>>> +       ndev = alloc_etherdev(sizeof(struct altera_tse_private));
>>>> +       if (!ndev) {
>>>> +               dev_err(&pdev->dev, "Could not allocate network
>>>> device\n");
>>>> +               return -ENODEV;
>>>> +       }
>>>> +
>>>> +       SET_NETDEV_DEV(ndev, &pdev->dev);
>>>> +
>>>> +       priv = netdev_priv(ndev);
>>>> +       priv->device = &pdev->dev;
>>>> +       priv->dev = ndev;
>>>> +       priv->msg_enable = netif_msg_init(debug, default_msg_level);
>>>> +
>>>> +       if (of_device_is_compatible(np, "altr,tse-1.0") ||
>>>> +           of_device_is_compatible(np, "ALTR,tse-1.0")) {
>>>
>>>
>>> Use the .data pointer associated with the compatible string to help you do
>>> that, see below.
>>
>> Noted. If we can agree that use of the dmaengine api is inappropriate
>> in this case, I'll update the code to use a .data pointer as
>> suggested. Thank you for the comment.
>
> I think not using the dmaengine API is fine.
>
>>
>>>
>>> [snip]
>>>
>>>
>>>> +       /* get supplemental address settings for this instance */
>>>> +       ret = altera_tse_get_of_prop(pdev, "altr,enable-sup-addr",
>>>> +                                    &priv->added_unicast);
>>>> +       if (ret)
>>>> +               priv->added_unicast = 0;
>>>> +
>>>> +       /* Max MTU is 1500, ETH_DATA_LEN */
>>>> +       priv->max_mtu = ETH_DATA_LEN;
>>>
>>>
>>> How about VLANs? If this is always 1500, just let the core ethernet
>>> functions configure the MTU for your interface.
>>
>> The TSE core handles frame size expansion for VLAN tagged frames, so
>> it's ok (tested). At some point, frame sizes > 1518 may be supported
>> (the core supports Jumbo frames, the driver is intentionally simple
>> for initial submission). Your comment is noted, I accept the
>> suggestion.
>
> In case this needs to be matched against a newer version of the RTL,
> or whatever HW configuration some RTL user is allowed to make, you
> could probaly use the 'max-frame-size' ePAPR-defined property for
> this?

Yes, another oversight. Thank you for catching this and the suggestion.

>
>>
>>>
>>>
>>>> +
>>>> +       /* The DMA buffer size already accounts for an alignment bias
>>>> +        * to avoid unaligned access exceptions for the NIOS processor,
>>>> +        */
>>>> +       priv->rx_dma_buf_sz = ALTERA_RXDMABUFFER_SIZE;
>>>> +
>>>> +       /* get default MAC address from device tree */
>>>> +       macaddr = of_get_property(pdev->dev.of_node, "local-mac-address",
>>>> &len);
>>>> +       if (macaddr && len == ETH_ALEN)
>>>> +               memcpy(ndev->dev_addr, macaddr, ETH_ALEN);
>>>> +
>>>> +       /* If we didn't get a valid address, generate a random one */
>>>> +       if (!is_valid_ether_addr(ndev->dev_addr))
>>>> +               eth_hw_addr_random(ndev);
>>>> +
>>>> +       ret = altera_tse_get_phy_iface_prop(pdev, &priv->phy_iface);
>>>> +       if (ret == -ENOENT) {
>>>> +               /* backward compatability, assume RGMII */
>>>> +               dev_warn(&pdev->dev,
>>>> +                        "cannot obtain PHY interface mode, assuming
>>>> RGMII\n");
>>>> +               priv->phy_iface = PHY_INTERFACE_MODE_RGMII;
>>>> +       } else if (ret) {
>>>> +               dev_err(&pdev->dev, "unknown PHY interface mode\n");
>>>> +               goto out_free;
>>>> +       }
>>>> +
>>>> +       /* try to get PHY address from device tree, use PHY autodetection
>>>> if
>>>> +        * no valid address is given
>>>> +        */
>>>> +       ret = altera_tse_get_of_prop(pdev, "altr,phy-addr",
>>>> &priv->phy_addr);
>>>> +       if (ret)
>>>> +               priv->phy_addr = POLL_PHY;
>>>
>>>
>>> Please do not use such as custom property, either you use an Ethernet PHY
>>> device tree node as described in ePAPR; or you do not and use a fixed-link
>>> property instead.
>>
>> Agreed, the code tries phy handles as described in the ePAPR v1.1
>> specification, then falls back to the method in question. The intent
>> is to support legacy device trees as well. Is there a preferred way to
>> handle legacy configurations that we may encounter in the wild?
>
> Well, ideally a new driver should have no legacy at all, but I
> understand that situation might not be the case, I believe it should
> be duly noted in the Device Tree binding documentation (have not
> reviewed that patch yet). Issuing a warning might be beneficial as
> well to spot "old" DT bindings and help troubleshooting setups?
> --
> Florian

Thank you for the constructive comments and suggestions. I'll get this
turned about in 1-2 days.

All the best,

Vince
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists