| Message ID | fa38a6511de22a2055acc96929bfe5705696a173.1685818269.git.xdrudis@tinet.cat |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2. | expand |
On 6/4/23 10:13, Xavier Drudis Ferran wrote: > > This clock doesn't seem needed but appears in a phandle list used by > ehci-generic.c to bulk enable it. The phandle list comes from linux, > where it is needed for suspend/resume to work [1]. > > My tests give the same results with or without this patch, but Marek > Vasut found it weird to declare an empty clk_ops [2]. > > So I adapted the code from linux 6.1-rc8 so that it hopefully works > if it ever has some user. For now, without real use, it seems to > at least not give any errors when called. > > Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/ > [2] https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/ > > Cc: Simon Glass <sjg@chromium.org> > Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > Cc: Kever Yang <kever.yang@rock-chips.com> > Cc: Lukasz Majewski <lukma@denx.de> > Cc: Sean Anderson <seanga2@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Christoph Fritz <chf.fritz@googlemail.com> > Cc: Jagan Teki <jagan@amarulasolutions.com> > > Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat> > --- > > v6: just retested over current next branch and some corrections > to message and headers > (no changes to code). > > > v5: ignores the return value from property_enable() which is not > an error code in U-Boot (unlike in linux). This avoid a false > failure of rockchip_usb2phy_clk_disable() that interfered with > clock disable and appeared to cause hang or reset. > > --- > drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 78 ++++++++++++++++++- > 1 file changed, 76 insertions(+), 2 deletions(-) > > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c > index 2f31350134..451841b025 100644 > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c > @@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg { > > struct rockchip_usb2phy_cfg { > unsigned int reg; > + struct usb2phy_reg clkout_ctl; > const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS]; > }; > > @@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base, > return writel(val, reg_base + reg->offset); > } > > +static inline bool property_enabled(void *reg_base, > + const struct usb2phy_reg *reg) > +{ > + unsigned int tmp, orig; > + unsigned int mask = GENMASK(reg->bitend, reg->bitstart); > + > + orig = readl(reg_base + reg->offset); > + > + tmp = (orig & mask) >> reg->bitstart; Use FIELD_GET() macro if possible. > + return tmp != reg->disable; > +} > + > static const > struct rockchip_usb2phy_port_cfg *us2phy_get_port(struct phy *phy) > { > @@ -168,7 +181,63 @@ static struct phy_ops rockchip_usb2phy_ops = { > .of_xlate = rockchip_usb2phy_of_xlate, > }; [...]
Thanks for looking at this. El Sun, Jun 04, 2023 at 11:33:21AM +0200, Marek Vasut deia: > > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c > > index 2f31350134..451841b025 100644 > > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c > > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c > > @@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg { > > struct rockchip_usb2phy_cfg { > > unsigned int reg; > > + struct usb2phy_reg clkout_ctl; > > const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS]; > > }; > > @@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base, > > return writel(val, reg_base + reg->offset); > > } > > +static inline bool property_enabled(void *reg_base, > > + const struct usb2phy_reg *reg) > > +{ > > + unsigned int tmp, orig; > > + unsigned int mask = GENMASK(reg->bitend, reg->bitstart); > > + > > + orig = readl(reg_base + reg->offset); > > + > > + tmp = (orig & mask) >> reg->bitstart; > > Use FIELD_GET() macro if possible. > It would be possible, but it seems to require a constant mask. Now the mask is read from a cfg struct, so it's not constant. Currently the mask bitend and bitstart are really always the same value (4 and 4) for all (2) SOCs, so I could change the code to make it a constant and use FIELD_GET(), but I'd see two drawbacks: - It makes code more different than needed from linux drivers/phy/rockchip/phy-rockchip-inno-usb2.c - It will stop working if we ever support rk3366 here, the mask there is bit 15. So I'd rather leave it as it is. But you made me realise I was missing the clkout_ctl struct for rk3568, so I'll copy from linux in v7. I can't test it, though. Thank you.
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c index 2f31350134..451841b025 100644 --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c @@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg { struct rockchip_usb2phy_cfg { unsigned int reg; + struct usb2phy_reg clkout_ctl; const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS]; }; @@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base, return writel(val, reg_base + reg->offset); } +static inline bool property_enabled(void *reg_base, + const struct usb2phy_reg *reg) +{ + unsigned int tmp, orig; + unsigned int mask = GENMASK(reg->bitend, reg->bitstart); + + orig = readl(reg_base + reg->offset); + + tmp = (orig & mask) >> reg->bitstart; + return tmp != reg->disable; +} + static const struct rockchip_usb2phy_port_cfg *us2phy_get_port(struct phy *phy) { @@ -168,7 +181,63 @@ static struct phy_ops rockchip_usb2phy_ops = { .of_xlate = rockchip_usb2phy_of_xlate, }; +/** + * round_rate() - Adjust a rate to the exact rate a clock can provide. + * @clk: The clock to manipulate. + * @rate: Desidered clock rate in Hz. + * + * Return: rounded rate in Hz, or -ve error code. + */ +ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate) +{ + return 480000000; +} + +/** + * enable() - Enable a clock. + * @clk: The clock to manipulate. + * + * Return: zero on success, or -ve error code. + */ +int rockchip_usb2phy_clk_enable(struct clk *clk) +{ + struct udevice *parent = dev_get_parent(clk->dev); + struct rockchip_usb2phy *priv = dev_get_priv(parent); + const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg; + + /* turn on 480m clk output if it is off */ + if (!property_enabled(priv->reg_base, &phy_cfg->clkout_ctl)) { + property_enable(priv->reg_base, &phy_cfg->clkout_ctl, true); + + /* waiting for the clk become stable */ + usleep_range(1200, 1300); + } + + return 0; +} + +/** + * disable() - Disable a clock. + * @clk: The clock to manipulate. + * + * Return: zero on success, or -ve error code. + */ +int rockchip_usb2phy_clk_disable(struct clk *clk) +{ + struct udevice *parent = dev_get_parent(clk->dev); + struct rockchip_usb2phy *priv = dev_get_priv(parent); + const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg; + + /* turn off 480m clk output */ + property_enable(priv->reg_base, &phy_cfg->clkout_ctl, false); + + return 0; +} + static struct clk_ops rockchip_usb2phy_clk_ops = { + .enable = rockchip_usb2phy_clk_enable, + .disable = rockchip_usb2phy_clk_disable, + .round_rate = rockchip_usb2phy_clk_round_rate }; static int rockchip_usb2phy_probe(struct udevice *dev) @@ -254,8 +323,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev) if (!ret) { node = dev_ofnode(dev); - name = ofnode_get_name(node); - dev_dbg(dev, "clk for node %s\n", name); + name = "clk_usbphy_480m"; + dev_read_string_index(dev, "clock-output-names", 0, &name); + + dev_dbg(dev, "clk %s for node %s\n", name, ofnode_get_name(node)); + ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock", name, node, &usb2phy_dev); if (ret) { @@ -270,6 +342,7 @@ static int rockchip_usb2phy_bind(struct udevice *dev) static const struct rockchip_usb2phy_cfg rk3399_usb2phy_cfgs[] = { { .reg = 0xe450, + .clkout_ctl = { 0xe450, 4, 4, 1, 0 }, .port_cfgs = { [USB2PHY_PORT_OTG] = { .phy_sus = { 0xe454, 1, 0, 2, 1 }, @@ -291,6 +364,7 @@ static const struct rockchip_usb2phy_cfg rk3399_usb2phy_cfgs[] = { }, { .reg = 0xe460, + .clkout_ctl = { 0xe460, 4, 4, 1, 0 }, .port_cfgs = { [USB2PHY_PORT_OTG] = { .phy_sus = { 0xe464, 1, 0, 2, 1 },
This clock doesn't seem needed but appears in a phandle list used by ehci-generic.c to bulk enable it. The phandle list comes from linux, where it is needed for suspend/resume to work [1]. My tests give the same results with or without this patch, but Marek Vasut found it weird to declare an empty clk_ops [2]. So I adapted the code from linux 6.1-rc8 so that it hopefully works if it ever has some user. For now, without real use, it seems to at least not give any errors when called. Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/ [2] https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/ Cc: Simon Glass <sjg@chromium.org> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> Cc: Kever Yang <kever.yang@rock-chips.com> Cc: Lukasz Majewski <lukma@denx.de> Cc: Sean Anderson <seanga2@gmail.com> Cc: Marek Vasut <marex@denx.de> Cc: Christoph Fritz <chf.fritz@googlemail.com> Cc: Jagan Teki <jagan@amarulasolutions.com> Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat> --- v6: just retested over current next branch and some corrections to message and headers (no changes to code). v5: ignores the return value from property_enable() which is not an error code in U-Boot (unlike in linux). This avoid a false failure of rockchip_usb2phy_clk_disable() that interfered with clock disable and appeared to cause hang or reset. --- drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 78 ++++++++++++++++++- 1 file changed, 76 insertions(+), 2 deletions(-)