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

[<Sergiu.Moga@microchip.com>]

On 03.01.2023 01:26, Marek Vasut wrote:
> 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 ?
> 

Unfortunately, the other older boards would not build and work anymore 
if I were to remove this.

>> +#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 ?
> 

I would rather keep the same DT properties as those in Linux and have 
them be in concordance with our USB DT binding, which specifies the 
atmel,vbus-gpio property and no vbus-supply.


Other than that, I am fine with the other requested changes.

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