Cleanup Octeon DWC3 glue code

From: Ladislav Michl <[email protected]>

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 <[email protected]>

Reported-by kernel test robot <[email protected]>

Closes https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/[email protected]/__;!!A4F2R9G_pg!bRAo7p44EQ5aHcAiBbTC-iYlO5IygKxZRV6HRGojsnNL0q_IRd7VyOnuC5SwtQBM2Dg_1O9NEeEhStq16Eo1OcYm1A$

---
CHANGES:
-v5: new patch

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

From: Ladislav Michl <[email protected]>

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 <[email protected]>

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here

---
CHANGES:
-v5: new patch

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

Quoted 16 lines · Ladislav Michl

From: Ladislav Michl <[email protected]>

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 <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

---
CHANGES:
- v4: new patch
- v5: Philippe's review tag

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

drivers/usb/dwc3/dwc3-octeon.c static int dwc3_octeon_config_power(struct device *dev, void __iomem *base)

Quoted 5 lines

drivers/usb/dwc3/dwc3-octeon.c +0 -0

index 69fe50cfa719..24e75881b5cf 100644
@@ -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?

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

drivers/usb/dwc3/dwc3-octeon.c +5 -43

index 24e75881b5cf..0dc45dda134c 100644
@@ -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. */

drivers/usb/dwc3/dwc3-octeon.c +12 -1

+		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;

drivers/usb/dwc3/dwc3-octeon.c +8 -1

+			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;
 

Signature · 1 line

2.39.2

Quoted 94 lines · Ladislav Michl

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

drivers/usb/dwc3/dwc3-octeon.c +5 -43

index 24e75881b5cf..0dc45dda134c 100644
@@ -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);
 

drivers/usb/dwc3/dwc3-octeon.c static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon) · line 426

Quoted 11 lines

drivers/usb/dwc3/dwc3-octeon.c +8 -2

 	/* 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.

Quoted 26 lines

drivers/usb/dwc3/dwc3-octeon.c +18 -1

+		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);

drivers/usb/dwc3/dwc3-octeon.c static int dwc3_octeon_probe(struct platform_device *pdev) · line 500

Quoted 4 lines

drivers/usb/dwc3/dwc3-octeon.c +4 -0

+			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.

Quoted 22 lines

drivers/usb/dwc3/dwc3-octeon.c +8 -1

+			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;
 

Signature · 1 line

2.39.2

Thanks,
Thinh

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

Quoted 2 lines · Ladislav Michl

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(-)

drivers/usb/dwc3/dwc3-octeon.c +4 -3

index 90e1ae66769f..f35dca899d6e 100644
@@ -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)

Signature · 1 line

2.39.2

Quoted 95 lines · Thinh Nguyen

Quoted 94 lines · Ladislav Michl

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

drivers/usb/dwc3/dwc3-octeon.c +5 -43

index 24e75881b5cf..0dc45dda134c 100644
@@ -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);
 

Quoted 16 lines

drivers/usb/dwc3/dwc3-octeon.c static int dwc3_octeon_clocks_start(struct dwc3_octeon *octeon) · line 426

Quoted 11 lines

drivers/usb/dwc3/dwc3-octeon.c +8 -2

 	/* 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.

Quoted 6 lines

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.

Quoted 2 lines

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?

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

drivers/usb/dwc3/dwc3-octeon.c +0 -0

index 24e75881b5cf..0dc45dda134c 100644

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

drivers/usb/dwc3/dwc3-octeon.c +60 -75

index 0dc45dda134c..330bcb59cc95 100644
@@ -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;
 

Signature · 1 line

2.39.2

From: Ladislav Michl <[email protected]>

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

Signed-off-by Ladislav Michl <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

---
CHANGES:
- v4: new patch
- v5: Philippe's review tag

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

drivers/usb/dwc3/dwc3-octeon.c +2 -2

index 330bcb59cc95..d578110f7afb 100644
@@ -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. */

Signature · 1 line

2.39.2

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

drivers/usb/dwc3/dwc3-octeon.c +4 -6

index d578110f7afb..6f47262a117a 100644
@@ -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 <[email protected]>");
+MODULE_AUTHOR("Ladislav Michl <[email protected]>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("DesignWare USB3 OCTEON III Glue Layer");

Signature · 1 line

2.39.2

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

drivers/usb/dwc3/dwc3-octeon.c +3 -5

index d578110f7afb..6f47262a117a 100644
@@ -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>

From: Ladislav Michl <[email protected]>

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 <[email protected]>

---
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(-)

(quoted diff) +0 -0

b/drivers/usb/dwc3/dwc3-octeon.c index d578110f7afb..6f47262a117a 100644

drivers/usb/dwc3/dwc3-octeon.c +2 -3

@@ -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>