Hello,
On 06/10/2011 10:15 AM, Anand Kumar N wrote:
> From: Jonghun Han<jonghun.han@xxxxxxxxxxx>
>
> This patch adds struct s3c_fb_driverdata s3c_fb_data_exynos4 for EXYNOS4.
> The clk_type is added to distinguish clock type in it and lcd_clk is added
> in structure s3c_fb to calculate divider for lcd panel.
>
> Please refer to below diagrams about clocks of FIMD IP. FIMD driver needs
these ascii diagrams are quite bulky, how about moving them to the cover
letter message ?
> two clocks for FIMD IP and LCD pixel clock. Actually, the LCD pixel clock
> can be selected from 1.clk 'lcd' and 2.SCLK_FIMD before EXYNOS4. But from
> EXYNOS4, the 2.SCLK_FIMD can be used only for source of LCD pixel clock.
>
> FIMD_CLK_TYPE0:
> ------------------------------------
> dsys bus
> ----------------+-------------------
> |
> |1.clk 'lcd'
> |
> | FIMD block
> +---+-----------+
> 4.mout_mpll |\ | | |
> --------|m| | +-+-+ +----+ |
> |u|-+ | | +-+core| |
> |x| | | | +----+ |
> |/ | | | |\ |
> | | +-|m| +---+ |
> | | |u|--+div| |
> +------+---|x| +---+ |
> 2.SCLK_FIMD | |/ | |
> | | |
> +----------+----+
> |
> inside of SoC |
> -----------------------+--------------------------
> outside of SoC |
> | 3.LCD pixel clock
> |
> +--------------+
> | LCD module |
> +--------------+
>
> FIMD_CLK_TYPE1:
> ------------------------------------
> dsys bus
> ----------------+-------------------
> |
> |1.clk 'fimd'
> |
> | FIMD block
> +---+-----------+
> 4.mout_mpll |\ | | |
> --------|m| | +-+-+ +----+ |
> |u|-+ | | +-+core| |
> |x| | | | +----+ |
> |/ | | | |\ |
> | | +-|m| +---+ |
> | | |u|--+div| |
> +------+---|x| +---+ |
> 2.SCLK_FIMD | |/ | |
> | | |
> +----------+----+
> |
> inside of SoC |
> -----------------------+--------------------------
> outside of SoC |
> | 3.LCD pixel clock
> |
> +--------------+
> | LCD module |
> +--------------+
>
> FIMD_CLK_TYPE1:
> ------------------------------------
> dsys bus
> ----------------+-------------------
> |
> |1.clk 'fimd'
> |
> | FIMD block
> +---+-----------+
> 4.mout_mpll |\ | | |
> --------|m| | | +----+ |
> |u|-+ | +---+core| |
> |x| | | +----+ |
> |/ | | |
> | | +---+ |
> | | +--+div| |
> +------+-----+ +---+ |
> 2.SCLK_FIMD | | |
> | | |
> +----------+----+
> |
> inside of SoC |
> -----------------------+--------------------------
> -----------------------+--------------------------
> outside of SoC |
> | 3.LCD pixel clock
> |
> +--------------+
> | LCD module |
> +--------------+
>
> FIMD_CLK_TYPE1:
> ------------------------------------
> dsys bus
> ----------------+-------------------
> |
> |1.clk 'fimd'
> |
> | FIMD block
> +---+-----------+
> 4.mout_mpll |\ | | |
> --------|m| | | +----+ |
> |u|-+ | +---+core| |
> |x| | | +----+ |
> |/ | | |
> | | +---+ |
> | | +--+div| |
> +------+-----+ +---+ |
> 2.SCLK_FIMD | | |
> | | |
> +----------+----+
> |
> inside of SoC |
> -----------------------+--------------------------
> outside of SoC |
> | 3.LCD pixel clock
> |
> +--------------+
> | LCD module |
> +--------------+
Sorry, the 2 above diagrams look almost identical to me.
Is there really any difference between them (except double line
above)?
>
> Change-Id: I52b69285151dc4d67bc0be5d0020ca49ba58f911
What is this (gerrit?) change ID here for ?
> Signed-off-by: Jonghun Han<jonghun.han@xxxxxxxxxxx>
> Signed-off-by: Jingoo Han<jg1.han@xxxxxxxxxxx>
> ---
> drivers/video/Kconfig | 2 +-
> drivers/video/s3c-fb.c | 131 ++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 117 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 549b960..963b8b7 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2027,7 +2027,7 @@ config FB_TMIO_ACCELL
>
> config FB_S3C
> tristate "Samsung S3C framebuffer support"
> - depends on FB&& S3C_DEV_FB
> + depends on FB&& (S3C_DEV_FB || S5P_DEV_FIMD0)
> select FB_CFB_FILLRECT
> select FB_CFB_COPYAREA
> select FB_CFB_IMAGEBLIT
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 0352afa..7445740 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -66,6 +66,9 @@ struct s3c_fb;
> #define VIDOSD_C(win, variant) (OSD_BASE(win, variant) + 0x08)
> #define VIDOSD_D(win, variant) (OSD_BASE(win, variant) + 0x0C)
>
> +#define FIMD_CLK_TYPE0 0
> +#define FIMD_CLK_TYPE1 1
> +
> /**
> * struct s3c_fb_variant - fb variant information
> * @is_2443: Set if S3C2443/S3C2416 style hardware.
> @@ -98,6 +101,7 @@ struct s3c_fb_variant {
>
> unsigned int has_prtcon:1;
> unsigned int has_shadowcon:1;
> + unsigned int clk_type:1;
Would "has_pixclk_mux" instead of clk_type make more sense ?
If so then you could get rid of the above FIMD_CLK_TYPE* definitions.
> };
>
> /**
> @@ -185,7 +189,8 @@ struct s3c_fb_vsync {
> * @slock: The spinlock protection for this data sturcture.
> * @dev: The device that we bound to, for printing, etc.
> * @regs_res: The resource we claimed for the IO registers.
> - * @bus_clk: The clk (hclk) feeding our interface and possibly pixclk.
> + * @bus_clk: The clk (hclk) feeding FIMD IP core.
> + * @lcd_clk: The clk (sclk) feeding our interface and possibly pixclk.
> * @regs: The mapped hardware registers.
> * @variant: Variant information for this hardware.
> * @enabled: A bitmask of enabled hardware windows.
> @@ -200,6 +205,7 @@ struct s3c_fb {
> struct device *dev;
> struct resource *regs_res;
> struct clk *bus_clk;
> + struct clk *lcd_clk;
> void __iomem *regs;
> struct s3c_fb_variant variant;
>
> @@ -337,7 +343,7 @@ static int s3c_fb_check_var(struct fb_var_screeninfo *var,
> */
> static int s3c_fb_calc_pixclk(struct s3c_fb *sfb, unsigned int pixclk)
> {
> - unsigned long clk = clk_get_rate(sfb->bus_clk);
> + unsigned long clk = clk_get_rate(sfb->lcd_clk);
> unsigned long long tmp;
> unsigned int result;
>
> @@ -1315,8 +1321,10 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
> struct s3c_fb_platdata *pd;
> struct s3c_fb *sfb;
> struct resource *res;
> + struct clk *mout_mpll = NULL;
> int win;
> int ret = 0;
> + u32 rate = 134000000;
How about making this the variant property ? Or is it same for all SoCs?
>
> platid = platform_get_device_id(pdev);
> fbdrv = (struct s3c_fb_driverdata *)platid->driver_data;
> @@ -1346,22 +1354,62 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
>
> spin_lock_init(&sfb->slock);
>
> - sfb->bus_clk = clk_get(dev, "lcd");
> - if (IS_ERR(sfb->bus_clk)) {
> - dev_err(dev, "failed to get bus clock\n");
> - ret = PTR_ERR(sfb->bus_clk);
> + switch (sfb->variant.clk_type) {
> + case FIMD_CLK_TYPE0:
> + sfb->bus_clk = clk_get(dev, "lcd");
> + if (IS_ERR(sfb->bus_clk)) {
> + dev_err(dev, "failed to get bus clock\n");
> + ret = PTR_ERR(sfb->bus_clk);
> + goto err_sfb;
> + }
> +
> + clk_enable(sfb->bus_clk);
> +
> + sfb->lcd_clk = sfb->bus_clk;
> + break;
> +
> + case FIMD_CLK_TYPE1:
> + sfb->bus_clk = clk_get(&pdev->dev, "fimd");
> + if (IS_ERR(sfb->bus_clk)) {
> + dev_err(&pdev->dev, "failed to get clock for fimd\n");
> + ret = PTR_ERR(sfb->bus_clk);
> + goto err_sfb;
> + }
> + clk_enable(sfb->bus_clk);
> +
> + sfb->lcd_clk = clk_get(&pdev->dev, "sclk_fimd");
> + if (IS_ERR(sfb->lcd_clk)) {
> + dev_err(&pdev->dev, "failed to get sclk for fimd\n");
> + ret = PTR_ERR(sfb->lcd_clk);
> + goto err_bus_clk;
> + }
> +
> + mout_mpll = clk_get(&pdev->dev, "mout_mpll");
> + if (IS_ERR(mout_mpll)) {
> + dev_err(&pdev->dev, "failed to get mout_mpll\n");
> + ret = PTR_ERR(mout_mpll);
> + goto err_lcd_clk;
> + }
> + clk_set_parent(sfb->lcd_clk, mout_mpll);
> + clk_put(mout_mpll);
> +
> + clk_set_rate(sfb->lcd_clk, rate);
> + clk_enable(sfb->lcd_clk);
Please, do not hard code parent clocks in the driver.
> + dev_dbg(&pdev->dev, "set fimd sclk rate to %d\n", rate);
> + break;
> +
> + default:
> + dev_err(dev, "failed to enable clock for FIMD\n");
> goto err_sfb;
> }
I'm not really a clock expert, but IMHO there is an issue in Thomas'
migration to clkdev proposal [1] that the device clock connection ids
(con_id) and clock names related to them are identical. Mostly it works
but in situation like this one it is not possible to remap two clocks
of different names onto a single connection id.
For instance, looking at arch/arm/mach-omap2/clock3xxx_data.c:
static struct clk i2c3_fck = {
.name = "i2c3_fck",
^^^^^^^^^^
.ops = &clkops_omap2_dflt_wait,
.parent = &core_96m_fck,
...
};
static struct clk i2c2_fck = {
.name = "i2c2_fck",
^^^^^^^^^^^
.ops = &clkops_omap2_dflt_wait,
...
};
static struct omap_clk omap3xxx_clks[] = {
...
CLK("omap_i2c.3", "fck", &i2c3_fck, CK_3XXX),
^^^^^^^^^^^^^^^^^
CLK("omap_i2c.2", "fck", &i2c2_fck, CK_3XXX),
^^^^^^^^^^^^^^^^^
...
Different clock names (i2c3_fck, i2c3_fck,..) are mapped to single
unified id (fck), which the driver only needs to care about.
The name is common across H/W instances and also SoC variants.
So it doesn't have to be driver's business to resolve clock differences.
The same (con_id) name can be used on distinct SoC versionproviding similar/same peripheral device IP.
It's aeasy to figure out by just comparing omap3xxx_clks[] and
omap44xx_clks arrays.
That said I wouldn't go for a "devname" in struct clk, just create
an additional table matching device names, con_id and struct clk and
use it while registering clk_lookup items into clkdev.
>
> - clk_enable(sfb->bus_clk);
> -
> pm_runtime_enable(sfb->dev);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> dev_err(dev, "failed to find registers\n");
> ret = -ENOENT;
> - goto err_clk;
> + goto err_lcd_clk;
> }
>
> sfb->regs_res = request_mem_region(res->start, resource_size(res),
> @@ -1369,7 +1417,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
> if (!sfb->regs_res) {
> dev_err(dev, "failed to claim register region\n");
> ret = -ENOENT;
> - goto err_clk;
> + goto err_lcd_clk;
> }
>
> sfb->regs = ioremap(res->start, resource_size(res));
> @@ -1451,9 +1499,15 @@ err_ioremap:
> err_req_region:
> release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
>
> -err_clk:
> - clk_disable(sfb->bus_clk);
> - clk_put(sfb->bus_clk);
> +err_lcd_clk:
> + clk_disable(sfb->lcd_clk);
> + clk_put(sfb->lcd_clk);
> +
> +err_bus_clk:
> + if (sfb->variant.clk_type != FIMD_CLK_TYPE0) {
> + clk_disable(sfb->bus_clk);
> + clk_put(sfb->bus_clk);
> + }
>
> err_sfb:
> kfree(sfb);
> @@ -1482,8 +1536,20 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
>
> iounmap(sfb->regs);
>
> - clk_disable(sfb->bus_clk);
> - clk_put(sfb->bus_clk);
> + switch (sfb->variant.clk_type) {
> + case FIMD_CLK_TYPE1:
> + clk_disable(sfb->lcd_clk);
> + clk_put(sfb->lcd_clk);
> + /* fall through to default case */
> + case FIMD_CLK_TYPE0:
> + clk_disable(sfb->bus_clk);
> + clk_put(sfb->bus_clk);
> + break;
> + default:
Considering clk_type data type this is unreachable code.
> + dev_err(sfb->dev, "invalid clock type(%d)\n",
> + sfb->variant.clk_type);
> + break;
> + }
...
--
Regards,
Sylwester
[1] http://www.spinics.net/lists/arm-kernel/msg127901.html
http://www.spinics.net/lists/arm-kernel/msg127907.html
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
