Re: [PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
On Mon, Jun 19, 2023 at 12:38 PM Xavier Drudis Ferran wrote: > > El Mon, Jun 19, 2023 at 12:04:51PM +0530, Jagan Teki deia: > > > > Please merge these two asap. Better would these two be part of the > > coming release? > > > > How do you mean ? > > They're both in master and next now. Ohh. Okay, I didn't see that. > > see commits: > > e81512ac30c154c320b54036919cd3a6f4cc1516 > 40359c94405b103d25233d8d727d671748b751b9 > > Or do you mean I should send fixes to comments and static qualifiers > for 3 functions as you pointed out ? Yes, may be second one. No problem. Thanks, Jagan.
Re: [PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
El Mon, Jun 19, 2023 at 12:04:51PM +0530, Jagan Teki deia: > > Please merge these two asap. Better would these two be part of the > coming release? > How do you mean ? They're both in master and next now. see commits: e81512ac30c154c320b54036919cd3a6f4cc1516 40359c94405b103d25233d8d727d671748b751b9 Or do you mean I should send fixes to comments and static qualifiers for 3 functions as you pointed out ? https://lists.denx.de/pipermail/u-boot/2023-June/519708.html I wasn't sure if that was a notice for me to do it better next time or it required a clean up patch.
Re: [PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
Hi Kever, On Tue, Jun 6, 2023 at 6:24 AM Kever Yang wrote: > > > On 2023/6/5 23:06, 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 > > Cc: Philipp Tomsich > > Cc: Kever Yang > > Cc: Lukasz Majewski > > Cc: Sean Anderson > > Cc: Marek Vasut > > Cc: Christoph Fritz > > Cc: Jagan Teki > > > > Signed-off-by: Xavier Drudis Ferran > Reviewed-by: Kever Yang Please merge these two asap. Better would these two be part of the coming release? Thanks, Jagan.
Re: [PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
On Mon, Jun 5, 2023 at 8:37 PM 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
> Cc: Philipp Tomsich
> Cc: Kever Yang
> Cc: Lukasz Majewski
> Cc: Sean Anderson
> Cc: Marek Vasut
> Cc: Christoph Fritz
> Cc: Jagan Teki
>
> Signed-off-by: Xavier Drudis Ferran
> ---
>
> v7: add clkout_ctl values for rk3568 (from linux).
> UNTESTED (I don't have the hardware).
>
> 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 | 80 ++-
> 1 file changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 732d37201d..be5f79490c 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -56,6 +56,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];
> };
>
> @@ -77,6 +78,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)
> {
> @@ -169,7 +182,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.
> + */
I forgot to comment, this last time. I feel these explicit comments
wouldn't be required as clk-uclass clearly documented.
> +ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
static
> +{
> + return 48000;
> +}
> +
> +/**
> + * enable() - Enable a clock.
> + * @clk: The clock to manipulate.
> + *
> + * Return: zero on success, or -ve error code.
> + */
ditto.
> +int rockchip_usb2phy_clk_enable(struct clk *clk)
ditto.
> +{
> + 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, _cfg->clkout_ctl)) {
> + property_enable(priv->reg_base, _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.
> + */
ditto.
> +int rockchip_usb2phy_clk_disable(struct clk *clk)
ditto.
Thanks,
Jagan.
Re: [PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
On Mon, Jun 5, 2023 at 8:37 PM 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 > Cc: Philipp Tomsich > Cc: Kever Yang > Cc: Lukasz Majewski > Cc: Sean Anderson > Cc: Marek Vasut > Cc: Christoph Fritz > Cc: Jagan Teki > > Signed-off-by: Xavier Drudis Ferran > --- Reviewed-by: Jagan Teki Tested-by: Jagan Teki # rk3399, rk3328, rv1126
Re: [PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
On 2023/6/5 23:06, 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
Cc: Philipp Tomsich
Cc: Kever Yang
Cc: Lukasz Majewski
Cc: Sean Anderson
Cc: Marek Vasut
Cc: Christoph Fritz
Cc: Jagan Teki
Signed-off-by: Xavier Drudis Ferran
Reviewed-by: Kever Yang
Thanks,
- Kever
---
v7: add clkout_ctl values for rk3568 (from linux).
UNTESTED (I don't have the hardware).
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 | 80 ++-
1 file changed, 78 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 732d37201d..be5f79490c 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -56,6 +56,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];
};
@@ -77,6 +78,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)
{
@@ -169,7 +182,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 48000;
+}
+
+/**
+ * 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, _cfg->clkout_ctl)) {
+ property_enable(priv->reg_base, _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, _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)
@@ -255,8 +324,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
}
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, );
+
+ dev_dbg(dev, "clk %s for node %s\n", name, ofnode_get_name(node));
+
ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
[PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
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
Cc: Philipp Tomsich
Cc: Kever Yang
Cc: Lukasz Majewski
Cc: Sean Anderson
Cc: Marek Vasut
Cc: Christoph Fritz
Cc: Jagan Teki
Signed-off-by: Xavier Drudis Ferran
---
v7: add clkout_ctl values for rk3568 (from linux).
UNTESTED (I don't have the hardware).
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 | 80 ++-
1 file changed, 78 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 732d37201d..be5f79490c 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -56,6 +56,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];
};
@@ -77,6 +78,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)
{
@@ -169,7 +182,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 48000;
+}
+
+/**
+ * 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, _cfg->clkout_ctl)) {
+ property_enable(priv->reg_base, _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, _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)
@@ -255,8 +324,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
}
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, );
+
+ 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, _dev);
if (ret) {
@@ -276,6 +348,7 @@ bind_fail:
static const
