From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Jacques Hiblot Date: Tue, 18 Apr 2017 15:32:22 +0200 Subject: [U-Boot] [PATCH 03/11] drivers: phy: add generic PHY framework In-Reply-To: References: <1492168089-15437-1-git-send-email-jjhiblot@ti.com> <1492168089-15437-4-git-send-email-jjhiblot@ti.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 15/04/2017 19:10, Simon Glass wrote: > Hi Jean-Jacques, > > On 14 April 2017 at 05:08, 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 >> --- >> 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 >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include >> +#include >> + >> +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. IMO 'phy' would be the best option. Unfortunately there are already tons of functions starting with 'phy_' and they're used for the ethernet phy. So I would propose to use 'phy' everywhere except for the API where 'generic_phy_' can be used to prefix the functions. What do you think ? >> + .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 >> + * >> + * 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()? I'll work on documenting this. Too bad that linux also lacks this documentation. The core idea is that probe() does not do much hardware-wise, it sets up everyting (irqs, mapping, clocks) but the actual initialization of the hardware is done in init(). Jean-Jacques >> + * @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 >