| Message ID | 1417788824-5082-3-git-send-email-ajaykumar.rs@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
Dear Ajay Kumar, On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs@samsung.com> wrote: > Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by > exynos video driver. > > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> > --- > arch/arm/cpu/armv7/exynos/clock.c | 63 > +++++++++++++++++++++++++++++++- > arch/arm/include/asm/arch-exynos/clk.h | 3 ++ > 2 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/cpu/armv7/exynos/clock.c > b/arch/arm/cpu/armv7/exynos/clock.c > index 8fab135..1a34ad6 100644 > --- a/arch/arm/cpu/armv7/exynos/clock.c > +++ b/arch/arm/cpu/armv7/exynos/clock.c > @@ -1066,6 +1066,36 @@ static unsigned long exynos5420_get_lcd_clk(void) > return pclk; > } > > +static unsigned long exynos5800_get_lcd_clk(void) > +{ > + struct exynos5420_clock *clk = > + (struct exynos5420_clock *)samsung_get_base_clock(); > + unsigned long sclk; > + unsigned sel; > just unsigned? why don't you specify in detail? > + unsigned ratio; > + > + sel = (readl(&clk->src_disp10) >> 4) & 7; > please add comment how you get "sel" from disp10. and if 7 means mask then please use 0x7. it looks more clearly. + > + /* > + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into > + * PLLs. The first element is a placeholder to bypass the > + * default settig. > + */ > + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, > + IPLL, EPLL, RPLL}; > please don't define local variable at middle of function. you can move it to top of the function or it seems to use sel is true then you can move it into the if statement. > + if (sel) > + sclk = get_pll_clk(reg_map[sel]); > + else > + sclk = 24000000; > please define this value. > + /* > + * CLK_DIV_DISP10 > + * FIMD1_RATIO [3:0] > + */ > + ratio = readl(&clk->div_disp10) & 0xf; > + > + return sclk / (ratio + 1); > +} > + > void exynos4_set_lcd_clk(void) > { > struct exynos4_clock *clk = > @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) > writel(cfg, &clk->div_disp10); > } > > +void exynos5800_set_lcd_clk(void) > +{ > + struct exynos5420_clock *clk = > + (struct exynos5420_clock *)samsung_get_base_clock(); > + unsigned int cfg; > + > + /* > + * Use RPLL for pixel clock > + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] > + * ================== > + * 111: SCLK_RPLL > + */ > + cfg = readl(&clk->src_disp10) | (7 << 4); > + writel(cfg, &clk->src_disp10); > + > + /* > + * CLK_DIV_DISP10 > + * FIMD1_RATIO [3:0] > + */ > + cfg = readl(&clk->div_disp10); > + cfg &= ~(0xf << 0); > + cfg |= (0 << 0); > it looks meaningless. > + writel(cfg, &clk->div_disp10); > +} > + > void exynos4_set_mipi_clk(void) > { > struct exynos4_clock *clk = > @@ -1669,8 +1724,10 @@ unsigned long get_lcd_clk(void) > if (cpu_is_exynos4()) > return exynos4_get_lcd_clk(); > else { > - if (proid_is_exynos5420() || proid_is_exynos5800()) > + if (proid_is_exynos5420()) > return exynos5420_get_lcd_clk(); > + else if (proid_is_exynos5800()) > + return exynos5800_get_lcd_clk(); > else > return exynos5_get_lcd_clk(); > } > @@ -1683,8 +1740,10 @@ void set_lcd_clk(void) > else { > if (proid_is_exynos5250()) > exynos5_set_lcd_clk(); > - else if (proid_is_exynos5420() || proid_is_exynos5800()) > + else if (proid_is_exynos5420()) > exynos5420_set_lcd_clk(); > + else > + exynos5800_set_lcd_clk(); > } > } > > diff --git a/arch/arm/include/asm/arch-exynos/clk.h > b/arch/arm/include/asm/arch-exynos/clk.h > index db24dc0..bf3d348 100644 > --- a/arch/arm/include/asm/arch-exynos/clk.h > +++ b/arch/arm/include/asm/arch-exynos/clk.h > @@ -16,6 +16,9 @@ > #define BPLL 5 > #define RPLL 6 > #define SPLL 7 > +#define CPLL 8 > +#define DPLL 9 > +#define IPLL 10 > > #define MASK_PRE_RATIO(x) (0xff << ((x << 4) + 8)) > #define MASK_RATIO(x) (0xf << (x << 4)) > -- > 1.7.9.5 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > Thanks, Minkyu Kang.
On Fri, Dec 5, 2014 at 9:02 PM, Minkyu Kang <promsoft@gmail.com> wrote: > Dear Ajay Kumar, > > On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs@samsung.com> wrote: > >> Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by >> exynos video driver. >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> --- >> arch/arm/cpu/armv7/exynos/clock.c | 63 >> +++++++++++++++++++++++++++++++- >> arch/arm/include/asm/arch-exynos/clk.h | 3 ++ >> 2 files changed, 64 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/exynos/clock.c >> b/arch/arm/cpu/armv7/exynos/clock.c >> index 8fab135..1a34ad6 100644 >> --- a/arch/arm/cpu/armv7/exynos/clock.c >> +++ b/arch/arm/cpu/armv7/exynos/clock.c >> @@ -1066,6 +1066,36 @@ static unsigned long exynos5420_get_lcd_clk(void) >> return pclk; >> } >> >> +static unsigned long exynos5800_get_lcd_clk(void) >> +{ >> + struct exynos5420_clock *clk = >> + (struct exynos5420_clock *)samsung_get_base_clock(); >> + unsigned long sclk; >> + unsigned sel; >> > > just unsigned? why don't you specify in detail? > > >> + unsigned ratio; >> + >> + sel = (readl(&clk->src_disp10) >> 4) & 7; >> > > please add comment how you get "sel" from disp10. > and if 7 means mask then please use 0x7. it looks more clearly. > > + >> + /* >> + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into >> + * PLLs. The first element is a placeholder to bypass the >> + * default settig. >> + */ >> + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, >> + IPLL, EPLL, RPLL}; >> > > please don't define local variable at middle of function. > you can move it to top of the function or > it seems to use sel is true then you can move it into the if statement. > > >> + if (sel) >> + sclk = get_pll_clk(reg_map[sel]); >> + else >> + sclk = 24000000; >> > > please define this value. > > >> + /* >> + * CLK_DIV_DISP10 >> + * FIMD1_RATIO [3:0] >> + */ >> + ratio = readl(&clk->div_disp10) & 0xf; >> + >> + return sclk / (ratio + 1); >> +} >> + >> void exynos4_set_lcd_clk(void) >> { >> struct exynos4_clock *clk = >> @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) >> writel(cfg, &clk->div_disp10); >> } >> >> +void exynos5800_set_lcd_clk(void) >> +{ >> + struct exynos5420_clock *clk = >> + (struct exynos5420_clock *)samsung_get_base_clock(); >> + unsigned int cfg; >> + >> + /* >> + * Use RPLL for pixel clock >> + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] >> + * ================== >> + * 111: SCLK_RPLL >> + */ >> + cfg = readl(&clk->src_disp10) | (7 << 4); >> + writel(cfg, &clk->src_disp10); >> + >> + /* >> + * CLK_DIV_DISP10 >> + * FIMD1_RATIO [3:0] >> + */ >> + cfg = readl(&clk->div_disp10); >> + cfg &= ~(0xf << 0); >> + cfg |= (0 << 0); >> > > it looks meaningless. > > >> + writel(cfg, &clk->div_disp10); >> +} >> + >> void exynos4_set_mipi_clk(void) >> { >> struct exynos4_clock *clk = >> @@ -1669,8 +1724,10 @@ unsigned long get_lcd_clk(void) >> if (cpu_is_exynos4()) >> return exynos4_get_lcd_clk(); >> else { >> - if (proid_is_exynos5420() || proid_is_exynos5800()) >> + if (proid_is_exynos5420()) >> return exynos5420_get_lcd_clk(); >> + else if (proid_is_exynos5800()) >> + return exynos5800_get_lcd_clk(); >> else >> return exynos5_get_lcd_clk(); >> } >> @@ -1683,8 +1740,10 @@ void set_lcd_clk(void) >> else { >> if (proid_is_exynos5250()) >> exynos5_set_lcd_clk(); >> - else if (proid_is_exynos5420() || proid_is_exynos5800()) >> + else if (proid_is_exynos5420()) >> exynos5420_set_lcd_clk(); >> + else >> + exynos5800_set_lcd_clk(); >> } >> } >> >> diff --git a/arch/arm/include/asm/arch-exynos/clk.h >> b/arch/arm/include/asm/arch-exynos/clk.h >> index db24dc0..bf3d348 100644 >> --- a/arch/arm/include/asm/arch-exynos/clk.h >> +++ b/arch/arm/include/asm/arch-exynos/clk.h >> @@ -16,6 +16,9 @@ >> #define BPLL 5 >> #define RPLL 6 >> #define SPLL 7 >> +#define CPLL 8 >> +#define DPLL 9 >> +#define IPLL 10 >> >> #define MASK_PRE_RATIO(x) (0xff << ((x << 4) + 8)) >> #define MASK_RATIO(x) (0xf << (x << 4)) >> -- >> 1.7.9.5 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot >> > > > Thanks, > Minkyu Kang. > -- > from. prom. > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
Hi Minkyu, On Mon, Dec 8, 2014 at 11:07 AM, Ajay kumar <ajaynumb@gmail.com> wrote: > On Fri, Dec 5, 2014 at 9:02 PM, Minkyu Kang <promsoft@gmail.com> wrote: >> Dear Ajay Kumar, >> >> On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs@samsung.com> wrote: >> >>> Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by >>> exynos video driver. >>> >>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >>> --- >>> arch/arm/cpu/armv7/exynos/clock.c | 63 >>> +++++++++++++++++++++++++++++++- >>> arch/arm/include/asm/arch-exynos/clk.h | 3 ++ >>> 2 files changed, 64 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/cpu/armv7/exynos/clock.c >>> b/arch/arm/cpu/armv7/exynos/clock.c >>> index 8fab135..1a34ad6 100644 >>> --- a/arch/arm/cpu/armv7/exynos/clock.c >>> +++ b/arch/arm/cpu/armv7/exynos/clock.c >>> @@ -1066,6 +1066,36 @@ static unsigned long exynos5420_get_lcd_clk(void) >>> return pclk; >>> } >>> >>> +static unsigned long exynos5800_get_lcd_clk(void) >>> +{ >>> + struct exynos5420_clock *clk = >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >>> + unsigned long sclk; >>> + unsigned sel; >>> >> >> just unsigned? why don't you specify in detail? I will fix this. >> >>> + unsigned ratio; >>> + >>> + sel = (readl(&clk->src_disp10) >> 4) & 7; >>> >> >> please add comment how you get "sel" from disp10. >> and if 7 means mask then please use 0x7. it looks more clearly. Ok. >> + >>> + /* >>> + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into >>> + * PLLs. The first element is a placeholder to bypass the >>> + * default settig. >>> + */ >>> + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, >>> + IPLL, EPLL, RPLL}; >>> >> >> please don't define local variable at middle of function. >> you can move it to top of the function or >> it seems to use sel is true then you can move it into the if statement. I will move this to the top of the function. >> >>> + if (sel) >>> + sclk = get_pll_clk(reg_map[sel]); >>> + else >>> + sclk = 24000000; >>> >> >> please define this value. Ok. >> >>> + /* >>> + * CLK_DIV_DISP10 >>> + * FIMD1_RATIO [3:0] >>> + */ >>> + ratio = readl(&clk->div_disp10) & 0xf; >>> + >>> + return sclk / (ratio + 1); >>> +} >>> + >>> void exynos4_set_lcd_clk(void) >>> { >>> struct exynos4_clock *clk = >>> @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) >>> writel(cfg, &clk->div_disp10); >>> } >>> >>> +void exynos5800_set_lcd_clk(void) >>> +{ >>> + struct exynos5420_clock *clk = >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >>> + unsigned int cfg; >>> + >>> + /* >>> + * Use RPLL for pixel clock >>> + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] >>> + * ================== >>> + * 111: SCLK_RPLL >>> + */ >>> + cfg = readl(&clk->src_disp10) | (7 << 4); >>> + writel(cfg, &clk->src_disp10); >>> + >>> + /* >>> + * CLK_DIV_DISP10 >>> + * FIMD1_RATIO [3:0] >>> + */ >>> + cfg = readl(&clk->div_disp10); >>> + cfg &= ~(0xf << 0); >>> + cfg |= (0 << 0); >>> >> >> it looks meaningless. Why not? I agree that the calculation can be skipped, and directly FIMD clock divider can be set to 0. But, I prefer to keep the readability. In fact, similar "meaningless" code is already part of the tree: http://git.denx.de/?p=u-boot/u-boot-samsung.git;a=blob;f=arch/arm/cpu/armv7/exynos/clock.c;h=8fab135bebf4ef6900677847b60a8e1a1520254c;hb=HEAD#l1194 Ajay >> >>> + writel(cfg, &clk->div_disp10); >>> +} >>> + >>> void exynos4_set_mipi_clk(void) >>> { >>> struct exynos4_clock *clk = >>> @@ -1669,8 +1724,10 @@ unsigned long get_lcd_clk(void) >>> if (cpu_is_exynos4()) >>> return exynos4_get_lcd_clk(); >>> else { >>> - if (proid_is_exynos5420() || proid_is_exynos5800()) >>> + if (proid_is_exynos5420()) >>> return exynos5420_get_lcd_clk(); >>> + else if (proid_is_exynos5800()) >>> + return exynos5800_get_lcd_clk(); >>> else >>> return exynos5_get_lcd_clk(); >>> } >>> @@ -1683,8 +1740,10 @@ void set_lcd_clk(void) >>> else { >>> if (proid_is_exynos5250()) >>> exynos5_set_lcd_clk(); >>> - else if (proid_is_exynos5420() || proid_is_exynos5800()) >>> + else if (proid_is_exynos5420()) >>> exynos5420_set_lcd_clk(); >>> + else >>> + exynos5800_set_lcd_clk(); >>> } >>> } >>> >>> diff --git a/arch/arm/include/asm/arch-exynos/clk.h >>> b/arch/arm/include/asm/arch-exynos/clk.h >>> index db24dc0..bf3d348 100644 >>> --- a/arch/arm/include/asm/arch-exynos/clk.h >>> +++ b/arch/arm/include/asm/arch-exynos/clk.h >>> @@ -16,6 +16,9 @@ >>> #define BPLL 5 >>> #define RPLL 6 >>> #define SPLL 7 >>> +#define CPLL 8 >>> +#define DPLL 9 >>> +#define IPLL 10 >>> >>> #define MASK_PRE_RATIO(x) (0xff << ((x << 4) + 8)) >>> #define MASK_RATIO(x) (0xf << (x << 4)) >>> -- >>> 1.7.9.5 >>> >>> _______________________________________________ >>> U-Boot mailing list >>> U-Boot@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot >>> >> >> >> Thanks, >> Minkyu Kang. >> -- >> from. prom. >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot >>
Hi Ajay, On 7 December 2014 at 22:43, Ajay kumar <ajaynumb@gmail.com> wrote: > Hi Minkyu, > > > On Mon, Dec 8, 2014 at 11:07 AM, Ajay kumar <ajaynumb@gmail.com> wrote: > > On Fri, Dec 5, 2014 at 9:02 PM, Minkyu Kang <promsoft@gmail.com> wrote: > >> Dear Ajay Kumar, > >> > >> On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs@samsung.com> > wrote: > >> > >>> Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by > >>> exynos video driver. > >>> > >>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> > >>> --- > >>> arch/arm/cpu/armv7/exynos/clock.c | 63 > >>> +++++++++++++++++++++++++++++++- > >>> arch/arm/include/asm/arch-exynos/clk.h | 3 ++ > >>> 2 files changed, 64 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/arch/arm/cpu/armv7/exynos/clock.c > >>> b/arch/arm/cpu/armv7/exynos/clock.c > >>> index 8fab135..1a34ad6 100644 > >>> --- a/arch/arm/cpu/armv7/exynos/clock.c > >>> +++ b/arch/arm/cpu/armv7/exynos/clock.c > >>> @@ -1066,6 +1066,36 @@ static unsigned long > exynos5420_get_lcd_clk(void) > >>> return pclk; > >>> } > >>> > >>> +static unsigned long exynos5800_get_lcd_clk(void) > >>> +{ > >>> + struct exynos5420_clock *clk = > >>> + (struct exynos5420_clock *)samsung_get_base_clock(); > >>> + unsigned long sclk; > >>> + unsigned sel; > >>> > >> > >> just unsigned? why don't you specify in detail? > I will fix this. > > >> > >>> + unsigned ratio; > >>> + > >>> + sel = (readl(&clk->src_disp10) >> 4) & 7; > >>> > >> > >> please add comment how you get "sel" from disp10. > >> and if 7 means mask then please use 0x7. it looks more clearly. > Ok. > > >> + > >>> + /* > >>> + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into > >>> + * PLLs. The first element is a placeholder to bypass the > >>> + * default settig. > >>> + */ > >>> + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, > >>> + IPLL, EPLL, RPLL}; > >>> > >> > >> please don't define local variable at middle of function. > >> you can move it to top of the function or > >> it seems to use sel is true then you can move it into the if statement. > I will move this to the top of the function. > > >> > >>> + if (sel) > >>> + sclk = get_pll_clk(reg_map[sel]); > >>> + else > >>> + sclk = 24000000; > >>> > >> > >> please define this value. > Ok. > > >> > >>> + /* > >>> + * CLK_DIV_DISP10 > >>> + * FIMD1_RATIO [3:0] > >>> + */ > >>> + ratio = readl(&clk->div_disp10) & 0xf; > >>> + > >>> + return sclk / (ratio + 1); > >>> +} > >>> + > >>> void exynos4_set_lcd_clk(void) > >>> { > >>> struct exynos4_clock *clk = > >>> @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) > >>> writel(cfg, &clk->div_disp10); > >>> } > >>> > >>> +void exynos5800_set_lcd_clk(void) > >>> +{ > >>> + struct exynos5420_clock *clk = > >>> + (struct exynos5420_clock *)samsung_get_base_clock(); > >>> + unsigned int cfg; > >>> + > >>> + /* > >>> + * Use RPLL for pixel clock > >>> + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] > >>> + * ================== > >>> + * 111: SCLK_RPLL > >>> + */ > >>> + cfg = readl(&clk->src_disp10) | (7 << 4); > >>> + writel(cfg, &clk->src_disp10); > >>> + > >>> + /* > >>> + * CLK_DIV_DISP10 > >>> + * FIMD1_RATIO [3:0] > >>> + */ > >>> + cfg = readl(&clk->div_disp10); > >>> + cfg &= ~(0xf << 0); > >>> + cfg |= (0 << 0); > >>> > >> > >> it looks meaningless. > Why not? > I agree that the calculation can be skipped, and directly FIMD > clock divider can be set to 0. But, I prefer to keep the readability. > In fact, similar "meaningless" code is already part of the tree: > > http://git.denx.de/?p=u-boot/u-boot-samsung.git;a=blob;f=arch/arm/cpu/armv7/exynos/clock.c;h=8fab135bebf4ef6900677847b60a8e1a1520254c;hb=HEAD#l1194 Well perhaps a #define for the shift value? Then it would mean something. Also this can probably use clrsetbits_le32(). Regards, Simon
Hi Ajay, On 8 December 2014 at 15:40, Simon Glass <sjg@google.com> wrote: > Hi Ajay, > > > On 7 December 2014 at 22:43, Ajay kumar <ajaynumb@gmail.com> wrote: >> >> Hi Minkyu, >> >> >> On Mon, Dec 8, 2014 at 11:07 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >> > On Fri, Dec 5, 2014 at 9:02 PM, Minkyu Kang <promsoft@gmail.com> wrote: >> >> Dear Ajay Kumar, >> >> >> >> On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs@samsung.com> >> >> wrote: >> >> >> >>> Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by >> >>> exynos video driver. >> >>> >> >>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> >>> --- >> >>> arch/arm/cpu/armv7/exynos/clock.c | 63 >> >>> +++++++++++++++++++++++++++++++- >> >>> arch/arm/include/asm/arch-exynos/clk.h | 3 ++ >> >>> 2 files changed, 64 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/arch/arm/cpu/armv7/exynos/clock.c >> >>> b/arch/arm/cpu/armv7/exynos/clock.c >> >>> index 8fab135..1a34ad6 100644 >> >>> --- a/arch/arm/cpu/armv7/exynos/clock.c >> >>> +++ b/arch/arm/cpu/armv7/exynos/clock.c >> >>> @@ -1066,6 +1066,36 @@ static unsigned long >> >>> exynos5420_get_lcd_clk(void) >> >>> return pclk; >> >>> } >> >>> >> >>> +static unsigned long exynos5800_get_lcd_clk(void) >> >>> +{ >> >>> + struct exynos5420_clock *clk = >> >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >> >>> + unsigned long sclk; >> >>> + unsigned sel; >> >>> >> >> >> >> just unsigned? why don't you specify in detail? >> I will fix this. >> >> >> >> >>> + unsigned ratio; >> >>> + >> >>> + sel = (readl(&clk->src_disp10) >> 4) & 7; >> >>> >> >> >> >> please add comment how you get "sel" from disp10. >> >> and if 7 means mask then please use 0x7. it looks more clearly. >> Ok. >> >> >> + >> >>> + /* >> >>> + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into >> >>> + * PLLs. The first element is a placeholder to bypass the >> >>> + * default settig. >> >>> + */ >> >>> + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, >> >>> + IPLL, EPLL, RPLL}; >> >>> >> >> >> >> please don't define local variable at middle of function. >> >> you can move it to top of the function or >> >> it seems to use sel is true then you can move it into the if statement. >> I will move this to the top of the function. >> >> >> >> >>> + if (sel) >> >>> + sclk = get_pll_clk(reg_map[sel]); >> >>> + else >> >>> + sclk = 24000000; >> >>> >> >> >> >> please define this value. >> Ok. >> >> >> >> >>> + /* >> >>> + * CLK_DIV_DISP10 >> >>> + * FIMD1_RATIO [3:0] >> >>> + */ >> >>> + ratio = readl(&clk->div_disp10) & 0xf; >> >>> + >> >>> + return sclk / (ratio + 1); >> >>> +} >> >>> + >> >>> void exynos4_set_lcd_clk(void) >> >>> { >> >>> struct exynos4_clock *clk = >> >>> @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) >> >>> writel(cfg, &clk->div_disp10); >> >>> } >> >>> >> >>> +void exynos5800_set_lcd_clk(void) >> >>> +{ >> >>> + struct exynos5420_clock *clk = >> >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >> >>> + unsigned int cfg; >> >>> + >> >>> + /* >> >>> + * Use RPLL for pixel clock >> >>> + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] >> >>> + * ================== >> >>> + * 111: SCLK_RPLL >> >>> + */ >> >>> + cfg = readl(&clk->src_disp10) | (7 << 4); >> >>> + writel(cfg, &clk->src_disp10); >> >>> + >> >>> + /* >> >>> + * CLK_DIV_DISP10 >> >>> + * FIMD1_RATIO [3:0] >> >>> + */ >> >>> + cfg = readl(&clk->div_disp10); >> >>> + cfg &= ~(0xf << 0); >> >>> + cfg |= (0 << 0); >> >>> >> >> >> >> it looks meaningless. >> Why not? >> I agree that the calculation can be skipped, and directly FIMD >> clock divider can be set to 0. But, I prefer to keep the readability. >> In fact, similar "meaningless" code is already part of the tree: >> >> http://git.denx.de/?p=u-boot/u-boot-samsung.git;a=blob;f=arch/arm/cpu/armv7/exynos/clock.c;h=8fab135bebf4ef6900677847b60a8e1a1520254c;hb=HEAD#l1194 > > > Well perhaps a #define for the shift value? Then it would mean something. > > Also this can probably use clrsetbits_le32(). This is one of four patches that I think were never applied and need a respin. Are you able to do that? Then Pi LCD will work OK I think. http://patchwork.ozlabs.org/patch/418116/ http://patchwork.ozlabs.org/patch/418117/ http://patchwork.ozlabs.org/patch/418118/ http://patchwork.ozlabs.org/patch/418119/ Regards. Simon
Hi Simon, On Mon, Mar 2, 2015 at 7:53 AM, Simon Glass <sjg@google.com> wrote: > Hi Ajay, > > On 8 December 2014 at 15:40, Simon Glass <sjg@google.com> wrote: >> Hi Ajay, >> >> >> On 7 December 2014 at 22:43, Ajay kumar <ajaynumb@gmail.com> wrote: >>> >>> Hi Minkyu, >>> >>> >>> On Mon, Dec 8, 2014 at 11:07 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >>> > On Fri, Dec 5, 2014 at 9:02 PM, Minkyu Kang <promsoft@gmail.com> wrote: >>> >> Dear Ajay Kumar, >>> >> >>> >> On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs@samsung.com> >>> >> wrote: >>> >> >>> >>> Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by >>> >>> exynos video driver. >>> >>> >>> >>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >>> >>> --- >>> >>> arch/arm/cpu/armv7/exynos/clock.c | 63 >>> >>> +++++++++++++++++++++++++++++++- >>> >>> arch/arm/include/asm/arch-exynos/clk.h | 3 ++ >>> >>> 2 files changed, 64 insertions(+), 2 deletions(-) >>> >>> >>> >>> diff --git a/arch/arm/cpu/armv7/exynos/clock.c >>> >>> b/arch/arm/cpu/armv7/exynos/clock.c >>> >>> index 8fab135..1a34ad6 100644 >>> >>> --- a/arch/arm/cpu/armv7/exynos/clock.c >>> >>> +++ b/arch/arm/cpu/armv7/exynos/clock.c >>> >>> @@ -1066,6 +1066,36 @@ static unsigned long >>> >>> exynos5420_get_lcd_clk(void) >>> >>> return pclk; >>> >>> } >>> >>> >>> >>> +static unsigned long exynos5800_get_lcd_clk(void) >>> >>> +{ >>> >>> + struct exynos5420_clock *clk = >>> >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >>> >>> + unsigned long sclk; >>> >>> + unsigned sel; >>> >>> >>> >> >>> >> just unsigned? why don't you specify in detail? >>> I will fix this. >>> >>> >> >>> >>> + unsigned ratio; >>> >>> + >>> >>> + sel = (readl(&clk->src_disp10) >> 4) & 7; >>> >>> >>> >> >>> >> please add comment how you get "sel" from disp10. >>> >> and if 7 means mask then please use 0x7. it looks more clearly. >>> Ok. >>> >>> >> + >>> >>> + /* >>> >>> + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into >>> >>> + * PLLs. The first element is a placeholder to bypass the >>> >>> + * default settig. >>> >>> + */ >>> >>> + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, >>> >>> + IPLL, EPLL, RPLL}; >>> >>> >>> >> >>> >> please don't define local variable at middle of function. >>> >> you can move it to top of the function or >>> >> it seems to use sel is true then you can move it into the if statement. >>> I will move this to the top of the function. >>> >>> >> >>> >>> + if (sel) >>> >>> + sclk = get_pll_clk(reg_map[sel]); >>> >>> + else >>> >>> + sclk = 24000000; >>> >>> >>> >> >>> >> please define this value. >>> Ok. >>> >>> >> >>> >>> + /* >>> >>> + * CLK_DIV_DISP10 >>> >>> + * FIMD1_RATIO [3:0] >>> >>> + */ >>> >>> + ratio = readl(&clk->div_disp10) & 0xf; >>> >>> + >>> >>> + return sclk / (ratio + 1); >>> >>> +} >>> >>> + >>> >>> void exynos4_set_lcd_clk(void) >>> >>> { >>> >>> struct exynos4_clock *clk = >>> >>> @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) >>> >>> writel(cfg, &clk->div_disp10); >>> >>> } >>> >>> >>> >>> +void exynos5800_set_lcd_clk(void) >>> >>> +{ >>> >>> + struct exynos5420_clock *clk = >>> >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >>> >>> + unsigned int cfg; >>> >>> + >>> >>> + /* >>> >>> + * Use RPLL for pixel clock >>> >>> + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] >>> >>> + * ================== >>> >>> + * 111: SCLK_RPLL >>> >>> + */ >>> >>> + cfg = readl(&clk->src_disp10) | (7 << 4); >>> >>> + writel(cfg, &clk->src_disp10); >>> >>> + >>> >>> + /* >>> >>> + * CLK_DIV_DISP10 >>> >>> + * FIMD1_RATIO [3:0] >>> >>> + */ >>> >>> + cfg = readl(&clk->div_disp10); >>> >>> + cfg &= ~(0xf << 0); >>> >>> + cfg |= (0 << 0); >>> >>> >>> >> >>> >> it looks meaningless. >>> Why not? >>> I agree that the calculation can be skipped, and directly FIMD >>> clock divider can be set to 0. But, I prefer to keep the readability. >>> In fact, similar "meaningless" code is already part of the tree: >>> >>> http://git.denx.de/?p=u-boot/u-boot-samsung.git;a=blob;f=arch/arm/cpu/armv7/exynos/clock.c;h=8fab135bebf4ef6900677847b60a8e1a1520254c;hb=HEAD#l1194 >> >> >> Well perhaps a #define for the shift value? Then it would mean something. >> >> Also this can probably use clrsetbits_le32(). > > This is one of four patches that I think were never applied and need a > respin. Are you able to do that? Then Pi LCD will work OK I think. Yes, I am respinning this patch. Also, trying to move out GPIOs from smdk5420.c. Will be posting them tomorrow. Ajay > http://patchwork.ozlabs.org/patch/418116/ > http://patchwork.ozlabs.org/patch/418117/ > http://patchwork.ozlabs.org/patch/418118/ > http://patchwork.ozlabs.org/patch/418119/ > > Regards. > Simon
Hi Ajay, On 2 March 2015 at 09:07, Ajay kumar <ajaynumb@gmail.com> wrote: > Hi Simon, > > On Mon, Mar 2, 2015 at 7:53 AM, Simon Glass <sjg@google.com> wrote: >> Hi Ajay, >> >> On 8 December 2014 at 15:40, Simon Glass <sjg@google.com> wrote: >>> Hi Ajay, >>> >>> >>> On 7 December 2014 at 22:43, Ajay kumar <ajaynumb@gmail.com> wrote: >>>> >>>> Hi Minkyu, >>>> >>>> >>>> On Mon, Dec 8, 2014 at 11:07 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >>>> > On Fri, Dec 5, 2014 at 9:02 PM, Minkyu Kang <promsoft@gmail.com> wrote: >>>> >> Dear Ajay Kumar, >>>> >> >>>> >> On 5 December 2014 at 23:13, Ajay Kumar <ajaykumar.rs@samsung.com> >>>> >> wrote: >>>> >> >>>> >>> Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by >>>> >>> exynos video driver. >>>> >>> >>>> >>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >>>> >>> --- >>>> >>> arch/arm/cpu/armv7/exynos/clock.c | 63 >>>> >>> +++++++++++++++++++++++++++++++- >>>> >>> arch/arm/include/asm/arch-exynos/clk.h | 3 ++ >>>> >>> 2 files changed, 64 insertions(+), 2 deletions(-) >>>> >>> >>>> >>> diff --git a/arch/arm/cpu/armv7/exynos/clock.c >>>> >>> b/arch/arm/cpu/armv7/exynos/clock.c >>>> >>> index 8fab135..1a34ad6 100644 >>>> >>> --- a/arch/arm/cpu/armv7/exynos/clock.c >>>> >>> +++ b/arch/arm/cpu/armv7/exynos/clock.c >>>> >>> @@ -1066,6 +1066,36 @@ static unsigned long >>>> >>> exynos5420_get_lcd_clk(void) >>>> >>> return pclk; >>>> >>> } >>>> >>> >>>> >>> +static unsigned long exynos5800_get_lcd_clk(void) >>>> >>> +{ >>>> >>> + struct exynos5420_clock *clk = >>>> >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >>>> >>> + unsigned long sclk; >>>> >>> + unsigned sel; >>>> >>> >>>> >> >>>> >> just unsigned? why don't you specify in detail? >>>> I will fix this. >>>> >>>> >> >>>> >>> + unsigned ratio; >>>> >>> + >>>> >>> + sel = (readl(&clk->src_disp10) >> 4) & 7; >>>> >>> >>>> >> >>>> >> please add comment how you get "sel" from disp10. >>>> >> and if 7 means mask then please use 0x7. it looks more clearly. >>>> Ok. >>>> >>>> >> + >>>> >>> + /* >>>> >>> + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into >>>> >>> + * PLLs. The first element is a placeholder to bypass the >>>> >>> + * default settig. >>>> >>> + */ >>>> >>> + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, >>>> >>> + IPLL, EPLL, RPLL}; >>>> >>> >>>> >> >>>> >> please don't define local variable at middle of function. >>>> >> you can move it to top of the function or >>>> >> it seems to use sel is true then you can move it into the if statement. >>>> I will move this to the top of the function. >>>> >>>> >> >>>> >>> + if (sel) >>>> >>> + sclk = get_pll_clk(reg_map[sel]); >>>> >>> + else >>>> >>> + sclk = 24000000; >>>> >>> >>>> >> >>>> >> please define this value. >>>> Ok. >>>> >>>> >> >>>> >>> + /* >>>> >>> + * CLK_DIV_DISP10 >>>> >>> + * FIMD1_RATIO [3:0] >>>> >>> + */ >>>> >>> + ratio = readl(&clk->div_disp10) & 0xf; >>>> >>> + >>>> >>> + return sclk / (ratio + 1); >>>> >>> +} >>>> >>> + >>>> >>> void exynos4_set_lcd_clk(void) >>>> >>> { >>>> >>> struct exynos4_clock *clk = >>>> >>> @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) >>>> >>> writel(cfg, &clk->div_disp10); >>>> >>> } >>>> >>> >>>> >>> +void exynos5800_set_lcd_clk(void) >>>> >>> +{ >>>> >>> + struct exynos5420_clock *clk = >>>> >>> + (struct exynos5420_clock *)samsung_get_base_clock(); >>>> >>> + unsigned int cfg; >>>> >>> + >>>> >>> + /* >>>> >>> + * Use RPLL for pixel clock >>>> >>> + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] >>>> >>> + * ================== >>>> >>> + * 111: SCLK_RPLL >>>> >>> + */ >>>> >>> + cfg = readl(&clk->src_disp10) | (7 << 4); >>>> >>> + writel(cfg, &clk->src_disp10); >>>> >>> + >>>> >>> + /* >>>> >>> + * CLK_DIV_DISP10 >>>> >>> + * FIMD1_RATIO [3:0] >>>> >>> + */ >>>> >>> + cfg = readl(&clk->div_disp10); >>>> >>> + cfg &= ~(0xf << 0); >>>> >>> + cfg |= (0 << 0); >>>> >>> >>>> >> >>>> >> it looks meaningless. >>>> Why not? >>>> I agree that the calculation can be skipped, and directly FIMD >>>> clock divider can be set to 0. But, I prefer to keep the readability. >>>> In fact, similar "meaningless" code is already part of the tree: >>>> >>>> http://git.denx.de/?p=u-boot/u-boot-samsung.git;a=blob;f=arch/arm/cpu/armv7/exynos/clock.c;h=8fab135bebf4ef6900677847b60a8e1a1520254c;hb=HEAD#l1194 >>> >>> >>> Well perhaps a #define for the shift value? Then it would mean something. >>> >>> Also this can probably use clrsetbits_le32(). >> >> This is one of four patches that I think were never applied and need a >> respin. Are you able to do that? Then Pi LCD will work OK I think. > Yes, I am respinning this patch. Also, trying to move out GPIOs from > smdk5420.c. Will be posting them tomorrow. OK great. You might find gpio_request_by_name() useful, and gpio_request_by_name_nodev() where the thing requesting the GPIOs does not use driver model yet. Regards, Simon
diff --git a/arch/arm/cpu/armv7/exynos/clock.c b/arch/arm/cpu/armv7/exynos/clock.c index 8fab135..1a34ad6 100644 --- a/arch/arm/cpu/armv7/exynos/clock.c +++ b/arch/arm/cpu/armv7/exynos/clock.c @@ -1066,6 +1066,36 @@ static unsigned long exynos5420_get_lcd_clk(void) return pclk; } +static unsigned long exynos5800_get_lcd_clk(void) +{ + struct exynos5420_clock *clk = + (struct exynos5420_clock *)samsung_get_base_clock(); + unsigned long sclk; + unsigned sel; + unsigned ratio; + + sel = (readl(&clk->src_disp10) >> 4) & 7; + + /* + * Mapping of CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] values into + * PLLs. The first element is a placeholder to bypass the + * default settig. + */ + const int reg_map[] = {0, CPLL, DPLL, MPLL, SPLL, + IPLL, EPLL, RPLL}; + if (sel) + sclk = get_pll_clk(reg_map[sel]); + else + sclk = 24000000; + /* + * CLK_DIV_DISP10 + * FIMD1_RATIO [3:0] + */ + ratio = readl(&clk->div_disp10) & 0xf; + + return sclk / (ratio + 1); +} + void exynos4_set_lcd_clk(void) { struct exynos4_clock *clk = @@ -1197,6 +1227,31 @@ void exynos5420_set_lcd_clk(void) writel(cfg, &clk->div_disp10); } +void exynos5800_set_lcd_clk(void) +{ + struct exynos5420_clock *clk = + (struct exynos5420_clock *)samsung_get_base_clock(); + unsigned int cfg; + + /* + * Use RPLL for pixel clock + * CLK_SRC_DISP10 CLKMUX_FIMD1 [6:4] + * ================== + * 111: SCLK_RPLL + */ + cfg = readl(&clk->src_disp10) | (7 << 4); + writel(cfg, &clk->src_disp10); + + /* + * CLK_DIV_DISP10 + * FIMD1_RATIO [3:0] + */ + cfg = readl(&clk->div_disp10); + cfg &= ~(0xf << 0); + cfg |= (0 << 0); + writel(cfg, &clk->div_disp10); +} + void exynos4_set_mipi_clk(void) { struct exynos4_clock *clk = @@ -1669,8 +1724,10 @@ unsigned long get_lcd_clk(void) if (cpu_is_exynos4()) return exynos4_get_lcd_clk(); else { - if (proid_is_exynos5420() || proid_is_exynos5800()) + if (proid_is_exynos5420()) return exynos5420_get_lcd_clk(); + else if (proid_is_exynos5800()) + return exynos5800_get_lcd_clk(); else return exynos5_get_lcd_clk(); } @@ -1683,8 +1740,10 @@ void set_lcd_clk(void) else { if (proid_is_exynos5250()) exynos5_set_lcd_clk(); - else if (proid_is_exynos5420() || proid_is_exynos5800()) + else if (proid_is_exynos5420()) exynos5420_set_lcd_clk(); + else + exynos5800_set_lcd_clk(); } } diff --git a/arch/arm/include/asm/arch-exynos/clk.h b/arch/arm/include/asm/arch-exynos/clk.h index db24dc0..bf3d348 100644 --- a/arch/arm/include/asm/arch-exynos/clk.h +++ b/arch/arm/include/asm/arch-exynos/clk.h @@ -16,6 +16,9 @@ #define BPLL 5 #define RPLL 6 #define SPLL 7 +#define CPLL 8 +#define DPLL 9 +#define IPLL 10 #define MASK_PRE_RATIO(x) (0xff << ((x << 4) + 8)) #define MASK_RATIO(x) (0xf << (x << 4))
Add get_lcd_clk and set_lcd_clk callbacks for Exynos5800 needed by exynos video driver. Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> --- arch/arm/cpu/armv7/exynos/clock.c | 63 +++++++++++++++++++++++++++++++- arch/arm/include/asm/arch-exynos/clk.h | 3 ++ 2 files changed, 64 insertions(+), 2 deletions(-)