From: Ondrej Jirman <[email protected]>
This series implements USB 3 support for Xunlong Orange Pi 3 board.
This is a re-hash of the Icenowy's earlier USB3 work[1] without code
that caused controversy previously. Orange Pi 3 board doesn't need vbus
supply to be dynamically enabled, so that code is not needed to support
USB3 on this board.
Most of patches are already reviewed. I've converted dt-bindings to yaml
format, and added the Orange Pi 3 board modifications.
Hopefully with this series we can get USB3 support into mainline for
Orange Pi 3, and build on it later to support more boards, where
supporting them is more complicated.
Please take a look.
thank you and regards,
Ondrej Jirman
[1] https://lore.kernel.org/patchwork/patch/1058919/
Changes since Icenowy v5 series:
- use earlier patches that did not include VBUS regulator/connector
code
- converted dt bindings to yaml
- added patch to enable usb3 on Orange Pi 3
Icenowy Zheng (2):
phy: allwinner: add phy driver for USB3 PHY on Allwinner H6 SoC
arm64: dts: allwinner: h6: add USB3 device nodes
Ondrej Jirman (2):
dt-bindings: Add bindings for USB3 phy on Allwinner H6
arm64: dts: allwinner: orange-pi-3: Enable USB 3.0 host support
.../phy/allwinner,sun50i-h6-usb3-phy.yaml | 47 +++++
.../dts/allwinner/sun50i-h6-orangepi-3.dts | 8 +
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 32 +++
drivers/phy/allwinner/Kconfig | 12 ++
drivers/phy/allwinner/Makefile | 1 +
drivers/phy/allwinner/phy-sun50i-usb3.c | 195 ++++++++++++++++++
6 files changed, 295 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml
create mode 100644 drivers/phy/allwinner/phy-sun50i-usb3.c
--
2.23.0
From: Ondrej Jirman <[email protected]>
Enable Allwinner's USB 3.0 phy and the host controller. Orange Pi 3
board has GL3510 USB 3.0 4-port hub connected to the SoC's USB 3.0
port. All four ports are exposed via USB3-A connectors. VBUS is
always on, since it's powered directly from DCIN (VCC-5V) and
not switchable.
Signed-off-by: Ondrej Jirman <[email protected]>
---
arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
index eb379cd402ac..259af5b0f1a7 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
@@ -94,6 +94,10 @@
status = "okay";
};
+&dwc3 {
+ status = "okay";
+};
+
&ehci0 {
status = "okay";
};
@@ -285,3 +289,7 @@
usb3_vbus-supply = <®_vcc5v>;
status = "okay";
};
+
+&usb3phy {
+ status = "okay";
+};
--
2.23.0
From: Icenowy Zheng <[email protected]>
Allwinner H6 SoC contains a USB3 PHY (with USB2 DP/DM lines also
controlled).
Add a driver for it.
The register operations in this driver is mainly extracted from the BSP
USB3 driver.
Signed-off-by: Ondrej Jirman <[email protected]>
Signed-off-by: Icenowy Zheng <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
---
drivers/phy/allwinner/Kconfig | 12 ++
drivers/phy/allwinner/Makefile | 1 +
drivers/phy/allwinner/phy-sun50i-usb3.c | 195 ++++++++++++++++++++++++
3 files changed, 208 insertions(+)
create mode 100644 drivers/phy/allwinner/phy-sun50i-usb3.c
diff --git a/drivers/phy/allwinner/Kconfig b/drivers/phy/allwinner/Kconfig
index 215425296c77..fcae35ddd430 100644
--- a/drivers/phy/allwinner/Kconfig
+++ b/drivers/phy/allwinner/Kconfig
@@ -45,3 +45,15 @@ config PHY_SUN9I_USB
sun9i SoCs.
This driver controls each individual USB 2 host PHY.
+
+config PHY_SUN50I_USB3
+ tristate "Allwinner sun50i SoC USB3 PHY driver"
+ depends on ARCH_SUNXI && HAS_IOMEM && OF
+ depends on RESET_CONTROLLER
+ select USB_COMMON
+ select GENERIC_PHY
+ help
+ Enable this to support the USB3.0-capable transceiver that is
+ part of some Allwinner sun50i SoCs.
+
+ This driver controls each individual USB 2+3 host PHY combo.
diff --git a/drivers/phy/allwinner/Makefile b/drivers/phy/allwinner/Makefile
index 799a65c0b58d..bd74901a1255 100644
--- a/drivers/phy/allwinner/Makefile
+++ b/drivers/phy/allwinner/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_PHY_SUN4I_USB) += phy-sun4i-usb.o
obj-$(CONFIG_PHY_SUN6I_MIPI_DPHY) += phy-sun6i-mipi-dphy.o
obj-$(CONFIG_PHY_SUN9I_USB) += phy-sun9i-usb.o
+obj-$(CONFIG_PHY_SUN50I_USB3) += phy-sun50i-usb3.o
diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c
new file mode 100644
index 000000000000..8e170a4d0a11
--- /dev/null
+++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Allwinner sun50i(H6) USB 3.0 phy driver
+ *
+ * Copyright (C) 2017 Icenowy Zheng <[email protected]>
+ *
+ * Based on phy-sun9i-usb.c, which is:
+ *
+ * Copyright (C) 2014-2015 Chen-Yu Tsai <[email protected]>
+ *
+ * Based on code from Allwinner BSP, which is:
+ *
+ * Copyright (c) 2010-2015 Allwinner Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/usb/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+/* Interface Status and Control Registers */
+#define SUNXI_ISCR 0x00
+#define SUNXI_PIPE_CLOCK_CONTROL 0x14
+#define SUNXI_PHY_TUNE_LOW 0x18
+#define SUNXI_PHY_TUNE_HIGH 0x1c
+#define SUNXI_PHY_EXTERNAL_CONTROL 0x20
+
+/* USB2.0 Interface Status and Control Register */
+#define SUNXI_ISCR_FORCE_VBUS (3 << 12)
+
+/* PIPE Clock Control Register */
+#define SUNXI_PCC_PIPE_CLK_OPEN (1 << 6)
+
+/* PHY External Control Register */
+#define SUNXI_PEC_EXTERN_VBUS (3 << 1)
+#define SUNXI_PEC_SSC_EN (1 << 24)
+#define SUNXI_PEC_REF_SSP_EN (1 << 26)
+
+/* PHY Tune High Register */
+#define SUNXI_TX_DEEMPH_3P5DB(n) ((n) << 19)
+#define SUNXI_TX_DEEMPH_3P5DB_MASK GENMASK(24, 19)
+#define SUNXI_TX_DEEMPH_6DB(n) ((n) << 13)
+#define SUNXI_TX_DEEMPH_6GB_MASK GENMASK(18, 13)
+#define SUNXI_TX_SWING_FULL(n) ((n) << 6)
+#define SUNXI_TX_SWING_FULL_MASK GENMASK(12, 6)
+#define SUNXI_LOS_BIAS(n) ((n) << 3)
+#define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
+#define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
+#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
+
+struct sun50i_usb3_phy {
+ struct phy *phy;
+ void __iomem *regs;
+ struct reset_control *reset;
+ struct clk *clk;
+};
+
+static void sun50i_usb3_phy_open(struct sun50i_usb3_phy *phy)
+{
+ u32 val;
+
+ val = readl(phy->regs + SUNXI_PHY_EXTERNAL_CONTROL);
+ val |= SUNXI_PEC_EXTERN_VBUS;
+ val |= SUNXI_PEC_SSC_EN | SUNXI_PEC_REF_SSP_EN;
+ writel(val, phy->regs + SUNXI_PHY_EXTERNAL_CONTROL);
+
+ val = readl(phy->regs + SUNXI_PIPE_CLOCK_CONTROL);
+ val |= SUNXI_PCC_PIPE_CLK_OPEN;
+ writel(val, phy->regs + SUNXI_PIPE_CLOCK_CONTROL);
+
+ val = readl(phy->regs + SUNXI_ISCR);
+ val |= SUNXI_ISCR_FORCE_VBUS;
+ writel(val, phy->regs + SUNXI_ISCR);
+
+ /*
+ * All the magic numbers written to the PHY_TUNE_{LOW_HIGH}
+ * registers are directly taken from the BSP USB3 driver from
+ * Allwiner.
+ */
+ writel(0x0047fc87, phy->regs + SUNXI_PHY_TUNE_LOW);
+
+ val = readl(phy->regs + SUNXI_PHY_TUNE_HIGH);
+ val &= ~(SUNXI_TXVBOOSTLVL_MASK | SUNXI_LOS_BIAS_MASK |
+ SUNXI_TX_SWING_FULL_MASK | SUNXI_TX_DEEMPH_6GB_MASK |
+ SUNXI_TX_DEEMPH_3P5DB_MASK);
+ val |= SUNXI_TXVBOOSTLVL(0x7);
+ val |= SUNXI_LOS_BIAS(0x7);
+ val |= SUNXI_TX_SWING_FULL(0x55);
+ val |= SUNXI_TX_DEEMPH_6DB(0x20);
+ val |= SUNXI_TX_DEEMPH_3P5DB(0x15);
+ writel(val, phy->regs + SUNXI_PHY_TUNE_HIGH);
+}
+
+static int sun50i_usb3_phy_init(struct phy *_phy)
+{
+ struct sun50i_usb3_phy *phy = phy_get_drvdata(_phy);
+ int ret;
+
+ ret = clk_prepare_enable(phy->clk);
+ if (ret)
+ goto err_clk;
+
+ ret = reset_control_deassert(phy->reset);
+ if (ret)
+ goto err_reset;
+
+ sun50i_usb3_phy_open(phy);
+ return 0;
+
+err_reset:
+ clk_disable_unprepare(phy->clk);
+
+err_clk:
+ return ret;
+}
+
+static int sun50i_usb3_phy_exit(struct phy *_phy)
+{
+ struct sun50i_usb3_phy *phy = phy_get_drvdata(_phy);
+
+ reset_control_assert(phy->reset);
+ clk_disable_unprepare(phy->clk);
+
+ return 0;
+}
+
+static const struct phy_ops sun50i_usb3_phy_ops = {
+ .init = sun50i_usb3_phy_init,
+ .exit = sun50i_usb3_phy_exit,
+ .owner = THIS_MODULE,
+};
+
+static int sun50i_usb3_phy_probe(struct platform_device *pdev)
+{
+ struct sun50i_usb3_phy *phy;
+ struct device *dev = &pdev->dev;
+ struct phy_provider *phy_provider;
+ struct resource *res;
+
+ phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(phy->clk)) {
+ if (PTR_ERR(phy->clk) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get phy clock\n");
+ return PTR_ERR(phy->clk);
+ }
+
+ phy->reset = devm_reset_control_get(dev, NULL);
+ if (IS_ERR(phy->reset)) {
+ dev_err(dev, "failed to get reset control\n");
+ return PTR_ERR(phy->reset);
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ phy->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(phy->regs))
+ return PTR_ERR(phy->regs);
+
+ phy->phy = devm_phy_create(dev, NULL, &sun50i_usb3_phy_ops);
+ if (IS_ERR(phy->phy)) {
+ dev_err(dev, "failed to create PHY\n");
+ return PTR_ERR(phy->phy);
+ }
+
+ phy_set_drvdata(phy->phy, phy);
+ phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+ return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id sun50i_usb3_phy_of_match[] = {
+ { .compatible = "allwinner,sun50i-h6-usb3-phy" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, sun50i_usb3_phy_of_match);
+
+static struct platform_driver sun50i_usb3_phy_driver = {
+ .probe = sun50i_usb3_phy_probe,
+ .driver = {
+ .of_match_table = sun50i_usb3_phy_of_match,
+ .name = "sun50i-usb3-phy",
+ }
+};
+module_platform_driver(sun50i_usb3_phy_driver);
+
+MODULE_DESCRIPTION("Allwinner sun50i USB 3.0 phy driver");
+MODULE_AUTHOR("Icenowy Zheng <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.23.0
From: Ondrej Jirman <[email protected]>
The new Allwinner H6 SoC contains a USB3 PHY that is wired to the
external USB3 pins of the SoC.
Add a device tree binding for the PHY.
Signed-off-by: Ondrej Jirman <[email protected]>
---
.../phy/allwinner,sun50i-h6-usb3-phy.yaml | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml
diff --git a/Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml b/Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml
new file mode 100644
index 000000000000..2fdc890748db
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 Ondrej Jirman <[email protected]>
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/phy/allwinner,sun50i-h6-usb3-phy.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Allwinner sun50i USB3 PHY
+
+maintainers:
+ - Ondrej Jirman <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - allwinner,sun50i-h6-usb3-phy
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+ "#phy-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - resets
+ - "#phy-cells"
+
+examples:
+ - |
+ #include <dt-bindings/clock/sun50i-h6-ccu.h>
+ #include <dt-bindings/reset/sun50i-h6-ccu.h>
+ phy@5210000 {
+ compatible = "allwinner,sun50i-h6-usb3-phy";
+ reg = <0x5210000 0x10000>;
+ clocks = <&ccu CLK_USB_PHY1>;
+ resets = <&ccu RST_USB_PHY1>;
+ #phy-cells = <0>;
+ };
--
2.23.0
On Sun, Oct 20, 2019 at 03:42:26PM +0200, [email protected] wrote:
> From: Ondrej Jirman <[email protected]>
>
> The new Allwinner H6 SoC contains a USB3 PHY that is wired to the
> external USB3 pins of the SoC.
>
> Add a device tree binding for the PHY.
>
> Signed-off-by: Ondrej Jirman <[email protected]>
> ---
> .../phy/allwinner,sun50i-h6-usb3-phy.yaml | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml b/Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml
> new file mode 100644
> index 000000000000..2fdc890748db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/allwinner,sun50i-h6-usb3-phy.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 Ondrej Jirman <[email protected]>
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/phy/allwinner,sun50i-h6-usb3-phy.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Allwinner sun50i USB3 PHY
H6 would be more appropriate here instead of sun50i. There's a bunch
of sun50i SoCs already, and only one uses it.
With that fixed,
Acked-by: Maxime Ripard <[email protected]>
Maxime
On Sun, Oct 20, 2019 at 03:42:27PM +0200, [email protected] wrote:
> From: Icenowy Zheng <[email protected]>
>
> Allwinner H6 SoC contains a USB3 PHY (with USB2 DP/DM lines also
> controlled).
>
> Add a driver for it.
>
> The register operations in this driver is mainly extracted from the BSP
> USB3 driver.
>
> Signed-off-by: Ondrej Jirman <[email protected]>
> Signed-off-by: Icenowy Zheng <[email protected]>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Maxime
On Sun, Oct 20, 2019 at 03:42:29PM +0200, [email protected] wrote:
> From: Ondrej Jirman <[email protected]>
>
> Enable Allwinner's USB 3.0 phy and the host controller. Orange Pi 3
> board has GL3510 USB 3.0 4-port hub connected to the SoC's USB 3.0
> port. All four ports are exposed via USB3-A connectors. VBUS is
> always on, since it's powered directly from DCIN (VCC-5V) and
> not switchable.
>
> Signed-off-by: Ondrej Jirman <[email protected]>
Those last two patches are fine with me, I'll merge them once the phy
driver will be merged.
Maxime
Hello Maxime,
On Mon, Oct 21, 2019 at 01:09:46PM +0200, Maxime Ripard wrote:
> On Sun, Oct 20, 2019 at 03:42:29PM +0200, [email protected] wrote:
> > From: Ondrej Jirman <[email protected]>
> >
> > Enable Allwinner's USB 3.0 phy and the host controller. Orange Pi 3
> > board has GL3510 USB 3.0 4-port hub connected to the SoC's USB 3.0
> > port. All four ports are exposed via USB3-A connectors. VBUS is
> > always on, since it's powered directly from DCIN (VCC-5V) and
> > not switchable.
> >
> > Signed-off-by: Ondrej Jirman <[email protected]>
>
> Those last two patches are fine with me, I'll merge them once the phy
> driver will be merged.
PHY driver has been merged. You can now pull these patches too, when
you like.
See here: https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/log/?h=next
Thank you,
o.
> Maxime
Hi,
On Mon, Nov 04, 2019 at 01:16:48PM +0100, Ondřej Jirman wrote:
> Hello Maxime,
>
> On Mon, Oct 21, 2019 at 01:09:46PM +0200, Maxime Ripard wrote:
> > On Sun, Oct 20, 2019 at 03:42:29PM +0200, [email protected] wrote:
> > > From: Ondrej Jirman <[email protected]>
> > >
> > > Enable Allwinner's USB 3.0 phy and the host controller. Orange Pi 3
> > > board has GL3510 USB 3.0 4-port hub connected to the SoC's USB 3.0
> > > port. All four ports are exposed via USB3-A connectors. VBUS is
> > > always on, since it's powered directly from DCIN (VCC-5V) and
> > > not switchable.
> > >
> > > Signed-off-by: Ondrej Jirman <[email protected]>
> >
> > Those last two patches are fine with me, I'll merge them once the phy
> > driver will be merged.
>
> PHY driver has been merged. You can now pull these patches too, when
> you like.
>
> See here: https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/log/?h=next
Thanks for letting me know, I just applied it.
Maxime
Arguments are supposed to be ordered high then low.
Signed-off-by: Rikard Falkeborn <[email protected]>
---
Spotted while trying to add compile time checks of GENMASK arguments.
Patch has only been compile tested.
drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c
index 1169f3e83a6f..b1c04f71a31d 100644
--- a/drivers/phy/allwinner/phy-sun50i-usb3.c
+++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
@@ -49,7 +49,7 @@
#define SUNXI_LOS_BIAS(n) ((n) << 3)
#define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
#define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
-#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
+#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
struct sun50i_usb3_phy {
struct phy *phy;
--
2.24.0
Hello Rikard,
On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote:
> Arguments are supposed to be ordered high then low.
>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> Spotted while trying to add compile time checks of GENMASK arguments.
> Patch has only been compile tested.
thank you!
Tested-by: Ondrej Jirman <[email protected]>
regards,
o.
> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c
> index 1169f3e83a6f..b1c04f71a31d 100644
> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c
> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
> @@ -49,7 +49,7 @@
> #define SUNXI_LOS_BIAS(n) ((n) << 3)
> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
>
> struct sun50i_usb3_phy {
> struct phy *phy;
> --
> 2.24.0
>
On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote:
> Arguments are supposed to be ordered high then low.
>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> Spotted while trying to add compile time checks of GENMASK arguments.
> Patch has only been compile tested.
My feeling, personally, is that GENMASK() really isn't worth the pain
it causes. Can we instead get rid of this thing and just use easier
to understand and less error-prone hex masks please?
I don't care what anyone else says, personally I'm going to stick with
using hex masks as I find them way easier to get right first time than
a problematical opaque macro - and I really don't want the effort of
finding out that I've got the arguments wrong when I build it. It's
just _way_ easier and less error prone to use a hex mask straight off.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
On Thu, 2019-11-07 at 23:39 +0000, Russell King - ARM Linux admin wrote:
> On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote:
> > Arguments are supposed to be ordered high then low.
> >
> > Signed-off-by: Rikard Falkeborn <[email protected]>
> > ---
> > Spotted while trying to add compile time checks of GENMASK arguments.
> > Patch has only been compile tested.
>
> My feeling, personally, is that GENMASK() really isn't worth the pain
> it causes. Can we instead get rid of this thing and just use easier
> to understand and less error-prone hex masks please?
>
> I don't care what anyone else says, personally I'm going to stick with
> using hex masks as I find them way easier to get right first time than
> a problematical opaque macro - and I really don't want the effort of
> finding out that I've got the arguments wrong when I build it. It's
> just _way_ easier and less error prone to use a hex mask straight off.
I agree, but there are already more than 8000 uses of this rather
silly (and perhaps backwards argument order) macro in the kernel.
Hi,
On Thu 07 Nov 19, 23:39, Russell King - ARM Linux admin wrote:
> On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote:
> > Arguments are supposed to be ordered high then low.
> >
> > Signed-off-by: Rikard Falkeborn <[email protected]>
> > ---
> > Spotted while trying to add compile time checks of GENMASK arguments.
> > Patch has only been compile tested.
>
> My feeling, personally, is that GENMASK() really isn't worth the pain
> it causes. Can we instead get rid of this thing and just use easier
> to understand and less error-prone hex masks please?
One advantage it has is that is matches the order in which bit fields are
usually given in datasheets, so I personally found that it makes verification
of fields much more straightforward and immediate.
My 2 cents are that it makes sense for hardware registers.
Note that I have recently introduced a SHIFT_AND_MASK_BITS macro[0] for a V4L2
driver, that I (and Mauro) would like to move to linux/bits.h eventually.
> I don't care what anyone else says, personally I'm going to stick with
> using hex masks as I find them way easier to get right first time than
> a problematical opaque macro - and I really don't want the effort of
> finding out that I've got the arguments wrong when I build it. It's
> just _way_ easier and less error prone to use a hex mask straight off.
I guess it's a matter of personal habit.
[0]: https://git.linuxtv.org/media_tree.git/commit/?id=06eff2150d4db991ca236f3d05a9dc0101475aea
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
于 2019年11月8日 GMT+08:00 上午5:45:14, "Ondřej Jirman" <[email protected]> 写到:
>Hello Rikard,
>
>On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote:
>> Arguments are supposed to be ordered high then low.
>>
>> Signed-off-by: Rikard Falkeborn <[email protected]>
>> ---
>> Spotted while trying to add compile time checks of GENMASK arguments.
>> Patch has only been compile tested.
>
>thank you!
>
>Tested-by: Ondrej Jirman <[email protected]>
Does it affect or fix the performance?
>
>regards,
> o.
>
>> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c
>b/drivers/phy/allwinner/phy-sun50i-usb3.c
>> index 1169f3e83a6f..b1c04f71a31d 100644
>> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c
>> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
>> @@ -49,7 +49,7 @@
>> #define SUNXI_LOS_BIAS(n) ((n) << 3)
>> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
>> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
>> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
>> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
>>
>> struct sun50i_usb3_phy {
>> struct phy *phy;
>> --
>> 2.24.0
>>
On Fri, Nov 08, 2019 at 07:29:21PM +0800, Icenowy Zheng wrote:
>
>
> 于 2019年11月8日 GMT+08:00 上午5:45:14, "Ondřej Jirman" <[email protected]> 写到:
> >Hello Rikard,
> >
> >On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote:
> >> Arguments are supposed to be ordered high then low.
> >>
> >> Signed-off-by: Rikard Falkeborn <[email protected]>
> >> ---
> >> Spotted while trying to add compile time checks of GENMASK arguments.
> >> Patch has only been compile tested.
> >
> >thank you!
> >
> >Tested-by: Ondrej Jirman <[email protected]>
>
> Does it affect or fix the performance?
See here: https://forum.armbian.com/topic/10131-orange-pi-lite2-usb3-now-working/?do=findComment&comment=88904
Quote:
> It may or may not help. On Opi3 I see no change, probably because HUB is
> really close to the SoC, but on boards without a HUB, SoC's USB3 phy will
> have to drive the signal over the longer cable and this patch might benefit
> those boards.
Maybe someone with boards without PHY will test it more.
regards,
o.
> >
> >regards,
> > o.
> >
> >> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c
> >b/drivers/phy/allwinner/phy-sun50i-usb3.c
> >> index 1169f3e83a6f..b1c04f71a31d 100644
> >> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c
> >> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
> >> @@ -49,7 +49,7 @@
> >> #define SUNXI_LOS_BIAS(n) ((n) << 3)
> >> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
> >> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
> >> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
> >> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
> >>
> >> struct sun50i_usb3_phy {
> >> struct phy *phy;
> >> --
> >> 2.24.0
> >>
On Fri, Nov 08, 2019 at 12:41:39PM +0100, megous hlavni wrote:
> On Fri, Nov 08, 2019 at 07:29:21PM +0800, Icenowy Zheng wrote:
> >
> >
> > 于 2019年11月8日 GMT+08:00 上午5:45:14, "Ondřej Jirman" <[email protected]> 写到:
> > >Hello Rikard,
> > >
> > >On Thu, Nov 07, 2019 at 09:46:45PM +0100, Rikard Falkeborn wrote:
> > >> Arguments are supposed to be ordered high then low.
> > >>
> > >> Signed-off-by: Rikard Falkeborn <[email protected]>
> > >> ---
> > >> Spotted while trying to add compile time checks of GENMASK arguments.
> > >> Patch has only been compile tested.
> > >
> > >thank you!
> > >
> > >Tested-by: Ondrej Jirman <[email protected]>
> >
> > Does it affect or fix the performance?
>
> See here: https://forum.armbian.com/topic/10131-orange-pi-lite2-usb3-now-working/?do=findComment&comment=88904
>
> Quote:
>
> > It may or may not help. On Opi3 I see no change, probably because HUB is
> > really close to the SoC, but on boards without a HUB, SoC's USB3 phy will
> > have to drive the signal over the longer cable and this patch might benefit
> > those boards.
>
> Maybe someone with boards without PHY will test it more.
Eh, on boards without a USB3 HUB.
> regards,
> o.
>
> > >
> > >regards,
> > > o.
> > >
> > >> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c
> > >b/drivers/phy/allwinner/phy-sun50i-usb3.c
> > >> index 1169f3e83a6f..b1c04f71a31d 100644
> > >> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c
> > >> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
> > >> @@ -49,7 +49,7 @@
> > >> #define SUNXI_LOS_BIAS(n) ((n) << 3)
> > >> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
> > >> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
> > >> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
> > >> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
> > >>
> > >> struct sun50i_usb3_phy {
> > >> struct phy *phy;
> > >> --
> > >> 2.24.0
> > >>
Arguments are supposed to be ordered high then low.
Fixes: a228890f9458 ("phy: allwinner: add phy driver for USB3 PHY on Allwinner H6 SoC")
Signed-off-by: Rikard Falkeborn <[email protected]>
Tested-by: Ondrej Jirman <[email protected]>
---
v1->v2: Add fixes tax. Add Ondrejs Tested-by. No functional change.
drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c
index 1169f3e83a6f..b1c04f71a31d 100644
--- a/drivers/phy/allwinner/phy-sun50i-usb3.c
+++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
@@ -49,7 +49,7 @@
#define SUNXI_LOS_BIAS(n) ((n) << 3)
#define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
#define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
-#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
+#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
struct sun50i_usb3_phy {
struct phy *phy;
--
2.24.0
On Sun, Nov 10, 2019 at 01:43:55PM +0100, Rikard Falkeborn wrote:
> Arguments are supposed to be ordered high then low.
>
> Fixes: a228890f9458 ("phy: allwinner: add phy driver for USB3 PHY on Allwinner H6 SoC")
> Signed-off-by: Rikard Falkeborn <[email protected]>
> Tested-by: Ondrej Jirman <[email protected]>
> ---
> v1->v2: Add fixes tax. Add Ondrejs Tested-by. No functional change.
>
> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c
> index 1169f3e83a6f..b1c04f71a31d 100644
> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c
> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
> @@ -49,7 +49,7 @@
> #define SUNXI_LOS_BIAS(n) ((n) << 3)
> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
>
> struct sun50i_usb3_phy {
> struct phy *phy;
> --
> 2.24.0
>
Ping
On Sun, Nov 10, 2019 at 01:43:55PM +0100, Rikard Falkeborn wrote:
> Arguments are supposed to be ordered high then low.
>
> Fixes: a228890f9458 ("phy: allwinner: add phy driver for USB3 PHY on Allwinner H6 SoC")
> Signed-off-by: Rikard Falkeborn <[email protected]>
> Tested-by: Ondrej Jirman <[email protected]>
> ---
> v1->v2: Add fixes tax. Add Ondrejs Tested-by. No functional change.
>
> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c
> index 1169f3e83a6f..b1c04f71a31d 100644
> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c
> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
> @@ -49,7 +49,7 @@
> #define SUNXI_LOS_BIAS(n) ((n) << 3)
> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
>
> struct sun50i_usb3_phy {
> struct phy *phy;
> --
> 2.24.0
>
Ping.
On Sun, Feb 23, 2020 at 7:42 AM Ondrej Jirman <[email protected]> wrote:
>
> From: Rikard Falkeborn <[email protected]>
>
> Arguments are supposed to be ordered high then low.
>
> Fixes: a228890f9458 ("phy: allwinner: add phy driver for USB3 PHY on Allwinner H6 SoC")
> Signed-off-by: Rikard Falkeborn <[email protected]>
> Tested-by: Ondrej Jirman <[email protected]>
> Signed-off-by: Ondrej Jirman <[email protected]>
Acked-by: Chen-Yu Tsai <[email protected]>
> ---
> v1->v2: Add fixes tax. Add Ondrejs Tested-by. No functional change.
>
> This was last sent in Nov last year. I'm resending, because it probably
> got forgotten. The only change is adding my SoB, which I understand is
> required for the sender of the patch.
>
> drivers/phy/allwinner/phy-sun50i-usb3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun50i-usb3.c b/drivers/phy/allwinner/phy-sun50i-usb3.c
> index 1169f3e83a6f..b1c04f71a31d 100644
> --- a/drivers/phy/allwinner/phy-sun50i-usb3.c
> +++ b/drivers/phy/allwinner/phy-sun50i-usb3.c
> @@ -49,7 +49,7 @@
> #define SUNXI_LOS_BIAS(n) ((n) << 3)
> #define SUNXI_LOS_BIAS_MASK GENMASK(5, 3)
> #define SUNXI_TXVBOOSTLVL(n) ((n) << 0)
> -#define SUNXI_TXVBOOSTLVL_MASK GENMASK(0, 2)
> +#define SUNXI_TXVBOOSTLVL_MASK GENMASK(2, 0)
>
> struct sun50i_usb3_phy {
> struct phy *phy;
> --
> 2.24.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20191110124355.1569-1-rikard.falkeborn%40gmail.com.
On Sun, Feb 23, 2020 at 12:41:25AM +0100, Ondrej Jirman wrote:
> From: Rikard Falkeborn <[email protected]>
>
> Arguments are supposed to be ordered high then low.
>
> Fixes: a228890f9458 ("phy: allwinner: add phy driver for USB3 PHY on Allwinner H6 SoC")
> Signed-off-by: Rikard Falkeborn <[email protected]>
> Tested-by: Ondrej Jirman <[email protected]>
> Signed-off-by: Ondrej Jirman <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Maxime