Re: [RFC PATCHv2 2/7] OMAP SSI: Introducing OMAP SSI driver

From: Carlos Chinea
Date: Tue May 18 2010 - 05:05:18 EST

Next message: Jean Delvare: "Re: [lm-sensors] [PATCH] hwmon: Add driver for intel PCI thermal subsystem"
Previous message: Mikael Starvik: "RE: [PATCH] cris:Check kmalloc return value before use data"
Next in thread: Sebastien Jan: "Re: [RFC PATCHv2 2/7] OMAP SSI: Introducing OMAP SSI driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 2010-05-14 at 16:41 +0200, ext Sebastien Jan wrote:
> Hi Carlos,
>
> Please see my comments inlined.
>
> On Friday 07 May 2010 17:18:32 Carlos Chinea wrote:
> [strip]
> > diff --git a/drivers/hsi/controllers/omap_ssi.c
> [strip]
> > +
> > +/**
> > + * struct omap_ssm_ctx - OMAP synchronous serial module (TX/RX) context
> > + * @mode: Bit transmission mode
> > + * @channels: Number of channels
> > + * @framesize: Frame size in bits
> > + * @timeout: RX frame timeout
> > + * @divisot: TX divider
>
> Typo: s/divisot/divisor
>
> > + * @arb_mode: Arbitration mode for TX frame (Round robin, priority)
> > + */
> > +struct omap_ssm_ctx {
> > + u32 mode;
> > + u32 channels;
> > + u32 frame_size;
> > + union {
> > + u32 timeout; /* Rx Only */
> > + struct {
> > + u32 arb_mode;
> > + u32 divisor;
> > + }; /* Tx only */
> > + };
> > +};
> > +
> > +/**
> > + * struct omap_ssi_port - OMAP SSI port data
> > + * @dev: device associated to the port (HSI port)
> > + * @sst_dma: SSI transmitter physical base address
> > + * @ssr_dma: SSI receiver physical base address
> > + * @sst_base: SSI transmitter base address
> > + * @ssr_base: SSI receiver base address
>
> wk_lock description is missing here
>
> > + * @lock: Spin lock to serialize access to the SSI port
> > + * @channels: Current number of channels configured (1,2,4 or 8)
> > + * @txqueue: TX message queues
> > + * @rxqueue: RX message queues
> > + * @brkqueue: Queue of incoming HWBREAK requests (FRAME mode)
> > + * @irq: IRQ number
> > + * @wake_irq: IRQ number for incoming wake line (-1 if none)
> > + * @pio_tasklet: Bottom half for PIO transfers and events
> > + * @wake_tasklet: Bottom half for incoming wake events
> > + * @wkin_cken: Keep track of clock references due to the incoming wake
> > line
> > + * @wake_refcount: Reference count for output wake line
>
> s/wake_refcount/wk_refcount
>
> > + * @sys_mpu_enable: Context for the interrupt enable register for irq 0
> > + * @sst: Context for the synchronous serial transmitter
> > + * @ssr: Context for the synchronous serial receiver
> > + */
> > +struct omap_ssi_port {
> > + struct device *dev;
> > + dma_addr_t sst_dma;
> > + dma_addr_t ssr_dma;
> > + unsigned long sst_base;
> > + unsigned long ssr_base;
> > + spinlock_t wk_lock;
> > + spinlock_t lock;
> > + unsigned int channels;
> > + struct list_head txqueue[SSI_MAX_CHANNELS];
> > + struct list_head rxqueue[SSI_MAX_CHANNELS];
> > + struct list_head brkqueue;
> > + unsigned int irq;
> > + int wake_irq;
> > + struct tasklet_struct pio_tasklet;
> > + struct tasklet_struct wake_tasklet;
> > + unsigned int wkin_cken:1; /* Workaround */
> > + int wk_refcount;
> > + /* OMAP SSI port context */
> > + u32 sys_mpu_enable; /* We use only one irq */
> > + struct omap_ssm_ctx sst;
> > + struct omap_ssm_ctx ssr;
> > +};
> > +
>
> [strip]
>
> > +
> > +static void ssi_pio_complete(struct hsi_port *port, struct list_head
> > *queue) +{
> > + struct hsi_controller *ssi =
> > to_hsi_controller(port->device.parent); + struct omap_ssi_controller
> > *omap_ssi = hsi_controller_drvdata(ssi); + struct omap_ssi_port
> > *omap_port = hsi_port_drvdata(port);
> > + struct hsi_msg *msg;
> > + u32 *buf;
> > + u32 val;
> > +
> > + spin_lock(&omap_port->lock);
> > + msg = list_first_entry(queue, struct hsi_msg, link);
> > + if ((!msg->sgt.nents) || (!msg->sgt.sgl->length)) {
> > + msg->actual_len = 0;
> > + msg->status = HSI_STATUS_PENDING;
> > + }
> > + if (msg->status == HSI_STATUS_PROCEDING) {
> > + buf = sg_virt(msg->sgt.sgl) + msg->actual_len;
> > + if (msg->ttype == HSI_MSG_WRITE)
> > + __raw_writel(*buf, omap_port->sst_base +
> > +
> > SSI_SST_BUFFER_CH_REG(msg->channel)); + else
> > + *buf = __raw_readl(omap_port->ssr_base +
> > +
> > SSI_SSR_BUFFER_CH_REG(msg->channel)); +
> > dev_dbg(&port->device, "ch %d ttype %d 0x%08x\n", msg->channel, +
> > msg->ttype, *buf); +
> > msg->actual_len += sizeof(*buf);
> > + if (msg->actual_len >= msg->sgt.sgl->length)
> > + msg->status = HSI_STATUS_COMPLETED;
> > + /*
> > + * Wait for the last written frame to be really sent before
> > + * we call the complete callback
> > + */
> > + if ((msg->status == HSI_STATUS_PROCEDING) ||
> > + ((msg->status == HSI_STATUS_COMPLETED) &&
> > + (msg->ttype == HSI_MSG_WRITE)))
> > + goto out;
> > +
> > + }
> > + if (msg->status == HSI_STATUS_PROCEDING)
> > + goto out;
> > + /* Transfer completed at this point */
> > + val = __raw_readl(omap_ssi->sys + SSI_MPU_ENABLE_REG(port->num,
> > 0)); + if (msg->ttype == HSI_MSG_WRITE) {
> > + val &= ~SSI_DATAACCEPT(msg->channel);
> > + ssi_clk_disable(ssi); /* Release clocks for write transfer
> > */ + } else {
> > + val &= ~SSI_DATAAVAILABLE(msg->channel);
> > + }
> > + __raw_writel(val, omap_ssi->sys + SSI_MPU_ENABLE_REG(port->num,
> > 0)); + list_del(&msg->link);
> > + spin_unlock(&omap_port->lock);
> > + msg->complete(msg);
> > + spin_lock(&omap_port->lock);
> > + ssi_start_transfer(queue);
> > +out:
> > + spin_unlock(&omap_port->lock);
> > +}
> > +
> > +static void ssi_gdd_complete(struct hsi_controller *ssi, unsigned int lch)
> > +{
> > + struct omap_ssi_controller *omap_ssi = hsi_controller_drvdata(ssi);
> > + struct hsi_msg *msg = omap_ssi->gdd_trn[lch].msg;
> > + struct hsi_port *port = to_hsi_port(msg->cl->device.parent);
> > + unsigned int dir;
> > + u32 csr;
> > + u32 val;
> > +
> > + spin_lock(&omap_ssi->lock);
> > +
> > + val = __raw_readl(omap_ssi->sys + SSI_GDD_MPU_IRQ_ENABLE_REG);
> > + val &= ~SSI_GDD_LCH(lch);
> > + __raw_writel(val, omap_ssi->sys + SSI_GDD_MPU_IRQ_ENABLE_REG);
> > +
> > + if (msg->ttype == HSI_MSG_READ) {
> > + dir = DMA_FROM_DEVICE;
> > + val = SSI_DATAAVAILABLE(msg->channel);
> > + ssi_clk_disable(ssi);
> > + } else {
> > + dir = DMA_TO_DEVICE;
> > + val = SSI_DATAACCEPT(msg->channel);
> > + /* Keep clocks reference for write pio event */
> > + }
> > + dma_unmap_sg(&ssi->device, msg->sgt.sgl, msg->sgt.nents, dir);
> > + csr = __raw_readw(omap_ssi->gdd + SSI_GDD_CSR_REG(lch));
> > + omap_ssi->gdd_trn[lch].msg = NULL; /* release GDD lch */
> > + if (csr & SSI_CSR_TOUR) { /* Timeout error */
> > + msg->status = HSI_STATUS_ERROR;
> > + msg->actual_len = 0;
> > + list_del(&msg->link); /* Dequeue msg */
> > + spin_unlock(&omap_ssi->lock);
> > + msg->complete(msg);
> > + return;
> > + }
> > +
> > + val |= __raw_readl(omap_ssi->sys + SSI_MPU_ENABLE_REG(port->num,
> > 0)); + __raw_writel(val, omap_ssi->sys +
> > SSI_MPU_ENABLE_REG(port->num, 0)); +
> > + msg->status = HSI_STATUS_COMPLETED;
> > + msg->actual_len = sg_dma_len(msg->sgt.sgl);
> > + spin_unlock(&omap_ssi->lock);
> > +}
>
> Don't you need to check the queue related to this transfer at this point, to
> start the potentially next queued transfer on the same channel? (calling
> ssi_start_transfer(), like in ssi_pio_complete()?)
>

No this is done in ssi_pio_complete(). Notice that we do not call the
complete callback at any point here. We just arm the pio interrupt for
that channel and transfer direction. AFAIK, this is the SW logic
expected by the OMAP SSI HW.

Br,
--
Carlos Chinea <carlos.chinea@xxxxxxxxx>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Jean Delvare: "Re: [lm-sensors] [PATCH] hwmon: Add driver for intel PCI thermal subsystem"
Previous message: Mikael Starvik: "RE: [PATCH] cris:Check kmalloc return value before use data"
Next in thread: Sebastien Jan: "Re: [RFC PATCHv2 2/7] OMAP SSI: Introducing OMAP SSI driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]