[PATCH 2/4] usb: ohci-at91: Enable OHCI functionality and register into DM

[Marek Vasut]

On 12/23/22 13:34, Sergiu Moga wrote:

[...]

> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index 9b955c1bd6..9ae55c6e5d 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -5,6 +5,9 @@
>    */
>   
>   #include <common.h>
> +
> +#if !(CONFIG_IS_ENABLED(DM_USB))
> +
>   #include <asm/arch/clk.h>
>   
>   int usb_cpu_init(void)
> @@ -62,3 +65,183 @@ int usb_cpu_init_fail(void)
>   {
>   	return usb_cpu_stop();
>   }

Would it be possible to just remove the non-DM functionality ?

> +#elif CONFIG_IS_ENABLED(DM_GPIO)

I think you want plain '#else' here, and then make the driver select 
DM_GPIO in case DM_USB is enabled in Kconfig entry.

> +#include <clk.h>
> +#include <dm.h>
> +#include <asm/gpio.h>
> +#include <usb.h>
> +#include "ohci.h"
> +
> +#define AT91_MAX_USBH_PORTS        3
>
> +#define at91_for_each_port(index)	\
> +		for ((index) = 0; (index) < AT91_MAX_USBH_PORTS; (index)++)
> +
> +struct at91_usbh_data {
> +	enum usb_init_type init_type;
> +	struct gpio_desc vbus_pin[AT91_MAX_USBH_PORTS];

drivers/usb/host/ehci-generic.c:        ret = 
device_get_supply_regulator(dev, "vbus-supply",

I wonder if we can instead use regulator for vbus-supply here too ?

> +	u8 ports;				/* number of ports on root hub */
> +};
> +
> +struct ohci_at91_priv {
> +	struct clk *iclk;
> +	struct clk *fclk;
> +	struct clk *hclk;

Have a look at clk.*bulk functions in U-Boot , then you can do 
clk_get_bulk() / clk_enable_bulk() etc. below.

> +	bool clocked;
> +};
> +
> +static void at91_start_clock(struct ohci_at91_priv *ohci_at91)
> +{
> +	if (ohci_at91->clocked)
> +		return;
> +
> +	clk_set_rate(ohci_at91->fclk, 48000000);
> +	clk_prepare_enable(ohci_at91->hclk);
> +	clk_prepare_enable(ohci_at91->iclk);
> +	clk_prepare_enable(ohci_at91->fclk);

E.g. here you can use clk_enable_bulk() .

> +	ohci_at91->clocked = true;

Why is this here, are you suffering from clock inbalance ? You 
shouldn't, so remove this and the if (ohci_at91->clocked) test above.

> +}
> +
> +static void at91_stop_clock(struct ohci_at91_priv *ohci_at91)
> +{
> +	if (!ohci_at91->clocked)

Is this really needed ?

> +		return;
> +
> +	clk_disable_unprepare(ohci_at91->fclk);
> +	clk_disable_unprepare(ohci_at91->iclk);
> +	clk_disable_unprepare(ohci_at91->hclk);
> +	ohci_at91->clocked = false;
> +}
> +
> +static void ohci_at91_set_power(struct at91_usbh_data *pdata, int port,
> +				bool enable)
> +{
> +	if (!dm_gpio_is_valid(&pdata->vbus_pin[port]))
> +		return;

This could be regulator instead.

> +	if (enable)
> +		dm_gpio_set_dir_flags(&pdata->vbus_pin[port],
> +				      GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
> +	else
> +		dm_gpio_set_dir_flags(&pdata->vbus_pin[port], 0);
> +}
> +
> +static void at91_start_hc(struct udevice *dev)
> +{
> +	struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
> +
> +	at91_start_clock(ohci_at91);
> +}
> +
> +static void at91_stop_hc(struct udevice *dev)
> +{
> +	struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
> +
> +	at91_stop_clock(ohci_at91);
> +}
> +
> +static int ohci_atmel_deregister(struct udevice *dev)
> +{
> +	struct at91_usbh_data *pdata = dev_get_plat(dev);
> +	int i;
> +
> +	at91_stop_hc(dev);
> +
> +	at91_for_each_port(i) {
> +		if (i >= pdata->ports)

You can just wrap this test into the at91_for_each_port() macro , just 
use "index < min(AT91_MAX_USBH_PORTS, pdata->ports)" or so.

> +			break;
> +
> +		ohci_at91_set_power(pdata, i, false);
> +	}
> +
> +	return ohci_deregister(dev);
> +}
> +
> +static int ohci_atmel_child_pre_probe(struct udevice *dev)
> +{
> +	struct udevice *ohci_controller = dev_get_parent(dev);
> +	struct at91_usbh_data *pdata = dev_get_plat(ohci_controller);
> +	int i;
> +
> +	at91_for_each_port(i) {
> +		if (i >= pdata->ports)
> +			break;
> +
> +		ohci_at91_set_power(pdata, i, true);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ohci_atmel_probe(struct udevice *dev)
> +{
> +	struct at91_usbh_data *pdata = dev_get_plat(dev);
> +	struct ohci_at91_priv *ohci_at91 = dev_get_priv(dev);
> +	int		      i;
> +	int		      ret;
> +	u32		      ports;
> +	struct ohci_regs      *regs = (struct ohci_regs *)dev_read_addr(dev);
> +
> +	if (!dev_read_u32(dev, "num-ports", &ports))
> +		pdata->ports = ports;

dev_read_u32_default() ?

> +	at91_for_each_port(i) {
> +		if (i >= pdata->ports)
> +			break;
> +
> +		gpio_request_by_name(dev, "atmel,vbus-gpio", i,
> +				     &pdata->vbus_pin[i], GPIOD_IS_OUT |
> +				     GPIOD_IS_OUT_ACTIVE);
> +	}
> +
> +	ohci_at91->iclk = devm_clk_get(dev, "ohci_clk");
> +	if (IS_ERR(ohci_at91->iclk)) {
> +		ret = PTR_ERR(ohci_at91->iclk);
> +		goto fail;
> +	}
> +
> +	ohci_at91->fclk = devm_clk_get(dev, "uhpck");
> +	if (IS_ERR(ohci_at91->fclk)) {
> +		ret = PTR_ERR(ohci_at91->fclk);
> +		goto fail;
> +	}
> +
> +	ohci_at91->hclk = devm_clk_get(dev, "hclk");
> +	if (IS_ERR(ohci_at91->hclk)) {
> +		ret = PTR_ERR(ohci_at91->hclk);

clk_get_bulk() would simplify this.

[...]