On Fri, Feb 24, 2017 at 12:07:11PM -0700, Mathieu Poirier wrote:
> On Thu, Feb 23, 2017 at 09:57:47AM +0800, Leo Yan wrote:
[...]
> > +/* bits definition for EDDEVID1 */
> > +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> > +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> > +#define EDDEVID1_PCSR_NO_OFFSET (0x1)
>
> Hi Leo,
>
> The above #define isn't used in the code - please remove.
Thanks a lot for quite detailed reivew and suggestion for below
refactors. I will follow up them and send out new version for
reviewing.
Thanks,
Leo Yan
> > +
> > +/* bits definition for EDDEVID */
> > +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> > +#define EDDEVID_IMPL_NONE (0x0)
> > +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> > +#define EDDEVID_IMPL_FULL (0x3)
> > +
> > +struct debug_drvdata {
> > + void __iomem *base;
> > + struct device *dev;
> > + int cpu;
> > + struct clk *dbg_clk;
> > +
> > + bool edpcsr_present;
> > + bool edvidsr_present;
> > + bool pc_has_offset;
> > +
> > + u32 eddevid;
> > + u32 eddevid1;
> > +
> > + u32 edpcsr;
> > + u32 edpcsr_hi;
> > + u32 edprsr;
> > + u32 edvidsr;
> > + u32 edcidsr;
> > +};
> > +
> > +static int debug_count;
>
> No need to make this global - please move this to debug_probe()
>
> > +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> > +
> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
> > +{
> > + /* Unlocks the debug registers */
> > + writel_relaxed(0x0, drvdata->base + EDOSLAR);
> > + wmb();
> > +}
> > +
> > +/*
> > + * According to ARM DDI 0487A.k, before access external debug
> > + * registers should firstly check the access permission; if any
> > + * below condition has been met then cannot access debug
> > + * registers to avoid lockup issue:
> > + *
> > + * - CPU power domain is powered off;
> > + * - The OS Double Lock is locked;
> > + *
> > + * By checking EDPRSR can get to know if meet these conditions.
> > + */
> > +static bool debug_access_permit(struct debug_drvdata *drvdata)
>
> s/debug_access_permit/debug_access_permitted
>
> > +{
> > + /* CPU is powered off */
> > + if (!(drvdata->edprsr & EDPRSR_PU))
> > + return false;
> > +
> > + /* The OS Double Lock is locked */
> > + if (drvdata->edprsr & EDPRSR_DLK)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static void debug_read_regs(struct debug_drvdata *drvdata)
> > +{
> > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> > +
> > + if (!debug_access_permit(drvdata))
> > + return;
> > +
> > + if (!drvdata->edpcsr_present)
> > + return;
> > +
> > + CS_UNLOCK(drvdata->base);
> > +
> > + debug_os_unlock(drvdata);
> > +
> > + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR);
> > +
> > + /*
> > + * As described in ARM DDI 0487A.k, if the processing
> > + * element (PE) is in debug state, or sample-based
> > + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF;
> > + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become
> > + * UNKNOWN state. So directly bail out for this case.
> > + */
> > + if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
> > + CS_LOCK(drvdata->base);
> > + return;
> > + }
> > +
> > + /*
> > + * A read of the EDPCSR normally has the side-effect of
> > + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI;
> > + * at this point it's safe to read value from them.
> > + */
> > + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> > +#ifdef CONFIG_64BIT
> > + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> > +#endif
> > +
> > + if (drvdata->edvidsr_present)
> > + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> > +
> > + CS_LOCK(drvdata->base);
> > +}
> > +
> > +#ifndef CONFIG_64BIT
> > +static bool debug_pc_has_offset(struct debug_drvdata *drvdata)
> > +{
> > + u32 pcsr_offset;
> > +
> > + pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> > +
> > + return (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> > +}
> > +
> > +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata,
> > + unsigned long pc)
> > +{
> > + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> > +
> > + if (debug_pc_has_offset(drvdata)) {
> > + arm_inst_offset = 8;
> > + thumb_inst_offset = 4;
> > + }
> > +
> > + /* Handle thumb instruction */
> > + if (pc & EDPCSR_THUMB) {
> > + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset;
> > + return pc;
> > + }
> > +
> > + /*
> > + * Handle arm instruction offset, if the arm instruction
> > + * is not 4 byte alignment then it's possible the case
> > + * for implementation defined; keep original value for this
> > + * case and print info for notice.
> > + */
> > + if (pc & BIT(1))
> > + pr_emerg("Instruction offset is implementation defined\n");
> > + else
> > + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset;
> > +
> > + return pc;
> > +}
> > +#endif
> > +
> > +static void debug_dump_regs(struct debug_drvdata *drvdata)
> > +{
> > + unsigned long pc;
> > +
> > + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr,
> > + drvdata->edprsr & EDPRSR_PU ? "On" : "Off",
> > + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock");
> > +
> > + if (!debug_access_permit(drvdata) || !drvdata->edpcsr_present) {
> > + pr_emerg("No permission to access debug registers!\n");
> > + return;
> > + }
> > +
> > + if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
> > + pr_emerg("CPU is in Debug state or profiling is prohibited!\n");
> > + return;
> > + }
> > +
> > +#ifdef CONFIG_64BIT
> > + pc = (unsigned long)drvdata->edpcsr_hi << 32 |
> > + (unsigned long)drvdata->edpcsr;
> > +#else
> > + pc = debug_adjust_pc(drvdata, (unsigned long)drvdata->edpcsr);
> > +#endif
> > +
> > + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> > + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> > +
> > + if (!drvdata->edvidsr_present)
> > + return;
> > +
> > + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%s VMID:%x)\n",
> > + drvdata->edvidsr,
> > + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure",
> > + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" :
> > + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"),
> > + drvdata->edvidsr & EDVIDSR_HV ? "64bits" : "32bits",
> > + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> > +}
> > +
> > +/*
> > + * Dump out information on panic.
> > + */
> > +static int debug_notifier_call(struct notifier_block *self,
> > + unsigned long v, void *p)
> > +{
> > + int cpu;
> > +
> > + pr_emerg("ARM external debug module:\n");
> > +
> > + for_each_possible_cpu(cpu) {
> > +
> > + if (!per_cpu(debug_drvdata, cpu))
> > + continue;
> > +
> > + pr_emerg("CPU[%d]:\n", per_cpu(debug_drvdata, cpu)->cpu);
> > +
> > + debug_read_regs(per_cpu(debug_drvdata, cpu));
> > + debug_dump_regs(per_cpu(debug_drvdata, cpu));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct notifier_block debug_notifier = {
> > + .notifier_call = debug_notifier_call,
> > +};
> > +
> > +static void debug_init_arch_data(void *info)
> > +{
> > + struct debug_drvdata *drvdata = info;
> > + u32 mode;
> > +
> > + CS_UNLOCK(drvdata->base);
> > +
> > + debug_os_unlock(drvdata);
> > +
> > + /* Read device info */
> > + drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID);
> > + drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> > +
> > + /* Parse implementation feature */
> > + mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE;
> > + if (mode == EDDEVID_IMPL_FULL) {
> > + drvdata->edpcsr_present = true;
> > + drvdata->edvidsr_present = true;
> > + } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
> > + drvdata->edpcsr_present = true;
> > + drvdata->edvidsr_present = false;
> > + } else {
> > + drvdata->edpcsr_present = false;
> > + drvdata->edvidsr_present = false;
> > + }
> > +
> > + CS_LOCK(drvdata->base);
> > +}
> > +
> > +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> > +{
> > + int ret;
> > + void __iomem *base;
> > + struct device *dev = &adev->dev;
> > + struct coresight_platform_data *pdata = NULL;
> > + struct debug_drvdata *drvdata;
> > + struct resource *res = &adev->res;
> > + struct device_node *np = adev->dev.of_node;
> > +
> > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > + if (!drvdata)
> > + return -ENOMEM;
> > +
> > + if (np) {
> > + pdata = of_get_coresight_platform_data(dev, np);
> > + if (IS_ERR(pdata))
> > + return PTR_ERR(pdata);
> > + adev->dev.platform_data = pdata;
> > + }
>
> I wonder if dragging a coresight_platform_data is worth it. From what I see the
> only thing it is used for is to retrieve the CPU. As such I think it is better
> get the portion of code in of_get_coresight_platform_data() that deals with the
> CPU and create a new function (of_coresight_get_cpu()). That way you can reuse
> it here and in of_get_coresight_platform_data().
>
> > +
> > + drvdata->dev = &adev->dev;
> > + drvdata->dbg_clk = devm_clk_get(&adev->dev, "dbg_clk");
> > + if (IS_ERR(drvdata->dbg_clk)) {
> > + dev_err(dev, "debug clock initialization failed.\n");
> > + return PTR_ERR(drvdata->dbg_clk);
> > + }
> > +
> > + ret = clk_prepare_enable(drvdata->dbg_clk);
> > + if (ret)
> > + return ret;
> > +
> > + dev_set_drvdata(dev, drvdata);
> > +
> > + /* Validity for the resource is already checked by the AMBA core */
> > + base = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + drvdata->base = base;
> > + drvdata->cpu = pdata ? pdata->cpu : 0;
> > +
> > + get_online_cpus();
> > + per_cpu(debug_drvdata, drvdata->cpu) = drvdata;
> > +
> > + if (smp_call_function_single(drvdata->cpu,
> > + debug_init_arch_data, drvdata, 1))
> > + dev_err(dev, "Debug arch init failed\n");
> > +
> > + if (!debug_count++)
> > + atomic_notifier_chain_register(&panic_notifier_list,
> > + &debug_notifier);
> > + put_online_cpus();
>
> There is no need to hold the CPUs in place through the chain registration. This
> should go before the if().
>
> > +
> > + dev_info(dev, "%s initialized\n", (char *)id->data);
>
> fprintf(buffer, (char *)id->data, drvdata->cpu);
> dev_info(dev, "%s initialized\n", buffer);
>
> > + return 0;
> > +}
> > +
> > +static struct amba_id debug_ids[] = {
> > + { /* Debug for Cortex-A53 */
> > + .id = 0x000bbd03,
> > + .mask = 0x000fffff,
> > + .data = "debug",
>
> .data = "Coresight debug-CPU%d",
>
> > + },
> > + { /* Debug for Cortex-A57 */
> > + .id = 0x000bbd07,
> > + .mask = 0x000fffff,
> > + .data = "debug",
> > + },
> > + { /* Debug for Cortex-A72 */
> > + .id = 0x000bbd08,
> > + .mask = 0x000fffff,
> > + .data = "debug",
> > + },
> > + { 0, 0},
> > +};
> > +
> > +static struct amba_driver debug_driver = {
> > + .drv = {
> > + .name = "coresight-debug",
> > + .suppress_bind_attrs = true,
> > + },
> > + .probe = debug_probe,
> > + .id_table = debug_ids,
> > +};
> > +builtin_amba_driver(debug_driver);
> > --
> > 2.7.4
> >
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
