| Message ID | ZMd/oMRx8ze22/kK@lenoch (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Cleanup Octeon DWC3 glue code | expand |
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
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
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)
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
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
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
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;