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 DMhelpGeneric PHY support.This framework is designed to provide a generic interface for PHYdevices.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 DMhelpGeneric PHY support in SPL.This framework is designed to provide a generic interface for PHYdevices.+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