* [PATCH net-next 0/2] cpsw: fix resource leak for v3.8
From: Richard Cochran @ 2012-11-03 8:25 UTC (permalink / raw)
To: linux-arm-kernel
While looking at the idea of removing all of the register offsets in
the CPSW's device tree, I noticed that the driver would be leaking IO
mappings. Although this is, strictly speaking, a bug fix, still it can
wait to appear in v3.8, since there is no way to use the driver in
v3.7 (or earlier) anyhow.
Thanks,
Richard
Richard Cochran (2):
cpsw: rename register banks to match the reference manual, part 2
cpsw: fix leaking IO mappings
drivers/net/ethernet/ti/cpsw.c | 39 +++++++++++++++++++--------------------
1 files changed, 19 insertions(+), 20 deletions(-)
--
1.7.2.5
^ permalink raw reply
* [PATCH net-next 1/2] cpsw: rename register banks to match the reference manual, part 2
From: Richard Cochran @ 2012-11-03 8:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1351930782.git.richardcochran@gmail.com>
The code mixes up the CPSW_SS and the CPSW_WR register naming. This patch
changes the names to conform to the published Technical Reference Manual
from TI, in order to make working on the code less confusing.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/ti/cpsw.c | 26 +++++++++++++-------------
1 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 023d439..6215246 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -286,7 +286,7 @@ struct cpsw_priv {
struct platform_device *pdev;
struct net_device *ndev;
struct resource *cpsw_res;
- struct resource *cpsw_ss_res;
+ struct resource *cpsw_wr_res;
struct napi_struct napi;
struct device *dev;
struct cpsw_platform_data data;
@@ -1270,25 +1270,25 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
priv->host_port_regs = regs + data->host_port_reg_ofs;
priv->cpts.reg = regs + data->cpts_reg_ofs;
- priv->cpsw_ss_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- if (!priv->cpsw_ss_res) {
+ priv->cpsw_wr_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!priv->cpsw_wr_res) {
dev_err(priv->dev, "error getting i/o resource\n");
ret = -ENOENT;
goto clean_clk_ret;
}
- if (!request_mem_region(priv->cpsw_ss_res->start,
- resource_size(priv->cpsw_ss_res), ndev->name)) {
+ if (!request_mem_region(priv->cpsw_wr_res->start,
+ resource_size(priv->cpsw_wr_res), ndev->name)) {
dev_err(priv->dev, "failed request i/o region\n");
ret = -ENXIO;
goto clean_clk_ret;
}
- regs = ioremap(priv->cpsw_ss_res->start,
- resource_size(priv->cpsw_ss_res));
+ regs = ioremap(priv->cpsw_wr_res->start,
+ resource_size(priv->cpsw_wr_res));
if (!regs) {
dev_err(priv->dev, "unable to map i/o region\n");
- goto clean_cpsw_ss_iores_ret;
+ goto clean_cpsw_wr_iores_ret;
}
priv->wr_regs = regs;
@@ -1409,9 +1409,9 @@ clean_dma_ret:
cpdma_ctlr_destroy(priv->dma);
clean_iomap_ret:
iounmap(priv->regs);
-clean_cpsw_ss_iores_ret:
- release_mem_region(priv->cpsw_ss_res->start,
- resource_size(priv->cpsw_ss_res));
+clean_cpsw_wr_iores_ret:
+ release_mem_region(priv->cpsw_wr_res->start,
+ resource_size(priv->cpsw_wr_res));
clean_cpsw_iores_ret:
release_mem_region(priv->cpsw_res->start,
resource_size(priv->cpsw_res));
@@ -1442,8 +1442,8 @@ static int __devexit cpsw_remove(struct platform_device *pdev)
iounmap(priv->regs);
release_mem_region(priv->cpsw_res->start,
resource_size(priv->cpsw_res));
- release_mem_region(priv->cpsw_ss_res->start,
- resource_size(priv->cpsw_ss_res));
+ release_mem_region(priv->cpsw_wr_res->start,
+ resource_size(priv->cpsw_wr_res));
pm_runtime_disable(&pdev->dev);
clk_put(priv->clk);
kfree(priv->slaves);
--
1.7.2.5
^ permalink raw reply related
* [PATCH net-next 2/2] cpsw: fix leaking IO mappings
From: Richard Cochran @ 2012-11-03 8:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1351930782.git.richardcochran@gmail.com>
The CPSW driver remaps two different IO regions, but fails to unmap them
both. This patch fixes the issue by calling iounmap in the appropriate
places.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/ti/cpsw.c | 17 ++++++++---------
1 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 6215246..7654a62 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1252,14 +1252,12 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
ret = -ENOENT;
goto clean_clk_ret;
}
-
if (!request_mem_region(priv->cpsw_res->start,
resource_size(priv->cpsw_res), ndev->name)) {
dev_err(priv->dev, "failed request i/o region\n");
ret = -ENXIO;
goto clean_clk_ret;
}
-
regs = ioremap(priv->cpsw_res->start, resource_size(priv->cpsw_res));
if (!regs) {
dev_err(priv->dev, "unable to map i/o region\n");
@@ -1274,16 +1272,14 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
if (!priv->cpsw_wr_res) {
dev_err(priv->dev, "error getting i/o resource\n");
ret = -ENOENT;
- goto clean_clk_ret;
+ goto clean_iomap_ret;
}
-
if (!request_mem_region(priv->cpsw_wr_res->start,
resource_size(priv->cpsw_wr_res), ndev->name)) {
dev_err(priv->dev, "failed request i/o region\n");
ret = -ENXIO;
- goto clean_clk_ret;
+ goto clean_iomap_ret;
}
-
regs = ioremap(priv->cpsw_wr_res->start,
resource_size(priv->cpsw_wr_res));
if (!regs) {
@@ -1326,7 +1322,7 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
if (!priv->dma) {
dev_err(priv->dev, "error initializing dma\n");
ret = -ENOMEM;
- goto clean_iomap_ret;
+ goto clean_wr_iomap_ret;
}
priv->txch = cpdma_chan_create(priv->dma, tx_chan_num(0),
@@ -1407,11 +1403,13 @@ clean_dma_ret:
cpdma_chan_destroy(priv->txch);
cpdma_chan_destroy(priv->rxch);
cpdma_ctlr_destroy(priv->dma);
-clean_iomap_ret:
- iounmap(priv->regs);
+clean_wr_iomap_ret:
+ iounmap(priv->wr_regs);
clean_cpsw_wr_iores_ret:
release_mem_region(priv->cpsw_wr_res->start,
resource_size(priv->cpsw_wr_res));
+clean_iomap_ret:
+ iounmap(priv->regs);
clean_cpsw_iores_ret:
release_mem_region(priv->cpsw_res->start,
resource_size(priv->cpsw_res));
@@ -1442,6 +1440,7 @@ static int __devexit cpsw_remove(struct platform_device *pdev)
iounmap(priv->regs);
release_mem_region(priv->cpsw_res->start,
resource_size(priv->cpsw_res));
+ iounmap(priv->wr_regs);
release_mem_region(priv->cpsw_wr_res->start,
resource_size(priv->cpsw_wr_res));
pm_runtime_disable(&pdev->dev);
--
1.7.2.5
^ permalink raw reply related
* [PATCH v2 2/2] ARM: OMAP: omap_device: Correct resource handling for DT boot
From: Kevin Hilman @ 2012-11-03 8:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351596296-13825-3-git-send-email-peter.ujfalusi@ti.com>
On 10/30/2012 12:24 PM, Peter Ujfalusi wrote:
> When booting with DT the OF core can fill up the resources provided within
> the DT blob.
> The current way of handling the DT boot prevents us from removing hwmod data
> for platforms only suppose to boot with DT (OMAP5 for example) since we need
> to keep the whole hwmod database intact in order to have more resources in
> hwmod than in DT (to be able to append the DMA resource from hwmod).
>
> To fix this issue we just examine the OF provided resources:
> If we do not have resources we use hwmod to fill them.
> If we have resources we check if we already able to recive DMA resource, if
> no we only append the DMA resurce from hwmod to the OF provided ones.
>
> In this way we can start removing hwmod data for devices which have their
> resources correctly configured in DT without regressions.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>ke
Acked-by: Kevin Hilman <khilman@ti.com>
Benoit, feel free to take this one as well.
Kevin
^ permalink raw reply
* [RFC 00/15] Add basic suspend-resume support for AM33XX
From: Bedia, Vaibhav @ 2012-11-03 8:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50944655.20602@gmail.com>
Hi Daniel,
On Sat, Nov 03, 2012 at 03:46:53, Daniel Mack wrote:
[...]
>
> What event did you use to bring the system back to life? I tried a GPIO
> button which has "linux,wakeup" set and is connected to GPIO bank 0, but
> without success.
I used uart wakeup in my testing. I see that you have CPSW driver also
in your kernel. Can you try with a minimal set of drivers, similar to what
BeagleBone has on Tony's master branch right now.
Mugunthan mentioned he has a fix of the WARN_ON from CPSW driver that you
see in the logs and that will be posted shortly. Can you mention what other
patches you have pulled in?
Regards,
Vaibhav
^ permalink raw reply
* [PATCH 02/15] ARM: OMAP2+: mailbox: Add support for AM33XX
From: Bedia, Vaibhav @ 2012-11-03 8:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+Bv8XY8x=BDgGDNHPMRqz3xUbzKf8+o0MJQDovHY+NqBh6NVw@mail.gmail.com>
Hi Russ,
On Sat, Nov 03, 2012 at 05:44:21, Russ Dill wrote:
[...]
> > - if (!cpu_is_omap44xx())
> > + if (!cpu_is_omap44xx() || !soc_is_am33xx())
> > bit = mbox_read_reg(p->irqdisable) & ~bit;
>
> Do you mean &&?
>
I didn't change that line but it looks ok to me :)
Regards,
Vaibhav
^ permalink raw reply
* [PATCH for soc] socfpga: map uart into virtual address space so that early_printk() works
From: Pavel Machek @ 2012-11-03 11:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121029002724.GA27891@elf.ucw.cz>
Hi!
> Early printk code needs UART to be mapped early during
> boot. early_printk() is left there during the start-up; it is useful
> as our emulators are fairly slow.
>
> Signed-off-by: Pavel Machek <pavel@denx.de>
> Acked-by: Dinh Nguyen <dinguyen@altera.com>
Could we get this one applied? No comments for quite a while... and it
is needed for machine not to crash with early_printk used...
Pavel
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index ab81ea9..49fb62b 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -37,6 +37,15 @@ static struct map_desc scu_io_desc __initdata = {
> .type = MT_DEVICE,
> };
>
> +
> +
> +static struct map_desc uart_io_desc __initdata = {
> + .virtual = 0xfec02000,
> + .pfn = __phys_to_pfn(0xffc02000),
> + .length = SZ_8K,
> + .type = MT_DEVICE,
> +};
> +
> static void __init socfpga_scu_map_io(void)
> {
> unsigned long base;
> @@ -51,6 +60,8 @@ static void __init socfpga_scu_map_io(void)
> static void __init socfpga_map_io(void)
> {
> socfpga_scu_map_io();
> + iotable_init(&uart_io_desc, 1);
> + early_printk("Early printk initialized\n");
> }
>
> const static struct of_device_id irq_match[] = {
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* [PATCH 11/15] ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
From: Kevin Hilman @ 2012-11-03 11:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351859566-24818-12-git-send-email-vaibhav.bedia@ti.com>
On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> AM33XX has only one usable timer in the WKUP domain.
> Currently the timer instance in WKUP domain is used
> as the clockevent and the timer in non-WKUP domain
> as the clocksource. The timer in WKUP domain can keep
> running in suspend from a 32K clock and hence serve
> as the persistent clock. To enable this, interchange
> the timers used as clocksource and clockevent for
> AM33XX.
Doesn't this also mean that you won't get timer wakeups
in idle? Or are you keeping the domain where the clockevent is
on during idle?
Kevin
^ permalink raw reply
* [PATCH v4] Network driver for the Armada 370 and Armada XP ARM Marvell SoCs
From: Francois Romieu @ 2012-11-03 11:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121102232137.3c9da5e4@skate>
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> :
[...]
> Any comments on this patch set? What should I do to make it progress
> towards integration?
Nits review above. I'll search more substantial things this evening.
It looks quite good.
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> :
[...]
> +mvneta_rxq_next_desc_get(struct mvneta_rx_queue *rxq)
> +{
> + int rx_desc = rxq->next_desc_to_proc;
> + rxq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(rxq, rx_desc);
Insert an empty line after local variables declaration.
[...]
> +static struct mvneta_tx_desc *
> +mvneta_txq_next_desc_get(struct mvneta_tx_queue *txq)
> +{
> + int tx_desc = txq->next_desc_to_proc;
> + txq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(txq, tx_desc);
Insert an empty line after local variables declaration.
[...]
> +static struct mvneta_tx_queue *mvneta_tx_done_policy(struct mvneta_port *pp,
> + u32 cause)
> +{
> + int queue;
> + queue = fls(cause) - 1;
Insert an empty line after local variables declaration.
You may set this variable when you declare it.
> + if (queue < 0 || queue >= txq_number)
> + return NULL;
> + return &pp->txqs[queue];
return (queue < 0 || queue >= txq_number) ? NULL : &pp->txqs[queue];
[...]
> +static int mvneta_rx_refill(struct mvneta_port *pp,
> + struct mvneta_rx_desc *rx_desc)
> +
> +{
> + dma_addr_t phys_addr;
> + struct sk_buff *skb;
> +
> + skb = netdev_alloc_skb(pp->dev, pp->pkt_size);
> + if (!skb)
> + return 1;
> +
> + phys_addr = dma_map_single(pp->dev->dev.parent, skb->head,
> + MVNETA_RX_BUF_SIZE(pp->pkt_size),
> + DMA_FROM_DEVICE);
> + if (unlikely(dma_mapping_error(pp->dev->dev.parent,
> + phys_addr))) {
if (unlikely(dma_mapping_error(pp->dev->dev.parent, phys_addr))) {
> + dev_kfree_skb(skb);
> + return 1;
> + }
> +
> + mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)skb);
> +
> + return 0;
> +}
This is a bool returning function in disguise.
Were it supposed to be an error returning function, it should return <= 0
[...]
> +static struct mvneta_rx_queue *mvneta_rx_policy(struct mvneta_port *pp,
> + u32 cause)
> +{
> + int queue = fls(cause >> 8) - 1;
Insert an empty line after local variables declaration.
> + if (queue < 0 || queue >= rxq_number)
> + return NULL;
> + return &pp->rxqs[queue];
See mvneta_tx_done_policy above.
[...]
> +static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
> + struct mvneta_rx_queue *rxq)
> +{
> + int rx_done, i;
> +
> + rx_done = mvneta_rxq_busy_desc_num_get(pp, rxq);
> + for (i = 0; i < rxq->size; i++) {
> + struct mvneta_rx_desc *rx_desc = rxq->descs + i;
> + struct sk_buff *skb = (struct sk_buff *)rx_desc->buf_cookie;
> + dev_kfree_skb_any(skb);
Insert an empty line after local variables declaration.
[...]
> +static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
> + struct mvneta_rx_queue *rxq)
> +{
> + struct net_device *dev = pp->dev;
> + int rx_done, rx_filled;
> +
> + /* Get number of received packets */
> + rx_done = mvneta_rxq_busy_desc_num_get(pp, rxq);
> +
> + if (rx_todo > rx_done)
> + rx_todo = rx_done;
> +
> + rx_done = 0;
> + rx_filled = 0;
> +
> + /* Fairness NAPI loop */
> + while (rx_done < rx_todo) {
> + struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
> + struct sk_buff *skb;
> + u32 rx_status;
> + int rx_bytes, err;
> +
> + prefetch(rx_desc);
> + rx_done++;
> + rx_filled++;
> + rx_status = rx_desc->status;
> + skb = (struct sk_buff *)rx_desc->buf_cookie;
> +
> + if (((rx_status & MVNETA_RXD_FIRST_LAST_DESC)
> + != MVNETA_RXD_FIRST_LAST_DESC)
> + || (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
Operators appear at the end.
A dedicated (inline) function will help.
[...]
> +static int mvneta_tx_frag_process(struct mvneta_port *pp, struct sk_buff *skb,
[...]
> + if (i == (skb_shinfo(skb)->nr_frags - 1)) {
> + /* Last descriptor */
> + tx_desc->command = (MVNETA_TXD_L_DESC |
> + MVNETA_TXD_Z_PAD);
tx_desc->command = MVNETA_TXD_L_DESC | MVNETA_TXD_Z_PAD;
[...]
> +error:
> + /* Release all descriptors that were used to map fragments of
> + * this packet, as well as the corresponding DMA mappings */
> + for (j = i - 1; j >= 0; j--) {
j isn't needed. Recycle i.
[...]
> +static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct mvneta_port *pp = netdev_priv(dev);
> + struct mvneta_tx_queue *txq = &pp->txqs[txq_def];
> + struct netdev_queue *nq;
> + struct mvneta_tx_desc *tx_desc;
> + int frags = 0;
> + u32 tx_cmd;
Long lines first when possible.
[...]
> + /* Continue with other skb fragments */
> + if (mvneta_tx_frag_process(pp, skb, txq)) {
got out_unmap;
> + dma_unmap_single(dev->dev.parent,
> + tx_desc->buf_phys_addr,
> + tx_desc->data_size,
> + DMA_TO_DEVICE);
> + mvneta_txq_desc_put(txq);
> + frags = 0;
> + goto out;
> + }
[...]
> +static void mvneta_txq_done_force(struct mvneta_port *pp,
> + struct mvneta_tx_queue *txq)
> +
> +{
> + int tx_done = txq->count;
> + mvneta_txq_bufs_free(pp, txq, tx_done);
Insert an empty line after local variables declaration.
[...]
> +static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
> + int num)
> +{
> + int i;
> + struct net_device *dev = pp->dev;
Long lines first when possible.
[...]
> +static int mvneta_rxq_init(struct mvneta_port *pp,
> + struct mvneta_rx_queue *rxq)
> +
> +{
> + rxq->size = pp->rx_ring_size;
> +
> + /* Allocate memory for RX descriptors */
> + rxq->descs = dma_alloc_coherent(pp->dev->dev.parent,
> + rxq->size * MVNETA_DESC_ALIGNED_SIZE,
> + &rxq->descs_phys,
> + GFP_KERNEL);
&rxq->descs_phys, GFP_KERNEL);
> + if (rxq->descs == NULL) {
> + netdev_err(pp->dev,
> + "rxQ=%d: Can't allocate %d bytes for %d RX descr\n",
> + rxq->id, rxq->size * MVNETA_DESC_ALIGNED_SIZE,
> + rxq->size);
> + return -ENOMEM;
> + }
> +
> + BUG_ON(rxq->descs !=
> + PTR_ALIGN(rxq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));
There is no reason to crash.
(...]
> +static int mvneta_txq_init(struct mvneta_port *pp,
> + struct mvneta_tx_queue *txq)
> +{
> + txq->size = pp->tx_ring_size;
> +
> + /* Allocate memory for TX descriptors */
> + txq->descs = dma_alloc_coherent(pp->dev->dev.parent,
> + txq->size * MVNETA_DESC_ALIGNED_SIZE,
> + &txq->descs_phys,
> + DMA_BIDIRECTIONAL);
&txq->descs_phys, DMA_BIDIRECTIONAL);
> + if (txq->descs == NULL) {
> + netdev_err(pp->dev,
> + "txQ=%d: Can't allocate %d bytes for %d TX descr\n",
> + txq->id, txq->size * MVNETA_DESC_ALIGNED_SIZE,
> + txq->size);
> + return -ENOMEM;
> + }
> +
> + /* Make sure descriptor address is cache line size aligned */
> + BUG_ON(txq->descs !=
> + PTR_ALIGN(txq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));
There is no reason to crash.
[...]
> + txq->tx_skb = kmalloc(txq->size * sizeof(*txq->tx_skb),
> + GFP_KERNEL);
txq->tx_skb = kmalloc(txq->size * sizeof(*txq->tx_skb), GFP_KERNEL);
[...]
> +static void mvneta_txq_deinit(struct mvneta_port *pp,
> + struct mvneta_tx_queue *txq)
> +{
> + kfree(txq->tx_skb);
> +
> + if (txq->descs)
> + dma_free_coherent(pp->dev->dev.parent,
> + txq->size * MVNETA_DESC_ALIGNED_SIZE,
> + txq->descs,
> + txq->descs_phys);
txq->descs, txq->descs_phys);
[...]
> +static void mvneta_cleanup_txqs(struct mvneta_port *pp)
> +{
> + int queue;
> + for (queue = 0; queue < txq_number; queue++)
> + mvneta_txq_deinit(pp, &pp->txqs[queue]);
Insert an empty line after local variables declaration.
[...]
> +static void mvneta_cleanup_rxqs(struct mvneta_port *pp)
> +{
> + int queue;
> + for (queue = 0; queue < rxq_number; queue++)
> + mvneta_rxq_deinit(pp, &pp->rxqs[queue]);
Insert an empty line after local variables declaration.
[...]
> +static int mvneta_setup_rxqs(struct mvneta_port *pp)
> +{
> + int queue;
> +
> + for (queue = 0; queue < rxq_number; queue++) {
> + int err = mvneta_rxq_init(pp, &pp->rxqs[queue]);
> + if (err) {
> + netdev_err(pp->dev,
> + "%s: can't create RxQ rxq=%d\n",
netdev_err(pp->dev, "%s: can't create RxQ rxq=%d\n",
> + __func__, queue);
> + mvneta_cleanup_rxqs(pp);
> + return -ENODEV;
mvneta_rxq_init should return a proper error code and it should be
propagated (option: break instead of return and mvneta_setup_rxqs scoped
err variable)
[...]
> +static int mvneta_setup_txqs(struct mvneta_port *pp)
> +{
> + int queue;
> +
> + for (queue = 0; queue < txq_number; queue++) {
> + int err = mvneta_txq_init(pp, &pp->txqs[queue]);
> + if (err) {
> + netdev_err(pp->dev,
> + "%s: can't create TxQ txq=%d\n",
> + __func__, queue);
> + mvneta_cleanup_txqs(pp);
> + return err;
> + }
> + }
See mvneta_setup_rxqs above.
[...]
> +static void mvneta_start_dev(struct mvneta_port *pp)
> +{
> + mvneta_max_rx_size_set(pp, pp->pkt_size);
> + mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
> +
> + /* start the Rx/Tx activity */
> + mvneta_port_enable(pp);
> +
> + /* Enable polling on the port */
> + napi_enable(&pp->napi);
> +
> + /* Unmask interrupts */
> + mvneta_interrupts_unmask(pp);
> + smp_call_function_many(cpu_online_mask,
> + mvneta_interrupts_unmask,
> + pp, 1);
smp_call_function_many(cpu_online_mask, mvneta_interrupts_unmask,
(as done in mvneta_stop_dev)
> +
> + phy_start(pp->phy_dev);
> + netif_tx_start_all_queues(pp->dev);
> +}
> +
> +static void mvneta_stop_dev(struct mvneta_port *pp)
> +{
> + phy_stop(pp->phy_dev);
> +
> + napi_disable(&pp->napi);
> +
> + /* Stop upper layer */
> + netif_carrier_off(pp->dev);
Useless comment.
> +
> + mvneta_port_down(pp);
> + netif_tx_stop_all_queues(pp->dev);
> +
> + /* Stop the port activity */
> + mvneta_port_disable(pp);
> +
> + /* Clear all ethernet port interrupts */
> + mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
> + mvreg_write(pp, MVNETA_INTR_OLD_CAUSE, 0);
> +
> + /* Mask all interrupts */
> + mvneta_interrupts_mask(pp);
Useless comment.
> + smp_call_function_many(cpu_online_mask, mvneta_interrupts_mask,
> + pp, 1);
> +
> + /* Reset TX port here. */
> + mvneta_tx_reset(pp);
> + mvneta_rx_reset(pp);
Useless comment.
> +}
> +
> +/* tx timeout callback - display a message and stop/start the network device */
> +static void mvneta_tx_timeout(struct net_device *dev)
> +{
> + struct mvneta_port *pp = netdev_priv(dev);
> + netdev_info(dev, "tx timeout\n");
Insert an empty line after local variables declaration.
[...]
> +static int mvneta_check_mtu_valid(struct net_device *dev, int mtu)
> +{
> + if (mtu < 68) {
> + netdev_err(dev, "cannot change mtu to less than 68\n");
> + return -EINVAL;
> + }
> +
> + if (mtu > 9676 /* 9700 - 20 and rounding to 8 */) {
Please use a different comment style, say on a proper line, or a define with
a comment above it.
[...]
> +static int mvneta_ethtool_set_coalesce(struct net_device *dev,
> + struct ethtool_coalesce *c)
> +{
> + int queue;
> + struct mvneta_port *pp = netdev_priv(dev);
Long lines first when possible.
[...]
> +static const struct net_device_ops mvneta_netdev_ops = {
> + .ndo_open = mvneta_open,
> + .ndo_stop = mvneta_stop,
> + .ndo_start_xmit = mvneta_tx,
> + .ndo_set_rx_mode = mvneta_set_rx_mode,
> + .ndo_set_mac_address = mvneta_set_mac_addr,
> + .ndo_change_mtu = mvneta_change_mtu,
> + .ndo_tx_timeout = mvneta_tx_timeout,
> + .ndo_get_stats64 = mvneta_get_stats64,
Please use tabs before '=' and line things up.
> +};
> +
> +const struct ethtool_ops mvneta_eth_tool_ops = {
> + .get_link = ethtool_op_get_link,
> + .get_settings = mvneta_ethtool_get_settings,
> + .set_settings = mvneta_ethtool_set_settings,
> + .set_coalesce = mvneta_ethtool_set_coalesce,
> + .get_coalesce = mvneta_ethtool_get_coalesce,
> + .get_drvinfo = mvneta_ethtool_get_drvinfo,
> + .get_ringparam = mvneta_ethtool_get_ringparam,
> + .set_ringparam = mvneta_ethtool_set_ringparam,
Please use tabs before '=' and line things up.
[...]
> +static int __devinit mvneta_init(struct mvneta_port *pp, int phy_addr)
> +{
> + int queue, i, ret = 0;
> +
> + /* Disable port */
> + mvneta_port_disable(pp);
> +
> + /* Set port default values */
> + mvneta_defaults_set(pp);
> +
> + pp->txqs = kzalloc(txq_number * sizeof(struct mvneta_tx_queue),
> + GFP_KERNEL);
> + if (!pp->txqs) {
> + netdev_err(pp->dev, "out of memory in allocating tx queue\n");
I doubt a message is really needed when a GFP_KERNEL oom triggers.
(...]
> +static void __devinit mvneta_conf_mbus_windows(struct mvneta_port *pp,
> + const struct mbus_dram_target_info *dram)
> +{
[...]
> + for (i = 0; i < dram->num_cs; i++) {
> + const struct mbus_dram_window *cs = dram->cs + i;
> + mvreg_write(pp, MVNETA_WIN_BASE(i),
> + (cs->base & 0xffff0000) |
> + (cs->mbus_attr << 8) |
> + dram->mbus_dram_target_id);
mvreg_write(pp, MVNETA_WIN_BASE(i), (cs->base & 0xffff0000) |
(cs->mbus_attr << 8) | dram->mbus_dram_target_id);
[...]
> +static int __devinit mvneta_probe(struct platform_device *pdev)
> +{
> + int err = -EINVAL;
> + struct mvneta_port *pp;
> + struct net_device *dev;
> + u32 phy_addr, clk_rate_hz;
> + int phy_mode;
> + const char *mac_addr;
> + const struct mbus_dram_target_info *dram_target_info;
> + struct device_node *dn = pdev->dev.of_node;
Long lines first when possible.
--
Ueimor
^ permalink raw reply
* [PATCH v3 04/11] clk: davinci - add pll divider clock driver
From: Sekhar Nori @ 2012-11-03 12:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5093D070.7060900@ti.com>
On 11/2/2012 7:23 PM, Murali Karicheri wrote:
> On 11/02/2012 07:33 AM, Sekhar Nori wrote:
>> On 10/25/2012 9:41 PM, Murali Karicheri wrote:
>>
>>> pll dividers are present in the pll controller of DaVinci and Other
>>> SoCs that re-uses the same hardware IP. This has a enable bit for
>>> bypass the divider or enable the driver. This is a sub class of the
>>> clk-divider clock checks the enable bit to calculare the rate and
>>> invoke the recalculate() function of the clk-divider if enabled.
>>>
>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>> ---
>>> +/**
>>> + * clk_register_davinci_plldiv - register function for DaVinci PLL
>>> divider clk
>>> + *
>>> + * @dev: device ptr
>>> + * @name: name of the clock
>>> + * @parent_name: name of parent clock
>>> + * @plldiv_data: ptr to pll divider data
>>> + * @lock: ptr to spinlock passed to divider clock
>>> + */
>>> +struct clk *clk_register_davinci_plldiv(struct device *dev,
>> Why do you need a dev pointer here and which device does it point to? In
>> the only usage of this API in the series, you pass a NULL here. I should
>> have probably asked this question on one of the earlier patches itself.
>>
> I did a grep in the drivers/clk directory. All of the platform drivers
> are having the device ptr and all of them are called with NULL. I am not
> sure what is the intent of this arg in the API. As per documentation of
I just took a look at the mxs example you referenced below and it does
not take a dev pointer.
struct clk *mxs_clk_div(const char *name, const char *parent_name,
void __iomem *reg, u8 shift, u8 width, u8 busy)
{
> the clk_register() API, the device ptr points to the device that is
> registering this clk. So if a specific device driver ever has to
> register a PLL div clk, this will be non NULL. In the normal use case,
> clk is registered in a platform specific code and is always passed NULL.
>
> The platform/SoC specific clock initialization code will be using
> davinci_plldiv_clk() that doesn't have a device ptr arg.
> So this can be changed in future in sync with other drivers (assuming
> this will get removed if unused), and changes
> doesn't impact the platform code that initialize the clock. So IMO, we
> should keep this arg so that it is in sync with other driver APIs.
I think you should get rid of this unused arg and introduce it when you
actually need it. That way we are clear about why we need it.
>
> + const char *name, const char *parent_name,
> + struct clk_plldiv_data *plldiv_data,
> + spinlock_t *lock)
> +{
> + struct clk_div *div;
> + struct clk *clk;
> + struct clk_init_data init;
> +
> + div = kzalloc(sizeof(*div), GFP_KERNEL);
> + if (!div)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = name;
> + init.ops = &clk_div_ops;
> + init.flags = plldiv_data->flags;
> + init.parent_names = (parent_name ? &parent_name : NULL);
> + init.num_parents = (parent_name ? 1 : 0);
> +
> + div->reg = plldiv_data->reg;
> + div->en_id = plldiv_data->en_id;
> +
> + div->divider.reg = plldiv_data->reg;
> + div->divider.shift = plldiv_data->shift;
> + div->divider.width = plldiv_data->width;
> + div->divider.flags = plldiv_data->divider_flags;
> + div->divider.lock = lock;
> + div->divider.hw.init = &init;
> + div->ops = &clk_divider_ops;
> +
> + clk = clk_register(NULL, &div->divider.hw);
>
>> Shouldn't you be calling clk_register_divider() here which in turn will
>> do clk_register()?
> As stated in the top of the file, this is a subclass driver of clk-div
> similar in line with mxs/clk-div.c. The
> driver registers the clock instead of calling clk_register_divider() so
> that it's ops function has a chance to do whatever it wants to do and
> call the divider ops function after that.
I see that now. I should have paid more attention.
Regards,
Sekhar
^ permalink raw reply
* [PATCH v3 02/11] clk: davinci - add PSC clock driver
From: Sekhar Nori @ 2012-11-03 12:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351181518-11882-3-git-send-email-m-karicheri2@ti.com>
On 10/25/2012 9:41 PM, Murali Karicheri wrote:
> This is the driver for the Power Sleep Controller (PSC) hardware
> found on DM SoCs as well Keystone SoCs (c6x). This driver borrowed
> code from arch/arm/mach-davinci/psc.c and implemented the driver
> as per common clock provider API. The PSC module is responsible for
> enabling/disabling the Power Domain and Clock domain for different IPs
> present in the SoC. The driver is configured through the clock data
> passed to the driver through struct clk_psc_data.
>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
> +/**
> + * struct clk_psc - DaVinci PSC clock driver data
> + *
> + * @hw: clk_hw for the psc
> + * @psc_data: Driver specific data
> + */
> +struct clk_psc {
> + struct clk_hw hw;
> + struct clk_psc_data *psc_data;
> + spinlock_t *lock;
Unused member? I don't see this being used.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 12/15] ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
From: Kevin Hilman @ 2012-11-03 12:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351859566-24818-13-git-send-email-vaibhav.bedia@ti.com>
On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
>
> The current OMAP timer code registers two timers -
> one as clocksource and one as clockevent.
> AM33XX has only one usable timer in the WKUP domain
> so one of the timers needs suspend-resume support
> to restore the configuration to pre-suspend state.
The changelog describes "what", but doesn't answer "why?"
> commit adc78e6 (timekeeping: Add suspend and resume
> of clock event devices) introduced .suspend and .resume
> callbacks for clock event devices. Leverages these
> callbacks to have AM33XX clockevent timer which is
> in not in WKUP domain to behave properly across system
> suspend.
You say it behaves properly without describing what improper
behavior is happening.
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
> ---
> arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++
> 1 files changed, 31 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 6584ee0..e8781fd 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
> }
> }
>
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> + char name[10];
> + struct omap_hwmod *oh;
> +
> + sprintf(name, "timer%d", 2);
hard-coded timer ID? the rest of the code is using timer_id
> + oh = omap_hwmod_lookup(name);
> + if (!oh)
> + return;
> +
> + omap_hwmod_idle(oh);
> +}
> +
> +static void omap_clkevt_resume(struct clock_event_device *unused)
> +{
> + char name[10];
> + struct omap_hwmod *oh;
> +
> + sprintf(name, "timer%d", 2);
> + oh = omap_hwmod_lookup(name);
> + if (!oh)
> + return;
> +
> + omap_hwmod_enable(oh);
> + __omap_dm_timer_load_start(&clkev,
> + OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
> + __omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> +}
I don't like the sprintf/hwmod_lookup for every suspend/resume. Just add a
new file-global static 'struct omap_hwmod clockevt_oh' along side
clockevent_gpt,
and assign it at init time in dmtimer_init_one. Then you don't have to
do this
sprintf/lookup on every suspend/resume.
Kevin
> static struct clock_event_device clockevent_gpt = {
> .name = "gp_timer",
> .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> @@ -142,6 +171,8 @@ static struct clock_event_device clockevent_gpt = {
> .rating = 300,
> .set_next_event = omap2_gp_timer_set_next_event,
> .set_mode = omap2_gp_timer_set_mode,
> + .suspend = omap_clkevt_suspend,
> + .resume = omap_clkevt_resume,
> };
>
> static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
>
^ permalink raw reply
* [PATCH 13/15] ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
From: Kevin Hilman @ 2012-11-03 12:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351859566-24818-14-git-send-email-vaibhav.bedia@ti.com>
On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
Even simple patches need a simple changelog.
Kevin
> arch/arm/boot/dts/am33xx.dtsi | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index bb31bff..e2cbf24 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -210,5 +210,16 @@
> interrupt-parent = <&intc>;
> interrupts = <91>;
> };
> +
> + ocmcram: ocmcram at 40300000 {
> + compatible = "ti,ocmcram";
> + ti,hwmods = "ocmcram";
> + ti,no_idle_on_suspend;
> + };
> +
> + wkup_m3: wkup_m3 at 44d00000 {
> + compatible = "ti,wkup_m3";
> + ti,hwmods = "wkup_m3";
> + };
> };
> };
>
^ permalink raw reply
* [PATCH 14/15] ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
From: Kevin Hilman @ 2012-11-03 12:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351859566-24818-15-git-send-email-vaibhav.bedia@ti.com>
On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> AM33XX PM code depends on Mailbox module for IPC
> between MPU and WKUP_M3.
OK, but what if I try to build for AM33xx without starting from
omap2plus_defconfig?
IOW, instead of changing the default defconfig, AM33xx should select
this when PM
is enabled. Something like the (untested) change below.
Kevin
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index d669e22..93c1a37 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -109,6 +109,7 @@ config SOC_AM33XX
bool "AM33XX support"
default y
select ARM_CPU_SUSPEND if PM
+ select OMAP_MBOX_FWK if PM
select CPU_V7
select MULTI_IRQ_HANDLER
^ permalink raw reply related
* [PATCH v3 03/11] clk: davinci - common clk utilities to init clk driver
From: Sekhar Nori @ 2012-11-03 12:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351181518-11882-4-git-send-email-m-karicheri2@ti.com>
On 10/25/2012 9:41 PM, Murali Karicheri wrote:
> This is the common clk driver initialization functions for DaVinci
> SoCs and other SoCs that uses similar hardware architecture.
> clock.h also defines struct types for clock definitions in a SoC
> and clock data type for configuring clk-mux. The initialization
> functions are used by clock initialization code in a specific
> platform/SoC.
>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
> drivers/clk/davinci/clock.c | 112 +++++++++++++++++++++++++++++++++++++++++++
> drivers/clk/davinci/clock.h | 80 +++++++++++++++++++++++++++++++
> 2 files changed, 192 insertions(+)
> create mode 100644 drivers/clk/davinci/clock.c
> create mode 100644 drivers/clk/davinci/clock.h
>
> diff --git a/drivers/clk/davinci/clock.c b/drivers/clk/davinci/clock.c
> new file mode 100644
> index 0000000..ad02149
> --- /dev/null
> +++ b/drivers/clk/davinci/clock.c
> @@ -0,0 +1,112 @@
> +/*
> + * clock.c - davinci clock initialization functions for various clocks
> + *
> + * Copyright (C) 2006-2012 Texas Instruments.
> + * Copyright (C) 2008-2009 Deep Root Systems, LLC
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/init.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +
> +#include "clk-pll.h"
> +#include "clk-psc.h"
> +#include "clk-div.h"
> +#include "clock.h"
> +
> +static DEFINE_SPINLOCK(_lock);
> +
> +#ifdef CONFIG_CLK_DAVINCI_PLL
> +struct clk *davinci_pll_clk(const char *name, const char *parent,
> + u32 phys_pllm, u32 phys_prediv, u32 phys_postdiv,
> + struct clk_pll_data *pll_data)
> +{
> + struct clk *clkp = NULL;
> +
> + pll_data->reg_pllm = ioremap(phys_pllm, 4);
> + if (WARN_ON(!pll_data->reg_pllm))
> + return clkp;
I would prefer ERR_PTR(-ENOMEM) here. Same comment applies to other
instances elsewhere in the patch.
> diff --git a/drivers/clk/davinci/clock.h b/drivers/clk/davinci/clock.h
> new file mode 100644
> index 0000000..73204b8
> --- /dev/null
> +++ b/drivers/clk/davinci/clock.h
> @@ -0,0 +1,80 @@
> +/*
> + * TI DaVinci Clock definitions - Contains Macros and Types used for
> + * defining various clocks on a DaVinci SoC
> + *
> + * Copyright (C) 2012 Texas Instruments
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __DAVINCI_CLOCK_H
> +#define __DAVINCI_CLOCK_H
> +
> +#include <linux/types.h>
> +
> +/* general flags: */
> +#define ALWAYS_ENABLED BIT(0)
This is not used in this patch. Can you add the define along with its
usage so it is immediately clear why you need it?
> +/**
> + * struct davinci_clk - struct for defining DaVinci clocks for a SoC.
> + *
> + * @name: name of the clock
> + * @parent: name of parent clock
> + * @flags: General flags for all drivers used by platform clock init code
> + * @data: data specific to a clock used by the driver
> + * @dev_id: dev_id used to look up this clock. If this is NULL
> + * clock name is used for lookup.
> + */
> +struct davinci_clk {
> + const char *name;
> + const char *parent;
> + u32 flags;
> + void *data;
> + char *dev_id;
Similarly dont see this being used as well.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 11/15] ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
From: Bedia, Vaibhav @ 2012-11-03 12:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5095037A.2020503@deeprootsystems.com>
Hi Kevin,
On Sat, Nov 03, 2012 at 17:13:54, Kevin Hilman wrote:
> On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> > AM33XX has only one usable timer in the WKUP domain.
> > Currently the timer instance in WKUP domain is used
> > as the clockevent and the timer in non-WKUP domain
> > as the clocksource. The timer in WKUP domain can keep
> > running in suspend from a 32K clock and hence serve
> > as the persistent clock. To enable this, interchange
> > the timers used as clocksource and clockevent for
> > AM33XX.
>
> Doesn't this also mean that you won't get timer wakeups
> in idle? Or are you keeping the domain where the clockevent is
> on during idle?
>
The lowest idle state that we are targeting will have MPU powered
off with external memory in self-refresh mode. Peripheral domain
with the clockevent will be kept on.
Regards,
Vaibhav
^ permalink raw reply
* Possible regression in 3.7-rc3 on Marvell Kirkwood
From: Josh Coombs @ 2012-11-03 12:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121103080300.GA9907@lunn.ch>
I'll fire up with ignore log level to get the full traces and post
those this afternoon. Given it will be a LOT of text, would you
prefer I include them in the email, pastebin the output and provide
links, or dump the log onto my webserver with a link?
Josh C
On Sat, Nov 3, 2012 at 4:03 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Josh
>
> Are you not seeing a stack trace?
>
> [ 64.312377] BUG: scheduling while atomic: crond/151/0x40000300
> [ 79.771862] BUG: scheduling while atomic: swapper/0/0x40000500
> [ 81.826267] BUG: scheduling while atomic: swapper/0/0x40000500
> [ 90.330911] BUG: scheduling while atomic: swapper/0/0x40000500
>
> printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
> prev->comm, prev->pid, preempt_count());
>
> debug_show_held_locks(prev);
> print_modules();
> if (irqs_disabled())
> print_irqtrace_events(prev);
> dump_stack();
> add_taint(TAINT_WARN);
>
> Andrew
^ permalink raw reply
* [PATCH 11/15] ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
From: Kevin Hilman @ 2012-11-03 13:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <B5906170F1614E41A8A28DE3B8D121433EBFEA0E@DBDE01.ent.ti.com>
T
On 11/03/2012 12:47 PM, Bedia, Vaibhav wrote:
> Hi Kevin,
>
> On Sat, Nov 03, 2012 at 17:13:54, Kevin Hilman wrote:
>> On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
>>> AM33XX has only one usable timer in the WKUP domain.
>>> Currently the timer instance in WKUP domain is used
>>> as the clockevent and the timer in non-WKUP domain
>>> as the clocksource. The timer in WKUP domain can keep
>>> running in suspend from a 32K clock and hence serve
>>> as the persistent clock. To enable this, interchange
>>> the timers used as clocksource and clockevent for
>>> AM33XX.
>>
>> Doesn't this also mean that you won't get timer wakeups
>> in idle? Or are you keeping the domain where the clockevent is
>> on during idle?
>>
>
> The lowest idle state that we are targeting will have MPU powered
> off with external memory in self-refresh mode. Peripheral domain
> with the clockevent will be kept on.
Is this a limitation of the hardware? or the software?
Kevin
P.S. I haven't yet made my way through the TRM, so I'll probably figure
this out when I read it, but having this summarized in the changelog would
be helpful since even after I read the TRM, I'll forget. I'm saving the
TRM
reading for entertainment on upcoming flight home from Linaro Connect.
^ permalink raw reply
* [PATCH 12/15] ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
From: Bedia, Vaibhav @ 2012-11-03 13:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50950AC7.1030707@deeprootsystems.com>
On Sat, Nov 03, 2012 at 17:45:03, Kevin Hilman wrote:
> On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> > From: Vaibhav Hiremath <hvaibhav@ti.com>
> >
> > The current OMAP timer code registers two timers -
> > one as clocksource and one as clockevent.
> > AM33XX has only one usable timer in the WKUP domain
> > so one of the timers needs suspend-resume support
> > to restore the configuration to pre-suspend state.
>
> The changelog describes "what", but doesn't answer "why?"
>
Sorry I'll try to take of this in the future.
> > commit adc78e6 (timekeeping: Add suspend and resume
> > of clock event devices) introduced .suspend and .resume
> > callbacks for clock event devices. Leverages these
> > callbacks to have AM33XX clockevent timer which is
> > in not in WKUP domain to behave properly across system
> > suspend.
>
> You say it behaves properly without describing what improper
> behavior is happening.
>
There are two issues. One is that the clockevent timer doesn't
get idled which blocks PER domain transition. The next one is that
the clockevent doesn't generate any further interrupts once the
system resumes. We need to restore the pre-suspend configuration.
I haven't tried but I guess we could have used the save and restore
of timer registers here.
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
> > ---
> > arch/arm/mach-omap2/timer.c | 31 +++++++++++++++++++++++++++++++
> > 1 files changed, 31 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> > index 6584ee0..e8781fd 100644
> > --- a/arch/arm/mach-omap2/timer.c
> > +++ b/arch/arm/mach-omap2/timer.c
> > @@ -135,6 +135,35 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
> > }
> > }
> >
> > +static void omap_clkevt_suspend(struct clock_event_device *unused)
> > +{
> > + char name[10];
> > + struct omap_hwmod *oh;
> > +
> > + sprintf(name, "timer%d", 2);
>
> hard-coded timer ID? the rest of the code is using timer_id
Will fix.
>
> > + oh = omap_hwmod_lookup(name);
> > + if (!oh)
> > + return;
> > +
> > + omap_hwmod_idle(oh);
> > +}
> > +
> > +static void omap_clkevt_resume(struct clock_event_device *unused)
> > +{
> > + char name[10];
> > + struct omap_hwmod *oh;
> > +
> > + sprintf(name, "timer%d", 2);
> > + oh = omap_hwmod_lookup(name);
> > + if (!oh)
> > + return;
> > +
> > + omap_hwmod_enable(oh);
> > + __omap_dm_timer_load_start(&clkev,
> > + OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
> > + __omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> > +}
>
> I don't like the sprintf/hwmod_lookup for every suspend/resume. Just add a
> new file-global static 'struct omap_hwmod clockevt_oh' along side
> clockevent_gpt,
> and assign it at init time in dmtimer_init_one. Then you don't have to
> do this
> sprintf/lookup on every suspend/resume.
>
Ok. Will make the changes accordingly.
Regards,
Vaibhav
^ permalink raw reply
* [PATCH 13/15] ARM: DTS: AM33XX: Add nodes for OCMCRAM and Mailbox
From: Bedia, Vaibhav @ 2012-11-03 13:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50950B06.8010800@deeprootsystems.com>
On Sat, Nov 03, 2012 at 17:46:06, Kevin Hilman wrote:
> On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> > Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
>
> Even simple patches need a simple changelog.
Again, sorry about this. Will take care in the future.
Regards,
Vaibhav
^ permalink raw reply
* [PATCH 14/15] ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
From: Bedia, Vaibhav @ 2012-11-03 13:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50950C09.6000108@deeprootsystems.com>
On Sat, Nov 03, 2012 at 17:50:25, Kevin Hilman wrote:
> On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
> > AM33XX PM code depends on Mailbox module for IPC
> > between MPU and WKUP_M3.
>
> OK, but what if I try to build for AM33xx without starting from
> omap2plus_defconfig?
I honestly didn't think about this scenario.
>
> IOW, instead of changing the default defconfig, AM33xx should select
> this when PM
> is enabled. Something like the (untested) change below.
>
Will try it out. Thanks for the suggestion.
Regards,
Vaibhav
^ permalink raw reply
* [PATCH v3 05/11] clk: davinci - add dm644x clock initialization
From: Sekhar Nori @ 2012-11-03 13:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1351181518-11882-6-git-send-email-m-karicheri2@ti.com>
On 10/25/2012 9:41 PM, Murali Karicheri wrote:
> This patch adds dm644x clock initialization code that consists of
> clocks data for various clocks and clock register callouts to
> various clock drivers. It uses following clk drivers for this
>
> 1. clk-fixed-rate - for ref clock
> 2. clk-mux - for mux at the input and output of main pll
> 3. davinci specific clk-pll for main pll clock
> 4. davinci specific clk-div for pll divider clock
> 5. clk-fixed-factor for fixed factor clock such as auxclk
> 6. davinci specific clk-psc for psc clocks
>
> This patch also moves all of the PLL and PSC register definitions
> from clock.h and psc.h under davinci to the clk/davinci folder so
> that various soc specific clock initialization code can share these
> definitions.
Except this patch does not move the defines, it creates a copy of them
(which is bad since you quickly lose track of which is the correct
copy). Is this done to avoid including mach/ header files here? It will
actually be better to include the mach/ files here as a temporary
solution and then remove the include mach/ files once all the SoCs have
been converted over.
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
> drivers/clk/davinci/dm644x-clock.c | 304 ++++++++++++++++++++++++++++++++++++
> drivers/clk/davinci/pll.h | 83 ++++++++++
> drivers/clk/davinci/psc.h | 215 +++++++++++++++++++++++++
> 3 files changed, 602 insertions(+)
> create mode 100644 drivers/clk/davinci/dm644x-clock.c
> create mode 100644 drivers/clk/davinci/pll.h
> create mode 100644 drivers/clk/davinci/psc.h
>
> +/* all clocks available in DM644x SoCs */
> +enum dm644x_clk {
> + clkin, oscin, ref_clk_mux, pll1, pll1_plldiv_clk_mux, auxclk,
> + clk_pll1_sysclk1, clk_pll1_sysclk2, clk_pll1_sysclk3, clk_pll1_sysclk4,
> + clk_pll1_sysclk5, clk_pll1_sysclkbp, pll2, pll2_plldiv_clk_mux,
> + clk_pll2_sysclk1, clk_pll2_sysclk2, clk_pll2_sysclkbp, dsp, arm, vicp,
> + vpss_master, vpss_slave, uart0, uart1, uart2, emac, i2c, ide, asp,
> + mmcsd, spi, gpio, usb, vlynq, aemif, pwm0, pwm1, pwm2, timer0, timer1,
> + timer2, clk_max
> +};
> +
> +static struct davinci_clk *psc_clocks[] = {
> + &clk_dsp, &clk_arm, &clk_vicp, &clk_vpss_master, &clk_vpss_slave,
> + &clk_uart0, &clk_uart1, &clk_uart2, &clk_emac, &clk_i2c, &clk_ide,
> + &clk_asp0, &clk_mmcsd, &clk_spi, &clk_gpio, &clk_usb, &clk_vlynq,
> + &clk_aemif, &clk_pwm0, &clk_pwm1, &clk_pwm2, &clk_timer0, &clk_timer1,
> + &clk_timer2
> +};
You rely on perfect order between this array and dm644x_clk enum above.
Can you initialize this array using the enum as the index so that it is
clear. Current method is too error prone.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 12/15] ARM: OMAP: timer: Add suspend-resume callbacks for clockevent device
From: Kevin Hilman @ 2012-11-03 13:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <B5906170F1614E41A8A28DE3B8D121433EBFEA88@DBDE01.ent.ti.com>
On 11/03/2012 01:17 PM, Bedia, Vaibhav wrote:
> On Sat, Nov 03, 2012 at 17:45:03, Kevin Hilman wrote:
>> On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
>>> From: Vaibhav Hiremath <hvaibhav@ti.com>
>>>
>>> The current OMAP timer code registers two timers -
>>> one as clocksource and one as clockevent.
>>> AM33XX has only one usable timer in the WKUP domain
>>> so one of the timers needs suspend-resume support
>>> to restore the configuration to pre-suspend state.
>>
>> The changelog describes "what", but doesn't answer "why?"
>>
>
> Sorry I'll try to take of this in the future.
Thanks. Here's a general rule. Assume you (or I) will be reading this
a year from now and will have forgotten the details. The changelog then
serves as our long-term memory. :)
>>> commit adc78e6 (timekeeping: Add suspend and resume
>>> of clock event devices) introduced .suspend and .resume
>>> callbacks for clock event devices. Leverages these
>>> callbacks to have AM33XX clockevent timer which is
>>> in not in WKUP domain to behave properly across system
>>> suspend.
>>
>> You say it behaves properly without describing what improper
>> behavior is happening.
>>
>
> There are two issues. One is that the clockevent timer doesn't
> get idled which blocks PER domain transition. The next one is that
> the clockevent doesn't generate any further interrupts once the
> system resumes.
Please include both in the next rev of the changelog.
>We need to restore the pre-suspend configuration.
> I haven't tried but I guess we could have used the save and restore
> c timer registers here.
Yes, please try with that. Won't that be necessary anyways for situations
where the powerdomain goes off?
Kevin
^ permalink raw reply
* [PATCH 14/15] ARM: OMAP2+: omap2plus_defconfig: Enable Mailbox
From: Kevin Hilman @ 2012-11-03 13:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <B5906170F1614E41A8A28DE3B8D121433EBFEA96@DBDE01.ent.ti.com>
On 11/03/2012 01:17 PM, Bedia, Vaibhav wrote:
> On Sat, Nov 03, 2012 at 17:50:25, Kevin Hilman wrote:
>> On 11/02/2012 01:32 PM, Vaibhav Bedia wrote:
>>> AM33XX PM code depends on Mailbox module for IPC
>>> between MPU and WKUP_M3.
>>
>> OK, but what if I try to build for AM33xx without starting from
>> omap2plus_defconfig?
>
> I honestly didn't think about this scenario.
>
>>
>> IOW, instead of changing the default defconfig, AM33xx should select
>> this when PM
>> is enabled. Something like the (untested) change below.
>>
>
> Will try it out. Thanks for the suggestion.
You're welcome.
See... sometimes maintainers can be useful for something other than
complaining. ;)
Kevin
^ permalink raw reply
* [PATCH 11/15] ARM: OMAP: timer: Interchange clksrc and clkevt for AM33XX
From: Bedia, Vaibhav @ 2012-11-03 13:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5095165E.10403@deeprootsystems.com>
On Sat, Nov 03, 2012 at 18:34:30, Kevin Hilman wrote:
[...]
> >>
> >> Doesn't this also mean that you won't get timer wakeups
> >> in idle? Or are you keeping the domain where the clockevent is
> >> on during idle?
> >>
> >
> > The lowest idle state that we are targeting will have MPU powered
> > off with external memory in self-refresh mode. Peripheral domain
> > with the clockevent will be kept on.
>
> Is this a limitation of the hardware? or the software?
>
Well, making the lowest idle state same as the suspend state will require
us to involve WKUP_M3 in the idle path and wakeup sources get limited to
the IPs in the WKUP domain alone. There's no IO daisy chaining in AM33XX
so that's one big difference compared to OMAP.
The other potential problem is that the IPC mechanism that we have
uses interrupts. Assuming that the lowest idle state, say Cx, is the same
as the suspend state, we'll need to communicate with the WKUP_M3 using
interrupts once we decide to enter Cx. I am not sure if we can do something
in the cpuidle implementation to work around the "interrupt for idle"
problem. We could probably not wait for an ACK when we want to enter Cx,
but the problem of limited wakeup sources remains. If we let the various
drivers block the entry to Cx, since almost all the IPs are in the
peripheral domain a system which uses anything other than UART and Timer
in WKUP domain will probably never be able enter Cx.
Regards,
Vaibhav
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox