[U-Boot] [PATCH v2 1/2] at91: video: Support driver-model for the HLCD driver
Wu, Songjun
Songjun.Wu at microchip.com
Mon Mar 13 08:19:58 UTC 2017
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 <songjun.wu at microchip.com>
>> ---
>>
>> 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 <asm/io.h>
>> #include <asm/arch/gpio.h>
>> #include <asm/arch/clk.h>
>> +#include <clk.h>
>> +#include <dm.h>
>> +#include <fdtdec.h>
>> #include <lcd.h>
>> +#include <video.h>
>> #include <atmel_hlcdc.h>
>>
>> #if defined(CONFIG_LCD_LOGO)
>> #include <bmp_logo.h>
>> #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, &clk);
>> + if (ret)
>> + return -EINVAL;
>> +
>> + ret = clk_enable(&clk);
>> + if (ret)
>> + return ret;
>> +
>> + clk_rate = clk_get_rate(&clk);
>> + if (!clk_rate)
>> + return -ENODEV;
>
> Clock are still enabled if you fail here ...
>
>> + priv->clk_rate = clk_rate;
>> +
>> + clk_free(&clk);
>> +
>> + 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 = &priv->timing;
>> + struct lcd_dma_desc *desc;
>> + unsigned long value, vl_clk_pol;
>> +
>> + /* Disable DISP signal */
>> + lcdc_writel(®s->lcdc_lcddis, LCDC_LCDDIS_DISPDIS);
>> + while ((lcdc_readl(®s->lcdc_lcdsr) & LCDC_LCDSR_DISPSTS))
>> + udelay(1);
>
> wait_for_bit(), fix globally ... and don't use unbounded loops ...
>
>> + /* Disable synchronization */
>> + lcdc_writel(®s->lcdc_lcddis, LCDC_LCDDIS_SYNCDIS);
>> + while ((lcdc_readl(®s->lcdc_lcdsr) & LCDC_LCDSR_LCDSTS))
>> + udelay(1);
>> + /* Disable pixel clock */
>> + lcdc_writel(®s->lcdc_lcddis, LCDC_LCDDIS_CLKDIS);
>> + while ((lcdc_readl(®s->lcdc_lcdsr) & LCDC_LCDSR_CLKSTS))
>> + udelay(1);
>> + /* Disable PWM */
>> + lcdc_writel(®s->lcdc_lcddis, LCDC_LCDDIS_PWMDIS);
>> + while ((lcdc_readl(®s->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 to reduce this lengthy condition here.
> ie u32 foo = LCDC_BAR | LCDC_BAZ;
> if (value < 1)
> foo |= LCDC_QUUX;
> else
> ...
>
> writel(...);
>
>> + /* Using system clock as pixel clock */
>> + lcdc_writel(®s->lcdc_lcdcfg0,
>> + LCDC_LCDCFG0_CLKDIV(0)
>> + | LCDC_LCDCFG0_CGDISHCR
>> + | LCDC_LCDCFG0_CGDISHEO
>> + | LCDC_LCDCFG0_CGDISOVR1
>> + | LCDC_LCDCFG0_CGDISBASE
>> + | vl_clk_pol
>> + | LCDC_LCDCFG0_CLKSEL);
>> +
>> + } else {
>> + lcdc_writel(®s->lcdc_lcdcfg0,
>> + LCDC_LCDCFG0_CLKDIV(value - 2)
>> + | LCDC_LCDCFG0_CGDISHCR
>> + | LCDC_LCDCFG0_CGDISHEO
>> + | LCDC_LCDCFG0_CGDISOVR1
>> + | LCDC_LCDCFG0_CGDISBASE
>> + | vl_clk_pol);
>> + }
>> +
>> + /* Initialize control register 5 */
>> + value = 0;
>> +
>> + if (!(timing->flags & DISPLAY_FLAGS_HSYNC_HIGH))
>> + value |= LCDC_LCDCFG5_HSPOL;
>> + if (!(timing->flags & DISPLAY_FLAGS_VSYNC_HIGH))
>> + value |= LCDC_LCDCFG5_VSPOL;
>> +
>> +#ifndef LCD_OUTPUT_BPP
>
> Just make sure that's always defined . If possible, get this from DT.
>
>> + /* Output is 24bpp */
>> + value |= LCDC_LCDCFG5_MODE_OUTPUT_24BPP;
>> +#else
>> + switch (LCD_OUTPUT_BPP) {
>> + case 12:
>> + value |= LCDC_LCDCFG5_MODE_OUTPUT_12BPP;
>> + break;
>> + case 16:
>> + value |= LCDC_LCDCFG5_MODE_OUTPUT_16BPP;
>> + break;
>> + case 18:
>> + value |= LCDC_LCDCFG5_MODE_OUTPUT_18BPP;
>> + break;
>> + case 24:
>> + value |= LCDC_LCDCFG5_MODE_OUTPUT_24BPP;
>> + break;
>> + default:
>> + BUG();
>> + break;
>> + }
>> +#endif
>> +
>> + value |= LCDC_LCDCFG5_GUARDTIME(priv->guard_time);
>> + value |= (LCDC_LCDCFG5_DISPDLY | LCDC_LCDCFG5_VSPDLYS);
>> + lcdc_writel(®s->lcdc_lcdcfg5, value);
>> +
>> + /* Vertical & Horizontal Timing */
>> + value = LCDC_LCDCFG1_VSPW(timing->vsync_len.typ - 1);
>> + value |= LCDC_LCDCFG1_HSPW(timing->hsync_len.typ - 1);
>> + lcdc_writel(®s->lcdc_lcdcfg1, value);
>> +
>> + value = LCDC_LCDCFG2_VBPW(timing->vback_porch.typ);
>> + value |= LCDC_LCDCFG2_VFPW(timing->vfront_porch.typ - 1);
>> + lcdc_writel(®s->lcdc_lcdcfg2, value);
>> +
>> + value = LCDC_LCDCFG3_HBPW(timing->hback_porch.typ - 1);
>> + value |= LCDC_LCDCFG3_HFPW(timing->hfront_porch.typ - 1);
>> + lcdc_writel(®s->lcdc_lcdcfg3, value);
>> +
>> + /* Display size */
>> + value = LCDC_LCDCFG4_RPF(timing->vactive.typ - 1);
>> + value |= LCDC_LCDCFG4_PPL(timing->hactive.typ - 1);
>> + lcdc_writel(®s->lcdc_lcdcfg4, value);
>> +
>> + lcdc_writel(®s->lcdc_basecfg0,
>> + LCDC_BASECFG0_BLEN_AHB_INCR4 | LCDC_BASECFG0_DLBO);
>> +
>> + switch (VNBITS(priv->vl_bpix)) {
>> + case 16:
>> + lcdc_writel(®s->lcdc_basecfg1,
>> + LCDC_BASECFG1_RGBMODE_16BPP_RGB_565);
>> + break;
>> + case 32:
>> + lcdc_writel(®s->lcdc_basecfg1,
>> + LCDC_BASECFG1_RGBMODE_24BPP_RGB_888);
>> + break;
>> + default:
>> + BUG();
>> + break;
>> + }
>> +
>> + lcdc_writel(®s->lcdc_basecfg2, LCDC_BASECFG2_XSTRIDE(0));
>> + lcdc_writel(®s->lcdc_basecfg3, 0);
>> + lcdc_writel(®s->lcdc_basecfg4, LCDC_BASECFG4_DMA);
>> +
>> + /* Disable all interrupts */
>> + lcdc_writel(®s->lcdc_lcdidr, ~0UL);
>> + lcdc_writel(®s->lcdc_baseidr, ~0UL);
>> +
>> + /* Setup the DMA descriptor, this descriptor will loop to itself */
>> + desc = (struct lcd_dma_desc *)(uc_plat->base - 16);
>> +
>> + desc->address = (u32)uc_plat->base;
>> +
>> + /* Disable DMA transfer interrupt & descriptor loaded interrupt. */
>> + desc->control = LCDC_BASECTRL_ADDIEN | LCDC_BASECTRL_DSCRIEN
>> + | LCDC_BASECTRL_DMAIEN | LCDC_BASECTRL_DFETCH;
>> + desc->next = (u32)desc;
>> +
>> + /* Flush the DMA descriptor if we enabled dcache */
>> + flush_dcache_range((u32)desc, (u32)desc + sizeof(*desc));
>> +
>> + lcdc_writel(®s->lcdc_baseaddr, desc->address);
>> + lcdc_writel(®s->lcdc_basectrl, desc->control);
>> + lcdc_writel(®s->lcdc_basenext, desc->next);
>> + lcdc_writel(®s->lcdc_basecher,
>> + LCDC_BASECHER_CHEN | LCDC_BASECHER_UPDATEEN);
>> +
>> + /* Enable LCD */
>> + value = lcdc_readl(®s->lcdc_lcden);
>> + lcdc_writel(®s->lcdc_lcden, value | LCDC_LCDEN_CLKEN);
>> + while (!(lcdc_readl(®s->lcdc_lcdsr) & LCDC_LCDSR_CLKSTS))
>> + udelay(1);
>
> wait_for_bit() ...
>
>> + value = lcdc_readl(®s->lcdc_lcden);
>> + lcdc_writel(®s->lcdc_lcden, value | LCDC_LCDEN_SYNCEN);
>> + while (!(lcdc_readl(®s->lcdc_lcdsr) & LCDC_LCDSR_LCDSTS))
>> + udelay(1);
>> + value = lcdc_readl(®s->lcdc_lcden);
>> + lcdc_writel(®s->lcdc_lcden, value | LCDC_LCDEN_DISPEN);
>> + while (!(lcdc_readl(®s->lcdc_lcdsr) & LCDC_LCDSR_DISPSTS))
>> + udelay(1);
>> + value = lcdc_readl(®s->lcdc_lcden);
>> + lcdc_writel(®s->lcdc_lcden, value | LCDC_LCDEN_PWMEN);
>> + while (!(lcdc_readl(®s->lcdc_lcdsr) & LCDC_LCDSR_PWMSTS))
>> + udelay(1);
>> +}
>> +
>> +static int atmel_hlcdc_probe(struct udevice *dev)
>> +{
>> + struct video_priv *uc_priv = dev_get_uclass_priv(dev);
>> + struct atmel_hlcdc_priv *priv = dev_get_priv(dev);
>> + int ret;
>> +
>> + ret = at91_hlcdc_enable_clk(dev);
>> + if (ret)
>> + return ret;
>> +
>> + atmel_hlcdc_init(dev);
>> +
>> + uc_priv->xsize = priv->timing.hactive.typ;
>> + uc_priv->ysize = priv->timing.vactive.typ;
>> + uc_priv->bpix = priv->vl_bpix;
>> +
>> + /* Enable flushing if we enabled dcache */
>> + video_set_flush_dcache(dev, true);
>> +
>> + return 0;
>> +}
>> +
>> +static int atmel_hlcdc_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + struct atmel_hlcdc_priv *priv = dev_get_priv(dev);
>> + const void *blob = gd->fdt_blob;
>> + int node = dev->of_offset;
>> +
>> + priv->regs = (struct atmel_hlcd_regs *)dev_get_addr(dev);
>> + if (!priv->regs) {
>> + debug("%s: No display controller address\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + if (fdtdec_decode_display_timing(blob, dev->of_offset,
>> + 0, &priv->timing)) {
>> + debug("%s: Failed to decode display timing\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + if (priv->timing.hactive.typ > LCD_MAX_WIDTH)
>> + priv->timing.hactive.typ = LCD_MAX_WIDTH;
>> +
>> + if (priv->timing.vactive.typ > LCD_MAX_HEIGHT)
>> + priv->timing.vactive.typ = LCD_MAX_HEIGHT;
>> +
>> + priv->vl_bpix = fdtdec_get_int(blob, node, "atmel,vl-bpix", 0);
>> + if (!priv->vl_bpix) {
>> + debug("%s: Failed to get bits per pixel\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + priv->guard_time = fdtdec_get_int(blob, node, "atmel,guard-time", 1);
>> +
>> + return 0;
>> +}
>> +
>> +static int atmel_hlcdc_bind(struct udevice *dev)
>> +{
>> + struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
>> +
>> + uc_plat->size = LCD_MAX_WIDTH * LCD_MAX_HEIGHT *
>> + (1 << LCD_MAX_LOG2_BPP) / 8;
>> +
>> + debug("%s: Frame buffer size %x\n", __func__, uc_plat->size);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct udevice_id atmel_hlcdc_ids[] = {
>> + { .compatible = "atmel,sama5d2-hlcdc" },
>> + { }
>> +};
>> +
>> +U_BOOT_DRIVER(atmel_hlcdfb) = {
>> + .name = "atmel_hlcdfb",
>> + .id = UCLASS_VIDEO,
>> + .of_match = atmel_hlcdc_ids,
>> + .bind = atmel_hlcdc_bind,
>> + .probe = atmel_hlcdc_probe,
>> + .ofdata_to_platdata = atmel_hlcdc_ofdata_to_platdata,
>> + .priv_auto_alloc_size = sizeof(struct atmel_hlcdc_priv),
>> +};
>> +
>> +#endif
>
> [...]
>
>> diff --git a/include/configs/sama5d4ek.h b/include/configs/sama5d4ek.h
>> index 680d5918d7..4d537b22aa 100644
>> --- a/include/configs/sama5d4ek.h
>> +++ b/include/configs/sama5d4ek.h
>> @@ -91,7 +91,11 @@
>> #define CONFIG_LCD_INFO
>> #define CONFIG_LCD_INFO_BELOW_LOGO
>> #define CONFIG_SYS_WHITE_ON_BLACK
>> +
>> +#ifndef CONFIG_DM_VIDEO
>> #define CONFIG_ATMEL_HLCD
>
> This should use Kconfig ...
>
>> +#endif
>> +
>> #define CONFIG_ATMEL_LCD_RGB565
>>
>> #ifdef CONFIG_SYS_USE_SERIALFLASH
>>
>
>
More information about the U-Boot
mailing list