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

[Sebastien Jan]

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()?)

--
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/