| Message ID | 20230208091529.31356-9-hayashi.kunihiko@socionext.com |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | usb: dwc3: Refactor dwc3-generic and apply to dwc3-uniphier | expand |
On 2/8/23 10:15, Kunihiko Hayashi wrote: [...] > +static int uniphier_usb3phy_init(struct phy *phy) > +{ > + struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev); > + int ret; > + > + ret = clk_enable_bulk(&priv->clks); > + if (ret) > + goto out_clk; This should be just 'return ret;' > + ret = reset_deassert_bulk(&priv->rsts); > + if (ret) > + goto out_rst; This should be goto out_rst, however ... > + return 0; > + > +out_rst: > + reset_release_bulk(&priv->rsts); > +out_clk: > + clk_release_bulk(&priv->clks); ... the out_rst: should only do: out_rst: clk_disable_bulk(); return ret out_clk part can be removed. > + return ret; > +} > + > +static int uniphier_usb3phy_probe(struct udevice *dev) > +{ > + struct uniphier_usb3phy_priv *priv = dev_get_priv(dev); > + int ret; > + > + ret = clk_get_bulk(dev, &priv->clks); > + if (ret) { > + if (ret != -ENOSYS && ret != -ENOENT) { This can be single line: if (ret && ret != -ENOSYS && ret != -ENOENT) return ret; > + printf("Failed to get clocks\n"); > + return ret; > + } > + } > + > + ret = reset_get_bulk(dev, &priv->rsts); > + if (ret) { > + if (ret != -ENOSYS && ret != -ENOENT) { Same here. > + printf("Failed to get resets\n"); > + clk_release_bulk(&priv->clks); Better use goto out_clk fail path. > + return ret; > + } > + } > + > + return 0; > +} > + > +static struct phy_ops uniphier_usb3phy_ops = { > + .init = uniphier_usb3phy_init, You should implement .exit callback too, that one should do these two steps: reset_assert_bulk() clk_disable_bulk() > +}; [...]
Hi Marek, On 2023/02/10 23:09, Marek Vasut wrote: > On 2/8/23 10:15, Kunihiko Hayashi wrote: > > [...] > >> +static int uniphier_usb3phy_init(struct phy *phy) >> +{ >> + struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev); >> + int ret; >> + >> + ret = clk_enable_bulk(&priv->clks); >> + if (ret) >> + goto out_clk; > > This should be just 'return ret;' > >> + ret = reset_deassert_bulk(&priv->rsts); >> + if (ret) >> + goto out_rst; > > This should be goto out_rst, however ... > >> + return 0; >> + >> +out_rst: >> + reset_release_bulk(&priv->rsts); >> +out_clk: >> + clk_release_bulk(&priv->clks); > > ... the out_rst: should only do: > > out_rst: > clk_disable_bulk(); > return ret > > out_clk part can be removed. I see. These operations are unpaired. I'll fix them. >> + return ret; >> +} >> + >> +static int uniphier_usb3phy_probe(struct udevice *dev) >> +{ >> + struct uniphier_usb3phy_priv *priv = dev_get_priv(dev); >> + int ret; >> + >> + ret = clk_get_bulk(dev, &priv->clks); >> + if (ret) { >> + if (ret != -ENOSYS && ret != -ENOENT) { > > This can be single line: > > if (ret && ret != -ENOSYS && ret != -ENOENT) > return ret; OK, This will be fixed. >> + printf("Failed to get clocks\n"); >> + return ret; >> + } >> + } >> + >> + ret = reset_get_bulk(dev, &priv->rsts); >> + if (ret) { >> + if (ret != -ENOSYS && ret != -ENOENT) { > > Same here. > >> + printf("Failed to get resets\n"); >> + clk_release_bulk(&priv->clks); > > Better use goto out_clk fail path. > >> + return ret; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static struct phy_ops uniphier_usb3phy_ops = { >> + .init = uniphier_usb3phy_init, > > You should implement .exit callback too, that one should do these two steps: > > reset_assert_bulk() > clk_disable_bulk() I think so, however, when I added .exit() and executed "usb stop;usb start", unfortunately the command got stuck. Currently uniphier clk doesn't support CLK_CCF and can't be nested. I think more changes are needed. Thank you, --- Best Regards Kunihiko Hayashi
On 2/13/23 04:08, Kunihiko Hayashi wrote: > Hi Marek, Hello Hayashi-san, > On 2023/02/10 23:09, Marek Vasut wrote: >> On 2/8/23 10:15, Kunihiko Hayashi wrote: >> >> [...] >> >>> +static int uniphier_usb3phy_init(struct phy *phy) >>> +{ >>> + struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev); >>> + int ret; >>> + >>> + ret = clk_enable_bulk(&priv->clks); >>> + if (ret) >>> + goto out_clk; >> >> This should be just 'return ret;' >> >>> + ret = reset_deassert_bulk(&priv->rsts); >>> + if (ret) >>> + goto out_rst; >> >> This should be goto out_rst, however ... >> >>> + return 0; >>> + >>> +out_rst: >>> + reset_release_bulk(&priv->rsts); >>> +out_clk: >>> + clk_release_bulk(&priv->clks); >> >> ... the out_rst: should only do: >> >> out_rst: >> clk_disable_bulk(); >> return ret >> >> out_clk part can be removed. > > I see. These operations are unpaired. > I'll fix them. Thank you >>> + return ret; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static struct phy_ops uniphier_usb3phy_ops = { >>> + .init = uniphier_usb3phy_init, >> >> You should implement .exit callback too, that one should do these two >> steps: >> >> reset_assert_bulk() >> clk_disable_bulk() > > I think so, however, when I added .exit() and executed "usb stop;usb > start", > unfortunately the command got stuck. > > Currently uniphier clk doesn't support CLK_CCF and can't be nested. > I think more changes are needed. Do you know where exactly the system hangs ? Do you expect to add the CCF support ? If so, then add at least a FIXME code comment that the exit callback must be implemented, so that this is not forgotten once CCF is in place. I don't want this series to grow much further into some "rework everything at once" thing.
Hi Marek, Sorry for late reply. On 2023/02/14 6:06, Marek Vasut wrote: > On 2/13/23 04:08, Kunihiko Hayashi wrote: >> Hi Marek, > > Hello Hayashi-san, > [...] >> I think so, however, when I added .exit() and executed "usb stop;usb >> start", >> unfortunately the command got stuck. >> >> Currently uniphier clk doesn't support CLK_CCF and can't be nested. >> I think more changes are needed. > > Do you know where exactly the system hangs ? Yes. The controller-reset driver controls the clock/reset for the glue in probe(). The phy driver controls the clock/reset for the glue and the phy in init(). There is an inconsistency. When submitting "usb stop", the phy driver disables all the clock/reset in exit(). When submitting "usb start" again, the controller-reset driver accesses the glue register without enabling the clock/reset, and the system hangs. > Do you expect to add the CCF support ? Yes, first I thought the clock needed the CCF support to count the clock function calls, but there is no support to count the reset function calls. I need another solution. Currently the phy driver controls all the clock/reset in init() and exit(), however, it should control the phy clock/reset only. > If so, then add at least a FIXME code comment that the exit callback > must be implemented, so that this is not forgotten once CCF is in place. > I don't want this series to grow much further into some "rework > everything at once" thing. As above, I should put the glue clock/reset into probe(), and the phy clock/reset into init() without using bulk function. Thank you, --- Best Regards Kunihiko Hayashi
On 2/16/23 17:14, Kunihiko Hayashi wrote: > Hi Marek, Hello Hayashi-san, > Sorry for late reply. > > On 2023/02/14 6:06, Marek Vasut wrote: >> On 2/13/23 04:08, Kunihiko Hayashi wrote: >>> Hi Marek, >> >> Hello Hayashi-san, >> > > [...] > >>> I think so, however, when I added .exit() and executed "usb stop;usb >>> start", >>> unfortunately the command got stuck. >>> >>> Currently uniphier clk doesn't support CLK_CCF and can't be nested. >>> I think more changes are needed. >> >> Do you know where exactly the system hangs ? > > Yes. > > The controller-reset driver controls the clock/reset for the glue > in probe(). The phy driver controls the clock/reset for the glue and > the phy in init(). There is an inconsistency. > > When submitting "usb stop", the phy driver disables all the clock/reset > in exit(). > > When submitting "usb start" again, the controller-reset driver accesses > the glue register without enabling the clock/reset, and the system hangs. Shouldn't the PHY enable its own set of clock, while the controller should only enable its own (different) set of clock ? If it is the same clock and there is no refcounting, then maybe what you should do for now is enable/disable all the clock in the controller driver only, and deal with the PHY once CCF with refcounting is in place ? >> Do you expect to add the CCF support ? > > Yes, first I thought the clock needed the CCF support to count the clock > function calls, but there is no support to count the reset function calls. > I need another solution. > > Currently the phy driver controls all the clock/reset in init() and exit(), > however, it should control the phy clock/reset only. If that's possible on this hardware, that would be good, let controller control its clock and PHY control its (different) clock. >> If so, then add at least a FIXME code comment that the exit callback >> must be implemented, so that this is not forgotten once CCF is in place. >> I don't want this series to grow much further into some "rework >> everything at once" thing. > > As above, I should put the glue clock/reset into probe(), > and the phy clock/reset into init() without using bulk function. OK
Hi Marek, On 2023/02/18 4:58, Marek Vasut wrote: > On 2/16/23 17:14, Kunihiko Hayashi wrote: >> Hi Marek, > > Hello Hayashi-san, > >> Sorry for late reply. >> >> On 2023/02/14 6:06, Marek Vasut wrote: >>> On 2/13/23 04:08, Kunihiko Hayashi wrote: >>>> Hi Marek, >>> >>> Hello Hayashi-san, >>> >> >> [...] >> >>>> I think so, however, when I added .exit() and executed "usb stop;usb >>>> start", >>>> unfortunately the command got stuck. >>>> >>>> Currently uniphier clk doesn't support CLK_CCF and can't be nested. >>>> I think more changes are needed. >>> >>> Do you know where exactly the system hangs ? >> >> Yes. >> >> The controller-reset driver controls the clock/reset for the glue >> in probe(). The phy driver controls the clock/reset for the glue and >> the phy in init(). There is an inconsistency. >> >> When submitting "usb stop", the phy driver disables all the clock/reset >> in exit(). >> >> When submitting "usb start" again, the controller-reset driver accesses >> the glue register without enabling the clock/reset, and the system hangs. > > Shouldn't the PHY enable its own set of clock, while the controller > should only enable its own (different) set of clock ? PHY has this own clock/reset, however, PHY is in the glue logic. So accessing to PHY also needs to enable the glue clock (same as the controller clock). > If it is the same clock and there is no refcounting, then maybe what you > should do for now is enable/disable all the clock in the controller > driver only, and deal with the PHY once CCF with refcounting is in place ? PHY needs the glue(controller) clock and the phy clock. After all, it's wrong to control all the clocks/resets at the same time. In this time, I don't event think about CCF (sorry to confuse you). >>> Do you expect to add the CCF support ? >> >> Yes, first I thought the clock needed the CCF support to count the clock >> function calls, but there is no support to count the reset function calls. >> I need another solution. >> >> Currently the phy driver controls all the clock/reset in init() and >> exit(), >> however, it should control the phy clock/reset only. > > If that's possible on this hardware, that would be good, let controller > control its clock and PHY control its (different) clock. Yes. I'll separate these controls. >>> If so, then add at least a FIXME code comment that the exit callback >>> must be implemented, so that this is not forgotten once CCF is in place. >>> I don't want this series to grow much further into some "rework >>> everything at once" thing. >> >> As above, I should put the glue clock/reset into probe(), >> and the phy clock/reset into init() without using bulk function. > > OK Thank you, --- Best Regards Kunihiko Hayashi
diff --git a/drivers/phy/socionext/Kconfig b/drivers/phy/socionext/Kconfig index bcd579e98ec1..de87d5b0109b 100644 --- a/drivers/phy/socionext/Kconfig +++ b/drivers/phy/socionext/Kconfig @@ -10,3 +10,11 @@ config PHY_UNIPHIER_PCIE help Enable this to support PHY implemented in PCIe controller on UniPhier SoCs. + +config PHY_UNIPHIER_USB3 + bool "UniPhier USB3 PHY driver" + depends on PHY && ARCH_UNIPHIER + imply REGMAP + help + Enable this to support PHY implemented in USB3 controller + on UniPhier SoCs. diff --git a/drivers/phy/socionext/Makefile b/drivers/phy/socionext/Makefile index 5484360b70ff..94d3aa68cfac 100644 --- a/drivers/phy/socionext/Makefile +++ b/drivers/phy/socionext/Makefile @@ -4,3 +4,4 @@ # obj-$(CONFIG_PHY_UNIPHIER_PCIE) += phy-uniphier-pcie.o +obj-$(CONFIG_PHY_UNIPHIER_USB3) += phy-uniphier-usb3.o diff --git a/drivers/phy/socionext/phy-uniphier-usb3.c b/drivers/phy/socionext/phy-uniphier-usb3.c new file mode 100644 index 000000000000..ec3774569287 --- /dev/null +++ b/drivers/phy/socionext/phy-uniphier-usb3.c @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * phy_uniphier_usb3.c - Socionext UniPhier Usb3 PHY driver + * Copyright 2019-2023 Socionext, Inc. + */ + +#include <common.h> +#include <dm.h> +#include <generic-phy.h> + +#include <clk.h> +#include <reset.h> + +struct uniphier_usb3phy_priv { + struct clk_bulk clks; + struct reset_ctl_bulk rsts; +}; + +static int uniphier_usb3phy_init(struct phy *phy) +{ + struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev); + int ret; + + ret = clk_enable_bulk(&priv->clks); + if (ret) + goto out_clk; + + ret = reset_deassert_bulk(&priv->rsts); + if (ret) + goto out_rst; + + return 0; + +out_rst: + reset_release_bulk(&priv->rsts); +out_clk: + clk_release_bulk(&priv->clks); + + return ret; +} + +static int uniphier_usb3phy_probe(struct udevice *dev) +{ + struct uniphier_usb3phy_priv *priv = dev_get_priv(dev); + int ret; + + ret = clk_get_bulk(dev, &priv->clks); + if (ret) { + if (ret != -ENOSYS && ret != -ENOENT) { + printf("Failed to get clocks\n"); + return ret; + } + } + + ret = reset_get_bulk(dev, &priv->rsts); + if (ret) { + if (ret != -ENOSYS && ret != -ENOENT) { + printf("Failed to get resets\n"); + clk_release_bulk(&priv->clks); + return ret; + } + } + + return 0; +} + +static struct phy_ops uniphier_usb3phy_ops = { + .init = uniphier_usb3phy_init, +}; + +static const struct udevice_id uniphier_usb3phy_ids[] = { + { .compatible = "socionext,uniphier-pro4-usb3-ssphy" }, + { .compatible = "socionext,uniphier-pro5-usb3-hsphy" }, + { .compatible = "socionext,uniphier-pro5-usb3-ssphy" }, + { .compatible = "socionext,uniphier-pxs2-usb3-hsphy" }, + { .compatible = "socionext,uniphier-pxs2-usb3-ssphy" }, + { .compatible = "socionext,uniphier-ld20-usb3-hsphy" }, + { .compatible = "socionext,uniphier-ld20-usb3-ssphy" }, + { .compatible = "socionext,uniphier-pxs3-usb3-hsphy" }, + { .compatible = "socionext,uniphier-pxs3-usb3-ssphy" }, + { .compatible = "socionext,uniphier-nx1-usb3-hsphy" }, + { .compatible = "socionext,uniphier-nx1-usb3-ssphy" }, + { } +}; + +U_BOOT_DRIVER(uniphier_usb3_phy) = { + .name = "uniphier-usb3-phy", + .id = UCLASS_PHY, + .of_match = uniphier_usb3phy_ids, + .ops = &uniphier_usb3phy_ops, + .probe = uniphier_usb3phy_probe, + .priv_auto = sizeof(struct uniphier_usb3phy_priv), +};
Add USB3 PHY driver support to control clocks and resets for the phy. Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- drivers/phy/socionext/Kconfig | 8 ++ drivers/phy/socionext/Makefile | 1 + drivers/phy/socionext/phy-uniphier-usb3.c | 93 +++++++++++++++++++++++ 3 files changed, 102 insertions(+) create mode 100644 drivers/phy/socionext/phy-uniphier-usb3.c