lkml.org 
[lkml]   [2010]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v1] ppc44x:PHY fixup for USB on canyonlands board
Date
Hi Rup,

On Tuesday 23 November 2010 13:56:53 Rupjyoti Sarmah wrote:
> This fix is a reset for USB PHY that requires some amount of time for power
> to be stable on Canyonlands.

Since this is version 2 of your patch, "[PATCH v2]" would have been a bit
better in the subject line. Its also a good practice to summarize the changes
between patch versions below the "---" line.

Please find a some further comments below.

<snip>

> --- a/arch/powerpc/platforms/44x/44x.h
> +++ b/arch/powerpc/platforms/44x/44x.h
> @@ -4,4 +4,9 @@
> extern u8 as1_readb(volatile u8 __iomem *addr);
> extern void as1_writeb(u8 data, volatile u8 __iomem *addr);
>
> +#define BCSR_USB_EN 0x11

This define is wrong here. Its not common for all 44x platforms but
Canyonlands specific.

> +#define GPIO0_OSRH 0xC
> +#define GPIO0_TSRH 0x14
> +#define GPIO0_ISR1H 0x34
> +
> #endif /* __POWERPC_PLATFORMS_44X_44X_H */
> diff --git a/arch/powerpc/platforms/44x/Makefile
> b/arch/powerpc/platforms/44x/Makefile index 82ff326..6854e73 100644
> --- a/arch/powerpc/platforms/44x/Makefile
> +++ b/arch/powerpc/platforms/44x/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_WARP) += warp.o
> obj-$(CONFIG_XILINX_VIRTEX_5_FXT) += virtex.o
> obj-$(CONFIG_XILINX_ML510) += virtex_ml510.o
> obj-$(CONFIG_ISS4xx) += iss4xx.o
> +obj-$(CONFIG_CANYONLANDS)+= canyonlands.o
> diff --git a/arch/powerpc/platforms/44x/canyonlands.c
> b/arch/powerpc/platforms/44x/canyonlands.c new file mode 100644
> index 0000000..f13b62f
> --- /dev/null
> +++ b/arch/powerpc/platforms/44x/canyonlands.c
> @@ -0,0 +1,122 @@
> +/*
> + * This contain platform specific code for Canyonalnds board based on
^^^^^^^^^^^
Canyonlands

> + * APM ppc44x series of processors.
> + *
> + * Copyright (c) 2010, Applied Micro Circuits Corporation
> + * Author: Rupjyoti Sarmah <rsarmah@apm.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <asm/pci-bridge.h>
> +#include <asm/ppc4xx.h>
> +#include <asm/udbg.h>
> +#include <asm/uic.h>
> +#include <linux/of_platform.h>
> +#include "44x.h"
> +
> +static __initdata struct of_device_id ppc44x_of_bus[] = {
> + { .compatible = "ibm,plb4", },
> + { .compatible = "ibm,opb", },
> + { .compatible = "ibm,ebc", },
> + { .compatible = "simple-bus", },
> + {},
> +};
> +
> +static int __init ppc44x_device_probe(void)
> +{
> + of_platform_bus_probe(NULL, ppc44x_of_bus, NULL);
> +
> + return 0;
> +}
> +machine_device_initcall(canyonlands, ppc44x_device_probe);
> +
> +/* Using this code only for the Canyonlands board. */
> +
> +static int __init ppc44x_probe(void)
> +{
> + unsigned long root = of_get_flat_dt_root();
> + if (of_flat_dt_is_compatible(root, "amcc,canyonlands")) {
> + ppc_pci_set_flags(PPC_PCI_REASSIGN_ALL_RSRC);
> + return 1;
> + }
> + return 0;
> +}
> +
> +/* PHY fixup code on Canyonlands kit. */

PHY is a bit unspecific. One might think that this is an ethernet PHY (see
remark below about better comments in the code)?

> +
> +static int __init ppc460ex_canyonlands_fixup(void)
> +{
> + u8 __iomem *bcsr ;
> + void __iomem *vaddr;

Double space before *vaddr.

> + struct device_node *np;
> + u32 val ;
> +
> + np = of_find_compatible_node(NULL, NULL, "apm,ppc460ex-bcsr");
> + if (!np) {
> + printk(KERN_ERR "failed did not find apm, ppc460ex bcsr
node\n");
> + return -ENODEV;
> + }
> +
> + bcsr = of_iomap(np, 0);
> + of_node_put(np);
> +
> + if (!bcsr) {
> + printk(KERN_CRIT "Could not remap bcsr\n");
> + return -ENODEV;
> + }
> +
> + np = of_find_compatible_node(NULL, NULL, "ibm,ppc4xx-gpio");
> + vaddr = of_iomap(np, 0);
> + if (!vaddr) {
> + printk(KERN_CRIT "Could not get gpio node address\n");
> + return -ENODEV;
> + }
> +
> + setbits8(&bcsr[7], BCSR_USB_EN);
> + udelay(100000);
> +
> + clrbits8(&bcsr[7], BCSR_USB_EN);
> + udelay(100000);

Thats a total bootup delay of 200ms. Is this really needed?

> +
> + /* configure gpio16 and gpio19 as alternate1 */
> +
> + /* GPIO0_ISR1H for alternate 1 settings */
> + val = in_be32(vaddr + GPIO0_ISR1H);
> + out_be32((vaddr + GPIO0_ISR1H), val | 0x4200000);

setbits32 might be even simpler.

> + /* GPIO0_OSRH for alternate 1 settings */
> + val = in_be32(vaddr + GPIO0_OSRH);
> + out_be32((vaddr + GPIO0_OSRH), val | 0x42000000);

Same here.

> + /* GPIO0_TSRH for alternate 1 settings */
> + val = in_be32(vaddr + GPIO0_TSRH);
> + out_be32((vaddr + GPIO0_TSRH), val | 0x42000000);

And here.

And I suggest to add a few comments to the code explaining why exactly you are
setting/clearing the bits in the BCSR and the GPIO registers.

Cheers,
Stefan


\
 
 \ /
  Last update: 2010-11-23 14:23    [from the cache]
©2003-2011 Jasper Spaans