On 3/7/2013 17:12, Andreas Bießmann wrote:
Dear Bo Shen,

On 28.02.13 08:00, Bo Shen wrote:
Add OHCI support for sama5d3x devices

Signed-off-by: Bo Shen <[email protected]>
---
  drivers/usb/host/ohci-at91.c |   14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index efd711d..35a282e 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -42,7 +42,7 @@ int usb_cpu_init(void)
        while ((readl(&pmc->sr) & AT91_PMC_LOCKB) != AT91_PMC_LOCKB)
                ;
  #elif defined(CONFIG_AT91SAM9G45) || defined(CONFIG_AT91SAM9M10G45) || \
-       defined(CONFIG_AT91SAM9X5)
+       defined(CONFIG_AT91SAM9X5) || defined(CONFIG_SAMA5D3)

I think these ifdeffery increases alarmingly here and there. Can we find
a better solution like

#if defined(ATMEL_OHCI_NEEDS_UPLL)

or whatever we can call it. I mean to classify this and enable it in the
respective SoC headers. Maybe here we can distinguish upon the IP version?

        /* Enable UPLL */
        writel(readl(&pmc->uckr) | AT91_PMC_UPLLEN | AT91_PMC_BIASEN,
                &pmc->uckr);
@@ -54,7 +54,12 @@ int usb_cpu_init(void)
  #endif

        /* Enable USB host clock. */
+#if defined(CONFIG_SAMA5D3)

I think this ifdef is Ok instead.

+       writel(1 << (ATMEL_ID_UHP - 32), &pmc->pcer1);
+#else
        writel(1 << ATMEL_ID_UHP, &pmc->pcer);
+#endif
+
  #ifdef CONFIG_AT91SAM9261
        writel(ATMEL_PMC_UHP | AT91_PMC_HCK0, &pmc->scer);
  #else
@@ -69,7 +74,12 @@ int usb_cpu_stop(void)
        at91_pmc_t *pmc = (at91_pmc_t *)ATMEL_BASE_PMC;

        /* Disable USB host clock. */
+#if defined(CONFIG_SAMA5D3)
+       writel(1 << (ATMEL_ID_UHP - 32), &pmc->pcdr1);
+#else
        writel(1 << ATMEL_ID_UHP, &pmc->pcdr);
+#endif
+
  #ifdef CONFIG_AT91SAM9261
        writel(ATMEL_PMC_UHP | AT91_PMC_HCK0, &pmc->scdr);
  #else
@@ -83,7 +93,7 @@ int usb_cpu_stop(void)
        while ((readl(&pmc->sr) & AT91_PMC_LOCKB) != 0)
                ;
  #elif defined(CONFIG_AT91SAM9G45) || defined(CONFIG_AT91SAM9M10G45) || \
-       defined(CONFIG_AT91SAM9X5)
+       defined(CONFIG_AT91SAM9X5) || defined(CONFIG_SAMA5D3)
        /* Disable UPLL */
        writel(readl(&pmc->uckr) & (~AT91_PMC_UPLLEN), &pmc->uckr);
        while ((readl(&pmc->sr) & AT91_PMC_LOCKU) == AT91_PMC_LOCKU)


I think the ifdef comment above is no show stopper for this patch but
should be considered at least for a future patch.

OK.

I will consider this for a future patch, not in this one.

Best Regards,
Bo Shen

The rest looks sane to me.

Best regards

Andreas Bießmann


_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to