| Message ID | 1491565329-20267-4-git-send-email-jjhiblot@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
On Fri, Apr 07, 2017 at 01:42:02PM +0200, Jean-Jacques Hiblot 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> Reviewed-by: Tom Rini <trini@konsulko.com>
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? > > 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? > 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? > + 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. > + > +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@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? > + */ > + > +#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 :-) > + > + 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' ? > + .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@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). > + */ > +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? > +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
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 > >> 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@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@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 >
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. > > >> >>> +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
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 >
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/ 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 + bool "PHY Core" + depends on DM + help + Generic PHY support. + + This framework is designed to provide a generic interface for PHY + devices. + +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@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. + */ + +#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; + + 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; +} + + + +UCLASS_DRIVER(simple_generic_phy) = { + .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@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 + */ +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); +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 */
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