[U-Boot] [PATCH v2 03/10] drivers: phy: add generic PHY framework

[Simon Glass <sjg@chromium.org>]

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