Re: [4/5] powerpc: PA Semi PWRficient platform support

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 05 September 2006 19:29, Olof Johansson wrote:
> Base patch for PA6T and PA6T-1682M. This introduces the
> arch/powerpc/platform/pasemi directory, together with basic
> implementations for various setup.
> 
> Much of this was based on other platform code, i.e. Maple, etc.
 
Very nice patch set, as expected ;-)

See below for the mandatory nitpicking.


> Index: merge/arch/powerpc/Kconfig
> ===================================================================
> --- merge.orig/arch/powerpc/Kconfig
> +++ merge/arch/powerpc/Kconfig
> @@ -413,6 +409,17 @@ config PPC_MAPLE
>            This option enables support for the Maple 970FX Evaluation Board.
>  	  For more informations, refer to <http://www.970eval.com>
>  
> +config PPC_PASEMI
> +	depends on PPC_MULTIPLATFORM && PPC64
> +	bool "PA Semi SoC-based platforms"
> +	default n
> +	select MPIC
> +	select PPC_UDBG_16550
> +	select GENERIC_TBSYNC
> +	help
> +	  This option enables support for PA Semi's PWRficient line
> +	  of SoC processors, including PA6T-1682M

IIRC, the GENERIC_TBSYNC code is really inefficient. Don't you
have any other way of implementing that?

> +
> +#undef DEBUG
> +

I know that this is done in other places as well, but it seems
rather pointless, and it provides setting DEBUG from EXTRA_CFLAGS
in the Makefile.


> +static struct pci_ops pa_pxp_ops =
> +{
> +	pa_pxp_read_config,
> +	pa_pxp_write_config
> +};

spacing: '{' after '=', and ',' after the final member.

> +
> +static void __init setup_pa_pxp(struct pci_controller* hose)
> +{
> +	hose->ops = &pa_pxp_ops;
> +	hose->cfg_data = ioremap(0xe0000000, 0x1000000);
> +}

Shouldn't that be in the device tree?

> +	bus_range = (int *) get_property(dev, "bus-range", &len);

Unneeded cast

> +	if (bus_range == NULL || len < 2 * sizeof(int)) {
> +		printk(KERN_WARNING "Can't get bus-range for %s, assume bus 0\n",
> +		dev->full_name);
> +	}

if (!bus_range || len < 2 * sizeof(int)) {

> +
> +	hose = pcibios_alloc_controller(dev);
> +	if (hose == NULL)
> +		return -ENOMEM;

if (!hose)

> +	if (root == NULL) {

if (!root)
> +#undef DEBUG

remove

> +extern int pas_set_rtc_time(struct rtc_time *tm);
> +extern void pas_get_rtc_time(struct rtc_time *tm);
> +extern unsigned long pas_get_boot_time(void);
> +extern void pas_pci_init(void);
> +extern void pas_pcibios_fixup(void);

Extern declarations should never be in a source file, please
move them to a header

> +static void iommu_dev_setup_null(struct pci_dev *dev) { }
> +static void iommu_bus_setup_null(struct pci_bus *bus) { }
> +
> +static void __init pas_init_early(void)
> +{
> +	/* No iommu code yet */
> +	ppc_md.iommu_dev_setup = iommu_dev_setup_null;
> +	ppc_md.iommu_bus_setup = iommu_bus_setup_null;
> +	pci_direct_iommu_init();
> +}
> +
> +/* No legacy IO on our parts */
> +static int pas_check_legacy_ioport(unsigned int baseport)
> +{
> +	return -ENODEV;
> +}

Should we maybe change the default behavior so that you don't need
to provide nops for these functions?

> +
> +static __init void pas_init_IRQ(void)
> +{
> +	struct device_node *np = NULL;
> +	struct device_node *root, *mpic_node  = NULL;
> +	unsigned long openpic_addr = 0;

These three should not be initialized.


> +	BUG_ON(mpic == NULL);

BUG_ON(!mpic);

> +
> +#undef DEBUG

remove

> +void pas_get_rtc_time(struct rtc_time *tm)
> +{
> +}
> +
> +int pas_set_rtc_time(struct rtc_time *tm)
> +{
> +	return -ENODEV;
> +}

again, it probably makes sense to not have to provide these.

	Arnd <><