Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756919Ab1E3OlX (ORCPT ); Mon, 30 May 2011 10:41:23 -0400 Received: from smtp.nokia.com ([147.243.128.26]:31363 "EHLO mgw-da02.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751512Ab1E3OlV (ORCPT ); Mon, 30 May 2011 10:41:21 -0400 Subject: Re: [PATCH 1/2] HSI: add ST-E HSI controller From: Carlos Chinea To: ext Linus Walleij Cc: linux-kernel@vger.kernel.org, Lee Jones , Pawel Szyszuk , Linus Walleij In-Reply-To: <1306340850-31364-1-git-send-email-linus.walleij@stericsson.com> References: <1306340850-31364-1-git-send-email-linus.walleij@stericsson.com> Content-Type: text/plain; charset="UTF-8" Date: Mon, 30 May 2011 17:37:41 +0300 Message-ID: <1306766262.2819.10721.camel@groo> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Content-Transfer-Encoding: 7bit X-Nokia-AV: Clean Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 50918 Lines: 1722 Hi, My 2 cents for this patch bellow: On Wed, 2011-05-25 at 18:27 +0200, ext Linus Walleij wrote: > From: Pawel Szyszuk > > Signed-off-by: Pawel Szyszuk > Signed-off-by: Linus Walleij > --- > drivers/hsi/controllers/Kconfig | 10 + > drivers/hsi/controllers/Makefile | 1 + > drivers/hsi/controllers/ste_hsi.c | 1536 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 1547 insertions(+), 0 deletions(-) > create mode 100644 drivers/hsi/controllers/ste_hsi.c > > diff --git a/drivers/hsi/controllers/Kconfig b/drivers/hsi/controllers/Kconfig > index 3efe0f0..76d339e 100644 > --- a/drivers/hsi/controllers/Kconfig > +++ b/drivers/hsi/controllers/Kconfig > @@ -3,6 +3,16 @@ > # > comment "HSI controllers" > > +config STE_HSI > + tristate "STE HSI controller driver" > + depends on (ARCH_U8500 || ARCH_NOMADIK) && HSI > + default n > + help > + ST-Ericsson HSI controller. > + If you say Y here, you will enable the U8500 HSI hardware driver. > + > + If unsure, say N. > + > config OMAP_SSI > tristate "OMAP SSI hardware driver" > depends on ARCH_OMAP && HSI > diff --git a/drivers/hsi/controllers/Makefile b/drivers/hsi/controllers/Makefile > index c4ba2c2..475637a 100644 > --- a/drivers/hsi/controllers/Makefile > +++ b/drivers/hsi/controllers/Makefile > @@ -2,4 +2,5 @@ > # Makefile for HSI controllers drivers > # > > +obj-$(CONFIG_STE_HSI) += ste_hsi.o > obj-$(CONFIG_OMAP_SSI) += omap_ssi.o > diff --git a/drivers/hsi/controllers/ste_hsi.c b/drivers/hsi/controllers/ste_hsi.c > new file mode 100644 > index 0000000..1a16234 > --- /dev/null > +++ b/drivers/hsi/controllers/ste_hsi.c > @@ -0,0 +1,1536 @@ > +/* > + * Copyright (C) ST-Ericsson SA 2010 > + * > + * License Terms: GNU General Public License v2 > + * Author: Marcin Mielczarczyk for ST-Ericsson > + * Author: Lukasz Baj for ST-Ericsson > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#ifdef CONFIG_STE_DMA40 > +#include > +#include > +#endif > + > +#include > + > +/** > + * struct ste_hsi_controller - Nomadik HSI controller data > + * @dev: device associated to the controller (HSI controller) > + * @rx_base: HSI receiver registers base address > + * @tx_base: HSI transmitter registers base address > + */ You may want to add few more fields ;) > +struct ste_hsi_controller { > + struct device *dev; > + struct clk *tx_clk; > + struct clk *rx_clk; > + struct clk *ssitx_clk; > + struct clk *ssirx_clk; > + struct delayed_work clk_work; > + unsigned char __iomem *rx_base; > + unsigned char __iomem *tx_base; Hmmm why the above two lines are not void __iomem * ? That should also remove some typecasting later on. > + int overrun_irq[STE_HSI_MAX_CHANNELS]; > + int ck_refcount; > + spinlock_t ck_lock; > + spinlock_t lock; > + unsigned int use_dma:1; > + unsigned int ck_on:1; > + /* physical address of rx and tx controller */ > + dma_addr_t rx_dma_base; > + dma_addr_t tx_dma_base; > +}; > + > +#ifdef CONFIG_STE_DMA40 > +struct ste_hsi_channel_dma { > + struct dma_chan *dma_chan; > + struct dma_async_tx_descriptor *desc; > + dma_cookie_t cookie; > +}; > +#endif > + > +struct ste_hsi_port { > + struct device *dev; > + struct list_head txqueue[STE_HSI_MAX_CHANNELS]; > + struct list_head rxqueue[STE_HSI_MAX_CHANNELS]; > + struct list_head brkqueue; > + int tx_irq; > + int rx_irq; > + int excep_irq; > + struct tasklet_struct rx_tasklet; > + struct tasklet_struct tx_tasklet; > + struct tasklet_struct exception_tasklet; > + struct tasklet_struct overrun_tasklet; > + unsigned char channels; > +#ifdef CONFIG_STE_DMA40 > + struct ste_hsi_channel_dma tx_dma[STE_HSI_MAX_CHANNELS]; > + struct ste_hsi_channel_dma rx_dma[STE_HSI_MAX_CHANNELS]; > +#endif > +}; > + Adding some documentation for the above structs would be nice. > +#define hsi_to_ste_port(port) (hsi_port_drvdata(port)) > +#define hsi_to_ste_controller(con) (hsi_controller_drvdata(con)) > +#define client_to_ste_port(cl) (hsi_port_drvdata(hsi_get_port(cl))) > +#define client_to_hsi(cl) \ > + (to_hsi_controller(hsi_get_port(cl)->device.parent)) > +#define client_to_ste_controller(cl) \ > + (hsi_controller_drvdata(client_to_hsi(cl))) > +#define ste_port_to_ste_controller(port) \ > + ((struct ste_hsi_controller *)hsi_controller_drvdata( \ > + to_hsi_controller(port->dev->parent))) > + > +static u32 ste_hsir_periphid[8] = { 0x2C, 0, 0x8, 0x18, 0xD, 0xF0, 0x5, 0xB1 }; > +static u32 ste_hsit_periphid[8] = { 0x2B, 0, 0x8, 0x18, 0xD, 0xF0, 0x5, 0xB1 }; Why do these two arrays need to be global ? Moreover, I think they are potentially platform_data material. > + > +/* > + * linux/amba/bus.h macros can not be used, because 8 bytes are validated: > + * PERIPHID0..3 and PCELLID0..3 for HSIR and HSIT. > + */ > +static inline int compare_periphid(u32 *id1, u32 *id2, int count) > +{ > + while (count && *id1++ == *id2++) > + count--; > + > + return count; > +} Seeing later what you are using this function for, I will recommend something like: static inline int compare_periphid(u32 *id1, void __iomem *id2, unsigned int count) { while (count && *id1++ == readl(id2)) { count--; id2 += sizeof(*id1); } return count; } > + > +static void ste_hsi_clk_free(struct clk **pclk) > +{ > + if (IS_ERR(*pclk) && *pclk != NULL) I guess this condition should be: (!IS_ERR(*pclk) && *pclk != NULL) > + clk_put(*pclk); > + *pclk = NULL; > +} > + > +static void ste_hsi_clks_free(struct ste_hsi_controller *ste_hsi) > +{ > + ste_hsi_clk_free(&ste_hsi->rx_clk); > + ste_hsi_clk_free(&ste_hsi->tx_clk); > + ste_hsi_clk_free(&ste_hsi->ssirx_clk); > + ste_hsi_clk_free(&ste_hsi->ssitx_clk); > +} > + > +static int ste_hsi_clock_enable(struct hsi_controller *hsi) > +{ > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + int err = 0; > + > + spin_lock_bh(&ste_hsi->ck_lock); > + if (ste_hsi->ck_refcount++ || ste_hsi->ck_on) If you have refcounting for the clocks, why do yo need the ck_on flag ? > + goto out; > + > + err = clk_enable(ste_hsi->ssirx_clk); > + if (unlikely(err)) > + goto out; > + > + err = clk_enable(ste_hsi->ssitx_clk); > + if (unlikely(err)) > + clk_disable(ste_hsi->ssirx_clk); > + > + err = clk_enable(ste_hsi->rx_clk); > + if (unlikely(err)) { > + clk_disable(ste_hsi->ssitx_clk); > + clk_disable(ste_hsi->ssirx_clk); > + } > + > + err = clk_enable(ste_hsi->tx_clk); > + if (unlikely(err)) { > + clk_disable(ste_hsi->rx_clk); > + clk_disable(ste_hsi->ssitx_clk); > + clk_disable(ste_hsi->ssirx_clk); > + Maybe better to continue using goto statements here and in the above conditions and have the whole cleanup at the end of the function. > } > + > + ste_hsi->ck_on = 1; > +out: > + if (err) > + ste_hsi->ck_refcount--; > + > + spin_unlock_bh(&ste_hsi->ck_lock); > + > + return err; > +} > + > +static void ste_hsi_delayed_disable_clock(struct work_struct *work) > +{ > + struct ste_hsi_controller *ste_hsi; > + ste_hsi = container_of(work, struct ste_hsi_controller, clk_work.work); > + > + spin_lock_bh(&ste_hsi->ck_lock); > + > + /* > + * If clock should not be off (enable clock called in meantime) > + * or clock is already off nothing to do > + */ > + if (ste_hsi->ck_refcount || !ste_hsi->ck_on) Ohh I see why you want ck_on now, but yet again, why don't you increase the refcounting when you delay the disabling of the clocks... ... then this function will be pretty much a one liner calling sti_hsi_clock_disable() > + goto out; > + > + if (readl(ste_hsi->tx_base + STE_HSI_TX_STATE) != STE_HSI_STATE_IDLE || > + readl(ste_hsi->rx_base + STE_HSI_RX_STATE) > + != STE_HSI_STATE_IDLE || > + readl(ste_hsi->rx_base + STE_HSI_RX_BUFSTATE) != 0) { > + /* Try again later */ > + int err = schedule_delayed_work(&ste_hsi->clk_work, HZ); > + if (err < 0) > + dev_err(ste_hsi->dev, "Error scheduling work\n"); > + goto out; > + } > + > + /* Actual clocks disable */ > + clk_disable(ste_hsi->tx_clk); > + clk_disable(ste_hsi->rx_clk); > + clk_disable(ste_hsi->ssitx_clk); > + clk_disable(ste_hsi->ssirx_clk); > + ste_hsi->ck_on = 0; > + > +out: > + spin_unlock_bh(&ste_hsi->ck_lock); > +} > + > +static void ste_hsi_clock_disable(struct hsi_controller *hsi) > +{ > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + > + spin_lock_bh(&ste_hsi->ck_lock); > + > + /* Sanity check */ > + if (ste_hsi->ck_refcount <= 0) > + WARN_ON(ste_hsi->ck_refcount <= 0); > + The if statement is redundant. I will suggest also WARN_ON_ONCE(). Otherwise once this is trigger you can easily have an storm of them. > + /* Need clock to be disable now? */ > + if (--ste_hsi->ck_refcount) > + goto out; > + > + /* > + * If receiver or transmitter is in the middle something delay clock off > + */ > + if (readl(ste_hsi->tx_base + STE_HSI_TX_STATE) != STE_HSI_STATE_IDLE || > + readl(ste_hsi->rx_base + STE_HSI_RX_STATE) > + != STE_HSI_STATE_IDLE || > + readl(ste_hsi->rx_base + STE_HSI_RX_BUFSTATE) != 0) { > + int err = schedule_delayed_work(&ste_hsi->clk_work, HZ); > + if (err < 0) > + dev_err(&hsi->device, "Error scheduling work\n"); > + > + goto out; > + } > + > + /* Actual clocks disabled */ > + clk_disable(ste_hsi->tx_clk); > + clk_disable(ste_hsi->rx_clk); > + clk_disable(ste_hsi->ssitx_clk); > + clk_disable(ste_hsi->ssirx_clk); > + ste_hsi->ck_on = 0; > + > +out: > + spin_unlock_bh(&ste_hsi->ck_lock); > +} > + > +static int ste_hsi_start_irq(struct hsi_msg *msg) > +{ > + struct hsi_port *port = hsi_get_port(msg->cl); > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + u32 val; > + int err; > + > + err = ste_hsi_clock_enable(hsi); > + if (unlikely(err)) > + return err; > + > + msg->actual_len = 0; > + msg->status = HSI_STATUS_PROCEEDING; > + > + if (msg->ttype == HSI_MSG_WRITE) { > + val = readl(ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM) | > + (1 << msg->channel); > + writel(val, ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM); > + } else { > + val = readl(ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM) | > + (1 << msg->channel); > + writel(val, ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM); > + > + val = readl(ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM) | > + (1 << msg->channel); > + writel(val, ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM); > + } > + > + return 0; > +} > + > +static int ste_hsi_start_transfer(struct ste_hsi_port *ste_port, > + struct list_head *queue); > +#ifdef CONFIG_STE_DMA40 > +static void ste_hsi_dma_callback(void *dma_async_param) > +{ > + struct hsi_msg *msg = dma_async_param; > + struct hsi_controller *hsi = client_to_hsi(msg->cl); > + struct ste_hsi_port *ste_port = client_to_ste_port(msg->cl); > + struct ste_hsi_controller *ste_hsi = client_to_ste_controller(msg->cl); > + struct list_head *queue; > + struct dma_chan *chan; > + struct ste_hsi_channel_dma *hsi_dma_chan; > + char *dma_enable_address; > + enum dma_data_direction direction; > + u32 dma_mask; > + > + /* Message finished, remove from list and notify client */ > + spin_lock_bh(&ste_hsi->lock); > + list_del(&msg->link); > + > + if (msg->ttype == HSI_MSG_WRITE) { > + queue = &ste_port->txqueue[msg->channel]; > + direction = DMA_TO_DEVICE; > + dma_enable_address = ste_hsi->tx_base + STE_HSI_TX_DMAEN; > + hsi_dma_chan = &ste_port->tx_dma[msg->channel]; > + } else { > + queue = &ste_port->rxqueue[msg->channel]; > + direction = DMA_FROM_DEVICE; > + dma_enable_address = ste_hsi->rx_base + STE_HSI_RX_DMAEN; > + hsi_dma_chan = &ste_port->rx_dma[msg->channel]; > + } > + > + dma_sync_sg_for_cpu(&hsi->device, msg->sgt.sgl, > + msg->sgt.nents, direction); > + chan = hsi_dma_chan->dma_chan; > + > + /* disable DMA channel on HSI controller */ > + dma_mask = readl(dma_enable_address); > + writel(dma_mask & ~(1 << msg->channel), dma_enable_address); > + > + hsi_dma_chan->desc = NULL; > + > + dma_unmap_sg(&hsi->device, msg->sgt.sgl, msg->sgt.nents, direction); > + > + msg->status = HSI_STATUS_COMPLETED; > + msg->actual_len = sg_dma_len(msg->sgt.sgl); > + > + spin_unlock_bh(&ste_hsi->lock); > + > + msg->complete(msg); > + > + ste_hsi_clock_disable(hsi); > + > + spin_lock_bh(&ste_hsi->lock); > + ste_hsi_start_transfer(ste_port, queue); > + spin_unlock_bh(&ste_hsi->lock); > +} > + > +static void dma_device_control(struct ste_hsi_channel_dma *chan, > + enum dma_ctrl_cmd cmd, unsigned long arg) > +{ > + chan->dma_chan->device->device_control(chan->dma_chan, cmd, arg); > +} > + > +static void ste_hsi_terminate_dma_chan(struct ste_hsi_channel_dma *chan) > +{ > + if (chan->desc) { > + dma_device_control(chan, DMA_TERMINATE_ALL, 0); > + chan->desc = NULL; > + } > + chan->cookie = 0; > +} > + > +static void ste_hsi_terminate_dma(struct ste_hsi_port *ste_port) > +{ > + int i; > + > + for (i = 0; i < ste_port->channels; ++i) { > + ste_hsi_terminate_dma_chan(&ste_port->tx_dma[i]); > + ste_hsi_terminate_dma_chan(&ste_port->rx_dma[i]); > + } > +} > + > +static int ste_hsi_start_dma(struct hsi_msg *msg) > +{ > + struct hsi_controller *hsi = client_to_hsi(msg->cl); > + struct ste_hsi_port *ste_port = client_to_ste_port(msg->cl); > + struct ste_hsi_controller *ste_hsi = client_to_ste_controller(msg->cl); > + struct dma_async_tx_descriptor *desc; > + struct dma_chan *chan; > + struct ste_hsi_channel_dma *hsi_dma_chan; > + char *dma_enable_address; > + enum dma_data_direction direction; > + u32 dma_mask; > + int err; > + > + err = ste_hsi_clock_enable(hsi); > + if (unlikely(err)) > + return err; > + > + if (msg->ttype == HSI_MSG_WRITE) { > + direction = DMA_TO_DEVICE; > + dma_enable_address = ste_hsi->tx_base + STE_HSI_TX_DMAEN; > + hsi_dma_chan = &ste_port->tx_dma[msg->channel]; > + } else { > + u32 val; > + direction = DMA_FROM_DEVICE; > + dma_enable_address = ste_hsi->rx_base + STE_HSI_RX_DMAEN; > + hsi_dma_chan = &ste_port->rx_dma[msg->channel]; > + > + /* enable overrun for this channel */ > + val = readl(ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM) | > + (1 << msg->channel); > + writel(val, ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM); > + } > + > + chan = hsi_dma_chan->dma_chan; > + > + if (0 == dma_map_sg(&hsi->device, msg->sgt.sgl, msg->sgt.nents, > + direction)) { > + dev_dbg(&hsi->device, "DMA map SG failed !\n"); > + err = -ENOMEM; > + goto out; > + } > + /* Prepare the scatterlist */ > + desc = chan->device->device_prep_slave_sg(chan, > + msg->sgt.sgl, > + msg->sgt.nents, > + direction, > + DMA_PREP_INTERRUPT | > + DMA_CTRL_ACK); > + > + if (!desc) { > + dma_unmap_sg(&hsi->device, msg->sgt.sgl, msg->sgt.nents, > + direction); > + /* "Complete" DMA (errorpath) */ > + ste_hsi_terminate_dma_chan(hsi_dma_chan); > + err = -EBUSY; > + goto out; > + } > + desc->callback = ste_hsi_dma_callback; > + desc->callback_param = msg; > + hsi_dma_chan->cookie = desc->tx_submit(desc); > + hsi_dma_chan->desc = desc; > + > + /* Fire the DMA transaction */ > + chan->device->device_issue_pending(chan); > + > + /* Enable DMA channel on HSI controller */ > + dma_mask = readl(dma_enable_address); > + writel(dma_mask | 1 << msg->channel, dma_enable_address); > + > +out: > + if (unlikely(err)) > + ste_hsi_clock_disable(hsi); > + > + return err; > +} > + > +static void __init ste_hsi_init_dma(struct ste_hsi_platform_data *data, > + struct hsi_controller *hsi) > +{ > + struct hsi_port *port; > + struct ste_hsi_port *ste_port; > + struct ste_hsi_controller *ste_hsi = hsi_to_ste_controller(hsi); > + dma_cap_mask_t mask; > + int i, ch; > + > + ste_hsi->use_dma = 1; > + /* Try to acquire a generic DMA engine slave channel */ > + dma_cap_zero(mask); > + dma_cap_set(DMA_SLAVE, mask); > + > + for (i = 0; i < hsi->num_ports; ++i) { > + port = &hsi->port[i]; > + ste_port = hsi_port_drvdata(port); > + > + for (ch = 0; ch < STE_HSI_MAX_CHANNELS; ++ch) { > + ste_port->tx_dma[ch].dma_chan = > + dma_request_channel(mask, > + data->port_cfg[i].dma_filter, > + &data->port_cfg[i]. > + dma_tx_cfg[ch]); > + > + ste_port->rx_dma[ch].dma_chan = > + dma_request_channel(mask, > + data->port_cfg[i].dma_filter, > + &data->port_cfg[i]. > + dma_rx_cfg[ch]); > + } > + } > +} > + > +static int ste_hsi_setup_dma(struct hsi_client *cl) > +{ > + int i; > + struct hsi_port *port = to_hsi_port(cl->device.parent); > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + struct dma_slave_config rx_conf = { > + .src_addr = 0, /* dynamic data */ > + .src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES, > + .direction = DMA_FROM_DEVICE, > + .src_maxburst = 1, > + }; > + struct dma_slave_config tx_conf = { > + .dst_addr = 0, /* dynamic data */ > + .dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES, > + .direction = DMA_TO_DEVICE, > + .dst_maxburst = 1, > + }; > + > + if (!ste_hsi->use_dma) > + return 0; > + > + for (i = 0; i < ste_port->channels; ++i) { > + struct dma_chan *chan; > + > + chan = ste_port->tx_dma[i].dma_chan; > + tx_conf.dst_addr = (dma_addr_t) ste_hsi->tx_dma_base + > + STE_HSI_TX_BUFFERX + 4 * i; > + chan->device->device_control(chan, > + DMA_SLAVE_CONFIG, > + (unsigned long)&tx_conf); > + > + chan = ste_port->rx_dma[i].dma_chan; > + rx_conf.src_addr = (dma_addr_t) ste_hsi->rx_dma_base + > + STE_HSI_RX_BUFFERX + 4 * i; > + chan->device->device_control(chan, > + DMA_SLAVE_CONFIG, > + (unsigned long)&rx_conf); > + } > + > + return 0; > +} > + > +#else > +#define ste_hsi_init_dma(data, hsi) do { } while (0) > +#define ste_hsi_start_dma ste_hsi_start_irq > +#define ste_hsi_terminate_dma(ste_port) do { } while (0) > +#define ste_hsi_setup_dma(cl) do { } while (0) > +#endif > + > +static int ste_hsi_start_transfer(struct ste_hsi_port *ste_port, > + struct list_head *queue) > +{ > + struct hsi_msg *msg; > + int err; > + > + if (list_empty(queue)) > + return 0; > + > + msg = list_first_entry(queue, struct hsi_msg, link); > + if (msg->status != HSI_STATUS_QUEUED) > + return 0; > + > + msg->actual_len = 0; > + msg->status = HSI_STATUS_PROCEEDING; > + > + if (ste_port_to_ste_controller(ste_port)->use_dma) > + err = ste_hsi_start_dma(msg); > + els > + err = ste_hsi_start_irq(msg); > + > + return err; > +} > + > +static void ste_hsi_receive_data(struct hsi_port *port, unsigned int channel) > +{ > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + struct list_head *queue = &ste_port->rxqueue[channel]; > + struct hsi_msg *msg; > + char *bufferx; > + u8 *buf; > + int span; > + > + spin_lock_bh(&ste_hsi->lock); spin_lock() is enough here, this is always called inside a tasklet, right ? > + > + if (list_empty(queue)) > + goto out; > + > + 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_PROCEEDING && msg->ttype == HSI_MSG_READ) { > + unsigned char len; > + bufferx = ste_hsi->rx_base + STE_HSI_RX_BUFFERX + 4 * channel; > + > + len = readl(ste_hsi->rx_base + STE_HSI_RX_GAUGEX + 4 * channel); > + buf = sg_virt(msg->sgt.sgl); > + buf += msg->actual_len; > + while (len--) { > + *(u32 *) buf = readl(bufferx); > + buf += 4; > + msg->actual_len += 4; > + if (msg->actual_len >= msg->sgt.sgl->length) { > + msg->status = HSI_STATUS_COMPLETED; > + break; > + } > + } > + } > + > + /* re-enable interrupt by watermark manipulation */ > + span = readl(ste_hsi->rx_base + STE_HSI_RX_SPANX + 4 * channel); > + writel(span, ste_hsi->rx_base + STE_HSI_RX_WATERMARKX + 4 * channel); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_WATERMARKX + 4 * channel); > + > + /* > + * If message was not transmitted completely enable interrupt for > + * further work > + */ > + if (msg->status == HSI_STATUS_PROCEEDING) { > + u32 val; > + val = readl(ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM) | > + (1 << channel); > + writel(val, ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM); > + goto out; > + } > + > + /* Message finished, remove from list and notify client */ > + list_del(&msg->link); > + spin_unlock_bh(&ste_hsi->lock); ditto > + msg->complete(msg); > + > + ste_hsi_clock_disable(hsi); > + > + spin_lock_bh(&ste_hsi->lock); > + ditto > + ste_hsi_start_transfer(ste_port, queue); > +out: > + spin_unlock_bh(&ste_hsi->lock); > +} > + > +static void ste_hsi_transmit_data(struct hsi_port *port, unsigned int channel) > +{ > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + struct list_head *queue = &ste_port->txqueue[channel]; > + struct hsi_msg *msg; > + u8 *buf; > + int span; > + > + if (list_empty(queue)) > + return; > + > + spin_lock_bh(&ste_hsi->lock); ditto > + 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_PROCEEDING && > + msg->ttype == HSI_MSG_WRITE) { > + unsigned char free_space; > + > + free_space = readl(ste_hsi->tx_base + > + STE_HSI_TX_GAUGEX + 4 * channel); > + buf = sg_virt(msg->sgt.sgl); > + buf += msg->actual_len; > + while (free_space--) { > + writel(*(u32 *) buf, ste_hsi->tx_base + > + STE_HSI_TX_BUFFERX + 4 * channel); > + buf += 4; > + msg->actual_len += 4; > + if (msg->actual_len >= msg->sgt.sgl->length) { > + msg->status = HSI_STATUS_COMPLETED; > + break; > + } > + } > + } > + > + span = readl(ste_hsi->tx_base + STE_HSI_TX_SPANX + 4 * channel); > + writel(span, ste_hsi->tx_base + STE_HSI_TX_WATERMARKX + 4 * channel); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_WATERMARKX + 4 * channel); > + > + if (msg->status == HSI_STATUS_PROCEEDING) { > + u32 val; > + val = readl(ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM) | > + (1 << channel); > + writel(val, ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM); > + goto out; > + } > + > + list_del(&msg->link); > + spin_unlock_bh(&ste_hsi->lock); > + ditto > msg->complete(msg); > + > + ste_hsi_clock_disable(hsi); > + > + spin_lock_bh(&ste_hsi->lock); > + ditto > ste_hsi_start_transfer(ste_port, queue); > +out: > + spin_unlock_bh(&ste_hsi->lock); ditto > +} > + > +static void ste_hsi_rx_tasklet(unsigned long data) > +{ > + struct hsi_port *port = (struct hsi_port *)data; > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + u32 irq_status, irq_mask; > + unsigned int i; > + > + irq_status = readl(ste_hsi->rx_base + STE_HSI_RX_WATERMARKIS); > + if (!irq_status) > + goto out; > + > + irq_mask = readl(ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM); > + writel(irq_mask & ~irq_status, > + ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM); > + writel(irq_mask, ste_hsi->rx_base + STE_HSI_RX_WATERMARKIC); > + > + for (i = 0; i < STE_HSI_MAX_CHANNELS; ++i) { > + if (irq_status & (1 << i)) > + ste_hsi_receive_data(port, i); > + } > +out: > + enable_irq(ste_port->rx_irq); > +} > + > +static irqreturn_t ste_hsi_rx_isr(int irq, void *data) > +{ > + struct hsi_port *port = data; > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + > + disable_irq_nosync(irq); > + tasklet_hi_schedule(&ste_port->rx_tasklet); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t ste_hsi_tx_isr(int irq, void *data) > +{ > + struct hsi_port *port = data; > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + > + disable_irq_nosync(irq); > + tasklet_hi_schedule(&ste_port->tx_tasklet); > + > + return IRQ_HANDLED; > +} > + > +static void ste_hsi_tx_tasklet(unsigned long data) > +{ > + struct hsi_port *port = (struct hsi_port *)data; > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + u32 irq_status, irq_mask; > + unsigned int i; > + > + irq_status = readl(ste_hsi->tx_base + STE_HSI_TX_WATERMARKIS); > + if (!irq_status) > + goto out; > + > + irq_mask = readl(ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM); > + writel(irq_mask & ~irq_status, > + ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM); > + writel(irq_mask, ste_hsi->tx_base + STE_HSI_TX_WATERMARKIC); > + > + for (i = 0; i < STE_HSI_MAX_CHANNELS; ++i) { > + if (irq_status & (1 << i)) > + ste_hsi_transmit_data(port, i); > + } > +out: > + enable_irq(ste_port->tx_irq); > +} > + > +static void ste_hsi_break_complete(struct hsi_port *port, > + struct ste_hsi_controller *ste_hsi) > +{ > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + struct hsi_msg *msg, *tmp; > + u32 mask; > + > + dev_dbg(port->device.parent, "HWBREAK received\n"); > + > + spin_lock_bh(&ste_hsi->lock); ditto > + > + mask = readl(ste_hsi->rx_base + STE_HSI_RX_EXCEPIM); > + writel(mask & ~STE_HSI_EXCEP_BREAK, > + ste_hsi->rx_base + STE_HSI_RX_EXCEPIM); > + > + spin_unlock_bh(&ste_hsi->lock); ditto > + > + list_for_each_entry_safe(msg, tmp, &ste_port->brkqueue, link) { > + msg->status = HSI_STATUS_COMPLETED; > + list_del(&msg->link); > + msg->complete(msg); > + } This can create infinite loop if two clients rearm the HWBREAK in their completion callback, plus list_del() should be protected with lock. The same infinite loop issue is in the OMAP driver. I will suggest something like: LIST_HEAD(bq); .... list_splice_init(&ste_port->brkqueue, bq); spin_unlock(&ste_hsi->lock); list_for_each_entry_safe(msg, tmp, &bq, link) { ... > +} > + > +static void ste_hsi_error(struct hsi_port *port) > + > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + struct hsi_msg *msg; > + unsigned int i; > + > + for (i = 0; i < ste_port->channels; i++) { > + if (list_empty(&ste_port->rxqueue[i])) > + continue; > + msg = list_first_entry(&ste_port->rxqueue[i], struct hsi_msg, > + link); > + list_del(&msg->link); > + msg->status = HSI_STATUS_ERROR; > + msg->complete(msg); > + /* Now restart queued reads if any */ > + ste_hsi_start_transfer(ste_port, &ste_port->rxqueue[i]); > + } > +} > + > +static void ste_hsi_exception_tasklet(unsigned long data) > +{ > + struct hsi_port *port = (struct hsi_port *)data; > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + u32 error_status; > + u32 error_interrupts; > + > + error_status = readl(ste_hsi->rx_base + STE_HSI_RX_EXCEP); > + /* > + * sometimes interrupt that cause running this tasklet is already > + * inactive so base handling of exception on masked interrupt status > + * not on exception state register. > + */ > + error_interrupts = readl(ste_hsi->rx_base + STE_HSI_RX_EXCEPMIS); > + > + if (error_interrupts & STE_HSI_EXCEP_BREAK) > + ste_hsi_break_complete(port, ste_hsi); > + > + if (error_interrupts & STE_HSI_EXCEP_TIMEOUT) > + dev_err(&hsi->device, "timeout exception occurred\n"); > + if (error_interrupts & STE_HSI_EXCEP_OVERRUN) > + dev_err(&hsi->device, "overrun exception occurred\n"); > + if (error_interrupts & STE_HSI_EXCEP_PARITY) > + dev_err(&hsi->device, "parity exception occurred\n"); > + > + if (error_interrupts & ~STE_HSI_EXCEP_BREAK) > + ste_hsi_error(port); Shouldn't the spin_lock lock be claimed before calling ste_hsi_error() and then released after it. Or then put the proper locking for accessing the queues in ste_hsi_error() ? > + > + /* Acknowledge exception interrupts */ > + writel(error_status, ste_hsi->rx_base + STE_HSI_RX_ACK); > + > + enable_irq(ste_port->excep_irq); > +} > + > +static irqreturn_t ste_hsi_exception_isr(int irq, void *data) > +{ > + struct hsi_port *port = data; > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + > + disable_irq_nosync(irq); > + tasklet_hi_schedule(&ste_port->exception_tasklet); > + > + return IRQ_HANDLED; > +} > + > +static void ste_hsi_overrun_tasklet(unsigned long data) > +{ > + struct hsi_controller *hsi = (struct hsi_controller *)data; > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + struct hsi_port *hsi_port = &hsi->port[0]; > + struct ste_hsi_port *ste_port = hsi_port_drvdata(hsi_port); > + struct hsi_msg *msg; > + > + unsigned int channel; > + u8 rised_overrun; > + u8 mask; > + u8 blocked = 0; > + > + rised_overrun = (u8) readl(ste_hsi->rx_base + STE_HSI_RX_OVERRUNMIS); I guess we could use readb() here and forget about the casting. Or there is something arch dependent that prevents this ? > + mask = rised_overrun; > + for (channel = 0; mask; ++channel, mask >>= 1) { > + if (!(mask & 1)) > + continue; > + > + do { > + /* > + * No more messages, block interrupt > + */ > + if (list_empty(&ste_port->rxqueue[channel])) { > + blocked |= 1 << channel; > + break; > + } > + /* > + * Complete with error > + */ > + msg = list_first_entry(&ste_port->rxqueue[channel], > + struct hsi_msg, link); > + list_del(&msg->link); Access to the lists should be protected with lock. > + msg->status = HSI_STATUS_ERROR; > + msg->complete(msg); > + > + /* > + * Now restart queued reads if any > + * If start_transfer fails, try with next message > + */ > + if (ste_hsi_start_transfer(ste_port, > + &ste_port->rxqueue[channel])) > + continue; > + } while (0); do {} while(0) Why ? > + } > + > + /* Overrun acknowledge */ > + writel(rised_overrun, ste_hsi->rx_base + STE_HSI_RX_OVERRUNACK); > + writel(~blocked & readl(ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM), > + ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM); > + > + /* > + * Enable all that should not be blocked > + */ > + mask = rised_overrun & ~blocked; > + for (channel = 0; mask; ++channel, mask >>= 1) > + enable_irq(ste_hsi->overrun_irq[channel]); > +} > + > +static irqreturn_t ste_hsi_overrun_isr(int irq, void *data) > +{ > + struct hsi_port *port = data; > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + > + disable_irq_nosync(irq); > + tasklet_hi_schedule(&ste_port->overrun_tasklet); > + > + return IRQ_HANDLED; > +} > + > +static void __init ste_hsi_queues_init(struct ste_hsi_port *ste_port) > +{ > + unsigned int ch; > + > + for (ch = 0; ch < STE_HSI_MAX_CHANNELS; ch++) { > + INIT_LIST_HEAD(&ste_port->txqueue[ch]); > + INIT_LIST_HEAD(&ste_port->rxqueue[ch]); > + } > + INIT_LIST_HEAD(&ste_port->brkqueue); > +} > + > +static int __init ste_hsi_get_iomem(struct platform_device *pdev, > + const char *res_name, > + unsigned char __iomem **base, > + dma_addr_t *phy) > +{ > + struct resource *mem; > + struct resource *ioarea; > + > + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, res_name); > + if (!mem) { > + dev_err(&pdev->dev, "IO memory region missing!\n"); > + return -ENXIO; > + } > + > + ioarea = devm_request_mem_region(&pdev->dev, mem->start, > + resource_size(mem), > + dev_name(&pdev->dev)); > + if (!ioarea) { > + dev_err(&pdev->dev, "Can't request IO memory region!\n"); > + return -ENXIO; > + } > + > + *base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem)); > + if (!base) { > + dev_err(&pdev->dev, "%s IO remap failed!\n", mem->name); > + return -ENXIO; > + } > + if (phy) > + *phy = (dma_addr_t) mem->start; > + > + return 0; > +} > + > +static int __init ste_hsi_get_irq(struct platform_device *pdev, > + const char *res_name, > + irqreturn_t(*isr) (int, void *), void *data, > + int *irq_number) > +{ > + struct resource *irq; > + int err; > + > + irq = platform_get_resource_byname(pdev, IORESOURCE_IRQ, res_name); > + if (!irq) { > + dev_err(&pdev->dev, "IO memory region missing!\n"); > + return -ENXIO; > + } > + > + err = devm_request_irq(&pdev->dev, irq->start, isr, > + IRQF_DISABLED, irq->name, data); > + if (err) > + dev_err(&pdev->dev, "%s IRQ request failed!\n", irq->name); > + > + if (irq_number) > + *irq_number = irq->start; > + > + return err; > +} > + > +static void ste_hsi_flush_queue(struct list_head *queue, struct hsi_client *cl) > +{ > + struct list_head *node, *tmp; > + struct hsi_msg *msg; > + > + list_for_each_safe(node, tmp, queue) { > + msg = list_entry(node, struct hsi_msg, link); > + if ((cl) && (cl != msg->cl)) > + continue; > + list_del(node); > + > + if (msg->destructor) > + msg->destructor(msg); > + else > + hsi_free_msg(msg); > + } > +} > + > +static int ste_hsi_async_break(struct hsi_msg *msg) > +{ > + struct hsi_port *port = hsi_get_port(msg->cl); > + struct ste_hsi_port *ste_port = hsi_to_ste_port(port); > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + int err; > + > + err = ste_hsi_clock_enable(hsi); > + if (unlikely(err)) > + return err; > + > + if (msg->ttype == HSI_MSG_WRITE) { > + if (port->tx_cfg.mode != HSI_MODE_FRAME) { > + err = -EINVAL; > + goto out; > + } > + writel(1, ste_hsi->tx_base + STE_HSI_TX_BREAK); > + msg->status = HSI_STATUS_COMPLETED; > + msg->complete(msg); > + } else { > + u32 mask; > + if (port->rx_cfg.mode != HSI_MODE_FRAME) { > + err = -EINVAL; > + goto out; > + } > + spin_lock_bh(&ste_hsi->lock); > + msg->status = HSI_STATUS_PROCEEDING; > + mask = readl(ste_hsi->rx_base + STE_HSI_RX_EXCEPIM); > + /* Enable break exception on controller */ > + if (!(mask & STE_HSI_EXCEP_BREAK)) > + writel(mask | STE_HSI_EXCEP_BREAK, > + ste_hsi->rx_base + STE_HSI_RX_EXCEPIM); > + > + list_add_tail(&msg->link, &ste_port->brkqueue); > + spin_unlock_bh(&ste_hsi->lock); > + } > + > +out: > + ste_hsi_clock_disable(hsi); > + return err; > +} > + > +static int ste_hsi_async(struct hsi_msg *msg) > +{ > + struct ste_hsi_controller *ste_hsi; > + struct ste_hsi_port *ste_port; > + struct list_head *queue; > + int err = 0; > + > + if (unlikely(!msg)) > + return -ENOSYS; > + > + if (msg->sgt.nents > 1) > + return -ENOSYS; > + > + if (unlikely(msg->break_frame)) > + return ste_hsi_async_break(msg); > + > + ste_port = client_to_ste_port(msg->cl); > + ste_hsi = client_to_ste_controller(msg->cl); > + > + if (msg->ttype == HSI_MSG_WRITE) { > + /* TX transfer */ > + BUG_ON(msg->channel >= ste_port->channels); > + queue = &ste_port->txqueue[msg->channel]; > + } else { > + /* RX transfer */ > + queue = &ste_port->rxqueue[msg->channel]; > + } > + > + spin_lock_bh(&ste_hsi->lock); > + list_add_tail(&msg->link, queue); > + msg->status = HSI_STATUS_QUEUED; > + > + err = ste_hsi_start_transfer(ste_port, queue); > + if (err) > + list_del(&msg->link); > + > + spin_unlock_bh(&ste_hsi->lock); > + > + return err; > +} > + > +static int ste_hsi_setup(struct hsi_client *cl) > +{ > + struct hsi_port *port = to_hsi_port(cl->device.parent); > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + int err, i, buffers; > + u32 div = 0; > + > + err = ste_hsi_clock_enable(hsi); > + if (unlikely(err)) > + return err; > + > + if (cl->tx_cfg.speed) { > + div = clk_get_rate(ste_hsi->tx_clk) / 1000 / cl->tx_cfg.speed; > + if (div) > + --div; > + } > + > + port->tx_cfg = cl->tx_cfg; > + port->rx_cfg = cl->rx_cfg; > + /* Configure TX */ > + writel(cl->tx_cfg.mode, ste_hsi->tx_base + STE_HSI_TX_MODE); > + writel(div, ste_hsi->tx_base + STE_HSI_TX_DIVISOR); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_PARITY); > + /* TODO: Wait for idle here */ > + writel(cl->tx_cfg.channels, ste_hsi->tx_base + STE_HSI_TX_CHANNELS); > + /* Calculate buffers number per channel */ > + buffers = STE_HSI_MAX_BUFFERS / cl->tx_cfg.channels; > + for (i = 0; i < cl->tx_cfg.channels; i++) { > + /* Set 32 bit long frames */ > + writel(31, ste_hsi->tx_base + STE_HSI_TX_FRAMELENX + 4 * i); > + writel(buffers * i, > + ste_hsi->tx_base + STE_HSI_TX_BASEX + 4 * i); > + writel(buffers - 1, > + ste_hsi->tx_base + STE_HSI_TX_SPANX + 4 * i); > + writel(buffers - 1, > + ste_hsi->tx_base + STE_HSI_TX_WATERMARKX + 4 * i); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_WATERMARKX + 4 * i); > + } > + > + /* Configure RX */ > + writel(cl->rx_cfg.mode, ste_hsi->rx_base + STE_HSI_RX_MODE); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_PARITY); > + writel(cl->rx_cfg.channels, ste_hsi->rx_base + STE_HSI_RX_CHANNELS); > + /* Calculate buffers number per channel */ > + buffers = STE_HSI_MAX_BUFFERS / cl->rx_cfg.channels; > + for (i = 0; i < cl->rx_cfg.channels; i++) { > + /* Set 32 bit long frames */ > + writel(31, ste_hsi->rx_base + STE_HSI_RX_FRAMELENX + 4 * i); > + writel(buffers * i, > + ste_hsi->rx_base + STE_HSI_RX_BASEX + 4 * i); > + writel(buffers - 1, > + ste_hsi->rx_base + STE_HSI_RX_SPANX + 4 * i); > + writel(buffers - 1, > + ste_hsi->rx_base + STE_HSI_RX_WATERMARKX + 4 * i); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_WATERMARKX + 4 * i); > + } > + > + ste_port->channels = max(cl->tx_cfg.channels, cl->rx_cfg.channels); > + > + ste_hsi_setup_dma(cl); > + > + ste_hsi_clock_disable(hsi); > + return err; > +} > + > +static int ste_hsi_flush(struct hsi_client *cl) > +{ > + struct hsi_port *port = to_hsi_port(cl->device.parent); > + struct ste_hsi_port *ste_port = hsi_port_drvdata(port); > + struct hsi_controller *hsi = to_hsi_controller(port->device.parent); > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + int i; > + > + ste_hsi_clock_enable(hsi); > + > + /* Enter sleep mode */ > + writel(STE_HSI_MODE_SLEEP, ste_hsi->rx_base + STE_HSI_RX_MODE); > + > + /* Disable DMA, and terminate all outstanding jobs */ > + writel(0, ste_hsi->tx_base + STE_HSI_TX_DMAEN); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_DMAEN); > + ste_hsi_terminate_dma(ste_port); > + > + /* Flush all HSIR and HSIT buffers */ > + writel(0, ste_hsi->tx_base + STE_HSI_TX_STATE); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_BUFSTATE); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_STATE); > + /* > + * BUFSTATE is cleared twice on purpose: > + * first time all fifos are cleared > + * second time to clear data that was in pipline buffer > + * and was transfered to fifos > + */ > + writel(0, ste_hsi->rx_base + STE_HSI_RX_BUFSTATE); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_BUFSTATE); > + > + /* Flush all errors */ > + writel(0, ste_hsi->rx_base + STE_HSI_RX_EXCEP); > + > + /* Clear interrupts */ > + writel(0, ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_WATERMARKIC); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_WATERMARKIC); > + writel(0xFF, ste_hsi->rx_base + STE_HSI_RX_OVERRUNACK); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_EXCEPIM); > + writel(0x0F, ste_hsi->rx_base + STE_HSI_RX_ACK); > + > + /* Dequeue all pending requests */ > + for (i = 0; i < ste_port->channels; i++) { > + /* Release write clocks */ > + if (!list_empty(&ste_port->txqueue[i])) > + ste_hsi_clock_disable(hsi); > + if (!list_empty(&ste_port->rxqueue[i])) > + ste_hsi_clock_disable(hsi); > + ste_hsi_flush_queue(&ste_port->txqueue[i], NULL); > + ste_hsi_flush_queue(&ste_port->rxqueue[i], NULL); > + } > + ste_hsi_flush_queue(&ste_port->brkqueue, NULL); > + > + ste_hsi_clock_disable(hsi); > + > + return 0; > +} > + > +static int ste_hsi_start_tx(struct hsi_client *cl) > +{ > + return 0; > +} > + > +static int ste_hsi_stop_tx(struct hsi_client *cl) > +{ > + return 0; > +} > + The above two functions are not necessary. The framework already set dummy callback functions for the ones that you don't need. > +static int ste_hsi_release(struct hsi_client *cl) > +{ > + int err; > + struct ste_hsi_controller *ste_hsi = client_to_ste_controller(cl); > + > + err = ste_hsi_flush(cl); > + cancel_delayed_work(&ste_hsi->clk_work); Why do you need to cancel delayed work here ? Isn't so that if you happen indeed to have clk_work scheduled, you will break PM ? You will leave the HSI clocks enabled although no one is using the hsi port anymore. > + > + return 0; > +} > + > +static int ste_hsi_ports_init(struct hsi_controller *hsi, > + struct platform_device *pdev) > +{ > + struct hsi_port *port; > + struct ste_hsi_port *ste_port; > + unsigned int i; > + char irq_name[20]; > + int err; > + > + for (i = 0; i < hsi->num_ports; i++) { > + ste_port = devm_kzalloc(&pdev->dev, sizeof *ste_port, > + GFP_KERNEL); > + if (!ste_port) > + return -ENOMEM; > + > + port = &hsi->port[i]; > + port->async = ste_hsi_async; > + port->setup = ste_hsi_setup; > + port->flush = ste_hsi_flush; > + port->start_tx = ste_hsi_start_tx; > + port->stop_tx = ste_hsi_stop_tx; The above two lines are not needed. See my previous comment on the matter. > + port->release = ste_hsi_release; > + hsi_port_set_drvdata(port, ste_port); > + ste_port->dev = &port->device; > + > + sprintf(irq_name, "hsi_rx_irq%d", i); > + err = ste_hsi_get_irq(pdev, irq_name, ste_hsi_rx_isr, port, > + &ste_port->rx_irq); > + if (err) > + return err; > + > + sprintf(irq_name, "hsi_tx_irq%d", i); > + err = ste_hsi_get_irq(pdev, irq_name, ste_hsi_tx_isr, port, > + &ste_port->tx_irq); > + if (err) > + return err; > + > + tasklet_init(&ste_port->rx_tasklet, ste_hsi_rx_tasklet, > + (unsigned long)port); > + > + tasklet_init(&ste_port->tx_tasklet, ste_hsi_tx_tasklet, > + (unsigned long)port); > + > + tasklet_init(&ste_port->exception_tasklet, > + ste_hsi_exception_tasklet, (unsigned long)port); > + > + tasklet_init(&ste_port->overrun_tasklet, > + ste_hsi_overrun_tasklet, (unsigned long)port); > + > + sprintf(irq_name, "hsi_rx_excep%d", i); > + err = ste_hsi_get_irq(pdev, irq_name, ste_hsi_exception_isr, > + port, &ste_port->excep_irq); > + if (err) > + return err; > + > + ste_hsi_queues_init(ste_port); > + } > + return 0; > +} > + > +static int __init ste_hsi_hw_init(struct hsi_controller *hsi) > +{ > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + int err; > + > + err = ste_hsi_clock_enable(hsi); > + if (unlikely(err)) > + return err; > + > + writel(0, ste_hsi->tx_base + STE_HSI_TX_BUFSTATE); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_FLUSHBITS); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_PRIORITY); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_BURSTLEN); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_PREAMBLE); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_DATASWAP); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_DMAEN); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_WATERMARKID); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_WATERMARKIC); > + writel(0, ste_hsi->tx_base + STE_HSI_TX_WATERMARKIM); > + > + /* 0x23 is reset value per DB8500 Design Spec */ > + writel(0x23, ste_hsi->rx_base + STE_HSI_RX_THRESHOLD); > + > + writel(0, ste_hsi->rx_base + STE_HSI_RX_BUFSTATE); > + > + /* Bits 0,1,2 set to 1 to clear exception flags */ > + writel(0x07, ste_hsi->rx_base + STE_HSI_RX_ACK); > + > + /* Bits 0..7 set to 1 to clear OVERRUN IRQ */ > + writel(0xFF, ste_hsi->rx_base + STE_HSI_RX_OVERRUNACK); > + > + writel(0, ste_hsi->rx_base + STE_HSI_RX_DMAEN); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_WATERMARKIC); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_WATERMARKIM); > + writel(0, ste_hsi->rx_base + STE_HSI_RX_OVERRUNIM); > + > + /* Flush all errors */ > + writel(0, ste_hsi->rx_base + STE_HSI_RX_EXCEP); > + > + /* 2 is Flush state, no RX exception generated afterwards */ > + writel(2, ste_hsi->rx_base + STE_HSI_RX_STATE); > + > + writel(0, ste_hsi->rx_base + STE_HSI_RX_EXCEPIM); > + > + ste_hsi_clock_disable(hsi); > + > + return err; > +} > + > +static int __init ste_hsi_add_controller(struct hsi_controller *hsi, > + struct platform_device *pdev) > +{ > + struct ste_hsi_controller *ste_hsi; > + char overrun_name[] = "hsi_rx_overrun_chxxx"; > + unsigned char i; > + int err; > + > + ste_hsi = kzalloc(sizeof(struct ste_hsi_controller), GFP_KERNEL); it is ok, although I think "sizeof(*ste_hsi)" is better. > + if (!ste_hsi) { > + dev_err(&pdev->dev, "Not enough memory for ste_hsi!\n"); > + return -ENOMEM; > + } > + > + spin_lock_init(&ste_hsi->lock); > + spin_lock_init(&ste_hsi->ck_lock); > + INIT_DELAYED_WORK(&ste_hsi->clk_work, ste_hsi_delayed_disable_clock); > + > + hsi->id = pdev->id; > + hsi->device.parent = &pdev->dev; > + dev_set_name(&hsi->device, "ste-hsi.%d", hsi->id); > + ste_hsi->dev = &hsi->device; > + hsi_controller_set_drvdata(hsi, ste_hsi); > + > + /* Get and reserve resources for receiver */ > + err = ste_hsi_get_iomem(pdev, "hsi_rx_base", &ste_hsi->rx_base, > + &ste_hsi->rx_dma_base); > + if (err) > + goto err_free_mem; > + dev_info(&pdev->dev, "hsi_rx_base = %p\n", ste_hsi->rx_base); > + > + /* Get and reserve resources for transmitter */ > + err = ste_hsi_get_iomem(pdev, "hsi_tx_base", &ste_hsi->tx_base, > + &ste_hsi->tx_dma_base); > + if (err) > + goto err_free_mem; > + dev_info(&pdev->dev, "hsi_tx_base = %p\n", ste_hsi->tx_base); > + > + /* Get HSIT HSITXCLK clock */ > + ste_hsi->tx_clk = clk_get(&pdev->dev, "hsit_hsitxclk"); > + if (IS_ERR(ste_hsi->tx_clk)) { > + dev_err(&hsi->device, "Couldn't get HSIT HSITXCLK clock\n"); > + err = PTR_ERR(ste_hsi->tx_clk); > + goto err_free_mem; > + } > + > + /* Get HSIR HSIRXCLK clock */ > + ste_hsi->rx_clk = clk_get(&pdev->dev, "hsir_hsirxclk"); > + if (IS_ERR(ste_hsi->rx_clk)) { > + dev_err(&hsi->device, "Couldn't get HSIR HSIRXCLK clock\n"); > + err = PTR_ERR(ste_hsi->rx_clk); > + goto err_clk_free; > + } > + > + /* Get HSIT HCLK clock */ > + ste_hsi->ssitx_clk = clk_get(&pdev->dev, "hsit_hclk"); > + if (IS_ERR(ste_hsi->ssitx_clk)) { > + dev_err(&hsi->device, "Couldn't get HSIT HCLK clock\n"); > + err = PTR_ERR(ste_hsi->ssitx_clk); > + goto err_clk_free; > + } > + > + /* Get HSIR HCLK clock */ > + ste_hsi->ssirx_clk = clk_get(&pdev->dev, "hsir_hclk"); > + if (IS_ERR(ste_hsi->ssirx_clk)) { > + dev_err(&hsi->device, "Couldn't get HSIR HCLK clock\n"); > + err = PTR_ERR(ste_hsi->ssirx_clk); > + goto err_clk_free; > + } > + > + err = ste_hsi_clock_enable(hsi); > + if (unlikely(err)) > + goto err_clk_free; > + > + /* Check if controller is at specified address */ > + if (compare_periphid(ste_hsir_periphid, > + (u32 *) (ste_hsi->rx_base + 0xFE0), 8)) { > + dev_err(&pdev->dev, "No hsir controller at = %p\n", > + ste_hsi->rx_base); > + err = -ENXIO; > + goto err_clk_free; > + } > + > + /* Check if controller is at specified address */ > + if (compare_periphid(ste_hsit_periphid, > + (u32 *) (ste_hsi->tx_base + 0xFE0), 8)) { > + dev_err(&pdev->dev, "No hsit controller at = %p\n", > + ste_hsi->tx_base); > + err = -ENXIO; > + goto err_clk_free; > + } > + ste_hsi_clock_disable(hsi); > + > + err = ste_hsi_hw_init(hsi); > + if (err) { > + dev_err(&pdev->dev, "Failed to init HSI controller!\n"); > + goto err_clk_free; > + } > + > + for (i = 0; i < STE_HSI_MAX_CHANNELS; i++) { > + sprintf(overrun_name, "hsi_rx_overrun_ch%d", i); > + err = ste_hsi_get_irq(pdev, overrun_name, ste_hsi_overrun_isr, > + hsi, &ste_hsi->overrun_irq[i]); > + if (err) > + goto err_clk_free; > + } > + > + err = ste_hsi_ports_init(hsi, pdev); > + if (err) > + goto err_clk_free; > + > + err = hsi_register_controller(hsi); > + if (err) > + goto err_clk_free; > + > + return 0; > + > +err_clk_free: > + ste_hsi_clks_free(ste_hsi); > +err_free_mem: > + kfree(ste_hsi); > + return err; > +} > + > +static int ste_hsi_remove_controller(struct hsi_controller *hsi, > + struct platform_device *pdev) > +{ > + struct ste_hsi_controller *ste_hsi = hsi_controller_drvdata(hsi); > + > + ste_hsi_clks_free(ste_hsi); > + hsi_unregister_controller(hsi); > + > + return 0; > +} > + > +static int __init ste_hsi_probe(struct platform_device *pdev) > +{ > + struct hsi_controller *hsi; > + struct ste_hsi_platform_data *pdata = pdev->dev.platform_data; > + int err; > + > + if (!pdata) { > + dev_err(&pdev->dev, "No HSI platform data!\n"); > + return -EINVAL; > + } > + > + hsi = hsi_alloc_controller(pdata->num_ports, GFP_KERNEL); > + if (!hsi) { > + dev_err(&pdev->dev, "No memory to allocate HSI controller!\n"); > + return -ENOMEM; > + } > + platform_set_drvdata(pdev, hsi); > + > + err = ste_hsi_add_controller(hsi, pdev); > + if (err < 0) { > + dev_err(&pdev->dev, "Can't add HSI controller!\n"); > + goto err_free_controller; > + } > + > + if (pdata->use_dma) > + ste_hsi_init_dma(pdata, hsi); > + > + return 0; > + > +err_free_controller: > + platform_set_drvdata(pdev, NULL); > + hsi_free_controller(hsi); > + > + return err; > +} > + > +static int __exit ste_hsi_remove(struct platform_device *pdev) > +{ > + struct hsi_controller *hsi = platform_get_drvdata(pdev); > + > + ste_hsi_remove_controller(hsi, pdev); > + hsi_free_controller(hsi); > + > + return 0; > +} > + > +static struct platform_driver ste_hsi_driver __refdata = { > + .driver = { > + .name = "ste_hsi", > + .owner = THIS_MODULE, > + }, > + .remove = __exit_p(ste_hsi_remove), > +}; > + > +static int __init ste_hsi_init(void) > +{ > + return platform_driver_probe(&ste_hsi_driver, ste_hsi_probe); > +} > + > +module_init(ste_hsi_init) > + > +static void __exit ste_hsi_exit(void) > +{ > + platform_driver_unregister(&ste_hsi_driver); > +} > + > +module_exit(ste_hsi_exit) > + > + MODULE_AUTHOR("Lukasz Baj + MODULE_DESCRIPTION("STE HSI driver."); > + MODULE_LICENSE("GPL"); It is not clear to me: Is the license GPLv2 only ? If it is so then you should say so also: MODULE_LICENSE("GPLv2") otherwise I will suggest to update the header to GPLv2 or later. Br, -- Carlos Chinea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/