[PATCH v5 0/7] Cleanup Octeon DWC3 glue code

[PATCH v5 0/7] Cleanup Octeon DWC3 glue code

[Ladislav Michl]

Hi!

The glue code currently lives in arch/mips/cavium-octeon/octeon-usb.c
and loops for each "cavium,octeon-7130-usb-uctl" compatible.
However there is no bond with dwc3 core code, so if anything goes
wrong in glue code, the loop breaks, leaving dwc3 in reset.
  
Later on when dwc3 core tries to read any device register, bus error
is emited, leading to kernel panic.

Therefore move it to drivers/usb/dwc3 while making it glue driver.
 
This is a fifth attempt splitted between more patches with warnings
for 32bit test targets silenced, see changelog appended to them.

Ladislav Michl (7):
  usb: dwc3: dwc3-octeon: Convert to glue driver
  usb: dwc3: dwc3-octeon: Use _ULL bitfields defines
  usb: dwc3: dwc3-octeon: Pass dwc3_octeon to setup functions
  usb: dwc3: dwc3-octeon: Avoid half-initialized controller state
  usb: dwc3: dwc3-octeon: Move node parsing into driver probe
  usb: dwc3: dwc3-octeon: Dump control register on clock init failure
  usb: dwc3: dwc3-octeon: Add SPDX header and copyright

 arch/mips/cavium-octeon/Makefile              |   1 -
 arch/mips/cavium-octeon/octeon-platform.c     |   1 -
 drivers/usb/dwc3/Kconfig                      |  10 +
 drivers/usb/dwc3/Makefile                     |   1 +
 .../usb/dwc3/dwc3-octeon.c                    | 429 +++++++++---------
 drivers/usb/dwc3/dwc3-of-simple.c             |   1 -
 6 files changed, 220 insertions(+), 223 deletions(-)
 rename arch/mips/cavium-octeon/octeon-usb.c => drivers/usb/dwc3/dwc3-octeon.c (61%)

-- 
2.39.2

[PATCH v5 1/7] usb: dwc3: dwc3-octeon: Convert to glue driver

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

DWC3 as implemented in Cavium SoC is using UCTL bridge unit
between I/O interconnect and USB controller.

Currently there is no bond with dwc3 core code, so if anything goes
wrong in UCTL setup dwc3 is left in reset, which leads to bus error
while trying to read any device register. Thus any failure in UCTL
initialization ends with kernel panic.

To avoid this move Octeon DWC3 glue code from arch/mips and make it
proper glue driver which is used instead of dwc3-of-simple.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 CHANGES:
 - v2: squashed move and glue conversion patch, fixed sparse warning
       and formatting issue. Set private data at the end of probe.
       Clear drvdata on remove. Added host mode only notice.
       Collected ack for move from arch/mips.
 - v3: more descriptive commit message, dropped unrelated changes
 - v4: rename dwc3_data to dwc3_octeon, collect Thinh's ack.
 - v5: none

 arch/mips/cavium-octeon/Makefile              |   1 -
 arch/mips/cavium-octeon/octeon-platform.c     |   1 -
 drivers/usb/dwc3/Kconfig                      |  10 ++
 drivers/usb/dwc3/Makefile                     |   1 +
 .../usb/dwc3/dwc3-octeon.c                    | 105 ++++++++++--------
 drivers/usb/dwc3/dwc3-of-simple.c             |   1 -
 6 files changed, 68 insertions(+), 51 deletions(-)
 rename arch/mips/cavium-octeon/octeon-usb.c => drivers/usb/dwc3/dwc3-octeon.c (91%)

diff --git a/arch/mips/cavium-octeon/Makefile b/arch/mips/cavium-octeon/Makefile
index 7c02e542959a..2a5926578841 100644
--- a/arch/mips/cavium-octeon/Makefile
+++ b/arch/mips/cavium-octeon/Makefile
@@ -18,4 +18,3 @@ obj-y += crypto/
 obj-$(CONFIG_MTD)		      += flash_setup.o
 obj-$(CONFIG_SMP)		      += smp.o
 obj-$(CONFIG_OCTEON_ILM)	      += oct_ilm.o
-obj-$(CONFIG_USB)		      += octeon-usb.o
diff --git a/arch/mips/cavium-octeon/octeon-platform.c b/arch/mips/cavium-octeon/octeon-platform.c
index ce05c0dd3acd..235c77ce7b18 100644
--- a/arch/mips/cavium-octeon/octeon-platform.c
+++ b/arch/mips/cavium-octeon/octeon-platform.c
@@ -450,7 +450,6 @@ static const struct of_device_id octeon_ids[] __initconst = {
 	{ .compatible = "cavium,octeon-3860-bootbus", },
 	{ .compatible = "cavium,mdio-mux", },
 	{ .compatible = "gpio-leds", },
-	{ .compatible = "cavium,octeon-7130-usb-uctl", },
 	{},
 };
 
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index be954a9abbe0..98efcbb76c88 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -168,4 +168,14 @@ config USB_DWC3_AM62
 	  The Designware Core USB3 IP is programmed to operate in
 	  in USB 2.0 mode only.
 	  Say 'Y' or 'M' here if you have one such device
+
+config USB_DWC3_OCTEON
+	tristate "Cavium Octeon Platforms"
+	depends on CAVIUM_OCTEON_SOC || COMPILE_TEST
+	default USB_DWC3
+	help
+	  Support Cavium Octeon platforms with DesignWare Core USB3 IP.
+	  Only the host mode is currently supported.
+	  Say 'Y' or 'M' here if you have one such device.
+
 endif
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index 9f66bd82b639..fe1493d4bbe5 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_USB_DWC3_ST)		+= dwc3-st.o
 obj-$(CONFIG_USB_DWC3_QCOM)		+= dwc3-qcom.o
 obj-$(CONFIG_USB_DWC3_IMX8MP)		+= dwc3-imx8mp.o
 obj-$(CONFIG_USB_DWC3_XILINX)		+= dwc3-xilinx.o
+obj-$(CONFIG_USB_DWC3_OCTEON)		+= dwc3-octeon.o
diff --git a/arch/mips/cavium-octeon/octeon-usb.c b/drivers/usb/dwc3/dwc3-octeon.c
similarity index 91%
rename from arch/mips/cavium-octeon/octeon-usb.c
rename to drivers/usb/dwc3/dwc3-octeon.c
index 2add435ad038..7134cdfc0fb6 100644
--- a/arch/mips/cavium-octeon/octeon-usb.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -187,7 +187,10 @@
 #define USBDRD_UCTL_ECC				0xf0
 #define USBDRD_UCTL_SPARE1			0xf8
 
-static DEFINE_MUTEX(dwc3_octeon_clocks_mutex);
+struct dwc3_octeon {
+	struct device *dev;
+	void __iomem *base;
+};
 
 #ifdef CONFIG_CAVIUM_OCTEON_SOC
 #include <asm/octeon/octeon.h>
@@ -233,6 +236,11 @@ static inline uint64_t dwc3_octeon_readq(void __iomem *addr)
 static inline void dwc3_octeon_writeq(void __iomem *base, uint64_t val) { }
 
 static inline void dwc3_octeon_config_gpio(int index, int gpio) { }
+
+static uint64_t octeon_get_io_clock_rate(void)
+{
+	return 150000000;
+}
 #endif
 
 static int dwc3_octeon_get_divider(void)
@@ -494,58 +502,59 @@ static void __init dwc3_octeon_phy_reset(void __iomem *base)
 	dwc3_octeon_writeq(uctl_ctl_reg, val);
 }
 
-static int __init dwc3_octeon_device_init(void)
+static int dwc3_octeon_probe(struct platform_device *pdev)
 {
-	const char compat_node_name[] = "cavium,octeon-7130-usb-uctl";
-	struct platform_device *pdev;
-	struct device_node *node;
-	struct resource *res;
-	void __iomem *base;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct dwc3_octeon *octeon;
+	int err;
 
-	/*
-	 * There should only be three universal controllers, "uctl"
-	 * in the device tree. Two USB and a SATA, which we ignore.
-	 */
-	node = NULL;
-	do {
-		node = of_find_node_by_name(node, "uctl");
-		if (!node)
-			return -ENODEV;
-
-		if (of_device_is_compatible(node, compat_node_name)) {
-			pdev = of_find_device_by_node(node);
-			if (!pdev)
-				return -ENODEV;
-
-			/*
-			 * The code below maps in the registers necessary for
-			 * setting up the clocks and reseting PHYs. We must
-			 * release the resources so the dwc3 subsystem doesn't
-			 * know the difference.
-			 */
-			base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
-			if (IS_ERR(base)) {
-				put_device(&pdev->dev);
-				return PTR_ERR(base);
-			}
+	octeon = devm_kzalloc(dev, sizeof(*octeon), GFP_KERNEL);
+	if (!octeon)
+		return -ENOMEM;
 
-			mutex_lock(&dwc3_octeon_clocks_mutex);
-			if (dwc3_octeon_clocks_start(&pdev->dev, base) == 0)
-				dev_info(&pdev->dev, "clocks initialized.\n");
-			dwc3_octeon_set_endian_mode(base);
-			dwc3_octeon_phy_reset(base);
-			mutex_unlock(&dwc3_octeon_clocks_mutex);
-			devm_iounmap(&pdev->dev, base);
-			devm_release_mem_region(&pdev->dev, res->start,
-						resource_size(res));
-			put_device(&pdev->dev);
-		}
-	} while (node != NULL);
+	octeon->dev = dev;
+	octeon->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(octeon->base))
+		return PTR_ERR(octeon->base);
 
-	return 0;
+	err = dwc3_octeon_clocks_start(dev, octeon->base);
+	if (err)
+		return err;
+
+	dwc3_octeon_set_endian_mode(octeon->base);
+	dwc3_octeon_phy_reset(octeon->base);
+
+	platform_set_drvdata(pdev, octeon);
+
+	return of_platform_populate(node, NULL, NULL, dev);
+}
+
+static void dwc3_octeon_remove(struct platform_device *pdev)
+{
+	struct dwc3_octeon *octeon = platform_get_drvdata(pdev);
+
+	of_platform_depopulate(octeon->dev);
+	platform_set_drvdata(pdev, NULL);
 }
-device_initcall(dwc3_octeon_device_init);
 
+static const struct of_device_id dwc3_octeon_of_match[] = {
+	{ .compatible = "cavium,octeon-7130-usb-uctl" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, dwc3_octeon_of_match);
+
+static struct platform_driver dwc3_octeon_driver = {
+	.probe		= dwc3_octeon_probe,
+	.remove_new	= dwc3_octeon_remove,
+	.driver		= {
+		.name	= "dwc3-octeon",
+		.of_match_table = dwc3_octeon_of_match,
+	},
+};
+module_platform_driver(dwc3_octeon_driver);
+
+MODULE_ALIAS("platform:dwc3-octeon");
 MODULE_AUTHOR("David Daney <david.daney@cavium.com>");
 MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("USB driver for OCTEON III SoC");
+MODULE_DESCRIPTION("DesignWare USB3 OCTEON III Glue Layer");
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index 7e6ad8fe61a5..d1539fc9eabd 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -170,7 +170,6 @@ static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops = {
 
 static const struct of_device_id of_dwc3_simple_match[] = {
 	{ .compatible = "rockchip,rk3399-dwc3" },
-	{ .compatible = "cavium,octeon-7130-usb-uctl" },
 	{ .compatible = "sprd,sc9860-dwc3" },
 	{ .compatible = "allwinner,sun50i-h6-dwc3" },
 	{ .compatible = "hisilicon,hi3670-dwc3" },
-- 
2.39.2

[PATCH v5 2/7] usb: dwc3: dwc3-octeon: Use _ULL bitfields defines

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

While driver is intended to run on 64bit machines, it is compile time
tested for 32bit targets as well. Here shift count overflow is reported
for bits greater than 31, so use _ULL versions of BIT and GENMASK macros
to silence these warnings.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202307260537.MROrhVNM-lkp@intel.com/
---
 CHANGES:
 -v5: new patch

 drivers/usb/dwc3/dwc3-octeon.c | 78 +++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index 7134cdfc0fb6..69fe50cfa719 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -24,9 +24,9 @@
 /* BIST fast-clear mode select. A BIST run with this bit set
  * clears all entries in USBH RAMs to 0x0.
  */
-# define USBDRD_UCTL_CTL_CLEAR_BIST		BIT(63)
+# define USBDRD_UCTL_CTL_CLEAR_BIST		BIT_ULL(63)
 /* 1 = Start BIST and cleared by hardware */
-# define USBDRD_UCTL_CTL_START_BIST		BIT(62)
+# define USBDRD_UCTL_CTL_START_BIST		BIT_ULL(62)
 /* Reference clock select for SuperSpeed and HighSpeed PLLs:
  *	0x0 = Both PLLs use DLMC_REF_CLK0 for reference clock
  *	0x1 = Both PLLs use DLMC_REF_CLK1 for reference clock
@@ -35,32 +35,32 @@
  *	0x3 = SuperSpeed PLL uses DLMC_REF_CLK1 for reference clock &
  *	      HighSpeed PLL uses PLL_REF_CLK for reference clck
  */
-# define USBDRD_UCTL_CTL_REF_CLK_SEL		GENMASK(61, 60)
+# define USBDRD_UCTL_CTL_REF_CLK_SEL		GENMASK_ULL(61, 60)
 /* 1 = Spread-spectrum clock enable, 0 = SS clock disable */
-# define USBDRD_UCTL_CTL_SSC_EN			BIT(59)
+# define USBDRD_UCTL_CTL_SSC_EN			BIT_ULL(59)
 /* Spread-spectrum clock modulation range:
  *	0x0 = -4980 ppm downspread
  *	0x1 = -4492 ppm downspread
  *	0x2 = -4003 ppm downspread
  *	0x3 - 0x7 = Reserved
  */
-# define USBDRD_UCTL_CTL_SSC_RANGE		GENMASK(58, 56)
+# define USBDRD_UCTL_CTL_SSC_RANGE		GENMASK_ULL(58, 56)
 /* Enable non-standard oscillator frequencies:
  *	[55:53] = modules -1
  *	[52:47] = 2's complement push amount, 0 = Feature disabled
  */
-# define USBDRD_UCTL_CTL_SSC_REF_CLK_SEL	GENMASK(55, 47)
+# define USBDRD_UCTL_CTL_SSC_REF_CLK_SEL	GENMASK_ULL(55, 47)
 /* Reference clock multiplier for non-standard frequencies:
  *	0x19 = 100MHz on DLMC_REF_CLK* if REF_CLK_SEL = 0x0 or 0x1
  *	0x28 = 125MHz on DLMC_REF_CLK* if REF_CLK_SEL = 0x0 or 0x1
  *	0x32 =  50MHz on DLMC_REF_CLK* if REF_CLK_SEL = 0x0 or 0x1
  *	Other Values = Reserved
  */
-# define USBDRD_UCTL_CTL_MPLL_MULTIPLIER	GENMASK(46, 40)
+# define USBDRD_UCTL_CTL_MPLL_MULTIPLIER	GENMASK_ULL(46, 40)
 /* Enable reference clock to prescaler for SuperSpeed functionality.
  * Should always be set to "1"
  */
-# define USBDRD_UCTL_CTL_REF_SSP_EN		BIT(39)
+# define USBDRD_UCTL_CTL_REF_SSP_EN		BIT_ULL(39)
 /* Divide the reference clock by 2 before entering the
  * REF_CLK_FSEL divider:
  *	If REF_CLK_SEL = 0x0 or 0x1, then only 0x0 is legal
@@ -68,21 +68,21 @@
  *		0x1 = DLMC_REF_CLK* is 125MHz
  *		0x0 = DLMC_REF_CLK* is another supported frequency
  */
-# define USBDRD_UCTL_CTL_REF_CLK_DIV2		BIT(38)
+# define USBDRD_UCTL_CTL_REF_CLK_DIV2		BIT_ULL(38)
 /* Select reference clock freqnuency for both PLL blocks:
  *	0x27 = REF_CLK_SEL is 0x0 or 0x1
  *	0x07 = REF_CLK_SEL is 0x2 or 0x3
  */
-# define USBDRD_UCTL_CTL_REF_CLK_FSEL		GENMASK(37, 32)
+# define USBDRD_UCTL_CTL_REF_CLK_FSEL		GENMASK_ULL(37, 32)
 /* Controller clock enable. */
-# define USBDRD_UCTL_CTL_H_CLK_EN		BIT(30)
+# define USBDRD_UCTL_CTL_H_CLK_EN		BIT_ULL(30)
 /* Select bypass input to controller clock divider:
  *	0x0 = Use divided coprocessor clock from H_CLKDIV
  *	0x1 = Use clock from GPIO pins
  */
-# define USBDRD_UCTL_CTL_H_CLK_BYP_SEL		BIT(29)
+# define USBDRD_UCTL_CTL_H_CLK_BYP_SEL		BIT_ULL(29)
 /* Reset controller clock divider. */
-# define USBDRD_UCTL_CTL_H_CLKDIV_RST		BIT(28)
+# define USBDRD_UCTL_CTL_H_CLKDIV_RST		BIT_ULL(28)
 /* Clock divider select:
  *	0x0 = divide by 1
  *	0x1 = divide by 2
@@ -93,29 +93,29 @@
  *	0x6 = divide by 24
  *	0x7 = divide by 32
  */
-# define USBDRD_UCTL_CTL_H_CLKDIV_SEL		GENMASK(26, 24)
+# define USBDRD_UCTL_CTL_H_CLKDIV_SEL		GENMASK_ULL(26, 24)
 /* USB3 port permanently attached: 0x0 = No, 0x1 = Yes */
-# define USBDRD_UCTL_CTL_USB3_PORT_PERM_ATTACH	BIT(21)
+# define USBDRD_UCTL_CTL_USB3_PORT_PERM_ATTACH	BIT_ULL(21)
 /* USB2 port permanently attached: 0x0 = No, 0x1 = Yes */
-# define USBDRD_UCTL_CTL_USB2_PORT_PERM_ATTACH	BIT(20)
+# define USBDRD_UCTL_CTL_USB2_PORT_PERM_ATTACH	BIT_ULL(20)
 /* Disable SuperSpeed PHY: 0x0 = No, 0x1 = Yes */
-# define USBDRD_UCTL_CTL_USB3_PORT_DISABLE	BIT(18)
+# define USBDRD_UCTL_CTL_USB3_PORT_DISABLE	BIT_ULL(18)
 /* Disable HighSpeed PHY: 0x0 = No, 0x1 = Yes */
-# define USBDRD_UCTL_CTL_USB2_PORT_DISABLE	BIT(16)
+# define USBDRD_UCTL_CTL_USB2_PORT_DISABLE	BIT_ULL(16)
 /* Enable PHY SuperSpeed block power: 0x0 = No, 0x1 = Yes */
-# define USBDRD_UCTL_CTL_SS_POWER_EN		BIT(14)
+# define USBDRD_UCTL_CTL_SS_POWER_EN		BIT_ULL(14)
 /* Enable PHY HighSpeed block power: 0x0 = No, 0x1 = Yes */
-# define USBDRD_UCTL_CTL_HS_POWER_EN		BIT(12)
+# define USBDRD_UCTL_CTL_HS_POWER_EN		BIT_ULL(12)
 /* Enable USB UCTL interface clock: 0xx = No, 0x1 = Yes */
-# define USBDRD_UCTL_CTL_CSCLK_EN		BIT(4)
+# define USBDRD_UCTL_CTL_CSCLK_EN		BIT_ULL(4)
 /* Controller mode: 0x0 = Host, 0x1 = Device */
-# define USBDRD_UCTL_CTL_DRD_MODE		BIT(3)
+# define USBDRD_UCTL_CTL_DRD_MODE		BIT_ULL(3)
 /* PHY reset */
-# define USBDRD_UCTL_CTL_UPHY_RST		BIT(2)
+# define USBDRD_UCTL_CTL_UPHY_RST		BIT_ULL(2)
 /* Software reset UAHC */
-# define USBDRD_UCTL_CTL_UAHC_RST		BIT(1)
+# define USBDRD_UCTL_CTL_UAHC_RST		BIT_ULL(1)
 /* Software resets UCTL */
-# define USBDRD_UCTL_CTL_UCTL_RST		BIT(0)
+# define USBDRD_UCTL_CTL_UCTL_RST		BIT_ULL(0)
 
 #define USBDRD_UCTL_BIST_STATUS			0x08
 #define USBDRD_UCTL_SPARE0			0x10
@@ -130,59 +130,59 @@
  */
 #define USBDRD_UCTL_HOST_CFG			0xe0
 /* Indicates minimum value of all received BELT values */
-# define USBDRD_UCTL_HOST_CFG_HOST_CURRENT_BELT	GENMASK(59, 48)
+# define USBDRD_UCTL_HOST_CFG_HOST_CURRENT_BELT	GENMASK_ULL(59, 48)
 /* HS jitter adjustment */
-# define USBDRD_UCTL_HOST_CFG_FLA		GENMASK(37, 32)
+# define USBDRD_UCTL_HOST_CFG_FLA		GENMASK_ULL(37, 32)
 /* Bus-master enable: 0x0 = Disabled (stall DMAs), 0x1 = enabled */
-# define USBDRD_UCTL_HOST_CFG_BME		BIT(28)
+# define USBDRD_UCTL_HOST_CFG_BME		BIT_ULL(28)
 /* Overcurrent protection enable: 0x0 = unavailable, 0x1 = available */
-# define USBDRD_UCTL_HOST_OCI_EN		BIT(27)
+# define USBDRD_UCTL_HOST_OCI_EN		BIT_ULL(27)
 /* Overcurrent sene selection:
  *	0x0 = Overcurrent indication from off-chip is active-low
  *	0x1 = Overcurrent indication from off-chip is active-high
  */
-# define USBDRD_UCTL_HOST_OCI_ACTIVE_HIGH_EN	BIT(26)
+# define USBDRD_UCTL_HOST_OCI_ACTIVE_HIGH_EN	BIT_ULL(26)
 /* Port power control enable: 0x0 = unavailable, 0x1 = available */
-# define USBDRD_UCTL_HOST_PPC_EN		BIT(25)
+# define USBDRD_UCTL_HOST_PPC_EN		BIT_ULL(25)
 /* Port power control sense selection:
  *	0x0 = Port power to off-chip is active-low
  *	0x1 = Port power to off-chip is active-high
  */
-# define USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN	BIT(24)
+# define USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN	BIT_ULL(24)
 
 /*
  * UCTL Shim Features Register
  */
 #define USBDRD_UCTL_SHIM_CFG			0xe8
 /* Out-of-bound UAHC register access: 0 = read, 1 = write */
-# define USBDRD_UCTL_SHIM_CFG_XS_NCB_OOB_WRN	BIT(63)
+# define USBDRD_UCTL_SHIM_CFG_XS_NCB_OOB_WRN	BIT_ULL(63)
 /* SRCID error log for out-of-bound UAHC register access:
  *	[59:58] = chipID
  *	[57] = Request source: 0 = core, 1 = NCB-device
  *	[56:51] = Core/NCB-device number, [56] always 0 for NCB devices
  *	[50:48] = SubID
  */
-# define USBDRD_UCTL_SHIM_CFG_XS_NCB_OOB_OSRC	GENMASK(59, 48)
+# define USBDRD_UCTL_SHIM_CFG_XS_NCB_OOB_OSRC	GENMASK_ULL(59, 48)
 /* Error log for bad UAHC DMA access: 0 = Read log, 1 = Write log */
-# define USBDRD_UCTL_SHIM_CFG_XM_BAD_DMA_WRN	BIT(47)
+# define USBDRD_UCTL_SHIM_CFG_XM_BAD_DMA_WRN	BIT_ULL(47)
 /* Encoded error type for bad UAHC DMA */
-# define USBDRD_UCTL_SHIM_CFG_XM_BAD_DMA_TYPE	GENMASK(43, 40)
+# define USBDRD_UCTL_SHIM_CFG_XM_BAD_DMA_TYPE	GENMASK_ULL(43, 40)
 /* Select the IOI read command used by DMA accesses */
-# define USBDRD_UCTL_SHIM_CFG_DMA_READ_CMD	BIT(12)
+# define USBDRD_UCTL_SHIM_CFG_DMA_READ_CMD	BIT_ULL(12)
 /* Select endian format for DMA accesses to the L2C:
  *	0x0 = Little endian
  *	0x1 = Big endian
  *	0x2 = Reserved
  *	0x3 = Reserved
  */
-# define USBDRD_UCTL_SHIM_CFG_DMA_ENDIAN_MODE	GENMASK(9, 8)
+# define USBDRD_UCTL_SHIM_CFG_DMA_ENDIAN_MODE	GENMASK_ULL(9, 8)
 /* Select endian format for IOI CSR access to UAHC:
  *	0x0 = Little endian
  *	0x1 = Big endian
  *	0x2 = Reserved
  *	0x3 = Reserved
  */
-# define USBDRD_UCTL_SHIM_CFG_CSR_ENDIAN_MODE	GENMASK(1, 0)
+# define USBDRD_UCTL_SHIM_CFG_CSR_ENDIAN_MODE	GENMASK_ULL(1, 0)
 
 #define USBDRD_UCTL_ECC				0xf0
 #define USBDRD_UCTL_SPARE1			0xf8
-- 
2.39.2

Re: [PATCH v5 2/7] usb: dwc3: dwc3-octeon: Use _ULL bitfields defines

[Thinh Nguyen]

On Mon, Jul 31, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> While driver is intended to run on 64bit machines, it is compile time
> tested for 32bit targets as well. Here shift count overflow is reported
> for bits greater than 31, so use _ULL versions of BIT and GENMASK macros
> to silence these warnings.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/202307260537.MROrhVNM-lkp@intel.com/__;!!A4F2R9G_pg!bRAo7p44EQ5aHcAiBbTC-iYlO5IygKxZRV6HRGojsnNL0q_IRd7VyOnuC5SwtQBM2Dg_1O9NEeEhStq16Eo1OcYm1A$ 
> ---
>  CHANGES:
>  -v5: new patch
> 
>  drivers/usb/dwc3/dwc3-octeon.c | 78 +++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 39 deletions(-)
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

Re: [PATCH v5 2/7] usb: dwc3: dwc3-octeon: Use _ULL bitfields defines

[Philippe Mathieu-Daudé]

On 31/7/23 11:31, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> While driver is intended to run on 64bit machines, it is compile time
> tested for 32bit targets as well. Here shift count overflow is reported
> for bits greater than 31, so use _ULL versions of BIT and GENMASK macros
> to silence these warnings.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202307260537.MROrhVNM-lkp@intel.com/
> ---
>   CHANGES:
>   -v5: new patch
> 
>   drivers/usb/dwc3/dwc3-octeon.c | 78 +++++++++++++++++-----------------
>   1 file changed, 39 insertions(+), 39 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v5 3/7] usb: dwc3: dwc3-octeon: Pass dwc3_octeon to setup functions

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

Pass dwc3_octeon instead of just the base. It fits with the
function names and it requires less change in the future if
access to dwc3_octeon is needed.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 CHANGES:
 - v4: new patch
 - v5: Philippe's review tag

 drivers/usb/dwc3/dwc3-octeon.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index 69fe50cfa719..24e75881b5cf 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -300,12 +300,13 @@ static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)
 	return 0;
 }
 
-static int dwc3_octeon_clocks_start(struct device *dev, void __iomem *base)
+static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
 {
 	int i, div, mpll_mul, ref_clk_fsel, ref_clk_sel = 2;
 	u32 clock_rate;
 	u64 val;
-	void __iomem *uctl_ctl_reg = base + USBDRD_UCTL_CTL;
+	struct device *dev = octeon->dev;
+	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
 
 	if (dev->of_node) {
 		const char *ss_clock_type;
@@ -452,8 +453,8 @@ static int dwc3_octeon_clocks_start(struct device *dev, void __iomem *base)
 	/* Step 8b: Wait 10 controller-clock cycles. */
 	udelay(10);
 
-	/* Steo 8c: Setup power-power control. */
-	if (dwc3_octeon_config_power(dev, base))
+	/* Step 8c: Setup power control. */
+	if (dwc3_octeon_config_power(dev, octeon->base))
 		return -EINVAL;
 
 	/* Step 8d: Deassert UAHC reset signal. */
@@ -477,10 +478,10 @@ static int dwc3_octeon_clocks_start(struct device *dev, void __iomem *base)
 	return 0;
 }
 
-static void __init dwc3_octeon_set_endian_mode(void __iomem *base)
+static void dwc3_octeon_set_endian_mode(struct dwc3_octeon *octeon)
 {
 	u64 val;
-	void __iomem *uctl_shim_cfg_reg = base + USBDRD_UCTL_SHIM_CFG;
+	void __iomem *uctl_shim_cfg_reg = octeon->base + USBDRD_UCTL_SHIM_CFG;
 
 	val = dwc3_octeon_readq(uctl_shim_cfg_reg);
 	val &= ~USBDRD_UCTL_SHIM_CFG_DMA_ENDIAN_MODE;
@@ -492,10 +493,10 @@ static void __init dwc3_octeon_set_endian_mode(void __iomem *base)
 	dwc3_octeon_writeq(uctl_shim_cfg_reg, val);
 }
 
-static void __init dwc3_octeon_phy_reset(void __iomem *base)
+static void dwc3_octeon_phy_reset(struct dwc3_octeon *octeon)
 {
 	u64 val;
-	void __iomem *uctl_ctl_reg = base + USBDRD_UCTL_CTL;
+	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
 
 	val = dwc3_octeon_readq(uctl_ctl_reg);
 	val &= ~USBDRD_UCTL_CTL_UPHY_RST;
@@ -518,12 +519,12 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	if (IS_ERR(octeon->base))
 		return PTR_ERR(octeon->base);
 
-	err = dwc3_octeon_clocks_start(dev, octeon->base);
+	err = dwc3_octeon_clocks_start(octeon);
 	if (err)
 		return err;
 
-	dwc3_octeon_set_endian_mode(octeon->base);
-	dwc3_octeon_phy_reset(octeon->base);
+	dwc3_octeon_set_endian_mode(octeon);
+	dwc3_octeon_phy_reset(octeon);
 
 	platform_set_drvdata(pdev, octeon);
 
-- 
2.39.2

Re: [PATCH v5 3/7] usb: dwc3: dwc3-octeon: Pass dwc3_octeon to setup functions

[Thinh Nguyen]

On Mon, Jul 31, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Pass dwc3_octeon instead of just the base. It fits with the
> function names and it requires less change in the future if
> access to dwc3_octeon is needed.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  CHANGES:
>  - v4: new patch
>  - v5: Philippe's review tag
> 
>  drivers/usb/dwc3/dwc3-octeon.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index 69fe50cfa719..24e75881b5cf 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -300,12 +300,13 @@ static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)

May as well do it for dwc3_octeon_config_power() too?

BR,
Thinh

Re: [PATCH v5 3/7] usb: dwc3: dwc3-octeon: Pass dwc3_octeon to setup functions

[Thinh Nguyen]

On Tue, Aug 01, 2023, Thinh Nguyen wrote:
> On Mon, Jul 31, 2023, Ladislav Michl wrote:
> > From: Ladislav Michl <ladis@linux-mips.org>
> > 
> > Pass dwc3_octeon instead of just the base. It fits with the
> > function names and it requires less change in the future if
> > access to dwc3_octeon is needed.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  CHANGES:
> >  - v4: new patch
> >  - v5: Philippe's review tag
> > 
> >  drivers/usb/dwc3/dwc3-octeon.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> > index 69fe50cfa719..24e75881b5cf 100644
> > --- a/drivers/usb/dwc3/dwc3-octeon.c
> > +++ b/drivers/usb/dwc3/dwc3-octeon.c
> > @@ -300,12 +300,13 @@ static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)
> 
> May as well do it for dwc3_octeon_config_power() too?
> 

Nevermind this. I just notice you rewrote this function later. Ignore
this comment.

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

[PATCH v5 4/7] usb: dwc3: dwc3-octeon: Avoid half-initialized controller state

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

Power gpio configuration is done from the middle of
dwc3_octeon_clocks_start leaving hardware in half-initialized
state if it fails. As that indicates dwc3_octeon_clocks_start
does more than just initialize the clocks rename it appropriately
and verify power gpio configuration in advance at the beginning
of device probe.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 CHANGES:
 - v4: new patch
 - v5: use uintptr_t instead of u64 to retype base address to make 32bit
       compilers happy.

 drivers/usb/dwc3/dwc3-octeon.c | 90 ++++++++++++++++------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index 24e75881b5cf..0dc45dda134c 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -192,6 +192,8 @@ struct dwc3_octeon {
 	void __iomem *base;
 };
 
+#define DWC3_GPIO_POWER_NONE	(-1)
+
 #ifdef CONFIG_CAVIUM_OCTEON_SOC
 #include <asm/octeon/octeon.h>
 static inline uint64_t dwc3_octeon_readq(void __iomem *addr)
@@ -258,55 +260,15 @@ static int dwc3_octeon_get_divider(void)
 	return div;
 }
 
-static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)
-{
-	uint32_t gpio_pwr[3];
-	int gpio, len, power_active_low;
-	struct device_node *node = dev->of_node;
-	u64 val;
-	void __iomem *uctl_host_cfg_reg = base + USBDRD_UCTL_HOST_CFG;
-
-	if (of_find_property(node, "power", &len) != NULL) {
-		if (len == 12) {
-			of_property_read_u32_array(node, "power", gpio_pwr, 3);
-			power_active_low = gpio_pwr[2] & 0x01;
-			gpio = gpio_pwr[1];
-		} else if (len == 8) {
-			of_property_read_u32_array(node, "power", gpio_pwr, 2);
-			power_active_low = 0;
-			gpio = gpio_pwr[1];
-		} else {
-			dev_err(dev, "invalid power configuration\n");
-			return -EINVAL;
-		}
-		dwc3_octeon_config_gpio(((u64)base >> 24) & 1, gpio);
-
-		/* Enable XHCI power control and set if active high or low. */
-		val = dwc3_octeon_readq(uctl_host_cfg_reg);
-		val |= USBDRD_UCTL_HOST_PPC_EN;
-		if (power_active_low)
-			val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
-		else
-			val |= USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
-		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
-	} else {
-		/* Disable XHCI power control and set if active high. */
-		val = dwc3_octeon_readq(uctl_host_cfg_reg);
-		val &= ~USBDRD_UCTL_HOST_PPC_EN;
-		val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
-		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
-		dev_info(dev, "power control disabled\n");
-	}
-	return 0;
-}
-
-static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
+static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
+			     int power_gpio, int power_active_low)
 {
 	int i, div, mpll_mul, ref_clk_fsel, ref_clk_sel = 2;
 	u32 clock_rate;
 	u64 val;
 	struct device *dev = octeon->dev;
 	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
+	void __iomem *uctl_host_cfg_reg = octeon->base + USBDRD_UCTL_HOST_CFG;
 
 	if (dev->of_node) {
 		const char *ss_clock_type;
@@ -454,8 +416,21 @@ static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
 	udelay(10);
 
 	/* Step 8c: Setup power control. */
-	if (dwc3_octeon_config_power(dev, octeon->base))
-		return -EINVAL;
+	val = dwc3_octeon_readq(uctl_host_cfg_reg);
+	val |= USBDRD_UCTL_HOST_PPC_EN;
+	if (power_gpio == DWC3_GPIO_POWER_NONE) {
+		val &= ~USBDRD_UCTL_HOST_PPC_EN;
+	} else {
+		val |= USBDRD_UCTL_HOST_PPC_EN;
+		dwc3_octeon_config_gpio(((__force uintptr_t)octeon->base >> 24) & 1,
+					power_gpio);
+		dev_dbg(dev, "power control is using gpio%d\n", power_gpio);
+	}
+	if (power_active_low)
+		val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
+	else
+		val |= USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
+	dwc3_octeon_writeq(uctl_host_cfg_reg, val);
 
 	/* Step 8d: Deassert UAHC reset signal. */
 	val = dwc3_octeon_readq(uctl_ctl_reg);
@@ -508,7 +483,28 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 	struct dwc3_octeon *octeon;
-	int err;
+	int power_active_low, power_gpio;
+	int err, len;
+
+	power_gpio = DWC3_GPIO_POWER_NONE;
+	power_active_low = 0;
+	if (of_find_property(node, "power", &len)) {
+		u32 gpio_pwr[3];
+
+		switch (len) {
+		case 8:
+			of_property_read_u32_array(node, "power", gpio_pwr, 2);
+			break;
+		case 12:
+			of_property_read_u32_array(node, "power", gpio_pwr, 3);
+			power_active_low = gpio_pwr[2] & 0x01;
+			break;
+		default:
+			dev_err(dev, "invalid power configuration\n");
+			return -EINVAL;
+		}
+		power_gpio = gpio_pwr[1];
+	}
 
 	octeon = devm_kzalloc(dev, sizeof(*octeon), GFP_KERNEL);
 	if (!octeon)
@@ -519,7 +515,7 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	if (IS_ERR(octeon->base))
 		return PTR_ERR(octeon->base);
 
-	err = dwc3_octeon_clocks_start(octeon);
+	err = dwc3_octeon_setup(octeon, power_gpio, power_active_low);
 	if (err)
 		return err;
 
-- 
2.39.2

Re: [PATCH v5 4/7] usb: dwc3: dwc3-octeon: Avoid half-initialized controller state

[Thinh Nguyen]

On Mon, Jul 31, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Power gpio configuration is done from the middle of
> dwc3_octeon_clocks_start leaving hardware in half-initialized
> state if it fails. As that indicates dwc3_octeon_clocks_start
> does more than just initialize the clocks rename it appropriately
> and verify power gpio configuration in advance at the beginning
> of device probe.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  CHANGES:
>  - v4: new patch
>  - v5: use uintptr_t instead of u64 to retype base address to make 32bit
>        compilers happy.
> 
>  drivers/usb/dwc3/dwc3-octeon.c | 90 ++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index 24e75881b5cf..0dc45dda134c 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -192,6 +192,8 @@ struct dwc3_octeon {
>  	void __iomem *base;
>  };
>  
> +#define DWC3_GPIO_POWER_NONE	(-1)
> +
>  #ifdef CONFIG_CAVIUM_OCTEON_SOC
>  #include <asm/octeon/octeon.h>
>  static inline uint64_t dwc3_octeon_readq(void __iomem *addr)
> @@ -258,55 +260,15 @@ static int dwc3_octeon_get_divider(void)
>  	return div;
>  }
>  
> -static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)
> -{
> -	uint32_t gpio_pwr[3];
> -	int gpio, len, power_active_low;
> -	struct device_node *node = dev->of_node;
> -	u64 val;
> -	void __iomem *uctl_host_cfg_reg = base + USBDRD_UCTL_HOST_CFG;
> -
> -	if (of_find_property(node, "power", &len) != NULL) {
> -		if (len == 12) {
> -			of_property_read_u32_array(node, "power", gpio_pwr, 3);
> -			power_active_low = gpio_pwr[2] & 0x01;
> -			gpio = gpio_pwr[1];
> -		} else if (len == 8) {
> -			of_property_read_u32_array(node, "power", gpio_pwr, 2);
> -			power_active_low = 0;
> -			gpio = gpio_pwr[1];
> -		} else {
> -			dev_err(dev, "invalid power configuration\n");
> -			return -EINVAL;
> -		}
> -		dwc3_octeon_config_gpio(((u64)base >> 24) & 1, gpio);
> -
> -		/* Enable XHCI power control and set if active high or low. */
> -		val = dwc3_octeon_readq(uctl_host_cfg_reg);
> -		val |= USBDRD_UCTL_HOST_PPC_EN;
> -		if (power_active_low)
> -			val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> -		else
> -			val |= USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> -		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
> -	} else {
> -		/* Disable XHCI power control and set if active high. */
> -		val = dwc3_octeon_readq(uctl_host_cfg_reg);
> -		val &= ~USBDRD_UCTL_HOST_PPC_EN;
> -		val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> -		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
> -		dev_info(dev, "power control disabled\n");
> -	}
> -	return 0;
> -}
> -
> -static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
> +static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
> +			     int power_gpio, int power_active_low)
>  {
>  	int i, div, mpll_mul, ref_clk_fsel, ref_clk_sel = 2;
>  	u32 clock_rate;
>  	u64 val;
>  	struct device *dev = octeon->dev;
>  	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
> +	void __iomem *uctl_host_cfg_reg = octeon->base + USBDRD_UCTL_HOST_CFG;
>  
>  	if (dev->of_node) {
>  		const char *ss_clock_type;
> @@ -454,8 +416,21 @@ static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
>  	udelay(10);
>  
>  	/* Step 8c: Setup power control. */
> -	if (dwc3_octeon_config_power(dev, octeon->base))
> -		return -EINVAL;
> +	val = dwc3_octeon_readq(uctl_host_cfg_reg);
> +	val |= USBDRD_UCTL_HOST_PPC_EN;
> +	if (power_gpio == DWC3_GPIO_POWER_NONE) {
> +		val &= ~USBDRD_UCTL_HOST_PPC_EN;
> +	} else {
> +		val |= USBDRD_UCTL_HOST_PPC_EN;
> +		dwc3_octeon_config_gpio(((__force uintptr_t)octeon->base >> 24) & 1,
> +					power_gpio);

Let's not cast it like this. It's not readable. Make the logic
intentional and clear:
e.g.: int index = !!(octeon->base & BIT(24));
dwc3_octeon_config_gpio(index, power_gpio);

It's odd that the "index" is being used as boolean here.

Regardless, I don't know what this magic offset BIT(24) means. If
there's some context, then you can refactor the
dwc3_octeon_config_gpio() as below:

dwc3_octeon_config_gpio(power_gpio, is_bit24) where "is_bit24" is some
other meaningful boolean name.


> +		dev_dbg(dev, "power control is using gpio%d\n", power_gpio);
> +	}
> +	if (power_active_low)
> +		val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> +	else
> +		val |= USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> +	dwc3_octeon_writeq(uctl_host_cfg_reg, val);
>  
>  	/* Step 8d: Deassert UAHC reset signal. */
>  	val = dwc3_octeon_readq(uctl_ctl_reg);
> @@ -508,7 +483,28 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *node = dev->of_node;
>  	struct dwc3_octeon *octeon;
> -	int err;
> +	int power_active_low, power_gpio;
> +	int err, len;
> +
> +	power_gpio = DWC3_GPIO_POWER_NONE;
> +	power_active_low = 0;
> +	if (of_find_property(node, "power", &len)) {
> +		u32 gpio_pwr[3];
> +
> +		switch (len) {
> +		case 8:
> +			of_property_read_u32_array(node, "power", gpio_pwr, 2);
> +			break;
> +		case 12:
> +			of_property_read_u32_array(node, "power", gpio_pwr, 3);
> +			power_active_low = gpio_pwr[2] & 0x01;

It would be better for these magic numbers (e.g. 0x01) to be written in
macros or at least documented in the future. That update can be done in
a separate commit in the future.

> +			break;
> +		default:
> +			dev_err(dev, "invalid power configuration\n");
> +			return -EINVAL;
> +		}
> +		power_gpio = gpio_pwr[1];
> +	}
>  
>  	octeon = devm_kzalloc(dev, sizeof(*octeon), GFP_KERNEL);
>  	if (!octeon)
> @@ -519,7 +515,7 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
>  	if (IS_ERR(octeon->base))
>  		return PTR_ERR(octeon->base);
>  
> -	err = dwc3_octeon_clocks_start(octeon);
> +	err = dwc3_octeon_setup(octeon, power_gpio, power_active_low);
>  	if (err)
>  		return err;
>  
> -- 
> 2.39.2
> 

Thanks,
Thinh

Re: [PATCH v5 4/7] usb: dwc3: dwc3-octeon: Avoid half-initialized controller state

[Ladislav Michl]

On Tue, Aug 01, 2023 at 12:38:43AM +0000, Thinh Nguyen wrote:
> On Mon, Jul 31, 2023, Ladislav Michl wrote:
> > From: Ladislav Michl <ladis@linux-mips.org>
> > 
> > Power gpio configuration is done from the middle of
> > dwc3_octeon_clocks_start leaving hardware in half-initialized
> > state if it fails. As that indicates dwc3_octeon_clocks_start
> > does more than just initialize the clocks rename it appropriately
> > and verify power gpio configuration in advance at the beginning
> > of device probe.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  CHANGES:
> >  - v4: new patch
> >  - v5: use uintptr_t instead of u64 to retype base address to make 32bit
> >        compilers happy.
> > 
> >  drivers/usb/dwc3/dwc3-octeon.c | 90 ++++++++++++++++------------------
> >  1 file changed, 43 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> > index 24e75881b5cf..0dc45dda134c 100644
> > --- a/drivers/usb/dwc3/dwc3-octeon.c
> > +++ b/drivers/usb/dwc3/dwc3-octeon.c
> > @@ -192,6 +192,8 @@ struct dwc3_octeon {
> >  	void __iomem *base;
> >  };
> >  
> > +#define DWC3_GPIO_POWER_NONE	(-1)
> > +
> >  #ifdef CONFIG_CAVIUM_OCTEON_SOC
> >  #include <asm/octeon/octeon.h>
> >  static inline uint64_t dwc3_octeon_readq(void __iomem *addr)
> > @@ -258,55 +260,15 @@ static int dwc3_octeon_get_divider(void)
> >  	return div;
> >  }
> >  
> > -static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)
> > -{
> > -	uint32_t gpio_pwr[3];
> > -	int gpio, len, power_active_low;
> > -	struct device_node *node = dev->of_node;
> > -	u64 val;
> > -	void __iomem *uctl_host_cfg_reg = base + USBDRD_UCTL_HOST_CFG;
> > -
> > -	if (of_find_property(node, "power", &len) != NULL) {
> > -		if (len == 12) {
> > -			of_property_read_u32_array(node, "power", gpio_pwr, 3);
> > -			power_active_low = gpio_pwr[2] & 0x01;
> > -			gpio = gpio_pwr[1];
> > -		} else if (len == 8) {
> > -			of_property_read_u32_array(node, "power", gpio_pwr, 2);
> > -			power_active_low = 0;
> > -			gpio = gpio_pwr[1];
> > -		} else {
> > -			dev_err(dev, "invalid power configuration\n");
> > -			return -EINVAL;
> > -		}
> > -		dwc3_octeon_config_gpio(((u64)base >> 24) & 1, gpio);
> > -
> > -		/* Enable XHCI power control and set if active high or low. */
> > -		val = dwc3_octeon_readq(uctl_host_cfg_reg);
> > -		val |= USBDRD_UCTL_HOST_PPC_EN;
> > -		if (power_active_low)
> > -			val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > -		else
> > -			val |= USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > -		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
> > -	} else {
> > -		/* Disable XHCI power control and set if active high. */
> > -		val = dwc3_octeon_readq(uctl_host_cfg_reg);
> > -		val &= ~USBDRD_UCTL_HOST_PPC_EN;
> > -		val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > -		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
> > -		dev_info(dev, "power control disabled\n");
> > -	}
> > -	return 0;
> > -}
> > -
> > -static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
> > +static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
> > +			     int power_gpio, int power_active_low)
> >  {
> >  	int i, div, mpll_mul, ref_clk_fsel, ref_clk_sel = 2;
> >  	u32 clock_rate;
> >  	u64 val;
> >  	struct device *dev = octeon->dev;
> >  	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
> > +	void __iomem *uctl_host_cfg_reg = octeon->base + USBDRD_UCTL_HOST_CFG;
> >  
> >  	if (dev->of_node) {
> >  		const char *ss_clock_type;
> > @@ -454,8 +416,21 @@ static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
> >  	udelay(10);
> >  
> >  	/* Step 8c: Setup power control. */
> > -	if (dwc3_octeon_config_power(dev, octeon->base))
> > -		return -EINVAL;
> > +	val = dwc3_octeon_readq(uctl_host_cfg_reg);
> > +	val |= USBDRD_UCTL_HOST_PPC_EN;
> > +	if (power_gpio == DWC3_GPIO_POWER_NONE) {
> > +		val &= ~USBDRD_UCTL_HOST_PPC_EN;
> > +	} else {
> > +		val |= USBDRD_UCTL_HOST_PPC_EN;
> > +		dwc3_octeon_config_gpio(((__force uintptr_t)octeon->base >> 24) & 1,
> > +					power_gpio);
> 
> Let's not cast it like this. It's not readable. Make the logic
> intentional and clear:
> e.g.: int index = !!(octeon->base & BIT(24));
> dwc3_octeon_config_gpio(index, power_gpio);

I'd prefer to stick with original code.

> It's odd that the "index" is being used as boolean here.
>
> Regardless, I don't know what this magic offset BIT(24) means. If
> there's some context, then you can refactor the
> dwc3_octeon_config_gpio() as below:

Context is a bit scary and perhaps could be documented as described later.

> dwc3_octeon_config_gpio(power_gpio, is_bit24) where "is_bit24" is some
> other meaningful boolean name.

As there is no pinctrl driver for octeon, configuration is done here.
There are two UCTLs: at 0x1180068000000 and second at 0x1180069000000.
We are just using bit 24 to distiguish between them. No matter how you
rewrite this function, it is still horrible hack and making it "nice"
does not solve anything. For that reason I stick with original code as
there is no point touching anything that just should not exist.

Once Octeon gets its pinctlr driver, this function will disapear
altogether. The very same is true for clock parsing - there is no clk api.

But note that might as well never happen as documentation is under NDA
and I have it only for single SoC as well as I have only single SoC
available for testing, so it is quite hard to write proper drivers
without breaking anything.

Anyway, what about just passing octeon into dwc3_octeon_config_gpio
and use all that dirty magic inside. Would that work work for you?

> > +		dev_dbg(dev, "power control is using gpio%d\n", power_gpio);
> > +	}
> > +	if (power_active_low)
> > +		val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > +	else
> > +		val |= USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > +	dwc3_octeon_writeq(uctl_host_cfg_reg, val);
> >  
> >  	/* Step 8d: Deassert UAHC reset signal. */
> >  	val = dwc3_octeon_readq(uctl_ctl_reg);
> > @@ -508,7 +483,28 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *node = dev->of_node;
> >  	struct dwc3_octeon *octeon;
> > -	int err;
> > +	int power_active_low, power_gpio;
> > +	int err, len;
> > +
> > +	power_gpio = DWC3_GPIO_POWER_NONE;
> > +	power_active_low = 0;
> > +	if (of_find_property(node, "power", &len)) {
> > +		u32 gpio_pwr[3];
> > +
> > +		switch (len) {
> > +		case 8:
> > +			of_property_read_u32_array(node, "power", gpio_pwr, 2);
> > +			break;
> > +		case 12:
> > +			of_property_read_u32_array(node, "power", gpio_pwr, 3);
> > +			power_active_low = gpio_pwr[2] & 0x01;
> 
> It would be better for these magic numbers (e.g. 0x01) to be written in
> macros or at least documented in the future. That update can be done in
> a separate commit in the future.

Sure. In the future, this should just wanish as noted above.

> > +			break;
> > +		default:
> > +			dev_err(dev, "invalid power configuration\n");
> > +			return -EINVAL;
> > +		}
> > +		power_gpio = gpio_pwr[1];
> > +	}
> >  
> >  	octeon = devm_kzalloc(dev, sizeof(*octeon), GFP_KERNEL);
> >  	if (!octeon)
> > @@ -519,7 +515,7 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
> >  	if (IS_ERR(octeon->base))
> >  		return PTR_ERR(octeon->base);
> >  
> > -	err = dwc3_octeon_clocks_start(octeon);
> > +	err = dwc3_octeon_setup(octeon, power_gpio, power_active_low);
> >  	if (err)
> >  		return err;
> >  
> > -- 
> > 2.39.2
> > 
> 
> Thanks,
> Thinh

Re: [PATCH v5 4/7] usb: dwc3: dwc3-octeon: Avoid half-initialized controller state

[Ladislav Michl]

On Tue, Aug 01, 2023 at 07:37:37AM +0200, Ladislav Michl wrote:
> Anyway, what about just passing octeon into dwc3_octeon_config_gpio
> and use all that dirty magic inside. Would that work work for you?

Something like this:

[PATCH] usb: dwc3: dwc3-octeon: Consolidate pinmux configuration

As there is no pinctrl driver for Octeon, pinmux configuration is done
in dwc3_octeon_config_gpio function. It has been always done the tricky
way: there are two UCTLs; first at 0x1180068000000 and second at
0x1180069000000, so address based test is used to get index to configure
pin muxing, because DT does not provide that information.

To make pinmux configuration a little less hackish until proper solution
is developed, move all its logic into dwc3_octeon_config_gpio function.
---
 drivers/usb/dwc3/dwc3-octeon.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index 90e1ae66769f..f35dca899d6e 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -206,9 +206,10 @@ static inline void dwc3_octeon_writeq(void __iomem *base, uint64_t val)
 	cvmx_writeq_csr(base, val);
 }
 
-static void dwc3_octeon_config_gpio(int index, int gpio)
+static void dwc3_octeon_config_gpio(struct dwc3_octeon *octeon, int gpio)
 {
 	union cvmx_gpio_bit_cfgx gpio_bit;
+	int index = ((__force uintptr_t)octeon->base >> 24) & 1;
 
 	if ((OCTEON_IS_MODEL(OCTEON_CN73XX) ||
 	    OCTEON_IS_MODEL(OCTEON_CNF75XX))
@@ -237,7 +238,7 @@ static inline uint64_t dwc3_octeon_readq(void __iomem *addr)
 
 static inline void dwc3_octeon_writeq(void __iomem *base, uint64_t val) { }
 
-static inline void dwc3_octeon_config_gpio(int index, int gpio) { }
+static inline void dwc3_octeon_config_gpio(struct dwc3_octeon *octeon, int gpio) { }
 
 static uint64_t octeon_get_io_clock_rate(void)
 {
@@ -422,7 +423,7 @@ static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
 		val &= ~USBDRD_UCTL_HOST_PPC_EN;
 	} else {
 		val |= USBDRD_UCTL_HOST_PPC_EN;
-		dwc3_octeon_config_gpio(((u64)octeon->base >> 24) & 1, power_gpio);
+		dwc3_octeon_config_gpio(octeon, power_gpio);
 		dev_dbg(dev, "power control is using gpio%d\n", power_gpio);
 	}
 	if (power_active_low)
-- 
2.39.2

Re: [PATCH v5 4/7] usb: dwc3: dwc3-octeon: Avoid half-initialized controller state

[Thinh Nguyen]

On Tue, Aug 01, 2023, Ladislav Michl wrote:
> On Tue, Aug 01, 2023 at 07:37:37AM +0200, Ladislav Michl wrote:
> > Anyway, what about just passing octeon into dwc3_octeon_config_gpio
> > and use all that dirty magic inside. Would that work work for you?
> 
> Something like this:
> 
> [PATCH] usb: dwc3: dwc3-octeon: Consolidate pinmux configuration
> 
> As there is no pinctrl driver for Octeon, pinmux configuration is done
> in dwc3_octeon_config_gpio function. It has been always done the tricky
> way: there are two UCTLs; first at 0x1180068000000 and second at
> 0x1180069000000, so address based test is used to get index to configure
> pin muxing, because DT does not provide that information.
> 
> To make pinmux configuration a little less hackish until proper solution
> is developed, move all its logic into dwc3_octeon_config_gpio function.
> ---
>  drivers/usb/dwc3/dwc3-octeon.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index 90e1ae66769f..f35dca899d6e 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -206,9 +206,10 @@ static inline void dwc3_octeon_writeq(void __iomem *base, uint64_t val)
>  	cvmx_writeq_csr(base, val);
>  }
>  
> -static void dwc3_octeon_config_gpio(int index, int gpio)
> +static void dwc3_octeon_config_gpio(struct dwc3_octeon *octeon, int gpio)
>  {
>  	union cvmx_gpio_bit_cfgx gpio_bit;
> +	int index = ((__force uintptr_t)octeon->base >> 24) & 1;
>  
>  	if ((OCTEON_IS_MODEL(OCTEON_CN73XX) ||
>  	    OCTEON_IS_MODEL(OCTEON_CNF75XX))
> @@ -237,7 +238,7 @@ static inline uint64_t dwc3_octeon_readq(void __iomem *addr)
>  
>  static inline void dwc3_octeon_writeq(void __iomem *base, uint64_t val) { }
>  
> -static inline void dwc3_octeon_config_gpio(int index, int gpio) { }
> +static inline void dwc3_octeon_config_gpio(struct dwc3_octeon *octeon, int gpio) { }
>  
>  static uint64_t octeon_get_io_clock_rate(void)
>  {
> @@ -422,7 +423,7 @@ static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
>  		val &= ~USBDRD_UCTL_HOST_PPC_EN;
>  	} else {
>  		val |= USBDRD_UCTL_HOST_PPC_EN;
> -		dwc3_octeon_config_gpio(((u64)octeon->base >> 24) & 1, power_gpio);
> +		dwc3_octeon_config_gpio(octeon, power_gpio);
>  		dev_dbg(dev, "power control is using gpio%d\n", power_gpio);
>  	}
>  	if (power_active_low)
> -- 
> 2.39.2
> 

No, it doesn't make it any better. Let's revisit this later.

Thanks,
Thinh

Re: [PATCH v5 4/7] usb: dwc3: dwc3-octeon: Avoid half-initialized controller state

[Thinh Nguyen]

On Tue, Aug 01, 2023, Ladislav Michl wrote:
> On Tue, Aug 01, 2023 at 12:38:43AM +0000, Thinh Nguyen wrote:
> > On Mon, Jul 31, 2023, Ladislav Michl wrote:
> > > From: Ladislav Michl <ladis@linux-mips.org>
> > > 
> > > Power gpio configuration is done from the middle of
> > > dwc3_octeon_clocks_start leaving hardware in half-initialized
> > > state if it fails. As that indicates dwc3_octeon_clocks_start
> > > does more than just initialize the clocks rename it appropriately
> > > and verify power gpio configuration in advance at the beginning
> > > of device probe.
> > > 
> > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > > ---
> > >  CHANGES:
> > >  - v4: new patch
> > >  - v5: use uintptr_t instead of u64 to retype base address to make 32bit
> > >        compilers happy.
> > > 
> > >  drivers/usb/dwc3/dwc3-octeon.c | 90 ++++++++++++++++------------------
> > >  1 file changed, 43 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> > > index 24e75881b5cf..0dc45dda134c 100644
> > > --- a/drivers/usb/dwc3/dwc3-octeon.c
> > > +++ b/drivers/usb/dwc3/dwc3-octeon.c
> > > @@ -192,6 +192,8 @@ struct dwc3_octeon {
> > >  	void __iomem *base;
> > >  };
> > >  
> > > +#define DWC3_GPIO_POWER_NONE	(-1)
> > > +
> > >  #ifdef CONFIG_CAVIUM_OCTEON_SOC
> > >  #include <asm/octeon/octeon.h>
> > >  static inline uint64_t dwc3_octeon_readq(void __iomem *addr)
> > > @@ -258,55 +260,15 @@ static int dwc3_octeon_get_divider(void)
> > >  	return div;
> > >  }
> > >  
> > > -static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)
> > > -{
> > > -	uint32_t gpio_pwr[3];
> > > -	int gpio, len, power_active_low;
> > > -	struct device_node *node = dev->of_node;
> > > -	u64 val;
> > > -	void __iomem *uctl_host_cfg_reg = base + USBDRD_UCTL_HOST_CFG;
> > > -
> > > -	if (of_find_property(node, "power", &len) != NULL) {
> > > -		if (len == 12) {
> > > -			of_property_read_u32_array(node, "power", gpio_pwr, 3);
> > > -			power_active_low = gpio_pwr[2] & 0x01;
> > > -			gpio = gpio_pwr[1];
> > > -		} else if (len == 8) {
> > > -			of_property_read_u32_array(node, "power", gpio_pwr, 2);
> > > -			power_active_low = 0;
> > > -			gpio = gpio_pwr[1];
> > > -		} else {
> > > -			dev_err(dev, "invalid power configuration\n");
> > > -			return -EINVAL;
> > > -		}
> > > -		dwc3_octeon_config_gpio(((u64)base >> 24) & 1, gpio);
> > > -
> > > -		/* Enable XHCI power control and set if active high or low. */
> > > -		val = dwc3_octeon_readq(uctl_host_cfg_reg);
> > > -		val |= USBDRD_UCTL_HOST_PPC_EN;
> > > -		if (power_active_low)
> > > -			val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > > -		else
> > > -			val |= USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > > -		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
> > > -	} else {
> > > -		/* Disable XHCI power control and set if active high. */
> > > -		val = dwc3_octeon_readq(uctl_host_cfg_reg);
> > > -		val &= ~USBDRD_UCTL_HOST_PPC_EN;
> > > -		val &= ~USBDRD_UCTL_HOST_PPC_ACTIVE_HIGH_EN;
> > > -		dwc3_octeon_writeq(uctl_host_cfg_reg, val);
> > > -		dev_info(dev, "power control disabled\n");
> > > -	}
> > > -	return 0;
> > > -}
> > > -
> > > -static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
> > > +static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
> > > +			     int power_gpio, int power_active_low)
> > >  {
> > >  	int i, div, mpll_mul, ref_clk_fsel, ref_clk_sel = 2;
> > >  	u32 clock_rate;
> > >  	u64 val;
> > >  	struct device *dev = octeon->dev;
> > >  	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
> > > +	void __iomem *uctl_host_cfg_reg = octeon->base + USBDRD_UCTL_HOST_CFG;
> > >  
> > >  	if (dev->of_node) {
> > >  		const char *ss_clock_type;
> > > @@ -454,8 +416,21 @@ static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon)
> > >  	udelay(10);
> > >  
> > >  	/* Step 8c: Setup power control. */
> > > -	if (dwc3_octeon_config_power(dev, octeon->base))
> > > -		return -EINVAL;
> > > +	val = dwc3_octeon_readq(uctl_host_cfg_reg);
> > > +	val |= USBDRD_UCTL_HOST_PPC_EN;
> > > +	if (power_gpio == DWC3_GPIO_POWER_NONE) {
> > > +		val &= ~USBDRD_UCTL_HOST_PPC_EN;
> > > +	} else {
> > > +		val |= USBDRD_UCTL_HOST_PPC_EN;
> > > +		dwc3_octeon_config_gpio(((__force uintptr_t)octeon->base >> 24) & 1,
> > > +					power_gpio);
> > 
> > Let's not cast it like this. It's not readable. Make the logic
> > intentional and clear:
> > e.g.: int index = !!(octeon->base & BIT(24));
> > dwc3_octeon_config_gpio(index, power_gpio);
> 
> I'd prefer to stick with original code.

I didn't know the reasoning behind the original code. My example would
only apply if the logic is meant for a certain context.

> 
> > It's odd that the "index" is being used as boolean here.
> >
> > Regardless, I don't know what this magic offset BIT(24) means. If
> > there's some context, then you can refactor the
> > dwc3_octeon_config_gpio() as below:
> 
> Context is a bit scary and perhaps could be documented as described later.
> 
> > dwc3_octeon_config_gpio(power_gpio, is_bit24) where "is_bit24" is some
> > other meaningful boolean name.
> 
> As there is no pinctrl driver for octeon, configuration is done here.
> There are two UCTLs: at 0x1180068000000 and second at 0x1180069000000.
> We are just using bit 24 to distiguish between them. No matter how you

This is the context I wanted to check as it's not obvious from the
original code.

> rewrite this function, it is still horrible hack and making it "nice"
> does not solve anything. For that reason I stick with original code as
> there is no point touching anything that just should not exist.

If you were able to explain it to me, I think it's not impossible to
make this clearer in the code/document. But we can leave that for the
future.

> 
> Once Octeon gets its pinctlr driver, this function will disapear
> altogether. The very same is true for clock parsing - there is no clk api.
> 
> But note that might as well never happen as documentation is under NDA
> and I have it only for single SoC as well as I have only single SoC
> available for testing, so it is quite hard to write proper drivers
> without breaking anything.
> 
> Anyway, what about just passing octeon into dwc3_octeon_config_gpio
> and use all that dirty magic inside. Would that work work for you?
> 

For this series, we can take in the original code as they are more about
moving the code.

Thanks,
Thinh

Re: [PATCH v5 4/7] usb: dwc3: dwc3-octeon: Avoid half-initialized controller state

[Thinh Nguyen]

On Mon, Jul 31, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Power gpio configuration is done from the middle of
> dwc3_octeon_clocks_start leaving hardware in half-initialized
> state if it fails. As that indicates dwc3_octeon_clocks_start
> does more than just initialize the clocks rename it appropriately
> and verify power gpio configuration in advance at the beginning
> of device probe.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  CHANGES:
>  - v4: new patch
>  - v5: use uintptr_t instead of u64 to retype base address to make 32bit
>        compilers happy.
> 
>  drivers/usb/dwc3/dwc3-octeon.c | 90 ++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index 24e75881b5cf..0dc45dda134c 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -192,6 +192,8 @@ struct dwc3_octeon {
>  	void __iomem *base;
>  };
>  

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

[PATCH v5 5/7] usb: dwc3: dwc3-octeon: Move node parsing into driver probe

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

Parse and verify device tree node first, then setup UCTL bridge
using verified values. This avoids needless allocations
in case DT misconfiguration was found in the middle of setup.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 CHANGES:
 - v2: if else block bracket according CodingStyle
 - v3: more descriptive commit message, power gpio parsing in probe too,
       checkpatch --strict passed
 - v4: move changes unrelated to parsing move into separate patches
 - v5: none

 drivers/usb/dwc3/dwc3-octeon.c | 135 +++++++++++++++------------------
 1 file changed, 60 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index 0dc45dda134c..330bcb59cc95 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -261,69 +261,15 @@ static int dwc3_octeon_get_divider(void)
 }
 
 static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
+			     int ref_clk_sel, int ref_clk_fsel, int mpll_mul,
 			     int power_gpio, int power_active_low)
 {
-	int i, div, mpll_mul, ref_clk_fsel, ref_clk_sel = 2;
-	u32 clock_rate;
 	u64 val;
+	int div;
 	struct device *dev = octeon->dev;
 	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
 	void __iomem *uctl_host_cfg_reg = octeon->base + USBDRD_UCTL_HOST_CFG;
 
-	if (dev->of_node) {
-		const char *ss_clock_type;
-		const char *hs_clock_type;
-
-		i = of_property_read_u32(dev->of_node,
-					 "refclk-frequency", &clock_rate);
-		if (i) {
-			dev_err(dev, "No UCTL \"refclk-frequency\"\n");
-			return -EINVAL;
-		}
-		i = of_property_read_string(dev->of_node,
-					    "refclk-type-ss", &ss_clock_type);
-		if (i) {
-			dev_err(dev, "No UCTL \"refclk-type-ss\"\n");
-			return -EINVAL;
-		}
-		i = of_property_read_string(dev->of_node,
-					    "refclk-type-hs", &hs_clock_type);
-		if (i) {
-			dev_err(dev, "No UCTL \"refclk-type-hs\"\n");
-			return -EINVAL;
-		}
-		if (strcmp("dlmc_ref_clk0", ss_clock_type) == 0) {
-			if (strcmp(hs_clock_type, "dlmc_ref_clk0") == 0)
-				ref_clk_sel = 0;
-			else if (strcmp(hs_clock_type, "pll_ref_clk") == 0)
-				ref_clk_sel = 2;
-			else
-				dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
-					 hs_clock_type);
-		} else if (strcmp(ss_clock_type, "dlmc_ref_clk1") == 0) {
-			if (strcmp(hs_clock_type, "dlmc_ref_clk1") == 0)
-				ref_clk_sel = 1;
-			else if (strcmp(hs_clock_type, "pll_ref_clk") == 0)
-				ref_clk_sel = 3;
-			else {
-				dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
-					 hs_clock_type);
-				ref_clk_sel = 3;
-			}
-		} else
-			dev_warn(dev, "Invalid SS clock type %s, using dlmc_ref_clk0 instead\n",
-				 ss_clock_type);
-
-		if ((ref_clk_sel == 0 || ref_clk_sel == 1) &&
-		    (clock_rate != 100000000))
-			dev_warn(dev, "Invalid UCTL clock rate of %u, using 100000000 instead\n",
-				 clock_rate);
-
-	} else {
-		dev_err(dev, "No USB UCTL device node\n");
-		return -EINVAL;
-	}
-
 	/*
 	 * Step 1: Wait for all voltages to be stable...that surely
 	 *         happened before starting the kernel. SKIP
@@ -367,24 +313,6 @@ static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
 	val &= ~USBDRD_UCTL_CTL_REF_CLK_SEL;
 	val |= FIELD_PREP(USBDRD_UCTL_CTL_REF_CLK_SEL, ref_clk_sel);
 
-	ref_clk_fsel = 0x07;
-	switch (clock_rate) {
-	default:
-		dev_warn(dev, "Invalid ref_clk %u, using 100000000 instead\n",
-			 clock_rate);
-		fallthrough;
-	case 100000000:
-		mpll_mul = 0x19;
-		if (ref_clk_sel < 2)
-			ref_clk_fsel = 0x27;
-		break;
-	case 50000000:
-		mpll_mul = 0x32;
-		break;
-	case 125000000:
-		mpll_mul = 0x28;
-		break;
-	}
 	val &= ~USBDRD_UCTL_CTL_REF_CLK_FSEL;
 	val |= FIELD_PREP(USBDRD_UCTL_CTL_REF_CLK_FSEL, ref_clk_fsel);
 
@@ -483,8 +411,64 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 	struct dwc3_octeon *octeon;
+	const char *hs_clock_type, *ss_clock_type;
+	int ref_clk_sel, ref_clk_fsel, mpll_mul;
 	int power_active_low, power_gpio;
 	int err, len;
+	u32 clock_rate;
+
+	if (of_property_read_u32(node, "refclk-frequency", &clock_rate)) {
+		dev_err(dev, "No UCTL \"refclk-frequency\"\n");
+		return -EINVAL;
+	}
+	if (of_property_read_string(node, "refclk-type-ss", &ss_clock_type)) {
+		dev_err(dev, "No UCTL \"refclk-type-ss\"\n");
+		return -EINVAL;
+	}
+	if (of_property_read_string(node, "refclk-type-hs", &hs_clock_type)) {
+		dev_err(dev, "No UCTL \"refclk-type-hs\"\n");
+		return -EINVAL;
+	}
+
+	ref_clk_sel = 2;
+	if (strcmp("dlmc_ref_clk0", ss_clock_type) == 0) {
+		if (strcmp(hs_clock_type, "dlmc_ref_clk0") == 0)
+			ref_clk_sel = 0;
+		else if (strcmp(hs_clock_type, "pll_ref_clk"))
+			dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
+				 hs_clock_type);
+	} else if (strcmp(ss_clock_type, "dlmc_ref_clk1") == 0) {
+		if (strcmp(hs_clock_type, "dlmc_ref_clk1") == 0) {
+			ref_clk_sel = 1;
+		} else {
+			ref_clk_sel = 3;
+			if (strcmp(hs_clock_type, "pll_ref_clk"))
+				dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
+					 hs_clock_type);
+		}
+	} else {
+		dev_warn(dev, "Invalid SS clock type %s, using dlmc_ref_clk0 instead\n",
+			 ss_clock_type);
+	}
+
+	ref_clk_fsel = 0x07;
+	switch (clock_rate) {
+	default:
+		dev_warn(dev, "Invalid ref_clk %u, using 100000000 instead\n",
+			 clock_rate);
+		fallthrough;
+	case 100000000:
+		mpll_mul = 0x19;
+		if (ref_clk_sel < 2)
+			ref_clk_fsel = 0x27;
+		break;
+	case 50000000:
+		mpll_mul = 0x32;
+		break;
+	case 125000000:
+		mpll_mul = 0x28;
+		break;
+	}
 
 	power_gpio = DWC3_GPIO_POWER_NONE;
 	power_active_low = 0;
@@ -515,7 +499,8 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
 	if (IS_ERR(octeon->base))
 		return PTR_ERR(octeon->base);
 
-	err = dwc3_octeon_setup(octeon, power_gpio, power_active_low);
+	err = dwc3_octeon_setup(octeon, ref_clk_sel, ref_clk_fsel, mpll_mul,
+				power_gpio, power_active_low);
 	if (err)
 		return err;
 
-- 
2.39.2

Re: [PATCH v5 5/7] usb: dwc3: dwc3-octeon: Move node parsing into driver probe

[Thinh Nguyen]

On Mon, Jul 31, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Parse and verify device tree node first, then setup UCTL bridge
> using verified values. This avoids needless allocations
> in case DT misconfiguration was found in the middle of setup.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  CHANGES:
>  - v2: if else block bracket according CodingStyle
>  - v3: more descriptive commit message, power gpio parsing in probe too,
>        checkpatch --strict passed
>  - v4: move changes unrelated to parsing move into separate patches
>  - v5: none
> 
>  drivers/usb/dwc3/dwc3-octeon.c | 135 +++++++++++++++------------------
>  1 file changed, 60 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index 0dc45dda134c..330bcb59cc95 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -261,69 +261,15 @@ static int dwc3_octeon_get_divider(void)
>  }
>  
>  static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
> +			     int ref_clk_sel, int ref_clk_fsel, int mpll_mul,
>  			     int power_gpio, int power_active_low)
>  {
> -	int i, div, mpll_mul, ref_clk_fsel, ref_clk_sel = 2;
> -	u32 clock_rate;
>  	u64 val;
> +	int div;
>  	struct device *dev = octeon->dev;
>  	void __iomem *uctl_ctl_reg = octeon->base + USBDRD_UCTL_CTL;
>  	void __iomem *uctl_host_cfg_reg = octeon->base + USBDRD_UCTL_HOST_CFG;
>  
> -	if (dev->of_node) {
> -		const char *ss_clock_type;
> -		const char *hs_clock_type;
> -
> -		i = of_property_read_u32(dev->of_node,
> -					 "refclk-frequency", &clock_rate);
> -		if (i) {
> -			dev_err(dev, "No UCTL \"refclk-frequency\"\n");
> -			return -EINVAL;
> -		}
> -		i = of_property_read_string(dev->of_node,
> -					    "refclk-type-ss", &ss_clock_type);
> -		if (i) {
> -			dev_err(dev, "No UCTL \"refclk-type-ss\"\n");
> -			return -EINVAL;
> -		}
> -		i = of_property_read_string(dev->of_node,
> -					    "refclk-type-hs", &hs_clock_type);
> -		if (i) {
> -			dev_err(dev, "No UCTL \"refclk-type-hs\"\n");
> -			return -EINVAL;
> -		}
> -		if (strcmp("dlmc_ref_clk0", ss_clock_type) == 0) {
> -			if (strcmp(hs_clock_type, "dlmc_ref_clk0") == 0)
> -				ref_clk_sel = 0;
> -			else if (strcmp(hs_clock_type, "pll_ref_clk") == 0)
> -				ref_clk_sel = 2;
> -			else
> -				dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
> -					 hs_clock_type);
> -		} else if (strcmp(ss_clock_type, "dlmc_ref_clk1") == 0) {
> -			if (strcmp(hs_clock_type, "dlmc_ref_clk1") == 0)
> -				ref_clk_sel = 1;
> -			else if (strcmp(hs_clock_type, "pll_ref_clk") == 0)
> -				ref_clk_sel = 3;
> -			else {
> -				dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
> -					 hs_clock_type);
> -				ref_clk_sel = 3;
> -			}
> -		} else
> -			dev_warn(dev, "Invalid SS clock type %s, using dlmc_ref_clk0 instead\n",
> -				 ss_clock_type);
> -
> -		if ((ref_clk_sel == 0 || ref_clk_sel == 1) &&
> -		    (clock_rate != 100000000))
> -			dev_warn(dev, "Invalid UCTL clock rate of %u, using 100000000 instead\n",
> -				 clock_rate);
> -
> -	} else {
> -		dev_err(dev, "No USB UCTL device node\n");
> -		return -EINVAL;
> -	}
> -
>  	/*
>  	 * Step 1: Wait for all voltages to be stable...that surely
>  	 *         happened before starting the kernel. SKIP
> @@ -367,24 +313,6 @@ static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
>  	val &= ~USBDRD_UCTL_CTL_REF_CLK_SEL;
>  	val |= FIELD_PREP(USBDRD_UCTL_CTL_REF_CLK_SEL, ref_clk_sel);
>  
> -	ref_clk_fsel = 0x07;
> -	switch (clock_rate) {
> -	default:
> -		dev_warn(dev, "Invalid ref_clk %u, using 100000000 instead\n",
> -			 clock_rate);
> -		fallthrough;
> -	case 100000000:
> -		mpll_mul = 0x19;
> -		if (ref_clk_sel < 2)
> -			ref_clk_fsel = 0x27;
> -		break;
> -	case 50000000:
> -		mpll_mul = 0x32;
> -		break;
> -	case 125000000:
> -		mpll_mul = 0x28;
> -		break;
> -	}
>  	val &= ~USBDRD_UCTL_CTL_REF_CLK_FSEL;
>  	val |= FIELD_PREP(USBDRD_UCTL_CTL_REF_CLK_FSEL, ref_clk_fsel);
>  
> @@ -483,8 +411,64 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *node = dev->of_node;
>  	struct dwc3_octeon *octeon;
> +	const char *hs_clock_type, *ss_clock_type;
> +	int ref_clk_sel, ref_clk_fsel, mpll_mul;
>  	int power_active_low, power_gpio;
>  	int err, len;
> +	u32 clock_rate;
> +
> +	if (of_property_read_u32(node, "refclk-frequency", &clock_rate)) {
> +		dev_err(dev, "No UCTL \"refclk-frequency\"\n");
> +		return -EINVAL;
> +	}
> +	if (of_property_read_string(node, "refclk-type-ss", &ss_clock_type)) {
> +		dev_err(dev, "No UCTL \"refclk-type-ss\"\n");
> +		return -EINVAL;
> +	}
> +	if (of_property_read_string(node, "refclk-type-hs", &hs_clock_type)) {
> +		dev_err(dev, "No UCTL \"refclk-type-hs\"\n");
> +		return -EINVAL;
> +	}
> +
> +	ref_clk_sel = 2;
> +	if (strcmp("dlmc_ref_clk0", ss_clock_type) == 0) {
> +		if (strcmp(hs_clock_type, "dlmc_ref_clk0") == 0)
> +			ref_clk_sel = 0;
> +		else if (strcmp(hs_clock_type, "pll_ref_clk"))
> +			dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
> +				 hs_clock_type);
> +	} else if (strcmp(ss_clock_type, "dlmc_ref_clk1") == 0) {
> +		if (strcmp(hs_clock_type, "dlmc_ref_clk1") == 0) {
> +			ref_clk_sel = 1;
> +		} else {
> +			ref_clk_sel = 3;
> +			if (strcmp(hs_clock_type, "pll_ref_clk"))
> +				dev_warn(dev, "Invalid HS clock type %s, using pll_ref_clk instead\n",
> +					 hs_clock_type);
> +		}
> +	} else {
> +		dev_warn(dev, "Invalid SS clock type %s, using dlmc_ref_clk0 instead\n",
> +			 ss_clock_type);
> +	}
> +
> +	ref_clk_fsel = 0x07;
> +	switch (clock_rate) {
> +	default:
> +		dev_warn(dev, "Invalid ref_clk %u, using 100000000 instead\n",
> +			 clock_rate);
> +		fallthrough;
> +	case 100000000:
> +		mpll_mul = 0x19;
> +		if (ref_clk_sel < 2)
> +			ref_clk_fsel = 0x27;
> +		break;
> +	case 50000000:
> +		mpll_mul = 0x32;
> +		break;
> +	case 125000000:
> +		mpll_mul = 0x28;
> +		break;
> +	}
>  
>  	power_gpio = DWC3_GPIO_POWER_NONE;
>  	power_active_low = 0;
> @@ -515,7 +499,8 @@ static int dwc3_octeon_probe(struct platform_device *pdev)
>  	if (IS_ERR(octeon->base))
>  		return PTR_ERR(octeon->base);
>  
> -	err = dwc3_octeon_setup(octeon, power_gpio, power_active_low);
> +	err = dwc3_octeon_setup(octeon, ref_clk_sel, ref_clk_fsel, mpll_mul,
> +				power_gpio, power_active_low);
>  	if (err)
>  		return err;
>  
> -- 
> 2.39.2
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

[PATCH v5 6/7] usb: dwc3: dwc3-octeon: Dump control register on clock init failure

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

It might be interesting to know control register value in case
clock fails to enable.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 CHANGES:
 - v4: new patch
 - v5: Philippe's review tag

 drivers/usb/dwc3/dwc3-octeon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index 330bcb59cc95..d578110f7afb 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -299,8 +299,8 @@ static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
 	val = dwc3_octeon_readq(uctl_ctl_reg);
 	if ((div != FIELD_GET(USBDRD_UCTL_CTL_H_CLKDIV_SEL, val)) ||
 	    (!(FIELD_GET(USBDRD_UCTL_CTL_H_CLK_EN, val)))) {
-		dev_err(dev, "dwc3 controller clock init failure.\n");
-			return -EINVAL;
+		dev_err(dev, "clock init failure (UCTL_CTL=%016llx)\n", val);
+		return -EINVAL;
 	}
 
 	/* Step 4c: Deassert the controller clock divider reset. */
-- 
2.39.2

Re: [PATCH v5 6/7] usb: dwc3: dwc3-octeon: Dump control register on clock init failure

[Thinh Nguyen]

On Mon, Jul 31, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> It might be interesting to know control register value in case
> clock fails to enable.

Please also note that you did a minor reformat too.

> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  CHANGES:
>  - v4: new patch
>  - v5: Philippe's review tag
> 
>  drivers/usb/dwc3/dwc3-octeon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index 330bcb59cc95..d578110f7afb 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -299,8 +299,8 @@ static int dwc3_octeon_setup(struct dwc3_octeon *octeon,
>  	val = dwc3_octeon_readq(uctl_ctl_reg);
>  	if ((div != FIELD_GET(USBDRD_UCTL_CTL_H_CLKDIV_SEL, val)) ||
>  	    (!(FIELD_GET(USBDRD_UCTL_CTL_H_CLK_EN, val)))) {
> -		dev_err(dev, "dwc3 controller clock init failure.\n");
> -			return -EINVAL;
> +		dev_err(dev, "clock init failure (UCTL_CTL=%016llx)\n", val);
> +		return -EINVAL;
>  	}
>  
>  	/* Step 4c: Deassert the controller clock divider reset. */
> -- 
> 2.39.2
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thinh

[PATCH v5 7/7] usb: dwc3: dwc3-octeon: Add SPDX header and copyright

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

Assign copyright to indicate driver rewrite is done for RACOM s.r.o.
As David no longer works for Marvell (Cavium), I'm to blame for breakage.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 CHANGES:
 - v2: None
 - v3: None
 - v4: Assign copyring to RACOM s.r.o., Mírová 1283, Nové Město na Moravě
 - v5: None

 drivers/usb/dwc3/dwc3-octeon.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
index d578110f7afb..6f47262a117a 100644
--- a/drivers/usb/dwc3/dwc3-octeon.c
+++ b/drivers/usb/dwc3/dwc3-octeon.c
@@ -1,11 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * XHCI HCD glue for Cavium Octeon III SOCs.
+ * DWC3 glue for Cavium Octeon III SOCs.
  *
  * Copyright (C) 2010-2017 Cavium Networks
- *
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file "COPYING" in the main directory of this archive
- * for more details.
+ * Copyright (C) 2023 RACOM s.r.o.
  */
 
 #include <linux/bitfield.h>
@@ -537,6 +535,6 @@ static struct platform_driver dwc3_octeon_driver = {
 module_platform_driver(dwc3_octeon_driver);
 
 MODULE_ALIAS("platform:dwc3-octeon");
-MODULE_AUTHOR("David Daney <david.daney@cavium.com>");
+MODULE_AUTHOR("Ladislav Michl <ladis@linux-mips.org>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("DesignWare USB3 OCTEON III Glue Layer");
-- 
2.39.2

Re: [PATCH v5 7/7] usb: dwc3: dwc3-octeon: Add SPDX header and copyright

[Thinh Nguyen]

On Mon, Jul 31, 2023, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Assign copyright to indicate driver rewrite is done for RACOM s.r.o.
> As David no longer works for Marvell (Cavium), I'm to blame for breakage.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  CHANGES:
>  - v2: None
>  - v3: None
>  - v4: Assign copyring to RACOM s.r.o., Mírová 1283, Nové Město na Moravě
>  - v5: None
> 
>  drivers/usb/dwc3/dwc3-octeon.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index d578110f7afb..6f47262a117a 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -1,11 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
> - * XHCI HCD glue for Cavium Octeon III SOCs.
> + * DWC3 glue for Cavium Octeon III SOCs.
>   *
>   * Copyright (C) 2010-2017 Cavium Networks
> - *
> - * This file is subject to the terms and conditions of the GNU General Public
> - * License.  See the file "COPYING" in the main directory of this archive
> - * for more details.
> + * Copyright (C) 2023 RACOM s.r.o.
>   */
>  
>  #include <linux/bitfield.h>
> @@ -537,6 +535,6 @@ static struct platform_driver dwc3_octeon_driver = {
>  module_platform_driver(dwc3_octeon_driver);
>  
>  MODULE_ALIAS("platform:dwc3-octeon");
> -MODULE_AUTHOR("David Daney <david.daney@cavium.com>");
> +MODULE_AUTHOR("Ladislav Michl <ladis@linux-mips.org>");
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("DesignWare USB3 OCTEON III Glue Layer");
> -- 
> 2.39.2
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

Re: [PATCH v5 7/7] usb: dwc3: dwc3-octeon: Add SPDX header and copyright

[Philippe Mathieu-Daudé]

On 31/7/23 11:33, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Assign copyright to indicate driver rewrite is done for RACOM s.r.o.
> As David no longer works for Marvell (Cavium), I'm to blame for breakage.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>   CHANGES:
>   - v2: None
>   - v3: None
>   - v4: Assign copyring to RACOM s.r.o., Mírová 1283, Nové Město na Moravě
>   - v5: None
> 
>   drivers/usb/dwc3/dwc3-octeon.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c b/drivers/usb/dwc3/dwc3-octeon.c
> index d578110f7afb..6f47262a117a 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -1,11 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>   /*
> - * XHCI HCD glue for Cavium Octeon III SOCs.
> + * DWC3 glue for Cavium Octeon III SOCs.
>    *
>    * Copyright (C) 2010-2017 Cavium Networks
> - *
> - * This file is subject to the terms and conditions of the GNU General Public
> - * License.  See the file "COPYING" in the main directory of this archive
> - * for more details.
> + * Copyright (C) 2023 RACOM s.r.o.
>    */
>   
>   #include <linux/bitfield.h>
> @@ -537,6 +535,6 @@ static struct platform_driver dwc3_octeon_driver = {
>   module_platform_driver(dwc3_octeon_driver);
>   
>   MODULE_ALIAS("platform:dwc3-octeon");
> -MODULE_AUTHOR("David Daney <david.daney@cavium.com>");
> +MODULE_AUTHOR("Ladislav Michl <ladis@linux-mips.org>");
>   MODULE_LICENSE("GPL");
>   MODULE_DESCRIPTION("DesignWare USB3 OCTEON III Glue Layer");

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

RE: [EXTERNAL] Re: [PATCH v5 7/7] usb: dwc3: dwc3-octeon: Add SPDX header and copyright

[David Daney]

Acked-by: David Daney <daviddaney@microsoft.com>

... and apologies for the Outlook style top-post. What have I become?
-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@linaro.org> 
Sent: Tuesday, August 1, 2023 8:51 AM
To: Ladislav Michl <oss-lists@triops.cz>; David Daney <daviddaney@microsoft.com>
Cc: linux-mips@vger.kernel.org; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; linux-usb@vger.kernel.org; Liang He <windhl@126.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Subject: [EXTERNAL] Re: [PATCH v5 7/7] usb: dwc3: dwc3-octeon: Add SPDX header and copyright

[You don't often get email from philmd@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

On 31/7/23 11:33, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
>
> Assign copyright to indicate driver rewrite is done for RACOM s.r.o.
> As David no longer works for Marvell (Cavium), I'm to blame for breakage.
>
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>   CHANGES:
>   - v2: None
>   - v3: None
>   - v4: Assign copyring to RACOM s.r.o., Mírová 1283, Nové Město na Moravě
>   - v5: None
>
>   drivers/usb/dwc3/dwc3-octeon.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-octeon.c 
> b/drivers/usb/dwc3/dwc3-octeon.c index d578110f7afb..6f47262a117a 
> 100644
> --- a/drivers/usb/dwc3/dwc3-octeon.c
> +++ b/drivers/usb/dwc3/dwc3-octeon.c
> @@ -1,11 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
>   /*
> - * XHCI HCD glue for Cavium Octeon III SOCs.
> + * DWC3 glue for Cavium Octeon III SOCs.
>    *
>    * Copyright (C) 2010-2017 Cavium Networks
> - *
> - * This file is subject to the terms and conditions of the GNU 
> General Public
> - * License.  See the file "COPYING" in the main directory of this 
> archive
> - * for more details.
> + * Copyright (C) 2023 RACOM s.r.o.
>    */
>
>   #include <linux/bitfield.h>
> @@ -537,6 +535,6 @@ static struct platform_driver dwc3_octeon_driver = {
>   module_platform_driver(dwc3_octeon_driver);
>
>   MODULE_ALIAS("platform:dwc3-octeon");
> -MODULE_AUTHOR("David Daney <david.daney@cavium.com>");
> +MODULE_AUTHOR("Ladislav Michl <ladis@linux-mips.org>");
>   MODULE_LICENSE("GPL");
>   MODULE_DESCRIPTION("DesignWare USB3 OCTEON III Glue Layer");

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>