On Mon, 2011-02-28 at 08:57 +0100, Richard Cochran wrote: > The eTSEC includes a PTP clock with quite a few features. This patch adds > support for the basic clock adjustment functions, plus two external time > stamps, one alarm, and the PPS callback. Just a minor question on the locking, but otherwise looks ok. > --- /dev/null > +++ b/drivers/net/gianfar_ptp.c > @@ -0,0 +1,579 @@ [snip] > +/* > + * Register access functions > + */ So what are the locking rules on the functions below? I assume the etsects->lock needs to be held prior to calling, so that should be made explicit in a comment. > +static u64 tmr_cnt_read(struct etsects *etsects) > +{ > + u64 ns; > + u32 lo, hi; > + > + lo = gfar_read(&etsects->regs->tmr_cnt_l); > + hi = gfar_read(&etsects->regs->tmr_cnt_h); > + ns = ((u64) hi) << 32; > + ns |= lo; > + return ns; > +} > + > +static void tmr_cnt_write(struct etsects *etsects, u64 ns) > +{ > + u32 hi = ns >> 32; > + u32 lo = ns & 0xffffffff; > + > + gfar_write(&etsects->regs->tmr_cnt_l, lo); > + gfar_write(&etsects->regs->tmr_cnt_h, hi); > +} > + > +static void set_alarm(struct etsects *etsects) > +{ > + u64 ns; > + u32 lo, hi; > + > + ns = tmr_cnt_read(etsects) + 1500000000ULL; > + ns = div_u64(ns, 1000000000UL) * 1000000000ULL; > + ns -= etsects->tclk_period; > + hi = ns >> 32; > + lo = ns & 0xffffffff; > + gfar_write(&etsects->regs->tmr_alarm1_l, lo); > + gfar_write(&etsects->regs->tmr_alarm1_h, hi); > +} > + > +static void set_fipers(struct etsects *etsects) > +{ > + u32 tmr_ctrl = gfar_read(&etsects->regs->tmr_ctrl); > + > + gfar_write(&etsects->regs->tmr_ctrl, tmr_ctrl & (~TE)); > + gfar_write(&etsects->regs->tmr_prsc, etsects->tmr_prsc); > + gfar_write(&etsects->regs->tmr_fiper1, etsects->tmr_fiper1); > + gfar_write(&etsects->regs->tmr_fiper2, etsects->tmr_fiper2); > + set_alarm(etsects); > + gfar_write(&etsects->regs->tmr_ctrl, tmr_ctrl|TE); > +} > + [snip] > +static int gianfar_ptp_probe(struct platform_device *dev) > +{ > + struct device_node *node = dev->dev.of_node; > + struct etsects *etsects; > + struct timespec now; > + int err = -ENOMEM; > + u32 tmr_ctrl; > + > + etsects = kzalloc(sizeof(*etsects), GFP_KERNEL); > + if (!etsects) > + goto no_memory; > + > + err = -ENODEV; > + > + etsects->caps = ptp_gianfar_caps; > + etsects->cksel = DEFAULT_CKSEL; > + > + if (get_of_u32(node, "fsl,tclk-period", &etsects->tclk_period) || > + get_of_u32(node, "fsl,tmr-prsc", &etsects->tmr_prsc) || > + get_of_u32(node, "fsl,tmr-add", &etsects->tmr_add) || > + get_of_u32(node, "fsl,tmr-fiper1", &etsects->tmr_fiper1) || > + get_of_u32(node, "fsl,tmr-fiper2", &etsects->tmr_fiper2) || > + get_of_u32(node, "fsl,max-adj", &etsects->caps.max_adj)) { > + pr_err("device tree node missing required elements\n"); > + goto no_node; > + } > + > + etsects->irq = platform_get_irq(dev, 0); > + > + if (etsects->irq == NO_IRQ) { > + pr_err("irq not in device tree\n"); > + goto no_node; > + } > + if (request_irq(etsects->irq, isr, 0, DRIVER, etsects)) { > + pr_err("request_irq failed\n"); > + goto no_node; > + } > + > + etsects->rsrc = platform_get_resource(dev, IORESOURCE_MEM, 0); > + if (!etsects->rsrc) { > + pr_err("no resource\n"); > + goto no_resource; > + } > + if (request_resource(&ioport_resource, etsects->rsrc)) { > + pr_err("resource busy\n"); > + goto no_resource; > + } > + > + spin_lock_init(&etsects->lock); > + > + etsects->regs = ioremap(etsects->rsrc->start, > + 1 + etsects->rsrc->end - etsects->rsrc->start); > + if (!etsects->regs) { > + pr_err("ioremap ptp registers failed\n"); > + goto no_ioremap; > + } > + getnstimeofday(&now); > + ptp_gianfar_settime(&etsects->caps, &now); > + > + tmr_ctrl = > + (etsects->tclk_period & TCLK_PERIOD_MASK) << TCLK_PERIOD_SHIFT | > + (etsects->cksel & CKSEL_MASK) << CKSEL_SHIFT; > + > + gfar_write(&etsects->regs->tmr_ctrl, tmr_ctrl); > + gfar_write(&etsects->regs->tmr_add, etsects->tmr_add); > + gfar_write(&etsects->regs->tmr_prsc, etsects->tmr_prsc); > + gfar_write(&etsects->regs->tmr_fiper1, etsects->tmr_fiper1); > + gfar_write(&etsects->regs->tmr_fiper2, etsects->tmr_fiper2); > + set_alarm(etsects); > + gfar_write(&etsects->regs->tmr_ctrl, tmr_ctrl|FS|RTPE|TE); Does any of the above need a lock should an irq land in the middle of the writes? thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html