Date:	Wed, 3 Apr 2013 19:48:42 +0530
From:	Kishon Vijay Abraham I <kishon@...com>
To:	<balbi@...com>
CC:	<gregkh@...uxfoundation.org>, <arnd@...db.de>,
	<akpm@...ux-foundation.org>, <swarren@...dotorg.org>,
	<sylvester.nawrocki@...il.com>, <rob@...dley.net>,
	<netdev@...r.kernel.org>, <davem@...emloft.net>,
	<cesarb@...arb.net>, <linux-usb@...r.kernel.org>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<tony@...mide.com>, <grant.likely@...retlab.ca>,
	<rob.herring@...xeda.com>, <b-cousson@...com>,
	<linux@....linux.org.uk>, <eballetbo@...il.com>,
	<javier@...hile0.org>, <mchehab@...hat.com>,
	<santosh.shilimkar@...com>, <broonie@...nsource.wolfsonmicro.com>,
	<swarren@...dia.com>, <linux-doc@...r.kernel.org>,
	<devicetree-discuss@...ts.ozlabs.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework

Hi,

On Wednesday 03 April 2013 07:12 PM, Felipe Balbi wrote:
> On Wed, Apr 03, 2013 at 06:23:49PM +0530, Kishon Vijay Abraham I wrote:
>> The PHY framework provides a set of APIs for the PHY drivers to
>> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
>> PHY with or without using phandle. To obtain a reference to the PHY without
>> using phandle, the platform specfic intialization code (say from board file)
>> should have already called phy_bind with the binding information. The binding
>> information consists of phy's device name, phy user device name and an index.
>> The index is used when the same phy user binds to mulitple phys.
>>
>> PHY drivers should create the PHY by passing phy_descriptor that has
>> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
>> power_on, power_off.
>>
>> The documentation for the generic PHY framework is added in
>> Documentation/phy.txt and the documentation for the sysfs entry is added
>> in Documentation/ABI/testing/sysfs-class-phy and the documentation for
>> dt binding is can be found at
>> Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@...com>
>> ---
>>   Documentation/ABI/testing/sysfs-class-phy          |   15 +
>>   .../devicetree/bindings/phy/phy-bindings.txt       |   67 +++
>>   Documentation/phy.txt                              |  113 ++++
>>   MAINTAINERS                                        |    7 +
>>   drivers/Kconfig                                    |    2 +
>>   drivers/Makefile                                   |    2 +
>>   drivers/phy/Kconfig                                |   13 +
>>   drivers/phy/Makefile                               |    5 +
>>   drivers/phy/phy-core.c                             |  616 ++++++++++++++++++++
>>   include/linux/phy/phy.h                            |  228 ++++++++
>>   10 files changed, 1068 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-class-phy
>>   create mode 100644 Documentation/devicetree/bindings/phy/phy-bindings.txt
>>   create mode 100644 Documentation/phy.txt
>>   create mode 100644 drivers/phy/Kconfig
>>   create mode 100644 drivers/phy/Makefile
>>   create mode 100644 drivers/phy/phy-core.c
>>   create mode 100644 include/linux/phy/phy.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-phy b/Documentation/ABI/testing/sysfs-class-phy
>> new file mode 100644
>> index 0000000..b735467
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-phy
>> @@ -0,0 +1,15 @@
>> +What:		/sys/class/phy/<phy>/label
>> +Date:		Apr 2013
>> +KernelVersion:	3.10
>> +Contact:	Kishon Vijay Abraham I <kishon@...com>
>> +Description:
>> +		This is a read-only file for getting the label of the phy.
>> +
>> +What:		/sys/class/phy/<phy>/phy_bind
>> +Date:		Apr 2013
>> +KernelVersion:	3.10
>> +Contact:	Kishon Vijay Abraham I <kishon@...com>
>> +Description:
>> +		This is a read-only file for reading the phy binding
>> +		information. It contains the device name of the controller,
>> +		the index and the device name of the PHY in that order.
>> diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
>> new file mode 100644
>> index 0000000..e7b246a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
>> @@ -0,0 +1,67 @@
>> +This document explains only the dt data binding. For general information about
>> +PHY subsystem refer Documentation/phy.txt
>> +
>> +PHY device node
>> +===============
>> +
>> +Required Properties:
>> +#phy-cells:	Number of cells in a PHY specifier;  The meaning of all those
>> +		cells is defined by the binding for the phy node. The PHY
>> +		provider can use the values in cells to find the appropriate
>> +		PHY.
>> +
>> +For example:
>> +
>> +phys: phy {
>> +    compatible = "xxx";
>> +    reg = <...>;
>> +    .
>> +    .
>> +    #phy-cells = <1>;
>> +    .
>> +    .
>> +};
>> +
>> +That node describes an IP block that implements 2 different PHYs. In order to
>> +differentiate between these 2 PHYs, an additonal specifier should be given
>> +while trying to get a reference to it.
>> +
>> +PHY user node
>> +=============
>> +
>> +Required Properties:
>> +phys : the phandle for the PHY device (used by the PHY subsystem)
>> +
>> +Optional properties:
>> +phy-names : the names of the PHY corresponding to the PHYs present in the
>> +	    *phys* phandle
>> +
>> +Example 1:
>> +usb1: usb_otg_ss@xxx {
>> +    compatible = "xxx";
>> +    reg = <xxx>;
>> +    .
>> +    .
>> +    phys = <&usb2_phy>, <&usb3_phy>;
>> +    phy-names = "usb2phy", "usb3phy";
>> +    .
>> +    .
>> +};
>> +
>> +This node represents a controller that uses two PHYs one for usb2 and one for
>> +usb3.
>> +
>> +Example 2:
>> +usb2: usb_otg_ss@xxx {
>> +    compatible = "xxx";
>> +    reg = <xxx>;
>> +    .
>> +    .
>> +    phys = <&phys 1>;
>> +    .
>> +    .
>> +};
>> +
>> +This node represents a controller that uses one of the PHYs which is defined
>> +previously. Note that the phy handle has an additional specifier "1" to
>> +differentiate between the two PHYs.
>> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
>> new file mode 100644
>> index 0000000..7785ec0
>> --- /dev/null
>> +++ b/Documentation/phy.txt
>> @@ -0,0 +1,113 @@
>> +			    PHY SUBSYSTEM
>> +		  Kishon Vijay Abraham I <kishon@...com>
>> +
>> +This document explains the Generic PHY Framework along with the APIs provided,
>> +and how-to-use.
>> +
>> +1. Introduction
>> +
>> +*PHY* is the abbreviation for physical layer. It is used to connect a device
>> +to the physical medium e.g., the USB controller has a PHY to provide functions
>> +such as serialization, de-serialization, encoding, decoding and is responsible
>> +for obtaining the required data transmission rate. Note that some USB
>> +controller has PHY functionality embedded into it and others use an external
>> +PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,
>> +SATA etc.
>> +
>> +The intention of creating this framework is to bring the phy drivers spread
>> +all over the Linux kernel to drivers/phy to increase code re-use and to
>> +increase code maintainability.
>> +
>> +This framework will be of use only to devices that uses external PHY (PHY
>> +functionality is not embedded within the controller).
>> +
>> +2. Creating the PHY
>> +
>> +The PHY driver should create the PHY in order for other peripheral controllers
>> +to make use of it. The PHY framework provides 2 APIs to create the PHY.
>> +
>> +struct phy *phy_create(struct device *dev, const char *label,
>> +	struct device_node *of_node, int type, struct phy_ops *ops,
>> +	void *priv);
>> +struct phy *devm_phy_create(struct device *dev, const char *label,
>> +	struct device_node *of_node, int type, struct phy_ops *ops,
>> +	void *priv);
>> +
>> +The PHY drivers can use one of the above 2 APIs to create the PHY by passing
>> +the device pointer, label, device node, type, phy ops and a driver data.
>> +phy_ops is a set of function pointers for performing PHY operations such as
>> +init, exit, suspend, resume, power_on and power_off.
>> +
>> +3. Binding the PHY to the controller
>> +
>> +The framework provides an API for binding the controller to the PHY in the
>> +case of non dt boot.
>> +
>> +struct phy_bind *phy_bind(const char *dev_name, int index,
>> +				const char *phy_dev_name);
>> +
>> +The API fills the phy_bind structure with the dev_name (device name of the
>> +controller), index and phy_dev_name (device name of the PHY). This will
>> +be used when the controller requests this phy. This API should be used by
>> +platform specific initialization code (board file).
>> +
>> +In the case of dt boot, the binding information should be added in the dt
>> +data of the controller.
>> +
>> +4. Getting a reference to the PHY
>> +
>> +Before the controller can make use of the PHY, it has to get a reference to
>> +it. This framework provides 6 APIs to get a reference to the PHY.
>> +
>> +struct phy *phy_get(struct device *dev, int index);
>> +struct phy *devm_phy_get(struct device *dev, int index);
>> +struct phy *of_phy_get(struct device *dev, const char *phandle, int index);
>> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle, int index);
>> +struct phy *of_phy_get_byname(struct device *dev, const char *string);
>> +struct phy *devm_of_phy_get_byname(struct device *dev, const char *string);
>> +
>> +phy_get and devm_phy_get can be used to get the PHY in non-dt boot. This API
>> +uses the binding information added using the phy_bind API to find and return
>> +the appropriate PHY. The only difference between the two APIs is that
>> +devm_phy_get associates the device with the PHY using devres on successful PHY
>> +get. On driver detach, release function is invoked on the the devres data and
>> +devres data is freed.
>> +
>> +of_phy_get and devm_of_phy_get can be used to get the PHY in dt boot. These
>> +APIs take the phandle and index to get a reference to the PHY. The only
>> +difference between the two APIs is that devm_of_phy_get associates the device
>> +with the PHY using devres on successful phy get. On driver detach, release
>> +function is invoked on the devres data and it is freed.
>> +
>> +of_phy_get_byname and devm_of_phy_get_byname can also be used to get the PHY
>> +in dt boot. It is same as the above API except that the user has to pass the
>> +phy name as filled in "phy-names" phandle in dt data and the framework will
>> +find the index and get the PHY.
>> +
>> +5. Releasing a reference to the PHY
>> +
>> +When the controller no longer needs the PHY, it has to release the reference
>> +to the PHY it has obtained using the APIs mentioned in the above section. The
>> +PHY framework provides 2 APIS to release a reference to the PHY.
>> +
>> +void phy_put(struct phy *phy);
>> +void devm_phy_put(struct device *dev, struct phy *phy);
>> +
>> +Both these APIs are used to release a reference to the PHY and devm_phy_put
>> +destroys the devres associated with this PHY.
>> +
>> +6. Destroying the PHY
>> +
>> +When the driver that created the PHY is unloaded, it should destroy the PHY it
>> +created using one of the following 2 APIs.
>> +
>> +void phy_destroy(struct phy *phy);
>> +void devm_phy_destroy(struct device *dev, struct phy *phy);
>> +
>> +Both these APIs destroys the PHY and devm_phy_destroy destroys the devres
>> +associated with this PHY.
>> +
>> +7. DeviceTree Binding
>> +
>> +The documentation for PHY dt binding can be found @
>> +Documentation/devicetree/bindings/phy/phy-bindings.txt
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 72b0843..f2674e7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3474,6 +3474,13 @@ S:	Maintained
>>   F:	include/asm-generic
>>   F:	include/uapi/asm-generic
>>
>> +GENERIC PHY FRAMEWORK
>> +M:	Kishon Vijay Abraham I <kishon@...com>
>> +L:	linux-kernel@...r.kernel.org
>> +S:	Supported
>> +F:	drivers/phy/
>> +F:	include/linux/phy/
>> +
>>   GENERIC UIO DRIVER FOR PCI DEVICES
>>   M:	"Michael S. Tsirkin" <mst@...hat.com>
>>   L:	kvm@...r.kernel.org
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 202fa6d..ad2c374a 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -162,4 +162,6 @@ source "drivers/irqchip/Kconfig"
>>
>>   source "drivers/ipack/Kconfig"
>>
>> +source "drivers/phy/Kconfig"
>> +
>>   endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index dce39a9..9da8321 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -45,6 +45,8 @@ obj-y				+= char/
>>   # gpu/ comes after char for AGP vs DRM startup
>>   obj-y				+= gpu/
>>
>> +obj-y				+= phy/
>> +
>>   obj-$(CONFIG_CONNECTOR)		+= connector/
>>
>>   # i810fb and intelfb depend on char/agp/
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> new file mode 100644
>> index 0000000..5f85909
>> --- /dev/null
>> +++ b/drivers/phy/Kconfig
>> @@ -0,0 +1,13 @@
>> +#
>> +# PHY
>> +#
>> +
>> +menuconfig GENERIC_PHY
>> +	tristate "PHY Subsystem"
>> +	help
>> +	  Generic PHY support.
>> +
>> +	  This framework is designed to provide a generic interface for PHY
>> +	  devices present in the kernel. This layer will have the generic
>> +	  API by which phy drivers can create PHY using the phy framework and
>> +	  phy users can obtain reference to the PHY.
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> new file mode 100644
>> index 0000000..9e9560f
>> --- /dev/null
>> +++ b/drivers/phy/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# Makefile for the phy drivers.
>> +#
>> +
>> +obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> new file mode 100644
>> index 0000000..1d753f2
>> --- /dev/null
>> +++ b/drivers/phy/phy-core.c
>> @@ -0,0 +1,616 @@
>> +/*
>> + * phy-core.c  --  Generic Phy framework.
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon@...com>
>> + *
>> + * 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, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +
>> +static struct class *phy_class;
>> +static DEFINE_MUTEX(phy_bind_mutex);
>> +static LIST_HEAD(phy_bind_list);
>> +static int phy_core_init(void);
>> +
>> +static void devm_phy_release(struct device *dev, void *res)
>> +{
>> +	struct phy *phy = *(struct phy **)res;
>> +
>> +	phy_put(phy);
>> +}
>> +
>> +static void devm_phy_consume(struct device *dev, void *res)
>> +{
>> +	struct phy *phy = *(struct phy **)res;
>> +
>> +	phy_destroy(phy);
>> +}
>> +
>> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
>> +{
>> +	return res == match_data;
>> +}
>> +
>> +static struct phy *phy_lookup(struct device *dev, int index)
>> +{
>> +	struct phy_bind *phy_bind = NULL;
>> +
>> +	list_for_each_entry(phy_bind, &phy_bind_list, list) {
>> +		if (!(strcmp(phy_bind->dev_name, dev_name(dev)) &&
>> +				phy_bind->index == index)) {
>> +			if (phy_bind->phy)
>> +				return phy_bind->phy;
>> +			else
>> +				return ERR_PTR(-EPROBE_DEFER);
>> +		}
>> +	}
>> +
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static struct phy *of_phy_lookup(struct device_node *node)
>> +{
>> +	struct phy *phy;
>> +	struct device *dev;
>> +	struct class_dev_iter iter;
>> +
>> +	class_dev_iter_init(&iter, phy_class, NULL, NULL);
>> +	while ((dev = class_dev_iter_next(&iter))) {
>> +		phy = container_of(dev, struct phy, dev);
>
> it would look a bit better if you provided a to_phy() macro. Specially
> since this container_of() repeats multiple times in this file.

hmm.. ok.
>
>> +/**
>> + * phy_put() - release the PHY
>> + * @phy: the phy returned by phy_get()
>> + *
>> + * Releases a refcount the caller received from phy_get().
>> + */
>> +void phy_put(struct phy *phy)
>> +{
>
> I would rather:
>
> if (WARN(IS_ERR(phy), "invalid parameter\n"))
> 	return;
>
> module_put(phy->ops->owner);
> put_device(&phy->dev);
>
> that way we can catch users passing bogus pointers here. When PHY layer
> is disabled, you want to make this is no-op with a static inline in a
> header anyway.

yeah. Have that no-op in header file.
>
>> +struct phy *of_phy_xlate(struct phy *phy, struct of_phandle_args *args)
>> +{
>> +	return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(of_phy_xlate);
>
> so you get a PHY and just return it ? What gives ?? (maybe I skipped
> some of the discussion...)

hmm.. this is for the common case where the PHY provider implements only 
one PHY. And both phy provider and phy_instance is represented by struct 
phy *.

For the case where PHY provider implements multiple PHYs (here it will 
have a single dt node), the PHY provider will implement it's own version 
of of_xlate that takes *of_phandle_args* as argument and finds the 
appropriate PHY.
>
>> +struct phy *of_phy_get(struct device *dev, int index)
>> +{
>> +	int ret;
>> +	struct phy *phy = NULL;
>> +	struct phy_bind *phy_map = NULL;
>> +	struct of_phandle_args args;
>> +	struct device_node *node;
>> +
>> +	if (!dev->of_node) {
>> +		dev_dbg(dev, "device does not have a device node entry\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells",
>> +		index, &args);
>> +	if (ret) {
>> +		dev_dbg(dev, "failed to get phy in %s node\n",
>> +			dev->of_node->full_name);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	phy = of_phy_lookup(args.np);
>> +	if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
>> +		phy = ERR_PTR(-EPROBE_DEFER);
>> +		goto err0;
>> +	}
>> +
>> +	phy = phy->ops->of_xlate(phy, &args);
>
> alright, so of_xlate() is optional, am I right ? How about not

Not really. of_xlate is mandatory (it's even checked in phy_create). 
Either the PHY provider can implement it's own version or use the 
implementation above (by filling the function pointer).

> implementing the above and have a check for of_xlate() being a valid
> pointer here ?

Having the way it is actually mandates the PHY providers to always 
provide of_xlate which IMO is better since some PHY providers wont 
accidentally be using the default implementation.
>
>> +struct phy *phy_create(struct device *dev, const char *label,
>> +	struct device_node *of_node, int type, struct phy_ops *ops,
>> +	void *priv)
>> +{
>> +	int ret;
>> +	struct phy *phy;
>> +	struct phy_bind *phy_bind;
>> +	const char *devname = NULL;
>> +
>> +	if (!dev) {
>> +		dev_err(dev, "no device provided for PHY\n");
>
> I'd call this a WARN() or am I too pedantic? :-p
>
>> +	if (!ops || !ops->of_xlate || !priv) {
>> +		dev_err(dev, "no PHY ops/PHY data provided\n");
>
> likewise here.

hmm.. ok.
>
>> +		ret = -EINVAL;
>> +		goto err0;
>> +	}
>> +
>> +	if (!phy_class)
>> +		phy_core_init();
>
> why don't you setup the class on module_init ? Then this would be a
> terrible error condition here :-)

This is for the case where the PHY driver gets loaded before the PHY 
framework. I could have returned EPROBE_DEFER here instead I thought 
will have it this way.
>
>> +static struct device_attribute phy_dev_attrs[] = {
>> +	__ATTR(label, 0444, phy_name_show, NULL),
>> +	__ATTR(phy_bind, 0444, phy_bind_show, NULL),
>
> you could expose a human-readable 'type' string. BTW, how are you using
> type ? USB2/USB3/etc ? Have you considered our OMAP5 SerDes pair which

Actually not using *type* anywhere. Just used as an additional info 
about the PHY. It's actually not even enum. Maybe I can remove it 
completely.
> are currently for USB3 and SATA (and could just as easily be used for
> PCIe)

Yeah. Me and Balaji were planning to work on it for having a single 
driver to be used for all the above.
>
>> +static void phy_release(struct device *dev)
>> +{
>> +	struct phy *phy;
>> +
>> +	phy = container_of(dev, struct phy, dev);
>> +	dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
>
> how about dev_vdbg() ? I doubt anyone will be waiting for this
> message... Just a thought
>
>> +static int phy_core_init(void)
>> +{
>> +	if (phy_class)
>> +		return 0;
>
> Weird.. if you initialize the class here, why do you need to initialize
> it during phy_create() ?
>
> What's going on ? Also, module_init() will only be called once, why this
> if (phy_class) check ?

er.. for the case where phy driver is loaded before this PHY framework, 
phy_create would have already called phy_core_init to create the 
phy_class. So module_init() is not needed at all since we have already 
created the phy_class. I think this looks a bit hacky.
Either we can have EPROBE_DEFER in phy_create or have this module as 
subsys_initcall() like I had it before. I would actually prefer it to be 
subsys_initcall().

Thanks
Kishon
--
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