[U-Boot] [PATCH 03/11] drivers: phy: add generic PHY framework
Simon Glass
sjg at chromium.org
Sat Apr 15 17:10:10 UTC 2017
Hi Jean-Jacques,
On 14 April 2017 at 05:08, 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>
> ---
> drivers/Kconfig | 2 ++
> drivers/Makefile | 1 +
> drivers/phy/Kconfig | 32 ++++++++++++++++++++++++
> drivers/phy/Makefile | 2 ++
> drivers/phy/phy-uclass.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/dm/uclass-id.h | 1 +
> include/generic-phy.h | 36 +++++++++++++++++++++++++++
> 7 files changed, 138 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
I mostly have minor things at this point. Note that I've applied the
patches from your series that I can, so please rebase on master.
Also can you please:
- check your version number (I think you might be up to v3 now)
- include a change log with each patch (patman might help you)
- rebase on u-boot-dm/master
I'm sorry if this comes across as a bit pedantic. But you are creating
a new uclass which I think will be quite important in U-Boot. I
suspect it will be used by USB, perhaps Ethernet and other systems, so
careful design and documentation is pretty important.
>
> 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..0be0624 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_$(SPL_)CLK) += clk/
> obj-$(CONFIG_$(SPL_)LED) += led/
> obj-$(CONFIG_$(SPL_)PINCTRL) += pinctrl/
> obj-$(CONFIG_$(SPL_)RAM) += ram/
> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy/
Please maintain the ordering here
>
> ifdef CONFIG_SPL_BUILD
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> new file mode 100644
> index 0000000..d66c9e3
> --- /dev/null
> +++ b/drivers/phy/Kconfig
> @@ -0,0 +1,32 @@
> +
> +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
PHY means?
> + devices. PHYs are commonly used for high speed interfaces such as
> + SATA or PCIe.
Please write out SATA and PCIe in full at least once. This is help so
we should not assume much.
> + The API provides functions to initialize/deinitialize the
> + phy, power on/off the phy, and reset the phy. It's meant to be as
Is it PHY or phy?
> + compatible as possible with the equivalent framework found in the
> + linux kernel.
> +
> +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. PHYs are commonly used for high speed interfaces such as
> + SATA or PCIe.
> + The API provides functions to initialize/deinitialize the
> + phy, power on/off the phy, and reset the phy. It's meant to be as
> + compatible as possible with the equivalent framework found in the
> + linux kernel.
> +
> +endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> new file mode 100644
> index 0000000..b29a8b9
> --- /dev/null
> +++ b/drivers/phy/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_$(SPL_)GENERIC_PHY) += phy-uclass.o
> +
> diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
> new file mode 100644
> index 0000000..e15ed43
> --- /dev/null
> +++ b/drivers/phy/phy-uclass.c
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
> + * Written by Jean-Jacques Hiblot <jjhiblot at ti.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <generic-phy.h>
> +
> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string)
Can you make this return an error code instead, and the device pointer
as a parameter? This is how it is generally down in DM. See
uclass_first_device() for example.
Also I think you should drop the dm_ in the name. That prefix is used
for core driver model functions, and DM versions of function that have
a non-DM analogue.
Also I think 'get' is too short. We normally use that for getting by
index. How about generic_phy_get_by_name() or, phy_get_by_name() if
you rename it?
> +{
> + struct udevice *dev;
> +
> + int rc = uclass_get_device_by_phandle(UCLASS_PHY, parent,
> + string, &dev);
> + if (rc) {
> + debug("unable to find generic_phy device (err: %d)\n", rc);
> + return ERR_PTR(rc);
> + }
> +
> + return dev;
> +}
> +
> +int generic_phy_init(struct udevice *dev)
> +{
> + struct generic_phy_ops const *ops = dev->driver->ops;
Please use a header-file macro to access ops. See clk-uclass.c for an example.
> +
> + return (ops && ops->init) ? ops->init(dev) : 0;
You don't need to check ops since it is invalid not to have one. So:
return ops->init ? ops->init(dev) : 0;
> +}
> +
> +int generic_phy_reset(struct udevice *dev)
> +{
> + struct generic_phy_ops const *ops = dev->driver->ops;
> +
> + return (ops && ops->reset) ? ops->reset(dev) : 0;
> +}
> +
> +int generic_phy_exit(struct udevice *dev)
> +{
> + struct generic_phy_ops const *ops = dev->driver->ops;
> +
> + return (ops && ops->exit) ? ops->exit(dev) : 0;
> +}
> +
> +int generic_phy_power_on(struct udevice *dev)
> +{
> + struct generic_phy_ops const *ops = dev->driver->ops;
> +
> + return (ops && ops->power_on) ? ops->power_on(dev) : 0;
> +}
> +
> +int generic_phy_power_off(struct udevice *dev)
> +{
> + struct generic_phy_ops const *ops = dev->driver->ops;
> +
> + return (ops && ops->power_off) ? ops->power_off(dev) : 0;
> +}
> +
> +UCLASS_DRIVER(generic_phy) = {
> + .id = UCLASS_PHY,
I would like the uclass name to match the header file and the uclass
name. So if you are calling this generic_phy, then the uclass should
be named this too. Same with the directory drivers/phy. If you want to
rename it all to 'phy' then that is fine too.
> + .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 */
Physical layer device? Can you make your comment a bit more useful?
>
> UCLASS_COUNT,
> UCLASS_INVALID = -1,
> diff --git a/include/generic-phy.h b/include/generic-phy.h
> new file mode 100644
> index 0000000..24475f0
> --- /dev/null
> +++ b/include/generic-phy.h
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
> + * Written by Jean-Jacques Hiblot <jjhiblot at ti.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef __GENERIC_PHY_H
> +#define __GENERIC_PHY_H
> +
> +/*
> + * struct udevice_ops - set of function pointers for phy operations
> + * @init: operation to be performed for initializing phy (optionnal)
optional
Can you please put these comments in front of each function in struct
generic_phy_ops as is done with other uclasses? Your description
should explain what the operation does. For example, what happens for
init()? Since the phy has already been probed, what should you not do
in probe() but do in init()?
> + * @exit: operation to be performed while exiting (optionnal)
exiting what? Please add some more detail.
> + * @reset: reset the phy (optionnal).
> + * @power_on: powering on the phy (optionnal)
> + * @power_off: powering off the phy (optionnal)
Are these optional because the phy might be powered on during one of
the other operations? Otherwise it doesn't seem helpful to have the
thing not ever power on.
> + */
> +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 udevice *phy);
Here you need to repeat your comments from above - see other uclasses
for an example.
> +int generic_phy_reset(struct udevice *phy);
> +int generic_phy_exit(struct udevice *phy);
> +int generic_phy_power_on(struct udevice *phy);
> +int generic_phy_power_off(struct udevice *phy);
> +
> +struct udevice *dm_generic_phy_get(struct udevice *parent, const char *string);
This function need a comment. Also instead of string can you think of
a descriptive name, e.g. phy_name?
> +
> +#endif /*__GENERIC_PHY_H */
> --
> 1.9.1
>
Regards,
Simon
More information about the U-Boot
mailing list