[U-Boot] [PATCH v2 03/10] drivers: phy: add generic PHY framework
Simon Glass
sjg at chromium.org
Sun Apr 9 19:27:33 UTC 2017
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?
>
> 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 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?
> + */
> +
> +#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 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).
> + */
> +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
More information about the U-Boot
mailing list