Re: [PATCH 3/7] ARM: at91: introduce basic SAMA5D4 support

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Dear Alexandre Belloni,

On Thu, 11 Sep 2014 17:54:08 +0200, Alexandre Belloni wrote:

> +#ifdef CONFIG_CACHE_L2X0
> +static void __init at91_init_l2cache(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> +	if (!np)
> +		return;
> +	of_node_put(np);
> +
> +	l2x0_of_init(0, ~0UL);
> +}
> +#else
> +static inline void at91_init_l2cache(void) {}
> +#endif
> +
>  static void __init sama5_dt_device_init(void)
>  {
> +	at91_init_l2cache();

Following Russell's cleanup of the L2 cache code, I don't think this is
necessary. The l2x0_of_init() function is automatically called for you
in init_IRQ(), as long as one of the l2c_aux_mask or l2c_aux_val fields
of your DT_MACHINE structure are non zero:

void __init init_IRQ(void)
{
        int ret;

        if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq)
                irqchip_init();
        else
                machine_desc->init_irq();

        if (IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_CACHE_L2X0) &&
            (machine_desc->l2c_aux_mask || machine_desc->l2c_aux_val)) {
                outer_cache.write_sec = machine_desc->l2c_write_sec;
                ret = l2x0_of_init(machine_desc->l2c_aux_val,
                                   machine_desc->l2c_aux_mask);
                if (ret)
                        pr_err("L2C: failed to init: %d\n", ret);
        }
}

Also, l2x0_of_init() already has a stub definition when
CONFIG_CACHE_L2X0 is not enabled, so the compile time conditional is
not necessary.

> +/* Firmware */
> +extern void atmel_firmware_init(void);
> +extern bool atmel_firmware_is_registered(void);

What is this firmware?

> diff --git a/arch/arm/mach-at91/include/mach/sama5d4.h b/arch/arm/mach-at91/include/mach/sama5d4.h
> new file mode 100644
> index 000000000000..af835e89d4c5
> --- /dev/null
> +++ b/arch/arm/mach-at91/include/mach/sama5d4.h
> @@ -0,0 +1,35 @@
> +/*
> + * Chip-specific header file for the SAMA5D4 family
> + *
> + *  Copyright (C) 2013 Atmel Corporation,
> + *                     Nicolas Ferre <nicolas.ferre@atmel.com>
> + *
> + * Common definitions.
> + * Based on SAMA5D4 datasheet.
> + *
> + * Licensed under GPLv2 or later.
> + */
> +
> +#ifndef SAMA5D4_H
> +#define SAMA5D4_H
> +
> +/*
> + * User Peripheral physical base addresses.
> + */
> +#define SAMA5D4_BASE_AIC	0xfc06e000 /* (AIC non-secure) Base Address */
> +#define SAMA5D4_BASE_USART3	0xfc00c000 /* (USART3 non-secure) Base Address */
> +#define SAMA5D4_BASE_PMC	0xf0018000 /* (PMC) Base Address */
> +#define SAMA5D4_BASE_MPDDRC	0xf0010000 /* (MPDDRC) Base Address */
> +#define SAMA5D4_BASE_PIOD	0xfc068000 /* (PIOD) Base Address */
> +#define SAMA5D4_BASE_PIOE	0xfc06d000 /* (PIOE) Base Address */

Are these definitions still all necessary? With the migration of most
(all?) peripherals to the Device Tree, less and less of those
definitions should be useful. For example, isn't the AIC fully
described in the Device Tree now?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com