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

[Simon Glass]

Hi Jean-Jacques,

On 13 April 2017 at 08:17, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
>
>
> On 09/04/2017 21:27, Simon Glass wrote:
>>
>> Hi,
>>
>> On 7 April 2017 at 05:42, Jean-Jacques Hiblot <jjhiblot at 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 at 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.

>
>
>>
>>> +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