On Saturday 18 October 2008, Maxime Bizon wrote:
> Signed-off-by: Maxime Bizon <mbizon@xxxxxxxxxx>
> ---
> arch/mips/bcm63xx/Kconfig | 6 +
> arch/mips/bcm63xx/Makefile | 1 +
> arch/mips/bcm63xx/dev-usb-ohci.c | 50 ++++++
> .../asm/mach-bcm63xx/bcm63xx_dev_usb_ohci.h | 6 +
The arch/mips stuff should be one patch, going through the MIPS tree
(presumably along with board init code calling it) ...
> drivers/usb/host/ohci-bcm63xx.c | 159 ++++++++++++++++++++
> drivers/usb/host/ohci-hcd.c | 5 +
> drivers/usb/host/ohci.h | 7 +-
... and the OHCI (and EHCI) stuff should be in another patch, going
through the USB tree.
I gave a really quick glance to the arch/mips stuff and it seemed
reasonable. Maybe a bit short on board-specific issues like pinmux
for "this board exposes only ports 1 & 3" etc, but for all I know
those issues don't even exist for these chips. ;)
The OHCI bits don't seem unusual ... a few comments though.
> --- /dev/null
> +++ b/drivers/usb/host/ohci-bcm63xx.c
> @@ -0,0 +1,159 @@
>
> +static const struct hc_driver ohci_bcm63xx_hc_driver = {
> + .description = hcd_name,
> + .product_desc = "BCM63XX integrated OHCI controller",
> + .hcd_priv_size = sizeof(struct ohci_hcd),
> +
> + .irq = ohci_irq,
> + .flags = HCD_USB11 | HCD_MEMORY,
> + .start = ohci_bcm63xx_start,
> + .stop = ohci_stop,
> + .shutdown = ohci_shutdown,
> + .urb_enqueue = ohci_urb_enqueue,
> + .urb_dequeue = ohci_urb_dequeue,
> + .endpoint_disable = ohci_endpoint_disable,
> + .get_frame_number = ohci_get_frame,
> + .hub_status_data = ohci_hub_status_data,
> + .hub_control = ohci_hub_control,
> + .start_port_reset = ohci_start_port_reset,
Saving power management for later, it seems. :)
> +};
> +
> +static int __devinit ohci_hcd_bcm63xx_drv_probe(struct platform_device *pdev)
> +{
> + struct resource *res_mem, *res_irq;
> + struct usb_hcd *hcd;
> + struct ohci_hcd *ohci;
> + u32 reg;
> + int ret;
> +
> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
irq = platform_get_irq(pdev, 0) is a bit cleaner.
> + if (!res_mem || !res_irq)
> + return -ENODEV;
> +
> + if (BCMCPU_IS_6348()) {
> + struct clk *clk;
> + /* enable USB host clock */
> + clk = clk_get(&pdev->dev, "usbh");
> + if (IS_ERR(clk))
> + return -ENODEV;
> +
> + clk_enable(clk);
> + usb_host_clock = clk;
> + bcm_rset_writel(RSET_OHCI_PRIV, 0, OHCI_PRIV_REG);
> +
> + } else if (BCMCPU_IS_6358()) {
> + reg = bcm_rset_readl(RSET_USBH_PRIV, USBH_PRIV_SWAP_REG);
> + reg &= ~USBH_PRIV_SWAP_OHCI_ENDN_MASK;
> + reg |= USBH_PRIV_SWAP_OHCI_DATA_MASK;
> + bcm_rset_writel(RSET_USBH_PRIV, reg, USBH_PRIV_SWAP_REG);
> + /* don't ask... */
Best if you describe the errata this works around, even if you'd
rather not go into detail about that part of the sausage-making.
That way the next person may be able to say "oh yeah, they finally
fixed that one", or whatever. ;)
> + bcm_rset_writel(RSET_USBH_PRIV, 0x1c0020, USBH_PRIV_TEST_REG);
> + } else
> + return 0;
> +
> +static struct platform_driver ohci_hcd_bcm63xx_driver = {
> + .probe = ohci_hcd_bcm63xx_drv_probe,
> + .remove = __devexit_p(ohci_hcd_bcm63xx_drv_remove),
> + .shutdown = usb_hcd_platform_shutdown,
> + .driver = {
> + .name = "bcm63xx_ohci",
> + .owner = THIS_MODULE,
> + .bus = &platform_bus_type
Don't set the bus type here; the platform_bus code does that.
> + },
> +};
> +
> +MODULE_ALIAS("platform:bcm63xx_ohci");
> --- a/drivers/usb/host/ohci.h
> +++ b/drivers/usb/host/ohci.h
> @@ -549,6 +549,11 @@ static inline struct usb_hcd *ohci_to_hcd (const struct ohci_hcd *ohci)
> #define writel_be(val, addr) out_be32((__force unsigned *)addr, val)
> #endif
>
> +#if defined(CONFIG_MIPS) && defined(CONFIG_BCM63XX)
> +#define readl_be(addr) __raw_readl((__force unsigned *)addr)
> +#define writel_be(val, addr) __raw_writel(val, (__force unsigned *)addr)
> +#endif
I'd expect those to be defined in <asm/...> or <mach/...> headers,
as they are in all other cases.
