| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150122204925.GF22288@saruman.tx.rr.com>
Date: Thu, 22 Jan 2015 14:49:25 -0600
From: Felipe Balbi <balbi@...com>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
CC: Felipe Balbi <balbi@...com>,
Kishon Vijay Abraham I <kishon@...com>,
Baolu Lu <baolu.lu@...ux.intel.com>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210
Hi,
On Wed, Jan 21, 2015 at 11:17:49AM +0200, Heikki Krogerus wrote:
> > > TUSB1210 ULPI PHY has vendor specific register for eye
> > > diagram tuning. On some platforms the system firmware has
> > > set optimized value to it. In order to not loose the
> > > optimized value, the driver stores it during probe and
> > > restores it every time the PHY is powered back on.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > > ---
> > > drivers/phy/ulpi/Kconfig | 11 ++++
> > > drivers/phy/ulpi/Makefile | 2 +
> > > drivers/phy/ulpi/tusb1210.c | 131 ++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 144 insertions(+)
> > > create mode 100644 drivers/phy/ulpi/tusb1210.c
> > >
> > > diff --git a/drivers/phy/ulpi/Kconfig b/drivers/phy/ulpi/Kconfig
> > > index 8007df2..7cd6f82 100644
> > > --- a/drivers/phy/ulpi/Kconfig
> > > +++ b/drivers/phy/ulpi/Kconfig
> > > @@ -7,3 +7,14 @@ config ULPI_PHY
> > > Say yes if you have ULPI PHY attached to your USB controller.
> > >
> > > If unsure, say N.
> > > +
> > > +if ULPI_PHY
> > > +
> > > +config ULPI_TUSB1210
> > > + tristate "TI TUSB1210 USB PHY module"
> > > + depends on POWER_SUPPLY
> > > + select USB_PHY
> > > + help
> > > + Support for TI TUSB1210 USB ULPI PHY.
> > > +
> > > +endif
> > > diff --git a/drivers/phy/ulpi/Makefile b/drivers/phy/ulpi/Makefile
> > > index 59e61cb..7ee6679 100644
> > > --- a/drivers/phy/ulpi/Makefile
> > > +++ b/drivers/phy/ulpi/Makefile
> > > @@ -1,2 +1,4 @@
> > > ulpiphy-y := ulpi.o
> > > obj-$(CONFIG_ULPI_PHY) += ulpiphy.o
> > > +
> > > +obj-$(CONFIG_ULPI_TUSB1210) += tusb1210.o
> > > diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> > > new file mode 100644
> > > index 0000000..ac77f98
> > > --- /dev/null
> > > +++ b/drivers/phy/ulpi/tusb1210.c
> >
> > do you really need this extra ulpi directory ?
> >
> > I wonder if phy-tusb1210.c as a name would be enough.
>
> IMO grouping the ULPI PHY drivers and other ULPI bus code into
> separate folder from the start is the right thing to do.
because... :-)
> > > @@ -0,0 +1,131 @@
> > > +/**
> > > + * tusb1210.c - TUSB1210 USB ULPI PHY driver
> > > + *
> > > + * Copyright (C) 2015 Intel Corporation
> > > + *
> > > + * Author: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +#include <linux/module.h>
> > > +#include <linux/phy/ulpi/driver.h>
> > > +#include <linux/phy/ulpi/regs.h>
> > > +#include <linux/gpio/consumer.h>
> > > +
> > > +#include "ulpi_phy.h"
> > > +
> > > +struct tusb1210 {
> > > + struct ulpi *ulpi;
> > > + struct phy *phy;
> > > + struct gpio_desc *gpio_reset;
> > > + struct gpio_desc *gpio_cs;
> > > + u8 ctx[1];
> > > +};
> > > +
> > > +static int tusb1210_power_on(struct phy *phy)
> > > +{
> > > + struct tusb1210 *tusb = phy_get_drvdata(phy);
> > > +
> > > + gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> > > + gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> > > +
> > > + /* Restore eye optimisation value */
> > > + ulpi_write(tusb->ulpi, ULPI_EXT_VENDOR_SPECIFIC, tusb->ctx[0]);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int tusb1210_power_off(struct phy *phy)
> > > +{
> > > + struct tusb1210 *tusb = phy_get_drvdata(phy);
> > > +
> > > + gpiod_set_value_cansleep(tusb->gpio_reset, 0);
> > > + gpiod_set_value_cansleep(tusb->gpio_cs, 0);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct phy_ops phy_ops = {
> > > + .power_on = tusb1210_power_on,
> > > + .power_off = tusb1210_power_off,
> > > + .init = tusb1210_power_on,
> > > + .exit = tusb1210_power_off,
> > > + .owner = THIS_MODULE,
> > > +};
> > > +
> > > +static int tusb1210_probe(struct ulpi *ulpi)
> > > +{
> > > + struct gpio_desc *gpio;
> > > + struct tusb1210 *tusb;
> > > + int ret;
> > > +
> > > + tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
> > > + if (!tusb)
> > > + return -ENOMEM;
> > > +
> > > + gpio = devm_gpiod_get(&ulpi->dev, "reset");
> > > + if (!IS_ERR(gpio)) {
> > > + ret = gpiod_direction_output(gpio, 0);
> > > + if (ret)
> > > + return ret;
> > > + tusb->gpio_reset = gpio;
> > > + }
> > > +
> > > + gpio = devm_gpiod_get(&ulpi->dev, "cs");
> > > + if (!IS_ERR(gpio)) {
> > > + ret = gpiod_direction_output(gpio, 0);
> > > + if (ret)
> > > + return ret;
> > > + tusb->gpio_cs = gpio;
> > > + }
> > > +
> > > + /* Store initial eye diagram optimisation value */
> > > + ret = ulpi_read(ulpi, ULPI_EXT_VENDOR_SPECIFIC);
> >
> > do they *all* use this register for eye diagram optimization or is this
> > something that Intel decided to do ?
> >
> > (sorry, don't know much about tusb1210 other than it sucks like hell :-)
>
> All I know that somebody needs to save the value. The ones using this
> PHY who don't need to save it can most likely live without the driver.
right, but what I mean is: is it mandatory that Eye diagram
configuration be stored in *this* register? Or is it more like a scratch
register which Intel just happens to be using for Eye diagram data ?
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + tusb->ctx[0] = ret;
> > > +
> > > + tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
> > > + if (IS_ERR(tusb->phy))
> > > + return PTR_ERR(tusb->phy);
> > > +
> > > + tusb->ulpi = ulpi;
> > > +
> > > + phy_set_drvdata(tusb->phy, tusb);
> > > + dev_set_drvdata(&ulpi->dev, tusb);
> > > + return 0;
> > > +}
> > > +
> > > +static void tusb1210_remove(struct ulpi *ulpi)
> > > +{
> > > + struct tusb1210 *tusb = dev_get_drvdata(&ulpi->dev);
> >
> > completely unrelated to $subject, but we might want to have a
> > ulpi_{set,get}_drvdata() at some point.
>
> Makes sense.
>
> > In fact, we might decide to add an entire ULPI bus, eventually, though
> > I'm still considering if there's any benefit to that.
>
> I don't think I understand this comment? ULPI bus is what I'm
> introducing in this set (the first patch in it)?
I mean introducing a real struct bus ulpi_bus_type :-) With match,
probe, remove, etc.
--
balbi
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists