[U-Boot,RFC,3/6] usb: exynos: add init_after_reset for usb reset

Some host controllers need addidional re-initialization
after ehci_reset() so we add .init_after_reset callback
which is requires to reinit the phy after controller reset.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/usb/host/ehci-exynos.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

On Mon, 1 Apr 2019 at 13:53, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Some host controllers need addidional re-initialization

Please run spell-check.

> after ehci_reset() so we add .init_after_reset callback
> which is requires to reinit the phy after controller reset.

s/requires/required/
.... but you do not re-init the phy. The exynos_usb_init() performs
the reset of usb3503 USB hub!

>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/usb/host/ehci-exynos.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> index b0f7bd4936..e6a542e092 100644
> --- a/drivers/usb/host/ehci-exynos.c
> +++ b/drivers/usb/host/ehci-exynos.c
> @@ -143,6 +143,23 @@ static void exynos5_setup_usb_phy(struct exynos_usb_phy *usb)
>                         EHCICTRL_ENAINCR16);
>  }
>
> +static int ehci_exynos_init_after_reset(struct ehci_ctrl *ehcntl)
> +{
> +       if (cpu_is_exynos4()) {
> +               if (proid_is_exynos4412()) {

No need for double indentation.

> +                       /*
> +                        * "usb reset" cmd: restart re-initialize the usb driver

Just "reinitialize", not restart reinitialize.

Best regards,
Krzysztof

> +                        */
> +                       exynos_usb_init();
> +               }
> +       }
> +       return 0;
> +}
> +
> +static const struct ehci_ops exynos_ehci_ops = {
> +       .init_after_reset = ehci_exynos_init_after_reset,
> +};
> +
>  static void exynos4412_setup_usb_phy(struct exynos4412_usb_phy *usb)
>  {
>         writel(CLK_24MHZ, &usb->usbphyclk);
> @@ -234,7 +251,8 @@ static int ehci_usb_probe(struct udevice *dev)
>         hcor = (struct ehci_hcor *)((uint32_t)ctx->hcd +
>                         HC_LENGTH(ehci_readl(&ctx->hcd->cr_capbase)));
>
> -       return ehci_register(dev, ctx->hcd, hcor, NULL, 0, USB_INIT_HOST);
> +       return ehci_register(dev, ctx->hcd, hcor, &exynos_ehci_ops,
> +                       0, USB_INIT_HOST);
>  }
>
>  static int ehci_usb_remove(struct udevice *dev)
> --
> 2.21.0
>

Hi Krzysztof,

On Mon, 1 Apr 2019 at 18:25, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, 1 Apr 2019 at 13:53, Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Some host controllers need addidional re-initialization
>
> Please run spell-check.
>
> > after ehci_reset() so we add .init_after_reset callback
> > which is requires to reinit the phy after controller reset.
>
> s/requires/required/

I did run checkpatch before on this, It did not spotted and error or warning.

> .... but you do not re-init the phy. The exynos_usb_init() performs
> the reset of usb3503 USB hub!
>

Yes that is needed as we do not get the usb back after "usb reset" command.

> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/usb/host/ehci-exynos.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
> > index b0f7bd4936..e6a542e092 100644
> > --- a/drivers/usb/host/ehci-exynos.c
> > +++ b/drivers/usb/host/ehci-exynos.c
> > @@ -143,6 +143,23 @@ static void exynos5_setup_usb_phy(struct exynos_usb_phy *usb)
> >                         EHCICTRL_ENAINCR16);
> >  }
> >
> > +static int ehci_exynos_init_after_reset(struct ehci_ctrl *ehcntl)
> > +{
> > +       if (cpu_is_exynos4()) {
> > +               if (proid_is_exynos4412()) {
>
> No need for double indentation.
>
> > +                       /*
> > +                        * "usb reset" cmd: restart re-initialize the usb driver
>
> Just "reinitialize", not restart reinitialize.
>

Ok.

> Best regards,
> Krzysztof
>
> > +                        */
> > +                       exynos_usb_init();
> > +               }
> > +       }
> > +       return 0;
> > +}
> > +
> > +static const struct ehci_ops exynos_ehci_ops = {
> > +       .init_after_reset = ehci_exynos_init_after_reset,
> > +};
> > +
> >  static void exynos4412_setup_usb_phy(struct exynos4412_usb_phy *usb)
> >  {
> >         writel(CLK_24MHZ, &usb->usbphyclk);
> > @@ -234,7 +251,8 @@ static int ehci_usb_probe(struct udevice *dev)
> >         hcor = (struct ehci_hcor *)((uint32_t)ctx->hcd +
> >                         HC_LENGTH(ehci_readl(&ctx->hcd->cr_capbase)));
> >
> > -       return ehci_register(dev, ctx->hcd, hcor, NULL, 0, USB_INIT_HOST);
> > +       return ehci_register(dev, ctx->hcd, hcor, &exynos_ehci_ops,
> > +                       0, USB_INIT_HOST);
> >  }
> >
> >  static int ehci_usb_remove(struct udevice *dev)
> > --
> > 2.21.0
> >

Best Regards
-Anand

On Mon, 1 Apr 2019 at 18:05, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Krzysztof,
>
> On Mon, 1 Apr 2019 at 18:25, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Mon, 1 Apr 2019 at 13:53, Anand Moon <linux.amoon@gmail.com> wrote:
> > >
> > > Some host controllers need addidional re-initialization
> >
> > Please run spell-check.
> >
> > > after ehci_reset() so we add .init_after_reset callback
> > > which is requires to reinit the phy after controller reset.
> >
> > s/requires/required/
>
> I did run checkpatch before on this, It did not spotted and error or warning.
>
> > .... but you do not re-init the phy. The exynos_usb_init() performs
> > the reset of usb3503 USB hub!
> >
>
> Yes that is needed as we do not get the usb back after "usb reset" command.

Then please update the commit message to describe what exactly you are
doing and what you want to achieve. Someone might think that you are
doing initialization in init-after-reset... but you want to perform
different reset after reset of ehci... and while writing it maybe you
will notice that it is a hack and probably not the best way to do it.

Best regards,
Krzysztof