drm/omap: Migrate minimum FCK/PCK ratio from Kconfig to dts
diff mbox series

Message ID 20190510194229.20628-1-aford173@gmail.com
State New
Headers show
Series
  • drm/omap: Migrate minimum FCK/PCK ratio from Kconfig to dts
Related show

Commit Message

Adam Ford May 10, 2019, 7:42 p.m. UTC
Currently the source code is compiled using hard-coded values
from CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.  This patch allows this
clock divider value to be moved to the device tree and be changed
without having to recompile the kernel.

Signed-off-by: Adam Ford <aford173@gmail.com>

Comments

Tomi Valkeinen May 28, 2019, 11:11 a.m. UTC | #1
Hi,

On 10/05/2019 22:42, Adam Ford wrote:
> Currently the source code is compiled using hard-coded values
> from CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.  This patch allows this
> clock divider value to be moved to the device tree and be changed
> without having to recompile the kernel.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>

I understand why you want to do this, but I'm not sure it's a good idea. 
It's really something the driver should figure out, and if we add it to 
the DT, it effectively becomes an ABI.

That said... I'm not sure how good of a job the driver could ever do, as 
it can't know the future scaling needs of the userspace at the time it 
is configuring the clock. And so, I'm not nacking this patch, but I 
don't feel very good about this patch...

The setting also affects all outputs (exluding venc), which may not be 
what the user wants. Then again, I think this setting is really only 
needed on OMAP2 & 3, which have only a single output. But that's the 
same with the current kconfig option, of course.

So, the current CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK is an ugly hack, in my 
opinion, and moving it to DT makes it a worse hack =). But I don't have 
any good suggestions either.

  Tomi
Adam Ford May 28, 2019, 3:09 p.m. UTC | #2
On Tue, May 28, 2019 at 4:11 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi,
>
> On 10/05/2019 22:42, Adam Ford wrote:
> > Currently the source code is compiled using hard-coded values
> > from CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.  This patch allows this
> > clock divider value to be moved to the device tree and be changed
> > without having to recompile the kernel.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
>
> I understand why you want to do this, but I'm not sure it's a good idea.
> It's really something the driver should figure out, and if we add it to
> the DT, it effectively becomes an ABI.
>
> That said... I'm not sure how good of a job the driver could ever do, as
> it can't know the future scaling needs of the userspace at the time it
> is configuring the clock. And so, I'm not nacking this patch, but I
> don't feel very good about this patch...
>
> The setting also affects all outputs (exluding venc), which may not be
> what the user wants. Then again, I think this setting is really only
> needed on OMAP2 & 3, which have only a single output. But that's the
> same with the current kconfig option, of course.
>
> So, the current CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK is an ugly hack, in my
> opinion, and moving it to DT makes it a worse hack =). But I don't have
> any good suggestions either.

As it stands the Logic PD OMAP35 and AM37/DM37 boards (SOM-LV and
Torpedo) require this to be hard coded to 4 or it hangs during start.
This is the case for all versions 4.2+.  I haven't tested it with
older stuff.  Tony has a DM3730 Torpedo kit and reported the hanging
issue to me. I told him to set that value to 4 to make it not hang.
He asked that I move it to the DT to avoid custom kernels.  I agree
it's a hack, but if it's create a customized defconfig file for 4
boards or modify the device tree, it seems like the device tree
approach is less intrusive.

adam
>
>   Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
H. Nikolaus Schaller May 28, 2019, 3:20 p.m. UTC | #3
> Am 28.05.2019 um 17:09 schrieb Adam Ford <aford173@gmail.com>:
> 
> On Tue, May 28, 2019 at 4:11 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> 
>> Hi,
>> 
>> On 10/05/2019 22:42, Adam Ford wrote:
>>> Currently the source code is compiled using hard-coded values
>>> from CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.  This patch allows this
>>> clock divider value to be moved to the device tree and be changed
>>> without having to recompile the kernel.
>>> 
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>> 
>> I understand why you want to do this, but I'm not sure it's a good idea.
>> It's really something the driver should figure out, and if we add it to
>> the DT, it effectively becomes an ABI.
>> 
>> That said... I'm not sure how good of a job the driver could ever do, as
>> it can't know the future scaling needs of the userspace at the time it
>> is configuring the clock. And so, I'm not nacking this patch, but I
>> don't feel very good about this patch...
>> 
>> The setting also affects all outputs (exluding venc), which may not be
>> what the user wants. Then again, I think this setting is really only
>> needed on OMAP2 & 3, which have only a single output. But that's the
>> same with the current kconfig option, of course.
>> 
>> So, the current CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK is an ugly hack, in my
>> opinion, and moving it to DT makes it a worse hack =). But I don't have
>> any good suggestions either.
> 
> As it stands the Logic PD OMAP35 and AM37/DM37 boards (SOM-LV and
> Torpedo) require this to be hard coded to 4 or it hangs during start.
> This is the case for all versions 4.2+.  I haven't tested it with
> older stuff.  Tony has a DM3730 Torpedo kit and reported the hanging
> issue to me. I told him to set that value to 4 to make it not hang.
> He asked that I move it to the DT to avoid custom kernels.  I agree
> it's a hack, but if it's create a customized defconfig file for 4
> boards or modify the device tree, it seems like the device tree
> approach is less intrusive.

Well, if this boards needs a factor 4 to be defined, it is IMHO
100 % correct to describe this in the DTS and nowhere else. Like
minimum and maximum voltage of a regulator which is also very board
specific.

Unless it can be figured out automatically. If it turns out later
that it can, I would assume the drivers can simply ignore the hint
in the DTS?

Just my 2cts without knowing details and having tested anything
on our DM37 boards.

BR,
Nikolaus
Tomi Valkeinen May 28, 2019, 3:53 p.m. UTC | #4
Hi,

On 28/05/2019 18:09, Adam Ford wrote:
> On Tue, May 28, 2019 at 4:11 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>> Hi,
>>
>> On 10/05/2019 22:42, Adam Ford wrote:
>>> Currently the source code is compiled using hard-coded values
>>> from CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.  This patch allows this
>>> clock divider value to be moved to the device tree and be changed
>>> without having to recompile the kernel.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>
>> I understand why you want to do this, but I'm not sure it's a good idea.
>> It's really something the driver should figure out, and if we add it to
>> the DT, it effectively becomes an ABI.
>>
>> That said... I'm not sure how good of a job the driver could ever do, as
>> it can't know the future scaling needs of the userspace at the time it
>> is configuring the clock. And so, I'm not nacking this patch, but I
>> don't feel very good about this patch...
>>
>> The setting also affects all outputs (exluding venc), which may not be
>> what the user wants. Then again, I think this setting is really only
>> needed on OMAP2 & 3, which have only a single output. But that's the
>> same with the current kconfig option, of course.
>>
>> So, the current CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK is an ugly hack, in my
>> opinion, and moving it to DT makes it a worse hack =). But I don't have
>> any good suggestions either.
> 
> As it stands the Logic PD OMAP35 and AM37/DM37 boards (SOM-LV and
> Torpedo) require this to be hard coded to 4 or it hangs during start.
> This is the case for all versions 4.2+.  I haven't tested it with
> older stuff.  Tony has a DM3730 Torpedo kit and reported the hanging
> issue to me. I told him to set that value to 4 to make it not hang.
> He asked that I move it to the DT to avoid custom kernels.  I agree
> it's a hack, but if it's create a customized defconfig file for 4
> boards or modify the device tree, it seems like the device tree
> approach is less intrusive.

Ok, well, I think that's a separate thing from its intended use. The 
point of the kconfig option is to ensure that the fclk/pclk ratio stays 
under a certain number to allow enough scaling range. It should never 
affect a basic non-scaling use case, unless you set it to a too high 
value, which prevents finding any pclk.

Has anyone debugged why the hang is happening?

If we can't fix the bug itself, rather than adding a DT option, we could 
change add a min_fck_per_pck field (as you do), keep the kconfig option, 
and set the min_fck_per_pck based on the kconfig option, _or_ in case of 
those affected SoCs, set the min_fck_per_pck even without the kconfig 
option.

  Tomi
Adam Ford May 31, 2019, 12:13 p.m. UTC | #5
On Tue, May 28, 2019 at 10:53 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi,
>
> On 28/05/2019 18:09, Adam Ford wrote:
> > On Tue, May 28, 2019 at 4:11 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>
> >> Hi,
> >>
> >> On 10/05/2019 22:42, Adam Ford wrote:
> >>> Currently the source code is compiled using hard-coded values
> >>> from CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.  This patch allows this
> >>> clock divider value to be moved to the device tree and be changed
> >>> without having to recompile the kernel.
> >>>
> >>> Signed-off-by: Adam Ford <aford173@gmail.com>
> >>
> >> I understand why you want to do this, but I'm not sure it's a good idea.
> >> It's really something the driver should figure out, and if we add it to
> >> the DT, it effectively becomes an ABI.
> >>
> >> That said... I'm not sure how good of a job the driver could ever do, as
> >> it can't know the future scaling needs of the userspace at the time it
> >> is configuring the clock. And so, I'm not nacking this patch, but I
> >> don't feel very good about this patch...
> >>
> >> The setting also affects all outputs (exluding venc), which may not be
> >> what the user wants. Then again, I think this setting is really only
> >> needed on OMAP2 & 3, which have only a single output. But that's the
> >> same with the current kconfig option, of course.
> >>
> >> So, the current CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK is an ugly hack, in my
> >> opinion, and moving it to DT makes it a worse hack =). But I don't have
> >> any good suggestions either.
> >
> > As it stands the Logic PD OMAP35 and AM37/DM37 boards (SOM-LV and
> > Torpedo) require this to be hard coded to 4 or it hangs during start.
> > This is the case for all versions 4.2+.  I haven't tested it with
> > older stuff.  Tony has a DM3730 Torpedo kit and reported the hanging
> > issue to me. I told him to set that value to 4 to make it not hang.
> > He asked that I move it to the DT to avoid custom kernels.  I agree
> > it's a hack, but if it's create a customized defconfig file for 4
> > boards or modify the device tree, it seems like the device tree
> > approach is less intrusive.
>
> Ok, well, I think that's a separate thing from its intended use. The
> point of the kconfig option is to ensure that the fclk/pclk ratio stays
> under a certain number to allow enough scaling range. It should never
> affect a basic non-scaling use case, unless you set it to a too high
> value, which prevents finding any pclk.
>
> Has anyone debugged why the hang is happening?

I tried debugging this years ago, and I was told to use the
CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.
>
> If we can't fix the bug itself, rather than adding a DT option, we could
> change add a min_fck_per_pck field (as you do), keep the kconfig option,
> and set the min_fck_per_pck based on the kconfig option, _or_ in case of
> those affected SoCs, set the min_fck_per_pck even without the kconfig
> option.

I am just curious if anyone else sees this.  If nobody is using this
hack, I wonder how much of the impact it will be.  I'm trying trying
to get my board to boot without hanging without creating a custom
defconfig.

adam
>
>   Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Rob Herring June 13, 2019, 8:22 p.m. UTC | #6
On Fri, May 10, 2019 at 02:42:29PM -0500, Adam Ford wrote:
> Currently the source code is compiled using hard-coded values
> from CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK.  This patch allows this
> clock divider value to be moved to the device tree and be changed
> without having to recompile the kernel.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
> index cd02516a40b6..42449d07c47e 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
> +++ b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
> @@ -40,7 +40,7 @@ Required properties:
>  Optional properties:
>  - max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
>  			in bytes per second
> -
> +- min-fck-pck-ratio:  Make sure that DISPC FCK is at least n x PCK

Assuming this patch progresses, this needs a vendor prefix and please 
split bindings to separate patch.

>  
>  RFBI
>  ----
> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
> index 4043ecb38016..bf84a8487aae 100644
> --- a/arch/arm/boot/dts/omap3.dtsi
> +++ b/arch/arm/boot/dts/omap3.dtsi
> @@ -751,7 +751,7 @@
>  			#size-cells = <1>;
>  			ranges;
>  
> -			dispc@48050400 {
> +			dispc: dispc@48050400 {

Unrelated change.

>  				compatible = "ti,omap3-dispc";
>  				reg = <0x48050400 0x400>;
>  				interrupts = <25>;
> diff --git a/drivers/gpu/drm/omapdrm/dss/Kconfig b/drivers/gpu/drm/omapdrm/dss/Kconfig
> index f24ebf7f61dd..d0666edcdf2a 100644
> --- a/drivers/gpu/drm/omapdrm/dss/Kconfig
> +++ b/drivers/gpu/drm/omapdrm/dss/Kconfig
> @@ -102,24 +102,6 @@ config OMAP2_DSS_DSI
>  
>  	  See http://www.mipi.org/ for DSI specifications.
>  
> -config OMAP2_DSS_MIN_FCK_PER_PCK
> -	int "Minimum FCK/PCK ratio (for scaling)"
> -	range 0 32
> -	default 0
> -	help
> -	  This can be used to adjust the minimum FCK/PCK ratio.
> -
> -	  With this you can make sure that DISPC FCK is at least
> -	  n x PCK. Video plane scaling requires higher FCK than
> -	  normally.
> -
> -	  If this is set to 0, there's no extra constraint on the
> -	  DISPC FCK. However, the FCK will at minimum be
> -	  2xPCK (if active matrix) or 3xPCK (if passive matrix).
> -
> -	  Max FCK is 173MHz, so this doesn't work if your PCK
> -	  is very high.
> -
>  config OMAP2_DSS_SLEEP_AFTER_VENC_RESET
>  	bool "Sleep 20ms after VENC reset"
>  	default y
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index ba82d916719c..09a130c53da2 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -198,6 +198,9 @@ struct dispc_device {
>  
>  	/* DISPC_CONTROL & DISPC_CONFIG lock*/
>  	spinlock_t control_lock;
> +
> +	/* Optional min-fck-pck-ratio */
> +	u32 min_fck_per_pck;
>  };
>  
>  enum omap_color_component {
> @@ -3683,15 +3686,8 @@ bool dispc_div_calc(struct dispc_device *dispc, unsigned long dispc_freq,
>  	unsigned long pck, lck;
>  	unsigned long lck_max;
>  	unsigned long pckd_hw_min, pckd_hw_max;
> -	unsigned int min_fck_per_pck;
>  	unsigned long fck;
>  
> -#ifdef CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK
> -	min_fck_per_pck = CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK;
> -#else
> -	min_fck_per_pck = 0;
> -#endif
> -
>  	pckd_hw_min = dispc->feat->min_pcd;
>  	pckd_hw_max = 255;
>  
> @@ -3723,7 +3719,7 @@ bool dispc_div_calc(struct dispc_device *dispc, unsigned long dispc_freq,
>  			else
>  				fck = lck;
>  
> -			if (fck < pck * min_fck_per_pck)
> +			if (fck < pck * dispc->min_fck_per_pck)
>  				continue;
>  
>  			if (func(lckd, pckd, lck, pck, data))
> @@ -4826,6 +4822,8 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
>  		}
>  	}
>  
> +	of_property_read_u32(np, "min-fck-pck-ratio", &dispc->min_fck_per_pck);
> +
>  	r = dispc_init_gamma_tables(dispc);
>  	if (r)
>  		goto err_free;
> -- 
> 2.17.1
>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
index cd02516a40b6..42449d07c47e 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
@@ -40,7 +40,7 @@  Required properties:
 Optional properties:
 - max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
 			in bytes per second
-
+- min-fck-pck-ratio:  Make sure that DISPC FCK is at least n x PCK
 
 RFBI
 ----
diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index 4043ecb38016..bf84a8487aae 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -751,7 +751,7 @@ 
 			#size-cells = <1>;
 			ranges;
 
-			dispc@48050400 {
+			dispc: dispc@48050400 {
 				compatible = "ti,omap3-dispc";
 				reg = <0x48050400 0x400>;
 				interrupts = <25>;
diff --git a/drivers/gpu/drm/omapdrm/dss/Kconfig b/drivers/gpu/drm/omapdrm/dss/Kconfig
index f24ebf7f61dd..d0666edcdf2a 100644
--- a/drivers/gpu/drm/omapdrm/dss/Kconfig
+++ b/drivers/gpu/drm/omapdrm/dss/Kconfig
@@ -102,24 +102,6 @@  config OMAP2_DSS_DSI
 
 	  See http://www.mipi.org/ for DSI specifications.
 
-config OMAP2_DSS_MIN_FCK_PER_PCK
-	int "Minimum FCK/PCK ratio (for scaling)"
-	range 0 32
-	default 0
-	help
-	  This can be used to adjust the minimum FCK/PCK ratio.
-
-	  With this you can make sure that DISPC FCK is at least
-	  n x PCK. Video plane scaling requires higher FCK than
-	  normally.
-
-	  If this is set to 0, there's no extra constraint on the
-	  DISPC FCK. However, the FCK will at minimum be
-	  2xPCK (if active matrix) or 3xPCK (if passive matrix).
-
-	  Max FCK is 173MHz, so this doesn't work if your PCK
-	  is very high.
-
 config OMAP2_DSS_SLEEP_AFTER_VENC_RESET
 	bool "Sleep 20ms after VENC reset"
 	default y
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index ba82d916719c..09a130c53da2 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -198,6 +198,9 @@  struct dispc_device {
 
 	/* DISPC_CONTROL & DISPC_CONFIG lock*/
 	spinlock_t control_lock;
+
+	/* Optional min-fck-pck-ratio */
+	u32 min_fck_per_pck;
 };
 
 enum omap_color_component {
@@ -3683,15 +3686,8 @@  bool dispc_div_calc(struct dispc_device *dispc, unsigned long dispc_freq,
 	unsigned long pck, lck;
 	unsigned long lck_max;
 	unsigned long pckd_hw_min, pckd_hw_max;
-	unsigned int min_fck_per_pck;
 	unsigned long fck;
 
-#ifdef CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK
-	min_fck_per_pck = CONFIG_OMAP2_DSS_MIN_FCK_PER_PCK;
-#else
-	min_fck_per_pck = 0;
-#endif
-
 	pckd_hw_min = dispc->feat->min_pcd;
 	pckd_hw_max = 255;
 
@@ -3723,7 +3719,7 @@  bool dispc_div_calc(struct dispc_device *dispc, unsigned long dispc_freq,
 			else
 				fck = lck;
 
-			if (fck < pck * min_fck_per_pck)
+			if (fck < pck * dispc->min_fck_per_pck)
 				continue;
 
 			if (func(lckd, pckd, lck, pck, data))
@@ -4826,6 +4822,8 @@  static int dispc_bind(struct device *dev, struct device *master, void *data)
 		}
 	}
 
+	of_property_read_u32(np, "min-fck-pck-ratio", &dispc->min_fck_per_pck);
+
 	r = dispc_init_gamma_tables(dispc);
 	if (r)
 		goto err_free;