Re: [U-Boot] [PATCH v2 1/2] at91: video: Support driver-model for the HLCD driver

2017-03-13 Thread Wu, Songjun 

Hi Marek,

Thank you for your comments.
I will fix these issues in next patch.

On 3/13/2017 13:05, Marek Vasut wrote:

On 03/01/2017 10:25 AM, Songjun Wu wrote:

Add driver-model support to this driver.

Signed-off-by: Songjun Wu 
---

Changes in v2:
- Due to the peripheral clock driver improvement, remove
  the unneccessary clock calling.

 board/atmel/at91sam9n12ek/at91sam9n12ek.c   |   2 +
 board/atmel/at91sam9x5ek/at91sam9x5ek.c |   2 +
 board/atmel/sama5d2_xplained/sama5d2_xplained.c |   2 +
 board/atmel/sama5d3xek/sama5d3xek.c |   2 +
 board/atmel/sama5d4_xplained/sama5d4_xplained.c |   2 +
 board/atmel/sama5d4ek/sama5d4ek.c   |   2 +
 drivers/video/Kconfig   |   9 +
 drivers/video/atmel_hlcdfb.c| 314 +++-


Split the driver from board changes.


 include/configs/at91sam9n12ek.h |   4 +
 include/configs/at91sam9x5ek.h  |   4 +
 include/configs/ma5d4evk.h  |   4 +
 include/configs/sama5d2_xplained.h  |   4 +
 include/configs/sama5d3xek.h|   4 +
 include/configs/sama5d4_xplained.h  |   4 +
 include/configs/sama5d4ek.h |   4 +
 15 files changed, 360 insertions(+), 3 deletions(-)


[...]


diff --git a/drivers/video/atmel_hlcdfb.c b/drivers/video/atmel_hlcdfb.c
index 960b474b76..0bcb92b2cf 100644
--- a/drivers/video/atmel_hlcdfb.c
+++ b/drivers/video/atmel_hlcdfb.c
@@ -10,13 +10,24 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 #include 
+#include 
 #include 

 #if defined(CONFIG_LCD_LOGO)
 #include 
 #endif

+DECLARE_GLOBAL_DATA_PTR;
+
+#define lcdc_readl(reg)__raw_readl((reg))
+#define lcdc_writel(reg, val)  __raw_writel((val), (reg))


Really ? Do we REALLY need more new accessors ? Just use readl/writel ...


+#ifndef CONFIG_DM_VIDEO
+
 /* configurable parameters */
 #define ATMEL_LCDC_CVAL_DEFAULT0xc8
 #define ATMEL_LCDC_DMA_BURST_LEN   8
@@ -26,9 +37,6 @@

 #define ATMEL_LCDC_FIFO_SIZE   512

-#define lcdc_readl(reg)__raw_readl((reg))
-#define lcdc_writel(reg, val)  __raw_writel((val), (reg))
-
 /*
  * the CLUT register map as following
  * RCLUT(24 ~ 16), GCLUT(15 ~ 8), BCLUT(7 ~ 0)
@@ -218,3 +226,303 @@ void lcd_ctrl_init(void *lcdbase)
/* Enable flushing if we enabled dcache */
lcd_set_flush_dcache(1);
 }
+
+#else
+
+enum {
+   LCD_MAX_WIDTH   = 1024,
+   LCD_MAX_HEIGHT  = 768,
+   LCD_MAX_LOG2_BPP= VIDEO_BPP16,
+};
+
+struct atmel_hlcdc_priv {
+   struct atmel_hlcd_regs *regs;
+   struct display_timing timing;
+   unsigned int vl_bpix;
+   unsigned int guard_time;
+   ulong clk_rate;
+};
+
+static int at91_hlcdc_enable_clk(struct udevice *dev)
+{
+   struct atmel_hlcdc_priv *priv = dev_get_priv(dev);
+   struct clk clk;
+   ulong clk_rate;
+   int ret;
+
+   ret = clk_get_by_index(dev, 0, );
+   if (ret)
+   return -EINVAL;
+
+   ret = clk_enable();
+   if (ret)
+   return ret;
+
+   clk_rate = clk_get_rate();
+   if (!clk_rate)
+   return -ENODEV;


Clock are still enabled if you fail here ...


+   priv->clk_rate = clk_rate;
+
+   clk_free();
+
+   return 0;
+}
+
+static void atmel_hlcdc_init(struct udevice *dev)
+{
+   struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
+   struct atmel_hlcdc_priv *priv = dev_get_priv(dev);
+   struct atmel_hlcd_regs *regs = priv->regs;
+   struct display_timing *timing = >timing;
+   struct lcd_dma_desc *desc;
+   unsigned long value, vl_clk_pol;
+
+   /* Disable DISP signal */
+   lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_DISPDIS);
+   while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_DISPSTS))
+   udelay(1);


wait_for_bit(), fix globally ... and don't use unbounded loops ...


+   /* Disable synchronization */
+   lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_SYNCDIS);
+   while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_LCDSTS))
+   udelay(1);
+   /* Disable pixel clock */
+   lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_CLKDIS);
+   while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_CLKSTS))
+   udelay(1);
+   /* Disable PWM */
+   lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_PWMDIS);
+   while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_PWMSTS))
+   udelay(1);
+
+   /* Set pixel clock */
+   value = priv->clk_rate / timing->pixelclock.typ;
+   if (priv->clk_rate % timing->pixelclock.typ)
+   value++;
+
+   vl_clk_pol = 0;
+   if (timing->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+   vl_clk_pol = LCDC_LCDCFG0_CLKPOL;
+
+   if (value < 1) {


I guess I'd just introduce a variable and ORR it with either CLKSEL or
whatever bits based on the value

Re: [U-Boot] [PATCH v2 1/2] at91: video: Support driver-model for the HLCD driver

2017-03-12 Thread Marek Vasut 
On 03/01/2017 10:25 AM, Songjun Wu wrote:
> Add driver-model support to this driver.
> 
> Signed-off-by: Songjun Wu 
> ---
> 
> Changes in v2:
> - Due to the peripheral clock driver improvement, remove
>   the unneccessary clock calling.
> 
>  board/atmel/at91sam9n12ek/at91sam9n12ek.c   |   2 +
>  board/atmel/at91sam9x5ek/at91sam9x5ek.c |   2 +
>  board/atmel/sama5d2_xplained/sama5d2_xplained.c |   2 +
>  board/atmel/sama5d3xek/sama5d3xek.c |   2 +
>  board/atmel/sama5d4_xplained/sama5d4_xplained.c |   2 +
>  board/atmel/sama5d4ek/sama5d4ek.c   |   2 +
>  drivers/video/Kconfig   |   9 +
>  drivers/video/atmel_hlcdfb.c| 314 
> +++-

Split the driver from board changes.

>  include/configs/at91sam9n12ek.h |   4 +
>  include/configs/at91sam9x5ek.h  |   4 +
>  include/configs/ma5d4evk.h  |   4 +
>  include/configs/sama5d2_xplained.h  |   4 +
>  include/configs/sama5d3xek.h|   4 +
>  include/configs/sama5d4_xplained.h  |   4 +
>  include/configs/sama5d4ek.h |   4 +
>  15 files changed, 360 insertions(+), 3 deletions(-)

[...]

> diff --git a/drivers/video/atmel_hlcdfb.c b/drivers/video/atmel_hlcdfb.c
> index 960b474b76..0bcb92b2cf 100644
> --- a/drivers/video/atmel_hlcdfb.c
> +++ b/drivers/video/atmel_hlcdfb.c
> @@ -10,13 +10,24 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  
>  #if defined(CONFIG_LCD_LOGO)
>  #include 
>  #endif
>  
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define lcdc_readl(reg)  __raw_readl((reg))
> +#define lcdc_writel(reg, val)__raw_writel((val), (reg))

Really ? Do we REALLY need more new accessors ? Just use readl/writel ...

> +#ifndef CONFIG_DM_VIDEO
> +
>  /* configurable parameters */
>  #define ATMEL_LCDC_CVAL_DEFAULT  0xc8
>  #define ATMEL_LCDC_DMA_BURST_LEN 8
> @@ -26,9 +37,6 @@
>  
>  #define ATMEL_LCDC_FIFO_SIZE 512
>  
> -#define lcdc_readl(reg)  __raw_readl((reg))
> -#define lcdc_writel(reg, val)__raw_writel((val), (reg))
> -
>  /*
>   * the CLUT register map as following
>   * RCLUT(24 ~ 16), GCLUT(15 ~ 8), BCLUT(7 ~ 0)
> @@ -218,3 +226,303 @@ void lcd_ctrl_init(void *lcdbase)
>   /* Enable flushing if we enabled dcache */
>   lcd_set_flush_dcache(1);
>  }
> +
> +#else
> +
> +enum {
> + LCD_MAX_WIDTH   = 1024,
> + LCD_MAX_HEIGHT  = 768,
> + LCD_MAX_LOG2_BPP= VIDEO_BPP16,
> +};
> +
> +struct atmel_hlcdc_priv {
> + struct atmel_hlcd_regs *regs;
> + struct display_timing timing;
> + unsigned int vl_bpix;
> + unsigned int guard_time;
> + ulong clk_rate;
> +};
> +
> +static int at91_hlcdc_enable_clk(struct udevice *dev)
> +{
> + struct atmel_hlcdc_priv *priv = dev_get_priv(dev);
> + struct clk clk;
> + ulong clk_rate;
> + int ret;
> +
> + ret = clk_get_by_index(dev, 0, );
> + if (ret)
> + return -EINVAL;
> +
> + ret = clk_enable();
> + if (ret)
> + return ret;
> +
> + clk_rate = clk_get_rate();
> + if (!clk_rate)
> + return -ENODEV;

Clock are still enabled if you fail here ...

> + priv->clk_rate = clk_rate;
> +
> + clk_free();
> +
> + return 0;
> +}
> +
> +static void atmel_hlcdc_init(struct udevice *dev)
> +{
> + struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
> + struct atmel_hlcdc_priv *priv = dev_get_priv(dev);
> + struct atmel_hlcd_regs *regs = priv->regs;
> + struct display_timing *timing = >timing;
> + struct lcd_dma_desc *desc;
> + unsigned long value, vl_clk_pol;
> +
> + /* Disable DISP signal */
> + lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_DISPDIS);
> + while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_DISPSTS))
> + udelay(1);

wait_for_bit(), fix globally ... and don't use unbounded loops ...

> + /* Disable synchronization */
> + lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_SYNCDIS);
> + while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_LCDSTS))
> + udelay(1);
> + /* Disable pixel clock */
> + lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_CLKDIS);
> + while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_CLKSTS))
> + udelay(1);
> + /* Disable PWM */
> + lcdc_writel(>lcdc_lcddis, LCDC_LCDDIS_PWMDIS);
> + while ((lcdc_readl(>lcdc_lcdsr) & LCDC_LCDSR_PWMSTS))
> + udelay(1);
> +
> + /* Set pixel clock */
> + value = priv->clk_rate / timing->pixelclock.typ;
> + if (priv->clk_rate % timing->pixelclock.typ)
> + value++;
> +
> + vl_clk_pol = 0;
> + if (timing->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
> + vl_clk_pol = LCDC_LCDCFG0_CLKPOL;
> +
> + if (value < 1) {

I guess I'd just introduce a variable and ORR it

[U-Boot] [PATCH v2 1/2] at91: video: Support driver-model for the HLCD driver

2017-03-06 Thread Songjun Wu 
Add driver-model support to this driver.

Signed-off-by: Songjun Wu 
---

Changes in v2:
- Due to the peripheral clock driver improvement, remove
  the unneccessary clock calling.

 board/atmel/at91sam9n12ek/at91sam9n12ek.c   |   2 +
 board/atmel/at91sam9x5ek/at91sam9x5ek.c |   2 +
 board/atmel/sama5d2_xplained/sama5d2_xplained.c |   2 +
 board/atmel/sama5d3xek/sama5d3xek.c |   2 +
 board/atmel/sama5d4_xplained/sama5d4_xplained.c |   2 +
 board/atmel/sama5d4ek/sama5d4ek.c   |   2 +
 drivers/video/Kconfig   |   9 +
 drivers/video/atmel_hlcdfb.c| 314 +++-
 include/configs/at91sam9n12ek.h |   4 +
 include/configs/at91sam9x5ek.h  |   4 +
 include/configs/ma5d4evk.h  |   4 +
 include/configs/sama5d2_xplained.h  |   4 +
 include/configs/sama5d3xek.h|   4 +
 include/configs/sama5d4_xplained.h  |   4 +
 include/configs/sama5d4ek.h |   4 +
 15 files changed, 360 insertions(+), 3 deletions(-)

diff --git a/board/atmel/at91sam9n12ek/at91sam9n12ek.c 
b/board/atmel/at91sam9n12ek/at91sam9n12ek.c
index fc4f50d219..e70a929299 100644
--- a/board/atmel/at91sam9n12ek/at91sam9n12ek.c
+++ b/board/atmel/at91sam9n12ek/at91sam9n12ek.c
@@ -79,6 +79,7 @@ static void at91sam9n12ek_nand_hw_init(void)
 #endif
 
 #ifdef CONFIG_LCD
+#ifndef CONFIG_DM_VIDEO
 vidinfo_t panel_info = {
.vl_col = 480,
.vl_row = 272,
@@ -104,6 +105,7 @@ void lcd_disable(void)
 {
at91_set_pio_output(AT91_PIO_PORTC, 25, 1); /* power down */
 }
+#endif
 
 #ifdef CONFIG_LCD_INFO
 void lcd_show_board_info(void)
diff --git a/board/atmel/at91sam9x5ek/at91sam9x5ek.c 
b/board/atmel/at91sam9x5ek/at91sam9x5ek.c
index b0d440d728..8504d604fa 100644
--- a/board/atmel/at91sam9x5ek/at91sam9x5ek.c
+++ b/board/atmel/at91sam9x5ek/at91sam9x5ek.c
@@ -108,6 +108,7 @@ int board_eth_init(bd_t *bis)
 }
 
 #ifdef CONFIG_LCD
+#ifndef CONFIG_DM_VIDEO
 vidinfo_t panel_info = {
.vl_col = 800,
.vl_row = 480,
@@ -136,6 +137,7 @@ void lcd_disable(void)
if (has_lcdc())
at91_set_a_periph(AT91_PIO_PORTC, 29, 0);   /* power down */
 }
+#endif
 
 static void at91sam9x5ek_lcd_hw_init(void)
 {
diff --git a/board/atmel/sama5d2_xplained/sama5d2_xplained.c 
b/board/atmel/sama5d2_xplained/sama5d2_xplained.c
index c5337af4de..6e8851aaf6 100644
--- a/board/atmel/sama5d2_xplained/sama5d2_xplained.c
+++ b/board/atmel/sama5d2_xplained/sama5d2_xplained.c
@@ -34,6 +34,7 @@ static void board_usb_hw_init(void)
 }
 
 #ifdef CONFIG_LCD
+#ifndef CONFIG_DM_VIDEO
 vidinfo_t panel_info = {
.vl_col = 480,
.vl_row = 272,
@@ -57,6 +58,7 @@ unsigned int has_lcdc(void)
 {
return 1;
 }
+#endif
 
 static void board_lcd_hw_init(void)
 {
diff --git a/board/atmel/sama5d3xek/sama5d3xek.c 
b/board/atmel/sama5d3xek/sama5d3xek.c
index ce67478f0b..a83e96575f 100644
--- a/board/atmel/sama5d3xek/sama5d3xek.c
+++ b/board/atmel/sama5d3xek/sama5d3xek.c
@@ -142,6 +142,7 @@ static void sama5d3xek_mci_hw_init(void)
 #endif
 
 #ifdef CONFIG_LCD
+#ifndef CONFIG_DM_VIDEO
 vidinfo_t panel_info = {
.vl_col = 800,
.vl_row = 480,
@@ -164,6 +165,7 @@ void lcd_enable(void)
 void lcd_disable(void)
 {
 }
+#endif
 
 static void sama5d3xek_lcd_hw_init(void)
 {
diff --git a/board/atmel/sama5d4_xplained/sama5d4_xplained.c 
b/board/atmel/sama5d4_xplained/sama5d4_xplained.c
index 23ec274468..6512429ba7 100644
--- a/board/atmel/sama5d4_xplained/sama5d4_xplained.c
+++ b/board/atmel/sama5d4_xplained/sama5d4_xplained.c
@@ -108,6 +108,7 @@ static void sama5d4_xplained_usb_hw_init(void)
 #endif
 
 #ifdef CONFIG_LCD
+#ifndef CONFIG_DM_VIDEO
 vidinfo_t panel_info = {
.vl_col = 480,
.vl_row = 272,
@@ -131,6 +132,7 @@ unsigned int has_lcdc(void)
 {
return 1;
 }
+#endif
 
 static void sama5d4_xplained_lcd_hw_init(void)
 {
diff --git a/board/atmel/sama5d4ek/sama5d4ek.c 
b/board/atmel/sama5d4ek/sama5d4ek.c
index 72bad23087..82b8590102 100644
--- a/board/atmel/sama5d4ek/sama5d4ek.c
+++ b/board/atmel/sama5d4ek/sama5d4ek.c
@@ -109,6 +109,7 @@ static void sama5d4ek_usb_hw_init(void)
 #endif
 
 #ifdef CONFIG_LCD
+#ifndef CONFIG_DM_VIDEO
 vidinfo_t panel_info = {
.vl_col = 800,
.vl_row = 480,
@@ -132,6 +133,7 @@ unsigned int has_lcdc(void)
 {
return 1;
 }
+#endif
 
 static void sama5d4ek_lcd_hw_init(void)
 {
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2069576958..44756ea18e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -371,6 +371,15 @@ config DISPLAY
   The devices provide a simple interface to start up the display,
   read display information and enable it.
 
+config ATMEL_HLCD
+   bool "Enable ATMEL video support using HLCDC"
+   depends on DM_VIDEO
+   help
+  HLCDC supports video output to an