[PATCH v3] spi: spi-sun6i: implement DMA-based transfer mode

[Alexander Kochetkov]

From: Alexander Kochetkov 

DMA-based transfer will be enabled if data length is larger than FIFO size
(64 bytes for A64). This greatly reduce number of interrupts for
transferring data.

For smaller data size PIO mode will be used. In PIO mode whole buffer will
be loaded into FIFO.

If driver failed to request DMA channels then it fallback for PIO mode.

Tested on SOPINE (https://www.pine64.org/sopine/)

Signed-off-by: Alexander Kochetkov 
---
Changes in v3:
- Remove use_dma from struct sun6i_spi

Changes in v2:

- Fix 'checkpatch.pl --strict' warnings
- Optimezed DMA transfers. Fix burst size and address width for
 DMA transfers. The code works for me an I did some tests with different
 values and conditions. Empirically found that trigger level used for
 PIO mode also can be used for DMA mode (TRM for A64 lacks that 
 description)
- Handling inside sun6i_spi_handler() leaved as is, it explicity states that
 sun6i_spi_drain_fifo() is not used in DMA mode. Yes, if we call
 sun6i_spi_drain_fifo() after DMA transfer, it will not drain anything
 becase DMA already drain FIFO.
 I can remove condition if it's better without it.

 drivers/spi/spi-sun6i.c | 194 
 1 file changed, 176 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 19238e1b76b4..5336c5e32694 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -52,10 +53,12 @@
 
 #define SUN6I_FIFO_CTL_REG 0x18
 #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK  0xff
+#define SUN6I_FIFO_CTL_RF_DRQ_EN   BIT(8)
 #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS  0
 #define SUN6I_FIFO_CTL_RF_RST  BIT(15)
 #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK  0xff
 #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS  16
+#define SUN6I_FIFO_CTL_TF_DRQ_EN   BIT(24)
 #define SUN6I_FIFO_CTL_TF_RST  BIT(31)
 
 #define SUN6I_FIFO_STA_REG 0x1c
@@ -83,6 +86,8 @@
 struct sun6i_spi {
struct spi_master   *master;
void __iomem*base_addr;
+   dma_addr_t  dma_addr_rx;
+   dma_addr_t  dma_addr_tx;
struct clk  *hclk;
struct clk  *mclk;
struct reset_control*rstc;
@@ -182,6 +187,68 @@ static size_t sun6i_spi_max_transfer_size(struct 
spi_device *spi)
return SUN6I_MAX_XFER_SIZE - 1;
 }
 
+static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
+struct spi_transfer *tfr)
+{
+   struct dma_async_tx_descriptor *rxdesc, *txdesc;
+   struct spi_master *master = sspi->master;
+
+   rxdesc = NULL;
+   if (tfr->rx_buf) {
+   struct dma_slave_config rxconf = {
+   .direction = DMA_DEV_TO_MEM,
+   .src_addr = sspi->dma_addr_rx,
+   .src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .src_maxburst = 8,
+   };
+
+   dmaengine_slave_config(master->dma_rx, );
+
+   rxdesc = dmaengine_prep_slave_sg(master->dma_rx,
+tfr->rx_sg.sgl,
+tfr->rx_sg.nents,
+DMA_DEV_TO_MEM,
+DMA_PREP_INTERRUPT);
+   if (!rxdesc)
+   return -EINVAL;
+   }
+
+   txdesc = NULL;
+   if (tfr->tx_buf) {
+   struct dma_slave_config txconf = {
+   .direction = DMA_MEM_TO_DEV,
+   .dst_addr = sspi->dma_addr_tx,
+   .dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .dst_maxburst = 8,
+   };
+
+   dmaengine_slave_config(master->dma_tx, );
+
+   txdesc = dmaengine_prep_slave_sg(master->dma_tx,
+tfr->tx_sg.sgl,
+tfr->tx_sg.nents,
+DMA_MEM_TO_DEV,
+DMA_PREP_INTERRUPT);
+   if (!txdesc) {
+   if (rxdesc)
+   dmaengine_terminate_sync(master->dma_rx);
+   return -EINVAL;
+   }
+   }
+
+   if (tfr->rx_buf) {
+   dmaengine_submit(rxdesc);
+   dma_async_issue_pending(master->dma_rx);
+   }
+
+   if (tfr->tx_buf) {
+   dmaengine_submit(txdesc);
+   dma_async_issue_pending(master->dma_tx);
+   }
+
+   return 0;
+}
+
 static int sun6i_spi_transfer_one(struct spi_master *master,
  struct spi_device *spi,
   

Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode

[Alexander Kochetkov]

> 19 окт. 2020 г., в 11:21, Maxime Ripard  написал(а):
> 
> Hi!
> 
> On Thu, Oct 15, 2020 at 06:47:40PM +0300, Alexander Kochetkov wrote:
>> DMA-based transfer will be enabled if data length is larger than FIFO size
>> (64 bytes for A64). This greatly reduce number of interrupts for
>> transferring data.
>> 
>> For smaller data size PIO mode will be used. In PIO mode whole buffer will
>> be loaded into FIFO.
>> 
>> If driver failed to request DMA channels then it fallback for PIO mode.
>> 
>> Tested on SOPINE (https://www.pine64.org/sopine/)
>> 
>> Signed-off-by: Alexander Kochetkov 
> 
> Thanks for working on this, it's been a bit overdue

Hi, Maxime!

We did custom A64 based computation module for our product.
Do you mean that A64 is obsolete or EOL product?
If so, can you recommend active replacement for A64 from Allwinner same price?

Alexander

[PATCH v2] spi: spi-sun6i: enable autosuspend feature

[Alexander Kochetkov]

From: Alexander Kochetkov 

If SPI is used for periodic polling any sensor, significant delays
sometimes appear. Switching on module clocks during resume lead to delays.
Enabling autosuspend mode causes the controller to not suspend between
SPI transfers and the delays disappear.

The commit also remove unnecessary call to pm_runtime_idle() used
to explicit put device to suspended state. Without pm_runtime_idle() PM
core will put device in the suspended state just after probe() returns.

Signed-off-by: Alexander Kochetkov 
---

Changes in v2:
- Extend commit description with explanation about removal
  of pm_runtime_idle()


 drivers/spi/spi-sun6i.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 29ea1e87ce7e..86f29c3e335a 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -22,6 +22,8 @@
 
 #include 
 
+#define SUN6I_AUTOSUSPEND_TIMEOUT  2000
+
 #define SUN6I_FIFO_DEPTH   128
 #define SUN8I_FIFO_DEPTH   64
 
@@ -652,9 +654,10 @@ static int sun6i_spi_probe(struct platform_device *pdev)
goto err_free_dma_rx;
}
 
+   pm_runtime_set_autosuspend_delay(>dev, SUN6I_AUTOSUSPEND_TIMEOUT);
+   pm_runtime_use_autosuspend(>dev);
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
-   pm_runtime_idle(>dev);
 
ret = devm_spi_register_master(>dev, master);
if (ret) {
-- 
2.17.1

[PATCH v2] spi: spi-sun6i: implement DMA-based transfer mode

[Alexander Kochetkov]

From: Alexander Kochetkov 

DMA-based transfer will be enabled if data length is larger than FIFO size
(64 bytes for A64). This greatly reduce number of interrupts for
transferring data.

For smaller data size PIO mode will be used. In PIO mode whole buffer will
be loaded into FIFO.

If driver failed to request DMA channels then it fallback for PIO mode.

Tested on SOPINE (https://www.pine64.org/sopine/)

Signed-off-by: Alexander Kochetkov 
---

Changes in v2:

- Fix 'checkpatch.pl --strict' warnings
- Optimezed DMA transfers. Fix burst size and address width for
  DMA transfers. The code works for me an I did some tests with different
  values and conditions. Empirically found that trigger level used for
  PIO mode also can be used for DMA mode (TRM for A64 lacks that 
  description)
- Handling inside sun6i_spi_handler() leaved as is, it explicity states that
  sun6i_spi_drain_fifo() is not used in DMA mode. Yes, if we call
  sun6i_spi_drain_fifo() after DMA transfer, it will not drain anything
  becase DMA already drain FIFO.
  I can remove condition if it's better without it.


 drivers/spi/spi-sun6i.c | 198 
 1 file changed, 179 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 19238e1b76b4..29ea1e87ce7e 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -52,10 +53,12 @@
 
 #define SUN6I_FIFO_CTL_REG 0x18
 #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK  0xff
+#define SUN6I_FIFO_CTL_RF_DRQ_EN   BIT(8)
 #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS  0
 #define SUN6I_FIFO_CTL_RF_RST  BIT(15)
 #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK  0xff
 #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS  16
+#define SUN6I_FIFO_CTL_TF_DRQ_EN   BIT(24)
 #define SUN6I_FIFO_CTL_TF_RST  BIT(31)
 
 #define SUN6I_FIFO_STA_REG 0x1c
@@ -83,6 +86,8 @@
 struct sun6i_spi {
struct spi_master   *master;
void __iomem*base_addr;
+   dma_addr_t  dma_addr_rx;
+   dma_addr_t  dma_addr_tx;
struct clk  *hclk;
struct clk  *mclk;
struct reset_control*rstc;
@@ -92,6 +97,7 @@ struct sun6i_spi {
const u8*tx_buf;
u8  *rx_buf;
int len;
+   booluse_dma;
unsigned long   fifo_depth;
 };
 
@@ -182,6 +188,68 @@ static size_t sun6i_spi_max_transfer_size(struct 
spi_device *spi)
return SUN6I_MAX_XFER_SIZE - 1;
 }
 
+static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
+struct spi_transfer *tfr)
+{
+   struct dma_async_tx_descriptor *rxdesc, *txdesc;
+   struct spi_master *master = sspi->master;
+
+   rxdesc = NULL;
+   if (tfr->rx_buf) {
+   struct dma_slave_config rxconf = {
+   .direction = DMA_DEV_TO_MEM,
+   .src_addr = sspi->dma_addr_rx,
+   .src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .src_maxburst = 8,
+   };
+
+   dmaengine_slave_config(master->dma_rx, );
+
+   rxdesc = dmaengine_prep_slave_sg(master->dma_rx,
+tfr->rx_sg.sgl,
+tfr->rx_sg.nents,
+DMA_DEV_TO_MEM,
+DMA_PREP_INTERRUPT);
+   if (!rxdesc)
+   return -EINVAL;
+   }
+
+   txdesc = NULL;
+   if (tfr->tx_buf) {
+   struct dma_slave_config txconf = {
+   .direction = DMA_MEM_TO_DEV,
+   .dst_addr = sspi->dma_addr_tx,
+   .dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+   .dst_maxburst = 8,
+   };
+
+   dmaengine_slave_config(master->dma_tx, );
+
+   txdesc = dmaengine_prep_slave_sg(master->dma_tx,
+tfr->tx_sg.sgl,
+tfr->tx_sg.nents,
+DMA_MEM_TO_DEV,
+DMA_PREP_INTERRUPT);
+   if (!txdesc) {
+   if (rxdesc)
+   dmaengine_terminate_sync(master->dma_rx);
+   return -EINVAL;
+   }
+   }
+
+   if (tfr->rx_buf) {
+   dmaengine_submit(rxdesc);
+   dma_async_issue_pending(master->dma_rx);
+   }
+
+   if (tfr->tx_buf) {
+   dmaengine_submit(txdesc);
+   dma_async_issue_

Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode

[Alexander Kochetkov]

Hi, Maxime! Thanks for reviewing patches!


>> 
>> +static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
>> + struct spi_transfer *tfr)
>> +{
>> +struct dma_async_tx_descriptor *rxdesc, *txdesc;
>> +struct spi_master *master = sspi->master;
>> +
>> +rxdesc = NULL;
>> +if (tfr->rx_buf) {
>> +struct dma_slave_config rxconf = {
>> +.direction = DMA_DEV_TO_MEM,
>> +.src_addr = sspi->dma_addr_rx,
>> +.src_addr_width = 1,
>> +.src_maxburst = 1,
>> +};
> 
> That doesn't really look optimal, the controller seems to be able to
> read / write 32 bits at a time from its FIFO and we probably can
> increase the burst length too?


I had doubts if it would work. I didn’t know will DMA work for transfers with 
lengths not
aligned to 32 bits. For example, if we init DMA with src_addr_width = 1 and
.src_maxburst = 8 will DMA work for transfer with length 11? I expect that DMA 
fill FIFO
with 16 bytes and SPI transfer only 11 bytes and 5 bytes will leave in TX fifo. 
 I did the test
and there is no additional data left in the fifo buffer. Also at reception the 
is no memory
overwrites.

I made test with src_addr_width = 4, src_maxburst = 1 and transfer length 3. 
Looks
like in that case DMA doesn’t issue 4 bytes transfer.

For test with src_addr_width = 4, src_maxburst = 8 I had to adjust 
RF_RDY_TRIG_LEVEL_BITS
and TF_ERQ_TRIG_LEVEL_BITS of FIFO_CTL_REG to half of FIFO (32 bytes). With the 
config
DMA will transfer burst of half of FIFO length during transfer and remaining 
bytes at the end of
transfer.


>> 
>> @@ -343,7 +436,8 @@ static irqreturn_t sun6i_spi_handler(int irq, void 
>> *dev_id)
>>  /* Transfer complete */
>>  if (status & SUN6I_INT_CTL_TC) {
>>  sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC);
>> -sun6i_spi_drain_fifo(sspi);
>> +if (!sspi->use_dma)
>> +sun6i_spi_drain_fifo(sspi);
> 
> Is it causing any issue? The FIFO will be drained only if there's
> something remaining in the FIFO, which shouldn't happen with DMA?
> 

No. It’s for make code patch explicit.
Remove the change?

Alexander.

[PATCH] spi: rockchip: enable autosuspend feature

[Alexander Kochetkov]

If SPI is used for periodic polling any sensor, significant delays
sometimes appear. Switching on module clocks during resume lead to delays.
Enabling autosuspend mode causes the controller to not suspend between
SPI transfers and the delays disappear.

Signed-off-by: Alexander Kochetkov 
---
 drivers/spi/spi-rockchip.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 2cc6d9951b52..3e77b1a79bc8 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -157,6 +157,8 @@
 
 #define ROCKCHIP_SPI_MAX_CS_NUM2
 
+#define ROCKCHIP_AUTOSUSPEND_TIMEOUT   2000
+
 struct rockchip_spi {
struct device *dev;
 
@@ -670,6 +672,8 @@ static int rockchip_spi_probe(struct platform_device *pdev)
goto err_disable_spiclk;
}
 
+   pm_runtime_set_autosuspend_delay(>dev, 
ROCKCHIP_AUTOSUSPEND_TIMEOUT);
+   pm_runtime_use_autosuspend(>dev);
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
 
-- 
2.17.1

[PATCH] spi: spi-sun6i: enable autosuspend feature

[Alexander Kochetkov]

If SPI is used for periodic polling any sensor, significant delays
sometimes appear. Switching on module clocks during resume lead to delays.
Enabling autosuspend mode causes the controller to not suspend between
SPI transfers and the delays disappear.

Signed-off-by: Alexander Kochetkov 
---
 drivers/spi/spi-sun6i.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 38e5b8af7da6..4cc0280e934c 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -22,6 +22,8 @@
 
 #include 
 
+#define SUN6I_AUTOSUSPEND_TIMEOUT  2000
+
 #define SUN6I_FIFO_DEPTH   128
 #define SUN8I_FIFO_DEPTH   64
 
@@ -639,9 +641,10 @@ static int sun6i_spi_probe(struct platform_device *pdev)
goto err_free_dma_rx;
}
 
+   pm_runtime_set_autosuspend_delay(>dev, SUN6I_AUTOSUSPEND_TIMEOUT);
+   pm_runtime_use_autosuspend(>dev);
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
-   pm_runtime_idle(>dev);
 
ret = devm_spi_register_master(>dev, master);
if (ret) {
-- 
2.17.1

[PATCH] spi: spi-sun6i: implement DMA-based transfer mode

[Alexander Kochetkov]

DMA-based transfer will be enabled if data length is larger than FIFO size
(64 bytes for A64). This greatly reduce number of interrupts for
transferring data.

For smaller data size PIO mode will be used. In PIO mode whole buffer will
be loaded into FIFO.

If driver failed to request DMA channels then it fallback for PIO mode.

Tested on SOPINE (https://www.pine64.org/sopine/)

Signed-off-by: Alexander Kochetkov 
---
 drivers/spi/spi-sun6i.c | 171 +---
 1 file changed, 159 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 19238e1b76b4..38e5b8af7da6 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -52,10 +53,12 @@
 
 #define SUN6I_FIFO_CTL_REG 0x18
 #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_MASK  0xff
+#define SUN6I_FIFO_CTL_RF_DRQ_EN   BIT(8)
 #define SUN6I_FIFO_CTL_RF_RDY_TRIG_LEVEL_BITS  0
 #define SUN6I_FIFO_CTL_RF_RST  BIT(15)
 #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_MASK  0xff
 #define SUN6I_FIFO_CTL_TF_ERQ_TRIG_LEVEL_BITS  16
+#define SUN6I_FIFO_CTL_TF_DRQ_EN   BIT(24)
 #define SUN6I_FIFO_CTL_TF_RST  BIT(31)
 
 #define SUN6I_FIFO_STA_REG 0x1c
@@ -83,6 +86,8 @@
 struct sun6i_spi {
struct spi_master   *master;
void __iomem*base_addr;
+   dma_addr_t  dma_addr_rx;
+   dma_addr_t  dma_addr_tx;
struct clk  *hclk;
struct clk  *mclk;
struct reset_control*rstc;
@@ -92,6 +97,7 @@ struct sun6i_spi {
const u8*tx_buf;
u8  *rx_buf;
int len;
+   booluse_dma;
unsigned long   fifo_depth;
 };
 
@@ -182,6 +188,66 @@ static size_t sun6i_spi_max_transfer_size(struct 
spi_device *spi)
return SUN6I_MAX_XFER_SIZE - 1;
 }
 
+static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
+struct spi_transfer *tfr)
+{
+   struct dma_async_tx_descriptor *rxdesc, *txdesc;
+   struct spi_master *master = sspi->master;
+
+   rxdesc = NULL;
+   if (tfr->rx_buf) {
+   struct dma_slave_config rxconf = {
+   .direction = DMA_DEV_TO_MEM,
+   .src_addr = sspi->dma_addr_rx,
+   .src_addr_width = 1,
+   .src_maxburst = 1,
+   };
+
+   dmaengine_slave_config(master->dma_rx, );
+
+   rxdesc = dmaengine_prep_slave_sg(
+   master->dma_rx,
+   tfr->rx_sg.sgl, tfr->rx_sg.nents,
+   DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+   if (!rxdesc)
+   return -EINVAL;
+   }
+
+   txdesc = NULL;
+   if (tfr->tx_buf) {
+   struct dma_slave_config txconf = {
+   .direction = DMA_MEM_TO_DEV,
+   .dst_addr = sspi->dma_addr_tx,
+   .dst_addr_width = 1,
+   .dst_maxburst = 1,
+   };
+
+   dmaengine_slave_config(master->dma_tx, );
+
+   txdesc = dmaengine_prep_slave_sg(
+   master->dma_tx,
+   tfr->tx_sg.sgl, tfr->tx_sg.nents,
+   DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT);
+   if (!txdesc) {
+   if (rxdesc)
+   dmaengine_terminate_sync(master->dma_rx);
+   return -EINVAL;
+   }
+   }
+
+   if (tfr->rx_buf) {
+   dmaengine_submit(rxdesc);
+   dma_async_issue_pending(master->dma_rx);
+   }
+
+   if (tfr->tx_buf) {
+   dmaengine_submit(txdesc);
+   dma_async_issue_pending(master->dma_tx);
+   }
+
+   return 0;
+}
+
 static int sun6i_spi_transfer_one(struct spi_master *master,
  struct spi_device *spi,
  struct spi_transfer *tfr)
@@ -201,6 +267,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
sspi->tx_buf = tfr->tx_buf;
sspi->rx_buf = tfr->rx_buf;
sspi->len = tfr->len;
+   sspi->use_dma = master->can_dma ?
+   master->can_dma(master, spi, tfr) : false;
 
/* Clear pending interrupts */
sun6i_spi_write(sspi, SUN6I_INT_STA_REG, ~0);
@@ -216,9 +284,17 @@ static int sun6i_spi_transfer_one(struct spi_master 
*master,
 * (See spi-sun4i.c)
 */
trig_level = sspi->fifo_depth / 4 * 3;
-   sun6i_spi_write(sspi, SUN6I_FIFO_CTL_REG,
-   (trig_l

[PATCH] arm64: dts: allwinner: replace numerical constant with CCU_CLKX

[Alexander Kochetkov]

From: Alexander Kochetkov 

Signed-off-by: Alexander Kochetkov 
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi 
b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index c26cc1fcaffd..dfeeb7350808 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -51,7 +51,7 @@
reg = <0>;
enable-method = "psci";
next-level-cache = <>;
-   clocks = < 21>;
+   clocks = < CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
};
@@ -62,7 +62,7 @@
reg = <1>;
enable-method = "psci";
next-level-cache = <>;
-   clocks = < 21>;
+   clocks = < CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
};
@@ -73,7 +73,7 @@
reg = <2>;
enable-method = "psci";
next-level-cache = <>;
-   clocks = < 21>;
+   clocks = < CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
};
@@ -84,7 +84,7 @@
reg = <3>;
enable-method = "psci";
next-level-cache = <>;
-   clocks = < 21>;
+   clocks = < CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
};
-- 
2.17.1

Re: [PATCH 2/2] mmc: dw_mmc-rockchip: fix transfer hangs on rk3188【请注意，邮件由linux-mmc-ow...@vger.kernel.org代发】

[Alexander Kochetkov]

Hello!

Forgot to mention transfer hags happen only on mem to dev transfers (dma writes 
to
device) and never on dev to mem.

Yea, I know, rk3188 and earlier are quite ancient, but we made custom hardware
based on rk3188 and some of our customers report problems.

For testing I use rk3188 based custom board with eMMC (probably rk3188-radxa 
rock
with SD can also be used for testing) with cpufreq  enabled.

For testing I made simple script, that do in loop following:
1. Creates 6 new empty partitions using mkfs.ext3 about 1Gb total
2. extract 100MB archive of linux image to 512Mb partition (about 400MB 
extracted size).
3. sleep random time from 60 to 120 sec

CPU load looks like that:
cpufreq stats: 312 MHz:32.63%, 504 MHz:0.00%, 600 MHz:0.00%, 816 MHz:0.38%, 
1.01 GHz:29.83%, 1.20 GHz:0.38%, 1.42 GHz:0.00%, 1.61 GHz:36.79%  (494481)

This test can run for 6 hours and than transfer can hang. I used 5 devices to 
test. Some
devices may run test for long time, but some may fail within an hour.

I played with CPU clock settings in u-boot and mmc bus clock settings dts file. 
I tried to lower eMMC bus
clock frequency to exclude PCB errors. Found that some combinations of settings
make my test run longer, but test fail anyway.

Also I found, that making following change to dw_mmc, result in high error 
count:

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 9c54d60..dcf7d36e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2905,10 +2905,9 @@ static int dw_mci_init_slot(struct dw_mci *host)
} else if (host->use_dma == TRANS_MODE_EDMAC) {
mmc->max_segs = 64;
mmc->max_blk_size = 65535;
-   mmc->max_blk_count = 65535;
-   mmc->max_req_size =
-   mmc->max_blk_size * mmc->max_blk_count;
-   mmc->max_seg_size = mmc->max_req_size;
+   mmc->max_seg_size = 0x1000;
+   mmc->max_req_size = mmc->max_seg_size * mmc->max_segs;
+   mmc->max_blk_count = mmc->max_req_size / 512;
} else {
/* TRANS_MODE_PIO */
mmc->max_segs = 64;

With this settings mmc core split large transfer to multiply item scatterlists 
and
increase scatterlists  switching rate inside pl330. So I assumed that the root 
of problem
is dma goes out of sync with device.

For, example, there is a patch in mainline linux:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/dma/pl330.c?h=v5.0.3=1d48745b192a7a45bbdd3557b4c039609569ca41
It fix the problem EDMA can get out of sync with device. But the patch don’t 
work for rk3188, because
rk3188 has PL330_QUIRK_BROKEN_NO_FLUSHP quirk.

I’ll try to backport EDMA driver from vendor 4.4 kernel and report test result.

Problem safer to fix patching dw_mmc code, than pl330 code. Because
patch change transfer parameters from known to work values:

mmc->max_segs = 64;
mmc->max_blk_size = 65535;
mmc->max_blk_count = 65535;
mmc->max_req_size =
   mmc->max_blk_size * mmc->max_blk_count;
mmc->max_seg_size = mmc->max_req_size;

to

mmc->max_segs = 1;
mmc->max_blk_size = 65535;
mmc->max_blk_count = 64 * 512;
mmc->max_req_size =
mmc->max_blk_size * mmc->max_blk_count;
mmc->max_seg_size = mmc->max_req_size;


> 21 марта 2019 г., в 5:31, Shawn Lin  написал(а):
> 
> + Caesar Wang
> 
> On 2019/3/21 1:48, Alexander Kochetkov wrote:
>> I've found that sometimes dw_mmc in my rk3188 based board stop transfer
>> any data with error:
>> kernel: dwmmc_rockchip 1021c000.dwmmc: Unexpected command timeout, state 3
>> Further digging into problem showed that sometimes one of EDMA-based
>> transfers hangs and abort with HTO error. I've made test, that 100%
> 
> I'm not sure what 100% means, but Caesar fired QA test for RK3036 with
> EDMA-based dwmmc in vendor 4.4 kernel, and seems not big deal. The
> vendor 4.4 kernel didn't patch anything else wrt EDMA code, but we did
> enhance PL330 code and fix some bug there, so you may have a try.
> 
>> reproduce the error. I found, that setting max_segs parameter to 1 fix
>> the problem.
>> I guess the problem is hardware related and relates to DMA controller
>> implementation for rk3188. Probably it can relates to missed FLUSHP,
>> see commit 271e1b86e691 ("dmaengine: pl330: add quirk for broken no
>> flushp"). It is possible that pl330 and dw_mmc become out of sync then
>> pl330 driver switch from one scatterlist to another. If we limit
>> scatterlist size to 1, we can avoid switching scatterlists and avoid
>

[PATCH 1/2] mmc: dw_mmc: add init_slot() hook to platform function table

[Alexander Kochetkov]

The init_slot() hook allow platform driver override slot defaults
provided by generic dw_mmc driver. It's required to fix EDMA based
transfer hangs observed on rockchip rk3188.

Signed-off-by: Alexander Kochetkov 
---
 drivers/mmc/host/dw_mmc.c |4 
 drivers/mmc/host/dw_mmc.h |2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 80dc2fd..d3ecee9 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2819,6 +2819,7 @@ static int dw_mci_init_slot_caps(struct dw_mci_slot *slot)
 
 static int dw_mci_init_slot(struct dw_mci *host)
 {
+   const struct dw_mci_drv_data *drv_data = host->drv_data;
struct mmc_host *mmc;
struct dw_mci_slot *slot;
int ret;
@@ -2876,6 +2877,9 @@ static int dw_mci_init_slot(struct dw_mci *host)
mmc->max_seg_size = mmc->max_req_size;
}
 
+   if (drv_data && drv_data->init_slot)
+   drv_data->init_slot(host);
+
dw_mci_get_cd(mmc);
 
ret = mmc_add_host(mmc);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 46e9f8e..de51c59 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -548,6 +548,7 @@ struct dw_mci_slot {
  * @caps: mmc subsystem specified capabilities of the controller(s).
  * @num_caps: number of capabilities specified by @caps.
  * @init: early implementation specific initialization.
+ * @init_slot: platform specific slot initialization.
  * @set_ios: handle bus specific extensions.
  * @parse_dt: parse implementation specific device tree properties.
  * @execute_tuning: implementation specific tuning procedure.
@@ -560,6 +561,7 @@ struct dw_mci_drv_data {
unsigned long   *caps;
u32 num_caps;
int (*init)(struct dw_mci *host);
+   void(*init_slot)(struct dw_mci *host);
void(*set_ios)(struct dw_mci *host, struct mmc_ios *ios);
int (*parse_dt)(struct dw_mci *host);
int (*execute_tuning)(struct dw_mci_slot *slot, u32 opcode);
-- 
1.7.9.5

[PATCH 2/2] mmc: dw_mmc-rockchip: fix transfer hangs on rk3188

[Alexander Kochetkov]

I've found that sometimes dw_mmc in my rk3188 based board stop transfer
any data with error:

kernel: dwmmc_rockchip 1021c000.dwmmc: Unexpected command timeout, state 3

Further digging into problem showed that sometimes one of EDMA-based
transfers hangs and abort with HTO error. I've made test, that 100%
reproduce the error. I found, that setting max_segs parameter to 1 fix
the problem.

I guess the problem is hardware related and relates to DMA controller
implementation for rk3188. Probably it can relates to missed FLUSHP,
see commit 271e1b86e691 ("dmaengine: pl330: add quirk for broken no
flushp"). It is possible that pl330 and dw_mmc become out of sync then
pl330 driver switch from one scatterlist to another. If we limit
scatterlist size to 1, we can avoid switching scatterlists and avoid
hardware problem. Setting max_segs to 1 tells mmc core to use maximum
one scatterlist for one transfer.

I guess that all other rk3xxx chips that lacks FLUSHP also affected by
the problem. So I made fix for all rk3xxx chips from rk2928 to rk3188.

Signed-off-by: Alexander Kochetkov 
---
 drivers/mmc/host/dw_mmc-rockchip.c |   19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c 
b/drivers/mmc/host/dw_mmc-rockchip.c
index 8c86a80..2eed922 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -292,6 +292,24 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
return 0;
 }
 
+static void dw_mci_rk2928_init_slot(struct dw_mci *host)
+{
+   struct mmc_host *mmc = host->slot->mmc;
+
+   if (host->use_dma == TRANS_MODE_EDMAC) {
+   /*
+* Using max_segs > 1 leads to rare EDMA transfer hangs
+* resulting in HTO errors.
+*/
+   mmc->max_segs = 1;
+   mmc->max_blk_size = 65535;
+   mmc->max_blk_count = 64 * 512;
+   mmc->max_req_size =
+   mmc->max_blk_size * mmc->max_blk_count;
+   mmc->max_seg_size = mmc->max_req_size;
+   }
+}
+
 static int dw_mci_rockchip_init(struct dw_mci *host)
 {
/* It is slot 8 on Rockchip SoCs */
@@ -314,6 +332,7 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
 
 static const struct dw_mci_drv_data rk2928_drv_data = {
.init   = dw_mci_rockchip_init,
+   .init_slot  = dw_mci_rk2928_init_slot,
 };
 
 static const struct dw_mci_drv_data rk3288_drv_data = {
-- 
1.7.9.5

[PATCH 0/2] Fix eMMC hang on rk3188 and earlier

[Alexander Kochetkov]

Hello!

I found, that sometimes dw_mmc driver stop transfer data to
eMMC card on my rk3188 based board. One of tranfers hangs then
doing EDMA transfer and controller gives HTO. And here is a fix.

Alexander Kochetkov (2):
  mmc: dw_mmc: add init_slot() hook to platform function table
  mmc: dw_mmc-rockchip: fix transfer hangs on rk3188

 drivers/mmc/host/dw_mmc-rockchip.c |   19 +++
 drivers/mmc/host/dw_mmc.c  |4 
 drivers/mmc/host/dw_mmc.h  |2 ++
 3 files changed, 25 insertions(+)

-- 
1.7.9.5

Re: [PATCH AUTOSEL for 3.18 34/63] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

Hello, Sasha!

Following 2 patches must be applied together with the patch:
5e0a39d0f727b35c8b7ef56ba0724c8ceb006297 clocksource/drivers/rockchip_timer: 
Implement clocksource timer
627988a66aee3c845aa2f1f874a3ddba8adb89d9 ARM: dts: rockchip: Add timer entries 
to rk3188 SoC

Alexander.

> 4 марта 2018 г., в 1:33, Sasha Levin <alexander.le...@microsoft.com> 
> написал(а):
> 
> From: Alexander Kochetkov <al.koc...@gmail.com>
> 
> [ Upstream commit 500d0aa918a2ea6bb918fee8adcf27dc2912bcd1 ]
> 
> The clocksource and the sched_clock provided by the arm_global_timer
> are quite unstable because their rates depend on the cpu frequency.
> 
> On the other side, the arm_global_timer has a higher rating than the
> rockchip_timer, it will be selected by default by the time framework
> while we want to use the stable rockchip clocksource.
> 
> Let's disable the arm_global_timer in order to have the rockchip
> clocksource selected by default.
> 
> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
> Reviewed-by: Heiko Stuebner <he...@sntech.de>
> Signed-off-by: Sasha Levin <alexander.le...@microsoft.com>
> ---
> arch/arm/boot/dts/rk3188.dtsi | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index ddaada788b45..72b0b4cb538f 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -404,6 +404,7 @@
> 
> _timer {
>   interrupts = ;
> + status = "disabled";
> };
> 
> _timer {
> -- 
> 2.14.1

Re: [PATCH AUTOSEL for 3.18 34/63] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

Hello, Sasha!

Following 2 patches must be applied together with the patch:
5e0a39d0f727b35c8b7ef56ba0724c8ceb006297 clocksource/drivers/rockchip_timer: 
Implement clocksource timer
627988a66aee3c845aa2f1f874a3ddba8adb89d9 ARM: dts: rockchip: Add timer entries 
to rk3188 SoC

Alexander.

> 4 марта 2018 г., в 1:33, Sasha Levin  
> написал(а):
> 
> From: Alexander Kochetkov 
> 
> [ Upstream commit 500d0aa918a2ea6bb918fee8adcf27dc2912bcd1 ]
> 
> The clocksource and the sched_clock provided by the arm_global_timer
> are quite unstable because their rates depend on the cpu frequency.
> 
> On the other side, the arm_global_timer has a higher rating than the
> rockchip_timer, it will be selected by default by the time framework
> while we want to use the stable rockchip clocksource.
> 
> Let's disable the arm_global_timer in order to have the rockchip
> clocksource selected by default.
> 
> Signed-off-by: Alexander Kochetkov 
> Signed-off-by: Daniel Lezcano 
> Reviewed-by: Heiko Stuebner 
> Signed-off-by: Sasha Levin 
> ---
> arch/arm/boot/dts/rk3188.dtsi | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index ddaada788b45..72b0b4cb538f 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -404,6 +404,7 @@
> 
> _timer {
>   interrupts = ;
> + status = "disabled";
> };
> 
> _timer {
> -- 
> 2.14.1

Re: [PATCH AUTOSEL for 4.4 062/115] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

Hello, Sasha!

Following 2 patches must be applied together with the patch:
5e0a39d0f727b35c8b7ef56ba0724c8ceb006297 clocksource/drivers/rockchip_timer: 
Implement clocksource timer
627988a66aee3c845aa2f1f874a3ddba8adb89d9 ARM: dts: rockchip: Add timer entries 
to rk3188 SoC

Alexander.

> 4 марта 2018 г., в 1:31, Sasha Levin <alexander.le...@microsoft.com> 
> написал(а):
> 
> From: Alexander Kochetkov <al.koc...@gmail.com>
> 
> [ Upstream commit 500d0aa918a2ea6bb918fee8adcf27dc2912bcd1 ]
> 
> The clocksource and the sched_clock provided by the arm_global_timer
> are quite unstable because their rates depend on the cpu frequency.
> 
> On the other side, the arm_global_timer has a higher rating than the
> rockchip_timer, it will be selected by default by the time framework
> while we want to use the stable rockchip clocksource.
> 
> Let's disable the arm_global_timer in order to have the rockchip
> clocksource selected by default.
> 
> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
> Reviewed-by: Heiko Stuebner <he...@sntech.de>
> Signed-off-by: Sasha Levin <alexander.le...@microsoft.com>
> ---
> arch/arm/boot/dts/rk3188.dtsi | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 6399942f1840..0e5142b6e914 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -513,6 +513,7 @@
> 
> _timer {
>   interrupts = ;
> + status = "disabled";
> };
> 
> _timer {
> -- 
> 2.14.1

Re: [PATCH AUTOSEL for 4.4 062/115] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

Hello, Sasha!

Following 2 patches must be applied together with the patch:
5e0a39d0f727b35c8b7ef56ba0724c8ceb006297 clocksource/drivers/rockchip_timer: 
Implement clocksource timer
627988a66aee3c845aa2f1f874a3ddba8adb89d9 ARM: dts: rockchip: Add timer entries 
to rk3188 SoC

Alexander.

> 4 марта 2018 г., в 1:31, Sasha Levin  
> написал(а):
> 
> From: Alexander Kochetkov 
> 
> [ Upstream commit 500d0aa918a2ea6bb918fee8adcf27dc2912bcd1 ]
> 
> The clocksource and the sched_clock provided by the arm_global_timer
> are quite unstable because their rates depend on the cpu frequency.
> 
> On the other side, the arm_global_timer has a higher rating than the
> rockchip_timer, it will be selected by default by the time framework
> while we want to use the stable rockchip clocksource.
> 
> Let's disable the arm_global_timer in order to have the rockchip
> clocksource selected by default.
> 
> Signed-off-by: Alexander Kochetkov 
> Signed-off-by: Daniel Lezcano 
> Reviewed-by: Heiko Stuebner 
> Signed-off-by: Sasha Levin 
> ---
> arch/arm/boot/dts/rk3188.dtsi | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 6399942f1840..0e5142b6e914 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -513,6 +513,7 @@
> 
> _timer {
>   interrupts = ;
> + status = "disabled";
> };
> 
> _timer {
> -- 
> 2.14.1

Re: [PATCH AUTOSEL for 4.9 124/219] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

Hello, Sasha!

Following 2 patches must be applied together with the patch:
5e0a39d0f727b35c8b7ef56ba0724c8ceb006297 clocksource/drivers/rockchip_timer: 
Implement clocksource timer
627988a66aee3c845aa2f1f874a3ddba8adb89d9 ARM: dts: rockchip: Add timer entries 
to rk3188 SoC

Alexander.


> 4 марта 2018 г., в 1:29, Sasha Levin <alexander.le...@microsoft.com> 
> написал(а):
> 
> From: Alexander Kochetkov <al.koc...@gmail.com>
> 
> [ Upstream commit 500d0aa918a2ea6bb918fee8adcf27dc2912bcd1 ]
> 
> The clocksource and the sched_clock provided by the arm_global_timer
> are quite unstable because their rates depend on the cpu frequency.
> 
> On the other side, the arm_global_timer has a higher rating than the
> rockchip_timer, it will be selected by default by the time framework
> while we want to use the stable rockchip clocksource.
> 
> Let's disable the arm_global_timer in order to have the rockchip
> clocksource selected by default.
> 
> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
> Reviewed-by: Heiko Stuebner <he...@sntech.de>
> Signed-off-by: Sasha Levin <alexander.le...@microsoft.com>
> ---
> arch/arm/boot/dts/rk3188.dtsi | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 31f81b265cef..73a200914f0b 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -530,6 +530,7 @@
> 
> _timer {
>   interrupts = ;
> + status = "disabled";
> };
> 
> _timer {
> -- 
> 2.14.1

Re: [PATCH AUTOSEL for 4.9 124/219] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

Hello, Sasha!

Following 2 patches must be applied together with the patch:
5e0a39d0f727b35c8b7ef56ba0724c8ceb006297 clocksource/drivers/rockchip_timer: 
Implement clocksource timer
627988a66aee3c845aa2f1f874a3ddba8adb89d9 ARM: dts: rockchip: Add timer entries 
to rk3188 SoC

Alexander.


> 4 марта 2018 г., в 1:29, Sasha Levin  
> написал(а):
> 
> From: Alexander Kochetkov 
> 
> [ Upstream commit 500d0aa918a2ea6bb918fee8adcf27dc2912bcd1 ]
> 
> The clocksource and the sched_clock provided by the arm_global_timer
> are quite unstable because their rates depend on the cpu frequency.
> 
> On the other side, the arm_global_timer has a higher rating than the
> rockchip_timer, it will be selected by default by the time framework
> while we want to use the stable rockchip clocksource.
> 
> Let's disable the arm_global_timer in order to have the rockchip
> clocksource selected by default.
> 
> Signed-off-by: Alexander Kochetkov 
> Signed-off-by: Daniel Lezcano 
> Reviewed-by: Heiko Stuebner 
> Signed-off-by: Sasha Levin 
> ---
> arch/arm/boot/dts/rk3188.dtsi | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 31f81b265cef..73a200914f0b 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -530,6 +530,7 @@
> 
> _timer {
>   interrupts = ;
> + status = "disabled";
> };
> 
> _timer {
> -- 
> 2.14.1

Re: [PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

> 29 дек. 2017 г., в 3:14, Stephen Boyd  написал(а):
> 
> I'm asking if the rate is capped on the consumer side with
> clk_set_max_rate() or if it's capped on the clk provider side to
> express a hardware constraint.
I do that using clk_set_max_rate() at provider size inside clk-rk3188.c.

> 
> Sounds like there are some things to be figured out here still. I
> can take a closer look next week. Maybe Heiko will respond before
> then.

I will be very grateful for the ideas. I can continue to work on this next
week too.

Happy New Year and Merry Christmas!

Regards,
Alexander.

Re: [PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

> 29 дек. 2017 г., в 3:14, Stephen Boyd  написал(а):
> 
> I'm asking if the rate is capped on the consumer side with
> clk_set_max_rate() or if it's capped on the clk provider side to
> express a hardware constraint.
I do that using clk_set_max_rate() at provider size inside clk-rk3188.c.

> 
> Sounds like there are some things to be figured out here still. I
> can take a closer look next week. Maybe Heiko will respond before
> then.

I will be very grateful for the ideas. I can continue to work on this next
week too.

Happy New Year and Merry Christmas!

Regards,
Alexander.

Re: [PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

Initial thread here:
https://www.spinics.net/lists/linux-clk/msg21682.html


> 27 дек. 2017 г., в 4:06, Stephen Boyd  написал(а):
> 
> Are these limits the min/max limits that the parent clk can
> output at? Or the min/max limits that software has constrained on
> the clk?
> 

Don’t know how to answer. For example, parent can output 768MHz,
but some IP work unstable with that parent rate. This issues was observed by
me and I didn’t get official confirmation from rockchip. So, I limit
such clock to 192MHz using clk_set_max_rate(). May be I have to limit clk rate
using another approach.

Anybody from rockchip can confirm that? Or may be contact me clarifying details?

> I haven't looked in detail at this
> rockchip_fractional_approximation() code, but it shouldn't be
> doing the work of both the child rate determination and the
> parent rate determination in one place. It should work with the
> parent to figure out the rate the parent can provide and then
> figure out how to achieve the desired rate from there.

Here is clock tree:

clock   rate
-   
xin24m  2400
  pll_gpll76800
 gpll   76800
i2s_src  76800
   i2s0_pre 19200
  i2s0_frac 16384000
 sclk_i2s0  16384000

I limit i2s0_pre rate to 192MHz in order to I2S IP work properly.
rockchip_fractional_approximation() get called for i2s0_frac.
if i2s0_pre rate is 20x times less than i2s0_frac, than 
rockchip_fractional_approximation()
tries to set i2s0_pre rate to i2s_src rate. It tries to increase it’s parent 
rate in order
to maximise relation between nominator and denominator.

If I convert rockchip_fractional_approximation() to rockchip_determine_rate(), 
than I get
min=0, max=192MHz limits inside rockchip_determine_rate(). May be there should 
be
new logic inside clk framework based on some new clk flags, that will provide 
max=768MHz
for rockchip_determine_rate().

Also found, that rockchip_fractional_approximation() increase parents rate 
unconditionally
without taking into account CLK_SET_RATE_PARENT flag.

Stephen, thanks a lot for deep description of determine_rate() background. I’ll 
taking some
time thinking about possible solutions.

Regards,
Alexander.

Re: [PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

Initial thread here:
https://www.spinics.net/lists/linux-clk/msg21682.html


> 27 дек. 2017 г., в 4:06, Stephen Boyd  написал(а):
> 
> Are these limits the min/max limits that the parent clk can
> output at? Or the min/max limits that software has constrained on
> the clk?
> 

Don’t know how to answer. For example, parent can output 768MHz,
but some IP work unstable with that parent rate. This issues was observed by
me and I didn’t get official confirmation from rockchip. So, I limit
such clock to 192MHz using clk_set_max_rate(). May be I have to limit clk rate
using another approach.

Anybody from rockchip can confirm that? Or may be contact me clarifying details?

> I haven't looked in detail at this
> rockchip_fractional_approximation() code, but it shouldn't be
> doing the work of both the child rate determination and the
> parent rate determination in one place. It should work with the
> parent to figure out the rate the parent can provide and then
> figure out how to achieve the desired rate from there.

Here is clock tree:

clock   rate
-   
xin24m  2400
  pll_gpll76800
 gpll   76800
i2s_src  76800
   i2s0_pre 19200
  i2s0_frac 16384000
 sclk_i2s0  16384000

I limit i2s0_pre rate to 192MHz in order to I2S IP work properly.
rockchip_fractional_approximation() get called for i2s0_frac.
if i2s0_pre rate is 20x times less than i2s0_frac, than 
rockchip_fractional_approximation()
tries to set i2s0_pre rate to i2s_src rate. It tries to increase it’s parent 
rate in order
to maximise relation between nominator and denominator.

If I convert rockchip_fractional_approximation() to rockchip_determine_rate(), 
than I get
min=0, max=192MHz limits inside rockchip_determine_rate(). May be there should 
be
new logic inside clk framework based on some new clk flags, that will provide 
max=768MHz
for rockchip_determine_rate().

Also found, that rockchip_fractional_approximation() increase parents rate 
unconditionally
without taking into account CLK_SET_RATE_PARENT flag.

Stephen, thanks a lot for deep description of determine_rate() background. I’ll 
taking some
time thinking about possible solutions.

Regards,
Alexander.

Re: [PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

> 21 дек. 2017 г., в 23:07, Stephen Boyd  написал(а):
> 
> Can you convert to the determine_rate op instead of round_rate?
> That function should tell you the min/max limits so that you
> don't need to query that information from the core.

I converted rockchip_fractional_approximation() to rockchip_determine_rate() 
(see the patch attached).
If it increase parent’s clock for out of limits value, than clock request will 
fail with -EINVAL, like
with round_rate() approach.

The problem is that min/max limits provided to determine_rate() is for clock 
for which the determine_rate()
was called. While rockchip_determine_rate() 
(rockchip_fractional_approximation()) requires information
about parent clock limits.

How can I know parents clock limits for current clock? Implement 
determine_rate() for each parent clocks
the same way I did for this one clock?

Regards,
Alexander.

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 3c1fb0d..1e0c701 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -174,23 +174,9 @@ static void rockchip_fractional_approximation(struct 
clk_hw *hw,
unsigned long *m, unsigned long *n)
 {
struct clk_fractional_divider *fd = to_clk_fd(hw);
-   unsigned long p_rate, p_parent_rate;
-   unsigned long min_rate = 0, max_rate = 0;
-   struct clk_hw *p_parent;
unsigned long scale;
-
-   p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
-   if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
-   p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
-   p_parent_rate = clk_hw_get_rate(p_parent);
-   clk_hw_get_boundaries(clk_hw_get_parent(hw),
-   _rate, _rate);
-   if (p_parent_rate < min_rate)
-   p_parent_rate = min_rate;
-   if (p_parent_rate > max_rate)
-   p_parent_rate = max_rate;
-   *parent_rate = p_parent_rate;
-   }
+   unsigned long rate_orig = rate;
+   unsigned long parent_rate_orig = *parent_rate;
 
/*
 * Get rate closer to *parent_rate to guarantee there is no overflow
@@ -204,8 +190,36 @@ static void rockchip_fractional_approximation(struct 
clk_hw *hw,
rational_best_approximation(rate, *parent_rate,
GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
m, n);
+
+   pr_info("%s: %s: rate:%lu -> %lu parent_rate:%lu -> %lu m:%lu n:%lu\n",
+   __func__, clk_hw_get_name(hw), rate_orig,  rate,
+   parent_rate_orig, *parent_rate,
+   *m, *n);
 }
 
+static int rockchip_determine_rate(struct clk_hw *hw,
+  struct clk_rate_request *req)
+{
+   unsigned long p_rate, p_parent_rate;
+   struct clk_hw *p_parent;
+   unsigned long best_parent_rate = req->best_parent_rate;
+
+   p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+   if ((req->rate * 20 > p_rate) && (p_rate % req->rate != 0)) {
+   p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
+   p_parent_rate = clk_hw_get_rate(p_parent);
+   req->best_parent_rate = p_parent_rate;
+   }
+
+   pr_info("%s: %s: rate:%lu min_rate:%lu max_rate:%lu 
best_parent_rate:%lu -> %lu best_parent_hw:%s\n",
+   __func__, clk_hw_get_name(hw), req->rate, req->min_rate, 
req->max_rate, best_parent_rate, req->best_parent_rate,
+   req->best_parent_hw ? clk_hw_get_name(req->best_parent_hw) : 
"");
+
+   return 0;
+}
+
+static struct clk_ops rockchip_clk_fractional_divider_ops;
+
 static struct clk *rockchip_clk_register_frac_branch(
struct rockchip_clk_provider *ctx, const char *name,
const char *const *parent_names, u8 num_parents,
@@ -253,7 +267,8 @@ static struct clk *rockchip_clk_register_frac_branch(
div->nmask = GENMASK(div->nwidth - 1, 0) << div->nshift;
div->lock = lock;
div->approximation = rockchip_fractional_approximation;
-   div_ops = _fractional_divider_ops;
+   div_ops = _clk_fractional_divider_ops;
+
 
clk = clk_register_composite(NULL, name, parent_names, num_parents,
 NULL, NULL,
@@ -392,6 +407,9 @@ struct rockchip_clk_provider * __init 
rockchip_clk_init(struct device_node *np,
ctx->grf = syscon_regmap_lookup_by_phandle(ctx->cru_node,
   "rockchip,grf");
 
+   rockchip_clk_fractional_divider_ops = clk_fractional_divider_ops;
+   rockchip_clk_fractional_divider_ops.determine_rate = 
rockchip_determine_rate;
+
return ctx;
 
 err_free:

Re: [PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

> 21 дек. 2017 г., в 23:07, Stephen Boyd  написал(а):
> 
> Can you convert to the determine_rate op instead of round_rate?
> That function should tell you the min/max limits so that you
> don't need to query that information from the core.

I converted rockchip_fractional_approximation() to rockchip_determine_rate() 
(see the patch attached).
If it increase parent’s clock for out of limits value, than clock request will 
fail with -EINVAL, like
with round_rate() approach.

The problem is that min/max limits provided to determine_rate() is for clock 
for which the determine_rate()
was called. While rockchip_determine_rate() 
(rockchip_fractional_approximation()) requires information
about parent clock limits.

How can I know parents clock limits for current clock? Implement 
determine_rate() for each parent clocks
the same way I did for this one clock?

Regards,
Alexander.

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 3c1fb0d..1e0c701 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -174,23 +174,9 @@ static void rockchip_fractional_approximation(struct 
clk_hw *hw,
unsigned long *m, unsigned long *n)
 {
struct clk_fractional_divider *fd = to_clk_fd(hw);
-   unsigned long p_rate, p_parent_rate;
-   unsigned long min_rate = 0, max_rate = 0;
-   struct clk_hw *p_parent;
unsigned long scale;
-
-   p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
-   if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
-   p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
-   p_parent_rate = clk_hw_get_rate(p_parent);
-   clk_hw_get_boundaries(clk_hw_get_parent(hw),
-   _rate, _rate);
-   if (p_parent_rate < min_rate)
-   p_parent_rate = min_rate;
-   if (p_parent_rate > max_rate)
-   p_parent_rate = max_rate;
-   *parent_rate = p_parent_rate;
-   }
+   unsigned long rate_orig = rate;
+   unsigned long parent_rate_orig = *parent_rate;
 
/*
 * Get rate closer to *parent_rate to guarantee there is no overflow
@@ -204,8 +190,36 @@ static void rockchip_fractional_approximation(struct 
clk_hw *hw,
rational_best_approximation(rate, *parent_rate,
GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
m, n);
+
+   pr_info("%s: %s: rate:%lu -> %lu parent_rate:%lu -> %lu m:%lu n:%lu\n",
+   __func__, clk_hw_get_name(hw), rate_orig,  rate,
+   parent_rate_orig, *parent_rate,
+   *m, *n);
 }
 
+static int rockchip_determine_rate(struct clk_hw *hw,
+  struct clk_rate_request *req)
+{
+   unsigned long p_rate, p_parent_rate;
+   struct clk_hw *p_parent;
+   unsigned long best_parent_rate = req->best_parent_rate;
+
+   p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+   if ((req->rate * 20 > p_rate) && (p_rate % req->rate != 0)) {
+   p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
+   p_parent_rate = clk_hw_get_rate(p_parent);
+   req->best_parent_rate = p_parent_rate;
+   }
+
+   pr_info("%s: %s: rate:%lu min_rate:%lu max_rate:%lu 
best_parent_rate:%lu -> %lu best_parent_hw:%s\n",
+   __func__, clk_hw_get_name(hw), req->rate, req->min_rate, 
req->max_rate, best_parent_rate, req->best_parent_rate,
+   req->best_parent_hw ? clk_hw_get_name(req->best_parent_hw) : 
"");
+
+   return 0;
+}
+
+static struct clk_ops rockchip_clk_fractional_divider_ops;
+
 static struct clk *rockchip_clk_register_frac_branch(
struct rockchip_clk_provider *ctx, const char *name,
const char *const *parent_names, u8 num_parents,
@@ -253,7 +267,8 @@ static struct clk *rockchip_clk_register_frac_branch(
div->nmask = GENMASK(div->nwidth - 1, 0) << div->nshift;
div->lock = lock;
div->approximation = rockchip_fractional_approximation;
-   div_ops = _fractional_divider_ops;
+   div_ops = _clk_fractional_divider_ops;
+
 
clk = clk_register_composite(NULL, name, parent_names, num_parents,
 NULL, NULL,
@@ -392,6 +407,9 @@ struct rockchip_clk_provider * __init 
rockchip_clk_init(struct device_node *np,
ctx->grf = syscon_regmap_lookup_by_phandle(ctx->cru_node,
   "rockchip,grf");
 
+   rockchip_clk_fractional_divider_ops = clk_fractional_divider_ops;
+   rockchip_clk_fractional_divider_ops.determine_rate = 
rockchip_determine_rate;
+
return ctx;
 
 err_free:

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

In order to provide a way to know clock limits to clock providers.

The patch is needed for fixing commit 5d890c2df900 ("clk: rockchip:
add special approximation to fix up fractional clk's jitter").
Custom approximation function introduced by the patch, can
select frequency rate larger than one configured using
clk_set_max_rate().

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/clk/clk.c|   14 --
 include/linux/clk-provider.h |2 ++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c8d83ac..8943aac 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -421,10 +421,11 @@ struct clk *__clk_lookup(const char *name)
return !core ? NULL : core->hw->clk;
 }
 
-static void clk_core_get_boundaries(struct clk_core *core,
-   unsigned long *min_rate,
-   unsigned long *max_rate)
+void clk_hw_get_boundaries(struct clk_hw *hw,
+  unsigned long *min_rate,
+  unsigned long *max_rate)
 {
+   struct clk_core *core = hw->core;
struct clk *clk_user;
 
*min_rate = core->min_rate;
@@ -436,6 +437,7 @@ static void clk_core_get_boundaries(struct clk_core *core,
hlist_for_each_entry(clk_user, >clks, clks_node)
*max_rate = min(*max_rate, clk_user->max_rate);
 }
+EXPORT_SYMBOL_GPL(clk_hw_get_boundaries);
 
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
   unsigned long max_rate)
@@ -894,7 +896,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned 
long rate)
int ret;
struct clk_rate_request req;
 
-   clk_core_get_boundaries(hw->core, _rate, _rate);
+   clk_hw_get_boundaries(hw, _rate, _rate);
req.rate = rate;
 
ret = clk_core_round_rate_nolock(hw->core, );
@@ -924,7 +926,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 
clk_prepare_lock();
 
-   clk_core_get_boundaries(clk->core, _rate, _rate);
+   clk_hw_get_boundaries(clk->core->hw, _rate, _rate);
req.rate = rate;
 
ret = clk_core_round_rate_nolock(clk->core, );
@@ -1353,7 +1355,7 @@ static struct clk_core *clk_calc_new_rates(struct 
clk_core *core,
if (parent)
best_parent_rate = parent->rate;
 
-   clk_core_get_boundaries(core, _rate, _rate);
+   clk_hw_get_boundaries(core->hw, _rate, _rate);
 
/* find the closest rate and parent clk/rate */
if (core->ops->determine_rate) {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5100ec1..2f10999 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -755,6 +755,8 @@ int __clk_mux_determine_rate_closest(struct clk_hw *hw,
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
   unsigned long max_rate);
+void clk_hw_get_boundaries(struct clk_hw *hw, unsigned long *min_rate,
+  unsigned long *max_rate);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {
-- 
1.7.9.5

[PATCH 0/2] Fix clock rate in the rockchip_fractional_approximation()

[Alexander Kochetkov]

Hello!

Here are two patches fixing issue in the rockchip_fractional_approximation().
rockchip_fractional_approximation() can select clock rate whose
value will violate clock limit settings (i.e. one configured with 
clk_set_max_rate() and clk_set_min_rate()).

rockchip_fractional_approximation() was introduced by commit 5d890c2df900
("clk: rockchip: add special approximation to fix up fractional clk's jitter") 
whose description states that setting denominator 20 times larger than
numerator will generate precise clock frequency. It's strange, because
on my custom rk3188-based board and on radxa rock I've observed strange
hardware issues. I2S, for example, sometimes doesn't setup correct
rate on external SCLK_I2S0. UART0, for example, started to receive '\0' 
characters instead of valid symbols, signals on UART_RX was good.

So, I use clk_set_max_rate() to limit max value of i2s0_pre, uart0_pre,
uart1_pre, uart2_pre, uart3_pre to value of aclk_cpu_pre. That fixes
strange I2S and UART issues for me. If that make sense, than I can
send the patch. But it will logically conflict with commit 5d890c2df900
("clk: rockchip: add special approximation to fix up fractional clk's jitter").

Alexander Kochetkov (2):
  clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and
expose
  clk: rockchip: limit clock rate in the
rockchip_fractional_approximation()

 drivers/clk/clk.c|   14 --
 drivers/clk/rockchip/clk.c   |7 +++
 include/linux/clk-provider.h |2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and expose

[Alexander Kochetkov]

In order to provide a way to know clock limits to clock providers.

The patch is needed for fixing commit 5d890c2df900 ("clk: rockchip:
add special approximation to fix up fractional clk's jitter").
Custom approximation function introduced by the patch, can
select frequency rate larger than one configured using
clk_set_max_rate().

Signed-off-by: Alexander Kochetkov 
---
 drivers/clk/clk.c|   14 --
 include/linux/clk-provider.h |2 ++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c8d83ac..8943aac 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -421,10 +421,11 @@ struct clk *__clk_lookup(const char *name)
return !core ? NULL : core->hw->clk;
 }
 
-static void clk_core_get_boundaries(struct clk_core *core,
-   unsigned long *min_rate,
-   unsigned long *max_rate)
+void clk_hw_get_boundaries(struct clk_hw *hw,
+  unsigned long *min_rate,
+  unsigned long *max_rate)
 {
+   struct clk_core *core = hw->core;
struct clk *clk_user;
 
*min_rate = core->min_rate;
@@ -436,6 +437,7 @@ static void clk_core_get_boundaries(struct clk_core *core,
hlist_for_each_entry(clk_user, >clks, clks_node)
*max_rate = min(*max_rate, clk_user->max_rate);
 }
+EXPORT_SYMBOL_GPL(clk_hw_get_boundaries);
 
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
   unsigned long max_rate)
@@ -894,7 +896,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned 
long rate)
int ret;
struct clk_rate_request req;
 
-   clk_core_get_boundaries(hw->core, _rate, _rate);
+   clk_hw_get_boundaries(hw, _rate, _rate);
req.rate = rate;
 
ret = clk_core_round_rate_nolock(hw->core, );
@@ -924,7 +926,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 
clk_prepare_lock();
 
-   clk_core_get_boundaries(clk->core, _rate, _rate);
+   clk_hw_get_boundaries(clk->core->hw, _rate, _rate);
req.rate = rate;
 
ret = clk_core_round_rate_nolock(clk->core, );
@@ -1353,7 +1355,7 @@ static struct clk_core *clk_calc_new_rates(struct 
clk_core *core,
if (parent)
best_parent_rate = parent->rate;
 
-   clk_core_get_boundaries(core, _rate, _rate);
+   clk_hw_get_boundaries(core->hw, _rate, _rate);
 
/* find the closest rate and parent clk/rate */
if (core->ops->determine_rate) {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5100ec1..2f10999 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -755,6 +755,8 @@ int __clk_mux_determine_rate_closest(struct clk_hw *hw,
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
   unsigned long max_rate);
+void clk_hw_get_boundaries(struct clk_hw *hw, unsigned long *min_rate,
+  unsigned long *max_rate);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {
-- 
1.7.9.5

[PATCH 0/2] Fix clock rate in the rockchip_fractional_approximation()

[Alexander Kochetkov]

Hello!

Here are two patches fixing issue in the rockchip_fractional_approximation().
rockchip_fractional_approximation() can select clock rate whose
value will violate clock limit settings (i.e. one configured with 
clk_set_max_rate() and clk_set_min_rate()).

rockchip_fractional_approximation() was introduced by commit 5d890c2df900
("clk: rockchip: add special approximation to fix up fractional clk's jitter") 
whose description states that setting denominator 20 times larger than
numerator will generate precise clock frequency. It's strange, because
on my custom rk3188-based board and on radxa rock I've observed strange
hardware issues. I2S, for example, sometimes doesn't setup correct
rate on external SCLK_I2S0. UART0, for example, started to receive '\0' 
characters instead of valid symbols, signals on UART_RX was good.

So, I use clk_set_max_rate() to limit max value of i2s0_pre, uart0_pre,
uart1_pre, uart2_pre, uart3_pre to value of aclk_cpu_pre. That fixes
strange I2S and UART issues for me. If that make sense, than I can
send the patch. But it will logically conflict with commit 5d890c2df900
("clk: rockchip: add special approximation to fix up fractional clk's jitter").

Alexander Kochetkov (2):
  clk: rename clk_core_get_boundaries() to clk_hw_get_boundaries() and
expose
  clk: rockchip: limit clock rate in the
rockchip_fractional_approximation()

 drivers/clk/clk.c|   14 --
 drivers/clk/rockchip/clk.c   |7 +++
 include/linux/clk-provider.h |2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 2/2] clk: rockchip: limit clock rate in the rockchip_fractional_approximation()

[Alexander Kochetkov]

rockchip_fractional_approximation() can choose clock rate that can
be larger than one configured using clk_set_max_rate(). Request
to setup correct clock rate whose parent rate will be adjusted
to out of range value will fail with -EINVAL.

Fixes: commit 5d890c2df900 ("clk: rockchip: add special approximation
to fix up fractional clk's jitter").

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/clk/rockchip/clk.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 35dbd63..3c1fb0d 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -175,6 +175,7 @@ static void rockchip_fractional_approximation(struct clk_hw 
*hw,
 {
struct clk_fractional_divider *fd = to_clk_fd(hw);
unsigned long p_rate, p_parent_rate;
+   unsigned long min_rate = 0, max_rate = 0;
struct clk_hw *p_parent;
unsigned long scale;
 
@@ -182,6 +183,12 @@ static void rockchip_fractional_approximation(struct 
clk_hw *hw,
if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
p_parent_rate = clk_hw_get_rate(p_parent);
+   clk_hw_get_boundaries(clk_hw_get_parent(hw),
+   _rate, _rate);
+   if (p_parent_rate < min_rate)
+   p_parent_rate = min_rate;
+   if (p_parent_rate > max_rate)
+   p_parent_rate = max_rate;
*parent_rate = p_parent_rate;
}
 
-- 
1.7.9.5

[PATCH 2/2] clk: rockchip: limit clock rate in the rockchip_fractional_approximation()

[Alexander Kochetkov]

rockchip_fractional_approximation() can choose clock rate that can
be larger than one configured using clk_set_max_rate(). Request
to setup correct clock rate whose parent rate will be adjusted
to out of range value will fail with -EINVAL.

Fixes: commit 5d890c2df900 ("clk: rockchip: add special approximation
to fix up fractional clk's jitter").

Signed-off-by: Alexander Kochetkov 
---
 drivers/clk/rockchip/clk.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 35dbd63..3c1fb0d 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -175,6 +175,7 @@ static void rockchip_fractional_approximation(struct clk_hw 
*hw,
 {
struct clk_fractional_divider *fd = to_clk_fd(hw);
unsigned long p_rate, p_parent_rate;
+   unsigned long min_rate = 0, max_rate = 0;
struct clk_hw *p_parent;
unsigned long scale;
 
@@ -182,6 +183,12 @@ static void rockchip_fractional_approximation(struct 
clk_hw *hw,
if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {
p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
p_parent_rate = clk_hw_get_rate(p_parent);
+   clk_hw_get_boundaries(clk_hw_get_parent(hw),
+   _rate, _rate);
+   if (p_parent_rate < min_rate)
+   p_parent_rate = min_rate;
+   if (p_parent_rate > max_rate)
+   p_parent_rate = max_rate;
*parent_rate = p_parent_rate;
}
 
-- 
1.7.9.5

Re: [PATCH] net: arc_emac: fix arc_emac_rx() error paths

[Alexander Kochetkov]

> 19 дек. 2017 г., в 18:22, David Miller <da...@davemloft.net> написал(а):
> 
> From: Alexander Kochetkov <al.koc...@gmail.com>
> Date: Fri, 15 Dec 2017 20:20:06 +0300
> 
>> arc_emac_rx() has some issues found by code review.
>> 
>> In case netdev_alloc_skb_ip_align() or dma_map_single() failure
>> rx fifo entry will not be returned to EMAC.
>> 
>> In case dma_map_single() failure previously allocated skb became
>> lost to driver. At the same time address of newly allocated skb
>> will not be provided to EMAC.
>> 
>> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> 
> This patch adds quite a few bugs, here is one which shows this is not
> functionally tested:

May be I don’t understand correctly, but I don’t see bug here.

Wrong dma mapping usage should immediately manifest itself (kernel
instability, koops and so on). The patch was tested on rk3188 and work fine for 
me.
Also I did simulations of netdev_alloc_skb_ip_align() and dma_map_single()
faults and can confirm that new error handling work.

Could someone else with ARC EMAC test the patch? I would be very grateful for 
the help.
Florian or Eric, can you test it on your hardware?

>> +
>> +/* unmap previosly mapped skb */
>> +dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
>> + dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
> 
> And then you unmap it.  "addr" is the new DMA mapping, not the old one.

The old mapping should be unmapped here. It refer to old skb what contains 
already
received packet. Because buffer doesn’t belong to EMAC anymore.
That old mapping point to old skb buffer. And netif_receive_skb() will be
called for old skb after that.

The new mapping «addr" will be unmapped then EMAC receive
new packet. Later by the code (after netif_receive_skb()) there are lines for
saving «addr» value:

rx_buff->skb = skb;
dma_unmap_addr_set(rx_buff, addr, addr);
dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE);

Regards,
Alexander.

Re: [PATCH] net: arc_emac: fix arc_emac_rx() error paths

[Alexander Kochetkov]

> 19 дек. 2017 г., в 18:22, David Miller  написал(а):
> 
> From: Alexander Kochetkov 
> Date: Fri, 15 Dec 2017 20:20:06 +0300
> 
>> arc_emac_rx() has some issues found by code review.
>> 
>> In case netdev_alloc_skb_ip_align() or dma_map_single() failure
>> rx fifo entry will not be returned to EMAC.
>> 
>> In case dma_map_single() failure previously allocated skb became
>> lost to driver. At the same time address of newly allocated skb
>> will not be provided to EMAC.
>> 
>> Signed-off-by: Alexander Kochetkov 
> 
> This patch adds quite a few bugs, here is one which shows this is not
> functionally tested:

May be I don’t understand correctly, but I don’t see bug here.

Wrong dma mapping usage should immediately manifest itself (kernel
instability, koops and so on). The patch was tested on rk3188 and work fine for 
me.
Also I did simulations of netdev_alloc_skb_ip_align() and dma_map_single()
faults and can confirm that new error handling work.

Could someone else with ARC EMAC test the patch? I would be very grateful for 
the help.
Florian or Eric, can you test it on your hardware?

>> +
>> +/* unmap previosly mapped skb */
>> +dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
>> + dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
> 
> And then you unmap it.  "addr" is the new DMA mapping, not the old one.

The old mapping should be unmapped here. It refer to old skb what contains 
already
received packet. Because buffer doesn’t belong to EMAC anymore.
That old mapping point to old skb buffer. And netif_receive_skb() will be
called for old skb after that.

The new mapping «addr" will be unmapped then EMAC receive
new packet. Later by the code (after netif_receive_skb()) there are lines for
saving «addr» value:

rx_buff->skb = skb;
dma_unmap_addr_set(rx_buff, addr, addr);
dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE);

Regards,
Alexander.

[PATCH v2] net: arc_emac: fix arc_emac_rx() error paths

[Alexander Kochetkov]

arc_emac_rx() has some issues found by code review.

In case netdev_alloc_skb_ip_align() or dma_map_single() failure
rx fifo entry will not be returned to EMAC.

In case dma_map_single() failure previously allocated skb became
lost to driver. At the same time address of newly allocated skb
will not be provided to EMAC.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
Changes in v2:
- Rebased against stable linux-4.14.y branch

 drivers/net/ethernet/arc/emac_main.c |   53 --
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index 363d909..bd277b0 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -212,39 +212,48 @@ static int arc_emac_rx(struct net_device *ndev, int 
budget)
continue;
}
 
-   pktlen = info & LEN_MASK;
-   stats->rx_packets++;
-   stats->rx_bytes += pktlen;
-   skb = rx_buff->skb;
-   skb_put(skb, pktlen);
-   skb->dev = ndev;
-   skb->protocol = eth_type_trans(skb, ndev);
-
-   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
-dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
-
-   /* Prepare the BD for next cycle */
-   rx_buff->skb = netdev_alloc_skb_ip_align(ndev,
-EMAC_BUFFER_SIZE);
-   if (unlikely(!rx_buff->skb)) {
+   /* Prepare the BD for next cycle. netif_receive_skb()
+* only if new skb was allocated and mapped to avoid holes
+* in the RX fifo.
+*/
+   skb = netdev_alloc_skb_ip_align(ndev, EMAC_BUFFER_SIZE);
+   if (unlikely(!skb)) {
+   if (net_ratelimit())
+   netdev_err(ndev, "cannot allocate skb\n");
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
-   /* Because receive_skb is below, increment rx_dropped */
stats->rx_dropped++;
continue;
}
 
-   /* receive_skb only if new skb was allocated to avoid holes */
-   netif_receive_skb(skb);
-
-   addr = dma_map_single(>dev, (void *)rx_buff->skb->data,
+   addr = dma_map_single(>dev, (void *)skb->data,
  EMAC_BUFFER_SIZE, DMA_FROM_DEVICE);
if (dma_mapping_error(>dev, addr)) {
if (net_ratelimit())
-   netdev_err(ndev, "cannot dma map\n");
-   dev_kfree_skb(rx_buff->skb);
+   netdev_err(ndev, "cannot map dma buffer\n");
+   dev_kfree_skb(skb);
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
+   stats->rx_dropped++;
continue;
}
+
+   /* unmap previosly mapped skb */
+   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
+dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
+
+   pktlen = info & LEN_MASK;
+   stats->rx_packets++;
+   stats->rx_bytes += pktlen;
+   skb_put(rx_buff->skb, pktlen);
+   rx_buff->skb->dev = ndev;
+   rx_buff->skb->protocol = eth_type_trans(rx_buff->skb, ndev);
+
+   netif_receive_skb(rx_buff->skb);
+
+   rx_buff->skb = skb;
dma_unmap_addr_set(rx_buff, addr, addr);
dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE);
 
-- 
1.7.9.5

[PATCH v2] net: arc_emac: fix arc_emac_rx() error paths

[Alexander Kochetkov]

arc_emac_rx() has some issues found by code review.

In case netdev_alloc_skb_ip_align() or dma_map_single() failure
rx fifo entry will not be returned to EMAC.

In case dma_map_single() failure previously allocated skb became
lost to driver. At the same time address of newly allocated skb
will not be provided to EMAC.

Signed-off-by: Alexander Kochetkov 
---
Changes in v2:
- Rebased against stable linux-4.14.y branch

 drivers/net/ethernet/arc/emac_main.c |   53 --
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index 363d909..bd277b0 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -212,39 +212,48 @@ static int arc_emac_rx(struct net_device *ndev, int 
budget)
continue;
}
 
-   pktlen = info & LEN_MASK;
-   stats->rx_packets++;
-   stats->rx_bytes += pktlen;
-   skb = rx_buff->skb;
-   skb_put(skb, pktlen);
-   skb->dev = ndev;
-   skb->protocol = eth_type_trans(skb, ndev);
-
-   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
-dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
-
-   /* Prepare the BD for next cycle */
-   rx_buff->skb = netdev_alloc_skb_ip_align(ndev,
-EMAC_BUFFER_SIZE);
-   if (unlikely(!rx_buff->skb)) {
+   /* Prepare the BD for next cycle. netif_receive_skb()
+* only if new skb was allocated and mapped to avoid holes
+* in the RX fifo.
+*/
+   skb = netdev_alloc_skb_ip_align(ndev, EMAC_BUFFER_SIZE);
+   if (unlikely(!skb)) {
+   if (net_ratelimit())
+   netdev_err(ndev, "cannot allocate skb\n");
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
-   /* Because receive_skb is below, increment rx_dropped */
stats->rx_dropped++;
continue;
}
 
-   /* receive_skb only if new skb was allocated to avoid holes */
-   netif_receive_skb(skb);
-
-   addr = dma_map_single(>dev, (void *)rx_buff->skb->data,
+   addr = dma_map_single(>dev, (void *)skb->data,
  EMAC_BUFFER_SIZE, DMA_FROM_DEVICE);
if (dma_mapping_error(>dev, addr)) {
if (net_ratelimit())
-   netdev_err(ndev, "cannot dma map\n");
-   dev_kfree_skb(rx_buff->skb);
+   netdev_err(ndev, "cannot map dma buffer\n");
+   dev_kfree_skb(skb);
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
+   stats->rx_dropped++;
continue;
}
+
+   /* unmap previosly mapped skb */
+   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
+dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
+
+   pktlen = info & LEN_MASK;
+   stats->rx_packets++;
+   stats->rx_bytes += pktlen;
+   skb_put(rx_buff->skb, pktlen);
+   rx_buff->skb->dev = ndev;
+   rx_buff->skb->protocol = eth_type_trans(rx_buff->skb, ndev);
+
+   netif_receive_skb(rx_buff->skb);
+
+   rx_buff->skb = skb;
dma_unmap_addr_set(rx_buff, addr, addr);
dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE);
 
-- 
1.7.9.5

[PATCH v2] net: arc_emac: restart stalled EMAC

[Alexander Kochetkov]

Under certain conditions EMAC stop reception of incoming packets and
continuously increment R_MISS register instead of saving data into
provided buffer. The commit implement workaround for such situation.
Then the stall detected EMAC will be restarted.

On device the stall looks like the device lost it's dynamic IP address.
ifconfig shows that interface error counter rapidly increments.
At the same time on the DHCP server we can see continues DHCP-requests
from device.

In real network stalls happen really rarely. To make them frequent the
broadcast storm[1] should be simulated. For simulation it is necessary
to make following connections:
1. connect radxarock to 1st port of switch
2. connect some PC to 2nd port of switch
3. connect two other free ports together using standard ethernet cable,
   in order to make a switching loop.

After that, is necessary to make a broadcast storm. For example, running on
PC 'ping' to some IP address triggers ARP-request storm. After some
time (~10sec), EMAC on rk3188 will stall.

Observed and tested on rk3188 radxarock.

[1] https://en.wikipedia.org/wiki/Broadcast_radiation

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---

Changes in v2:
- Rebased against stable linux-4.14.y branch

 drivers/net/ethernet/arc/emac.h  |2 +
 drivers/net/ethernet/arc/emac_main.c |  111 ++
 2 files changed, 113 insertions(+)

diff --git a/drivers/net/ethernet/arc/emac.h b/drivers/net/ethernet/arc/emac.h
index 3c63b16..d9efbc8 100644
--- a/drivers/net/ethernet/arc/emac.h
+++ b/drivers/net/ethernet/arc/emac.h
@@ -159,6 +159,8 @@ struct arc_emac_priv {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+
+   unsigned int rx_missed_errors;
 };
 
 /**
diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index 3241af1..363d909 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -26,6 +26,8 @@
 
 #include "emac.h"
 
+static void arc_emac_restart(struct net_device *ndev);
+
 /**
  * arc_emac_tx_avail - Return the number of available slots in the tx ring.
  * @priv: Pointer to ARC EMAC private data structure.
@@ -259,6 +261,53 @@ static int arc_emac_rx(struct net_device *ndev, int budget)
 }
 
 /**
+ * arc_emac_rx_miss_handle - handle R_MISS register
+ * @ndev:  Pointer to the net_device structure.
+ */
+static void arc_emac_rx_miss_handle(struct net_device *ndev)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct net_device_stats *stats = >stats;
+   unsigned int miss;
+
+   miss = arc_reg_get(priv, R_MISS);
+   if (miss) {
+   stats->rx_errors += miss;
+   stats->rx_missed_errors += miss;
+   priv->rx_missed_errors += miss;
+   }
+}
+
+/**
+ * arc_emac_rx_stall_check - check RX stall
+ * @ndev:  Pointer to the net_device structure.
+ * @budget:How many BDs requested to process on 1 call.
+ * @work_done: How many BDs processed
+ *
+ * Under certain conditions EMAC stop reception of incoming packets and
+ * continuously increment R_MISS register instead of saving data into
+ * provided buffer. This function detect that condition and restart
+ * EMAC.
+ */
+static void arc_emac_rx_stall_check(struct net_device *ndev,
+   int budget, unsigned int work_done)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct arc_emac_bd *rxbd;
+
+   if (work_done)
+   priv->rx_missed_errors = 0;
+
+   if (priv->rx_missed_errors && budget) {
+   rxbd = >rxbd[priv->last_rx_bd];
+   if (le32_to_cpu(rxbd->info) & FOR_EMAC) {
+   arc_emac_restart(ndev);
+   priv->rx_missed_errors = 0;
+   }
+   }
+}
+
+/**
  * arc_emac_poll - NAPI poll handler.
  * @napi:  Pointer to napi_struct structure.
  * @budget:How many BDs to process on 1 call.
@@ -272,6 +321,7 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
unsigned int work_done;
 
arc_emac_tx_clean(ndev);
+   arc_emac_rx_miss_handle(ndev);
 
work_done = arc_emac_rx(ndev, budget);
if (work_done < budget) {
@@ -279,6 +329,8 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
arc_reg_or(priv, R_ENABLE, RXINT_MASK | TXINT_MASK);
}
 
+   arc_emac_rx_stall_check(ndev, budget, work_done);
+
return work_done;
 }
 
@@ -320,6 +372,8 @@ static irqreturn_t arc_emac_intr(int irq, void 
*dev_instance)
if (status & MSER_MASK) {
stats->rx_missed_errors += 0x100;
stats->rx_errors += 0x100;
+   priv->rx_missed_errors += 0x100;
+   napi_schedule(>napi);
}
 
   

[PATCH v2] net: arc_emac: restart stalled EMAC

[Alexander Kochetkov]

Under certain conditions EMAC stop reception of incoming packets and
continuously increment R_MISS register instead of saving data into
provided buffer. The commit implement workaround for such situation.
Then the stall detected EMAC will be restarted.

On device the stall looks like the device lost it's dynamic IP address.
ifconfig shows that interface error counter rapidly increments.
At the same time on the DHCP server we can see continues DHCP-requests
from device.

In real network stalls happen really rarely. To make them frequent the
broadcast storm[1] should be simulated. For simulation it is necessary
to make following connections:
1. connect radxarock to 1st port of switch
2. connect some PC to 2nd port of switch
3. connect two other free ports together using standard ethernet cable,
   in order to make a switching loop.

After that, is necessary to make a broadcast storm. For example, running on
PC 'ping' to some IP address triggers ARP-request storm. After some
time (~10sec), EMAC on rk3188 will stall.

Observed and tested on rk3188 radxarock.

[1] https://en.wikipedia.org/wiki/Broadcast_radiation

Signed-off-by: Alexander Kochetkov 
---

Changes in v2:
- Rebased against stable linux-4.14.y branch

 drivers/net/ethernet/arc/emac.h  |2 +
 drivers/net/ethernet/arc/emac_main.c |  111 ++
 2 files changed, 113 insertions(+)

diff --git a/drivers/net/ethernet/arc/emac.h b/drivers/net/ethernet/arc/emac.h
index 3c63b16..d9efbc8 100644
--- a/drivers/net/ethernet/arc/emac.h
+++ b/drivers/net/ethernet/arc/emac.h
@@ -159,6 +159,8 @@ struct arc_emac_priv {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+
+   unsigned int rx_missed_errors;
 };
 
 /**
diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index 3241af1..363d909 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -26,6 +26,8 @@
 
 #include "emac.h"
 
+static void arc_emac_restart(struct net_device *ndev);
+
 /**
  * arc_emac_tx_avail - Return the number of available slots in the tx ring.
  * @priv: Pointer to ARC EMAC private data structure.
@@ -259,6 +261,53 @@ static int arc_emac_rx(struct net_device *ndev, int budget)
 }
 
 /**
+ * arc_emac_rx_miss_handle - handle R_MISS register
+ * @ndev:  Pointer to the net_device structure.
+ */
+static void arc_emac_rx_miss_handle(struct net_device *ndev)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct net_device_stats *stats = >stats;
+   unsigned int miss;
+
+   miss = arc_reg_get(priv, R_MISS);
+   if (miss) {
+   stats->rx_errors += miss;
+   stats->rx_missed_errors += miss;
+   priv->rx_missed_errors += miss;
+   }
+}
+
+/**
+ * arc_emac_rx_stall_check - check RX stall
+ * @ndev:  Pointer to the net_device structure.
+ * @budget:How many BDs requested to process on 1 call.
+ * @work_done: How many BDs processed
+ *
+ * Under certain conditions EMAC stop reception of incoming packets and
+ * continuously increment R_MISS register instead of saving data into
+ * provided buffer. This function detect that condition and restart
+ * EMAC.
+ */
+static void arc_emac_rx_stall_check(struct net_device *ndev,
+   int budget, unsigned int work_done)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct arc_emac_bd *rxbd;
+
+   if (work_done)
+   priv->rx_missed_errors = 0;
+
+   if (priv->rx_missed_errors && budget) {
+   rxbd = >rxbd[priv->last_rx_bd];
+   if (le32_to_cpu(rxbd->info) & FOR_EMAC) {
+   arc_emac_restart(ndev);
+   priv->rx_missed_errors = 0;
+   }
+   }
+}
+
+/**
  * arc_emac_poll - NAPI poll handler.
  * @napi:  Pointer to napi_struct structure.
  * @budget:How many BDs to process on 1 call.
@@ -272,6 +321,7 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
unsigned int work_done;
 
arc_emac_tx_clean(ndev);
+   arc_emac_rx_miss_handle(ndev);
 
work_done = arc_emac_rx(ndev, budget);
if (work_done < budget) {
@@ -279,6 +329,8 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
arc_reg_or(priv, R_ENABLE, RXINT_MASK | TXINT_MASK);
}
 
+   arc_emac_rx_stall_check(ndev, budget, work_done);
+
return work_done;
 }
 
@@ -320,6 +372,8 @@ static irqreturn_t arc_emac_intr(int irq, void 
*dev_instance)
if (status & MSER_MASK) {
stats->rx_missed_errors += 0x100;
stats->rx_errors += 0x100;
+   priv->rx_missed_errors += 0x100;
+   napi_schedule(>napi);
}
 
if (status & RXCR_MA

[PATCH] net: arc_emac: fix arc_emac_rx() error paths

[Alexander Kochetkov]

arc_emac_rx() has some issues found by code review.

In case netdev_alloc_skb_ip_align() or dma_map_single() failure
rx fifo entry will not be returned to EMAC.

In case dma_map_single() failure previously allocated skb became
lost to driver. At the same time address of newly allocated skb
will not be provided to EMAC.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/net/ethernet/arc/emac_main.c |   53 --
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index b2e0051..0ea57fe 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -212,39 +212,48 @@ static int arc_emac_rx(struct net_device *ndev, int 
budget)
continue;
}
 
-   pktlen = info & LEN_MASK;
-   stats->rx_packets++;
-   stats->rx_bytes += pktlen;
-   skb = rx_buff->skb;
-   skb_put(skb, pktlen);
-   skb->dev = ndev;
-   skb->protocol = eth_type_trans(skb, ndev);
-
-   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
-dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
-
-   /* Prepare the BD for next cycle */
-   rx_buff->skb = netdev_alloc_skb_ip_align(ndev,
-EMAC_BUFFER_SIZE);
-   if (unlikely(!rx_buff->skb)) {
+   /* Prepare the BD for next cycle. netif_receive_skb()
+* only if new skb was allocated and mapped to avoid holes
+* in the RX fifo.
+*/
+   skb = netdev_alloc_skb_ip_align(ndev, EMAC_BUFFER_SIZE);
+   if (unlikely(!skb)) {
+   if (net_ratelimit())
+   netdev_err(ndev, "cannot allocate skb\n");
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
-   /* Because receive_skb is below, increment rx_dropped */
stats->rx_dropped++;
continue;
}
 
-   /* receive_skb only if new skb was allocated to avoid holes */
-   netif_receive_skb(skb);
-
-   addr = dma_map_single(>dev, (void *)rx_buff->skb->data,
+   addr = dma_map_single(>dev, (void *)skb->data,
  EMAC_BUFFER_SIZE, DMA_FROM_DEVICE);
if (dma_mapping_error(>dev, addr)) {
if (net_ratelimit())
-   netdev_err(ndev, "cannot dma map\n");
-   dev_kfree_skb(rx_buff->skb);
+   netdev_err(ndev, "cannot map dma buffer\n");
+   dev_kfree_skb(skb);
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
+   stats->rx_dropped++;
continue;
}
+
+   /* unmap previosly mapped skb */
+   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
+dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
+
+   pktlen = info & LEN_MASK;
+   stats->rx_packets++;
+   stats->rx_bytes += pktlen;
+   skb_put(rx_buff->skb, pktlen);
+   rx_buff->skb->dev = ndev;
+   rx_buff->skb->protocol = eth_type_trans(rx_buff->skb, ndev);
+
+   netif_receive_skb(rx_buff->skb);
+
+   rx_buff->skb = skb;
dma_unmap_addr_set(rx_buff, addr, addr);
dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE);
 
-- 
1.7.9.5

[PATCH] net: arc_emac: fix arc_emac_rx() error paths

[Alexander Kochetkov]

arc_emac_rx() has some issues found by code review.

In case netdev_alloc_skb_ip_align() or dma_map_single() failure
rx fifo entry will not be returned to EMAC.

In case dma_map_single() failure previously allocated skb became
lost to driver. At the same time address of newly allocated skb
will not be provided to EMAC.

Signed-off-by: Alexander Kochetkov 
---
 drivers/net/ethernet/arc/emac_main.c |   53 --
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index b2e0051..0ea57fe 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -212,39 +212,48 @@ static int arc_emac_rx(struct net_device *ndev, int 
budget)
continue;
}
 
-   pktlen = info & LEN_MASK;
-   stats->rx_packets++;
-   stats->rx_bytes += pktlen;
-   skb = rx_buff->skb;
-   skb_put(skb, pktlen);
-   skb->dev = ndev;
-   skb->protocol = eth_type_trans(skb, ndev);
-
-   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
-dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
-
-   /* Prepare the BD for next cycle */
-   rx_buff->skb = netdev_alloc_skb_ip_align(ndev,
-EMAC_BUFFER_SIZE);
-   if (unlikely(!rx_buff->skb)) {
+   /* Prepare the BD for next cycle. netif_receive_skb()
+* only if new skb was allocated and mapped to avoid holes
+* in the RX fifo.
+*/
+   skb = netdev_alloc_skb_ip_align(ndev, EMAC_BUFFER_SIZE);
+   if (unlikely(!skb)) {
+   if (net_ratelimit())
+   netdev_err(ndev, "cannot allocate skb\n");
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
-   /* Because receive_skb is below, increment rx_dropped */
stats->rx_dropped++;
continue;
}
 
-   /* receive_skb only if new skb was allocated to avoid holes */
-   netif_receive_skb(skb);
-
-   addr = dma_map_single(>dev, (void *)rx_buff->skb->data,
+   addr = dma_map_single(>dev, (void *)skb->data,
  EMAC_BUFFER_SIZE, DMA_FROM_DEVICE);
if (dma_mapping_error(>dev, addr)) {
if (net_ratelimit())
-   netdev_err(ndev, "cannot dma map\n");
-   dev_kfree_skb(rx_buff->skb);
+   netdev_err(ndev, "cannot map dma buffer\n");
+   dev_kfree_skb(skb);
+   /* Return ownership to EMAC */
+   rxbd->info = cpu_to_le32(FOR_EMAC | EMAC_BUFFER_SIZE);
stats->rx_errors++;
+   stats->rx_dropped++;
continue;
}
+
+   /* unmap previosly mapped skb */
+   dma_unmap_single(>dev, dma_unmap_addr(rx_buff, addr),
+dma_unmap_len(rx_buff, len), DMA_FROM_DEVICE);
+
+   pktlen = info & LEN_MASK;
+   stats->rx_packets++;
+   stats->rx_bytes += pktlen;
+   skb_put(rx_buff->skb, pktlen);
+   rx_buff->skb->dev = ndev;
+   rx_buff->skb->protocol = eth_type_trans(rx_buff->skb, ndev);
+
+   netif_receive_skb(rx_buff->skb);
+
+   rx_buff->skb = skb;
dma_unmap_addr_set(rx_buff, addr, addr);
dma_unmap_len_set(rx_buff, len, EMAC_BUFFER_SIZE);
 
-- 
1.7.9.5

[PATCH] net: arc_emac: restart stalled EMAC

[Alexander Kochetkov]

Under certain conditions EMAC stop reception of incoming packets and
continuously increment R_MISS register instead of saving data into
provided buffer. The commit implement workaround for such situation.
Then the stall detected EMAC will be restarted.

On device the stall looks like the device lost it's dynamic IP address.
ifconfig shows that interface error counter rapidly increments.
At the same time on the DHCP server we can see continues DHCP-requests
from device.

In real network stalls happen really rarely. To make them frequent the
broadcast storm[1] should be simulated. For simulation it is necessary
to make following connections:
1. connect radxarock to 1st port of switch
2. connect some PC to 2nd port of switch
3. connect two other free ports together using standard ethernet cable,
   in order to make a switching loop.

After that, is necessary to make a broadcast storm. For example, running on
PC 'ping' to some IP address triggers ARP-request storm. After some
time (~10sec), EMAC on rk3188 will stall.

Observed and tested on rk3188 radxarock.

[1] https://en.wikipedia.org/wiki/Broadcast_radiation

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/net/ethernet/arc/emac.h  |2 +
 drivers/net/ethernet/arc/emac_main.c |  111 ++
 2 files changed, 113 insertions(+)

diff --git a/drivers/net/ethernet/arc/emac.h b/drivers/net/ethernet/arc/emac.h
index e4feb71..1c7afc5 100644
--- a/drivers/net/ethernet/arc/emac.h
+++ b/drivers/net/ethernet/arc/emac.h
@@ -158,6 +158,8 @@ struct arc_emac_priv {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+
+   unsigned int rx_missed_errors;
 };
 
 /**
diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index 68de2f2..b2e0051 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -26,6 +26,8 @@
 
 #include "emac.h"
 
+static void arc_emac_restart(struct net_device *ndev);
+
 /**
  * arc_emac_tx_avail - Return the number of available slots in the tx ring.
  * @priv: Pointer to ARC EMAC private data structure.
@@ -259,6 +261,53 @@ static int arc_emac_rx(struct net_device *ndev, int budget)
 }
 
 /**
+ * arc_emac_rx_miss_handle - handle R_MISS register
+ * @ndev:  Pointer to the net_device structure.
+ */
+static void arc_emac_rx_miss_handle(struct net_device *ndev)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct net_device_stats *stats = >stats;
+   unsigned int miss;
+
+   miss = arc_reg_get(priv, R_MISS);
+   if (miss) {
+   stats->rx_errors += miss;
+   stats->rx_missed_errors += miss;
+   priv->rx_missed_errors += miss;
+   }
+}
+
+/**
+ * arc_emac_rx_stall_check - check RX stall
+ * @ndev:  Pointer to the net_device structure.
+ * @budget:How many BDs requested to process on 1 call.
+ * @work_done: How many BDs processed
+ *
+ * Under certain conditions EMAC stop reception of incoming packets and
+ * continuously increment R_MISS register instead of saving data into
+ * provided buffer. This function detect that condition and restart
+ * EMAC.
+ */
+static void arc_emac_rx_stall_check(struct net_device *ndev,
+   int budget, unsigned int work_done)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct arc_emac_bd *rxbd;
+
+   if (work_done)
+   priv->rx_missed_errors = 0;
+
+   if (priv->rx_missed_errors && budget) {
+   rxbd = >rxbd[priv->last_rx_bd];
+   if (le32_to_cpu(rxbd->info) & FOR_EMAC) {
+   arc_emac_restart(ndev);
+   priv->rx_missed_errors = 0;
+   }
+   }
+}
+
+/**
  * arc_emac_poll - NAPI poll handler.
  * @napi:  Pointer to napi_struct structure.
  * @budget:How many BDs to process on 1 call.
@@ -272,6 +321,7 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
unsigned int work_done;
 
arc_emac_tx_clean(ndev);
+   arc_emac_rx_miss_handle(ndev);
 
work_done = arc_emac_rx(ndev, budget);
if (work_done < budget) {
@@ -279,6 +329,8 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
arc_reg_or(priv, R_ENABLE, RXINT_MASK | TXINT_MASK);
}
 
+   arc_emac_rx_stall_check(ndev, budget, work_done);
+
return work_done;
 }
 
@@ -320,6 +372,8 @@ static irqreturn_t arc_emac_intr(int irq, void 
*dev_instance)
if (status & MSER_MASK) {
stats->rx_missed_errors += 0x100;
stats->rx_errors += 0x100;
+   priv->rx_missed_errors += 0x100;
+   napi_schedule(>napi);
}
 
if (status & RXCR_MASK) {
@@ -720,6 +774,63 @@ st

[PATCH] net: arc_emac: restart stalled EMAC

[Alexander Kochetkov]

Under certain conditions EMAC stop reception of incoming packets and
continuously increment R_MISS register instead of saving data into
provided buffer. The commit implement workaround for such situation.
Then the stall detected EMAC will be restarted.

On device the stall looks like the device lost it's dynamic IP address.
ifconfig shows that interface error counter rapidly increments.
At the same time on the DHCP server we can see continues DHCP-requests
from device.

In real network stalls happen really rarely. To make them frequent the
broadcast storm[1] should be simulated. For simulation it is necessary
to make following connections:
1. connect radxarock to 1st port of switch
2. connect some PC to 2nd port of switch
3. connect two other free ports together using standard ethernet cable,
   in order to make a switching loop.

After that, is necessary to make a broadcast storm. For example, running on
PC 'ping' to some IP address triggers ARP-request storm. After some
time (~10sec), EMAC on rk3188 will stall.

Observed and tested on rk3188 radxarock.

[1] https://en.wikipedia.org/wiki/Broadcast_radiation

Signed-off-by: Alexander Kochetkov 
---
 drivers/net/ethernet/arc/emac.h  |2 +
 drivers/net/ethernet/arc/emac_main.c |  111 ++
 2 files changed, 113 insertions(+)

diff --git a/drivers/net/ethernet/arc/emac.h b/drivers/net/ethernet/arc/emac.h
index e4feb71..1c7afc5 100644
--- a/drivers/net/ethernet/arc/emac.h
+++ b/drivers/net/ethernet/arc/emac.h
@@ -158,6 +158,8 @@ struct arc_emac_priv {
unsigned int link;
unsigned int duplex;
unsigned int speed;
+
+   unsigned int rx_missed_errors;
 };
 
 /**
diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index 68de2f2..b2e0051 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -26,6 +26,8 @@
 
 #include "emac.h"
 
+static void arc_emac_restart(struct net_device *ndev);
+
 /**
  * arc_emac_tx_avail - Return the number of available slots in the tx ring.
  * @priv: Pointer to ARC EMAC private data structure.
@@ -259,6 +261,53 @@ static int arc_emac_rx(struct net_device *ndev, int budget)
 }
 
 /**
+ * arc_emac_rx_miss_handle - handle R_MISS register
+ * @ndev:  Pointer to the net_device structure.
+ */
+static void arc_emac_rx_miss_handle(struct net_device *ndev)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct net_device_stats *stats = >stats;
+   unsigned int miss;
+
+   miss = arc_reg_get(priv, R_MISS);
+   if (miss) {
+   stats->rx_errors += miss;
+   stats->rx_missed_errors += miss;
+   priv->rx_missed_errors += miss;
+   }
+}
+
+/**
+ * arc_emac_rx_stall_check - check RX stall
+ * @ndev:  Pointer to the net_device structure.
+ * @budget:How many BDs requested to process on 1 call.
+ * @work_done: How many BDs processed
+ *
+ * Under certain conditions EMAC stop reception of incoming packets and
+ * continuously increment R_MISS register instead of saving data into
+ * provided buffer. This function detect that condition and restart
+ * EMAC.
+ */
+static void arc_emac_rx_stall_check(struct net_device *ndev,
+   int budget, unsigned int work_done)
+{
+   struct arc_emac_priv *priv = netdev_priv(ndev);
+   struct arc_emac_bd *rxbd;
+
+   if (work_done)
+   priv->rx_missed_errors = 0;
+
+   if (priv->rx_missed_errors && budget) {
+   rxbd = >rxbd[priv->last_rx_bd];
+   if (le32_to_cpu(rxbd->info) & FOR_EMAC) {
+   arc_emac_restart(ndev);
+   priv->rx_missed_errors = 0;
+   }
+   }
+}
+
+/**
  * arc_emac_poll - NAPI poll handler.
  * @napi:  Pointer to napi_struct structure.
  * @budget:How many BDs to process on 1 call.
@@ -272,6 +321,7 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
unsigned int work_done;
 
arc_emac_tx_clean(ndev);
+   arc_emac_rx_miss_handle(ndev);
 
work_done = arc_emac_rx(ndev, budget);
if (work_done < budget) {
@@ -279,6 +329,8 @@ static int arc_emac_poll(struct napi_struct *napi, int 
budget)
arc_reg_or(priv, R_ENABLE, RXINT_MASK | TXINT_MASK);
}
 
+   arc_emac_rx_stall_check(ndev, budget, work_done);
+
return work_done;
 }
 
@@ -320,6 +372,8 @@ static irqreturn_t arc_emac_intr(int irq, void 
*dev_instance)
if (status & MSER_MASK) {
stats->rx_missed_errors += 0x100;
stats->rx_errors += 0x100;
+   priv->rx_missed_errors += 0x100;
+   napi_schedule(>napi);
}
 
if (status & RXCR_MASK) {
@@ -720,6 +774,63 @@ static i

Re: [PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Hello Vinod!

It looks like the patch did not get enough attention. If no one except me will 
test the patch,
how do we proceed? This patch does not depend on the SOC implementation and the
runtime environment. For me the patch looks fine and will not do harm if it 
will be committed.

Regards,
Alexander.

> 4 окт. 2017 г., в 14:37, Alexander Kochetkov <al.koc...@gmail.com> написал(а):
> 
> If two concurrent threads call pl330_get_desc() when DMAC descriptor
> pool is empty it is possible that allocation for one of threads will fail
> with message:
> 
> kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!
> 
> Here how that can happen. Thread A calls pl330_get_desc() to get
> descriptor. If DMAC descriptor pool is empty pl330_get_desc() allocates
> new descriptor on shared pool using add_desc() and then get newly
> allocated descriptor using pluck_desc(). At the same time thread B calls
> pluck_desc() and take newly allocated descriptor. In that case descriptor
> allocation for thread A will fail.
> 
> Using on-stack pool for new descriptor allow avoid the issue described.
> The patch modify pl330_get_desc() to use on-stack pool for allocation
> new descriptors.
> 
> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> ---
> drivers/dma/pl330.c |   39 ---
> 1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index b19ee04..deec4a4 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2390,7 +2390,8 @@ static inline void _init_desc(struct dma_pl330_desc 
> *desc)
> }
> 
> /* Returns the number of descriptors added to the DMAC pool */
> -static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
> +static int add_desc(struct list_head *pool, spinlock_t *lock,
> + gfp_t flg, int count)
> {
>   struct dma_pl330_desc *desc;
>   unsigned long flags;
> @@ -2400,27 +2401,28 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t 
> flg, int count)
>   if (!desc)
>   return 0;
> 
> - spin_lock_irqsave(>pool_lock, flags);
> + spin_lock_irqsave(lock, flags);
> 
>   for (i = 0; i < count; i++) {
>   _init_desc([i]);
> - list_add_tail([i].node, >desc_pool);
> + list_add_tail([i].node, pool);
>   }
> 
> - spin_unlock_irqrestore(>pool_lock, flags);
> + spin_unlock_irqrestore(lock, flags);
> 
>   return count;
> }
> 
> -static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
> +static struct dma_pl330_desc *pluck_desc(struct list_head *pool,
> +  spinlock_t *lock)
> {
>   struct dma_pl330_desc *desc = NULL;
>   unsigned long flags;
> 
> - spin_lock_irqsave(>pool_lock, flags);
> + spin_lock_irqsave(lock, flags);
> 
> - if (!list_empty(>desc_pool)) {
> - desc = list_entry(pl330->desc_pool.next,
> + if (!list_empty(pool)) {
> + desc = list_entry(pool->next,
>   struct dma_pl330_desc, node);
> 
>   list_del_init(>node);
> @@ -2429,7 +2431,7 @@ static struct dma_pl330_desc *pluck_desc(struct 
> pl330_dmac *pl330)
>   desc->txd.callback = NULL;
>   }
> 
> - spin_unlock_irqrestore(>pool_lock, flags);
> + spin_unlock_irqrestore(lock, flags);
> 
>   return desc;
> }
> @@ -2441,20 +2443,18 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
> dma_pl330_chan *pch)
>   struct dma_pl330_desc *desc;
> 
>   /* Pluck one desc from the pool of DMAC */
> - desc = pluck_desc(pl330);
> + desc = pluck_desc(>desc_pool, >pool_lock);
> 
>   /* If the DMAC pool is empty, alloc new */
>   if (!desc) {
> - if (!add_desc(pl330, GFP_ATOMIC, 1))
> - return NULL;
> + DEFINE_SPINLOCK(lock);
> + LIST_HEAD(pool);
> 
> - /* Try again */
> - desc = pluck_desc(pl330);
> - if (!desc) {
> - dev_err(pch->dmac->ddma.dev,
> - "%s:%d ALERT!\n", __func__, __LINE__);
> + if (!add_desc(, , GFP_ATOMIC, 1))
>   return NULL;
> - }
> +
> + desc = pluck_desc(, );
> + WARN_ON(!desc || !list_empty());
>   }
> 
>   /* Initialize the descriptor */
> @@ -2868,7 +2868,8 @@ static int __maybe_unused pl330_resume(struct device 
> *dev)
>   spin_lock_init(>pool_lock);
> 
>   /* Create a descriptor pool of default size */
> - if (!add_desc(pl330, GFP_KERNEL, NR_DEFAULT_DESC))
> + if (!add_desc(>desc_pool, >pool_lock,
> +   GFP_KERNEL, NR_DEFAULT_DESC))
>   dev_warn(>dev, "unable to allocate desc\n");
> 
>   INIT_LIST_HEAD(>channels);
> -- 
> 1.7.9.5
> 

Re: [PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Hello Vinod!

It looks like the patch did not get enough attention. If no one except me will 
test the patch,
how do we proceed? This patch does not depend on the SOC implementation and the
runtime environment. For me the patch looks fine and will not do harm if it 
will be committed.

Regards,
Alexander.

> 4 окт. 2017 г., в 14:37, Alexander Kochetkov  написал(а):
> 
> If two concurrent threads call pl330_get_desc() when DMAC descriptor
> pool is empty it is possible that allocation for one of threads will fail
> with message:
> 
> kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!
> 
> Here how that can happen. Thread A calls pl330_get_desc() to get
> descriptor. If DMAC descriptor pool is empty pl330_get_desc() allocates
> new descriptor on shared pool using add_desc() and then get newly
> allocated descriptor using pluck_desc(). At the same time thread B calls
> pluck_desc() and take newly allocated descriptor. In that case descriptor
> allocation for thread A will fail.
> 
> Using on-stack pool for new descriptor allow avoid the issue described.
> The patch modify pl330_get_desc() to use on-stack pool for allocation
> new descriptors.
> 
> Signed-off-by: Alexander Kochetkov 
> ---
> drivers/dma/pl330.c |   39 ---
> 1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index b19ee04..deec4a4 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2390,7 +2390,8 @@ static inline void _init_desc(struct dma_pl330_desc 
> *desc)
> }
> 
> /* Returns the number of descriptors added to the DMAC pool */
> -static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
> +static int add_desc(struct list_head *pool, spinlock_t *lock,
> + gfp_t flg, int count)
> {
>   struct dma_pl330_desc *desc;
>   unsigned long flags;
> @@ -2400,27 +2401,28 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t 
> flg, int count)
>   if (!desc)
>   return 0;
> 
> - spin_lock_irqsave(>pool_lock, flags);
> + spin_lock_irqsave(lock, flags);
> 
>   for (i = 0; i < count; i++) {
>   _init_desc([i]);
> - list_add_tail([i].node, >desc_pool);
> + list_add_tail([i].node, pool);
>   }
> 
> - spin_unlock_irqrestore(>pool_lock, flags);
> + spin_unlock_irqrestore(lock, flags);
> 
>   return count;
> }
> 
> -static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
> +static struct dma_pl330_desc *pluck_desc(struct list_head *pool,
> +  spinlock_t *lock)
> {
>   struct dma_pl330_desc *desc = NULL;
>   unsigned long flags;
> 
> - spin_lock_irqsave(>pool_lock, flags);
> + spin_lock_irqsave(lock, flags);
> 
> - if (!list_empty(>desc_pool)) {
> - desc = list_entry(pl330->desc_pool.next,
> + if (!list_empty(pool)) {
> + desc = list_entry(pool->next,
>   struct dma_pl330_desc, node);
> 
>   list_del_init(>node);
> @@ -2429,7 +2431,7 @@ static struct dma_pl330_desc *pluck_desc(struct 
> pl330_dmac *pl330)
>   desc->txd.callback = NULL;
>   }
> 
> - spin_unlock_irqrestore(>pool_lock, flags);
> + spin_unlock_irqrestore(lock, flags);
> 
>   return desc;
> }
> @@ -2441,20 +2443,18 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
> dma_pl330_chan *pch)
>   struct dma_pl330_desc *desc;
> 
>   /* Pluck one desc from the pool of DMAC */
> - desc = pluck_desc(pl330);
> + desc = pluck_desc(>desc_pool, >pool_lock);
> 
>   /* If the DMAC pool is empty, alloc new */
>   if (!desc) {
> - if (!add_desc(pl330, GFP_ATOMIC, 1))
> - return NULL;
> + DEFINE_SPINLOCK(lock);
> + LIST_HEAD(pool);
> 
> - /* Try again */
> - desc = pluck_desc(pl330);
> - if (!desc) {
> - dev_err(pch->dmac->ddma.dev,
> - "%s:%d ALERT!\n", __func__, __LINE__);
> + if (!add_desc(, , GFP_ATOMIC, 1))
>   return NULL;
> - }
> +
> + desc = pluck_desc(, );
> + WARN_ON(!desc || !list_empty());
>   }
> 
>   /* Initialize the descriptor */
> @@ -2868,7 +2868,8 @@ static int __maybe_unused pl330_resume(struct device 
> *dev)
>   spin_lock_init(>pool_lock);
> 
>   /* Create a descriptor pool of default size */
> - if (!add_desc(pl330, GFP_KERNEL, NR_DEFAULT_DESC))
> + if (!add_desc(>desc_pool, >pool_lock,
> +   GFP_KERNEL, NR_DEFAULT_DESC))
>   dev_warn(>dev, "unable to allocate desc\n");
> 
>   INIT_LIST_HEAD(>channels);
> -- 
> 1.7.9.5
> 

[PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

If two concurrent threads call pl330_get_desc() when DMAC descriptor
pool is empty it is possible that allocation for one of threads will fail
with message:

kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!

Here how that can happen. Thread A calls pl330_get_desc() to get
descriptor. If DMAC descriptor pool is empty pl330_get_desc() allocates
new descriptor on shared pool using add_desc() and then get newly
allocated descriptor using pluck_desc(). At the same time thread B calls
pluck_desc() and take newly allocated descriptor. In that case descriptor
allocation for thread A will fail.

Using on-stack pool for new descriptor allow avoid the issue described.
The patch modify pl330_get_desc() to use on-stack pool for allocation
new descriptors.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/dma/pl330.c |   39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b19ee04..deec4a4 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2390,7 +2390,8 @@ static inline void _init_desc(struct dma_pl330_desc *desc)
 }
 
 /* Returns the number of descriptors added to the DMAC pool */
-static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+static int add_desc(struct list_head *pool, spinlock_t *lock,
+   gfp_t flg, int count)
 {
struct dma_pl330_desc *desc;
unsigned long flags;
@@ -2400,27 +2401,28 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t 
flg, int count)
if (!desc)
return 0;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
for (i = 0; i < count; i++) {
_init_desc([i]);
-   list_add_tail([i].node, >desc_pool);
+   list_add_tail([i].node, pool);
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return count;
 }
 
-static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+static struct dma_pl330_desc *pluck_desc(struct list_head *pool,
+spinlock_t *lock)
 {
struct dma_pl330_desc *desc = NULL;
unsigned long flags;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
-   if (!list_empty(>desc_pool)) {
-   desc = list_entry(pl330->desc_pool.next,
+   if (!list_empty(pool)) {
+   desc = list_entry(pool->next,
struct dma_pl330_desc, node);
 
list_del_init(>node);
@@ -2429,7 +2431,7 @@ static struct dma_pl330_desc *pluck_desc(struct 
pl330_dmac *pl330)
desc->txd.callback = NULL;
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return desc;
 }
@@ -2441,20 +2443,18 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
dma_pl330_chan *pch)
struct dma_pl330_desc *desc;
 
/* Pluck one desc from the pool of DMAC */
-   desc = pluck_desc(pl330);
+   desc = pluck_desc(>desc_pool, >pool_lock);
 
/* If the DMAC pool is empty, alloc new */
if (!desc) {
-   if (!add_desc(pl330, GFP_ATOMIC, 1))
-   return NULL;
+   DEFINE_SPINLOCK(lock);
+   LIST_HEAD(pool);
 
-   /* Try again */
-   desc = pluck_desc(pl330);
-   if (!desc) {
-   dev_err(pch->dmac->ddma.dev,
-   "%s:%d ALERT!\n", __func__, __LINE__);
+   if (!add_desc(, , GFP_ATOMIC, 1))
return NULL;
-   }
+
+   desc = pluck_desc(, );
+   WARN_ON(!desc || !list_empty());
}
 
/* Initialize the descriptor */
@@ -2868,7 +2868,8 @@ static int __maybe_unused pl330_resume(struct device *dev)
spin_lock_init(>pool_lock);
 
/* Create a descriptor pool of default size */
-   if (!add_desc(pl330, GFP_KERNEL, NR_DEFAULT_DESC))
+   if (!add_desc(>desc_pool, >pool_lock,
+ GFP_KERNEL, NR_DEFAULT_DESC))
dev_warn(>dev, "unable to allocate desc\n");
 
INIT_LIST_HEAD(>channels);
-- 
1.7.9.5

[PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

If two concurrent threads call pl330_get_desc() when DMAC descriptor
pool is empty it is possible that allocation for one of threads will fail
with message:

kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!

Here how that can happen. Thread A calls pl330_get_desc() to get
descriptor. If DMAC descriptor pool is empty pl330_get_desc() allocates
new descriptor on shared pool using add_desc() and then get newly
allocated descriptor using pluck_desc(). At the same time thread B calls
pluck_desc() and take newly allocated descriptor. In that case descriptor
allocation for thread A will fail.

Using on-stack pool for new descriptor allow avoid the issue described.
The patch modify pl330_get_desc() to use on-stack pool for allocation
new descriptors.

Signed-off-by: Alexander Kochetkov 
---
 drivers/dma/pl330.c |   39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b19ee04..deec4a4 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2390,7 +2390,8 @@ static inline void _init_desc(struct dma_pl330_desc *desc)
 }
 
 /* Returns the number of descriptors added to the DMAC pool */
-static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+static int add_desc(struct list_head *pool, spinlock_t *lock,
+   gfp_t flg, int count)
 {
struct dma_pl330_desc *desc;
unsigned long flags;
@@ -2400,27 +2401,28 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t 
flg, int count)
if (!desc)
return 0;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
for (i = 0; i < count; i++) {
_init_desc([i]);
-   list_add_tail([i].node, >desc_pool);
+   list_add_tail([i].node, pool);
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return count;
 }
 
-static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+static struct dma_pl330_desc *pluck_desc(struct list_head *pool,
+spinlock_t *lock)
 {
struct dma_pl330_desc *desc = NULL;
unsigned long flags;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
-   if (!list_empty(>desc_pool)) {
-   desc = list_entry(pl330->desc_pool.next,
+   if (!list_empty(pool)) {
+   desc = list_entry(pool->next,
struct dma_pl330_desc, node);
 
list_del_init(>node);
@@ -2429,7 +2431,7 @@ static struct dma_pl330_desc *pluck_desc(struct 
pl330_dmac *pl330)
desc->txd.callback = NULL;
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return desc;
 }
@@ -2441,20 +2443,18 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
dma_pl330_chan *pch)
struct dma_pl330_desc *desc;
 
/* Pluck one desc from the pool of DMAC */
-   desc = pluck_desc(pl330);
+   desc = pluck_desc(>desc_pool, >pool_lock);
 
/* If the DMAC pool is empty, alloc new */
if (!desc) {
-   if (!add_desc(pl330, GFP_ATOMIC, 1))
-   return NULL;
+   DEFINE_SPINLOCK(lock);
+   LIST_HEAD(pool);
 
-   /* Try again */
-   desc = pluck_desc(pl330);
-   if (!desc) {
-   dev_err(pch->dmac->ddma.dev,
-   "%s:%d ALERT!\n", __func__, __LINE__);
+   if (!add_desc(, , GFP_ATOMIC, 1))
return NULL;
-   }
+
+   desc = pluck_desc(, );
+   WARN_ON(!desc || !list_empty());
}
 
/* Initialize the descriptor */
@@ -2868,7 +2868,8 @@ static int __maybe_unused pl330_resume(struct device *dev)
spin_lock_init(>pool_lock);
 
/* Create a descriptor pool of default size */
-   if (!add_desc(pl330, GFP_KERNEL, NR_DEFAULT_DESC))
+   if (!add_desc(>desc_pool, >pool_lock,
+ GFP_KERNEL, NR_DEFAULT_DESC))
dev_warn(>dev, "unable to allocate desc\n");
 
INIT_LIST_HEAD(>channels);
-- 
1.7.9.5

[PATCH v2 2/2] !!! FOR TESTING ONLY !!! dmaengine: pl330: add verbose message and set NR_DEFAULT_DESC to 1

[Alexander Kochetkov]

Commit add verbose output to pl330 showing what changes introduced by
commit 1/2 from series work as expected. You should see similar output
running modified kernel:

The patch tested on rk3188 radxdarock. Could someone else test it on
other hardware with pl330 DMA?

root@host:~# dmesg | grep pl330
[0.277520] dma-pl330 20018000.dma-controller: Loaded driver for PL330 
DMAC-241330
[0.277538] dma-pl330 20018000.dma-controller:   DBUFF-32x8bytes 
Num_Chans-6 Num_Peri-12 Num_Events-12
[0.279894] dma-pl330 20078000.dma-controller: Loaded driver for PL330 
DMAC-241330
[0.279910] dma-pl330 20078000.dma-controller:   DBUFF-64x8bytes 
Num_Chans-7 Num_Peri-20 Num_Events-14
[1.344804] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344832] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344853] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344873] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344893] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344912] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
--- rest of similar lines omitted ---

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/dma/pl330.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index deec4a4..3441c16 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -266,7 +266,7 @@ enum pl330_byteswap {
 
 /* The number of default descriptors */
 
-#define NR_DEFAULT_DESC16
+#define NR_DEFAULT_DESC1
 
 /* Delay for runtime PM autosuspend, ms */
 #define PL330_AUTOSUSPEND_DELAY 20
@@ -2455,6 +2455,9 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
dma_pl330_chan *pch)
 
desc = pluck_desc(, );
WARN_ON(!desc || !list_empty());
+
+   dev_err(pch->dmac->ddma.dev, "%s:%d Allocated one more 
descriptor\n",
+   __func__, __LINE__);
}
 
/* Initialize the descriptor */
-- 
1.7.9.5

[PATCH v2 2/2] !!! FOR TESTING ONLY !!! dmaengine: pl330: add verbose message and set NR_DEFAULT_DESC to 1

[Alexander Kochetkov]

Commit add verbose output to pl330 showing what changes introduced by
commit 1/2 from series work as expected. You should see similar output
running modified kernel:

The patch tested on rk3188 radxdarock. Could someone else test it on
other hardware with pl330 DMA?

root@host:~# dmesg | grep pl330
[0.277520] dma-pl330 20018000.dma-controller: Loaded driver for PL330 
DMAC-241330
[0.277538] dma-pl330 20018000.dma-controller:   DBUFF-32x8bytes 
Num_Chans-6 Num_Peri-12 Num_Events-12
[0.279894] dma-pl330 20078000.dma-controller: Loaded driver for PL330 
DMAC-241330
[0.279910] dma-pl330 20078000.dma-controller:   DBUFF-64x8bytes 
Num_Chans-7 Num_Peri-20 Num_Events-14
[1.344804] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344832] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344853] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344873] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344893] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
[1.344912] dma-pl330 20078000.dma-controller: pl330_get_desc:2458 Allocated 
one more descriptor
--- rest of similar lines omitted ---

Signed-off-by: Alexander Kochetkov 
---
 drivers/dma/pl330.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index deec4a4..3441c16 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -266,7 +266,7 @@ enum pl330_byteswap {
 
 /* The number of default descriptors */
 
-#define NR_DEFAULT_DESC16
+#define NR_DEFAULT_DESC1
 
 /* Delay for runtime PM autosuspend, ms */
 #define PL330_AUTOSUSPEND_DELAY 20
@@ -2455,6 +2455,9 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
dma_pl330_chan *pch)
 
desc = pluck_desc(, );
WARN_ON(!desc || !list_empty());
+
+   dev_err(pch->dmac->ddma.dev, "%s:%d Allocated one more 
descriptor\n",
+   __func__, __LINE__);
}
 
/* Initialize the descriptor */
-- 
1.7.9.5

[PATCH v2 0/2] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Here is the patch fixing descriptor allocation issue.

Could someone with pl330 hardware test it and confirm that
it doesn't brake current pl330 driver? I will be very grateful
to you!

Changes in v2:
- removed wrappers add_desc(), pluck_desc()
- fix code intendation 

Alexander Kochetkov (2):
  dmaengine: pl330: fix descriptor allocation fail
  !!! FOR TESTING ONLY !!! dmaengine: pl330: add verbose message and
set NR_DEFAULT_DESC to 1

 drivers/dma/pl330.c |   44 
 1 file changed, 24 insertions(+), 20 deletions(-)

-- 
1.7.9.5

[PATCH v2 0/2] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Here is the patch fixing descriptor allocation issue.

Could someone with pl330 hardware test it and confirm that
it doesn't brake current pl330 driver? I will be very grateful
to you!

Changes in v2:
- removed wrappers add_desc(), pluck_desc()
- fix code intendation 

Alexander Kochetkov (2):
  dmaengine: pl330: fix descriptor allocation fail
  !!! FOR TESTING ONLY !!! dmaengine: pl330: add verbose message and
set NR_DEFAULT_DESC to 1

 drivers/dma/pl330.c |   44 
 1 file changed, 24 insertions(+), 20 deletions(-)

-- 
1.7.9.5

Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Hello Vinod! Thanks for review!

> 26 сент. 2017 г., в 20:37, Vinod Koul  написал(а):
> 
> Tested-by please...

In order to test the patch the driver should be rebuild with NR_DEFAULT_DESC 
defined to 1
and with some trace code included. Is it OK if I provide second patch I used 
for testing
with trace showing how change work?

> one more wrapper why, we dont have any logic here!
The idea was to keep rest of driver code intact. Ok, I’ll send v2 with no 
wrappers.

> right justifed please


Some functions has two tabs on second line, some has alignment to beginning of
argument declaration. How correct?

1) or like this (two tabs)
static int add_desc(struct list_head *pool, spinlock_t *lock,
gfp_t flg, int count)

2) Like this:
static int add_desc(struct list_head *pool, spinlock_t *lock,
   gfp_t flg, int count)

Regards,
Alexander.

Re: [PATCH] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Hello Vinod! Thanks for review!

> 26 сент. 2017 г., в 20:37, Vinod Koul  написал(а):
> 
> Tested-by please...

In order to test the patch the driver should be rebuild with NR_DEFAULT_DESC 
defined to 1
and with some trace code included. Is it OK if I provide second patch I used 
for testing
with trace showing how change work?

> one more wrapper why, we dont have any logic here!
The idea was to keep rest of driver code intact. Ok, I’ll send v2 with no 
wrappers.

> right justifed please


Some functions has two tabs on second line, some has alignment to beginning of
argument declaration. How correct?

1) or like this (two tabs)
static int add_desc(struct list_head *pool, spinlock_t *lock,
gfp_t flg, int count)

2) Like this:
static int add_desc(struct list_head *pool, spinlock_t *lock,
   gfp_t flg, int count)

Regards,
Alexander.

[PATCH] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Thread A calls pl330_get_desc() to get descriptor. If DMAC descriptor
pool is empty pl330_get_desc() allocates new descriptor using add_desc()
and then get newly allocated descriptor using pluck_desc().
It is possible that another concurrent thread B calls pluck_desc()
and catch newly allocated descriptor. In that case descriptor allocation
for thread A will fail with:

kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!

The commit fix the issue by calling _add_desc() to allocate new descriptor
to the local on stack pool and than get it from local pool. So the issue
described will nether happen.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/dma/pl330.c |   44 +++-
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index f37f497..0e7f6c9 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2417,7 +2417,8 @@ static inline void _init_desc(struct dma_pl330_desc *desc)
 }
 
 /* Returns the number of descriptors added to the DMAC pool */
-static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+static int _add_desc(struct list_head *pool, spinlock_t *lock,
+   gfp_t flg, int count)
 {
struct dma_pl330_desc *desc;
unsigned long flags;
@@ -2427,27 +2428,33 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t 
flg, int count)
if (!desc)
return 0;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
for (i = 0; i < count; i++) {
_init_desc([i]);
-   list_add_tail([i].node, >desc_pool);
+   list_add_tail([i].node, pool);
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return count;
 }
 
-static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+{
+   return _add_desc(>desc_pool, >pool_lock, flg, count);
+}
+
+static struct dma_pl330_desc *_pluck_desc(struct list_head *pool,
+   spinlock_t *lock)
 {
struct dma_pl330_desc *desc = NULL;
unsigned long flags;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
-   if (!list_empty(>desc_pool)) {
-   desc = list_entry(pl330->desc_pool.next,
+   if (!list_empty(pool)) {
+   desc = list_entry(pool->next,
struct dma_pl330_desc, node);
 
list_del_init(>node);
@@ -2456,11 +2463,16 @@ static struct dma_pl330_desc *pluck_desc(struct 
pl330_dmac *pl330)
desc->txd.callback = NULL;
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return desc;
 }
 
+static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+{
+   return _pluck_desc(>desc_pool, >pool_lock);
+}
+
 static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 {
struct pl330_dmac *pl330 = pch->dmac;
@@ -2472,16 +2484,14 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
dma_pl330_chan *pch)
 
/* If the DMAC pool is empty, alloc new */
if (!desc) {
-   if (!add_desc(pl330, GFP_ATOMIC, 1))
-   return NULL;
+   DEFINE_SPINLOCK(lock);
+   LIST_HEAD(pool);
 
-   /* Try again */
-   desc = pluck_desc(pl330);
-   if (!desc) {
-   dev_err(pch->dmac->ddma.dev,
-   "%s:%d ALERT!\n", __func__, __LINE__);
+   if (!_add_desc(, , GFP_ATOMIC, 1))
return NULL;
-   }
+
+   desc = _pluck_desc(, );
+   WARN_ON(!desc || !list_empty());
}
 
/* Initialize the descriptor */
-- 
1.7.9.5

[PATCH] dmaengine: pl330: fix descriptor allocation fail

[Alexander Kochetkov]

Thread A calls pl330_get_desc() to get descriptor. If DMAC descriptor
pool is empty pl330_get_desc() allocates new descriptor using add_desc()
and then get newly allocated descriptor using pluck_desc().
It is possible that another concurrent thread B calls pluck_desc()
and catch newly allocated descriptor. In that case descriptor allocation
for thread A will fail with:

kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!

The commit fix the issue by calling _add_desc() to allocate new descriptor
to the local on stack pool and than get it from local pool. So the issue
described will nether happen.

Signed-off-by: Alexander Kochetkov 
---
 drivers/dma/pl330.c |   44 +++-
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index f37f497..0e7f6c9 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2417,7 +2417,8 @@ static inline void _init_desc(struct dma_pl330_desc *desc)
 }
 
 /* Returns the number of descriptors added to the DMAC pool */
-static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+static int _add_desc(struct list_head *pool, spinlock_t *lock,
+   gfp_t flg, int count)
 {
struct dma_pl330_desc *desc;
unsigned long flags;
@@ -2427,27 +2428,33 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t 
flg, int count)
if (!desc)
return 0;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
for (i = 0; i < count; i++) {
_init_desc([i]);
-   list_add_tail([i].node, >desc_pool);
+   list_add_tail([i].node, pool);
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return count;
 }
 
-static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+{
+   return _add_desc(>desc_pool, >pool_lock, flg, count);
+}
+
+static struct dma_pl330_desc *_pluck_desc(struct list_head *pool,
+   spinlock_t *lock)
 {
struct dma_pl330_desc *desc = NULL;
unsigned long flags;
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(lock, flags);
 
-   if (!list_empty(>desc_pool)) {
-   desc = list_entry(pl330->desc_pool.next,
+   if (!list_empty(pool)) {
+   desc = list_entry(pool->next,
struct dma_pl330_desc, node);
 
list_del_init(>node);
@@ -2456,11 +2463,16 @@ static struct dma_pl330_desc *pluck_desc(struct 
pl330_dmac *pl330)
desc->txd.callback = NULL;
}
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(lock, flags);
 
return desc;
 }
 
+static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+{
+   return _pluck_desc(>desc_pool, >pool_lock);
+}
+
 static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 {
struct pl330_dmac *pl330 = pch->dmac;
@@ -2472,16 +2484,14 @@ static struct dma_pl330_desc *pl330_get_desc(struct 
dma_pl330_chan *pch)
 
/* If the DMAC pool is empty, alloc new */
if (!desc) {
-   if (!add_desc(pl330, GFP_ATOMIC, 1))
-   return NULL;
+   DEFINE_SPINLOCK(lock);
+   LIST_HEAD(pool);
 
-   /* Try again */
-   desc = pluck_desc(pl330);
-   if (!desc) {
-   dev_err(pch->dmac->ddma.dev,
-   "%s:%d ALERT!\n", __func__, __LINE__);
+   if (!_add_desc(, , GFP_ATOMIC, 1))
return NULL;
-   }
+
+   desc = _pluck_desc(, );
+   WARN_ON(!desc || !list_empty());
}
 
/* Initialize the descriptor */
-- 
1.7.9.5

Re: [PATCH 0/2] Free running cyclic transfer implementation for pl330

[Alexander Kochetkov]

Hello!

Just to let you know, that I got following test report from Stephen (thanks a 
lot!).

The patch won’t work due to full cyclic transfer doesn’t fit into mcbufsz (256 
bytes long).
His application requested driver to do cyclic transfer with large number of 
cycles.
pl330 microcode has restriction on how far PC can change and this limit is 256 
bytes,
so increasing mcbufsz will not solve problem. Don’t want to make jump bridges or
something like on the fly CPU modified microcode.

> 14 апр. 2017 г., в 23:24, Stephen Barber  написал(а):
> 
> Hi Alexander,
> 
> Thanks for your patches!
> 
> I gave them a try on kevin (Samsung Chromebook Plus) with our
> chromeos-4.4 kernel branch, plus some cherry-picks on top to get
> things to apply cleanly. Here's what my tree looks like:
> 
> 344daf13fa4e (HEAD -> apply-pl330) dmaengine: pl330: don't emit code
> for one iteration loop
> 062596b83fec dmaengine: pl330: make cyclic transfer free runnable
> 0e75f2647b8e dmaengine: pl330: fix double lock
> 7d22b54b79f2 dmaengine: pl330: remove unused ‘regs’
> 8769d7115cec dmaengine: pl330: do not generate unaligned access
> 65ad077f685b dmaengine: pl330: convert callback to helper function
> 368e7aa6dffd dmaengine: add support to provide error result from a DMA
> transation
> 8f8afe84472f dmaengine: Add helper function to prep for error reporting
> 2acc1e704232 dmaengine: pl330: explicitly freeup irq
> ed36cde14cf0 (m/master, cros/chromeos-4.4) UPSTREAM: audit: add tty
> field to LOGIN event
> 
> Unfortunately when I start playing audio, things don't seem to work :(
> Rolling back HEAD to "dmaengine: pl330: fix double lock" works though.
> 
> The only thing I get from dmesg relevant to pl330 is this:
> [   59.203375] dma-pl330 ff6d.dma-controller:
> pl330_submit_req:1498 Try increasing mcbufsz (12810/256)
> [   59.203395] dma-pl330 ff6d.dma-controller: fill_queue:2023 Bad Desc(2)
> [   63.837390] dma-pl330 ff6d.dma-controller:
> pl330_submit_req:1498 Try increasing mcbufsz (12810/256)
> [   63.837410] dma-pl330 ff6d.dma-controller: fill_queue:2023 Bad Desc(2)
> 
> Thanks,
> Steve
> 


> 14 апр. 2017 г., в 21:04, Krzysztof Kozlowski  написал(а):
> 
> Let me know if you need more data. I wonder why you haven't experience
> this?

I don’t have idea why this might happen on Odroid and due to the fact the patch 
don’t
work for Stephen and I don’t have another idea how to implement that, it is 
better to
leave the problem along.

Krzysztof, thanks a lot for help. Really.

Regards,
Alexander.

Re: [PATCH 0/2] Free running cyclic transfer implementation for pl330

[Alexander Kochetkov]

Hello!

Just to let you know, that I got following test report from Stephen (thanks a 
lot!).

The patch won’t work due to full cyclic transfer doesn’t fit into mcbufsz (256 
bytes long).
His application requested driver to do cyclic transfer with large number of 
cycles.
pl330 microcode has restriction on how far PC can change and this limit is 256 
bytes,
so increasing mcbufsz will not solve problem. Don’t want to make jump bridges or
something like on the fly CPU modified microcode.

> 14 апр. 2017 г., в 23:24, Stephen Barber  написал(а):
> 
> Hi Alexander,
> 
> Thanks for your patches!
> 
> I gave them a try on kevin (Samsung Chromebook Plus) with our
> chromeos-4.4 kernel branch, plus some cherry-picks on top to get
> things to apply cleanly. Here's what my tree looks like:
> 
> 344daf13fa4e (HEAD -> apply-pl330) dmaengine: pl330: don't emit code
> for one iteration loop
> 062596b83fec dmaengine: pl330: make cyclic transfer free runnable
> 0e75f2647b8e dmaengine: pl330: fix double lock
> 7d22b54b79f2 dmaengine: pl330: remove unused ‘regs’
> 8769d7115cec dmaengine: pl330: do not generate unaligned access
> 65ad077f685b dmaengine: pl330: convert callback to helper function
> 368e7aa6dffd dmaengine: add support to provide error result from a DMA
> transation
> 8f8afe84472f dmaengine: Add helper function to prep for error reporting
> 2acc1e704232 dmaengine: pl330: explicitly freeup irq
> ed36cde14cf0 (m/master, cros/chromeos-4.4) UPSTREAM: audit: add tty
> field to LOGIN event
> 
> Unfortunately when I start playing audio, things don't seem to work :(
> Rolling back HEAD to "dmaengine: pl330: fix double lock" works though.
> 
> The only thing I get from dmesg relevant to pl330 is this:
> [   59.203375] dma-pl330 ff6d.dma-controller:
> pl330_submit_req:1498 Try increasing mcbufsz (12810/256)
> [   59.203395] dma-pl330 ff6d.dma-controller: fill_queue:2023 Bad Desc(2)
> [   63.837390] dma-pl330 ff6d.dma-controller:
> pl330_submit_req:1498 Try increasing mcbufsz (12810/256)
> [   63.837410] dma-pl330 ff6d.dma-controller: fill_queue:2023 Bad Desc(2)
> 
> Thanks,
> Steve
> 


> 14 апр. 2017 г., в 21:04, Krzysztof Kozlowski  написал(а):
> 
> Let me know if you need more data. I wonder why you haven't experience
> this?

I don’t have idea why this might happen on Odroid and due to the fact the patch 
don’t
work for Stephen and I don’t have another idea how to implement that, it is 
better to
leave the problem along.

Krzysztof, thanks a lot for help. Really.

Regards,
Alexander.

Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Hello David!

> 25 апр. 2017 г., в 17:36, David Miller  написал(а):
> 
> So... what are we doing here?
> 
> My understanding is that this should fix the same problem that commit
> 99f81afc139c6edd14d77a91ee91685a414a1c66 ("phy: micrel: Disable auto
> negotiation on startup") fixed and that this micrel commit should thus
> be reverted to improve MAC startup times which regressed.
> 
> Florian, any guidance?

Yes, this should be done.

I aksed Alexandre to check if 99f81afc139c6edd14d77a91ee91685a414a1c66 ("phy: 
micrel: Disable auto
negotiation on startup») can be reverted, and he answered what it may do that
sometime this/next week.

Alexander.

Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Hello David!

> 25 апр. 2017 г., в 17:36, David Miller  написал(а):
> 
> So... what are we doing here?
> 
> My understanding is that this should fix the same problem that commit
> 99f81afc139c6edd14d77a91ee91685a414a1c66 ("phy: micrel: Disable auto
> negotiation on startup") fixed and that this micrel commit should thus
> be reverted to improve MAC startup times which regressed.
> 
> Florian, any guidance?

Yes, this should be done.

I aksed Alexandre to check if 99f81afc139c6edd14d77a91ee91685a414a1c66 ("phy: 
micrel: Disable auto
negotiation on startup») can be reverted, and he answered what it may do that
sometime this/next week.

Alexander.

Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

> 21 апр. 2017 г., в 17:18, Roger Quadros  написал(а):
> 
> I think the following commit broke functionality with interrupt driven PHYs
> 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not 
> polling.")

Probably this one[1] broke, according to Alexandre’s commit[2].
And it was since Nov 16 2015. But it was hidden by some other commits.

For Roger problem became visible after 3c293f4e08b5 ("net: phy:
Trigger state machine on state change and not polling.»),

For my problem became visible after 529ed1275263 ("net: phy: phy drivers
should not set SUPPORTED_[Asym_]Pause»). As commit 529ed1275263 
removed SUPPORTED_Pause flag from PHY advertising property and
genphy_config_aneg() began to skip PHY auto-negotiation.

Alexander.

[1] Fixes: 321beec5047a (net: phy: Use interrupts when available in NOLINK 
state)
 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=321beec5047af83db90c88114b7e664b156f49fe
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99f81afc139c6edd14d77a91ee91685a414a1c66

Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

> 21 апр. 2017 г., в 17:18, Roger Quadros  написал(а):
> 
> I think the following commit broke functionality with interrupt driven PHYs
> 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not 
> polling.")

Probably this one[1] broke, according to Alexandre’s commit[2].
And it was since Nov 16 2015. But it was hidden by some other commits.

For Roger problem became visible after 3c293f4e08b5 ("net: phy:
Trigger state machine on state change and not polling.»),

For my problem became visible after 529ed1275263 ("net: phy: phy drivers
should not set SUPPORTED_[Asym_]Pause»). As commit 529ed1275263 
removed SUPPORTED_Pause flag from PHY advertising property and
genphy_config_aneg() began to skip PHY auto-negotiation.

Alexander.

[1] Fixes: 321beec5047a (net: phy: Use interrupts when available in NOLINK 
state)
 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=321beec5047af83db90c88114b7e664b156f49fe
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99f81afc139c6edd14d77a91ee91685a414a1c66

Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Hi, Alexandre!

Just found that you've fixed similar problem for Micrel PHY:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99f81afc139c6edd14d77a91ee91685a414a1c66

Could you please test if my patch[1] fix your problem?

As reverting your patch will speedup MAC startup time for boards with Micrel PHY
on 3~5 sec (auto-negotiation time[2]). Could you check that also?

Regards,
Alexander.

[1] https://lkml.org/lkml/2017/4/20/357
[2] http://www.ieee802.org/3/af/public/jan02/brown_1_0102.pdf


> 20 апр. 2017 г., в 14:00, Alexander Kochetkov <al.koc...@gmail.com> 
> написал(а):
> 
> The Ethernet link on an interrupt driven PHY was not coming up if the Ethernet
> cable was plugged before the Ethernet interface was brought up.
> 
> The patch trigger PHY state machine to update link state if PHY was requested 
> to
> do auto-negotiation and auto-negotiation complete flag already set.
> 
> During power-up cycle the PHY do auto-negotiation, generate interrupt and set
> auto-negotiation complete flag. Interrupt is handled by PHY state machine but
> doesn't update link state because PHY is in PHY_READY state. After some time
> MAC bring up, start and request PHY to do auto-negotiation. If there are no 
> new
> settings to advertise genphy_config_aneg() doesn't start PHY auto-negotiation.
> PHY continue to stay in auto-negotiation complete state and doesn't fire
> interrupt. At the same time PHY state machine expect that PHY started
> auto-negotiation and is waiting for interrupt from PHY and it won't get it.
> 
> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> Cc: stable <sta...@vger.kernel.org> # v4.9+
> ---
> drivers/net/phy/phy.c |   40 
> include/linux/phy.h   |1 +
> 2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7cc1b7d..2d9975b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -591,16 +591,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct 
> ifreq *ifr, int cmd)
> EXPORT_SYMBOL(phy_mii_ioctl);
> 
> /**
> - * phy_start_aneg - start auto-negotiation for this PHY device
> + * phy_start_aneg_priv - start auto-negotiation for this PHY device
>  * @phydev: the phy_device struct
> + * @sync: indicate whether we should wait for the workqueue cancelation
>  *
>  * Description: Sanitizes the settings (if we're not autonegotiating
>  *   them), and then calls the driver's config_aneg function.
>  *   If the PHYCONTROL Layer is operating, we change the state to
>  *   reflect the beginning of Auto-negotiation or forcing.
>  */
> -int phy_start_aneg(struct phy_device *phydev)
> +static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
> {
> + bool trigger = 0;
>   int err;
> 
>   mutex_lock(>lock);
> @@ -625,10 +627,40 @@ int phy_start_aneg(struct phy_device *phydev)
>   }
>   }
> 
> + /* Re-schedule a PHY state machine to check PHY status because
> +  * negotiation may already be done and aneg interrupt may not be
> +  * generated.
> +  */
> + if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) {
> + err = phy_aneg_done(phydev);
> + if (err > 0) {
> + trigger = true;
> + err = 0;
> + }
> + }
> +
> out_unlock:
>   mutex_unlock(>lock);
> +
> + if (trigger)
> + phy_trigger_machine(phydev, sync);
> +
>   return err;
> }
> +
> +/**
> + * phy_start_aneg - start auto-negotiation for this PHY device
> + * @phydev: the phy_device struct
> + *
> + * Description: Sanitizes the settings (if we're not autonegotiating
> + *   them), and then calls the driver's config_aneg function.
> + *   If the PHYCONTROL Layer is operating, we change the state to
> + *   reflect the beginning of Auto-negotiation or forcing.
> + */
> +int phy_start_aneg(struct phy_device *phydev)
> +{
> + return phy_start_aneg_priv(phydev, true);
> +}
> EXPORT_SYMBOL(phy_start_aneg);
> 
> /**
> @@ -656,7 +688,7 @@ void phy_start_machine(struct phy_device *phydev)
>  *   state machine runs.
>  */
> 
> -static void phy_trigger_machine(struct phy_device *phydev, bool sync)
> +void phy_trigger_machine(struct phy_device *phydev, bool sync)
> {
>   if (sync)
>   cancel_delayed_work_sync(>state_queue);
> @@ -1151,7 +1183,7 @@ void phy_state_machine(struct work_struct *work)
>   mutex_unlock(>lock);
> 
>   if (needs_aneg)
> - err = phy_start_aneg(phydev);
> + err = phy_start_aneg_priv(phydev, fal

Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Hi, Alexandre!

Just found that you've fixed similar problem for Micrel PHY:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99f81afc139c6edd14d77a91ee91685a414a1c66

Could you please test if my patch[1] fix your problem?

As reverting your patch will speedup MAC startup time for boards with Micrel PHY
on 3~5 sec (auto-negotiation time[2]). Could you check that also?

Regards,
Alexander.

[1] https://lkml.org/lkml/2017/4/20/357
[2] http://www.ieee802.org/3/af/public/jan02/brown_1_0102.pdf


> 20 апр. 2017 г., в 14:00, Alexander Kochetkov  
> написал(а):
> 
> The Ethernet link on an interrupt driven PHY was not coming up if the Ethernet
> cable was plugged before the Ethernet interface was brought up.
> 
> The patch trigger PHY state machine to update link state if PHY was requested 
> to
> do auto-negotiation and auto-negotiation complete flag already set.
> 
> During power-up cycle the PHY do auto-negotiation, generate interrupt and set
> auto-negotiation complete flag. Interrupt is handled by PHY state machine but
> doesn't update link state because PHY is in PHY_READY state. After some time
> MAC bring up, start and request PHY to do auto-negotiation. If there are no 
> new
> settings to advertise genphy_config_aneg() doesn't start PHY auto-negotiation.
> PHY continue to stay in auto-negotiation complete state and doesn't fire
> interrupt. At the same time PHY state machine expect that PHY started
> auto-negotiation and is waiting for interrupt from PHY and it won't get it.
> 
> Signed-off-by: Alexander Kochetkov 
> Cc: stable  # v4.9+
> ---
> drivers/net/phy/phy.c |   40 
> include/linux/phy.h   |1 +
> 2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7cc1b7d..2d9975b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -591,16 +591,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct 
> ifreq *ifr, int cmd)
> EXPORT_SYMBOL(phy_mii_ioctl);
> 
> /**
> - * phy_start_aneg - start auto-negotiation for this PHY device
> + * phy_start_aneg_priv - start auto-negotiation for this PHY device
>  * @phydev: the phy_device struct
> + * @sync: indicate whether we should wait for the workqueue cancelation
>  *
>  * Description: Sanitizes the settings (if we're not autonegotiating
>  *   them), and then calls the driver's config_aneg function.
>  *   If the PHYCONTROL Layer is operating, we change the state to
>  *   reflect the beginning of Auto-negotiation or forcing.
>  */
> -int phy_start_aneg(struct phy_device *phydev)
> +static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
> {
> + bool trigger = 0;
>   int err;
> 
>   mutex_lock(>lock);
> @@ -625,10 +627,40 @@ int phy_start_aneg(struct phy_device *phydev)
>   }
>   }
> 
> + /* Re-schedule a PHY state machine to check PHY status because
> +  * negotiation may already be done and aneg interrupt may not be
> +  * generated.
> +  */
> + if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) {
> + err = phy_aneg_done(phydev);
> + if (err > 0) {
> + trigger = true;
> + err = 0;
> + }
> + }
> +
> out_unlock:
>   mutex_unlock(>lock);
> +
> + if (trigger)
> + phy_trigger_machine(phydev, sync);
> +
>   return err;
> }
> +
> +/**
> + * phy_start_aneg - start auto-negotiation for this PHY device
> + * @phydev: the phy_device struct
> + *
> + * Description: Sanitizes the settings (if we're not autonegotiating
> + *   them), and then calls the driver's config_aneg function.
> + *   If the PHYCONTROL Layer is operating, we change the state to
> + *   reflect the beginning of Auto-negotiation or forcing.
> + */
> +int phy_start_aneg(struct phy_device *phydev)
> +{
> + return phy_start_aneg_priv(phydev, true);
> +}
> EXPORT_SYMBOL(phy_start_aneg);
> 
> /**
> @@ -656,7 +688,7 @@ void phy_start_machine(struct phy_device *phydev)
>  *   state machine runs.
>  */
> 
> -static void phy_trigger_machine(struct phy_device *phydev, bool sync)
> +void phy_trigger_machine(struct phy_device *phydev, bool sync)
> {
>   if (sync)
>   cancel_delayed_work_sync(>state_queue);
> @@ -1151,7 +1183,7 @@ void phy_state_machine(struct work_struct *work)
>   mutex_unlock(>lock);
> 
>   if (needs_aneg)
> - err = phy_start_aneg(phydev);
> + err = phy_start_aneg_priv(phydev, false);
>   else if (do_suspend)
>   phy_suspend(phydev);
>

Re: [PATCH] mmc: dw_mmc: hide clock message when card is resuming

[Alexander Kochetkov]

> 20 апр. 2017 г., в 13:06, Jaehoon Chung  написал(а):
> 
> I think you are not using the latest kernel. which version do you use?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ce69e2fea093b7fa3991c87849c4955cd47796c9
> 
> Could you check this?

Commit ce69e2fea093 («mmc: dw_mmc: silent verbose log when calling from PM 
context») fix
issue I have. So my patch not needed anymore.

I use 4.10.10, probably ce69e2fea093b7fa3991c87849c4955cd47796c9 should be 
backported to 4.10 branch.
4.9 branch doesn’t have commit e9748e0364fe ("mmc: dw_mmc: force setup bus if 
active slots exist») what
cause the problem.

I’ll send request to stable.

Regards,
Alexander.

Re: [PATCH] mmc: dw_mmc: hide clock message when card is resuming

[Alexander Kochetkov]

> 20 апр. 2017 г., в 13:06, Jaehoon Chung  написал(а):
> 
> I think you are not using the latest kernel. which version do you use?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ce69e2fea093b7fa3991c87849c4955cd47796c9
> 
> Could you check this?

Commit ce69e2fea093 («mmc: dw_mmc: silent verbose log when calling from PM 
context») fix
issue I have. So my patch not needed anymore.

I use 4.10.10, probably ce69e2fea093b7fa3991c87849c4955cd47796c9 should be 
backported to 4.10 branch.
4.9 branch doesn’t have commit e9748e0364fe ("mmc: dw_mmc: force setup bus if 
active slots exist») what
cause the problem.

I’ll send request to stable.

Regards,
Alexander.

[PATCH v2] net: arc_emac: switch to phy_start()/phy_stop()

[Alexander Kochetkov]

Currently driver use phy_start_aneg() in arc_emac_open() to bring
up PHY. But phy_start() function is more appropriate for this purposes.
Besides that it call phy_start_aneg() as part of PHY startup sequence
it also can correctly bring up PHY from error and suspended states.
So the patch replace phy_start_aneg() to phy_start().

Also the patch add call to phy_stop() to arc_emac_stop() to allow
the PHY device to be fully suspended when the interface is unused.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---

Changes in v2:
- Updated commit message to clarify changes

 drivers/net/ethernet/arc/emac_main.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index abc9f2a..188676d 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -434,7 +434,7 @@ static int arc_emac_open(struct net_device *ndev)
/* Enable EMAC */
arc_reg_or(priv, R_CTRL, EN_MASK);
 
-   phy_start_aneg(ndev->phydev);
+   phy_start(ndev->phydev);
 
netif_start_queue(ndev);
 
@@ -556,6 +556,8 @@ static int arc_emac_stop(struct net_device *ndev)
napi_disable(>napi);
netif_stop_queue(ndev);
 
+   phy_stop(ndev->phydev);
+
/* Disable interrupts */
arc_reg_clr(priv, R_ENABLE, RXINT_MASK | TXINT_MASK | ERR_MASK);
 
-- 
1.7.9.5

[PATCH v2] net: arc_emac: switch to phy_start()/phy_stop()

[Alexander Kochetkov]

Currently driver use phy_start_aneg() in arc_emac_open() to bring
up PHY. But phy_start() function is more appropriate for this purposes.
Besides that it call phy_start_aneg() as part of PHY startup sequence
it also can correctly bring up PHY from error and suspended states.
So the patch replace phy_start_aneg() to phy_start().

Also the patch add call to phy_stop() to arc_emac_stop() to allow
the PHY device to be fully suspended when the interface is unused.

Signed-off-by: Alexander Kochetkov 
---

Changes in v2:
- Updated commit message to clarify changes

 drivers/net/ethernet/arc/emac_main.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index abc9f2a..188676d 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -434,7 +434,7 @@ static int arc_emac_open(struct net_device *ndev)
/* Enable EMAC */
arc_reg_or(priv, R_CTRL, EN_MASK);
 
-   phy_start_aneg(ndev->phydev);
+   phy_start(ndev->phydev);
 
netif_start_queue(ndev);
 
@@ -556,6 +556,8 @@ static int arc_emac_stop(struct net_device *ndev)
napi_disable(>napi);
netif_stop_queue(ndev);
 
+   phy_stop(ndev->phydev);
+
/* Disable interrupts */
arc_reg_clr(priv, R_ENABLE, RXINT_MASK | TXINT_MASK | ERR_MASK);
 
-- 
1.7.9.5

[PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

The Ethernet link on an interrupt driven PHY was not coming up if the Ethernet
cable was plugged before the Ethernet interface was brought up.

The patch trigger PHY state machine to update link state if PHY was requested to
do auto-negotiation and auto-negotiation complete flag already set.

During power-up cycle the PHY do auto-negotiation, generate interrupt and set
auto-negotiation complete flag. Interrupt is handled by PHY state machine but
doesn't update link state because PHY is in PHY_READY state. After some time
MAC bring up, start and request PHY to do auto-negotiation. If there are no new
settings to advertise genphy_config_aneg() doesn't start PHY auto-negotiation.
PHY continue to stay in auto-negotiation complete state and doesn't fire
interrupt. At the same time PHY state machine expect that PHY started
auto-negotiation and is waiting for interrupt from PHY and it won't get it.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
Cc: stable <sta...@vger.kernel.org> # v4.9+
---
 drivers/net/phy/phy.c |   40 
 include/linux/phy.h   |1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7d..2d9975b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -591,16 +591,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq 
*ifr, int cmd)
 EXPORT_SYMBOL(phy_mii_ioctl);
 
 /**
- * phy_start_aneg - start auto-negotiation for this PHY device
+ * phy_start_aneg_priv - start auto-negotiation for this PHY device
  * @phydev: the phy_device struct
+ * @sync: indicate whether we should wait for the workqueue cancelation
  *
  * Description: Sanitizes the settings (if we're not autonegotiating
  *   them), and then calls the driver's config_aneg function.
  *   If the PHYCONTROL Layer is operating, we change the state to
  *   reflect the beginning of Auto-negotiation or forcing.
  */
-int phy_start_aneg(struct phy_device *phydev)
+static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
 {
+   bool trigger = 0;
int err;
 
mutex_lock(>lock);
@@ -625,10 +627,40 @@ int phy_start_aneg(struct phy_device *phydev)
}
}
 
+   /* Re-schedule a PHY state machine to check PHY status because
+* negotiation may already be done and aneg interrupt may not be
+* generated.
+*/
+   if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) {
+   err = phy_aneg_done(phydev);
+   if (err > 0) {
+   trigger = true;
+   err = 0;
+   }
+   }
+
 out_unlock:
mutex_unlock(>lock);
+
+   if (trigger)
+   phy_trigger_machine(phydev, sync);
+
return err;
 }
+
+/**
+ * phy_start_aneg - start auto-negotiation for this PHY device
+ * @phydev: the phy_device struct
+ *
+ * Description: Sanitizes the settings (if we're not autonegotiating
+ *   them), and then calls the driver's config_aneg function.
+ *   If the PHYCONTROL Layer is operating, we change the state to
+ *   reflect the beginning of Auto-negotiation or forcing.
+ */
+int phy_start_aneg(struct phy_device *phydev)
+{
+   return phy_start_aneg_priv(phydev, true);
+}
 EXPORT_SYMBOL(phy_start_aneg);
 
 /**
@@ -656,7 +688,7 @@ void phy_start_machine(struct phy_device *phydev)
  *   state machine runs.
  */
 
-static void phy_trigger_machine(struct phy_device *phydev, bool sync)
+void phy_trigger_machine(struct phy_device *phydev, bool sync)
 {
if (sync)
cancel_delayed_work_sync(>state_queue);
@@ -1151,7 +1183,7 @@ void phy_state_machine(struct work_struct *work)
mutex_unlock(>lock);
 
if (needs_aneg)
-   err = phy_start_aneg(phydev);
+   err = phy_start_aneg_priv(phydev, false);
else if (do_suspend)
phy_suspend(phydev);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7fc1105..b19ae66 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -840,6 +840,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int 
n,
 void phy_mac_interrupt(struct phy_device *phydev, int new_link);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
+void phy_trigger_machine(struct phy_device *phydev, bool sync);
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_ksettings_get(struct phy_device *phydev,
-- 
1.7.9.5

[PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

The Ethernet link on an interrupt driven PHY was not coming up if the Ethernet
cable was plugged before the Ethernet interface was brought up.

The patch trigger PHY state machine to update link state if PHY was requested to
do auto-negotiation and auto-negotiation complete flag already set.

During power-up cycle the PHY do auto-negotiation, generate interrupt and set
auto-negotiation complete flag. Interrupt is handled by PHY state machine but
doesn't update link state because PHY is in PHY_READY state. After some time
MAC bring up, start and request PHY to do auto-negotiation. If there are no new
settings to advertise genphy_config_aneg() doesn't start PHY auto-negotiation.
PHY continue to stay in auto-negotiation complete state and doesn't fire
interrupt. At the same time PHY state machine expect that PHY started
auto-negotiation and is waiting for interrupt from PHY and it won't get it.

Signed-off-by: Alexander Kochetkov 
Cc: stable  # v4.9+
---
 drivers/net/phy/phy.c |   40 
 include/linux/phy.h   |1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7d..2d9975b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -591,16 +591,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq 
*ifr, int cmd)
 EXPORT_SYMBOL(phy_mii_ioctl);
 
 /**
- * phy_start_aneg - start auto-negotiation for this PHY device
+ * phy_start_aneg_priv - start auto-negotiation for this PHY device
  * @phydev: the phy_device struct
+ * @sync: indicate whether we should wait for the workqueue cancelation
  *
  * Description: Sanitizes the settings (if we're not autonegotiating
  *   them), and then calls the driver's config_aneg function.
  *   If the PHYCONTROL Layer is operating, we change the state to
  *   reflect the beginning of Auto-negotiation or forcing.
  */
-int phy_start_aneg(struct phy_device *phydev)
+static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
 {
+   bool trigger = 0;
int err;
 
mutex_lock(>lock);
@@ -625,10 +627,40 @@ int phy_start_aneg(struct phy_device *phydev)
}
}
 
+   /* Re-schedule a PHY state machine to check PHY status because
+* negotiation may already be done and aneg interrupt may not be
+* generated.
+*/
+   if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) {
+   err = phy_aneg_done(phydev);
+   if (err > 0) {
+   trigger = true;
+   err = 0;
+   }
+   }
+
 out_unlock:
mutex_unlock(>lock);
+
+   if (trigger)
+   phy_trigger_machine(phydev, sync);
+
return err;
 }
+
+/**
+ * phy_start_aneg - start auto-negotiation for this PHY device
+ * @phydev: the phy_device struct
+ *
+ * Description: Sanitizes the settings (if we're not autonegotiating
+ *   them), and then calls the driver's config_aneg function.
+ *   If the PHYCONTROL Layer is operating, we change the state to
+ *   reflect the beginning of Auto-negotiation or forcing.
+ */
+int phy_start_aneg(struct phy_device *phydev)
+{
+   return phy_start_aneg_priv(phydev, true);
+}
 EXPORT_SYMBOL(phy_start_aneg);
 
 /**
@@ -656,7 +688,7 @@ void phy_start_machine(struct phy_device *phydev)
  *   state machine runs.
  */
 
-static void phy_trigger_machine(struct phy_device *phydev, bool sync)
+void phy_trigger_machine(struct phy_device *phydev, bool sync)
 {
if (sync)
cancel_delayed_work_sync(>state_queue);
@@ -1151,7 +1183,7 @@ void phy_state_machine(struct work_struct *work)
mutex_unlock(>lock);
 
if (needs_aneg)
-   err = phy_start_aneg(phydev);
+   err = phy_start_aneg_priv(phydev, false);
else if (do_suspend)
phy_suspend(phydev);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7fc1105..b19ae66 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -840,6 +840,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int 
n,
 void phy_mac_interrupt(struct phy_device *phydev, int new_link);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
+void phy_trigger_machine(struct phy_device *phydev, bool sync);
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_ksettings_get(struct phy_device *phydev,
-- 
1.7.9.5

[PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Hello Florian, Roger and Madalin!

This is slightly modified version of previous patch[1].

It take into account that phy_start_aneg() may be called from
differend places not only from PHY state machine. So the patch
use phy_trigger_machine() to schedule link state update
correctly.

I borrow some text from Roger's commit message.

Added 'Cc: stable <sta...@vger.kernel.org> tag'
because the problem exist on 4.9 and 4.10 branches.

Don't know what commit brake the code, so I don't add
Fixes tag.

Roger, could you please test this one patch?

Madalin, I found, that you tested Roger’s patch[2] and this
one patch will get upstream instead of patch[2]. Could you
please test it?

Regards,
Alexander.

[1] http://patchwork.ozlabs.org/patch/743773/
[2] https://lkml.org/lkml/2017/3/30/517

Alexander Kochetkov (1):
  net: phy: fix auto-negotiation stall due to unavailable interrupt

 drivers/net/phy/phy.c |   40 
 include/linux/phy.h   |1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Hello Florian, Roger and Madalin!

This is slightly modified version of previous patch[1].

It take into account that phy_start_aneg() may be called from
differend places not only from PHY state machine. So the patch
use phy_trigger_machine() to schedule link state update
correctly.

I borrow some text from Roger's commit message.

Added 'Cc: stable  tag'
because the problem exist on 4.9 and 4.10 branches.

Don't know what commit brake the code, so I don't add
Fixes tag.

Roger, could you please test this one patch?

Madalin, I found, that you tested Roger’s patch[2] and this
one patch will get upstream instead of patch[2]. Could you
please test it?

Regards,
Alexander.

[1] http://patchwork.ozlabs.org/patch/743773/
[2] https://lkml.org/lkml/2017/3/30/517

Alexander Kochetkov (1):
  net: phy: fix auto-negotiation stall due to unavailable interrupt

 drivers/net/phy/phy.c |   40 
 include/linux/phy.h   |1 +
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
1.7.9.5

Re: [PATCH] net: arc_emac: switch to phy_start()/phy_stop()

[Alexander Kochetkov]

> 20 апр. 2017 г., в 0:08, Florian Fainelli <f.faine...@gmail.com> написал(а):
> 
> This looks fine. If you wanted to go further, you could move the
> phy_connect(), phy_disconnect() calls down to the arc_emac_open()
> respectively arc_emac_stop() as this would also allow the PHY device to
> be fully suspended when the interface is unused.


I’ve checked patch phy_connect() is called from arc_emac_open() and
phy_disconnect() is called from arc_emac_stop().

So, I’ve made mistake in the commit message.

Thank you for review.

> 
> 19 апр. 2017 г., в 21:22, Sergei Shtylyov 
> <sergei.shtyl...@cogentembedded.com> написал(а):
> 
> On 04/19/2017 05:29 PM, Alexander Kochetkov wrote:
> 
>> The patch replace phy_start_aneg() with phy_start(). phy_start() call
> 
>   Replaces.
> 
>> phy_start_aneg() as a part of startup sequence and allow recover from
>> error (PHY_HALTED) state.
>> 
>> Also added call phy_stop() to arc_emac_remove() to stop PHY state machine
> 
>   To arc_emac_stop() maybe?
> 

Sergei, thanks for spell and gramma checking.

Regards,
Alexander.

Re: [PATCH] net: arc_emac: switch to phy_start()/phy_stop()

[Alexander Kochetkov]

> 20 апр. 2017 г., в 0:08, Florian Fainelli  написал(а):
> 
> This looks fine. If you wanted to go further, you could move the
> phy_connect(), phy_disconnect() calls down to the arc_emac_open()
> respectively arc_emac_stop() as this would also allow the PHY device to
> be fully suspended when the interface is unused.


I’ve checked patch phy_connect() is called from arc_emac_open() and
phy_disconnect() is called from arc_emac_stop().

So, I’ve made mistake in the commit message.

Thank you for review.

> 
> 19 апр. 2017 г., в 21:22, Sergei Shtylyov 
>  написал(а):
> 
> On 04/19/2017 05:29 PM, Alexander Kochetkov wrote:
> 
>> The patch replace phy_start_aneg() with phy_start(). phy_start() call
> 
>   Replaces.
> 
>> phy_start_aneg() as a part of startup sequence and allow recover from
>> error (PHY_HALTED) state.
>> 
>> Also added call phy_stop() to arc_emac_remove() to stop PHY state machine
> 
>   To arc_emac_stop() maybe?
> 

Sergei, thanks for spell and gramma checking.

Regards,
Alexander.

Re: [PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

> 19 апр. 2017 г., в 19:32, Florian Fainelli  написал(а):
> 
> http://patchwork.ozlabs.org/patch/743773/
> 
> Roger can you also test Alexander's patch?

If MAC use phy_start_aneg() instead of phy_start() my patch will not work as
expected. Roger, if patch don’t work for you please check what MAC bring up PHY 
using
phy_start():

http://patchwork.ozlabs.org/patch/752308/

Is it correct to start PHY inside MAC probe using phy_start_aneg()? Or 
phy_start() must be used?

And probably this tags should be added for my patch:
Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not 
polling.")
Cc: stable  # v4.9+

Because I bisected to commit 529ed1275263 ("net: phy: phy drivers
should not set SUPPORTED_[Asym_]Pause») that looks pretty good.

Also, there is another issue I found. link_timeout doesn’t work for interrupt 
driven PHY.
It is possible to implement timer to handle this case.
Florian, what do you think? Should this be fixed?

Alexander.

Re: [PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

> 19 апр. 2017 г., в 19:32, Florian Fainelli  написал(а):
> 
> http://patchwork.ozlabs.org/patch/743773/
> 
> Roger can you also test Alexander's patch?

If MAC use phy_start_aneg() instead of phy_start() my patch will not work as
expected. Roger, if patch don’t work for you please check what MAC bring up PHY 
using
phy_start():

http://patchwork.ozlabs.org/patch/752308/

Is it correct to start PHY inside MAC probe using phy_start_aneg()? Or 
phy_start() must be used?

And probably this tags should be added for my patch:
Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not 
polling.")
Cc: stable  # v4.9+

Because I bisected to commit 529ed1275263 ("net: phy: phy drivers
should not set SUPPORTED_[Asym_]Pause») that looks pretty good.

Also, there is another issue I found. link_timeout doesn’t work for interrupt 
driven PHY.
It is possible to implement timer to handle this case.
Florian, what do you think? Should this be fixed?

Alexander.

[PATCH] mmc: dw_mmc: hide clock message when card is resuming

[Alexander Kochetkov]

Commit e9748e0364fe ("mmc: dw_mmc: force setup bus if active slots exist")
made a change resulted in clock message appears every time the card is
resuming (every 5 second in average). Add condition that previously used to
print the message.

Fixes: e9748e0364fe ("mmc: dw_mmc: force setup bus if active slots exist")
Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/mmc/host/dw_mmc.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 73db085..faaf2c1 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1178,7 +1178,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, 
bool force_clkinit)
 
if ((clock != slot->__clk_old &&
!test_bit(DW_MMC_CARD_NEEDS_POLL, >flags)) ||
-   force_clkinit) {
+   (force_clkinit &&
+   (slot->mmc->pm_flags & MMC_PM_KEEP_POWER))) {
dev_info(>mmc->class_dev,
 "Bus speed (slot %d) = %dHz (slot req %dHz, 
actual %dHZ div = %d)\n",
 slot->id, host->bus_hz, clock,
-- 
1.7.9.5

[PATCH] mmc: dw_mmc: hide clock message when card is resuming

[Alexander Kochetkov]

Commit e9748e0364fe ("mmc: dw_mmc: force setup bus if active slots exist")
made a change resulted in clock message appears every time the card is
resuming (every 5 second in average). Add condition that previously used to
print the message.

Fixes: e9748e0364fe ("mmc: dw_mmc: force setup bus if active slots exist")
Signed-off-by: Alexander Kochetkov 
---
 drivers/mmc/host/dw_mmc.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 73db085..faaf2c1 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1178,7 +1178,8 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, 
bool force_clkinit)
 
if ((clock != slot->__clk_old &&
!test_bit(DW_MMC_CARD_NEEDS_POLL, >flags)) ||
-   force_clkinit) {
+   (force_clkinit &&
+   (slot->mmc->pm_flags & MMC_PM_KEEP_POWER))) {
dev_info(>mmc->class_dev,
 "Bus speed (slot %d) = %dHz (slot req %dHz, 
actual %dHZ div = %d)\n",
 slot->id, host->bus_hz, clock,
-- 
1.7.9.5

[PATCH] net: arc_emac: switch to phy_start()/phy_stop()

[Alexander Kochetkov]

The patch replace phy_start_aneg() with phy_start(). phy_start() call
phy_start_aneg() as a part of startup sequence and allow recover from
error (PHY_HALTED) state.

Also added call phy_stop() to arc_emac_remove() to stop PHY state machine
when MAC is down.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/net/ethernet/arc/emac_main.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index abc9f2a..188676d 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -434,7 +434,7 @@ static int arc_emac_open(struct net_device *ndev)
/* Enable EMAC */
arc_reg_or(priv, R_CTRL, EN_MASK);
 
-   phy_start_aneg(ndev->phydev);
+   phy_start(ndev->phydev);
 
netif_start_queue(ndev);
 
@@ -556,6 +556,8 @@ static int arc_emac_stop(struct net_device *ndev)
napi_disable(>napi);
netif_stop_queue(ndev);
 
+   phy_stop(ndev->phydev);
+
/* Disable interrupts */
arc_reg_clr(priv, R_ENABLE, RXINT_MASK | TXINT_MASK | ERR_MASK);
 
-- 
1.7.9.5

[PATCH] net: arc_emac: switch to phy_start()/phy_stop()

[Alexander Kochetkov]

The patch replace phy_start_aneg() with phy_start(). phy_start() call
phy_start_aneg() as a part of startup sequence and allow recover from
error (PHY_HALTED) state.

Also added call phy_stop() to arc_emac_remove() to stop PHY state machine
when MAC is down.

Signed-off-by: Alexander Kochetkov 
---
 drivers/net/ethernet/arc/emac_main.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/arc/emac_main.c 
b/drivers/net/ethernet/arc/emac_main.c
index abc9f2a..188676d 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -434,7 +434,7 @@ static int arc_emac_open(struct net_device *ndev)
/* Enable EMAC */
arc_reg_or(priv, R_CTRL, EN_MASK);
 
-   phy_start_aneg(ndev->phydev);
+   phy_start(ndev->phydev);
 
netif_start_queue(ndev);
 
@@ -556,6 +556,8 @@ static int arc_emac_stop(struct net_device *ndev)
napi_disable(>napi);
netif_stop_queue(ndev);
 
+   phy_stop(ndev->phydev);
+
/* Disable interrupts */
arc_reg_clr(priv, R_ENABLE, RXINT_MASK | TXINT_MASK | ERR_MASK);
 
-- 
1.7.9.5

Re: [PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Just found similar problem fixed in another PHY. See commit 99f81afc139c
("phy: micrel: Disable auto negotiation on startup»)

> 19 апр. 2017 г., в 16:46, Alexander Kochetkov <al.koc...@gmail.com> 
> написал(а):
> 
> The problem I fix related to SMSC LAN8710/LAN8720 PHY handled using
> interrupts. During power-up cycle the PHY do auto-negotiation, generate
> interrupt and set BMSR_ANEGCOMPLETE flag. Interrupt is handled by PHY
> state machine but doesn't update link because PHY is in PHY_READY state.
> After some time MAC bring up and connect with PHY. It start PHY using
> phy_start(). During startup PHY change state to PHY_AN but doesn't
> set BMCR_ANRESTART flag due to genphy_config_aneg() doesn't update MII_BMCR
> because there no new to advertising. As a result, state machine wait for
> interrupt from PHY and nether get "link is up". Because BMSR_ANEGCOMPLETE
> already set the patch schedule check link without waiting interrupt.
> In case genphy_config_aneg() update MII_BMCR and set BMCR_ANRESTART
> flag, BMSR_ANEGCOMPLETE will be cleared and state machine will continue
> on auto-negotiation interrupt.
> 
> Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
> ---
> drivers/net/phy/phy.c |   12 
> 1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7cc1b7d..da8f03d 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1169,6 +1169,18 @@ void phy_state_machine(struct work_struct *work)
>   if (phydev->irq == PHY_POLL)
>   queue_delayed_work(system_power_efficient_wq, 
> >state_queue,
>  PHY_STATE_TIME * HZ);
> +
> + /* Re-schedule a PHY state machine to check PHY status because
> +  * negotiation already done and aneg interrupt may not be generated.
> +  */
> + if (needs_aneg && (phydev->irq > 0) && (phydev->state == PHY_AN)) {
> + err = phy_aneg_done(phydev);
> + if (err > 0)
> + queue_delayed_work(system_power_efficient_wq,
> +>state_queue, 0);
> + if (err < 0)
> + phy_error(phydev);
> + }
> }
> 
> /**
> -- 
> 1.7.9.5
> 

Re: [PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

Just found similar problem fixed in another PHY. See commit 99f81afc139c
("phy: micrel: Disable auto negotiation on startup»)

> 19 апр. 2017 г., в 16:46, Alexander Kochetkov  
> написал(а):
> 
> The problem I fix related to SMSC LAN8710/LAN8720 PHY handled using
> interrupts. During power-up cycle the PHY do auto-negotiation, generate
> interrupt and set BMSR_ANEGCOMPLETE flag. Interrupt is handled by PHY
> state machine but doesn't update link because PHY is in PHY_READY state.
> After some time MAC bring up and connect with PHY. It start PHY using
> phy_start(). During startup PHY change state to PHY_AN but doesn't
> set BMCR_ANRESTART flag due to genphy_config_aneg() doesn't update MII_BMCR
> because there no new to advertising. As a result, state machine wait for
> interrupt from PHY and nether get "link is up". Because BMSR_ANEGCOMPLETE
> already set the patch schedule check link without waiting interrupt.
> In case genphy_config_aneg() update MII_BMCR and set BMCR_ANRESTART
> flag, BMSR_ANEGCOMPLETE will be cleared and state machine will continue
> on auto-negotiation interrupt.
> 
> Signed-off-by: Alexander Kochetkov 
> ---
> drivers/net/phy/phy.c |   12 
> 1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7cc1b7d..da8f03d 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1169,6 +1169,18 @@ void phy_state_machine(struct work_struct *work)
>   if (phydev->irq == PHY_POLL)
>   queue_delayed_work(system_power_efficient_wq, 
> >state_queue,
>  PHY_STATE_TIME * HZ);
> +
> + /* Re-schedule a PHY state machine to check PHY status because
> +  * negotiation already done and aneg interrupt may not be generated.
> +  */
> + if (needs_aneg && (phydev->irq > 0) && (phydev->state == PHY_AN)) {
> + err = phy_aneg_done(phydev);
> + if (err > 0)
> + queue_delayed_work(system_power_efficient_wq,
> +>state_queue, 0);
> + if (err < 0)
> + phy_error(phydev);
> + }
> }
> 
> /**
> -- 
> 1.7.9.5
> 

[PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

The problem I fix related to SMSC LAN8710/LAN8720 PHY handled using
interrupts. During power-up cycle the PHY do auto-negotiation, generate
interrupt and set BMSR_ANEGCOMPLETE flag. Interrupt is handled by PHY
state machine but doesn't update link because PHY is in PHY_READY state.
After some time MAC bring up and connect with PHY. It start PHY using
phy_start(). During startup PHY change state to PHY_AN but doesn't
set BMCR_ANRESTART flag due to genphy_config_aneg() doesn't update MII_BMCR
because there no new to advertising. As a result, state machine wait for
interrupt from PHY and nether get "link is up". Because BMSR_ANEGCOMPLETE
already set the patch schedule check link without waiting interrupt.
In case genphy_config_aneg() update MII_BMCR and set BMCR_ANRESTART
flag, BMSR_ANEGCOMPLETE will be cleared and state machine will continue
on auto-negotiation interrupt.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/net/phy/phy.c |   12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7d..da8f03d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1169,6 +1169,18 @@ void phy_state_machine(struct work_struct *work)
if (phydev->irq == PHY_POLL)
queue_delayed_work(system_power_efficient_wq, 
>state_queue,
   PHY_STATE_TIME * HZ);
+
+   /* Re-schedule a PHY state machine to check PHY status because
+* negotiation already done and aneg interrupt may not be generated.
+*/
+   if (needs_aneg && (phydev->irq > 0) && (phydev->state == PHY_AN)) {
+   err = phy_aneg_done(phydev);
+   if (err > 0)
+   queue_delayed_work(system_power_efficient_wq,
+  >state_queue, 0);
+   if (err < 0)
+   phy_error(phydev);
+   }
 }
 
 /**
-- 
1.7.9.5

[PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt

[Alexander Kochetkov]

The problem I fix related to SMSC LAN8710/LAN8720 PHY handled using
interrupts. During power-up cycle the PHY do auto-negotiation, generate
interrupt and set BMSR_ANEGCOMPLETE flag. Interrupt is handled by PHY
state machine but doesn't update link because PHY is in PHY_READY state.
After some time MAC bring up and connect with PHY. It start PHY using
phy_start(). During startup PHY change state to PHY_AN but doesn't
set BMCR_ANRESTART flag due to genphy_config_aneg() doesn't update MII_BMCR
because there no new to advertising. As a result, state machine wait for
interrupt from PHY and nether get "link is up". Because BMSR_ANEGCOMPLETE
already set the patch schedule check link without waiting interrupt.
In case genphy_config_aneg() update MII_BMCR and set BMCR_ANRESTART
flag, BMSR_ANEGCOMPLETE will be cleared and state machine will continue
on auto-negotiation interrupt.

Signed-off-by: Alexander Kochetkov 
---
 drivers/net/phy/phy.c |   12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7d..da8f03d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1169,6 +1169,18 @@ void phy_state_machine(struct work_struct *work)
if (phydev->irq == PHY_POLL)
queue_delayed_work(system_power_efficient_wq, 
>state_queue,
   PHY_STATE_TIME * HZ);
+
+   /* Re-schedule a PHY state machine to check PHY status because
+* negotiation already done and aneg interrupt may not be generated.
+*/
+   if (needs_aneg && (phydev->irq > 0) && (phydev->state == PHY_AN)) {
+   err = phy_aneg_done(phydev);
+   if (err > 0)
+   queue_delayed_work(system_power_efficient_wq,
+  >state_queue, 0);
+   if (err < 0)
+   phy_error(phydev);
+   }
 }
 
 /**
-- 
1.7.9.5

[PATCH 2/2] dmaengine: pl330: don't emit code for one iteration loop

[Alexander Kochetkov]

The patch remove one iteration outer loop in the _loop().
Removing loop saves 4 bytes of MicroCode buffer. This
savings make sense for free-running cyclic transfer
implementation.

DMALP_0 0
...
DMALPENDA_0 bjmpto_9

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/dma/pl330.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 56a2377..6126dde 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1268,7 +1268,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
lpend.bjump = 0;
szlpend = _emit_LPEND(1, buf, );
 
-   if (lcnt0) {
+   if (lcnt0 > 1) {
szlp *= 2;
szlpend *= 2;
}
@@ -1284,7 +1284,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
 
off = 0;
 
-   if (lcnt0) {
+   if (lcnt0 > 1) {
off += _emit_LP(dry_run, [off], 0, lcnt0);
ljmp0 = off;
}
@@ -1300,7 +1300,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
lpend.bjump = off - ljmp1;
off += _emit_LPEND(dry_run, [off], );
 
-   if (lcnt0) {
+   if (lcnt0 > 1) {
lpend.cond = ALWAYS;
lpend.forever = false;
lpend.loop = 0;
@@ -1309,7 +1309,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
}
 
*bursts = lcnt1 * cyc;
-   if (lcnt0)
+   if (lcnt0 > 1)
*bursts *= lcnt0;
 
return off;
-- 
1.7.9.5

[PATCH 2/2] dmaengine: pl330: don't emit code for one iteration loop

[Alexander Kochetkov]

The patch remove one iteration outer loop in the _loop().
Removing loop saves 4 bytes of MicroCode buffer. This
savings make sense for free-running cyclic transfer
implementation.

DMALP_0 0
...
DMALPENDA_0 bjmpto_9

Signed-off-by: Alexander Kochetkov 
---
 drivers/dma/pl330.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 56a2377..6126dde 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1268,7 +1268,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
lpend.bjump = 0;
szlpend = _emit_LPEND(1, buf, );
 
-   if (lcnt0) {
+   if (lcnt0 > 1) {
szlp *= 2;
szlpend *= 2;
}
@@ -1284,7 +1284,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
 
off = 0;
 
-   if (lcnt0) {
+   if (lcnt0 > 1) {
off += _emit_LP(dry_run, [off], 0, lcnt0);
ljmp0 = off;
}
@@ -1300,7 +1300,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
lpend.bjump = off - ljmp1;
off += _emit_LPEND(dry_run, [off], );
 
-   if (lcnt0) {
+   if (lcnt0 > 1) {
lpend.cond = ALWAYS;
lpend.forever = false;
lpend.loop = 0;
@@ -1309,7 +1309,7 @@ static inline int _loop(struct pl330_dmac *pl330, 
unsigned dry_run, u8 buf[],
}
 
*bursts = lcnt1 * cyc;
-   if (lcnt0)
+   if (lcnt0 > 1)
*bursts *= lcnt0;
 
return off;
-- 
1.7.9.5

[PATCH 1/2] dmaengine: pl330: make cyclic transfer free runnable

[Alexander Kochetkov]

The patch solve the I2S click problem on rk3188. Actually
all the devices using pl330 should have I2S click problem
due to pl330 cyclic transfer implementation.

Current implementation depends on soft irq. If pl330 unable
to submit next transfer in time some samples could be lost.
The lost samples heard as sound click. In order to check
lost samples, I've installed I2S interrupt handler to signal
overflow/underflow conditions. Sometimes I saw overflow
or underflow events and heard clicks.

The patch setup cyclic transfer such a way, that transfer can
run infinitely without CPU intervention. As a result,
lost samples and clicks gone.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/dma/pl330.c |  192 +--
 1 file changed, 94 insertions(+), 98 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 7539f73..56a2377 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -446,9 +446,6 @@ struct dma_pl330_chan {
int burst_len; /* the number of burst */
dma_addr_t fifo_addr;
 
-   /* for cyclic capability */
-   bool cyclic;
-
/* for runtime pm tracking */
bool active;
 };
@@ -532,6 +529,10 @@ struct dma_pl330_desc {
unsigned peri:5;
/* Hook to attach to DMAC's list of reqs with due callback */
struct list_head rqd;
+
+   /* For cyclic capability */
+   bool cyclic;
+   size_t num_periods;
 };
 
 struct _xfer_spec {
@@ -1333,16 +1334,19 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 }
 
 static inline int _setup_xfer(struct pl330_dmac *pl330,
- unsigned dry_run, u8 buf[],
+ unsigned dry_run, u8 buf[], u32 period,
  const struct _xfer_spec *pxs)
 {
struct pl330_xfer *x = >desc->px;
+   struct pl330_reqcfg *rqcfg = >desc->rqcfg;
int off = 0;
 
/* DMAMOV SAR, x->src_addr */
-   off += _emit_MOV(dry_run, [off], SAR, x->src_addr);
+   off += _emit_MOV(dry_run, [off], SAR,
+   x->src_addr + rqcfg->src_inc * period * x->bytes);
/* DMAMOV DAR, x->dst_addr */
-   off += _emit_MOV(dry_run, [off], DAR, x->dst_addr);
+   off += _emit_MOV(dry_run, [off], DAR,
+   x->dst_addr + rqcfg->dst_inc * period * x->bytes);
 
/* Setup Loop(s) */
off += _setup_loops(pl330, dry_run, [off], pxs);
@@ -1362,23 +1366,41 @@ static int _setup_req(struct pl330_dmac *pl330, 
unsigned dry_run,
struct pl330_xfer *x;
u8 *buf = req->mc_cpu;
int off = 0;
+   int period;
+   int again_off;
 
PL330_DBGMC_START(req->mc_bus);
 
/* DMAMOV CCR, ccr */
off += _emit_MOV(dry_run, [off], CCR, pxs->ccr);
+   again_off = off;
 
x = >desc->px;
/* Error if xfer length is not aligned at burst size */
if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
return -EINVAL;
 
-   off += _setup_xfer(pl330, dry_run, [off], pxs);
+   for (period = 0; period < pxs->desc->num_periods; period++) {
+   off += _setup_xfer(pl330, dry_run, [off], period, pxs);
 
-   /* DMASEV peripheral/event */
-   off += _emit_SEV(dry_run, [off], thrd->ev);
-   /* DMAEND */
-   off += _emit_END(dry_run, [off]);
+   /* DMASEV peripheral/event */
+   off += _emit_SEV(dry_run, [off], thrd->ev);
+   }
+
+   if (!pxs->desc->cyclic) {
+   /* DMAEND */
+   off += _emit_END(dry_run, [off]);
+   } else {
+   struct _arg_LPEND lpend;
+   /* LP */
+   off += _emit_LP(dry_run, [off], 0, 255);
+   /* LPEND */
+   lpend.cond = ALWAYS;
+   lpend.forever = false;
+   lpend.loop = 0;
+   lpend.bjump = off - again_off;
+   off += _emit_LPEND(dry_run, [off], );
+   }
 
return off;
 }
@@ -1640,12 +1662,13 @@ static int pl330_update(struct pl330_dmac *pl330)
 
/* Detach the req */
descdone = thrd->req[active].desc;
-   thrd->req[active].desc = NULL;
-
-   thrd->req_running = -1;
 
-   /* Get going again ASAP */
-   _start(thrd);
+   if (!descdone->cyclic) {
+   thrd->req[active].desc = NULL;
+   thrd->req_running = -1;
+   /* Get going again ASAP */
+   _start(thrd);
+   }
 
/* For now, just make a list of callbacks to be done */
list_add_tail(>rqd, >req_done);
@@ -2013,12 +2036,28 @@ stat

[PATCH 1/2] dmaengine: pl330: make cyclic transfer free runnable

[Alexander Kochetkov]

The patch solve the I2S click problem on rk3188. Actually
all the devices using pl330 should have I2S click problem
due to pl330 cyclic transfer implementation.

Current implementation depends on soft irq. If pl330 unable
to submit next transfer in time some samples could be lost.
The lost samples heard as sound click. In order to check
lost samples, I've installed I2S interrupt handler to signal
overflow/underflow conditions. Sometimes I saw overflow
or underflow events and heard clicks.

The patch setup cyclic transfer such a way, that transfer can
run infinitely without CPU intervention. As a result,
lost samples and clicks gone.

Signed-off-by: Alexander Kochetkov 
---
 drivers/dma/pl330.c |  192 +--
 1 file changed, 94 insertions(+), 98 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 7539f73..56a2377 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -446,9 +446,6 @@ struct dma_pl330_chan {
int burst_len; /* the number of burst */
dma_addr_t fifo_addr;
 
-   /* for cyclic capability */
-   bool cyclic;
-
/* for runtime pm tracking */
bool active;
 };
@@ -532,6 +529,10 @@ struct dma_pl330_desc {
unsigned peri:5;
/* Hook to attach to DMAC's list of reqs with due callback */
struct list_head rqd;
+
+   /* For cyclic capability */
+   bool cyclic;
+   size_t num_periods;
 };
 
 struct _xfer_spec {
@@ -1333,16 +1334,19 @@ static inline int _setup_loops(struct pl330_dmac *pl330,
 }
 
 static inline int _setup_xfer(struct pl330_dmac *pl330,
- unsigned dry_run, u8 buf[],
+ unsigned dry_run, u8 buf[], u32 period,
  const struct _xfer_spec *pxs)
 {
struct pl330_xfer *x = >desc->px;
+   struct pl330_reqcfg *rqcfg = >desc->rqcfg;
int off = 0;
 
/* DMAMOV SAR, x->src_addr */
-   off += _emit_MOV(dry_run, [off], SAR, x->src_addr);
+   off += _emit_MOV(dry_run, [off], SAR,
+   x->src_addr + rqcfg->src_inc * period * x->bytes);
/* DMAMOV DAR, x->dst_addr */
-   off += _emit_MOV(dry_run, [off], DAR, x->dst_addr);
+   off += _emit_MOV(dry_run, [off], DAR,
+   x->dst_addr + rqcfg->dst_inc * period * x->bytes);
 
/* Setup Loop(s) */
off += _setup_loops(pl330, dry_run, [off], pxs);
@@ -1362,23 +1366,41 @@ static int _setup_req(struct pl330_dmac *pl330, 
unsigned dry_run,
struct pl330_xfer *x;
u8 *buf = req->mc_cpu;
int off = 0;
+   int period;
+   int again_off;
 
PL330_DBGMC_START(req->mc_bus);
 
/* DMAMOV CCR, ccr */
off += _emit_MOV(dry_run, [off], CCR, pxs->ccr);
+   again_off = off;
 
x = >desc->px;
/* Error if xfer length is not aligned at burst size */
if (x->bytes % (BRST_SIZE(pxs->ccr) * BRST_LEN(pxs->ccr)))
return -EINVAL;
 
-   off += _setup_xfer(pl330, dry_run, [off], pxs);
+   for (period = 0; period < pxs->desc->num_periods; period++) {
+   off += _setup_xfer(pl330, dry_run, [off], period, pxs);
 
-   /* DMASEV peripheral/event */
-   off += _emit_SEV(dry_run, [off], thrd->ev);
-   /* DMAEND */
-   off += _emit_END(dry_run, [off]);
+   /* DMASEV peripheral/event */
+   off += _emit_SEV(dry_run, [off], thrd->ev);
+   }
+
+   if (!pxs->desc->cyclic) {
+   /* DMAEND */
+   off += _emit_END(dry_run, [off]);
+   } else {
+   struct _arg_LPEND lpend;
+   /* LP */
+   off += _emit_LP(dry_run, [off], 0, 255);
+   /* LPEND */
+   lpend.cond = ALWAYS;
+   lpend.forever = false;
+   lpend.loop = 0;
+   lpend.bjump = off - again_off;
+   off += _emit_LPEND(dry_run, [off], );
+   }
 
return off;
 }
@@ -1640,12 +1662,13 @@ static int pl330_update(struct pl330_dmac *pl330)
 
/* Detach the req */
descdone = thrd->req[active].desc;
-   thrd->req[active].desc = NULL;
-
-   thrd->req_running = -1;
 
-   /* Get going again ASAP */
-   _start(thrd);
+   if (!descdone->cyclic) {
+   thrd->req[active].desc = NULL;
+   thrd->req_running = -1;
+   /* Get going again ASAP */
+   _start(thrd);
+   }
 
/* For now, just make a list of callbacks to be done */
list_add_tail(>rqd, >req_done);
@@ -2013,12 +2036,28 @@ static void pl330_tasklet(unsigned long data)
 

[PATCH 0/2] Free running cyclic transfer implementation for pl330

[Alexander Kochetkov]

Hello!

This series contain free running cyclic transfer implementation for pl330.
It affect ALL chips using pl330 (not only rockchip) and
allow to run cyclic transfers without CPU intervention. As a result
it fix sound clicks (observed and not yet observed) because
sound clicks must be heard under heavy system load due to the way
how cyclic transfers implemented now for pl330.

My previous series[1] doesn't get enough attention (no one except me
tested it). And it don't get upstream:

> 8-03-2016, 6:03, Vinod Koul <vinod.k...@intel.com> *:
>Overall this series looks okay, but can someone test this. I would not like
pl330 to be broken again

Now I was asked about the series[1] again by guys from Rockchip,
so I send rebased against 4.10.10 version. Hope, someone might test it
and confirm that patches work fine.

Regards,
Alexander.

Alexander Kochetkov (2):
  dmaengine: pl330: make cyclic transfer free runnable
  dmaengine: pl330: don't emit code for one iteration loop

 drivers/dma/pl330.c |  200 +--
 1 file changed, 98 insertions(+), 102 deletions(-)

-- 
1.7.9.5

[PATCH 0/2] Free running cyclic transfer implementation for pl330

[Alexander Kochetkov]

Hello!

This series contain free running cyclic transfer implementation for pl330.
It affect ALL chips using pl330 (not only rockchip) and
allow to run cyclic transfers without CPU intervention. As a result
it fix sound clicks (observed and not yet observed) because
sound clicks must be heard under heavy system load due to the way
how cyclic transfers implemented now for pl330.

My previous series[1] doesn't get enough attention (no one except me
tested it). And it don't get upstream:

> 8-03-2016, 6:03, Vinod Koul  *:
>Overall this series looks okay, but can someone test this. I would not like
pl330 to be broken again

Now I was asked about the series[1] again by guys from Rockchip,
so I send rebased against 4.10.10 version. Hope, someone might test it
and confirm that patches work fine.

Regards,
Alexander.

Alexander Kochetkov (2):
  dmaengine: pl330: make cyclic transfer free runnable
  dmaengine: pl330: don't emit code for one iteration loop

 drivers/dma/pl330.c |  200 +--
 1 file changed, 98 insertions(+), 102 deletions(-)

-- 
1.7.9.5

Re: [PATCH v7 0/7] Implement clocksource for rockchip SoC using rockchip timer

[Alexander Kochetkov]

Hello, Daniel.

Due to recent comments from Mark[1], may be is better to apply v6[2] series 
instead of v7[3]?
Because my main goal was to fix wall time on rk3188. And I did it the same way 
how that was
already done for other timer drivers (one of possible solution).

You can rename CLOCKSOURCE_OF to TIMER_OF later. I can help with that, but I 
don’t
think it is good idea to combine my changes and timer framework 
cleanups/improvements
into single series.

And I thinks, that probably is is better to drop [3] and [4] and revert 
0c8893c9095d ("clockevents: Add a
clkevt-of mechanism like clksrc-of»).

What do you think?

[1] https://lkml.org/lkml/2017/3/29/286
[2] https://lkml.org/lkml/2017/1/31/401
[3] https://lkml.org/lkml/2017/3/22/508
[4] https://lkml.org/lkml/2017/3/22/420
[5] https://lkml.org/lkml/2017/3/22/426

> 22 марта 2017 г., в 18:48, Alexander Kochetkov <al.koc...@gmail.com> 
> написал(а):
> 
> Hello, Daniel, Heiko.
> 
> Here is try 7 :) Could you please take a look into it?
> 
> rockchip_timer init code implemented using CLOCKEVENT_OF_DECLARE()
> introduced in commit 0c8893c9095d ("clockevents: Add a clkevt-of
> mechanism like clksrc-of")
> 
> There is change in the arch/arm/mach-rockchip/rockchip.c.
> 
> This series should be applied after the commit:
> https://lkml.org/lkml/2017/3/22/426
> 
> As without the commit, you will get linker error ("clkevt-probe.c:63:
> undefined reference to `__clkevt_of_table’")
> 
> Regards,
> Alexander.
> 
> 
> Alexander Kochetkov (6):
>  dt-bindings: clarify compatible property for rockchip timers
>  ARM: dts: rockchip: update compatible property for rk322x timer
>  ARM: dts: rockchip: add clockevent attribute to rockchip timers
>  clocksource/drivers/rockchip_timer: implement clocksource timer
>  ARM: dts: rockchip: add timer entries to rk3188 SoC
>  ARM: dts: rockchip: disable arm-global-timer for rk3188
> 
> Daniel Lezcano (1):
>  clocksource/drivers/clksrc-evt-probe: Describe with the DT both the
>clocksource and the clockevent
> 
> .../bindings/timer/rockchip,rk-timer.txt   |   12 +-
> Documentation/devicetree/bindings/timer/timer.txt  |   38 
> arch/arm/boot/dts/rk3036.dtsi  |1 +
> arch/arm/boot/dts/rk3188.dtsi  |   19 ++
> arch/arm/boot/dts/rk322x.dtsi  |3 +-
> arch/arm/boot/dts/rk3288.dtsi  |1 +
> arch/arm/mach-rockchip/rockchip.c  |2 +
> arch/arm64/boot/dts/rockchip/rk3368.dtsi   |1 +
> drivers/clocksource/Kconfig|2 +
> drivers/clocksource/clkevt-probe.c |7 +
> drivers/clocksource/clksrc-probe.c |7 +
> drivers/clocksource/rockchip_timer.c   |  215 ++--
> 12 files changed, 241 insertions(+), 67 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
> 
> -- 
> 1.7.9.5
> 

Re: [PATCH v7 0/7] Implement clocksource for rockchip SoC using rockchip timer

[Alexander Kochetkov]

Hello, Daniel.

Due to recent comments from Mark[1], may be is better to apply v6[2] series 
instead of v7[3]?
Because my main goal was to fix wall time on rk3188. And I did it the same way 
how that was
already done for other timer drivers (one of possible solution).

You can rename CLOCKSOURCE_OF to TIMER_OF later. I can help with that, but I 
don’t
think it is good idea to combine my changes and timer framework 
cleanups/improvements
into single series.

And I thinks, that probably is is better to drop [3] and [4] and revert 
0c8893c9095d ("clockevents: Add a
clkevt-of mechanism like clksrc-of»).

What do you think?

[1] https://lkml.org/lkml/2017/3/29/286
[2] https://lkml.org/lkml/2017/1/31/401
[3] https://lkml.org/lkml/2017/3/22/508
[4] https://lkml.org/lkml/2017/3/22/420
[5] https://lkml.org/lkml/2017/3/22/426

> 22 марта 2017 г., в 18:48, Alexander Kochetkov  
> написал(а):
> 
> Hello, Daniel, Heiko.
> 
> Here is try 7 :) Could you please take a look into it?
> 
> rockchip_timer init code implemented using CLOCKEVENT_OF_DECLARE()
> introduced in commit 0c8893c9095d ("clockevents: Add a clkevt-of
> mechanism like clksrc-of")
> 
> There is change in the arch/arm/mach-rockchip/rockchip.c.
> 
> This series should be applied after the commit:
> https://lkml.org/lkml/2017/3/22/426
> 
> As without the commit, you will get linker error ("clkevt-probe.c:63:
> undefined reference to `__clkevt_of_table’")
> 
> Regards,
> Alexander.
> 
> 
> Alexander Kochetkov (6):
>  dt-bindings: clarify compatible property for rockchip timers
>  ARM: dts: rockchip: update compatible property for rk322x timer
>  ARM: dts: rockchip: add clockevent attribute to rockchip timers
>  clocksource/drivers/rockchip_timer: implement clocksource timer
>  ARM: dts: rockchip: add timer entries to rk3188 SoC
>  ARM: dts: rockchip: disable arm-global-timer for rk3188
> 
> Daniel Lezcano (1):
>  clocksource/drivers/clksrc-evt-probe: Describe with the DT both the
>clocksource and the clockevent
> 
> .../bindings/timer/rockchip,rk-timer.txt   |   12 +-
> Documentation/devicetree/bindings/timer/timer.txt  |   38 
> arch/arm/boot/dts/rk3036.dtsi  |1 +
> arch/arm/boot/dts/rk3188.dtsi  |   19 ++
> arch/arm/boot/dts/rk322x.dtsi  |3 +-
> arch/arm/boot/dts/rk3288.dtsi  |1 +
> arch/arm/mach-rockchip/rockchip.c  |2 +
> arch/arm64/boot/dts/rockchip/rk3368.dtsi   |1 +
> drivers/clocksource/Kconfig|2 +
> drivers/clocksource/clkevt-probe.c |7 +
> drivers/clocksource/clksrc-probe.c |7 +
> drivers/clocksource/rockchip_timer.c   |  215 ++--
> 12 files changed, 241 insertions(+), 67 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/timer/timer.txt
> 
> -- 
> 1.7.9.5
> 

[PATCH] clk: rockchip: limit i2s0_pre max rate on rk3188

[Alexander Kochetkov]

Change i2s0_pre from 768MHz to 192MHz and limit it's rate to 192MHz in
order to fix issues described below.

If right after the boot change i2s0_clk to 16.384MHz, real rate on
i2s0_clk pin may differ from 16.384MHz. The issue is random. Sometimes
rate on i2s0_clk pin equal to 16.384MHz, sometimes not.

There is another 100% reproducable issue. First we have to boot and see
the correct frequency on i2s0_clk pin (16.384MHz). Then we change its rate
to 8.192MHz (and it changes), then we change its rate again to 16.384MHz.
Rate leaves unchanged and equal to 8.192MHz.

'clk_summary' shows following clock connection in all the cases, where
rate was set to 16.384MHz (even then real rate differs).

clock   rate
-   
xin24m  2400
  pll_gpll  76800
 gpll   76800
i2s_src 76800
   i2s0_pre 76800
  i2s0_frac 16384000
 sclk_i2s0  16384000

Also I found, that if I change i2s0_pre to 192MHz all the issues described
above gone. I supposed that the issues is due to high i2s0_pre rate.
Probably rk3188 has some sort of frequency restrictions imposed. Haven't
found anything in the RK3188 TRM, so this is my assumption.

Anyway, with the patch, all the issues gone.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
---
 drivers/clk/rockchip/clk-rk3188.c |9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/rockchip/clk-rk3188.c 
b/drivers/clk/rockchip/clk-rk3188.c
index d0e722a..e883f48 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -849,6 +849,15 @@ static void __init rk3188a_clk_init(struct device_node *np)
__func__);
}
 
+   /* limit i2s0_pre max rate */
+   clk1 = __clk_lookup("aclk_cpu_pre");
+   clk2 = __clk_lookup("i2s0_pre");
+   if (clk1 && clk2) {
+   rate = clk_get_rate(clk1);
+   clk_set_max_rate(clk2, rate);
+   clk_set_rate(clk2, rate);
+   }
+
rockchip_clk_protect_critical(rk3188_critical_clocks,
  ARRAY_SIZE(rk3188_critical_clocks));
rockchip_clk_of_add_provider(np, ctx);
-- 
1.7.9.5

[PATCH] clk: rockchip: limit i2s0_pre max rate on rk3188

[Alexander Kochetkov]

Change i2s0_pre from 768MHz to 192MHz and limit it's rate to 192MHz in
order to fix issues described below.

If right after the boot change i2s0_clk to 16.384MHz, real rate on
i2s0_clk pin may differ from 16.384MHz. The issue is random. Sometimes
rate on i2s0_clk pin equal to 16.384MHz, sometimes not.

There is another 100% reproducable issue. First we have to boot and see
the correct frequency on i2s0_clk pin (16.384MHz). Then we change its rate
to 8.192MHz (and it changes), then we change its rate again to 16.384MHz.
Rate leaves unchanged and equal to 8.192MHz.

'clk_summary' shows following clock connection in all the cases, where
rate was set to 16.384MHz (even then real rate differs).

clock   rate
-   
xin24m  2400
  pll_gpll  76800
 gpll   76800
i2s_src 76800
   i2s0_pre 76800
  i2s0_frac 16384000
 sclk_i2s0  16384000

Also I found, that if I change i2s0_pre to 192MHz all the issues described
above gone. I supposed that the issues is due to high i2s0_pre rate.
Probably rk3188 has some sort of frequency restrictions imposed. Haven't
found anything in the RK3188 TRM, so this is my assumption.

Anyway, with the patch, all the issues gone.

Signed-off-by: Alexander Kochetkov 
---
 drivers/clk/rockchip/clk-rk3188.c |9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/rockchip/clk-rk3188.c 
b/drivers/clk/rockchip/clk-rk3188.c
index d0e722a..e883f48 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -849,6 +849,15 @@ static void __init rk3188a_clk_init(struct device_node *np)
__func__);
}
 
+   /* limit i2s0_pre max rate */
+   clk1 = __clk_lookup("aclk_cpu_pre");
+   clk2 = __clk_lookup("i2s0_pre");
+   if (clk1 && clk2) {
+   rate = clk_get_rate(clk1);
+   clk_set_max_rate(clk2, rate);
+   clk_set_rate(clk2, rate);
+   }
+
rockchip_clk_protect_critical(rk3188_critical_clocks,
  ARRAY_SIZE(rk3188_critical_clocks));
rockchip_clk_of_add_provider(np, ctx);
-- 
1.7.9.5

Re: [PATCH v7 5/7] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

The patch series should be applied after the patches [1] and [2] haven’t merged 
yet into the kernel.
That mention in the cover letter.

[1] https://lkml.org/lkml/2017/3/22/420
[2] https://lkml.org/lkml/2017/3/22/426

> 24 марта 2017 г., в 11:29, kbuild test robot <l...@intel.com> написал(а):
> 
> Hi Alexander,
> 
> [auto build test ERROR on tip/timers/core]
> [also build test ERROR on v4.11-rc3 next-20170323]
> [cannot apply to rockchip/for-next]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Alexander-Kochetkov/Implement-clocksource-for-rockchip-SoC-using-rockchip-timer/20170324-113008
> config: arm64-defconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>wget 
> https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
>chmod +x ~/bin/make.cross
># save the attached .config to linux build tree
>make.cross ARCH=arm64 
> 
> All errors (new ones prefixed by >>):
> 
>   In file included from include/linux/tick.h:7:0,
>from drivers//acpi/processor_idle.c:32:
>>> include/linux/clockchips.h:232:2: error: invalid preprocessing directive 
>>> #els
>#els
> ^~~
>>> include/linux/clockchips.h:233:19: error: static declaration of 
>>> 'clockevent_probe' follows non-static declaration
>static inline int clockevent_probe(void) { return 0; }
>  ^~~~
>   include/linux/clockchips.h:231:12: note: previous declaration of 
> 'clockevent_probe' was here
>extern int clockevent_probe(void);
>   ^~~~
> --
>>> drivers//clocksource/clkevt-probe.c:20:29: fatal error: linux/clockchip.h: 
>>> No such file or directory
>#include 
>^
>   compilation terminated.
> 
> vim +232 include/linux/clockchips.h
> 
> d316c57f Thomas Gleixner 2007-02-16  226  
> 376bc271 Daniel Lezcano  2016-04-19  227  #define CLOCKEVENT_OF_DECLARE(name, 
> compat, fn) \
> 376bc271 Daniel Lezcano  2016-04-19  228  OF_DECLARE_1_RET(clkevt, name, 
> compat, fn)
> 376bc271 Daniel Lezcano  2016-04-19  229  
> 376bc271 Daniel Lezcano  2016-04-19  230  #ifdef CONFIG_CLKEVT_PROBE
> 376bc271 Daniel Lezcano  2016-04-19  231  extern int clockevent_probe(void);
> 376bc271 Daniel Lezcano  2016-04-19 @232  #els
> 376bc271 Daniel Lezcano  2016-04-19 @233  static inline int 
> clockevent_probe(void) { return 0; }
> 376bc271 Daniel Lezcano  2016-04-19  234  #endif
> 376bc271 Daniel Lezcano  2016-04-19  235  
> 9eed56e8 Ingo Molnar 2015-04-02  236  #endif /* _LINUX_CLOCKCHIPS_H */
> 
> :: The code at line 232 was first introduced by commit
> :: 376bc27150f180d9f5eddec6a14117780177589d clockevents: Add a clkevt-of 
> mechanism like clksrc-of
> 
> :: TO: Daniel Lezcano <daniel.lezc...@linaro.org>
> :: CC: Daniel Lezcano <daniel.lezc...@linaro.org>
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
> <.config.gz>

Re: [PATCH v7 5/7] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

The patch series should be applied after the patches [1] and [2] haven’t merged 
yet into the kernel.
That mention in the cover letter.

[1] https://lkml.org/lkml/2017/3/22/420
[2] https://lkml.org/lkml/2017/3/22/426

> 24 марта 2017 г., в 11:29, kbuild test robot  написал(а):
> 
> Hi Alexander,
> 
> [auto build test ERROR on tip/timers/core]
> [also build test ERROR on v4.11-rc3 next-20170323]
> [cannot apply to rockchip/for-next]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Alexander-Kochetkov/Implement-clocksource-for-rockchip-SoC-using-rockchip-timer/20170324-113008
> config: arm64-defconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>wget 
> https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
>chmod +x ~/bin/make.cross
># save the attached .config to linux build tree
>make.cross ARCH=arm64 
> 
> All errors (new ones prefixed by >>):
> 
>   In file included from include/linux/tick.h:7:0,
>from drivers//acpi/processor_idle.c:32:
>>> include/linux/clockchips.h:232:2: error: invalid preprocessing directive 
>>> #els
>#els
> ^~~
>>> include/linux/clockchips.h:233:19: error: static declaration of 
>>> 'clockevent_probe' follows non-static declaration
>static inline int clockevent_probe(void) { return 0; }
>  ^~~~
>   include/linux/clockchips.h:231:12: note: previous declaration of 
> 'clockevent_probe' was here
>extern int clockevent_probe(void);
>   ^~~~
> --
>>> drivers//clocksource/clkevt-probe.c:20:29: fatal error: linux/clockchip.h: 
>>> No such file or directory
>#include 
>^
>   compilation terminated.
> 
> vim +232 include/linux/clockchips.h
> 
> d316c57f Thomas Gleixner 2007-02-16  226  
> 376bc271 Daniel Lezcano  2016-04-19  227  #define CLOCKEVENT_OF_DECLARE(name, 
> compat, fn) \
> 376bc271 Daniel Lezcano  2016-04-19  228  OF_DECLARE_1_RET(clkevt, name, 
> compat, fn)
> 376bc271 Daniel Lezcano  2016-04-19  229  
> 376bc271 Daniel Lezcano  2016-04-19  230  #ifdef CONFIG_CLKEVT_PROBE
> 376bc271 Daniel Lezcano  2016-04-19  231  extern int clockevent_probe(void);
> 376bc271 Daniel Lezcano  2016-04-19 @232  #els
> 376bc271 Daniel Lezcano  2016-04-19 @233  static inline int 
> clockevent_probe(void) { return 0; }
> 376bc271 Daniel Lezcano  2016-04-19  234  #endif
> 376bc271 Daniel Lezcano  2016-04-19  235  
> 9eed56e8 Ingo Molnar 2015-04-02  236  #endif /* _LINUX_CLOCKCHIPS_H */
> 
> :: The code at line 232 was first introduced by commit
> :: 376bc27150f180d9f5eddec6a14117780177589d clockevents: Add a clkevt-of 
> mechanism like clksrc-of
> 
> :: TO: Daniel Lezcano 
> :: CC: Daniel Lezcano 
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
> <.config.gz>

Re: [PATCH] ARM: dts: rockchip: increase SD clock frequency on Radxa Rock

[Alexander Kochetkov]

> 22 марта 2017 г., в 20:04, Alexander Kochetkov <al.koc...@gmail.com> 
> написал(а):
> 
> Ok. Let leave as is for now.
> 
> My radxa worked stable for a year with this settings.

Ok, I not sure what my radxa worked stable with this settings.
Hieko, thanks for catching that.

Sorry for your time.

Re: [PATCH] ARM: dts: rockchip: increase SD clock frequency on Radxa Rock

[Alexander Kochetkov]

> 22 марта 2017 г., в 20:04, Alexander Kochetkov  
> написал(а):
> 
> Ok. Let leave as is for now.
> 
> My radxa worked stable for a year with this settings.

Ok, I not sure what my radxa worked stable with this settings.
Hieko, thanks for catching that.

Sorry for your time.

Re: [PATCH] ARM: dts: rockchip: increase SD clock frequency on Radxa Rock

[Alexander Kochetkov]

> 22 марта 2017 г., в 19:47, Heiko Stuebner  написал(а):
> 
> sorry, but I will not apply this patch at this time.

> When testing on my radxarock with the card I always use upon entering the 
> rootfs everything explodes with -110 errors, while without that patch the 
> card 
> runs stable as far as I can tell.

> I also don't have the time to investigate further right now though.

Ok. Let leave as is for now.

My radxa worked stable for a year with this settings.
And my custom rk3188 based board also has this setting. Kernel 4.8.
And I saw -110 errors only then I disconnect SD-card from board.

Re: [PATCH] ARM: dts: rockchip: increase SD clock frequency on Radxa Rock

[Alexander Kochetkov]

> 22 марта 2017 г., в 19:47, Heiko Stuebner  написал(а):
> 
> sorry, but I will not apply this patch at this time.

> When testing on my radxarock with the card I always use upon entering the 
> rootfs everything explodes with -110 errors, while without that patch the 
> card 
> runs stable as far as I can tell.

> I also don't have the time to investigate further right now though.

Ok. Let leave as is for now.

My radxa worked stable for a year with this settings.
And my custom rk3188 based board also has this setting. Kernel 4.8.
And I saw -110 errors only then I disconnect SD-card from board.

Re: [PATCH] ARM: dts: rockchip: setup DMA-channels for mmc0 and emmc for rk3188

[Alexander Kochetkov]

Hello, Heiko!

> 22 марта 2017 г., в 18:54, Heiko Stuebner  написал(а):
> 
> I've applied a slightly different variant in [0] with your commit message and
> moved the dma properties to the mmc/emmc nodes in rk3xxx.dtsi - as the dma
> channels are the same on both rk3188 and rk3066.

Thank you! I had changes in the rk3xxx.dtsi initially, but them moved them
into rk3188. Don’t know why I did that. May be because I don’t have rk3066
based board to test on.

> While at it, I've also added the sdio dma as there is no need to leave it out.

I’ve tested mmc0 and emm and they work great. I haven’t tested sdio.

> 
> Also when changing the devicetree, please do not append stuff at the bottom
> but instead sort new nodes (the   in this case) and properties
> alphabetically.

Ok. Thank you.

I have working settings for eMMC for radxa rock. What do you think, is it good 
idea to add them
to the kernel DT? As Radxa Rock doesn’t come with eMMC, but eMMC can be 
soldered manually.

Alexander.

Re: [PATCH] ARM: dts: rockchip: setup DMA-channels for mmc0 and emmc for rk3188

[Alexander Kochetkov]

Hello, Heiko!

> 22 марта 2017 г., в 18:54, Heiko Stuebner  написал(а):
> 
> I've applied a slightly different variant in [0] with your commit message and
> moved the dma properties to the mmc/emmc nodes in rk3xxx.dtsi - as the dma
> channels are the same on both rk3188 and rk3066.

Thank you! I had changes in the rk3xxx.dtsi initially, but them moved them
into rk3188. Don’t know why I did that. May be because I don’t have rk3066
based board to test on.

> While at it, I've also added the sdio dma as there is no need to leave it out.

I’ve tested mmc0 and emm and they work great. I haven’t tested sdio.

> 
> Also when changing the devicetree, please do not append stuff at the bottom
> but instead sort new nodes (the   in this case) and properties
> alphabetically.

Ok. Thank you.

I have working settings for eMMC for radxa rock. What do you think, is it good 
idea to add them
to the kernel DT? As Radxa Rock doesn’t come with eMMC, but eMMC can be 
soldered manually.

Alexander.

[PATCH v7 7/7] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

The clocksource and the sched_clock provided by the arm_global_timer
are quite unstable because their rates depend on the cpu frequency.

On the other side, the arm_global_timer has a higher rating than the
rockchip_timer, it will be selected by default by the time framework
while we want to use the stable rockchip clocksource.

Let's disable the arm_global_timer in order to have the rockchip
clocksource selected by default.

Signed-off-by: Alexander Kochetkov <al.koc...@gmail.com>
Reviewed-by: Heiko Stuebner <he...@sntech.de>
---
 arch/arm/boot/dts/rk3188.dtsi |1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index a20d501..c316468 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -548,6 +548,7 @@
 
 _timer {
interrupts = ;
+   status = "disabled";
 };
 
 _timer {
-- 
1.7.9.5