Re: [PATCH v3 2/2] phy: Add a driver for the ATH79 USB phy

[Alban <albeu@free.fr>]

On Tue, 2 Feb 2016 11:41:15 +0530
Kishon Vijay Abraham I <kishon@ti.com> wrote:

> Hi,
> 
> On Friday 29 January 2016 01:22 AM, Alban Bedel wrote:
> > The ATH79 USB phy is very simple, it only have a reset. On some SoC a
> > second reset is used to force the phy in suspend mode regardless of the
> > USB controller status.
> > 
> > Signed-off-by: Alban Bedel <albeu@free.fr>
> > ---
> > Changelog:
> > v2: * Rebased on the simple PHY driver
> >     * Added myself as maintainer of the driver
> > ---
> >  MAINTAINERS                 |   8 +++
> >  drivers/phy/Kconfig         |   8 +++
> >  drivers/phy/Makefile        |   1 +
> >  drivers/phy/phy-ath79-usb.c | 116 ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 133 insertions(+)
> >  create mode 100644 drivers/phy/phy-ath79-usb.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 30aca4a..f536d52 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1866,6 +1866,14 @@ S:	Maintained
> >  F:	drivers/gpio/gpio-ath79.c
> >  F:	Documentation/devicetree/bindings/gpio/gpio-ath79.txt
> >  
> > +ATHEROS 71XX/9XXX USB PHY DRIVER
> > +M:	Alban Bedel <albeu@free.fr>
> > +W:	https://github.com/AlbanBedel/linux
> > +T:	git git://github.com/AlbanBedel/linux
> > +S:	Maintained
> > +F:	drivers/phy/phy-ath79-usb.c
> > +F:	Documentation/devicetree/bindings/phy/phy-ath79-usb.txt
> > +
> >  ATHEROS ATH GENERIC UTILITIES
> >  M:	"Luis R. Rodriguez" <mcgrof@do-not-panic.com>
> >  L:	linux-wireless@vger.kernel.org
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index efbaee4..6090c63 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -15,6 +15,14 @@ config GENERIC_PHY
> >  	  phy users can obtain reference to the PHY. All the users of this
> >  	  framework should select this config.
> >  
> > +config PHY_ATH79_USB
> > +	tristate "Atheros AR71XX/9XXX USB PHY driver"
> > +	depends on ATH79 || COMPILE_TEST
> > +	default y if USB_EHCI_HCD_PLATFORM
> > +	select PHY_SIMPLE
> > +	help
> > +	  Enable this to support the USB PHY on Atheros AR71XX/9XXX SoCs.
> > +
> >  config PHY_BERLIN_USB
> >  	tristate "Marvell Berlin USB PHY Driver"
> >  	depends on ARCH_BERLIN && RESET_CONTROLLER && HAS_IOMEM && OF
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index 1cc7268..5c9ca5f 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -3,6 +3,7 @@
> >  #
> >  
> >  obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
> > +obj-$(CONFIG_PHY_ATH79_USB)		+= phy-ath79-usb.o
> >  obj-$(CONFIG_PHY_BERLIN_USB)		+= phy-berlin-usb.o
> >  obj-$(CONFIG_PHY_BERLIN_SATA)		+= phy-berlin-sata.o
> >  obj-$(CONFIG_PHY_DM816X_USB)		+= phy-dm816x-usb.o
> > diff --git a/drivers/phy/phy-ath79-usb.c b/drivers/phy/phy-ath79-usb.c
> > new file mode 100644
> > index 0000000..ff49356
> > --- /dev/null
> > +++ b/drivers/phy/phy-ath79-usb.c
> > @@ -0,0 +1,116 @@
> > +/*
> > + * Copyright (C) 2015 Alban Bedel <albeu@free.fr>
> 
> 2016?

I'll fix this one too.

> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/phy/simple.h>
> > +#include <linux/reset.h>
> > +
> > +struct ath79_usb_phy {
> > +	struct simple_phy sphy;
> > +	struct reset_control *suspend_override;
> > +};
> > +
> > +static int ath79_usb_phy_power_on(struct phy *phy)
> > +{
> > +	struct ath79_usb_phy *priv = container_of(
> > +		phy_get_drvdata(phy), struct ath79_usb_phy, sphy);
> > +	int err;
> > +
> > +	err = simple_phy_power_on(phy);
> 
> Why do you need a separate driver for ath79_usb? We should be able to directly
> use simple phy driver right?

The ath79 PHY controller misuse the reset controller to control a
suspend override. When this reset line is deasserted the PHY is forced
in suspend mode, for normal operations it must be asserted. In short
this extra reset is inverted compared to the normal use of a reset line.

To support that that in the simple driver we would first need to find a
way to support multiple resets. Not impossible, but not trivial either
as we would need to at least get a name list from somewhere. Then we
would need another scheme to indicate that this extra reset need to be
inverted, again possible but it would start be a bit hacky. All in all
that didn't sound like a good idea for something that is supposed to be
a "simple" driver, so a dedicated driver sounded like the best approach
for this case.

Alban