[PATCH 1/2] video/fsl: make the diu driver work without platform hooks

[PATCH 1/2] video/fsl: make the diu driver work without platform hooks

[Jason Jin]

make the diu driver work without platform hooks.

So far the DIU driver does not have a mechanism to do the
board specific initialization. So on some platforms,
such as P1022, 8610 and 5121, The board specific initialization
is implmented in the platform file such p10222_ds.

This board sepecific initialization mechanism is not feasible i
for corenet platform as the corenet platform file is a
abstraction of serveral platforms.

However, the DIU is already initialized in u-boot and we can
rely on the settings in u-boot for corenet platform, the only
issue is that when DIU wake up from the deepsleep, some of the
board specific initialization will lost, such as the pixel clock
setting.

This patch try to make the diu work on the platform without board
specific initialization such as corenet(T1040 so far), and rely on
the board initialization in u-boot. The following patch will try to
fix the pixel clock saving issue for deepsleep.

Signed-off-by: Jason Jin <Jason.Jin@freescale.com>
---
 drivers/video/fsl-diu-fb.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
index e8758b9..4640846 100644
--- a/drivers/video/fsl-diu-fb.c
+++ b/drivers/video/fsl-diu-fb.c
@@ -479,7 +479,10 @@ static enum fsl_diu_monitor_port fsl_diu_name_to_port(const char *s)
 			port = FSL_DIU_PORT_DLVDS;
 	}
 
-	return diu_ops.valid_monitor_port(port);
+	if (diu_ops.valid_monitor_port)
+		return diu_ops.valid_monitor_port(port);
+	else
+		return port;
 }
 
 /*
@@ -846,7 +849,11 @@ static void update_lcdc(struct fb_info *info)
 
 	out_be32(&hw->vsyn_para, temp);
 
-	diu_ops.set_pixel_clock(var->pixclock);
+	/*if there is platform function for pixel clock setting, use it.
+	 * otherwise we rely on the settings in u-boot.
+	 */
+	if (diu_ops.set_pixel_clock)
+		diu_ops.set_pixel_clock(var->pixclock);
 
 #ifndef CONFIG_PPC_MPC512x
 	/*
-- 
1.8.0

[PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module

[Jason Jin]

For sleep, The diu module will power off and when
wake up for initialization will need.

Some platform such as the corenet plafrom does not provide
the DIU board sepecific initialization, but rely on the
DIU initializtion in u-boot. then the pixel clock resume will
be an issuel on this platform, This patch save the pixel clock
for diu before enter the deepsleep and then restore it after
resume, the extra pixelclock register information will be needed
in this situation.

Signed-off-by: Jason Jin <Jason.Jin@freescale.com>
Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
---
v2: clean up the resume function based on Timur's comments.
Add the pixel clock saving for the platforms without board sepecific
initializaton.

 drivers/video/fsl-diu-fb.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
index 4640846..fbdd166 100644
--- a/drivers/video/fsl-diu-fb.c
+++ b/drivers/video/fsl-diu-fb.c
@@ -372,6 +372,7 @@ struct fsl_diu_data {
 	unsigned int irq;
 	enum fsl_diu_monitor_port monitor_port;
 	struct diu __iomem *diu_reg;
+	void __iomem *pixelclk_ptr;
 	spinlock_t reg_lock;
 	u8 dummy_aoi[4 * 4 * 4];
 	struct diu_ad dummy_ad __aligned(8);
@@ -383,6 +384,7 @@ struct fsl_diu_data {
 	__le16 blank_cursor[MAX_CURS * MAX_CURS] __aligned(32);
 	uint8_t edid_data[EDID_LENGTH];
 	bool has_edid;
+	u32 saved_pixel_clock;
 } __aligned(32);
 
 /* Determine the DMA address of a member of the fsl_diu_data structure */
@@ -1625,19 +1627,47 @@ static irqreturn_t fsl_diu_isr(int irq, void *dev_id)
 static int fsl_diu_suspend(struct platform_device *ofdev, pm_message_t state)
 {
 	struct fsl_diu_data *data;
+	struct resource res;
+	int ret;
 
+	ret = 0;
 	data = dev_get_drvdata(&ofdev->dev);
 	disable_lcdc(data->fsl_diu_info);
 
-	return 0;
+	if (!diu_ops.set_pixel_clock) {
+		data->saved_pixel_clock = 0;
+		if (of_address_to_resource(ofdev->dev.of_node, 1, &res))
+			pr_err(KERN_ERR "No pixel clock set func and no pixel node!\n");
+		else {
+			data->pixelclk_ptr =
+				devm_ioremap(&ofdev->dev, res.start, resource_size(&res));
+			if (!data->pixelclk_ptr) {
+				pr_err(KERN_ERR "fslfb: could not map pixelclk register!\n");
+				ret = -ENOMEM;
+			} else
+				data->saved_pixel_clock = in_be32(data->pixelclk_ptr);
+		}
+	}
+
+	return ret;
 }
 
 static int fsl_diu_resume(struct platform_device *ofdev)
 {
 	struct fsl_diu_data *data;
+	unsigned int i;
 
 	data = dev_get_drvdata(&ofdev->dev);
-	enable_lcdc(data->fsl_diu_info);
+	if (!diu_ops.set_pixel_clock && data->pixelclk_ptr)
+		out_be32(data->pixelclk_ptr, data->saved_pixel_clock);
+
+	fsl_diu_enable_interrupts(data);
+	update_lcdc(data->fsl_diu_info);
+
+	for (i = 0; i < NUM_AOIS; i++) {
+		if (data->mfb[i].count)
+			fsl_diu_enable_panel(&data->fsl_diu_info[i]);
+	}
 
 	return 0;
 }
-- 
1.8.0

Re: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks

[Timur Tabi]

On 03/26/2014 12:41 PM, Jason Jin wrote:
> This board sepecific initialization mechanism is not feasible i
> for corenet platform as the corenet platform file is a
> abstraction of serveral platforms.

You can't make it 100% abstract.  The DIU driver requires some sort of 
board support.  I think you can make a generic platform file for the DIU.

> However, the DIU is already initialized in u-boot and we can
> rely on the settings in u-boot for corenet platform,

I don't like that at all.

> the only
> issue is that when DIU wake up from the deepsleep, some of the
> board specific initialization will lost, such as the pixel clock
> setting.

That is a BIG issue.  This is why we have a platform file -- to handle 
the platform issues.

Re: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module

[Timur Tabi]

On 03/26/2014 12:41 PM, Jason Jin wrote:
> +	if (!diu_ops.set_pixel_clock) {
> +		data->saved_pixel_clock = 0;
> +		if (of_address_to_resource(ofdev->dev.of_node, 1, &res))
> +			pr_err(KERN_ERR "No pixel clock set func and no pixel node!\n");
> +		else {
> +			data->pixelclk_ptr =
> +				devm_ioremap(&ofdev->dev, res.start, resource_size(&res));
> +			if (!data->pixelclk_ptr) {
> +				pr_err(KERN_ERR "fslfb: could not map pixelclk register!\n");
> +				ret = -ENOMEM;
> +			} else
> +				data->saved_pixel_clock = in_be32(data->pixelclk_ptr);
> +		}
> +	}

This seems very hackish.  What node does ofdev point to?  I wonder if 
this code should be in the platform file instead.

Also, use dev_info() instead of pr_err, and never use exclamation marks 
in driver messages.

RE: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks

[Jason.Jin]

> On 03/26/2014 12:41 PM, Jason Jin wrote:
> > This board sepecific initialization mechanism is not feasible i for
> > corenet platform as the corenet platform file is a abstraction of
> > serveral platforms.
>=20
> You can't make it 100% abstract.  The DIU driver requires some sort of
> board support.  I think you can make a generic platform file for the DIU.
>=20
[Jason Jin-R64188] Provide the generic diu platform file is reasonable, but=
 it is will be just the board specific initialization file collection, if w=
e really need to reinitialize something in kernel.=20

> > However, the DIU is already initialized in u-boot and we can rely on
> > the settings in u-boot for corenet platform,
>=20
> I don't like that at all.
[Jason Jin-R64188] What's the potential issue of this? Most of the IPs need=
 the platform initialization in u-boot. For example, the hwconfig in u-boot=
 used for board specific settings for most platform.

The diu_ops structure is still open for any board specific initialization i=
f the platform needed. We at least can let the platform to select to use th=
e diu_ops or not. This is the purse for this patch.

>=20
> > the only
> > issue is that when DIU wake up from the deepsleep, some of the board
> > specific initialization will lost, such as the pixel clock setting.
>=20
> That is a BIG issue.  This is why we have a platform file -- to handle
> the platform issues.
[Jason Jin-R64188] Yes, this is an issue, Actually, this is a SOC level iss=
ue, We can try to fix it in the driver by saving the pixel clock in suspend=
 function and restored it in resume function.

Thanks.

Re: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks

[Timur Tabi]

Jason.Jin@freescale.com wrote:
>>>>> However, the DIU is already initialized in u-boot and we can
>>>>> rely on the settings in u-boot for corenet platform,
>>>
>>> I don't like that at all.
> [Jason Jin-R64188] What's the potential issue of this? Most of the
> IPs need the platform initialization in u-boot. For example, the
> hwconfig in u-boot used for board specific settings for most
> platform.

The potential problem is that it's not stable.  Power management breaks 
your code.

I won't NACK your patch, but it feels like a band-aid rather than a real 
solution.

RE: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks

[Jason.Jin]

> Jason.Jin@freescale.com wrote:
> >>>>> However, the DIU is already initialized in u-boot and we can rely
> >>>>> on the settings in u-boot for corenet platform,
> >>>
> >>> I don't like that at all.
> > [Jason Jin-R64188] What's the potential issue of this? Most of the IPs
> > need the platform initialization in u-boot. For example, the hwconfig
> > in u-boot used for board specific settings for most platform.
>=20
> The potential problem is that it's not stable.  Power management breaks
> your code.
>=20
> I won't NACK your patch, but it feels like a band-aid rather than a real
> solution.
>=20
[Jason Jin-R64188] Thanks. The power management will break the pixel clock =
only. We can try to fix it separately. I'll try to explain it in that mail.

RE: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module

[Jason.Jin]

>=20
> On 03/26/2014 12:41 PM, Jason Jin wrote:
> > +	if (!diu_ops.set_pixel_clock) {
> > +		data->saved_pixel_clock =3D 0;
> > +		if (of_address_to_resource(ofdev->dev.of_node, 1, &res))
> > +			pr_err(KERN_ERR "No pixel clock set func and no pixel
> node!\n");
> > +		else {
> > +			data->pixelclk_ptr =3D
> > +				devm_ioremap(&ofdev->dev, res.start,
> resource_size(&res));
> > +			if (!data->pixelclk_ptr) {
> > +				pr_err(KERN_ERR "fslfb: could not map pixelclk
> register!\n");
> > +				ret =3D -ENOMEM;
> > +			} else
> > +				data->saved_pixel_clock =3D in_be32(data-
> >pixelclk_ptr);
> > +		}
> > +	}
>=20
> This seems very hackish.  What node does ofdev point to?  I wonder if
> this code should be in the platform file instead.
>=20
[Jason Jin-R64188] It's not hackish, we can provide the pixel clock registe=
r in the DIU node, I did not provide the dts update as this is only tested =
on T1040 platform. For other platforms such as p1022 and 8610, we still can=
 use the pixel clock setting function in the platform.=20

The dts node update for T1040 is:
display:display@180000 {
        compatible =3D "fsl,t1040-diu", "fsl,diu";
-       reg =3D <0x180000 1000>;
+       reg =3D <0x180000 1000 0xfc028 4>;
        interrupts =3D <74 2 0 0>;
};

If saving the pixel clock register likes a hackish. We can provide more inf=
ormation in the node to move the pixel clock settings to the driver. It see=
ms that we only need to provide another parameter(the ratio) for pixel cloc=
k setting.

> Also, use dev_info() instead of pr_err, and never use exclamation marks
> in driver messages.
Ok, Thanks.

Re: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module

[Timur Tabi]

Jason.Jin@freescale.com wrote:
> [Jason Jin-R64188] It's not hackish, we can provide the pixel clock register in the DIU node, I did not provide the dts update as this is only tested on T1040 platform. For other platforms such as p1022 and 8610, we still can use the pixel clock setting function in the platform.
>
> The dts node update for T1040 is:
> display:display@180000 {
>          compatible = "fsl,t1040-diu", "fsl,diu";
> -       reg = <0x180000 1000>;
> +       reg = <0x180000 1000 0xfc028 4>;
>          interrupts = <74 2 0 0>;
> };

This is hackish because you're specifying a single register that you 
want to preserve in the DTS file, instead of a platform function which 
is where it's supposed to be.

I will think about this some more.  I think you are trying too hard to 
avoid a platform file, which is why some of this code is hackish to me.

RE: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module

[Jason.Jin]

> Jason.Jin@freescale.com wrote:
> > [Jason Jin-R64188] It's not hackish, we can provide the pixel clock
> register in the DIU node, I did not provide the dts update as this is
> only tested on T1040 platform. For other platforms such as p1022 and 8610=
,
> we still can use the pixel clock setting function in the platform.
> >
> > The dts node update for T1040 is:
> > display:display@180000 {
> >          compatible =3D "fsl,t1040-diu", "fsl,diu";
> > -       reg =3D <0x180000 1000>;
> > +       reg =3D <0x180000 1000 0xfc028 4>;
> >          interrupts =3D <74 2 0 0>;
> > };
>=20
> This is hackish because you're specifying a single register that you want
> to preserve in the DTS file, instead of a platform function which is
> where it's supposed to be.
>=20
[Jason Jin-R64188] The pixel clock register is actually part of the DIU reg=
isters although it is not implemented in the diu module. Actually we can us=
e it to set pixel clock in the driver not just saving it in suspend. We can=
 provide the patch for discussion.=20

> I will think about this some more.  I think you are trying too hard to
> avoid a platform file, which is why some of this code is hackish to me.
[Jason Jin-R64188] Thanks. Could you please share you thinking for how to s=
etup the platform file then?