Re: [PATCH v1] ppc44x:PHY fixup for USB on canyonlands board

[Stefan Roese <sr@denx.de>]

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