[U-Boot] [PATCH v2 03/10] drivers: phy: add generic PHY framework
Jean-Jacques Hiblot
jjhiblot at ti.com
Thu Apr 13 14:17:28 UTC 2017
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
>
>> 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).
>
>> + bool "PHY Core"
>> + depends on DM
>> + help
>> + Generic PHY support.
>> +
>> + This framework is designed to provide a generic interface for PHY
>> + devices.
> Could you given a few examples of PHY devices and the types of
> operations you can perform on them.
OK
>
>> +
>> +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.
>> +
>> +endmenu
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> new file mode 100644
>> index 0000000..ccd15ed
>> --- /dev/null
>> +++ b/drivers/phy/Makefile
>> @@ -0,0 +1,5 @@
>> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy-uclass.o
>> +
>> +ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),)
>> +obj-y += marvell/
>> +endif
>> diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
>> new file mode 100644
>> index 0000000..4d1584d
>> --- /dev/null
>> +++ b/drivers/phy/phy-uclass.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
>> + * Written by Jean-Jacques Hiblot <jjhiblot at ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
> SPDX?
OK
>
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <generic-phy.h>
>> +
>> +#define get_ops(dev) ((struct generic_phy_ops *)(dev)->driver->ops)
>> +
>> +#define generic_phy_to_dev(x) ((struct udevice *)(x))
>> +#define dev_to_generic_phy(x) ((struct generic_phy *)(x))
>> +
>> +struct generic_phy *dm_generic_phy_get(struct udevice *dev, const char *string)
>> +{
>> + struct udevice *generic_phy_dev;
> dev is a shorter name :-)
indeed
>
>> +
>> + int rc = uclass_get_device_by_phandle(UCLASS_PHY, dev,
>> + string, &generic_phy_dev);
>> + if (rc) {
>> + error("unable to find generic_phy device %d\n", rc);
>> + return ERR_PTR(rc);
>> + }
>> + return dev_to_generic_phy(generic_phy_dev);
>> +}
>> +
>> +int generic_phy_init(struct generic_phy *generic_phy)
>> +{
>> + struct udevice *dev = generic_phy_to_dev(generic_phy);
>> + struct generic_phy_ops *ops = get_ops(dev);
>> +
>> + return (ops && ops->init) ? ops->init(dev) : 0;
>> +}
>> +
>> +int generic_phy_reset(struct generic_phy *generic_phy)
>> +{
>> + struct udevice *dev = generic_phy_to_dev(generic_phy);
>> + struct generic_phy_ops *ops = get_ops(dev);
>> +
>> + return (ops && ops->reset) ? ops->reset(dev) : 0;
>> +}
>> +
>> +int generic_phy_exit(struct generic_phy *generic_phy)
>> +{
>> + struct udevice *dev = generic_phy_to_dev(generic_phy);
>> + struct generic_phy_ops *ops = get_ops(dev);
>> +
>> + return (ops && ops->exit) ? ops->exit(dev) : 0;
>> +}
>> +
>> +int generic_phy_power_on(struct generic_phy *generic_phy)
>> +{
>> + struct udevice *dev = generic_phy_to_dev(generic_phy);
>> + struct generic_phy_ops *ops = get_ops(dev);
>> +
>> + return (ops && ops->power_on) ? ops->power_on(dev) : 0;
>> +}
>> +
>> +int generic_phy_power_off(struct generic_phy *generic_phy)
>> +{
>> + struct udevice *dev = generic_phy_to_dev(generic_phy);
>> + struct generic_phy_ops *ops = get_ops(dev);
>> +
>> + return (ops && ops->power_off) ? ops->power_off(dev) : 0;
>> +}
>> +
> Drop 2 extra blank ilnes
>
>> +
>> +
>> +UCLASS_DRIVER(simple_generic_phy) = {
> remove the word 'simple' ?
OK
>
>> + .id = UCLASS_PHY,
>> + .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 */
>>
>> UCLASS_COUNT,
>> UCLASS_INVALID = -1,
>> diff --git a/include/generic-phy.h b/include/generic-phy.h
>> new file mode 100644
>> index 0000000..f02e9ce
>> --- /dev/null
>> +++ b/include/generic-phy.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
>> + * Written by Jean-Jacques Hiblot <jjhiblot at ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __GENERIC_PHY_H
>> +#define __GENERIC_PHY_H
>> +
>> +struct generic_phy;
>> +/*
>> + * struct generic_phy_ops - set of function pointers for phy operations
>> + * @init: operation to be performed for initializing phy
>> + * @exit: operation to be performed while exiting
>> + * @power_on: powering on the phy
>> + * @power_off: powering off the phy
> Need to mention that these are all optional (from what I can tell above).
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.
>
>> +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
>
More information about the U-Boot
mailing list