Re: [PATCH 10/12] drivers: PMC MSP71xx ethernet driver

[Jeff Garzik <jeff@garzik.org>]

Marc St-Jean wrote:
> Jeff Garzik wrote:
>> Marc St-Jean wrote:
>>  > +     res = platform_get_resource(pldev, IORESOURCE_MEM, 0);
>>  > +     if (!res) {
>>  > +             printk(KERN_ERR "MSPETH(probe) %s: "
>>  > +                     "IOMEM resource not found for eth%d\n",
>>  > +                     dev->name, unit);
>>  > +             goto out_netdev;
>>  > +     }
>>  > +
>>  > +     /* reserve the memory region */
>>  > +     if (!request_mem_region(res->start, res->end - res->start + 1,
>>  > +                             cardname)) {
>>  > +             printk(KERN_ERR "MSPETH(probe) %s: unable to "
>>  > +                     "get memory/io address region 0x08%lx\n",
>>  > +                     dev->name, dev->base_addr);
>>  > +             goto out_netdev;
>>  > +     }
>>  > +
>>  > +     /* remap the memory */
>>  > +     mapaddr = ioremap_nocache(res->start, res->end - res->start + 1);
>>  > +     if (!mapaddr) {
>>  > +             printk(KERN_WARNING "MSPETH(probe) %s: "
>>  > +                     "unable to ioremap address 0x%08x\n",
>>  > +                     dev->name, res->start);
>>  > +             goto out_unreserve;
>>  > +     }
>>  > +
>>  > +     lp->mapaddr = mapaddr;
>>  > +     dev->base_addr = res->start;
>>  > +     dev->irq = platform_get_irq(pldev, 0);
>>  > +
>>  > +     /* remap the system reset registers */
>>  > +     lp->rstaddr = ioremap_nocache(MSP_RST_BASE, MSP_RST_SIZE);
>>  > +     if (!lp->rstaddr) {
>>  > +             printk(KERN_ERR "MSPETH(probe) %s: unable to "
>>  > +                     "ioremap address 0x%08x\n",
>>  > +                     dev->name, MSP_RST_BASE);
>>  > +             goto out_unmap;
>>  > +     }
>>  > +
>>  > +     /* set the logical and hardware units */
>>  > +     lp->unit = unit;
>>  > +     lp->hwunit = hwunit;
>>  > +
>>  > +     /* probe for PHYS attached to this MACs MDIO interface */
>>  > +     if (mspeth_phyprobe(dev))
>>  > +             goto out_unmap;
>>  > +
>>  > +     /* parse the environment and command line */
>>  > +     mspeth_init_cmdline(dev);
>>  > +     mspeth_init_phyaddr(dev);
>>  > +
>>  > +     /* MAC address */
>>  > +     dev->addr_len = ETH_ALEN;
>>  > +     for (i = 0; i < dev->addr_len; i++)
>>  > +             dev->dev_addr[i] = macaddr[i];
>>  > +
>>  > +     /* register the /proc entry */
>>  > +     snprintf(tmp_str, 128, "pmcmspeth%d", unit);
>>  > +     create_proc_read_entry(tmp_str, 0644, proc_net,
>>  > +                             mspeth_proc_info, dev);
>>
>> use sysfs
> 
> I thought sysfs was only required if the file was to be writable?
> Is this no longer the case?

That was never the case.

And these days, additions to procfs are insta-deprecated.  New code 
should start out with sysfs.


>>  > 
>> +/************************************************************************** 
>>
>>  > + * Probe the hardware and fill out the array of PHY control elements
>>  > + */
>>  > +static int
>>  > +mspeth_phyprobe(struct net_device *dev)
>>  > +{
>>  > +     struct mspeth_priv *lp = netdev_priv(dev);
>>  > +     u32 reg1;
>>  > +     int phyaddr;
>>  > +     struct mspeth_phy tmp_phy;
>>  > +     struct mspeth_phy **tail_pp;
>>  > +
>>  > +     tmp_phy.next_phy = NULL;
>>  > +     tmp_phy.hwunit = lp->hwunit;
>>  > +     tmp_phy.phyaddr = 0;
>>  > +     tmp_phy.memaddr = lp->mapaddr + MSPETH_MD_DATA;
>>  > +     tmp_phy.assigned = false;
>>  > +     tmp_phy.linkup = false;
>>  > +     spin_lock_init(&tmp_phy.lock);
>>  > +
>>  > +     /* find the tail of the phy list */
>>  > +     for (tail_pp = &root_phy_dev; *tail_pp != NULL;
>>  > +          tail_pp = &(*tail_pp)->next_phy) {;}
>>  > +
>>  > +     /* probe the phys and add to list */
>>  > +     for (phyaddr = 0; phyaddr < MD_MAX_PHY; phyaddr++) {
>>
>> for standard MII, you should start at PHY ID 1, since PHY ID 0 is often
>> a ghost.  normal loop looks like
>>
>>                 for (phy = 1; phy <= 32 && phy_idx < MII_CNT; phy++) {
>>                  int phyx = phy & 0x1f;
> 
> I'm not sure what you mean by ghost, an alias for ID 1?

An alias for a non-zero PHY ID.


> We have several boards which access either stand-alone or switch/phy-combos
> at ID 0. We don't have access to all boards (some are customers) and can't
> risk breaking them at this point.

Did you read my example code?  It clearly scans PHY ID 0.  What would break?


> I've only looked at mii.c briefly but it appears that it would involve a complete
> re-write of your mspeth_phy struct and all code accessing it. That is risky
> for a driver that has been solid for the last couple years and is already in
> production.
> 
> Is this mandatory?

We are not inclined to merge Yet Another Duplicate of mii.c, which means 
twice the maintenance headache over the lifetime of the Linux kernel.


>>  > + * cleared afterwards, although we don't change the pointers so
>>  > + * they don't need to be reallocated.
>>  > + */
>>  > +static void
>>  > +mspeth_mac_reset(struct net_device *dev)
>>  > +{
>>  > +     struct mspeth_priv *lp = netdev_priv(dev);
>>  > +     int i;
>>  > +     u32 rstpat;
>>  > +
>>  > +     /* hardware reset */
>>  > +     switch (lp->hwunit) {
>>  > +     case 0:
>>  > +             rstpat = MSP_EA_RST;
>>  > +             break;
>>  > +     case 1:
>>  > +             rstpat = MSP_EB_RST;
>>  > +             break;
>>  > +     case 2:
>>  > +             rstpat = MSP_EC_RST;
>>  > +             break;
>>  > +     default:
>>  > +             printk(KERN_WARNING
>>  > +                     "MSPETH(mac_reset) %s: Unsupported hwunit %d\n",
>>  > +                     dev->name, lp->hwunit);
>>  > +             return;
>>  > +     }
>>  > +
>>  > +     __raw_writel(rstpat, lp->rstaddr + MSPRST_SET);
>>  > +     mdelay(100);
>>  > +     __raw_writel(rstpat, lp->rstaddr + MSPRST_CLR);
>>
>> host bus posting bug?
> 
> Not sure what you mean here? The above lines reset one of the MACs within the
> SoC device.

Are writes delayed or posted in any way?

If yes, then the timing of mdelay() is not guaranteed.  Normally you 
must issue a read, following a write, before your delay.  That 
guarantees the full delay occurs after the write reaches the MAC.


>>  > +     /* load station address to ARC */
>>  > +     mspeth_set_arc_entry(dev, ARC_ENTRY_SOURCE, dev->dev_addr);
>>  > +
>>  > +     /* Enable ARC (broadcast and unicast) */
>>  > +     msp_write(lp, MSPETH_ARC_Ena, ARC_Ena_Bit(ARC_ENTRY_SOURCE));
>>  > +     msp_write(lp, MSPETH_ARC_Ctl, ARC_CompEn | ARC_BroadAcc);
>>  > +
>>  > +     /* configure DMA */
>>  > +     msp_write(lp, MSPETH_DMA_Ctl, DMA_CTL_CMD);
>>  > +
>>  > +     /* configure the RX/TX mac */
>>  > +     msp_write(lp, MSPETH_RxFragSize, 0);
>>  > +     msp_write(lp, MSPETH_TxPollCtr, TX_POLL_CNT);
>>  > +     msp_write(lp, MSPETH_TxThrsh, TX_THRESHOLD);
>>  > +
>>  > +     /* zero and enable the interrupts */
>>  > +     lp->fatal_icnt = 0;
>>  > +     msp_write(lp, MSPETH_Int_En, INT_EN_CMD);
>>  > +
>>  > +     /*
>>  > +      * Set queues
>>  > +      *
>>  > +      * hammtrev, 2005-11-25:
>>  > +      * Using the formula used in the old driver, which gives a
>>  > +      * little bit less than (RX_BUF_NUM - 1) << 5, allowing for more
>>  > +      * buffer descriptors attached to a frame descriptor.
>>  > +      */
>>  > +     msp_write(lp, MSPETH_FDA_Bas, (u32)lp->rxfd_base);
>>  > +     msp_write(lp, MSPETH_FDA_Lim, (RX_BUF_NUM - 1) << 5);
>>
>> you should be using DMA mapping functions (if only silly wrappers), not
>> direct casts
> 
> A bit of background to clarify the drivers use...
> This driver is for a proprietary MAC sitting across a proprietary bus. It is
> also device and arch specific, it will never be on a PCI card.
> 
> By mapping the frame and buffer descriptors to a uncached memory segment and
> allowing the DMA to make use of those addresses, we avoid a lot of extra
> code/cycles when manipulating those structures throughout the driver.
> 
> In order to accomplish this the MAC must be given the uncached address above.

See the "if only silly wrappers" comment.  You should create a 
DMA-mapping API, even if it intentionally degenerates inside the 
compiler's optimizer to a direct memory access and/or no-ops.  The gives 
the driver well-defined access points for accessing memory with unusual 
attributes such as uncached memory segments.


>>  > + * Open/initialize the board. This is called (in the current kernel)
>>  > + * sometime after booting when the 'ifconfig' program is run.
>>  > + *
>>  > + * This routine should set everything up anew at each open, even
>>  > + * registers that "should" only need to be set once at boot, so that
>>  > + * there is non-reboot way to recover if something goes wrong.
>>  > + */
>>  > +static int
>>  > +mspeth_open(struct net_device *dev)
>>  > +{
>>  > +     struct mspeth_priv *lp = netdev_priv(dev);
>>  > +     int err = -EBUSY;
>>  > +
>>  > +     /* reset the hardware, disabling/clearing all interrupts */
>>  > +     mspeth_mac_reset(dev);
>>  > +     mspeth_phy_reset(dev);
>>  > +
>>  > +     /* determine preset speed and duplex settings */
>>  > +     if (lp->option & MSP_OPT_10M)
>>  > +             lp->speed = 10;
>>  > +     else
>>  > +             lp->speed = 100;
>>  > +
>>  > +     if (lp->option & MSP_OPT_FDUP)
>>  > +             lp->fullduplex = true;
>>  > +     else
>>  > +             lp->fullduplex = false;
>>  > +
>>  > +     /* initialize the queues and the hardware */
>>  > +     if (!mspeth_init_queues(dev)) {
>>  > +             printk(KERN_ERR "MSPETH(open) %s: "
>>  > +                     "Unable to allocate queues\n", dev->name);
>>  > +             goto out_err;
>>  > +     }
>>  > +
>>  > +     /* allocate and initialize the tasklets */
>>  > +#ifndef CONFIG_MSPETH_NAPI
>>  > +     tasklet_init(&lp->rx_tasklet, mspeth_rx, (u32)dev);
>>  > +     tasklet_init(&lp->tx_tasklet, mspeth_txdone, (u32)dev);
>>  > +#endif
>>
>> with tasklets you don't need NAPI.  also, the casts are bogus
> 
> Some of our customers prefer NAPI over tasklets, that is why we have
> a configuration options so each one can choose the preferred method.

This again revisits the issue of /not/ duplicating code that exists 
elsewhere.  The CONFIG_MSPETH_NAPI choice is essentially between "NAPI" 
and "NAPI implemented solely within our driver".  Drivers that permit a 
NAPI compilation choice allow the user to choose between no software 
mitigation (no-NAPI) or software mitigation.  That is a reasonable choice.

But including your own software mitigation scheme is wasteful, and 
different from the NAPI/no-NAPI configurations that normally get accepted.

	Jeff