[U-Boot] [PATCH 03/11] drivers: phy: add generic PHY framework

[Jean-Jacques Hiblot <jjhiblot@ti.com>]

On 15/04/2017 19:10, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On 14 April 2017 at 05:08, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> The PHY framework provides a set of APIs to control a PHY. This API is
>> derived from the linux version of the generic PHY framework.
>> Currently the API supports init(), deinit(), power_on, power_off() and
>> reset(). The framework provides a way to get a reference to a phy from the
>> device-tree.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>   drivers/Kconfig          |  2 ++
>>   drivers/Makefile         |  1 +
>>   drivers/phy/Kconfig      | 32 ++++++++++++++++++++++++
>>   drivers/phy/Makefile     |  2 ++
>>   drivers/phy/phy-uclass.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/dm/uclass-id.h   |  1 +
>>   include/generic-phy.h    | 36 +++++++++++++++++++++++++++
>>   7 files changed, 138 insertions(+)
>>   create mode 100644 drivers/phy/Kconfig
>>   create mode 100644 drivers/phy/Makefile
>>   create mode 100644 drivers/phy/phy-uclass.c
>>   create mode 100644 include/generic-phy.h
> I mostly have minor things at this point. Note that I've applied the
> patches from your series that I can, so please rebase on master.
>
> Also can you please:
> - check your version number (I think you might be up to v3 now)
> - include a change log with each patch (patman might help you)
> - rebase on u-boot-dm/master
>
> I'm sorry if this comes across as a bit pedantic. But you are creating
> a new uclass which I think will be quite important in U-Boot. I
> suspect it will be used by USB, perhaps Ethernet and other systems, so
> careful design and documentation is pretty important.
>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 0e5d97d..a90ceca 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -88,6 +88,8 @@ source "drivers/video/Kconfig"
>>
>>   source "drivers/watchdog/Kconfig"
>>
>> +source "drivers/phy/Kconfig"
>> +
>>   config PHYS_TO_BUS
>>          bool "Custom physical to bus address mapping"
>>          help
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 5d8baa5..0be0624 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_$(SPL_)CLK)        += clk/
>>   obj-$(CONFIG_$(SPL_)LED)       += led/
>>   obj-$(CONFIG_$(SPL_)PINCTRL)   += pinctrl/
>>   obj-$(CONFIG_$(SPL_)RAM)       += ram/
>> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy/
> Please maintain the ordering here
>
>>   ifdef CONFIG_SPL_BUILD
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> new file mode 100644
>> index 0000000..d66c9e3
>> --- /dev/null
>> +++ b/drivers/phy/Kconfig
>> @@ -0,0 +1,32 @@
>> +
>> +menu "PHY Subsystem"
>> +
>> +config GENERIC_PHY
>> +       bool "PHY Core"
>> +       depends on DM
>> +       help
>> +         Generic PHY support.
>> +
>> +         This framework is designed to provide a generic interface for PHY
> PHY means?
>
>> +         devices. PHYs are commonly used for high speed interfaces such as
>> +         SATA or PCIe.
> Please write out SATA and PCIe in full at least once. This is help so
> we should not assume much.
>
>> +         The API provides functions to initialize/deinitialize the
>> +         phy, power on/off the phy, and reset the phy. It's meant to be as
> Is it PHY or phy?
>
>> +         compatible as possible with the equivalent framework found in the
>> +         linux kernel.
>> +
>> +config SPL_GENERIC_PHY
>> +       bool "PHY Core in SPL"
>> +       depends on DM
>> +       help
>> +         Generic PHY support in SPL.
>> +
>> +         This framework is designed to provide a generic interface for PHY
>> +         devices. PHYs are commonly used for high speed interfaces such as
>> +         SATA or PCIe.
>> +         The API provides functions to initialize/deinitialize the
>> +         phy, power on/off the phy, and reset the phy. It's meant to be as
>> +         compatible as possible with the equivalent framework found in the
>> +         linux kernel.
>> +
>> +endmenu
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> new file mode 100644
>> index 0000000..b29a8b9
>> --- /dev/null
>> +++ b/drivers/phy/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy-uclass.o
>> +
>> diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
>> new file mode 100644
>> index 0000000..e15ed43
>> --- /dev/null
>> +++ b/drivers/phy/phy-uclass.c
>> @@ -0,0 +1,64 @@
>> +/*
>> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
>> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <generic-phy.h>
>> +
>> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string)
> Can you make this return an error code instead, and the device pointer
> as a parameter? This is how it is generally down in DM. See
> uclass_first_device() for example.
>
> Also I think you should drop the dm_ in the name. That prefix is used
> for core driver model functions, and DM versions of function that have
> a non-DM analogue.
>
> Also I think 'get' is too short. We normally use that for getting by
> index. How about generic_phy_get_by_name() or, phy_get_by_name() if
> you rename it?
>
>> +{
>> +       struct udevice *dev;
>> +
>> +       int rc = uclass_get_device_by_phandle(UCLASS_PHY, parent,
>> +                                          string, &dev);
>> +       if (rc) {
>> +               debug("unable to find generic_phy device (err: %d)\n", rc);
>> +               return ERR_PTR(rc);
>> +       }
>> +
>> +       return dev;
>> +}
>> +
>> +int generic_phy_init(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
> Please use a header-file macro to access ops. See clk-uclass.c for an example.
>
>> +
>> +       return (ops && ops->init) ? ops->init(dev) : 0;
> You don't need to check ops since it is invalid not to have one. So:
>
> return ops->init ? ops->init(dev) : 0;
>
>> +}
>> +
>> +int generic_phy_reset(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->reset) ? ops->reset(dev) : 0;
>> +}
>> +
>> +int generic_phy_exit(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->exit) ? ops->exit(dev) : 0;
>> +}
>> +
>> +int generic_phy_power_on(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->power_on) ? ops->power_on(dev) : 0;
>> +}
>> +
>> +int generic_phy_power_off(struct udevice *dev)
>> +{
>> +       struct generic_phy_ops const *ops = dev->driver->ops;
>> +
>> +       return (ops && ops->power_off) ? ops->power_off(dev) : 0;
>> +}
>> +
>> +UCLASS_DRIVER(generic_phy) = {
>> +       .id             = UCLASS_PHY,
> I would like the uclass name to match the header file and the uclass
> name. So if you are calling this generic_phy, then the uclass should
> be named this too. Same with the directory drivers/phy. If you want to
> rename it all to 'phy' then that is fine too.
IMO 'phy' would be the best option.
Unfortunately there are already tons of functions starting with 'phy_' 
and they're used for the ethernet phy. So I would propose to use 'phy' 
everywhere except for the API where 'generic_phy_' can be used to prefix 
the functions.
What do you think ?
>> +       .name           = "generic_phy",
>> +};
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index 8c92d0b..9d34a32 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -83,6 +83,7 @@ enum uclass_id {
>>          UCLASS_VIDEO,           /* Video or LCD device */
>>          UCLASS_VIDEO_BRIDGE,    /* Video bridge, e.g. DisplayPort to LVDS */
>>          UCLASS_VIDEO_CONSOLE,   /* Text console driver for video device */
>> +       UCLASS_PHY,             /* generic PHY device */
> Physical layer device? Can you make your comment a bit more useful?
>
>>          UCLASS_COUNT,
>>          UCLASS_INVALID = -1,
>> diff --git a/include/generic-phy.h b/include/generic-phy.h
>> new file mode 100644
>> index 0000000..24475f0
>> --- /dev/null
>> +++ b/include/generic-phy.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
>> + * Written by Jean-Jacques Hiblot  <jjhiblot@ti.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef __GENERIC_PHY_H
>> +#define __GENERIC_PHY_H
>> +
>> +/*
>> + * struct udevice_ops - set of function pointers for phy operations
>> + * @init: operation to be performed for initializing phy (optionnal)
> optional
>
> Can you please put these comments in front of each function in struct
> generic_phy_ops as is done with other uclasses? Your description
> should explain what the operation does. For example, what happens for
> init()? Since the phy has already been probed, what should you not do
> in probe() but do in init()?
I'll work on documenting this. Too bad that linux also lacks this 
documentation.
The core idea is that probe() does not do much hardware-wise, it sets up 
everyting (irqs, mapping, clocks) but the actual initialization of the 
hardware is done in init().

Jean-Jacques

>> + * @exit: operation to be performed while exiting (optionnal)
> exiting what? Please add some more detail.
>
>> + * @reset: reset the phy (optionnal).
>> + * @power_on: powering on the phy (optionnal)
>> + * @power_off: powering off the phy (optionnal)
> Are these optional because the phy might be powered on during one of
> the other operations? Otherwise it doesn't seem helpful to have the
> thing not ever power on.
>
>> + */
>> +struct generic_phy_ops {
>> +       int     (*init)(struct udevice *phy);
>> +       int     (*exit)(struct udevice *phy);
>> +       int     (*reset)(struct udevice *phy);
>> +       int     (*power_on)(struct udevice *phy);
>> +       int     (*power_off)(struct udevice *phy);
>> +};
>> +
>> +
>> +int generic_phy_init(struct udevice *phy);
> Here you need to repeat your comments from above - see other uclasses
> for an example.
>
>> +int generic_phy_reset(struct udevice *phy);
>> +int generic_phy_exit(struct udevice *phy);
>> +int generic_phy_power_on(struct udevice *phy);
>> +int generic_phy_power_off(struct udevice *phy);
>> +
>> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string);
> This function need a comment. Also instead of string can you think of
> a descriptive name, e.g. phy_name?
>
>> +
>> +#endif /*__GENERIC_PHY_H */
>> --
>> 1.9.1
>>
> Regards,
> Simon
>