From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Jacques Hiblot Date: Thu, 13 Apr 2017 16:17:28 +0200 Subject: [U-Boot] [PATCH v2 03/10] drivers: phy: add generic PHY framework In-Reply-To: References: <1491565329-20267-1-git-send-email-jjhiblot@ti.com> <1491565329-20267-4-git-send-email-jjhiblot@ti.com> Message-ID: <3a7735db-fa6f-f130-4e8f-a35412fc7086@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 09/04/2017 21:27, Simon Glass wrote: > Hi, > > On 7 April 2017 at 05:42, 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 >> --- >> 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 >> + * >> + * 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 >> +#include >> +#include >> + >> +#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 >> + * >> + * 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 >