Some more comments... On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi <varun.se...@freescale.com> wrote: > +/* Handling access violations */ > +#define make64(high, low) (((u64)(high) << 32) | (low)) > + > +struct pamu_isr_data { > + void __iomem *pamu_reg_base; /* Base address of PAMU regs*/ > + unsigned int count; /* The number of PAMUs */ > +}; > + > +static struct paace *ppaact; > +static struct paace *spaact; > +static struct ome *omt; > + > +/* maximum subwindows permitted per liodn */ > +unsigned int max_subwindow_count; > +/* Number of SPAACT entries */ > +unsigned long max_subwins;
I don't like that these variables are not static... and they are referenced directly from code in fsl_pamu_domain.c. It would be better if fsl_pamu_domain.c called an accessor function-- like pamu_get_max_subwins. > +/* Pool for fspi allocation */ > +struct gen_pool *spaace_pool; spaace_pool should be static? I'm wondering if you should change pamu_isr_data into a more general struct analagous to struct intel_iommu. You could put in there the max # of subwins, etc. You could then provide an accessor to get at that data. [cut] > +/** > + * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves > subwindows > + * required for primary PAACE in the secondary > + * PAACE table. > + * @subwin_cnt: Number of subwindows to be reserved. > + * > + * A PPAACE entry may have a number of associated subwindows. A subwindow > + * corresponds to a SPAACE entry in the SPAACT table. Each PAACE entry stores > + * the index (fspi) of the first SPAACE entry in the SPAACT table. This > + * function returns the index of the first SPAACE entry. The remaining > + * SPAACE entries are reserved contiguously from that index. > + * > + * Returns a valid fspi index in the range of 0 - max_subwins on success. > + * If no SPAACE entry is available or the allocator can not reserve the > required > + * number of contiguous entries function returns ULONG_MAX indicating a > failure. > + * > +*/ > +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt) > +{ > + unsigned long spaace_addr; > + > + spaace_addr = gen_pool_alloc(spaace_pool, subwin_cnt * sizeof(struct > paace)); > + if (!spaace_addr) > + return ULONG_MAX; > + > + return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace)); > +} In order to keep things symmetric (with the free function) can we just call the above function: pamu_alloc_subwins() > +/* Release the subwindows reserved for a particular LIODN */ > +void pamu_free_subwins(int liodn) > +{ > + struct paace *ppaace; > + u32 subwin_cnt, size; > + > + ppaace = pamu_get_ppaace(liodn); > + if (!ppaace) { > + pr_err("Invalid liodn entry\n"); > + return; > + } > + > + if (get_bf(ppaace->addr_bitfields, PPAACE_AF_MW)) { > + subwin_cnt = 1UL << (get_bf(ppaace->impl_attr, PAACE_IA_WCE) > + 1); > + size = (subwin_cnt - 1) * sizeof(struct paace); > + gen_pool_free(spaace_pool, (unsigned > long)&spaact[ppaace->fspi], size); > + set_bf(ppaace->addr_bitfields, PPAACE_AF_MW, 0); > + } > +} [cut] > +/** > + * get_stash_id - Returns stash destination id corresponding to a > + * cache type and vcpu. > + * @stash_dest_hint: L1, L2 or L3 > + * @vcpu: vpcu target for a particular cache type. > + * > + * Returs stash on success or ~(u32)0 on failure. > + * > + */ > +u32 get_stash_id(u32 stash_dest_hint, u32 vcpu) > +{ The stash dest is really not a hint, right? It's the requested stash destination. So maybe just drop 'hint' from the name. The CPU here is really a physical CPU number and has nothing to do with vcpus I think. vcpu implies the index is inside a virtual machine...but this API is generic and may or may not be used with KVM. > + > +/* > + * Get the maximum number of PAACT table entries > + * and subwindows supported by PAMU > + */ > +static void get_pamu_cap_values(unsigned long pamu_reg_base) > +{ > + u32 pc_val; > + > + pc_val = in_be32((u32 *)(pamu_reg_base + PAMU_PC3)); > + /* Maximum number of subwindows per liodn */ > + max_subwindow_count = 1 << (1 + PAMU_PC3_MWCE(pc_val)); > + /* Total number of SPACCT entries */ > + max_subwins = PAACE_NUMBER_ENTRIES * max_subwindow_count; > +} If you follow the suggestion at the top of this file, this function would become something like-- init_pamu_capabilities(). And then create an accessor function to access max subwins, etc. Also, BTW, I don't see any support for the DOMAIN_ATTR_WINDOWS attribute in your patch. Was that coming in a later patch? [cut > +static int __init fsl_pamu_probe(struct platform_device *pdev) > +{ > + void __iomem *pamu_regs = NULL; > + struct ccsr_guts __iomem *guts_regs = NULL; > + u32 pamubypenr, pamu_counter; > + unsigned long pamu_reg_off; > + unsigned long pamu_reg_base; > + struct pamu_isr_data *data; > + struct device_node *guts_node; > + u64 size; > + struct page *p; > + int ret = 0; > + int irq; > + phys_addr_t ppaact_phys; > + phys_addr_t spaact_phys; > + phys_addr_t omt_phys; > + size_t mem_size = 0; > + unsigned int order = 0; > + u32 csd_port_id = 0; > + unsigned i; > + /* > + * enumerate all PAMUs and allocate and setup PAMU tables > + * for each of them, > + * NOTE : All PAMUs share the same LIODN tables. > + */ > + > + pamu_regs = of_iomap(pdev->dev.of_node, 0); > + if (!pamu_regs) { > + dev_err(&pdev->dev, "ioremap of PAMU node failed\n"); > + return -ENOMEM; Any reason not to be consistent with the other error handling-- set ret and goto error"? > + } > + of_get_address(pdev->dev.of_node, 0, &size, NULL); > + > + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (irq == NO_IRQ) { > + dev_warn(&pdev->dev, "no interrupts listed in PAMU node\n"); > + goto error; > + } > + > + data = kzalloc(sizeof(struct pamu_isr_data), GFP_KERNEL); > + if (!data) { > + iounmap(pamu_regs); > + return -ENOMEM; Any reason not to be consistent with the other error handling-- set ret and goto error"? Stuart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/