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

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

On 14/04/2017 12:36, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On 13 April 2017 at 08:17, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>
>> On 09/04/2017 21:27, Simon Glass wrote:
>>> Hi,
>>>
>>> On 7 April 2017 at 05:42, 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>
>>>> ---
>>>>    Makefile                 |  1 +
>>>>    drivers/Kconfig          |  2 ++
>>>>    drivers/Makefile         |  1 +
>>>>    drivers/phy/Kconfig      | 22 ++++++++++++++
>>>>    drivers/phy/Makefile     |  5 ++++
>>>>    drivers/phy/phy-uclass.c | 77
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/dm/uclass-id.h   |  1 +
>>>>    include/generic-phy.h    | 38 ++++++++++++++++++++++++
>>>>    8 files changed, 147 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
>>> Can you please create a sandbox driver and a test?
>> Sure. I'll add something. It'll be pretty basic though
> Basic is fine. It needs to create a device or two and call some methods.
>
>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index 2638acf..06454ce 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -650,6 +650,7 @@ libs-y += fs/
>>>>    libs-y += net/
>>>>    libs-y += disk/
>>>>    libs-y += drivers/
>>>> +libs-y += drivers/phy/
>>> Could this go in drivers/Makefile?
>> OK
>>
>>>>    libs-y += drivers/dma/
>>>>    libs-y += drivers/gpio/
>>>>    libs-y += drivers/i2c/
>>>> 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..4656509 100644
>>>> --- a/drivers/Makefile
>>>> +++ b/drivers/Makefile
>>>> @@ -47,6 +47,7 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
>>>>    obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
>>>>    obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
>>>>    obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
>>>> +obj-$(CONFIG_SPL_GENERIC_PHY) += phy/
>>>>    endif
>>>>
>>>>    ifdef CONFIG_TPL_BUILD
>>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>>> new file mode 100644
>>>> index 0000000..b6fed9e
>>>> --- /dev/null
>>>> +++ b/drivers/phy/Kconfig
>>>> @@ -0,0 +1,22 @@
>>>> +
>>>> +menu "PHY Subsystem"
>>>> +
>>>> +config GENERIC_PHY
>>> Just a question: do you think we need the word GENERIC in this config?
>>> I'm OK with it, but wonder if CONFIG_PHY would be enough?
>> GENERIC_PHY is the name of the config option in the kernel and the functions
>> are also prefixed with generic_phy_.
>> BTW the functions in linux are not prefixed with generic_phy_ but only phy_,
>> but in the case of u-boot  a lot of phy_xxx() functions already exist and
>> are not necessarily static (like phy_reset() for ther ethernet phy).
> OK.
> [..]
>
>>>> + */
>>>> +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 generic_phy *phy);
>>> Why do these not use struct udevice?
>> It's quite easy for the PHY driver to get its internal data structure from
>> the struct udevice*.
>> I could also have passed struct generic_phy * but it adds another
>> translation that I don't think is necessary.
> I'd like to change that for consistency. A uclass API is supposed to
> take a struct udevice * rather than anything else. It is confusing if
> one uclass does this differently. The translation is cheap and some
> users will have a struct udevice * readily available..
Yes I eventually figured this out while working on the unit tests v3. 
This has been changed.

Jean-Jacques
>
>>
>>>> +int generic_phy_reset(struct generic_phy *phy);
>>>> +int generic_phy_exit(struct generic_phy *phy);
>>>> +int generic_phy_power_on(struct generic_phy *phy);
>>>> +int generic_phy_power_off(struct generic_phy *phy);
>>>> +
>>>> +struct generic_phy *dm_generic_phy_get(struct udevice *dev, const char
>>>> *string);
>>>> +
>>>> +#endif /*__GENERIC_PHY_H */
>>>> --
>>>> 1.9.1
>>>>
>>> Regards,
>>> Simon
>>>
> Regards,
> Simon
>