[PATCH 2/2 RESEND] ARM: prima2: add NetWork on Chip driver for atlas7

[arnd@arndb.de (Arnd Bergmann)]

On Tuesday 14 April 2015 11:55:56 Barry Song wrote:
> diff --git a/Documentation/devicetree/bindings/bus/atlas7-noc.txt b/Documentation/devicetree/bindings/bus/atlas7-noc.txt
> new file mode 100644
> index 0000000..449ddb5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/atlas7-noc.txt
> @@ -0,0 +1,34 @@
> +Device tree bindings for CSRatlas7 NoC(Network on Chip)
> +
> +CSR atlas7 uses a NoC bus, SoC is splitted into mutiple MACROs. Every MACRO
> +holds some hardware modules. For each MACRO
> +properties:
> +- compatible : Should be "arteris, flexnoc"
> +- #address-cells: should be 1
> +- #size-cells: should be 1
> +- ranges : the child address space are mapped 1:1 onto the parent address space
> +
> +Sub-nodes:
> +All the devices connected to noc are described using sub-node to noc. For
> +example, AUDMSCM MARCO includes multimediam nodes such as KAS, AC97, IACC, I2S,
> +USP0~3, LVDS.
> +For each MARCO, there is at least a firewall sub-node. This firewall can detect
> +illegal hardware access for security protection.
> +
> +Firewall sub-nodes:
> +properties:
> +- compatible : Should be one of
> +	"sirf,nocfw-cpum",
> +	"sirf,nocfw-cgum",
> +	"sirf,nocfw-btm",
> +	"sirf,nocfw-gnssm",
> +	"sirf,nocfw-gpum",
> +	"sirf,nocfw-mediam",
> +	"sirf,nocfw-vdifm",
> +	"sirf,nocfw-audiom",
> +	"sirf,nocfw-ddrm",
> +	"sirf,nocfw-rtcm",
> +	"sirf,nocfw-dramfw",
> +	"sirf,nocfw-spramfw"
> +- reg: A resource specifier for the register space
> +- interrupts : Should be the interrupt number - optional

I think we should have a more generic binding here, which describes the
differences between the instances of this bus in separate properties
rather than keying them off the compatible string.

That way you get a simpler driver that is automatically reusable
across chip generations, potentially using small extensions, but
not per-chip instance lists.

> diff --git a/arch/arm/mach-prima2/common.c b/arch/arm/mach-prima2/common.c
> index 8cadb30..4a9dcad 100644
> --- a/arch/arm/mach-prima2/common.c
> +++ b/arch/arm/mach-prima2/common.c
> @@ -15,6 +15,13 @@
>  #include <linux/of_platform.h>
>  #include "common.h"
>  
> +static void __init sirfsoc_init_mach(void)
> +{
> +	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +
> +	sirfsoc_noc_init();
> +}
> +
>  static void __init sirfsoc_init_late(void)
>  {
>  	sirfsoc_pm_init();
> @@ -60,6 +67,7 @@ static const char *const atlas7_dt_match[] __initconst = {
>  DT_MACHINE_START(ATLAS7_DT, "Generic ATLAS7 (Flattened Device Tree)")
>  	/* Maintainer: Barry Song <baohua.song@csr.com> */
>  	.smp            = smp_ops(sirfsoc_smp_ops),
> +	.init_machine   = sirfsoc_init_mach,
>  	.dt_compat      = atlas7_dt_match,
>  MACHINE_END
>  #endif

I think we can avoid adding this part.
> +#define ddrm_SecureState_ReadClr0    0x1054
> +#define ddrm_SecureState_ReadSts0    0x1058
> +
> +#define ddrm_SecureState_ReadSet1    0x105C
> +#define ddrm_SecureState_ReadClr1    0x1060
> +#define ddrm_SecureState_ReadSts1    0x1064
> +
> +#define ddrm_SecureState_ReadSet2    0x1068
> +#define ddrm_SecureState_ReadClr2    0x106C
> +#define ddrm_SecureState_ReadSts2    0x1070
> +
> +#define ddrm_SecureState_ReadSet3    0x1074
> +#define ddrm_SecureState_ReadClr3    0x1078
> +#define ddrm_SecureState_ReadSts3    0x107C
> +
> +
> +#define ddrm_SecureState_WriteSet0    0x1080
> +#define ddrm_SecureState_WriteClr0    0x1084
> +#define ddrm_SecureState_WriteSts0    0x1088
> +
> +#define ddrm_SecureState_WriteSet1    0x108C
> +#define ddrm_SecureState_WriteClr1    0x1090
> +#define ddrm_SecureState_WriteSts1    0x1094
> +
> +#define ddrm_SecureState_WriteSet2    0x1098
> +#define ddrm_SecureState_WriteClr2    0x109C
> +#define ddrm_SecureState_WriteSts2    0x10A0
> +
> +#define ddrm_SecureState_WriteSet3    0x10A4
> +#define ddrm_SecureState_WriteClr3    0x10A8
> +#define ddrm_SecureState_WriteSts3    0x10AC
> +
> +struct dramfw_reg_secure_t {
> +	u32 readset;
> +	u32 readclr;
> +	u32 writeset;
> +	u32 writeclr;
> +};
>

Can you move the dram controller parts into a child driver at
drivers/memory?

> +static struct dramfw_reg_secure_t dramfw_reg_secure_list[] = {
> +	{ddrm_SecureState_ReadSet0, ddrm_SecureState_ReadClr0,
> +		ddrm_SecureState_WriteSet0, ddrm_SecureState_WriteClr0},
> +	{ddrm_SecureState_ReadSet1, ddrm_SecureState_ReadClr1,
> +		ddrm_SecureState_WriteSet1, ddrm_SecureState_WriteClr1},
> +	{ddrm_SecureState_ReadSet2, ddrm_SecureState_ReadClr2,
> +		ddrm_SecureState_WriteSet2, ddrm_SecureState_WriteClr2},
> +	{ddrm_SecureState_ReadSet3, ddrm_SecureState_ReadClr3,
> +		ddrm_SecureState_WriteSet3, ddrm_SecureState_WriteClr3}
> +};
> +
> +struct noc_info_t {
> +	const char *desc;
> +};
> +
> +static struct noc_info_t noc_initator_id_list[] = {
> +	{"dmac2_ac97_aux_fifo"},
> +	{"kas_dram"},
> +	{"afe_cvd_vip0"},
> +	{"usp0_axi_i"},
> +	{"sgx"},
> +	{"sdr"},
> +	{"dmac2_usp1rx"},
> +	{"dmac2_usp1tx"},

This table seems artificially soc specific. 

> +enum NOC_MACRO_IDX {
> +	CPUM_IDX = 0,
> +	CGUM_IDX,
> +	BTM_IDX,
> +	GNSSM_IDX,
> +	GPUM_IDX,
> +	MEDIAM_IDX,
> +	VDIFM_IDX,
> +	AUDIOM_IDX,
> +	DDRM_IDX,
> +	RTCM_IDX,
> +	DRAMFW_IDX,
> +	SPRFW_IDX,
> +};
> +
> +/*register firewall offset based on macro index*/
> +static u32 noc_regfw_offset_list[10] = {0x1050, 0x50, 0x1050, 0x1050,
> +		0x1050, 0x1050, 0x2050, 0x2050, 0x2050, 0x2050};
> +
> +/*dram firewall sched port:six*/
> +#define FW_A7 0x0
> +#define FW_DDR_BE 0x4000
> +#define FW_DDR_RTLL 0x8000
> +#define FW_DDR_RT   0xC000
> +#define FW_DDR_SGX 0x10000
> +#define FW_DDR_VXD 0x14000
> +
> +#define NOC_MACRO_NUM 12
> +#define NOC_MACRO_NAME_LEN 12
> +
> +struct noc_macro {
> +	void __iomem *mbase;
> +	spinlock_t		lock;
> +	u32 idx;
> +	u32 irq;
> +	u32 errlogoff;
> +	u32 faultenoff;
> +	char name[NOC_MACRO_NAME_LEN];
> +	int (*init_macro)(struct platform_device *);
> +};
> +
> +static int noc_macro_init(struct platform_device *);
> +static int noc_spram_firewall_init(struct platform_device *);
> +static int noc_dram_firewall_init(struct platform_device *);
> +static int noc_a7_init(struct platform_device *);

In general, please try to avoid forward declarations within on C file,
and just reorder the code appropriately.

> +static struct noc_macro noc_macro_list[] = {
> +	{
> +		.name = "cpum",
> +		.idx = CPUM_IDX,
> +		.errlogoff = NOC_CPUM_ERRLOG,
> +		.faultenoff = NOC_CPUM_FAULTEN,
> +		.init_macro = noc_a7_init,
> +	}, {
> +		.name = "cgum",
> +		.idx = CGUM_IDX,
> +	}, {
> +		.name = "btm",
> +		.idx = BTM_IDX,
> +	}, {
> +		.name = "gnssm",
> +		.idx = GNSSM_IDX,
> +	}, {
> +		.name = "gpum",
> +		.idx = GPUM_IDX,
> +	}, {
> +		.name = "mediam",
> +		.idx = MEDIAM_IDX,
> +	}, {
> +		.name = "vdifm",
> +		.idx = VDIFM_IDX,
> +	}, {
> +		.name = "audiom",
> +		.idx = AUDIOM_IDX,
> +		.errlogoff = NOC_AUDMSCM_ERRLOG,
> +		.faultenoff = NOC_AUDMSCM_FAULTEN,
> +		.init_macro = noc_macro_init,
> +	}, {
> +		.name = "ddrm",
> +		.idx = DDRM_IDX,
> +		.errlogoff = NOC_DDRM_ERRLOG,
> +		.faultenoff = NOC_DDRM_FAULTEN,
> +		.init_macro = noc_macro_init,
> +	}, {
> +		.name = "rtcm",
> +		.idx = RTCM_IDX,
> +		.errlogoff = NOC_RTCM_ERRLOG,
> +		.faultenoff = NOC_RTCM_FAULTEN,
> +		.init_macro = noc_macro_init,
> +	}, {
> +		.name = "dramfw",
> +		.idx = DRAMFW_IDX,
> +		.init_macro = noc_dram_firewall_init,
> +	}, {
> +		.name = "spramfw",
> +		.idx = SPRFW_IDX,
> +		.init_macro = noc_spram_firewall_init,
> +	},
> +};

The index values here could easily be DT properties, and you already
have a name for each from the device node. The fault enable and
error log can also be simple boolean properties.

> +static ssize_t regfw_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct noc_macro *nocm;
> +
> +	u32 idx, ns, a7, cssi, m3, kas;
> +
> +	if (sscanf(buf, "%x %x %x %x %x %x\n", &idx, &ns, &a7,
> +				&cssi, &m3, &kas) != 6 || idx > 9)
> +		return -EINVAL;
> +
> +	nocm = &noc_macro_list[idx];
> +	noc_regfw_set(nocm->mbase,
> +			noc_regfw_offset_list[idx], ns, a7, cssi, m3, kas);
> +	return len;
> +}
> +
> +static DEVICE_ATTR_WO(regfw);

Any new sysfs files need a full description in Documentation/ABI.

Without that documentation, it's also hard to tell whether this ABI
is good.

> +__init int sirfsoc_noc_init(void)
> +{
> +	struct device_node *np;
> +	const struct of_device_id *match;
> +	struct noc_macro *nocm;
> +	struct platform_device *pdev;
> +
> +	for_each_matching_node_and_match(np, sirfsoc_nocfw_ids, &match) {
> +		if (!of_device_is_available(np))
> +			continue;
> +
> +		nocm = (struct noc_macro *)match->data;
> +		nocm->mbase = of_iomap(np, 0);
> +		if (!nocm->mbase)
> +			return -ENOMEM;
> +
> +		spin_lock_init(&nocm->lock);
> +		pdev = of_find_device_by_node(np);
> +		platform_set_drvdata(pdev, nocm);
> +
> +		hook_fault_code(8, noc_abort_handler, SIGBUS, 0,
> +			"external abort on non-linefetch");
> +
> +		hook_fault_code(22, noc_abort_handler, SIGBUS, 0,
> +			"imprecise external abort");
> +
> +		if (nocm->init_macro)
> +			nocm->init_macro(pdev);
> +	}

This should become simpler if you have a platform driver for the
base device and use of_platform_populate to create its child devices.

	Arnd