| Message ID | 1587352883-8641-4-git-send-email-chunfeng.yun@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MediaTek xHCI host controller | expand |
On Mon, Apr 20, 2020 at 8:52 AM Chunfeng Yun <chunfeng.yun at mediatek.com> wrote: > > This patch adds a "bulk" API to the phy API in order to > get/enable/disable a group of phys associated with a device. > > The bulk API will avoid adding a copy of the same code to > manage a group of phys in drivers. > > Signed-off-by: Chunfeng Yun <chunfeng.yun at mediatek.com> > Reviewed-by: Weijie Gao <weijie.gao at mediatek.com> > --- > v6: add Reviewed-by Weijie > > v5: no changes > > v4: new patch > --- > drivers/phy/phy-uclass.c | 80 ++++++++++++++++++++++++++++++++++++++++ > include/generic-phy.h | 66 +++++++++++++++++++++++++++++++++ > 2 files changed, 146 insertions(+) > > diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c > index e201a90c8c..62580520ad 100644 > --- a/drivers/phy/phy-uclass.c > +++ b/drivers/phy/phy-uclass.c > @@ -6,6 +6,7 @@ > > #include <common.h> > #include <dm.h> > +#include <dm/devres.h> > #include <generic-phy.h> > > static inline struct phy_ops *phy_dev_ops(struct udevice *dev) > @@ -161,6 +162,85 @@ int generic_phy_power_off(struct phy *phy) > return ops->power_off ? ops->power_off(phy) : 0; > } > > +int generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk) > +{ > + int i, ret, count; > + > + bulk->count = 0; > + > + /* Return if no phy declared */ > + if (!dev_read_prop(dev, "phys", NULL)) > + return 0; > + > + count = dev_count_phandle_with_args(dev, "phys", "#phy-cells"); > + if (count < 1) > + return count; > + > + bulk->phys = devm_kcalloc(dev, count, sizeof(struct phy), GFP_KERNEL); > + if (!bulk->phys) > + return -ENOMEM; > + > + for (i = 0; i < count; i++) { > + ret = generic_phy_get_by_index(dev, i, &bulk->phys[i]); > + if (ret) { > + pr_err("Failed to get PHY%d for %s\n", i, dev->name); > + return ret; > + } > + bulk->count++; > + } > + > + return 0; > +} > + > +int generic_phy_enable_bulk(struct phy_bulk *bulk) > +{ > + struct phy *phys = bulk->phys; > + int count = bulk->count; > + int i, ret; > + > + for (i = 0; i < count; i++) { > + ret = generic_phy_init(&phys[i]); > + if (ret) { > + pr_err("Can't init PHY%d\n", i); > + goto phys_init_err; > + } > + } > + > + for (i = 0; i < count; i++) { > + ret = generic_phy_power_on(&phys[i]); > + if (ret) { > + pr_err("Can't power PHY%d\n", i); > + goto phys_poweron_err; > + } > + } Better to have bulk init, bulk power on separately since not all phy consumers will init, power on sequentially. > + > + return 0; > + > +phys_poweron_err: > + for (; i > 0; i--) > + generic_phy_power_off(&phys[i - 1]); > + > + i = count; > +phys_init_err: > + for (; i > 0; i--) > + generic_phy_exit(&phys[i - 1]); > + > + return ret; > +} > + > +int generic_phy_disable_bulk(struct phy_bulk *bulk) > +{ > + struct phy *phys = bulk->phys; > + int i, ret = 0; > + > + for (i = 0; i < bulk->count; i++) { > + ret |= generic_phy_power_off(&phys[i]); > + ret |= generic_phy_exit(&phys[i]); > + } Same, like above. Jagan.
On Sat, 2020-04-25 at 18:58 +0530, Jagan Teki wrote: > On Mon, Apr 20, 2020 at 8:52 AM Chunfeng Yun <chunfeng.yun at mediatek.com> wrote: > > > > This patch adds a "bulk" API to the phy API in order to > > get/enable/disable a group of phys associated with a device. > > > > The bulk API will avoid adding a copy of the same code to > > manage a group of phys in drivers. > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun at mediatek.com> > > Reviewed-by: Weijie Gao <weijie.gao at mediatek.com> > > --- > > v6: add Reviewed-by Weijie > > > > v5: no changes > > > > v4: new patch > > --- > > drivers/phy/phy-uclass.c | 80 ++++++++++++++++++++++++++++++++++++++++ > > include/generic-phy.h | 66 +++++++++++++++++++++++++++++++++ > > 2 files changed, 146 insertions(+) > > > > diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c > > index e201a90c8c..62580520ad 100644 > > --- a/drivers/phy/phy-uclass.c > > +++ b/drivers/phy/phy-uclass.c > > @@ -6,6 +6,7 @@ > > > > #include <common.h> > > #include <dm.h> > > +#include <dm/devres.h> > > #include <generic-phy.h> > > > > static inline struct phy_ops *phy_dev_ops(struct udevice *dev) > > @@ -161,6 +162,85 @@ int generic_phy_power_off(struct phy *phy) > > return ops->power_off ? ops->power_off(phy) : 0; > > } > > > > +int generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk) > > +{ > > + int i, ret, count; > > + > > + bulk->count = 0; > > + > > + /* Return if no phy declared */ > > + if (!dev_read_prop(dev, "phys", NULL)) > > + return 0; > > + > > + count = dev_count_phandle_with_args(dev, "phys", "#phy-cells"); > > + if (count < 1) > > + return count; > > + > > + bulk->phys = devm_kcalloc(dev, count, sizeof(struct phy), GFP_KERNEL); > > + if (!bulk->phys) > > + return -ENOMEM; > > + > > + for (i = 0; i < count; i++) { > > + ret = generic_phy_get_by_index(dev, i, &bulk->phys[i]); > > + if (ret) { > > + pr_err("Failed to get PHY%d for %s\n", i, dev->name); > > + return ret; > > + } > > + bulk->count++; > > + } > > + > > + return 0; > > +} > > + > > +int generic_phy_enable_bulk(struct phy_bulk *bulk) > > +{ > > + struct phy *phys = bulk->phys; > > + int count = bulk->count; > > + int i, ret; > > + > > + for (i = 0; i < count; i++) { > > + ret = generic_phy_init(&phys[i]); > > + if (ret) { > > + pr_err("Can't init PHY%d\n", i); > > + goto phys_init_err; > > + } > > + } > > + > > + for (i = 0; i < count; i++) { > > + ret = generic_phy_power_on(&phys[i]); > > + if (ret) { > > + pr_err("Can't power PHY%d\n", i); > > + goto phys_poweron_err; > > + } > > + } > > Better to have bulk init, bulk power on separately since not all phy > consumers will init, power on sequentially. Yes, It's will be flexible when provide bulk init/power-on, but currently a bulk-enable API is simpler way due to all cases use multi-phys do initialization and power-on sequentially, how about separating it until we really need it? > > > + > > + return 0; > > + > > +phys_poweron_err: > > + for (; i > 0; i--) > > + generic_phy_power_off(&phys[i - 1]); > > + > > + i = count; > > +phys_init_err: > > + for (; i > 0; i--) > > + generic_phy_exit(&phys[i - 1]); > > + > > + return ret; > > +} > > + > > +int generic_phy_disable_bulk(struct phy_bulk *bulk) > > +{ > > + struct phy *phys = bulk->phys; > > + int i, ret = 0; > > + > > + for (i = 0; i < bulk->count; i++) { > > + ret |= generic_phy_power_off(&phys[i]); > > + ret |= generic_phy_exit(&phys[i]); > > + } > > Same, like above. > > Jagan.
On Mon, Apr 27, 2020 at 7:47 AM Chunfeng Yun <chunfeng.yun at mediatek.com> wrote: > > On Sat, 2020-04-25 at 18:58 +0530, Jagan Teki wrote: > > On Mon, Apr 20, 2020 at 8:52 AM Chunfeng Yun <chunfeng.yun at mediatek.com> wrote: > > > > > > This patch adds a "bulk" API to the phy API in order to > > > get/enable/disable a group of phys associated with a device. > > > > > > The bulk API will avoid adding a copy of the same code to > > > manage a group of phys in drivers. > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun at mediatek.com> > > > Reviewed-by: Weijie Gao <weijie.gao at mediatek.com> > > > --- > > > v6: add Reviewed-by Weijie > > > > > > v5: no changes > > > > > > v4: new patch > > > --- > > > drivers/phy/phy-uclass.c | 80 ++++++++++++++++++++++++++++++++++++++++ > > > include/generic-phy.h | 66 +++++++++++++++++++++++++++++++++ > > > 2 files changed, 146 insertions(+) > > > > > > diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c > > > index e201a90c8c..62580520ad 100644 > > > --- a/drivers/phy/phy-uclass.c > > > +++ b/drivers/phy/phy-uclass.c > > > @@ -6,6 +6,7 @@ > > > > > > #include <common.h> > > > #include <dm.h> > > > +#include <dm/devres.h> > > > #include <generic-phy.h> > > > > > > static inline struct phy_ops *phy_dev_ops(struct udevice *dev) > > > @@ -161,6 +162,85 @@ int generic_phy_power_off(struct phy *phy) > > > return ops->power_off ? ops->power_off(phy) : 0; > > > } > > > > > > +int generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk) > > > +{ > > > + int i, ret, count; > > > + > > > + bulk->count = 0; > > > + > > > + /* Return if no phy declared */ > > > + if (!dev_read_prop(dev, "phys", NULL)) > > > + return 0; > > > + > > > + count = dev_count_phandle_with_args(dev, "phys", "#phy-cells"); > > > + if (count < 1) > > > + return count; > > > + > > > + bulk->phys = devm_kcalloc(dev, count, sizeof(struct phy), GFP_KERNEL); > > > + if (!bulk->phys) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < count; i++) { > > > + ret = generic_phy_get_by_index(dev, i, &bulk->phys[i]); > > > + if (ret) { > > > + pr_err("Failed to get PHY%d for %s\n", i, dev->name); > > > + return ret; > > > + } > > > + bulk->count++; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +int generic_phy_enable_bulk(struct phy_bulk *bulk) > > > +{ > > > + struct phy *phys = bulk->phys; > > > + int count = bulk->count; > > > + int i, ret; > > > + > > > + for (i = 0; i < count; i++) { > > > + ret = generic_phy_init(&phys[i]); > > > + if (ret) { > > > + pr_err("Can't init PHY%d\n", i); > > > + goto phys_init_err; > > > + } > > > + } > > > + > > > + for (i = 0; i < count; i++) { > > > + ret = generic_phy_power_on(&phys[i]); > > > + if (ret) { > > > + pr_err("Can't power PHY%d\n", i); > > > + goto phys_poweron_err; > > > + } > > > + } > > > > Better to have bulk init, bulk power on separately since not all phy > > consumers will init, power on sequentially. > Yes, It's will be flexible when provide bulk init/power-on, but > currently a bulk-enable API is simpler way due to all cases use > multi-phys do initialization and power-on sequentially, how about > separating it until we really need it? I think the best way is to create bulk init, bulk power on separately and use them with your bulk enable. This way it would satisfy both the use cases. Jagan.
On Wed, 2020-04-29 at 01:15 +0530, Jagan Teki wrote: > On Mon, Apr 27, 2020 at 7:47 AM Chunfeng Yun <chunfeng.yun at mediatek.com> wrote: > > > > On Sat, 2020-04-25 at 18:58 +0530, Jagan Teki wrote: > > > On Mon, Apr 20, 2020 at 8:52 AM Chunfeng Yun <chunfeng.yun at mediatek.com> wrote: > > > > > > > > This patch adds a "bulk" API to the phy API in order to > > > > get/enable/disable a group of phys associated with a device. > > > > > > > > The bulk API will avoid adding a copy of the same code to > > > > manage a group of phys in drivers. > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun at mediatek.com> > > > > Reviewed-by: Weijie Gao <weijie.gao at mediatek.com> > > > > --- > > > > v6: add Reviewed-by Weijie > > > > > > > > v5: no changes > > > > > > > > v4: new patch > > > > --- > > > > drivers/phy/phy-uclass.c | 80 ++++++++++++++++++++++++++++++++++++++++ > > > > include/generic-phy.h | 66 +++++++++++++++++++++++++++++++++ > > > > 2 files changed, 146 insertions(+) > > > > > > > > diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c > > > > index e201a90c8c..62580520ad 100644 > > > > --- a/drivers/phy/phy-uclass.c > > > > +++ b/drivers/phy/phy-uclass.c > > > > @@ -6,6 +6,7 @@ > > > > > > > > #include <common.h> > > > > #include <dm.h> > > > > +#include <dm/devres.h> > > > > #include <generic-phy.h> > > > > > > > > static inline struct phy_ops *phy_dev_ops(struct udevice *dev) > > > > @@ -161,6 +162,85 @@ int generic_phy_power_off(struct phy *phy) > > > > return ops->power_off ? ops->power_off(phy) : 0; > > > > } > > > > > > > > +int generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk) > > > > +{ > > > > + int i, ret, count; > > > > + > > > > + bulk->count = 0; > > > > + > > > > + /* Return if no phy declared */ > > > > + if (!dev_read_prop(dev, "phys", NULL)) > > > > + return 0; > > > > + > > > > + count = dev_count_phandle_with_args(dev, "phys", "#phy-cells"); > > > > + if (count < 1) > > > > + return count; > > > > + > > > > + bulk->phys = devm_kcalloc(dev, count, sizeof(struct phy), GFP_KERNEL); > > > > + if (!bulk->phys) > > > > + return -ENOMEM; > > > > + > > > > + for (i = 0; i < count; i++) { > > > > + ret = generic_phy_get_by_index(dev, i, &bulk->phys[i]); > > > > + if (ret) { > > > > + pr_err("Failed to get PHY%d for %s\n", i, dev->name); > > > > + return ret; > > > > + } > > > > + bulk->count++; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int generic_phy_enable_bulk(struct phy_bulk *bulk) > > > > +{ > > > > + struct phy *phys = bulk->phys; > > > > + int count = bulk->count; > > > > + int i, ret; > > > > + > > > > + for (i = 0; i < count; i++) { > > > > + ret = generic_phy_init(&phys[i]); > > > > + if (ret) { > > > > + pr_err("Can't init PHY%d\n", i); > > > > + goto phys_init_err; > > > > + } > > > > + } > > > > + > > > > + for (i = 0; i < count; i++) { > > > > + ret = generic_phy_power_on(&phys[i]); > > > > + if (ret) { > > > > + pr_err("Can't power PHY%d\n", i); > > > > + goto phys_poweron_err; > > > > + } > > > > + } > > > > > > Better to have bulk init, bulk power on separately since not all phy > > > consumers will init, power on sequentially. > > Yes, It's will be flexible when provide bulk init/power-on, but > > currently a bulk-enable API is simpler way due to all cases use > > multi-phys do initialization and power-on sequentially, how about > > separating it until we really need it? > > I think the best way is to create bulk init, bulk power on separately > and use them with your bulk enable. This way it would satisfy both the > use cases. Ok, will do it, thanks > > Jagan.
diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c index e201a90c8c..62580520ad 100644 --- a/drivers/phy/phy-uclass.c +++ b/drivers/phy/phy-uclass.c @@ -6,6 +6,7 @@ #include <common.h> #include <dm.h> +#include <dm/devres.h> #include <generic-phy.h> static inline struct phy_ops *phy_dev_ops(struct udevice *dev) @@ -161,6 +162,85 @@ int generic_phy_power_off(struct phy *phy) return ops->power_off ? ops->power_off(phy) : 0; } +int generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk) +{ + int i, ret, count; + + bulk->count = 0; + + /* Return if no phy declared */ + if (!dev_read_prop(dev, "phys", NULL)) + return 0; + + count = dev_count_phandle_with_args(dev, "phys", "#phy-cells"); + if (count < 1) + return count; + + bulk->phys = devm_kcalloc(dev, count, sizeof(struct phy), GFP_KERNEL); + if (!bulk->phys) + return -ENOMEM; + + for (i = 0; i < count; i++) { + ret = generic_phy_get_by_index(dev, i, &bulk->phys[i]); + if (ret) { + pr_err("Failed to get PHY%d for %s\n", i, dev->name); + return ret; + } + bulk->count++; + } + + return 0; +} + +int generic_phy_enable_bulk(struct phy_bulk *bulk) +{ + struct phy *phys = bulk->phys; + int count = bulk->count; + int i, ret; + + for (i = 0; i < count; i++) { + ret = generic_phy_init(&phys[i]); + if (ret) { + pr_err("Can't init PHY%d\n", i); + goto phys_init_err; + } + } + + for (i = 0; i < count; i++) { + ret = generic_phy_power_on(&phys[i]); + if (ret) { + pr_err("Can't power PHY%d\n", i); + goto phys_poweron_err; + } + } + + return 0; + +phys_poweron_err: + for (; i > 0; i--) + generic_phy_power_off(&phys[i - 1]); + + i = count; +phys_init_err: + for (; i > 0; i--) + generic_phy_exit(&phys[i - 1]); + + return ret; +} + +int generic_phy_disable_bulk(struct phy_bulk *bulk) +{ + struct phy *phys = bulk->phys; + int i, ret = 0; + + for (i = 0; i < bulk->count; i++) { + ret |= generic_phy_power_off(&phys[i]); + ret |= generic_phy_exit(&phys[i]); + } + + return ret; +} + UCLASS_DRIVER(phy) = { .id = UCLASS_PHY, .name = "phy", diff --git a/include/generic-phy.h b/include/generic-phy.h index 95caf58341..55395a50eb 100644 --- a/include/generic-phy.h +++ b/include/generic-phy.h @@ -122,6 +122,23 @@ struct phy_ops { int (*power_off)(struct phy *phy); }; +/** + * struct phy_bulk - A handle to (allowing control of) a bulk of phys. + * + * Consumers provide storage for the phy bulk. The content of the structure is + * managed solely by the phy API. A phy bulk struct is initialized + * by "get"ing the phy bulk struct. + * The phy bulk struct is passed to all other bulk phy APIs to apply + * the API to all the phy in the bulk struct. + * + * @phys: An array of phy handles. + * @count: The number of phy handles in the phys array. + */ +struct phy_bulk { + struct phy *phys; + unsigned int count; +}; + #ifdef CONFIG_PHY /** @@ -221,6 +238,39 @@ int generic_phy_get_by_index(struct udevice *user, int index, int generic_phy_get_by_name(struct udevice *user, const char *phy_name, struct phy *phy); +/** + * generic_phy_get_bulk - Get all phys of a device. + * + * This looks up and gets all phys of the consumer device; each device is + * assumed to have n phys associated with it somehow, and this function finds + * and gets all of them in a separate structure. + * + * @dev: The consumer device. + * @bulk A pointer to a phy bulk struct to initialize. + * @return 0 if OK, or a negative error code. + */ +int generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk); + +/** + * generic_phy_enable_bulk() - Enable (init and power on) all phys in a phy + * bulk struct. + * + * @bulk: A phy bulk struct that was previously successfully requested + * by generic_phy_get_bulk(). + * @return 0 if OK, or negative error code. + */ +int generic_phy_enable_bulk(struct phy_bulk *bulk); + +/** + * generic_phy_disable_bulk() - Enable (power off and exit) all phys in a phy + * bulk struct. + * + * @bulk: A phy bulk struct that was previously successfully requested + * by generic_phy_get_bulk(). + * @return 0 if OK, or negative error code. + */ +int generic_phy_disable_bulk(struct phy_bulk *bulk); + #else /* CONFIG_PHY */ static inline int generic_phy_init(struct phy *phy) @@ -260,6 +310,22 @@ static inline int generic_phy_get_by_name(struct udevice *user, const char *phy_ return 0; } +static inline int +generic_phy_get_bulk(struct udevice *dev, struct phy_bulk *bulk) +{ + return 0; +} + +static inline int generic_phy_enable_bulk(struct phy_bulk *bulk) +{ + return 0; +} + +static inline int generic_phy_disable_bulk(struct phy_bulk *bulk) +{ + return 0; +} + #endif /* CONFIG_PHY */ /**