Hi Cyril,
On Sat, May 15, 2010 at 04:12:24, Chemparathy, Cyril wrote:
> Davinci's MDIO controller is present on other TI devices, without an
> accompanying EMAC. For example, on tnetv107x, the same MDIO module is used in
> conjunction with a 3-port switch hardware.
>
> By separating the MDIO controller code into its own platform driver, this
> patch allows common logic to be reused on such platforms.
>
> Signed-off-by: Cyril Chemparathy <[email protected]>
> ---
> drivers/net/Kconfig | 10 ++
> drivers/net/Makefile | 1 +
> drivers/net/davinci_mdio.c | 388
> ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 399 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/davinci_mdio.c
>
[...]
> diff --git a/drivers/net/davinci_mdio.c b/drivers/net/davinci_mdio.c
> new file mode 100644
> index 0000000..0a2c265
> --- /dev/null
> +++ b/drivers/net/davinci_mdio.c
> @@ -0,0 +1,388 @@
> +/*
> + * DaVinci MDIO Module driver
> + *
> + * Copyright (C) 2010 Texas Instruments.
> + *
> + * Shamelessly ripped out of davinci_emac.c, original copyrights follow:
> + *
> + * Copyright (C) 2009 Texas Instruments.
> + *
> + *
> ---------------------------------------------------------------------------
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> ---------------------------------------------------------------------------
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/phy.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +
> +#define PHY_REG_MASK 0x1f
> +#define PHY_ID_MASK 0x1f
> +
> +#define MDIO_OUT_FREQ 1100000 /* 2.2 MHz */
The comment says 2.2 MHz, but the value is 1.1 MHz?
Also, can you keep this as a platform variable (with
a 2.2 MHz default)? The frequency depends on the board,
and although most boards work at 2.2 MHz, not having it
as a platform variable will make adding a board with a
different frequency requirement difficult.
> +
> +struct davinci_mdio_regs {
> + u32 version;
> + u32 control;
> +#define CONTROL_IDLE BIT(31)
> +#define CONTROL_ENABLE BIT(30)
> +#define CONTROL_MAX_DIV (0xff)
> +
> + u32 alive;
> + u32 link;
> + u32 linkintraw;
> + u32 linkintmasked;
> + u32 __reserved_0[2];
> + u32 userintraw;
> + u32 userintmasked;
> + u32 userintmaskset;
> + u32 userintmaskclr;
> + u32 __reserved_1[20];
> +
> + struct {
> + u32 access;
> +#define USERACCESS_GO BIT(31)
> +#define USERACCESS_WRITE BIT(30)
> +#define USERACCESS_ACK BIT(29)
> +#define USERACCESS_READ (0)
> +#define USERACCESS_DATA (0xffff)
> +
> + u32 physel;
> + } user[0];
> +};
Structure overlays are usually not preferred and
is difficult to debug. Can you please keep the
direct register access method that was being
followed in the original driver?
> +
> +struct davinci_mdio_data {
> + struct davinci_mdio_regs __iomem *regs;
> + spinlock_t lock;
> + struct clk *clk;
> + struct device *dev;
> + struct mii_bus *bus;
> + bool suspended;
> +};
> +
> +/* wait until hardware is ready for another user access */
> +static inline u32 wait_for_user_access(struct davinci_mdio_data *data)
> +{
> + struct davinci_mdio_regs __iomem *regs = data->regs;
> + u32 reg;
> +
> + while ((reg = __raw_readl(®s->user[0].access)) & USERACCESS_GO)
> + ;
I know the original code had this issue too, but there should be a
timeout here to avoid permanent lockup. Also, a cpu_relax() while
looping tight.
> +
> + return reg;
> +}
> +
> +/* wait until hardware state machine is idle */
> +static inline void wait_for_idle(struct davinci_mdio_data *data)
> +{
> + struct davinci_mdio_regs __iomem *regs = data->regs;
> +
> + while ((__raw_readl(®s->control) & CONTROL_IDLE) == 0)
> + ;
Same as above.
> +}
> +
> +static int davinci_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
> +{
> + struct davinci_mdio_data *data = bus->priv;
> + u32 reg;
> +
> + if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
> + return -EINVAL;
> +
> + spin_lock(&data->lock);
The mdiobus serializes the bus access anyway so is this
lock really needed?
> +
> + if (data->suspended) {
> + spin_unlock(&data->lock);
> + return -ENODEV;
> + }
Hmm, the devices should be suspended before the bus is.
So, there should probably be a WARN_ON() here since this
indicates some issue elsewhere in the system.
Or may be this is too paranoid and can be rid of altogether?
> +
> + wait_for_user_access(data);
> + reg = (USERACCESS_GO | USERACCESS_READ | (phy_reg << 21) |
> + (phy_id << 16));
> + __raw_writel(reg, &data->regs->user[0].access);
> + reg = wait_for_user_access(data);
> + spin_unlock(&data->lock);
> +
> + return (reg & USERACCESS_ACK) ? (reg & USERACCESS_DATA) : -EIO;
> +}
> +
> +static int davinci_mdio_write(struct mii_bus *bus, int phy_id,
> + int phy_reg, u16 phy_data)
> +{
> + struct davinci_mdio_data *data = bus->priv;
> + u32 reg;
> +
> + if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
> + return -EINVAL;
> +
> + spin_lock(&data->lock);
> +
> + if (data->suspended) {
> + spin_unlock(&data->lock);
> + return -ENODEV;
> + }
> +
> + wait_for_user_access(data);
> + reg = (USERACCESS_GO | USERACCESS_WRITE | (phy_reg << 21) |
> + (phy_id << 16) | (phy_data & USERACCESS_DATA));
> + __raw_writel(reg, &data->regs->user[0].access);
> + wait_for_user_access(data);
> + spin_unlock(&data->lock);
> +
> + return 0;
> +}
> +
> +static inline int count_bits(u32 data)
> +{
> + data = ((data & 0xaaaaaaaa) >> 1) + (data & 0x55555555);
> + data = ((data & 0xcccccccc) >> 2) + (data & 0x33333333);
> + data = ((data & 0xf0f0f0f0) >> 4) + (data & 0x0f0f0f0f);
> + data = ((data & 0xff00ff00) >> 8) + (data & 0x00ff00ff);
> + data = ((data & 0xffff0000) >> 16) + (data & 0x0000ffff);
> + return data;
> +}
Kernel already has a function to calculate the hamming weight
on u32. See hweight32() in lib/hweight.c
> +
> +static int __devinit davinci_mdio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct davinci_mdio_data *data;
> + struct resource *res;
> + u32 mdio_in_freq, mdio_out_freq, div, phy_mask, ver;
> + struct phy_device *phy;
> + int ret, addr;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + dev_err(dev, "failed to alloc device data\n");
> + return -ENOMEM;
> + }
> +
> + data->bus = mdiobus_alloc();
> + if (!data->bus) {
> + dev_err(dev, "failed to alloc mii bus\n");
> + ret = -ENOMEM;
> + goto bail_out;
> + }
> +
> + data->bus->name = dev_name(dev);
> + data->bus->read = davinci_mdio_read,
> + data->bus->write = davinci_mdio_write,
> + data->bus->parent = dev;
> + data->bus->priv = data;
> + snprintf(data->bus->id, MII_BUS_ID_SIZE, "%x", pdev->id);
> +
> + data->clk = clk_get(dev, NULL);
> + if (IS_ERR(data->clk)) {
> + data->clk = NULL;
> + dev_err(dev, "failed to get device clock\n");
> + ret = PTR_ERR(data->clk);
> + goto bail_out;
> + }
> +
> + clk_enable(data->clk);
> +
> + dev_set_drvdata(dev, data);
> + data->dev = dev;
> + spin_lock_init(&data->lock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(dev, "could not find register map resource\n");
> + ret = -ENOENT;
> + goto bail_out;
> + }
> +
> + res = devm_request_mem_region(dev, res->start, resource_size(res),
> + dev_name(dev));
> + if (!res) {
> + dev_err(dev, "could not allocate register map resource\n");
> + ret = -ENXIO;
> + goto bail_out;
> + }
> +
> + data->regs = devm_ioremap_nocache(dev, res->start, resource_size(res));
> + if (!data->regs) {
> + dev_err(dev, "could not map mdio registers\n");
> + ret = -ENOMEM;
> + goto bail_out;
> + }
> +
> + mdio_in_freq = clk_get_rate(data->clk);
> + div = (mdio_in_freq / MDIO_OUT_FREQ) - 1;
> + if (div > CONTROL_MAX_DIV)
> + div = CONTROL_MAX_DIV;
Hmm, just wondering if it is safe to just default on
the max possible value? It could be a result of an issue
elsewhere and so should be reported?
> + mdio_out_freq = mdio_in_freq / (div + 1);
> +
> + /* set enable and clock divider */
> + __raw_writel(div | CONTROL_ENABLE, &data->regs->control);
> +
> + /*
> + * wait for scan logic to settle:
> + * the scan time consists of (a) a large fixed component, and (b) a
> + * small component that varies with the mii bus frequency. These
> + * were estimated using measurements at 1.1 and 2.2 MHz on tnetv107x
> + * silicon. Since the effect of (b) was found to be largely
> + * negligible, we keep things simple here.
> + */
> + mdelay(1);
> +
> + /* dump hardware version info */
> + ver = __raw_readl(&data->regs->version);
> + dev_info(dev, "davinci mdio revision %d.%d\n",
> + (ver >> 8) & 0xff, ver & 0xff);
> +
> + /* get phy mask from the alive register */
> + phy_mask = __raw_readl(&data->regs->alive);
> + if (phy_mask) {
> + /* restrict mdio bus to live phys only */
> + dev_info(dev, "detected %d phys (mask %x)\n",
> + count_bits(phy_mask), ~phy_mask);
> + phy_mask = ~phy_mask;
> + } else {
> + /* desperately scan all phys */
> + dev_warn(dev, "failed to detect live phys, scanning all\n");
> + phy_mask = 0;
phy_mask is already zero.
> + }
> + data->bus->phy_mask = phy_mask;
Looks like phy_mask local variable can be eliminated?
> +
> + /* register the mii bus */
> + ret = mdiobus_register(data->bus);
> + if (ret)
> + goto bail_out;
> +
> + /* scan and dump the bus */
> + for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
> + phy = data->bus->phy_map[addr];
> + if (phy) {
> + dev_info(dev, "phy[%d]: device %s, driver %s\n",
> + phy->addr, dev_name(&phy->dev),
> + phy->drv ? phy->drv->name : "unknown");
> + }
> + }
> +
> + return 0;
> +
> +bail_out:
> + if (data->bus)
> + mdiobus_free(data->bus);
> +
> + if (data->clk) {
> + clk_disable(data->clk);
> + clk_put(data->clk);
> + }
> +
> + kfree(data);
> +
> + return 0;
return ret?
Thanks,
Sekhar
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
