From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Wed, 15 Apr 2015 17:53:12 +0200 Subject: [PATCH 2/2 RESEND] ARM: prima2: add NetWork on Chip driver for atlas7 In-Reply-To: <1429012556-14041-2-git-send-email-21cnbao@gmail.com> References: <1429012556-14041-1-git-send-email-21cnbao@gmail.com> <1429012556-14041-2-git-send-email-21cnbao@gmail.com> Message-ID: <4918744.ypGiyU8ujI@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > #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 */ > .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