Re: [PATCH 00/12] ARM: davinci: da850-evm: remove more legacy GPIO calls

[Sekhar Nori]

Hi Lee, Daniel, Jingoo,

On 25/06/19 10:04 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski 
> 
> This is another small step on the path to liberating davinci from legacy
> GPIO API calls and shrinking the davinci GPIO driver by not having to
> support the base GPIO number anymore.
> 
> This time we're removing the legacy calls used indirectly by the LCDC
> fbdev driver.
> 
> The first three patches modify the GPIO backlight driver. The first
> of them adds the necessary functionality, the other two are just
> tweaks and cleanups.

Can you take the first three patches for v5.3 - if its not too late? I
think that will make it easy for rest of patches to make into subsequent
kernel releases.

> 
> Next two patches enable the GPIO backlight driver in
> davinci_all_defconfig.
> 
> Patch 6/12 models the backlight GPIO as an actual GPIO backlight device.
> 
> Patches 7-9 extend the fbdev driver with regulator support and convert
> the da850-evm board file to using it.
> 
> Last three patches are improvements to the da8xx fbdev driver since
> we're already touching it in this series.

Thanks,
Sekhar

Re: [PATCH v4 0/5] Support for LEGO MINDSTORMS EV3 LCD display

[Sekhar Nori]

On Monday 07 August 2017 11:09 PM, David Lechner wrote:

>   ARM: dts: da850-lego-ev3: Add node for LCD display
>   ARM: davinci_all_defconfig: enable tinydrm and ST7586

Queuing these two patches (4/5 and 5/5) through davinci tree.

Thanks,
Sekhar
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] ARM: davinci_all_defconfig: enable dumb vga-dac drm bridge

[Sekhar Nori]

On Friday 25 November 2016 02:43 PM, Bartosz Golaszewski wrote:
> This enables the dumb-vga-dac driver by default for davinci boards.
> 
> The driver is needed for tilcdc support on da850-lcdk board.
> 
> Signed-off-by: Bartosz Golaszewski 
> ---
>  arch/arm/configs/davinci_all_defconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/configs/davinci_all_defconfig 
> b/arch/arm/configs/davinci_all_defconfig
> index b5e978f..ab1bf18 100644
> --- a/arch/arm/configs/davinci_all_defconfig
> +++ b/arch/arm/configs/davinci_all_defconfig
> @@ -127,6 +127,8 @@ CONFIG_REGULATOR_FIXED_VOLTAGE=y
>  CONFIG_REGULATOR_TPS6507X=y
>  CONFIG_DRM=m
>  CONFIG_DRM_TILCDC=m
> +CONFIG_DRM_BRIDGE=y

DRM_BRIDGE is a 'def_bool y'. So no need to explicitly enable it. And
actually it will get dropped with the next savedefconfig refresh anyway.

Applying this patch with this line dropped.

Thanks,
Sekhar

[PATCH v7 3/5] drm: bridge: add support for TI ths8135

[Sekhar Nori]

On Tuesday 03 January 2017 10:56 AM, Archit Taneja wrote:
> Hi Sekhar,
> 
> On 1/2/2017 4:38 PM, Sekhar Nori wrote:
>> Hi Archit,
>>
>> On Wednesday 14 December 2016 10:35 AM, Archit Taneja wrote:
>>>
>>>
>>> On 12/13/2016 03:39 PM, Bartosz Golaszewski wrote:
>>>> THS8135 is a configurable video DAC, but no configuration is actually
>>>> necessary to make it work.
>>>>
>>>> For now use the dumb-vga-dac driver to support it.
>>>
>>> Queued to drm-misc-next
>>
>> This patch and 2/5 are not in v4.10 kernel. Did you mean to queue them
>> to v4.10?
> 
> These missed out on 4.10 because the drm related pull requests were
> already sent by then. These 2 patches are queued for 4.11.

Alright, thanks for the update.

Regards,
Sekhar

[PATCH v7 5/5] ARM: dts: da850: specify the maximum pixel clock rate for tilcdc

[Sekhar Nori]

On Tuesday 13 December 2016 03:39 PM, Bartosz Golaszewski wrote:
> At maximum CPU frequency of 300 MHz the maximum pixel clock frequency
> is 37.5 MHz[1]. We must filter out any mode for which the calculated
> pixel clock rate would exceed this value.
> 
> Specify the max-pixelclock property for the display node for
> da850-lcdk.
> 
> [1] 
> http://processors.wiki.ti.com/index.php/OMAP-L1x/C674x/AM1x_LCD_Controller_(LCDC)_Throughput_and_Optimization_Techniques

This wiki page is not really a TI datasheet and can change without
notice. I changed this to refer to
http://www.ti.com/lit/ds/symlink/am1808.pdf (SPRS653E, revised March
2014), table 6-110.

Applied to v4.11/dt

Thanks,
Sekhar

[PATCH v7 4/5] ARM: dts: da850-lcdk: add the vga-bridge node

[Sekhar Nori]

On Wednesday 14 December 2016 03:24 PM, Tomi Valkeinen wrote:
> On 13/12/16 12:09, Bartosz Golaszewski wrote:
>> Add the vga-bridge node to the board DT together with corresponding
>> ports and vga connector. This allows to retrieve the edid info from
>> the display automatically.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> Reviewed-by: Laurent Pinchart 
>> ---
>>  arch/arm/boot/dts/da850-lcdk.dts | 51 
>> 
>>  1 file changed, 51 insertions(+)
> 
> Reviewed-by: Tomi Valkeinen 

Applied to v4.11/dt with Tomi's reviewed-by.

Thanks,
Sekhar

[PATCH v7 3/5] drm: bridge: add support for TI ths8135

[Sekhar Nori]

Hi Archit,

On Wednesday 14 December 2016 10:35 AM, Archit Taneja wrote:
> 
> 
> On 12/13/2016 03:39 PM, Bartosz Golaszewski wrote:
>> THS8135 is a configurable video DAC, but no configuration is actually
>> necessary to make it work.
>>
>> For now use the dumb-vga-dac driver to support it.
> 
> Queued to drm-misc-next

This patch and 2/5 are not in v4.10 kernel. Did you mean to queue them
to v4.10?

Thanks,
Sekhar

[PATCH v7 1/5] ARM: dts: da850: rename the display node label

[Sekhar Nori]

On Wednesday 14 December 2016 03:27 PM, Tomi Valkeinen wrote:
> On 13/12/16 12:09, Bartosz Golaszewski wrote:
>> The tilcdc node name is 'display' as per the ePAPR 1.1 recommendation.
>> The label is also 'display', but change it to 'lcdc' to make it clear
>> what the underlying hardware is.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>>  arch/arm/boot/dts/da850.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index 104155d..6b0ef3d 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -458,7 +458,7 @@
>>  dma-names = "tx", "rx";
>>  };
>>  
>> -display: display at 213000 {
>> +lcdc: display at 213000 {
>>  compatible = "ti,da850-tilcdc";
>>  reg = <0x213000 0x1000>;
>>  interrupts = <52>;
>>
> 
> Reviewed-by: Tomi Valkeinen 

Applied with Tomi's reviewed-by.

Thanks,
Sekhar

[PATCH v3 1/2] ARM: dts: da850-lcdk: add the dumb-vga-dac node

[Sekhar Nori]

On Tuesday 06 December 2016 06:32 PM, Bartosz Golaszewski wrote:
> 2016-12-05 13:49 GMT+01:00 Tomi Valkeinen :
>> On 29/11/16 13:57, Bartosz Golaszewski wrote:
>>> Add the dumb-vga-dac node to the board DT together with corresponding
>>> ports and vga connector. This allows to retrieve the edid info from
>>> the display automatically.
>>>
>>
>> It's a bit difficult to follow this as there's been so many patches
>> going around. But I take the above is the LCDC node in the base da850
>> dtsi file? In that case, what is the display_in supposed to present?
>> It's the first node in the "display chain", so it has no input.
>>
>> Also, don't touch da850.dtsi here, just add the "ports" node in the
>> da850-lcdk.dts file.
>>
>> If the da850.dtsi has not been merged yet, I'd change the name of the
>> lcdc node to something else than "display". It's rather vague. If it's
>> named "lcdc", reading da850-lcdk.dts becomes much easier, as you'll
>> refer to "lcdc".
>>
> 
> The node is already in Sekhar's branch.

The node name should be 'display' as thats the ePAPR 1.1 generic name
recommendation. The label is also set to 'display' though and that can
be changed to lcdc.

A pre-patch to fix that before we modify the node further is welcome.

Thanks,
Sekhar

[PATCH v2 2/2] ARM: dts: da850-lcdk: specify the maximum pixel clock rate for tilcdc

[Sekhar Nori]

On Monday 28 November 2016 05:45 PM, Bartosz Golaszewski wrote:
> Due to memory throughput constraints any display mode for which the
> pixel clock rate exceeds the recommended value of 37500 KHz must be
> filtered out.

I think there might be more reasons than memory throughput constraints
for the reasoning behind 37.5Mhz cap on pixel clock. Why not just refer
to the datasheet section that places this constraint so we know its a
hardware restriction.

> 
> Specify the max-pixelclock property for the display node for
> da850-lcdk.
> 
> Signed-off-by: Bartosz Golaszewski 
> ---
>  arch/arm/boot/dts/da850-lcdk.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/da850-lcdk.dts 
> b/arch/arm/boot/dts/da850-lcdk.dts
> index d864f11..1283263 100644
> --- a/arch/arm/boot/dts/da850-lcdk.dts
> +++ b/arch/arm/boot/dts/da850-lcdk.dts
> @@ -285,6 +285,7 @@
>  
>   {
>   status = "okay";
> + max-pixelclock = <37500>;

Should this not be in da850.dtsi since its an SoC imposed constraint? If
a board needs narrower constraint, it can override it. But I guess most
well designed boards will just hit the SoC constraint.

Thanks,
Sekhar

[PATCH] ARM: dts: da850: specify the maximum bandwidth for tilcdc

[Sekhar Nori]

On Monday 28 November 2016 01:12 PM, Tomi Valkeinen wrote:
> On 28/11/16 07:24, Sekhar Nori wrote:
>> On Friday 25 November 2016 09:07 PM, Bartosz Golaszewski wrote:
>>> It has been determined that the maximum resolution supported correctly
>>> by tilcdc rev1 on da850 SoCs is 800x600 at 60. Due to memory throughput
>>> constraints we must filter out higher modes.
>>>
>>> Specify the max-bandwidth property for the display node for
>>> da850-based boards.
>>>
>>> Signed-off-by: Bartosz Golaszewski 
>>> ---
>>>  arch/arm/boot/dts/da850.dtsi | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index 8e30d9b..9b7c444 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -452,6 +452,7 @@
>>> compatible = "ti,da850-tilcdc";
>>> reg = <0x213000 0x1000>;
>>> interrupts = <52>;
>>> +   max-bandwidth = <2880>;
>>
>> If this is effectively the max pixel clock that the device supports,
>> then why not use the datasheet specified value of 37.5 MHz (Tc = 26.66 ns).
> 
> There's a separate property for max-pixelclock. This one is maximum
> pixels per second (which does sound almost the same), but the doc says
> it's about the particular memory interface + LCDC combination.

DA850 supports both mDDR and DDR2, at slightly different speeds. So
memory bandwidth limitation is also board specific. This should probably
move to board file.

But I would like to know why using max-pixelclock is not good enough.
Have experiments shown that LCDC on DA850 LCDK underflows even if pixel
clock is below the datasheet recommendation?

Thanks,
Sekhar

[PATCH] ARM: dts: da850: enable the memctrl and mstpri nodes per board

[Sekhar Nori]

On Thursday 24 November 2016 03:01 PM, Bartosz Golaszewski wrote:
> Currently the memory controller and master priorities drivers are
> enabled in da850.dtsi. For boards for which there are no settings
> defined, this makes these drivers emit error messages.
> 
> Disable the nodes in da850.dtsi and only enable them for da850-lcdk -
> the only board that currently needs them.
> 
> Signed-off-by: Bartosz Golaszewski 

Applied to v4.10/dt

Thanks,
Sekhar

[PATCH] ARM: dts: da850: specify the maximum bandwidth for tilcdc

[Sekhar Nori]

On Friday 25 November 2016 09:07 PM, Bartosz Golaszewski wrote:
> It has been determined that the maximum resolution supported correctly
> by tilcdc rev1 on da850 SoCs is 800x600 at 60. Due to memory throughput
> constraints we must filter out higher modes.
> 
> Specify the max-bandwidth property for the display node for
> da850-based boards.
> 
> Signed-off-by: Bartosz Golaszewski 
> ---
>  arch/arm/boot/dts/da850.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 8e30d9b..9b7c444 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -452,6 +452,7 @@
>   compatible = "ti,da850-tilcdc";
>   reg = <0x213000 0x1000>;
>   interrupts = <52>;
> + max-bandwidth = <2880>;

If this is effectively the max pixel clock that the device supports,
then why not use the datasheet specified value of 37.5 MHz (Tc = 26.66 ns).

Thanks,
Sekhar

[PATCH v4 0/2] da8xx: fix section mismatch in new drivers

[Sekhar Nori]

On Wednesday 23 November 2016 07:09 PM, Bartosz Golaszewski wrote:
> Sekhar noticed there's a section mismatch in the da8xx-mstpri and
> da8xx-ddrctl drivers. This is caused by calling
> of_flat_dt_get_machine_name() which has an __init annotation.
> 
> This series makes the drivers drop the call and not print the
> machine name in the error message.

Applied both. Thanks!

Regards,
Sekhar

[PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

[Sekhar Nori]

On Thursday 24 November 2016 04:18 AM, David Lechner wrote:
> On 11/23/2016 04:32 PM, Kevin Hilman wrote:
>> David Lechner  writes:
>>
>>> On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
 2016-11-22 23:23 GMT+01:00 David Lechner :
> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>
>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>> controller drivers to da850.dtsi.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>> v1 -> v2:
>> - moved the priority controller node above the cfgchip node
>> - renamed added nodes to better reflect their purpose
>>
>>  arch/arm/boot/dts/da850.dtsi | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi
>> b/arch/arm/boot/dts/da850.dtsi
>> index 1bb1f6d..412eec6 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -210,6 +210,10 @@
>> };
>>
>> };
>> +   prictrl: priority-controller at 14110 {
>> +   compatible = "ti,da850-mstpri";
>> +   reg = <0x14110 0x0c>;
>
>
> I think we should add status = "disabled"; here and let boards opt in.
>
>> +   };
>> cfgchip: chip-controller at 1417c {
>> compatible = "ti,da830-cfgchip", "syscon",
>> "simple-mfd";
>> reg = <0x1417c 0x14>;
>> @@ -451,4 +455,8 @@
>>   1 0 0x6800 0x8000>;
>> status = "disabled";
>> };
>> +   memctrl: memory-controller at b000 {
>> +   compatible = "ti,da850-ddr-controller";
>> +   reg = <0xb000 0xe8>;
>
>
> same here. status = "disabled";
>
>> +   };
>>  };
>>

 Hi David,

 I did that initially[1][2] and it was rejected by Kevin[3] and
 Laurent[4].

 FYI this patch has already been queued by Sekhar.
>>>
>>> Thanks. I did not see those threads.
>>>
>>> FYI to maintainers, having these enabled by default causes error
>>> messages in the kernel log for other boards that are not supported by
>>> the drivers.
>>
>> Then the driver is too noisy and should be cleaned up.
>>
>>> Since there is only one board that is supported and soon
>>> to be 2 that are not, I would rather have this disabled by default to
>>> avoid the error messages.
>>
>> IMO, what exactly are the error messages? Sounds like the driver is
>> being too verbose, and calling things errors that are not really errors.
> 
> It is just one line per driver.
> 
> dev_err(dev, "no master priorities defined for this board\n");
> 
> and
> 
> dev_err(dev, "no settings defined for this board\n");
> 
> 
> Since "ti,da850-lcdk" is the only board supported in these drivers, all
> other boards will see these error messages.

Thats pretty bad. Sorry about that. The original justification for
keeping them enabled all the time was that they are in-SoC modules with
no external dependencies (like IO lines or voltage rails) so they can be
enabled on all boards that use DA850. While that remains true, the
configuration itself is board specific.

I think the error messages are still useful, so instead of silencing
them, I think we should go back to keeping these nodes disabled by
default and enabling only on boards which have support for it in the driver.

Thanks,
Sekhar

[PATCH v3 1/3] bus: da8xx-mstpri: drop the call to of_flat_dt_get_machine_name()

[Sekhar Nori]

Hi Bartosz,

On Wednesday 23 November 2016 04:36 PM, Bartosz Golaszewski wrote:
> In order to avoid a section mismatch use a locally implemented routine
> instead of of_flat_dt_get_machine_name() when printing the error
> message.
> 
> Signed-off-by: Bartosz Golaszewski 
> ---
>  drivers/bus/da8xx-mstpri.c | 23 +--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/da8xx-mstpri.c b/drivers/bus/da8xx-mstpri.c
> index 85f0b53..064eeb9 100644
> --- a/drivers/bus/da8xx-mstpri.c
> +++ b/drivers/bus/da8xx-mstpri.c
> @@ -16,7 +16,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  
>  /*
>   * REVISIT: Linux doesn't have a good framework for the kind of performance
> @@ -190,6 +189,26 @@ static const struct da8xx_mstpri_board_priorities 
> da8xx_mstpri_board_confs[] = {
>   },
>  };
>  
> +/*
> + * FIXME Remove this function once of/base gets a general routine for getting
> + * the machine model/compatible string.
> + */
> +static const char *da8xx_mstpri_machine_get_compatible(void)
> +{
> + struct device_node *root;
> + const char *compatible;
> + int ret = -1;
> +
> + root = of_find_node_by_path("/");
> + if (root) {
> + ret = of_property_read_string_index(root, "compatible",
> + 0, );
> + of_node_put(root);
> + }
> +
> + return ret ? NULL : compatible;
> +}

As I just noted in the thread for v1 of this patch, calling
of_node_put(root) while keeping a reference to its compatible property
for later use sounds really broken.

I think it is safest to fix this by not including the compatible name in
error message at all. The error message will be little less descriptive,
but thats better than adding questionable code.

Thats what Frank suggested first up, but I did not realize at that time
that printing compatible name will be this much effort.

Can you please send a v4?

Thanks,
Sekhar

[PATCH 1/3] of: base: add support to get machine compatible string

[Sekhar Nori]

On Wednesday 23 November 2016 05:37 PM, Sudeep Holla wrote:
>> So, the if(!of_node_get()) is just an expensive NULL pointer check. I
>> think
>> it is better to be explicit about it by not using of_node_get/put() at
>> all.
>> How about:
>>
> 
> Are we planning to use this in any time sensitive paths? Anyways I am
> fine removing them.

Not worried about the time taken as much as it serving as a bad example
and getting carried over to other places where the impact might actually
be real.

> 
>> +int of_machine_get_model_name(const char **model)
>> +{
>> +   int error;
>> +
>> +   if (!of_root)
>> +   return -EINVAL;
>> +
>> +   error = of_property_read_string(of_root, "model", model);
>> +   if (error)
>> +   error = of_property_read_string_index(of_root,
>> "compatible",
>> + 0, model);
>> +   return error;
>> +}
>> +EXPORT_SYMBOL(of_machine_get_model_name);
>>
>> I know the patch is already in -next so I guess it depends on how
>> strongly
>> Rob feels about this.
> 
> Frank expressed his concerns and it may be reverted.

Didn't notice that. I will check that thread. Thanks!

Regards,
Sekhar

[PATCH 1/3] of: base: add support to get machine compatible string

[Sekhar Nori]

On Wednesday 23 November 2016 03:35 PM, Sudeep Holla wrote:
> 
> 
> On 23/11/16 07:49, Sekhar Nori wrote:
>> On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote:
>>> Hi Sekhar,
>>>
>>> On 22/11/16 15:06, Sekhar Nori wrote:
>>>> Hi Sudeep,
>>>>
>>>> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote:
>>>>>
>>>>>
>>>>> On 22/11/16 10:41, Bartosz Golaszewski wrote:
>>>>>> Add a function allowing to retrieve the compatible string of the root
>>>>>> node of the device tree.
>>>>>>
>>>>>
>>>>> Rob has queued [1] and it's in -next today. You can reuse that if you
>>>>> are planning to target this for v4.11 or just use open coding in your
>>>>> driver for v4.10 and target this move for v4.11 to avoid cross tree
>>>>> dependencies as I already mentioned in your previous thread.
>>>>
>>>> I dont have your original patch in my mailbox, but I wonder if
>>>> returning a pointer to property string for a node whose reference has
>>>> already been released is safe to do? Probably not an issue for the root
>>>> node, but still feels counter-intuitive.
>>>>
>>>
>>> I am not sure if I understand the issue here. Are you referring a case
>>> where of_root is freed ?
>>
>> Yes, right, thats what I was hinting at. Since you are giving up the
>> reference to the device node before the function returns, the user can
>> be left with a dangling reference.
>>
> 
> Yes I agree.

So, the if(!of_node_get()) is just an expensive NULL pointer check. I think 
it is better to be explicit about it by not using of_node_get/put() at all. 
How about:

+int of_machine_get_model_name(const char **model)
+{
+   int error;
+
+   if (!of_root)
+   return -EINVAL;
+
+   error = of_property_read_string(of_root, "model", model);
+   if (error)
+   error = of_property_read_string_index(of_root, "compatible",
+ 0, model);
+   return error;
+}
+EXPORT_SYMBOL(of_machine_get_model_name);

I know the patch is already in -next so I guess it depends on how strongly 
Rob feels about this.

Thanks,
Sekhar

[PATCH 1/3] of: base: add support to get machine compatible string

[Sekhar Nori]

On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote:
> Hi Sekhar,
> 
> On 22/11/16 15:06, Sekhar Nori wrote:
>> Hi Sudeep,
>>
>> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote:
>>>
>>>
>>> On 22/11/16 10:41, Bartosz Golaszewski wrote:
>>>> Add a function allowing to retrieve the compatible string of the root
>>>> node of the device tree.
>>>>
>>>
>>> Rob has queued [1] and it's in -next today. You can reuse that if you
>>> are planning to target this for v4.11 or just use open coding in your
>>> driver for v4.10 and target this move for v4.11 to avoid cross tree
>>> dependencies as I already mentioned in your previous thread.
>>
>> I dont have your original patch in my mailbox, but I wonder if
>> returning a pointer to property string for a node whose reference has
>> already been released is safe to do? Probably not an issue for the root
>> node, but still feels counter-intuitive.
>>
> 
> I am not sure if I understand the issue here. Are you referring a case
> where of_root is freed ?

Yes, right, thats what I was hinting at. Since you are giving up the
reference to the device node before the function returns, the user can
be left with a dangling reference.

> Also I have seen drivers today just using this pointer directly, but
> it's better to copy the string(I just saw this done in one case)

Hmm, the reference is given up before the API returns, so I doubt
copying it later is any additional benefit.

I suspect this is a theoretical issue though since root device node is
probably never freed.

Thanks,
Sekhar

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote:
> Please note that the compatible property might contain several strings, not 
> just
> a single string.

So I guess the best thing to do is to use
of_property_read_string_index() and print the sting at index 0.

Thanks,
Sekhar

[PATCH 1/3] of: base: add support to get machine compatible string

[Sekhar Nori]

Hi Sudeep,

On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote:
> 
> 
> On 22/11/16 10:41, Bartosz Golaszewski wrote:
>> Add a function allowing to retrieve the compatible string of the root
>> node of the device tree.
>>
> 
> Rob has queued [1] and it's in -next today. You can reuse that if you
> are planning to target this for v4.11 or just use open coding in your
> driver for v4.10 and target this move for v4.11 to avoid cross tree
> dependencies as I already mentioned in your previous thread.

I dont have your original patch in my mailbox, but I wonder if 
returning a pointer to property string for a node whose reference has 
already been released is safe to do? Probably not an issue for the root 
node, but still feels counter-intuitive.

This is the code for reference:

+int of_machine_get_model_name(const char **model)
+{
+   int error;
+
+   if (!of_node_get(of_root))
+   return -EINVAL;
+
+   error = of_property_read_string(of_root, "model", model);
+   if (error)
+   error = of_property_read_string_index(of_root, "compatible",
+ 0, model);
+   of_node_put(of_root);
+
+   return error;
+}
+EXPORT_SYMBOL(of_machine_get_model_name);

Thanks,
Sekhar

[PATCH 1/3] of: base: add support to get machine compatible string

[Sekhar Nori]

On Tuesday 22 November 2016 04:36 PM, Sudeep Holla wrote:
> 
> 
> On 22/11/16 10:57, Bartosz Golaszewski wrote:
>> 2016-11-22 11:53 GMT+01:00 Sudeep Holla :
>>>
>>>
>>> On 22/11/16 10:41, Bartosz Golaszewski wrote:

 Add a function allowing to retrieve the compatible string of the root
 node of the device tree.

>>>
>>> Rob has queued [1] and it's in -next today. You can reuse that if you
>>> are planning to target this for v4.11 or just use open coding in your
>>> driver for v4.10 and target this move for v4.11 to avoid cross tree
>>> dependencies as I already mentioned in your previous thread.
>>
>> Rob's patch checks the model first - I'm not sure this is the behavior
>> we want here as Sekhar suggested we print the machine compatible.
> 
> IIUC, you are replacing of_flat_dt_get_machine_name and
> of_machine_get_model_name does exactly same. So I don't see any point in
> adding this new function and it's just used for logging purpose.
> Also Sekhar just gave example by using just compatible adding that
> function in the driver itself.
> 
> As Arnd suggested me[1], you should for v4.10 fix it in the driver
> itself to avoid the cross tree dependencies at this point similar to [2]

+1. Bartosz, can you please fix it in the driver for v4.10. If there is
an API available, we can move to it for v4.11

Thanks,
Sekhar

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

Hi Frank,

On Tuesday 22 November 2016 07:13 AM, Frank Rowand wrote:
> On 11/21/16 08:33, Sekhar Nori wrote:
>> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
>>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>> +{
>>> +   const struct da8xx_ddrctl_config_knob *knob;
>>> +   const struct da8xx_ddrctl_setting *setting;
>>> +   struct device_node *node;
>>> +   struct resource *res;
>>> +   void __iomem *ddrctl;
>>> +   struct device *dev;
>>> +   u32 reg;
>>> +
>>> +   dev = >dev;
>>> +   node = dev->of_node;
>>> +
>>> +   setting = da8xx_ddrctl_get_board_settings();
>>> +   if (!setting) {
>>> +   dev_err(dev, "no settings for board '%s'\n",
>>> +   of_flat_dt_get_machine_name());
>>> +   return -EINVAL;
>>> +   }
>>
>> This causes a section mismatch because of_flat_dt_get_machine_name() 
>> has an __init annotation. I did not notice that before, sorry.
>>
>> It can be fixed with a patch like below:
>>
>> ---8<---
>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
>> index a20e7bbbcbe0..9ca5aab3ac54 100644
>> --- a/drivers/memory/da8xx-ddrctl.c
>> +++ b/drivers/memory/da8xx-ddrctl.c
>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
>> *da8xx_ddrctl_get_board_settings(void)
>>  return NULL;
>>  }
>>  
>> +static const char* da8xx_ddrctl_get_machine_name(void)
>> +{
>> +const char *str;
>> +int ret;
>> +
>> +ret = of_property_read_string(of_root, "model", );
>> +if (ret)
>> +ret = of_property_read_string(of_root, "compatible", );
>> +
>> +return str;
>> +}
>> +
>>  static int da8xx_ddrctl_probe(struct platform_device *pdev)
>>  {
>>  const struct da8xx_ddrctl_config_knob *knob;
>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device 
>> *pdev)
>>  setting = da8xx_ddrctl_get_board_settings();
>>  if (!setting) {
>>  dev_err(dev, "no settings for board '%s'\n",
>> -of_flat_dt_get_machine_name());
> 
> da8xx_ddrctl_get_board_settings() tries to match based on the "compatible"
> property in the root node.  The "model" property in the root node has
> nothing to do with the failure to match. So creating and then using
> da8xx_ddrctl_get_machine_name() to potentially report model is not useful.
> 
> It should be sufficient to simply report that no compatible matched.

I agree with you on this. Even if model name is printed, you will have
to go back and check the compatible anyway. But I think it will be
useful to print the compatible instead of just reporting that nothing
matched.

Bartosz, if you agree too, could you send a fix patch just printing the
compatible?

Thanks,
Sekhar

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
> +{
> + const struct da8xx_ddrctl_config_knob *knob;
> + const struct da8xx_ddrctl_setting *setting;
> + struct device_node *node;
> + struct resource *res;
> + void __iomem *ddrctl;
> + struct device *dev;
> + u32 reg;
> +
> + dev = >dev;
> + node = dev->of_node;
> +
> + setting = da8xx_ddrctl_get_board_settings();
> + if (!setting) {
> + dev_err(dev, "no settings for board '%s'\n",
> + of_flat_dt_get_machine_name());
> + return -EINVAL;
> + }

This causes a section mismatch because of_flat_dt_get_machine_name() 
has an __init annotation. I did not notice that before, sorry.

It can be fixed with a patch like below:

---8<---
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
index a20e7bbbcbe0..9ca5aab3ac54 100644
--- a/drivers/memory/da8xx-ddrctl.c
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting 
*da8xx_ddrctl_get_board_settings(void)
return NULL;
 }

+static const char* da8xx_ddrctl_get_machine_name(void)
+{
+   const char *str;
+   int ret;
+
+   ret = of_property_read_string(of_root, "model", );
+   if (ret)
+   ret = of_property_read_string(of_root, "compatible", );
+
+   return str;
+}
+
 static int da8xx_ddrctl_probe(struct platform_device *pdev)
 {
const struct da8xx_ddrctl_config_knob *knob;
@@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
setting = da8xx_ddrctl_get_board_settings();
if (!setting) {
dev_err(dev, "no settings for board '%s'\n",
-   of_flat_dt_get_machine_name());
+   da8xx_ddrctl_get_machine_name());
return -EINVAL;
}
---8<--- 

A similar fix is required for the other driver in this series (patch 
2/5). I need some advise on whether I should introduce a common 
function to get the machine name post kernel boot-up (I cannot see an 
existing one). If yes, any advise on which file it should go into?

Thanks,
Sekhar

[PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

[Sekhar Nori]

On Tuesday 15 November 2016 04:30 PM, Bartosz Golaszewski wrote:
> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
> controller drivers to da850.dtsi.
> 
> Signed-off-by: Bartosz Golaszewski 

Applied to v4.10/dt

Thanks,
Sekhar

[PATCH 2/2] ARM: davinci_all_defconfig: enable the mstpri and ddrctl drivers

[Sekhar Nori]

On Monday 14 November 2016 11:02 PM, Bartosz Golaszewski wrote:
> With the da8xx memory controller and master peripheral priority
> drivers merged and corresponding device tree changes in place we can
> now enable appropriate options by default.
> 
> Signed-off-by: Bartosz Golaszewski 

Applied to v4.10/defconfig

Thanks,
Sekhar

[PATCH 1/2] ARM: dts: da850: add the mstpri and ddrctl nodes

[Sekhar Nori]

On Monday 14 November 2016 11:02 PM, Bartosz Golaszewski wrote:
> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
> controller drivers to da850.dtsi.
> 
> Signed-off-by: Bartosz Golaszewski 
> ---
>  arch/arm/boot/dts/da850.dtsi | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 1bb1f6d..1635218 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -440,6 +440,11 @@
>   interrupts = <52>;
>   status = "disabled";
>   };
> +
> + mstpri: mstpri at 14110 {
> + compatible = "ti,da850-mstpri";
> + reg = <0x14110 0x0c>;
> + };

Instead of adding the node to the end, can you place it above the
cfgchip node to keep it sorted by reg. We have not really followed that
in this file. May be we should have. But lets start with this.

>   };
>   aemif: aemif at 6800 {
>   compatible = "ti,da850-aemif";
> @@ -451,4 +456,8 @@
> 1 0 0x6800 0x8000>;
>   status = "disabled";
>   };
> + ddrctl: ddrctl at b000 {
> + compatible = "ti,da850-ddr-controller";
> + reg = <0xb000 0xe8>;
> + };

Can you name the node memory-controller at b000? Thats the generic name
suggested by ePAPR 1.1 for memory controllers.

I could not find any naming suggestions for the bus master priority
controller above, but based on the pattern used for other controllers,
may be it should be called priority-controller at 14110 instead of mstpri at 
14110

Thanks,
Sekhar

[PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver

[Sekhar Nori]

On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:35PM +0100, Bartosz Golaszewski wrote:
>> Create the driver for the da8xx master peripheral priority
>> configuration and implement support for writing to the three
>> Master Priority registers on da850 SoCs.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>>  .../devicetree/bindings/bus/ti,da850-mstpri.txt|  20 ++
> 
> Acked-by: Rob Herring 

Applied to v4.10/drivers

Thanks,
Sekhar

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:34PM +0100, Bartosz Golaszewski wrote:
>> Create a new driver for the da8xx DDR2/mDDR controller and implement
>> support for writing to the Peripheral Bus Burst Priority Register.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>>  .../memory-controllers/ti-da8xx-ddrctl.txt |  20 +++
> 
> Acked-by: Rob Herring 

Applied to v4.10/drivers

Thanks,
Sekhar

[PATCH v2 0/5] ARM: da850: new drivers for better LCDC support

[Sekhar Nori]

+ Arnd, Olof

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> This series adds two new drivers in order to better support the LCDC
> rev1 present on the da850 boards.
> 
> The first patch adds a new memory driver which allows to write to the
> DDR2/mDDR memory controller present on the da8xx SoCs.
> 
> The second patch adds a new bus driver which allows to interact with
> the MSTPRI registers of the SYSCFG0 module

I think patches 1/5 and 2/5 are ready to be merged. If there are no
objections I would like to send a pull request for them to be merged
through ARM-SoC tree for v4.10 kernel.

Thanks,
Sekhar

> 
> As is mentioned in the comments: we don't want to commit to supporting
> stable interfaces (DT bindings or sysfs attributes) so we hardcode the
> settings required by some boards (for now only da850-lcdk) with the
> hope that linux gets an appropriate framework for performance knobs
> in the future.
> 
> Potential extensions of these drivers should be straightforward in the
> future.
> 
> Subsequent patches add DT nodes for the new drivers: disabled nodes
> in da850.dtsi and enabled in da850-lcdk.dts.
> 
> The last patch adds a workaround for current lack of support for drm
> bridges in tilcdc.
> 
> Tested on a da850-lcdk with a display connected over VGA and two
> additional patches for tilcdc (sent to linux-drm): ran simple modetest
> for supported resolutions, used X.org and fluxbox as graphical
> environment, played video with mplayer.
> 
> v1 -> v2:
> - used regular readl()/writel() instead of __raw_** versions
> - used resource_size() instead of calculating the size by hand
> - used ioremap instead of syscon in patch [2/5]
> - added the DT nodes in patches [3/5]-[5/5]
> 
> Bartosz Golaszewski (5):
>   ARM: memory: da8xx-ddrctl: new driver
>   ARM: bus: da8xx-mstpri: new driver
>   ARM: dts: da850: add the mstpri and ddrctl nodes
>   ARM: dts: da850-lcdk: enable mstpri and ddrctl nodes
>   ARM: dts: da850-lcdk: add tilcdc panel node
> 
>  .../devicetree/bindings/bus/ti,da850-mstpri.txt|  20 ++
>  .../memory-controllers/ti-da8xx-ddrctl.txt |  20 ++
>  arch/arm/boot/dts/da850-lcdk.dts   |  71 ++
>  arch/arm/boot/dts/da850.dtsi   |  11 +
>  drivers/bus/Kconfig|   9 +
>  drivers/bus/Makefile   |   2 +
>  drivers/bus/da8xx-mstpri.c | 269 
> +
>  drivers/memory/Kconfig |   8 +
>  drivers/memory/Makefile|   1 +
>  drivers/memory/da8xx-ddrctl.c  | 175 ++
>  10 files changed, 586 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>  create mode 100644 
> Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>  create mode 100644 drivers/bus/da8xx-mstpri.c
>  create mode 100644 drivers/memory/da8xx-ddrctl.c
> 

[PATCH v2 5/5] ARM: dts: da850-lcdk: add tilcdc panel node

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> The tilcdc driver is not yet ready for working together with the
> dumb-vga-dac drm bridge. While the work on enabling drm_bridge
> support in tilcdc continues, enable the VGA connector on da850-lcdk
> with the following workaround: use the tilcdc-panel driver with
> a set of common (and tested) resolutions.
> 
> Once the drm bridge support is complete, we'll remove the node added
> by this patch and use the correct solution. This change will be
> transparent for the user.
> 
> Signed-off-by: Bartosz Golaszewski 

I dont think this can be applied. If this goes in v4.10, then the DT
blob becomes an ABI and driver will have to support tilcdc-panel driver
for DA850 forever.

Its probably better to wait for the proper driver support to arrive.

Thanks,
Sekhar

[PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> Create the driver for the da8xx master peripheral priority
> configuration and implement support for writing to the three
> Master Priority registers on da850 SoCs.
> 
> Signed-off-by: Bartosz Golaszewski 

Reviewed-by: Sekhar Nori 

Thanks,
Sekhar

[PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver

[Sekhar Nori]

On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote:
> Create a new driver for the da8xx DDR2/mDDR controller and implement
> support for writing to the Peripheral Bus Burst Priority Register.
> 
> Signed-off-by: Bartosz Golaszewski 

Reviewed-by: Sekhar Nori 

Thanks,
Sekhar

[PATCH 2/2] ARM: bus: da8xx-mstpri: new driver

[Sekhar Nori]

On Monday 31 October 2016 03:24 PM, Bartosz Golaszewski wrote:
> 2016-10-31 10:52 GMT+01:00 Sekhar Nori :
>> Hi Bartosz,
>>
>> On Monday 31 October 2016 03:10 PM, Bartosz Golaszewski wrote:
>>> 2016-10-31 5:30 GMT+01:00 Rob Herring :
>>>> On Wed, Oct 26, 2016 at 07:35:55PM +0200, Bartosz Golaszewski wrote:
>>>>> Create the driver for the da8xx master peripheral priority
>>>>> configuration and implement support for writing to the three
>>>>> Master Priority registers on da850 SoCs.
>>>>>
>>>>> Signed-off-by: Bartosz Golaszewski 
>>>>> ---
>>>>>  .../devicetree/bindings/bus/ti,da850-mstpri.txt|  20 ++
>>>>>  drivers/bus/Kconfig|   9 +
>>>>>  drivers/bus/Makefile   |   2 +
>>>>>  drivers/bus/da8xx-mstpri.c | 266 
>>>>> +
>>>>>  4 files changed, 297 insertions(+)
>>>>>  create mode 100644 
>>>>> Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>>>>>  create mode 100644 drivers/bus/da8xx-mstpri.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt 
>>>>> b/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>>>>> new file mode 100644
>>>>> index 000..225af09
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>>>>> @@ -0,0 +1,20 @@
>>>>> +* Device tree bindings for Texas Instruments da8xx master peripheral
>>>>> +  priority driver
>>>>> +
>>>>> +DA8XX SoCs feature a set of registers allowing to change the priority of 
>>>>> all
>>>>> +peripherals classified as masters.
>>>>> +
>>>>> +Documentation:
>>>>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>>>>> +
>>>>> +Required properties:
>>>>> +
>>>>> +- compatible:"ti,da850-mstpri", "syscon" - for da850 
>>>>> based boards
>>>>
>>>> Drop syscon. Doesn't look like it is needed and the example doesn't
>>>> match.
>>>
>>> Hi Rob,
>>>
>>> it is needed: syscon_regmap_lookup_by_compatible() fails without it. I
>>> fixed the example instead.
>>
>> Why are master priority registers under syscon? This driver should be
>> the only entity touching them. So do we need an MFD driver?
>>
> 
> It should, but syscfg0 registers are mapped all over the place. I

Not sure what you mean by this. Yes, the sysconfig module has many
functionalities. But the registers for a given functionality are all
grouped together AFAICS (except CHIPCFGn, see below).

Pinmux registers are part of syscfg module, but don't use syscon.

In the work going on for OHCI support, chipcfg registers are being put
under a syscon node. But that makes sense, I think, because chipcfg
registers are catering to really diverse functionality like PLL and EDMA
related stuff being placed in the same register - CHIPCFG0.

> thought it would be safer to put them under syscon and Kevin agreed.

Before using syscon for the whole syscfg space, I think we need some
analysis as to where the likely races are going to be.

Thanks,
Sekhar

[PATCH 2/2] ARM: bus: da8xx-mstpri: new driver

[Sekhar Nori]

Hi Bartosz,

On Monday 31 October 2016 03:10 PM, Bartosz Golaszewski wrote:
> 2016-10-31 5:30 GMT+01:00 Rob Herring :
>> On Wed, Oct 26, 2016 at 07:35:55PM +0200, Bartosz Golaszewski wrote:
>>> Create the driver for the da8xx master peripheral priority
>>> configuration and implement support for writing to the three
>>> Master Priority registers on da850 SoCs.
>>>
>>> Signed-off-by: Bartosz Golaszewski 
>>> ---
>>>  .../devicetree/bindings/bus/ti,da850-mstpri.txt|  20 ++
>>>  drivers/bus/Kconfig|   9 +
>>>  drivers/bus/Makefile   |   2 +
>>>  drivers/bus/da8xx-mstpri.c | 266 
>>> +
>>>  4 files changed, 297 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>>>  create mode 100644 drivers/bus/da8xx-mstpri.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt 
>>> b/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>>> new file mode 100644
>>> index 000..225af09
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/ti,da850-mstpri.txt
>>> @@ -0,0 +1,20 @@
>>> +* Device tree bindings for Texas Instruments da8xx master peripheral
>>> +  priority driver
>>> +
>>> +DA8XX SoCs feature a set of registers allowing to change the priority of 
>>> all
>>> +peripherals classified as masters.
>>> +
>>> +Documentation:
>>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>>> +
>>> +Required properties:
>>> +
>>> +- compatible:"ti,da850-mstpri", "syscon" - for da850 based 
>>> boards
>>
>> Drop syscon. Doesn't look like it is needed and the example doesn't
>> match.
> 
> Hi Rob,
> 
> it is needed: syscon_regmap_lookup_by_compatible() fails without it. I
> fixed the example instead.

Why are master priority registers under syscon? This driver should be
the only entity touching them. So do we need an MFD driver?

Thanks,
Sekhar

[PATCH 2/3] ARM: bus: da8xx-syscfg: new driver

[Sekhar Nori]

On Friday 21 October 2016 02:55 PM, Tomi Valkeinen wrote:
> On 20/10/16 22:39, Kevin Hilman wrote:
> 
>> However, after our discussion on IRC, we'll respin this without the DT
>> bindings at all.  Next version will just use static configuration data
>> in the drivers/bus driver based on SoC compatible string, since for the
>> use-cases I'm aware of, the settings are boot-time only.
> 
> If it's static boot time config, why not do it in the u-boot?

Hardware initialization dependencies with boot-loader are tough to track
and debug. The bootloader thats currently ships with the board may not
have these settings, for example. This forces everyone to update their
bootloader when shifting to mainline kernel.

Thanks,
Sekhar

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Thursday 20 October 2016 03:51 PM, Tomi Valkeinen wrote:
> On 20/10/16 13:07, Sekhar Nori wrote:
> 
>> Per me, compatible property is an ordered list precisely for the reason
>> that things should continue to "work" with as closely matched driver as
>> possible. So even if someone is running a kernel which does not
>> recognize "ti,da850-tilcdc", it should still be able to probe the driver
>> based on "ti,am33xx-tilcdc" and provide as close to full functionality
>> as possible.
>>
>> That said, I will not insist on keeping it around if Tomi is
>> uncomfortable. And having read the binding documentation accepted by
>> Jyri, it actually says the compatible property should be __one of__
>> "ti,am33xx-tilcdc" or "ti,da850-tilcdc".
> 
> Well, they are just not compatible as far as I know. If the LCDC on
> DA850 would be identified as AM335x LCDC, and used as such, it would not
> work at all. They have different registers, AM335x LCDC has registers
> that do not exist on DA850.
> 
> With our driver it happens to work, because the driver looks at the IP
> revision in the registers, and then decides that this IP is not AM335x
> LCDC even if the dts says so. But I see that as a driver "feature",
> nothing that the .dts can depend on.
> 
> Perhaps it might work the other way around, using DA850 driver on
> AM335x, as DA850 LCDC is a kind of subset of AM335x LCDC. But I'm not
> sure even about that.

Alright, thanks for the detailed explanation. I dropped the "ti,am33xx-
tilcdc" from the list and here is the updated patch I am queuing for
reference.

Thanks,
Sekhar

--8<--
Author: Karl Beldan 
AuthorDate: Wed Oct 5 15:05:32 2016 +0200
Commit: Sekhar Nori 
CommitDate: Thu Oct 20 15:57:21 2016 +0530

ARM: dts: da850: add a node for the LCD controller

Add pins used by the LCD controller and a disabled LCDC node to be
reused in device trees including da850.dtsi.

Signed-off-by: Karl Beldan 
[Bartosz:
  - added the commit description
  - changed the dt node name to a generic one
  - added a da850-specific compatible string
  - removed the tilcdc,panel node
  - moved the pins definitions to da850.dtsi as suggested by
    Sekhar Nori (was in: da850-lcdk.dts)]
Signed-off-by: Bartosz Golaszewski 
[nsekhar at ti.com: fix compatible property and remove interrupt-parent]
Signed-off-by: Sekhar Nori 

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b91c680..901d5c98d5f0 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -186,6 +186,27 @@
0xc 0x 0x
>;
};
+   lcd_pins: pinmux_lcd_pins {
+   pinctrl-single,bits = <
+   /*
+* LCD_D[2], LCD_D[3], LCD_D[4], 
LCD_D[5],
+* LCD_D[6], LCD_D[7]
+*/
+   0x40 0x2200 0xff00
+   /*
+* LCD_D[10], LCD_D[11], LCD_D[12], 
LCD_D[13],
+* LCD_D[14], LCD_D[15], LCD_D[0], 
LCD_D[1]
+*/
+   0x44 0x 0x
+   /* LCD_D[8], LCD_D[9] */
+   0x48 0x0022 0x00ff
+
+   /* LCD_PCLK */
+   0x48 0x0200 0x0f00
+   /* LCD_AC_ENB_CS, LCD_VSYNC, LCD_HSYNC 
*/
+   0x4c 0x0222 0x0fff
+   >;
+   };

};
edma0: edma at 0 {
@@ -399,6 +420,13 @@
< 0 1>;
dma-names = "tx", "rx";
};
+
+   display: display at 213000 {
+   compatible = "ti,da850-tilcdc";
+   reg = <0x213000 0x1000>;
+   interrupts = <52>;
+   status = "disabled";
+   };
};
aemif: aemif at 6800 {
compatible = "ti,da850-aemif";

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 07:31 PM, Bartosz Golaszewski wrote:
> 2016-10-17 14:29 GMT+02:00 Tomi Valkeinen :
>> On 17/10/16 14:40, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Monday 17 Oct 2016 10:33:58 Tomi Valkeinen wrote:
>>>> On 17/10/16 10:12, Sekhar Nori wrote:
>>>>> On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
>>>>>> On 15/10/16 20:42, Sekhar Nori wrote:
>>>>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> index f79e1b9..32908ae 100644
>>>>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>>>>> @@ -399,6 +420,14 @@
>>>>>>>>  < 0 1>;
>>>>>>>>  dma-names = "tx", "rx";
>>>>>>>>  };
>>>>>>>> +
>>>>>>>> +display: display at 213000 {
>>>>>>>> +compatible = "ti,am33xx-tilcdc", 
>>>>>>>> "ti,da850-tilcdc";
>>>>>>>
>>>>>>> This should instead be:
>>>>>>>
>>>>>>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>>>>>>
>>>>>>> as the closest match should appear first in the list.
>>>>>>
>>>>>> Actually I don't think that's correct. The LCDC on da850 is not
>>>>>> compatible with the LCDC on AM335x. I think it should be just
>>>>>> "ti,da850-tilcdc".
>>>>>
>>>>> So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
>>>>> the case, I wonder how the patch passed testing. Bartosz?
>>>>
>>>> AM3 has "version 2" of LCDC, whereas DA850 is v1. They are quite
>>>> similar, but different.
>>>>
>>>> The driver gets the version number from LCDC's register, and acts based
>>>> on that, so afaik the compatible string doesn't really affect the
>>>> functionality (as long as it matches).
>>>>
>>>> But even if it works with the current driver, I don't think
>>>> "ti,am33xx-tilcdc" and "ti,da850-tilcdc" are compatible in the HW level.
>>>
>>> If the hardware provides IP revision information, how about just "ti,lcdc" ?
>>
>> Maybe, and I agree that's the "correct" way, but looking at the history,
>> it's not just once or twice when we've suddenly found out some
>> difference or bug or such in an IP revision, or the integration to a
>> SoC, that can't be found based on the IP revision.
>>
>> That's why I feel it's usually safer to have the SoC revision there in
>> the compatible string.

I agree with Tomi here. Lets keep the soc part number in the compatible
string. More often than not, some undocumented, undiscovered issue pops
up over time. Its much safer to have the SoC revision in there.

>>
>> That said, we have only a few different old SoCs with LCDC (compared to,
>> say, OMAP DSS) so in this case perhaps just "ti,lcdc" would be fine.
>>
>>  Tomi
>>
> 
> I Sekhar is ok with this, I'll send a follow-up patch for that.

Per me, compatible property is an ordered list precisely for the reason
that things should continue to "work" with as closely matched driver as
possible. So even if someone is running a kernel which does not
recognize "ti,da850-tilcdc", it should still be able to probe the driver
based on "ti,am33xx-tilcdc" and provide as close to full functionality
as possible.

That said, I will not insist on keeping it around if Tomi is
uncomfortable. And having read the binding documentation accepted by
Jyri, it actually says the compatible property should be __one of__
"ti,am33xx-tilcdc" or "ti,da850-tilcdc".

Thanks,
Sekhar

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Monday 17 October 2016 11:26 AM, Tomi Valkeinen wrote:
> On 15/10/16 20:42, Sekhar Nori wrote:
> 
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index f79e1b9..32908ae 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>
>>> @@ -399,6 +420,14 @@
>>> < 0 1>;
>>> dma-names = "tx", "rx";
>>> };
>>> +
>>> +   display: display at 213000 {
>>> +   compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";
>>
>> This should instead be:
>>
>> compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";
>>
>> as the closest match should appear first in the list.
> 
> Actually I don't think that's correct. The LCDC on da850 is not
> compatible with the LCDC on AM335x. I think it should be just
> "ti,da850-tilcdc".

So if "ti,am33xx-tilcdc" is used, the display wont work at all? If thats
the case, I wonder how the patch passed testing. Bartosz?

Thanks,
Sekhar

[PATCH 2/2] ARM: dts: da850: add a node for the LCD controller

[Sekhar Nori]

On Wednesday 05 October 2016 06:35 PM, Bartosz Golaszewski wrote:
> From: Karl Beldan 
> 
> Add pins used by the LCD controller and a disabled LCDC node to be
> reused in device trees including da850.dtsi.
> 
> Signed-off-by: Karl Beldan 
> [Bartosz:
>   - added the commit description
>   - changed the dt node name to a generic one
>   - added a da850-specific compatible string
>   - removed the tilcdc,panel node
>   - moved the pins definitions to da850.dtsi as suggested by
> Sekhar Nori (was in: da850-lcdk.dts)]
> Signed-off-by: Bartosz Golaszewski 
> ---
>  arch/arm/boot/dts/da850.dtsi | 29 +
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index f79e1b9..32908ae 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi

> @@ -399,6 +420,14 @@
>   < 0 1>;
>   dma-names = "tx", "rx";
>   };
> +
> + display: display at 213000 {
> + compatible = "ti,am33xx-tilcdc", "ti,da850-tilcdc";

This should instead be:

compatible = "ti,da850-tilcdc", "ti,am33xx-tilcdc";

as the closest match should appear first in the list.

> + reg = <0x213000 0x1000>;
> + interrupt-parent = <>

No need of specifying the interrupt-parent as it is assumed to be that
from the parent node (soc) if left unspecified.

I made these two fixes locally and pushed the two patches in this series
to v4.10/dt branch of my tree (for URL see MAINTAINERS). Can you take a
look and make sure I did not mess anything up?

Regards,
Sekhar

[PATCH v2] drm: tilcdc: add a da850-specific compatible string

[Sekhar Nori]

On Friday 30 September 2016 07:22 PM, Bartosz Golaszewski wrote:
> Due to some potential tweaks for the da850 LCDC (for example: the
> required memory bandwith settings) we need a separate compatible
> for the IP present on the da850 boards.
> 
> Suggested-by: Sekhar Nori 
> Signed-off-by: Bartosz Golaszewski 
> ---
> v1 -> v2:
> - added the new compatible to the bindings documentation
> 
>  Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt | 4 ++--
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt 
> b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> index a83abd7..33b6e8a 100644
> --- a/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> +++ b/Documentation/devicetree/bindings/display/tilcdc/tilcdc.txt
> @@ -1,7 +1,7 @@
>  Device-Tree bindings for tilcdc DRM driver
>  
>  Required properties:
> - - compatible: value should be "ti,am33xx-tilcdc".
> + - compatible: value should be "ti,am33xx-tilcdc" or "ti,da850-tilcdc".

This documentation does not help much :( It should be on the lines of:

compatible: value should be "ti,am33xx-tilcdc" for AM335x based boards
value should be "ti,da850-tilcdc" for DA850/AM18x/OMAP-L138
based boards

There are many existing examples of the way compatible strings are
documented. You can take a look at them too. Also, since you are
introducing a new device-tree compatible, please keep the devicetree
list and maintainers in CC too. I don't think it shows up in
get_maintainer.pl, so you will have to remember to do it.

Thanks,
Sekhar

[PATCH] drm: tilcdc: add a da850-specific compatible string

[Sekhar Nori]

On Friday 30 September 2016 06:30 PM, Bartosz Golaszewski wrote:
> Due to some potential tweaks for the da850 LCDC (for example: the
> required memory bandwith settings) we need a separate compatible
> for the IP present on the da850 boards.
> 
> Suggested-by: Sekhar Nori 
> Signed-off-by: Bartosz Golaszewski 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 1 +

This patch should document the new compatible in
Documentation/devicetree/bindings/..

Thanks,
Sekhar

[PATCH] drm/tilcdc: free disp_clk when CONFIG_CPU_FREQ=n

[Sekhar Nori]

disp_clk is allocated irrespective of CPU_FREQ support
so it must be freed irrespective of it.

Fixes: b478e336b3e7 ("drm/tilcdc: Fix the error path in tilcdc_load()")
Signed-off-by: Sekhar Nori 
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c 
b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 0f283a3b932c..4674ed6c45dc 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -330,8 +330,8 @@ fail_cpufreq_unregister:
cpufreq_unregister_notifier(>freq_transition,
CPUFREQ_TRANSITION_NOTIFIER);
 fail_put_disp_clk:
-   clk_put(priv->disp_clk);
 #endif
+   clk_put(priv->disp_clk);

 fail_put_clk:
clk_put(priv->clk);
-- 
2.4.4.408.g16da57c

[RFC] drm/lcdc: add TI LCD Controller DRM driver

[Sekhar Nori]

Hi Rob,

On Monday, December 17, 2012, Rob Clark wrote:

> On Mon, Dec 17, 2012 at 8:39 AM, Rob Clark  gmail.com>
> wrote:
> >> I'm not very enthusiastic about adding ti-lcdc specific panel/chip
> >> drivers. It's not really a big deal if it's only kernel code, but you
> >> add device-tree bindings also, which is an external API that you need to
> >> support after adding it.
> >>
> >> I'd rather see the energy put to common display framework, and get this
> >> whole panel/chip driver issue solved in a generic manner.
> >
> > yeah, I was expecting to migrate to CDF once it exists, but needed
> > something for now.  I'm using the exercise to get my thoughts straight
> > on how CDF should fit into KMS.  (One thing I plan to add support for
> > is an i2c connected hdmi encoder.. which looks like it would fit well
> > in drivers/gpu/drm/i2c.. so the drm encoder-slave stuff might be the
> > way.)
> >
> > If you have any suggestions on the DT bindings, I'd like to hear 'em.
>
> btw, a little bit of-topic, but speaking of DT...
>
> Anybody have any clue about how backlight devices are supposed to work
> in this brave new DT world?


See Runtime interpreted power sequences here:
 http://lkml.indiana.edu/hypermail/linux/kernel/1208.2/00029.html

It is an attempt to address this need.

Thanks,
Sekhar

 In the old days, the board file would
> stuff a fxn ptr to control backlight in pdata for the driver.  But we
> don't have this any more.  I think I need some way to retrieve the
> 'struct backlight_device' ptr associated with the panel driver, so
> that the appropriate dpms fxn ptrs can enable/disable the backlight.
>
> I'm thinking the dt file should have something that looks roughly like:
>
> /* Settings for CDTech_S035Q01 / LCD3 cape: */
> panel {
> compatible = "lcdc,panel";
> pinctrl-names = "default";
> pinctrl-0 = <_lcd3_cape_lcd_pins>;
> panel-info {
> ac-bias   = <255>;
> ac-bias-intrpt= <0>;
> dma-burst-sz  = <16>;
> bpp   = <16>;
> fdd   = <0x80>;
> tft-alt-mode  = <0>;
> stn-565-mode  = <0>;
> mono-8bit-mode= <0>;
> invert-line-clock = <1>;
> invert-frm-clock  = <1>;
> sync-edge = <0>;
> sync-ctrl = <1>;
> raster-order  = <0>;
> fifo-th   = <0>;
> };
> display-timings {
> native-mode = <>;
> timing0: 320x240 {
> hactive = <320>;
> vactive = <240>;
> hback-porch = <21>;
> hfront-porch= <58>;
> hsync-len   = <47>;
> vback-porch = <11>;
> vfront-porch= <23>;
> vsync-len   = <2>;
> clock-frequency = <800>;
> };
> };
>
> backlight {
> compatible = "tps65217-backlight";
> isel = <1>;
> fdim = <200>;
>
> tps = <>;   /* link to the tps */
> brightness = <100>;
> };
> };
>
> display-timings is based on the of-videomode helpers patch..
> panel-info probably needs to be made to be something more generic, but
> we need something to know how to configure the crtc properly..
>
> but I'm not quite sure what to do with the backlight..
>
> BR,
> -R
>
> ___
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org 
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
-- next part --
An HTML attachment was scrubbed...
URL: 

Re: [RFC] drm/lcdc: add TI LCD Controller DRM driver

[Sekhar Nori]

Hi Rob,

On Monday, December 17, 2012, Rob Clark wrote:

 On Mon, Dec 17, 2012 at 8:39 AM, Rob Clark robdcl...@gmail.comjavascript:;
 wrote:
  I'm not very enthusiastic about adding ti-lcdc specific panel/chip
  drivers. It's not really a big deal if it's only kernel code, but you
  add device-tree bindings also, which is an external API that you need to
  support after adding it.
 
  I'd rather see the energy put to common display framework, and get this
  whole panel/chip driver issue solved in a generic manner.
 
  yeah, I was expecting to migrate to CDF once it exists, but needed
  something for now.  I'm using the exercise to get my thoughts straight
  on how CDF should fit into KMS.  (One thing I plan to add support for
  is an i2c connected hdmi encoder.. which looks like it would fit well
  in drivers/gpu/drm/i2c.. so the drm encoder-slave stuff might be the
  way.)
 
  If you have any suggestions on the DT bindings, I'd like to hear 'em.

 btw, a little bit of-topic, but speaking of DT...

 Anybody have any clue about how backlight devices are supposed to work
 in this brave new DT world?


See Runtime interpreted power sequences here:
 http://lkml.indiana.edu/hypermail/linux/kernel/1208.2/00029.html

It is an attempt to address this need.

Thanks,
Sekhar

 In the old days, the board file would
 stuff a fxn ptr to control backlight in pdata for the driver.  But we
 don't have this any more.  I think I need some way to retrieve the
 'struct backlight_device' ptr associated with the panel driver, so
 that the appropriate dpms fxn ptrs can enable/disable the backlight.

 I'm thinking the dt file should have something that looks roughly like:

 /* Settings for CDTech_S035Q01 / LCD3 cape: */
 panel {
 compatible = lcdc,panel;
 pinctrl-names = default;
 pinctrl-0 = bone_lcd3_cape_lcd_pins;
 panel-info {
 ac-bias   = 255;
 ac-bias-intrpt= 0;
 dma-burst-sz  = 16;
 bpp   = 16;
 fdd   = 0x80;
 tft-alt-mode  = 0;
 stn-565-mode  = 0;
 mono-8bit-mode= 0;
 invert-line-clock = 1;
 invert-frm-clock  = 1;
 sync-edge = 0;
 sync-ctrl = 1;
 raster-order  = 0;
 fifo-th   = 0;
 };
 display-timings {
 native-mode = timing0;
 timing0: 320x240 {
 hactive = 320;
 vactive = 240;
 hback-porch = 21;
 hfront-porch= 58;
 hsync-len   = 47;
 vback-porch = 11;
 vfront-porch= 23;
 vsync-len   = 2;
 clock-frequency = 800;
 };
 };

 backlight {
 compatible = tps65217-backlight;
 isel = 1;
 fdim = 200;

 tps = tps;   /* link to the tps */
 brightness = 100;
 };
 };

 display-timings is based on the of-videomode helpers patch..
 panel-info probably needs to be made to be something more generic, but
 we need something to know how to configure the crtc properly..

 but I'm not quite sure what to do with the backlight..

 BR,
 -R

 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org javascript:;
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel