Re: [PATCH 0/7] Add a DRM driver to support AI Processing Unit (APU)

[Kevin Hilman]

Jeffrey Hugo  writes:

> On 5/17/2023 8:52 AM, Alexandre Bailon wrote:
>> This adds a DRM driver that implements communication between the CPU and an
>> APU. The driver target embedded device that usually run inference using some
>> prebuilt models. The goal is to provide common infrastructure that could be
>> re-used to support many accelerators. Both kernel, userspace and firmware 
>> tries
>> to use standard and existing to leverage the development and maintenance 
>> effort.
>> The series implements two platform drivers, one for simulation and another 
>> one for
>> the mt8183 (compatible with mt8365).
>
> This looks like the 3 existing Accel drivers.  Why is this in DRM?

Yes, this belongs in accel.  I think Alex had some issues around the
infra in accel with device nodes not appearing/opening properly, but
I'll let him comment there.  But either way, the right approach should
be to fix any issues in accel and move it there.

[...]

>>   .../devicetree/bindings/gpu/mtk,apu-drm.yaml  |  38 ++
>>   drivers/gpu/drm/Kconfig   |   2 +
>>   drivers/gpu/drm/Makefile  |   1 +
>>   drivers/gpu/drm/apu/Kconfig   |  22 +
>>   drivers/gpu/drm/apu/Makefile  |  10 +
>>   drivers/gpu/drm/apu/apu_drv.c | 282 +
>>   drivers/gpu/drm/apu/apu_gem.c | 230 +++
>>   drivers/gpu/drm/apu/apu_internal.h| 205 ++
>>   drivers/gpu/drm/apu/apu_sched.c   | 592 ++
>>   drivers/gpu/drm/apu/simu_apu.c| 313 +
>>   include/uapi/drm/apu_drm.h|  81 +++
>
> "apu" seems too generic.  We already have 3 "AI processing units" over 
> in drivers/accel already...

Indeed, it is generic, but that's kind of the point for this driver
since it's targetted at generalizing the interface with "AI processing
units" on a growing number of embedded SoCs (ARM, RISC-V, etc.)  In
addition, the generic naming is intentional because the goal is bigger
than the kernel and is working towards a generic, shared "libAPU"
userspace[1], but also common firmware for DSP-style inference engines
(e.g. analgous Sound Open Firmware for audio DSPs.)

As usual, the various SoC vendors use different names (APU, NPU, NN
unit, etc.)  but we'd like a generic name for the class of devices
targetted by this driver.  And unfortunately, it looks like the equally
generic "Versatile processing unit" is already taken Intel's
drivers/accel/ivpu. :)

Maybe since this is more about generalizing the interface between the
CPU running linux and the APU, what about the name apu_if?  But I guess
that applies to the other 3 drivers in drivers/accell also.  Hmmm...

Naming things is hard[2], so we're definitly open to other ideas.  Any
suggestions?

Kevin

[1] https://gitlab.baylibre.com/baylibre/libapu/libapu

[2]
"There are 2 hard problems in computer science: cache invalidation,
 naming things and off-by-1 errors."
 -- https://twitter.com/secretGeek/status/7269997868

Re: [PATCH 14/17] ARM: omap1: remove dead code

[Kevin Hilman]

Arnd Bergmann  writes:

> From: Arnd Bergmann 
>
> After the removal of the unused board files, I went through the
> omap1 code to look for code that no longer has any callers
> and remove that.
>
> In particular, support for the omap7xx/omap8xx family is now
> completely unused, so I'm only leaving omap15xx/omap16xx/omap59xx.

Acked-by: Kevin Hilman 

with a few tears shed since omap7xx/omap8xx was the first family I
contributed to upstream. :(

Kevin

Re: New subsystem for acceleration devices

[Kevin Hilman]

Hi Oded (and sorry I misspelled your name last time),

Oded Gabbay  writes:

> On Tue, Aug 23, 2022 at 9:24 PM Kevin Hilman  wrote:
>>
>> Hi Obed,
>>
>> Oded Gabbay  writes:
>>
>> [...]
>>
>> > I want to update that I'm currently in discussions with Dave to figure
>> > out what's the best way to move forward. We are writing it down to do
>> > a proper comparison between the two paths (new accel subsystem or
>> > using drm). I guess it will take a week or so.
>>
>> Any update on the discussions with Dave? and/or are there any plans to
>> discuss this further at LPC/ksummit yet?
> Hi Kevin.
>
> We are still discussing the details, as at least the habanalabs driver
> is very complex and there are multiple parts that I need to see if and
> how they can be mapped to drm.
> Some of us will attend LPC so we will probably take advantage of that
> to talk more about this.

OK, looking forward to some more conversations at LPC.

>>
>> We (BayLibre) are upstreaming support for APUs on Mediatek SoCs, and are
>> using the DRM-based approach.  I'll also be at LPC and happy to discuss
>> in person.
>>
>> For some context on my/our interest: back in Sept 2020 we initially
>> submitted an rpmesg based driver for kernel communication[1].  After
>> review comments, we rewrote that based on DRM[2] and are now using it
>> for some MTK SoCs[3] and supporting our MTK customers with it.
>>
>> Hopefully we will get the kernel interfaces sorted out soon, but next,
>> there's the userspace side of things.  To that end, we're also working
>> on libAPU, a common, open userspace stack.  Alex Bailon recently
>> presented a proposal earlier this year at Embedded Recipes in Paris
>> (video[4], slides[5].)
>>
>> libAPU would include abstractions of the kernel interfaces for DRM
>> (using libdrm), remoteproc/rpmsg, virtio etc. but also goes farther and
>> proposes an open firmware for the accelerator side using
>> libMetal/OpenAMP + rpmsg for communication with (most likely closed
>> source) vendor firmware.  Think of this like sound open firmware (SOF[6]),
>> but for accelerators.
>
> I think your device and the habana device are very different in
> nature, and it is part of what Dave and I discussed, whether these two
> classes of devices can live together. I guess they can live together
> in the kernel, but in the userspace, not so much imo.

Yeah, for now I think focusing on how to handle both classes of devices
in the kernel is the most important.

> The first class is the edge inference devices (usually as part of some
> SoC). I think your description of the APU on MTK SoC is a classic
> example of such a device.

Correct.

> You usually have some firmware you load, you give it a graph and
> pointers for input and output and then you just execute the graph
> again and again to perform inference and just replace the inputs.
>
> The second class is the data-center, training accelerators, which
> habana's gaudi device is classified as such. These devices usually
> have a number of different compute engines, a fabric for scaling out,
> on-device memory, internal MMUs and RAS monitoring requirements. Those
> devices are usually operated via command queues, either through their
> kernel driver or directly from user-space. They have multiple APIs for
> memory management, RAS, scaling-out and command-submissions.

OK, I see.

>>
>> We've been using this succesfully for Mediatek SoCs (which have a
>> Cadence VP6 APU) and have submitted/published the code, including the
>> OpenAMP[7] and libmetal[8] parts in addition to the kernel parts already
>> mentioned.
> What's the difference between libmetal and other open-source low-level
> runtime drivers, such as oneAPI level-zero ?

TBH, I'd never heard of oneAPI before, so I'm assuming it's mainly
focused in the data center.  libmetal/openAMP are widely used
in the consumer, industrial embedded space, and heavily used by FPGAs in
many market segments.

> Currently we have our own runtime driver which is tightly coupled with
> our h/w. For example, the method the userspace "talks" to the
> data-plane firmware is very proprietary as it is hard-wired into the
> architecture of the entire ASIC and how it performs deep-learning
> training. Therefore, I don't see how this can be shared with other
> vendors. Not because of secrecy but because it is simply not relevant
> to any other ASIC.

OK, makes sense.

Thanks for clarifying your use case in more detail.

Kevin

Re: New subsystem for acceleration devices

[Kevin Hilman]

Hi Obed,

Oded Gabbay  writes:

[...]

> I want to update that I'm currently in discussions with Dave to figure
> out what's the best way to move forward. We are writing it down to do
> a proper comparison between the two paths (new accel subsystem or
> using drm). I guess it will take a week or so.

Any update on the discussions with Dave? and/or are there any plans to
discuss this further at LPC/ksummit yet?

We (BayLibre) are upstreaming support for APUs on Mediatek SoCs, and are
using the DRM-based approach.  I'll also be at LPC and happy to discuss
in person.

For some context on my/our interest: back in Sept 2020 we initially
submitted an rpmesg based driver for kernel communication[1].  After
review comments, we rewrote that based on DRM[2] and are now using it
for some MTK SoCs[3] and supporting our MTK customers with it.

Hopefully we will get the kernel interfaces sorted out soon, but next,
there's the userspace side of things.  To that end, we're also working
on libAPU, a common, open userspace stack.  Alex Bailon recently
presented a proposal earlier this year at Embedded Recipes in Paris
(video[4], slides[5].)

libAPU would include abstractions of the kernel interfaces for DRM
(using libdrm), remoteproc/rpmsg, virtio etc. but also goes farther and
proposes an open firmware for the accelerator side using
libMetal/OpenAMP + rpmsg for communication with (most likely closed
source) vendor firmware.  Think of this like sound open firmware (SOF[6]),
but for accelerators.

We've been using this succesfully for Mediatek SoCs (which have a
Cadence VP6 APU) and have submitted/published the code, including the
OpenAMP[7] and libmetal[8] parts in addition to the kernel parts already
mentioned.

We're to the point where we're pretty happy with how this works for MTK
SoCs, and wanting to collaborate with folks working on other platforms
and to see what's needed to support other kinds of accelerators with a
common userspace and open firmware infrastructure.

Kevin

[1] https://lore.kernel.org/r/20200930115350.5272-1-abai...@baylibre.com
[2] https://lore.kernel.org/r/20210917125945.620097-1-abai...@baylibre.com
[3] https://lore.kernel.org/r/20210819151340.741565-1-abai...@baylibre.com
[4] https://www.youtube.com/watch?v=Uj1FZoF8MMw=18211s
[5] https://embedded-recipes.org/2022/wp-content/uploads/2022/06/bailon.pdf
[6] https://www.sofproject.org/
[7] https://github.com/BayLibre/open-amp/tree/v2021.10-mtk 
[8] https://github.com/BayLibre/libmetal/tree/v2021.10-mtk 

Re: [PATCH v3 1/2] drm/panel: panel-boe-tv101wum-nl6: tune the power sequence to avoid leakage

[Kevin Hilman]

Jitao Shi  writes:

> "auo,kd101n80-45na" 2st LCD SPEC update, need to modify the timing
> between IOVCC and mipi data.
> The 2st version of SPEC modifies the timing requirements from IOVCC to
> Mipi Data. IOVCC is now required to take precedence over MIPI DATA,
> otherwise there is a risk of leakage. It is recommended that the time
> for MIPI to enter LP11 be postponed after IOVCC (delay20ms).

Similar to what Daniel said on v2:  You're changing the behavior of
*all* users of this panel driver with this patch, in order to fix a
single user (in the next patch.)

> Signed-off-by: Jitao Shi 
> Change-Id: Ic5212e2145a7dbf2efef9e5585904a93e1bc5a28

Please drop gerrit IDs from upstream submissions.

Kevin

> ---
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 88 
> +++---
>  include/drm/panel_boe_tv101wum_nl6.h   | 28 
>  2 files changed, 94 insertions(+), 22 deletions(-)
>  create mode 100644 include/drm/panel_boe_tv101wum_nl6.h
>
> diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c 
> b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> index db9d0b86d542..02efee06c430 100644
> --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
> @@ -49,7 +49,7 @@ struct boe_panel {
>   struct regulator *avee;
>   struct regulator *avdd;
>   struct gpio_desc *enable_gpio;
> -
> + int powered_refcnt;
>   bool prepared;
>  };
>  
> @@ -488,19 +488,15 @@ static int boe_panel_enter_sleep_mode(struct boe_panel 
> *boe)
>   return 0;
>  }
>  
> -static int boe_panel_unprepare(struct drm_panel *panel)
> +static int boe_panel_power_off(struct drm_panel *panel)
>  {
>   struct boe_panel *boe = to_boe_panel(panel);
> - int ret;
>  
> - if (!boe->prepared)
> - return 0;
> + if (WARN_ON(boe->powered_refcnt == 0))
> + return -EINVAL;
>  
> - ret = boe_panel_enter_sleep_mode(boe);
> - if (ret < 0) {
> - dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> - return ret;
> - }
> + if (--boe->powered_refcnt != 0)
> + return 0;
>  
>   msleep(150);
>  
> @@ -520,17 +516,45 @@ static int boe_panel_unprepare(struct drm_panel *panel)
>   regulator_disable(boe->pp1800);
>   }
>  
> + return 0;
> +}
> +
> +int panel_unprepare_power(struct drm_panel *panel)
> +{
> + if (of_device_is_compatible(panel->dev->of_node, "auo,kd101n80-45na"))
> + return boe_panel_power_off(panel);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(panel_unprepare_power);
> +
> +static int boe_panel_unprepare(struct drm_panel *panel)
> +{
> + struct boe_panel *boe = to_boe_panel(panel);
> + int ret;
> +
> + if (!boe->prepared)
> + return 0;
> +
> + ret = boe_panel_enter_sleep_mode(boe);
> + if (ret < 0) {
> + dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> + return ret;
> + }
> +
> + boe_panel_power_off(panel);
> +
>   boe->prepared = false;
>  
>   return 0;
>  }
>  
> -static int boe_panel_prepare(struct drm_panel *panel)
> +static int boe_panel_power_on(struct drm_panel *panel)
>  {
>   struct boe_panel *boe = to_boe_panel(panel);
>   int ret;
>  
> - if (boe->prepared)
> + if (++boe->powered_refcnt != 1)
>   return 0;
>  
>   gpiod_set_value(boe->enable_gpio, 0);
> @@ -558,18 +582,8 @@ static int boe_panel_prepare(struct drm_panel *panel)
>   gpiod_set_value(boe->enable_gpio, 1);
>   usleep_range(6000, 1);
>  
> - ret = boe_panel_init_dcs_cmd(boe);
> - if (ret < 0) {
> - dev_err(panel->dev, "failed to init panel: %d\n", ret);
> - goto poweroff;
> - }
> -
> - boe->prepared = true;
> -
>   return 0;
>  
> -poweroff:
> - regulator_disable(boe->avee);
>  poweroffavdd:
>   regulator_disable(boe->avdd);
>  poweroff1v8:
> @@ -580,6 +594,36 @@ static int boe_panel_prepare(struct drm_panel *panel)
>   return ret;
>  }
>  
> +int panel_prepare_power(struct drm_panel *panel)
> +{
> + if (of_device_is_compatible(panel->dev->of_node, "auo,kd101n80-45na"))
> + return boe_panel_power_on(panel);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(panel_prepare_power);
> +
> +static int boe_panel_prepare(struct drm_panel *panel)
> +{
> + struct boe_panel *boe = to_boe_panel(panel);
> + int ret;
> +
> + boe_panel_power_on(panel);
> +
> + if (boe->prepared)
> + return 0;
> +
> + ret = boe_panel_init_dcs_cmd(boe);
> + if (ret < 0) {
> + dev_err(panel->dev, "failed to init panel: %d\n", ret);
> + return ret;
> + }
> +
> + boe->prepared = true;
> +
> + return 0;
> +}
> +
>  static int boe_panel_enable(struct drm_panel *panel)
>  {
>   msleep(130);
> diff --git a/include/drm/panel_boe_tv101wum_nl6.h 
> b/include/drm/panel_boe_tv101wum_nl6.h
> new file mode 100644
> index 

Re: [PATCH] drm: meson_drv add shutdown function

[Kevin Hilman]

Neil Armstrong  writes:

> On 09/03/2021 18:13, Kevin Hilman wrote:
>> Hi Neil,
>> 
>> Neil Armstrong  writes:
>> 
>>> On 02/03/2021 05:22, Artem Lapkin wrote:
>>>> Problem: random stucks on reboot stage about 1/20 stuck/reboots
>>>> // debug kernel log
>>>> [4.496660] reboot: kernel restart prepare CMD:(null)
>>>> [4.498114] meson_ee_pwrc c883c000.system-controller:power-controller: 
>>>> shutdown begin
>>>> [4.503949] meson_ee_pwrc c883c000.system-controller:power-controller: 
>>>> shutdown domain 0:VPU...
>>>> ...STUCK...
>>>>
>>>> Solution: add shutdown function to meson_drm driver 
>>>> // debug kernel log
>>>> [5.231896] reboot: kernel restart prepare CMD:(null)
>>>> [5.246135] [drm:meson_drv_shutdown]
>>>> ...
>>>> [5.259271] meson_ee_pwrc c883c000.system-controller:power-controller: 
>>>> shutdown begin
>>>> [5.274688] meson_ee_pwrc c883c000.system-controller:power-controller: 
>>>> shutdown domain 0:VPU...
>>>> [5.338331] reboot: Restarting system
>>>> [5.358293] psci: PSCI_0_2_FN_SYSTEM_RESET reboot_mode:0 cmd:(null)
>>>> bl31 reboot reason: 0xd
>>>> bl31 reboot reason: 0x0
>>>> system cmd  1.
>>>> ...REBOOT...
>>>>
>>>> Tested: on VIM1 VIM2 VIM3 VIM3L khadas sbcs - 1000+ successful reboots
>>>> and Odroid boards, WeTek Play2 (GXBB)
>>>>
>>>> Tested-by: Christian Hewitt 
>>>> Signed-off-by: Artem Lapkin 
>>>>
>>>> ---
>>>>  drivers/gpu/drm/meson/meson_drv.c | 11 +++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/meson/meson_drv.c 
>>>> b/drivers/gpu/drm/meson/meson_drv.c
>>>> index 42c5d3246..693bb1293 100644
>>>> --- a/drivers/gpu/drm/meson/meson_drv.c
>>>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>>>> @@ -482,6 +482,16 @@ static int meson_probe_remote(struct platform_device 
>>>> *pdev,
>>>>return count;
>>>>  }
>>>>  
>>>> +static void meson_drv_shutdown(struct platform_device *pdev)
>>>> +{
>>>> +  struct meson_drm *priv = dev_get_drvdata(>dev);
>>>> +  struct drm_device *drm = priv->drm;
>>>> +
>>>> +  DRM_DEBUG_DRIVER("\n");
>>>> +  drm_kms_helper_poll_fini(drm);
>>>> +  drm_atomic_helper_shutdown(drm);
>>>> +}
>>>> +
>>>>  static int meson_drv_probe(struct platform_device *pdev)
>>>>  {
>>>>struct component_match *match = NULL;
>>>> @@ -553,6 +563,7 @@ static const struct dev_pm_ops meson_drv_pm_ops = {
>>>>  
>>>>  static struct platform_driver meson_drm_platform_driver = {
>>>>.probe  = meson_drv_probe,
>>>> +  .shutdown   = meson_drv_shutdown,
>>>>.driver = {
>>>>.name   = "meson-drm",
>>>>.of_match_table = dt_match,
>>>>
>>>
>>> Applied to drm-misc-fixes,
>> 
>> Could you submit this as a fix to stable (v5.10+) also?
>
> It should be done automagically since I added the Fixes tag

Oh, I didn't see the fixes tag, sorry.

Thanks!

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

Re: [PATCH] drm: meson_drv add shutdown function

[Kevin Hilman]

Hi Neil,

Neil Armstrong  writes:

> On 02/03/2021 05:22, Artem Lapkin wrote:
>> Problem: random stucks on reboot stage about 1/20 stuck/reboots
>> // debug kernel log
>> [4.496660] reboot: kernel restart prepare CMD:(null)
>> [4.498114] meson_ee_pwrc c883c000.system-controller:power-controller: 
>> shutdown begin
>> [4.503949] meson_ee_pwrc c883c000.system-controller:power-controller: 
>> shutdown domain 0:VPU...
>> ...STUCK...
>> 
>> Solution: add shutdown function to meson_drm driver 
>> // debug kernel log
>> [5.231896] reboot: kernel restart prepare CMD:(null)
>> [5.246135] [drm:meson_drv_shutdown]
>> ...
>> [5.259271] meson_ee_pwrc c883c000.system-controller:power-controller: 
>> shutdown begin
>> [5.274688] meson_ee_pwrc c883c000.system-controller:power-controller: 
>> shutdown domain 0:VPU...
>> [5.338331] reboot: Restarting system
>> [5.358293] psci: PSCI_0_2_FN_SYSTEM_RESET reboot_mode:0 cmd:(null)
>> bl31 reboot reason: 0xd
>> bl31 reboot reason: 0x0
>> system cmd  1.
>> ...REBOOT...
>> 
>> Tested: on VIM1 VIM2 VIM3 VIM3L khadas sbcs - 1000+ successful reboots
>> and Odroid boards, WeTek Play2 (GXBB)
>> 
>> Tested-by: Christian Hewitt 
>> Signed-off-by: Artem Lapkin 
>> 
>> ---
>>  drivers/gpu/drm/meson/meson_drv.c | 11 +++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/meson/meson_drv.c 
>> b/drivers/gpu/drm/meson/meson_drv.c
>> index 42c5d3246..693bb1293 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.c
>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>> @@ -482,6 +482,16 @@ static int meson_probe_remote(struct platform_device 
>> *pdev,
>>  return count;
>>  }
>>  
>> +static void meson_drv_shutdown(struct platform_device *pdev)
>> +{
>> +struct meson_drm *priv = dev_get_drvdata(>dev);
>> +struct drm_device *drm = priv->drm;
>> +
>> +DRM_DEBUG_DRIVER("\n");
>> +drm_kms_helper_poll_fini(drm);
>> +drm_atomic_helper_shutdown(drm);
>> +}
>> +
>>  static int meson_drv_probe(struct platform_device *pdev)
>>  {
>>  struct component_match *match = NULL;
>> @@ -553,6 +563,7 @@ static const struct dev_pm_ops meson_drv_pm_ops = {
>>  
>>  static struct platform_driver meson_drm_platform_driver = {
>>  .probe  = meson_drv_probe,
>> +.shutdown   = meson_drv_shutdown,
>>  .driver = {
>>  .name   = "meson-drm",
>>  .of_match_table = dt_match,
>> 
>
> Applied to drm-misc-fixes,

Could you submit this as a fix to stable (v5.10+) also?

Thanks,

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

Re: [PATCH] drm: meson_drv add shutdown function

[Kevin Hilman]

Artem Lapkin  writes:

> Problem: random stucks on reboot stage about 1/20 stuck/reboots
> // debug kernel log
> [4.496660] reboot: kernel restart prepare CMD:(null)
> [4.498114] meson_ee_pwrc c883c000.system-controller:power-controller: 
> shutdown begin
> [4.503949] meson_ee_pwrc c883c000.system-controller:power-controller: 
> shutdown domain 0:VPU...
> ...STUCK...
>
> Solution: add shutdown function to meson_drm driver 
> // debug kernel log
> [5.231896] reboot: kernel restart prepare CMD:(null)
> [5.246135] [drm:meson_drv_shutdown]
> ...
> [5.259271] meson_ee_pwrc c883c000.system-controller:power-controller: 
> shutdown begin
> [5.274688] meson_ee_pwrc c883c000.system-controller:power-controller: 
> shutdown domain 0:VPU...
> [5.338331] reboot: Restarting system
> [5.358293] psci: PSCI_0_2_FN_SYSTEM_RESET reboot_mode:0 cmd:(null)
> bl31 reboot reason: 0xd
> bl31 reboot reason: 0x0
> system cmd  1.
> ...REBOOT...
>
> Tested: on VIM1 VIM2 VIM3 VIM3L khadas sbcs - 1000+ successful reboots
> and Odroid boards, WeTek Play2 (GXBB)
>
> Tested-by: Christian Hewitt 
> Signed-off-by: Artem Lapkin 

Acked-by: Kevin Hilman 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 0/6] drm/meson: add support for Amlogic Video FBC

[Kevin Hilman]

Neil Armstrong  writes:

> Amlogic uses a proprietary lossless image compression protocol and format
> for their hardware video codec accelerators, either video decoders or
> video input encoders.
>
> It considerably reduces memory bandwidth while writing and reading
> frames in memory.
>
> The underlying storage is considered to be 3 components, 8bit or 10-bit
> per component, YCbCr 420, single plane :
> - DRM_FORMAT_YUV420_8BIT
> - DRM_FORMAT_YUV420_10BIT
>
> This modifier will be notably added to DMA-BUF frames imported from the V4L2
> Amlogic VDEC decoder.
>
> At least two layout are supported :
> - Basic: composed of a body and a header
> - Scatter: the buffer is filled with a IOMMU scatter table referring
>   to the encoder current memory layout. This mode if more efficient in terms
>   of memory allocation but frames are not dumpable and only valid during until
>   the buffer is freed and back in control of the encoder
>
> At least two options are supported :
> - Memory saving: when the pixel bpp is 8b, the size of the superblock can
>   be reduced, thus saving memory.
>
> This serie adds the missing register, updated the FBC decoder registers
> content to be committed by the crtc code.
>
> The Amlogic FBC has been tested with compressed content from the Amlogic
> HW VP9 decoder on S905X (GXL), S905D2 (G12A) and S905X3 (SM1) in 8bit
> (Scatter+Mem Saving on G12A/SM1, Mem Saving on GXL) and 10bit
> (Scatter on G12A/SM1, default on GXL).
>
> It's expected to work as-is on GXM and G12B SoCs.

Reviewed-by: Kevin Hilman 

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

Re: [PATCH v3 0/7] drm/meson: add support for Amlogic Video FBC

[Kevin Hilman]

Neil Armstrong  writes:

> Amlogic uses a proprietary lossless image compression protocol and format
> for their hardware video codec accelerators, either video decoders or
> video input encoders.
>
> It considerably reduces memory bandwidth while writing and reading
> frames in memory.
>
> The underlying storage is considered to be 3 components, 8bit or 10-bit
> per component, YCbCr 420, single plane :
> - DRM_FORMAT_YUV420_8BIT
> - DRM_FORMAT_YUV420_10BIT
>
> This modifier will be notably added to DMA-BUF frames imported from the V4L2
> Amlogic VDEC decoder.
>
> At least two layout are supported :
> - Basic: composed of a body and a header
> - Scatter: the buffer is filled with a IOMMU scatter table referring
>   to the encoder current memory layout. This mode if more efficient in terms
>   of memory allocation but frames are not dumpable and only valid during until
>   the buffer is freed and back in control of the encoder
>
> At least two options are supported :
> - Memory saving: when the pixel bpp is 8b, the size of the superblock can
>   be reduced, thus saving memory.
>
> This serie adds the missing register, updated the FBC decoder registers
> content to be committed by the crtc code.
>
> The Amlogic FBC has been tested with compressed content from the Amlogic
> HW VP9 decoder on S905X (GXL), S905D2 (G12A) and S905X3 (SM1) in 8bit
> (Scatter+Mem Saving on G12A/SM1, Mem Saving on GXL) and 10bit
> (Scatter on G12A/SM1, default on GXL).

Tested on meson-sm1-sei610 (VP9 60fps content).

Tested-by: Kevin Hilman 

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

Re: [PATCH 0/4] drm/meson: add support for Amlogic Video FBC

[Kevin Hilman]

Neil Armstrong  writes:

> Amlogic uses a proprietary lossless image compression protocol and format
> for their hardware video codec accelerators, either video decoders or
> video input encoders.
>
> It considerably reduces memory bandwidth while writing and reading
> frames in memory.
>
> The underlying storage is considered to be 3 components, 8bit or 10-bit
> per component, YCbCr 420, single plane :
> - DRM_FORMAT_YUV420_8BIT
> - DRM_FORMAT_YUV420_10BIT
>
> This modifier will be notably added to DMA-BUF frames imported from the V4L2
> Amlogic VDEC decoder.
>
> At least two options are supported :
> - Scatter mode: the buffer is filled with a IOMMU scatter table referring
>   to the encoder current memory layout. This mode if more efficient in terms
>   of memory allocation but frames are not dumpable and only valid during until
>   the buffer is freed and back in control of the encoder
> - Memory saving: when the pixel bpp is 8b, the size of the superblock can
>   be reduced, thus saving memory.
>
> This serie adds the missing register, updated the FBC decoder registers
> content to be committed by the crtc code.
>
> The Amlogic FBC has been tested with compressed content from the Amlogic
> HW VP9 decoder on S905X (GXL), S905D2 (G12A) and S905X3 (SM1) in 8bit
> (Scatter+Mem Saving on G12A/SM1, Mem Saving on GXL) and 10bit
> (Scatter on G12A/SM1, default on GXL).
>
> It's expected to work as-is on GXM and G12B SoCs.

Tested on meson-sm1-sei610 with 4k@60p (VP9) streams.

Tested-by: Kevin Hilman 

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

Re: [PATCH v3 0/9] drm/meson: add AFBC support

[Kevin Hilman]

Neil Armstrong  writes:

> This adds support for the ARM Framebuffer Compression decoders found
> in the Amlogic GXM and G12A SoCs.
>
> This patchset is a merge of v2 "drm/meson: add AFBC support" at [3] and v2
> "drm/meson: implement RDMA for AFBC reset on vsync" at [4].

Oops, replied to the wrong series earlier...

> The VPU embeds a "Register DMA" that can write a sequence of registers
> on the VPU AHB bus, either manually or triggered by an internal IRQ
> event like VSYNC or a line input counter.
>
> The Amlogic GXM and G12A AFBC decoder are totally different, the GXM only
> handling only the AFBC v1.0 modes and the G12A decoder handling the
> AFBC v1.2 modes.
>
> The G12A AFBC decoder is an external IP integrated in the video pipeline,
> and the GXM AFBC decoder seems to the an Amlogic custom decoder more
> tighly integrated in the video pipeline.
>
> The GXM AFBC decoder can handle only one AFBC plane for 2 available
> OSD planes available in HW, and the G12A AFBC decoder can handle up
> to 4 AFBC planes for up to 3 OSD planes available in HW.
>
> The Amlogic GXM supports 16x16 SPARSE and 16x16 SPLIT AFBC buffers up
> to 4k.
>
> On the other side, for G12A SPLIT is mandatory in 16x16 block mode, but
> for 4k modes 32x8+SPLIT AFBC buffers is manadatory for performances reasons.
>
> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
>
> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
>
> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> feeding the OSD1 VIU pixel input.
> This uses a weird "0x100" internal HW physical address on both
> sides to transfer the pixels.
>
> For Amlogic GXM, the supported pixel formats are the same as the normal
> linear OSD1 mode.
>
> On the other side, Amlogic added support for all AFBC v1.2 formats for
> the G12A AFBC integration.
>
> The initial RDMA implementation handles a single channel (over 8), triggered
> by the VSYNC irq and does not handle the RDMA irq.
>
> The RDMA will be usefull to reset and program the AFBC decoder unit
> on each vsync without involving the interrupt handler that can
> be masked for a long period of time, producing display glitches.
>
> For this we use the meson_rdma_writel_sync() which adds the register
> write tuple (VPU register offset and register value) to the RDMA buffer
> and write the value to the HW.
>
> When enabled, the RDMA is enabled to rewritte the same sequence at the
> next VSYNC event, until a new buffer is committed to the OSD plane.
>
> For testing, the only available AFBC buffer generation is the Android
> Yukawa Dvalin Android Mali blobs found at [1].
>
> Both SoCs has been tested using buffers generated under AOSP, but only
> G12A was tested using a runtime stream of AFBC buffers, GXM was only
> tested using static buffers loaded from files.

Reviewed-by: Kevin Hilman 

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

Re: [PATCH v3 8/9] drm/meson: hold 32 lines after vsync to give time for AFBC start

[Kevin Hilman]

Neil Armstrong  writes:

> When using an AFBC encoded frame, the AFBC Decoder must be resetted,

minor grammar nit: s/resetted/reset/

> configured and enabled at each vsync IRQ.
>
> To leave time for that, use the maximum lines hold time to give time
> for AFBC setup and avoid visual glitches.
>
> Signed-off-by: Neil Armstrong 

otherwise...

Reviewed-by: Kevin Hilman 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/meson: implement RDMA for AFBC reset on vsync

[Kevin Hilman]

Neil Armstrong  writes:

> The VPU embeds a "Register DMA" that can write a sequence of registers
> on the VPU AHB bus, either manually or triggered by an internal IRQ
> event like VSYNC or a line input counter.
>
> The initial implementation handles a single channel (over 8), triggered
> by the VSYNC irq and does not handle the RDMA irq.
>
> The RDMA will be usefull to reset and program the AFBC decoder unit
> on each vsync without involving the interrupt handler that can
> be masked for a long period of time, producing display glitches.
>
> For this we use the meson_rdma_writel_sync() which adds the register
> write tuple (VPU register offset and register value) to the RDMA buffer
> and write the value to the HW.
>
> When enabled, the RDMA is enabled to rewritte the same sequence at the
> next VSYNC event, until a new buffer is committed to the OSD plane.
>
> The the Amlogic G12A is switched to RDMA, the Amlogic GXM Decoder
> doesn't need a reset/reprogram at each vsync.
>
> Changes since v1 at [1]:
> - Fixed a regression when AFBC was not used, adding a reset() call for the 
> afbc module
> - Added a define for the RDMA descriptor size
> - Fixed overflow detection

Reviewed-by: Kevin Hilman 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/meson: enable runtime PM

[Kevin Hilman]

Neil Armstrong  writes:

> Hi Kevin,
>
> On 25/09/2019 21:31, Kevin Hilman wrote:
>> From: Kevin Hilman 
>> 
>> On some SoCs, the VPU is in a power-domain and needs runtime PM
>> enabled and used in order to keep the power domain on.
>> 
>> Signed-off-by: Kevin Hilman 
>> ---
>>  drivers/gpu/drm/meson/meson_drv.c | 4 
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/meson/meson_drv.c 
>> b/drivers/gpu/drm/meson/meson_drv.c
>> index 2310c96fff46..256b6a0e9c6b 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.c
>> +++ b/drivers/gpu/drm/meson/meson_drv.c
>> @@ -14,6 +14,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  #include 
>> @@ -274,6 +275,7 @@ static int meson_drv_bind_master(struct device *dev, 
>> bool has_components)
>>  
>>  /* Hardware Initialization */
>>  
>> +pm_runtime_get_sync(dev);
>>  meson_vpu_init(priv);
>>  meson_venc_init(priv);
>>  meson_vpp_init(priv);
>> @@ -416,6 +418,7 @@ static int meson_drv_probe(struct platform_device *pdev)
>>  struct device_node *ep, *remote;
>>  int count = 0;
>>  
>> +pm_runtime_enable(>dev);
>>  for_each_endpoint_of_node(np, ep) {
>>  remote = of_graph_get_remote_port_parent(ep);
>>  if (!remote || !of_device_is_available(remote)) {
>> @@ -440,6 +443,7 @@ static int meson_drv_probe(struct platform_device *pdev)
>>  }
>>  
>>  /* If no output endpoints were available, simply bail out */
>> +pm_runtime_disable(>dev);
>>  return 0;
>>  };
>>  
>> 
>
> I'll rather implement true runtime PM instead,

While this is a minimum implementation, can you explain what you mean by
"true" runtime PM?

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

Re: [PATCH RESEND] drm/meson: vclk: use the correct G12A frac max value

[Kevin Hilman]

Neil Armstrong  writes:

> When calculating the HDMI PLL settings for a DMT mode PHY frequency,
> use the correct max fractional PLL value for G12A VPU.
>
> With this fix, we can finally setup the 1024x768-60 mode.
>
> Fixes: 202b9808f8ed ("drm/meson: Add G12A Video Clock setup")
> Signed-off-by: Neil Armstrong 

Reviewed-by: Kevin Hilman 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/meson: enable runtime PM

[Kevin Hilman]

From: Kevin Hilman 

On some SoCs, the VPU is in a power-domain and needs runtime PM
enabled and used in order to keep the power domain on.

Signed-off-by: Kevin Hilman 
---
 drivers/gpu/drm/meson/meson_drv.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/meson/meson_drv.c 
b/drivers/gpu/drm/meson/meson_drv.c
index 2310c96fff46..256b6a0e9c6b 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -274,6 +275,7 @@ static int meson_drv_bind_master(struct device *dev, bool 
has_components)
 
/* Hardware Initialization */
 
+   pm_runtime_get_sync(dev);
meson_vpu_init(priv);
meson_venc_init(priv);
meson_vpp_init(priv);
@@ -416,6 +418,7 @@ static int meson_drv_probe(struct platform_device *pdev)
struct device_node *ep, *remote;
int count = 0;
 
+   pm_runtime_enable(>dev);
for_each_endpoint_of_node(np, ep) {
remote = of_graph_get_remote_port_parent(ep);
if (!remote || !of_device_is_available(remote)) {
@@ -440,6 +443,7 @@ static int meson_drv_probe(struct platform_device *pdev)
}
 
/* If no output endpoints were available, simply bail out */
+   pm_runtime_disable(>dev);
return 0;
 };
 
-- 
2.22.0

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

Re: [PATCH 0/2] drm/meson: add resume/suspend hooks

[Kevin Hilman]

Neil Armstrong  writes:

> On 27/08/2019 21:17, Kevin Hilman wrote:
>> Neil Armstrong  writes:
>> 
>>> This serie adds the resume/suspend hooks in the Amlogic Meson VPU main 
>>> driver
>>> and the DW-HDMI Glue driver to correctly save state and disable HW before
>>> suspend, and succesfully re-init the HW to recover functionnal display
>>> after resume.
>>>
>>> This serie has been tested on Amlogic G12A based SEI510 board, using
>>> the newly accepted VRTC driver and the rtcwake utility.
>> 
>> Tested-by: Kevin Hilman 
>> 
>> Tested on my G12A SEI510 board, and I verified that it fixes
>> suspend/resume issues previously seen.
>> 
>> Kevin
>> 
>
> Thanks,
>
> Applying to drm-misc-next (for v5.5), with a typo fix in the first patch 
> commit log:
> s/suspens/suspend

Is there any chance of getting this in a a fix for v5.4 so we have a
working suspend/resume in v5.4?

Thanks,

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

Re: [PATCH 0/2] drm/meson: add resume/suspend hooks

[Kevin Hilman]

Neil Armstrong  writes:

> This serie adds the resume/suspend hooks in the Amlogic Meson VPU main driver
> and the DW-HDMI Glue driver to correctly save state and disable HW before
> suspend, and succesfully re-init the HW to recover functionnal display
> after resume.
>
> This serie has been tested on Amlogic G12A based SEI510 board, using
> the newly accepted VRTC driver and the rtcwake utility.

Tested-by: Kevin Hilman 

Tested on my G12A SEI510 board, and I verified that it fixes
suspend/resume issues previously seen.

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

Re: [PATCH 0/9] drm: meson: global clean-up (use proper macros, update comments ...)

[Kevin Hilman]

Julien Masson  writes:

> This patch series aims to clean-up differents parts of the drm meson
> code source.
>
> Couple macros have been defined and used to set several registers
> instead of using magic constants.
>
> I also took the opportunity to:
> - add/remove/update comments
> - remove useless code
> - minor fix/improvment

Nice set of cleanups, thanks!  I especially like the extra in-code
comments.

Could you also add to the cover-letter how this was tested, and on what
platforms so we know it's not going to introduce any regressions.

Thanks,

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

Re: [PATCH 8/9] drm: meson: add macro used to enable HDMI PLL

[Kevin Hilman]

Julien Masson  writes:

> This patch add new macro HHI_HDMI_PLL_CNTL_EN which is used to enable
> HDMI PLL.
>
> Signed-off-by: Julien Masson 
> ---
>  drivers/gpu/drm/meson/meson_vclk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_vclk.c 
> b/drivers/gpu/drm/meson/meson_vclk.c
> index e7c2b439d0f7..be6e152fc75a 100644
> --- a/drivers/gpu/drm/meson/meson_vclk.c
> +++ b/drivers/gpu/drm/meson/meson_vclk.c
> @@ -96,6 +96,7 @@
>  #define HHI_VDAC_CNTL1   0x2F8 /* 0xbe offset in data sheet */
>  
>  #define HHI_HDMI_PLL_CNTL0x320 /* 0xc8 offset in data sheet */
> +#define HHI_HDMI_PLL_CNTL_EN BIT(30)
>  #define HHI_HDMI_PLL_CNTL2   0x324 /* 0xc9 offset in data sheet */
>  #define HHI_HDMI_PLL_CNTL3   0x328 /* 0xca offset in data sheet */
>  #define HHI_HDMI_PLL_CNTL4   0x32C /* 0xcb offset in data sheet */
> @@ -468,7 +469,7 @@ void meson_hdmi_pll_set_params(struct meson_drm *priv, 
> unsigned int m,
>  
>   /* Enable and unreset */
>   regmap_update_bits(priv->hhi, HHI_HDMI_PLL_CNTL,
> -0x7 << 28, 0x4 << 28);
> +0x7 << 28, HHI_HDMI_PLL_CNTL_EN);

still using a magic const for the mask.  Can use GENMASK() for this?

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

Re: [PATCH 4/9] drm: meson: vpp: use proper macros instead of magic constants

[Kevin Hilman]

Julien Masson  writes:

> This patch add new macros which are used to set the following
> registers:
> - VPP_OSD_SCALE_COEF_IDX
> - VPP_DOLBY_CTRL
> - VPP_OFIFO_SIZE
> - VPP_HOLD_LINES
> - VPP_SC_MISC
> - VPP_VADJ_CTRL
>
> Signed-off-by: Julien Masson 

[...]

> @@ -97,20 +97,22 @@ void meson_vpp_init(struct meson_drm *priv)
>   else if (meson_vpu_is_compatible(priv, "amlogic,meson-gxm-vpu")) {
>   writel_bits_relaxed(0xff << 16, 0xff << 16,
>   priv->io_base + _REG(VIU_MISC_CTRL1));
> - writel_relaxed(0x2, priv->io_base + _REG(VPP_DOLBY_CTRL));
> - writel_relaxed(0x1020080,
> + writel_relaxed(VPP_PPS_DUMMY_DATA_MODE,
> +priv->io_base + _REG(VPP_DOLBY_CTRL));
> + writel_relaxed(0x108080,

nit: still a magic constant here, and it's not obvious why it's
different from the current one.

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

Re: [PATCH 2/2] drm/meson: fix G12A primary plane disabling

[Kevin Hilman]

Neil Armstrong  writes:

> The G12A Primary plane was disabled by writing in the OSD1 configuration
> registers, but this caused the plane blender to stall instead of continuing
> blended only the overlay plane.

grammar nit: "...instead of continuing to blend only the overlay plane."

> Fix this by disabling the OSD1 plane in the blender registers, and also
> enabling it back using the same register.
>
> Fixes: 490f50c109d1 ("drm/meson: Add G12A support for OSD1 Plane")
> Signed-off-by: Neil Armstrong 

Reviewed-by: Kevin Hilman 

As noted elsewhere, this driver is also full of magic constants used in
register writes which makes reviewing this kind of change for
correctness that much more difficult, but since that's already been
pointed out elsewhere, and it's already on your TODO list, it should not
block this important fix.

Kevin

Re: [PATCH 1/2] drm/meson: fix primary plane disabling

[Kevin Hilman]

Neil Armstrong  writes:

> The primary plane disable logic is flawed, when the primary plane is
> disabled, it is re-enabled in the vsync irq when another plane is updated.
>
> Handle the plane disabling correctly by handling the primary plane
> enable flag in the primary plane update & disable callbacks.
>
> Fixes: 490f50c109d1 ("drm/meson: Add G12A support for OSD1 Plane")
> Signed-off-by: Neil Armstrong 

Reviewed-by: Kevin Hilman 

Re: [PATCH 5/5] drm/meson: Output in YUV444 if sink supports it

[Kevin Hilman]

Neil Armstrong  writes:

> With the YUV420 handling, we can dynamically setup the HDMI output
> pixel format depending on the mode and connector info.
> So now, we can output in YUV444, which is the native video pipeline
> format, directly to the HDMI Sink if it's supported without
> necessarily involving the HDMI Controller CSC.
>
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/gpu/drm/meson/meson_dw_hdmi.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 5d67e2beba58..8bf9db7f39a4 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -723,12 +723,23 @@ static int meson_venc_hdmi_encoder_atomic_check(struct 
> drm_encoder *encoder,
>   struct drm_display_mode *mode = _state->mode;
>   bool is_hdmi2_sink =
>   conn_state->connector->display_info.hdmi.scdc.supported;
> + bool specify_out_format = false;
> + u32 out_format;
>  
>   if (drm_mode_is_420_only(info, mode) ||
>   (!is_hdmi2_sink && drm_mode_is_420_also(info, mode)))
>   dw_hdmi->input_bus_format = MEDIA_BUS_FMT_UYYVYY8_0_5X24;
> - else
> + else {

nit: if the else has {} you should add to the 'if' (even if the if side
is a single statement): c.f. end of this section of CodingStyle:
https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces

>   dw_hdmi->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;
> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) {
> + out_format = MEDIA_BUS_FMT_YUV8_1X24;
> + specify_out_format = true;
> + }
> + }
> +
> + /* Set a connector bus format if required */
> + drm_display_info_set_bus_formats(info, _format,
> +  (specify_out_format ? 1 : 0));
>  

Otherwise,

Reviewed-by: Kevin Hilman 

Kevin

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

Re: [PATCH 4/5] drm/meson: Add YUV420 output support

[Kevin Hilman]

Neil Armstrong  writes:

> This patch adds support for the YUV420 output from the Amlogic Meson SoCs
> Video Processing Unit to the HDMI Controller.
>
> The YUV420 is obtained by generating a YUV444 pixel stream like
> the classic HDMI display modes, but then the Video Encoder output
> can be configured to down-sample the YUV444 pixel stream to a YUV420
> stream.
> In addition if pixel stream down-sampling, the Y Cb Cr components must
> also be mapped differently to align with the HDMI2.0 specifications.
>
> This mode needs a different clock generation scheme since the TMDS PHY
> clock must match the 10x ration with the YUV420 pixel clock, but

s/ration/ratio/

> the video encoder must run at 2x the pixel clock.
>
> This patch adds the TMDS PHY clock value in all the video clock setup
> in order to better support these specific uses cases and switch
> to the Common Clock framework for clocks handling in the future.
>
> When 420 is needed, it calls drm_bridge_format_set() for notify the
> bridge the input format has changed to YUV420.
>
> Signed-off-by: Neil Armstrong 

Reviewed-by: Kevin Hilman 

with a couple very minor nits to cleanup below...

> ---
>  drivers/gpu/drm/meson/meson_dw_hdmi.c   | 100 +++-
>  drivers/gpu/drm/meson/meson_vclk.c  |  93 --
>  drivers/gpu/drm/meson/meson_vclk.h  |   7 +-
>  drivers/gpu/drm/meson/meson_venc.c  |   6 +-
>  drivers/gpu/drm/meson/meson_venc.h  |  11 +++
>  drivers/gpu/drm/meson/meson_venc_cvbs.c |   3 +-
>  6 files changed, 174 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 779da21143b9..5d67e2beba58 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -159,6 +159,7 @@ struct meson_dw_hdmi {
>   struct regulator *hdmi_supply;
>   u32 irq_stat;
>   struct dw_hdmi *hdmi;
> + unsigned long input_bus_format;
>  };
>  #define encoder_to_meson_dw_hdmi(x) \
>   container_of(x, struct meson_dw_hdmi, encoder)
> @@ -308,6 +309,10 @@ static void meson_hdmi_phy_setup_mode(struct 
> meson_dw_hdmi *dw_hdmi,
>   struct meson_drm *priv = dw_hdmi->priv;
>   unsigned int pixel_clock = mode->clock;
>  
> + /* For 420, pixel clock is half unlike venc clock */
> + if (dw_hdmi->input_bus_format == MEDIA_BUS_FMT_UYYVYY8_0_5X24)
> + pixel_clock /= 2;
> +
>   if (dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
>   dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxm-dw-hdmi")) {
>   if (pixel_clock >= 371250) {
> @@ -383,25 +388,36 @@ static void dw_hdmi_set_vclk(struct meson_dw_hdmi 
> *dw_hdmi,
>  {
>   struct meson_drm *priv = dw_hdmi->priv;
>   int vic = drm_match_cea_mode(mode);
> + unsigned int phy_freq;
>   unsigned int vclk_freq;
>   unsigned int venc_freq;
>   unsigned int hdmi_freq;
>  
>   vclk_freq = mode->clock;
>  
> + /* For 420, pixel clock is half unlike venc clock */

minor grammar nit: put a comma after "half" (here and several other
places in the patch)

[...]

> @@ -722,17 +773,29 @@ static void meson_venc_hdmi_encoder_mode_set(struct 
> drm_encoder *encoder,
>   struct meson_dw_hdmi *dw_hdmi = encoder_to_meson_dw_hdmi(encoder);
>   struct meson_drm *priv = dw_hdmi->priv;
>   int vic = drm_match_cea_mode(mode);
> + unsigned int ycrcb_map = MESON_VENC_MAP_CB_Y_CR;
> + bool yuv420_mode = false;
>  
>   DRM_DEBUG_DRIVER("\"%s\" vic %d\n", mode->name, vic);
>  
> + if (dw_hdmi->input_bus_format == MEDIA_BUS_FMT_UYYVYY8_0_5X24) {
> + ycrcb_map = MESON_VENC_MAP_CR_Y_CB;
> + yuv420_mode = true;
> + }
> +
>   /* VENC + VENC-DVI Mode setup */
> - meson_venc_hdmi_mode_set(priv, vic, mode);
> + meson_venc_hdmi_mode_set(priv, vic, ycrcb_map, yuv420_mode, mode);
>  
>   /* VCLK Set clock */
>   dw_hdmi_set_vclk(dw_hdmi, mode);
>  
> - /* Setup YUV444 to HDMI-TX, no 10bit diphering */
> - writel_relaxed(0, priv->io_base + _REG(VPU_HDMI_FMT_CTRL));
> + if (dw_hdmi->input_bus_format == MEDIA_BUS_FMT_UYYVYY8_0_5X24)
> + /* Setup YUV420 to HDMI-TX, no 10bit diphering */
> + writel_relaxed(2 | (2 << 2),

nit: #define'd bitfields would be better, IIUC, I see these named as
"use average" and "Convert to 420" in the datasheet.

[...]

> @@ -1508,8 +1510,8 @@ void meson_venc_hdmi_mode_set(struct meson_drm *priv, 
> int vic,
>   writel_relaxed((use_enci ? 1 : 2) |
>

Re: [PATCH] drm/meson: Add zpos immutable property to planes

[Kevin Hilman]

Neil Armstrong  writes:

> Add immutable zpos property to primary and overlay planes to specify
> the current fixed zpos position.
>
> Fixes: f9a2348196d1 ("drm/meson: Support Overlay plane for video rendering")
> Signed-off-by: Neil Armstrong 

Reviewed-by: Kevin Hilman 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/meson: Add support for XBGR8888 & ABGR8888 formats

[Kevin Hilman]

Neil Armstrong  writes:

> Add missing XBGR & ABGR formats variants from the primary plane.
>
> Fixes: bbbe775ec5b5 ("drm: Add support for Amlogic Meson Graphic Controller")
> Signed-off-by: Neil Armstrong 

Reviewed-by: Kevin Hilman 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/meson: update with SPDX Licence identifier

[Kevin Hilman]

Neil Armstrong  writes:

> Signed-off-by: Neil Armstrong 

Reviewed-by: Kevin Hilman 

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

Re: [PATCH] drm/meson: fix G12A HDMI PLL settings for 4K60 1000/1001 variations

[Kevin Hilman]

Neil Armstrong  writes:

> The Amlogic G12A HDMI PLL needs some specific settings to lock with
> different fractional values for the 5,4GHz mode.
>
> Handle the 1000/1001 variation fractional case here to avoid having
> the PLL in an non lockable state.
>
> Fixes: 202b9808f8ed ("drm/meson: Add G12A Video Clock setup")
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/gpu/drm/meson/meson_vclk.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_vclk.c 
> b/drivers/gpu/drm/meson/meson_vclk.c
> index 44250eff8a3f..83fc2fc82001 100644
> --- a/drivers/gpu/drm/meson/meson_vclk.c
> +++ b/drivers/gpu/drm/meson/meson_vclk.c
> @@ -553,8 +553,17 @@ void meson_hdmi_pll_set_params(struct meson_drm *priv, 
> unsigned int m,
>  
>   /* G12A HDMI PLL Needs specific parameters for 5.4GHz */
>   if (m >= 0xf7) {
> - regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL4, 0xea68dc00);
> - regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL5, 0x65771290);
> + if (frac < 0x1) {
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL4,
> + 0x6a685c00);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL5,
> + 0x11551293);
> + } else {
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL4,
> + 0xea68dc00);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL5,
> + 0x65771290);
> + }
>   regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL6, 0x39272000);
>   regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL7, 0x5554);
>   } else {

Reviewed-by: Kevin Hilman 

nit: this is continuing with more magic constants, and it would be nice
to have them converted to #define'd bitfields.  But since that isn't a
new problem in this patch, it's fine to cleanup later.

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

Re: [PATCH 0/3] dt-bindings: meson: Add G12A display bindings

[Kevin Hilman]

Neil Armstrong  writes:

> On 13/03/2019 15:10, Neil Armstrong wrote:
>> This patchset adds the G12A specific bindings for the Display VPU
>> and VPU Power Control.
>> 
>> The Amlogic Meson G12A Display module is based on the Meson GXM SoC
>> with an updated Plane Blender, thus VPU architecture and interconnect
>> is the same.
>> 
>> Implementation then DT nodes will come in a further patchsets.
>> 
>> Neil Armstrong (3):
>>   dt-bindings: display: amlogic,meson-vpu: Add G12A compatible and ports
>>   dt-bindings: display: amlogic,meson-dw-hdmi: Add G12A compatible and
>> ports
>>   dt-bindings: power: amlogic,meson-gx-pwrc: Add G12A compatible
>> 
>>  .../devicetree/bindings/display/amlogic,meson-dw-hdmi.txt | 4 
>>  .../devicetree/bindings/display/amlogic,meson-vpu.txt | 4 
>>  .../devicetree/bindings/power/amlogic,meson-gx-pwrc.txt   | 4 +++-
>>  3 files changed, 11 insertions(+), 1 deletion(-)
>> 
>
> Kevin,
>
> I applied Patches 1 & 2 to drm-misc-next, can you take Patch 3 along :

Queued for v5.2,

Thanks,

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

Re: [PATCH v2 0/2] arm64: meson-gxm: Add support for the Mali T820 GPU

[Kevin Hilman]

Neil Armstrong  writes:

> On 15/03/2019 14:56, Neil Armstrong wrote:
>> This patchset adds :
>> - Optional reset properties in the midgard bindings
>> - Mali T820 Node in Amlogic Meson GXM DTSI
>> 
>> Changes since v1:
>> - Updated midgard DT wording following the recently submitted 
>>   bifrost bindings
>> 
>> Christian Hewitt (1):
>>   arm64: dts: meson-gxm: Add Mali-T820 node
>> 
>> Neil Armstrong (1):
>>   dt-bindings: gpu: mali-midgard: Add resets property
>> 
>>  .../bindings/gpu/arm,mali-midgard.txt | 14 ++
>>  arch/arm64/boot/dts/amlogic/meson-gxm.dtsi| 27 +++
>>  2 files changed, 41 insertions(+)
>> 
>
> Kevin, can you take both in your tree now the bindings are reviewed ?

Yes.

Queud for v5.2 (branch: v5.2/dt64)

Thanks,

Kevin



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

Re: [PATCH 0/5] Meson (32-bit): add support for the Mali GPU

[Kevin Hilman]

Neil Armstrong  writes:

> On 08/12/2018 18:12, Martin Blumenstingl wrote:
>> This series adds support for the Mali-450 GPU on Meson8 and Meson8b.
>> Meson6 uses a Mali-400 GPU but since we don't have a clock driver (and
>> I don't have a device for testing) Meson6 is left out in this series.
>> 
>> Meson8 uses a Mali-450 MP6 with six pixel processors. Meson8b (as
>> cost-reduced SoC) uses a Mali-450 MP2 with two pixel processors.
>> I tested both using the open source lima driver and a patched mesa
>> from the lima project as well. Since we don't have display support
>> on the 32-bit SoCs I used off-screen rendering as described in [0].
>> The result is (for example): [1]
>> 
>> The bootloader (at least on my boards) leaves the Mali clock disabled
>> by default. The board crashes when trying to access the Mali registers
>> with the "core" clock disabled.
>> Thus this series also implements the required clock driver changes. The
>> Mali clock tree on Meson8b and Meson8m2 is almost identical to the one
>> on GXBB (see the description of patch #3 for more details). Only Meson8
>> is slightly different as it doesn't have a glitch-free mux. Patch #2
>> prepares the meson8b clock driver so we can have different clocks per
>> SoC.
>> 
>> Dependencies:
>> - the .dts changes depend on my other series "ARM: dts: meson: add the
>>   APB/APB2 busses" from [2]
>> - the .dts changes from this series have no compile-time dependency on
>>   the clock driver changes because CLKID_MALI was defined in the
>>   dt-bindings since the first version of the clock driver (but it was
>>   not used until now).
>> - the .dts changes from this series have a runtime dependency on the
>>   clock driver changes (also from this series) if you have a kernel
>>   patched with the lima driver (without the lima driver there's no
>>   runtime dependency)
>> 
>> Other notes:
>> By default the GPU runs off the XTAL clock (24MHz). The lima driver
>> currently does not update the GPU clock rate. Different frequencies
>> have to be requested by adding the following properties to the Mali
>> GPU node (to run it at 510MHz for example):
>>   assigned-clocks = < CLKID_MALI>;
>>   assigned-clock-rates = <51000>;
>> 
>> 
>> [0] https://gitlab.freedesktop.org/lima/web/wikis/home
>> [1] https://abload.de/img/dump0myic0.png
>> [2] https://patchwork.kernel.org/cover/10719445/
>> 
>> 
>> Martin Blumenstingl (5):
>>   dt-bindings: gpu: mali-utgard: add Amlogic Meson8 and Meson8b
>> compatible
>>   clk: meson: meson8b: use a separate clock table for Meson8
>>   clk: meson: meson8b: add the GPU clock tree
>>   ARM: dts: meson8: add the Mali-450 MP6 GPU
>>   ARM: dts: meson8b: add the Mali-450 MP2 GPU
>> 
>>  .../bindings/gpu/arm,mali-utgard.txt  |   6 +
>>  arch/arm/boot/dts/meson8.dtsi |  58 +++
>>  arch/arm/boot/dts/meson8b.dtsi|  46 +++
>>  drivers/clk/meson/meson8b.c   | 349 +-
>>  drivers/clk/meson/meson8b.h   |   9 +-
>>  5 files changed, 461 insertions(+), 7 deletions(-)
>> 
>
> Applied patches 2 & 3 to next/drivers for Linux 5.1
>
> Kevin, have fun with the other patches !

Fun was had.

Patches 1, 4, 5 queued for v5.1 (branch: v5.1/dt)

Kevin

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

Re: [PATCH 0/2] arm64: meson-gxm: Add support for the Mali T820 GPU

[Kevin Hilman]

Neil Armstrong  writes:

> This patchset adds :
> - Optional reset properties in the midgard bindings
> - Mali T820 Node in Amlogic Meson GXM DTSI
>
> Christian Hewitt (1):
>   arm64: dts: meson-gxm: Add Mali-T820 node
>
> Neil Armstrong (1):
>   dt-bindings: gpu: mali-midgard: Add resets property

Queued for v4.21 (branch: dt64)

Thanks,

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

Re: [PATCH v2 09/13] ARM64: dts: meson-gx: Add support for HDMI output

[Kevin Hilman]

Neil Armstrong  writes:

> On 03/21/2017 04:25 PM, Neil Armstrong wrote:
>> Add HDMI output and connector nodes.
>> 
>> Signed-off-by: Neil Armstrong 

[...]

>
> Hi Kevin,
>
> Please take this one for the amlogic arm-soc DT tree.
>

Applied to v4.12/dt64,

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

Re: [PATCH v2 08/13] ARM64: dts: meson-gx: Add shared CMA dma memory pool

[Kevin Hilman]

Neil Armstrong  writes:

> On 03/21/2017 04:25 PM, Neil Armstrong wrote:
>> The HDMI modes needs more CMA memory to be reserved at boot-time.
>> 
>> Signed-off-by: Neil Armstrong 

[...]

> Hi Kevin,
>
> Please take this one for the amlogic arm-soc DT tree.
>

Applied to v4.12/dt64,

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

[PATCH v2 3/4] dt-bindings: display: add Amlogic Meson DRM Bindings

[Kevin Hilman]

Neil Armstrong  writes:

> Hi Laurent,
> On 11/30/2016 04:58 PM, Laurent Pinchart wrote:
>> Hi Neil,
>> 
>> On Wednesday 30 Nov 2016 16:43:44 Neil Armstrong wrote:
>>> Signed-off-by: Neil Armstrong 
>>> ---
>>>  .../bindings/display/meson/meson-drm.txt   | 101 ++
>> 
>> I forgot to mention that the file should not be named meson-drm.txt as DRM 
>> is 
>> a Linux-specific concept. You can name it meson.txt, but a better option 
>> would 
>> be amlogic,meson.txt. By the way does it really need a subdirectory ?
>
> I took example of the sun4i layout the naming, and no it does not need a 
> subdirector..
>
> I will move it to amlogic,meson.txt, seems far better.
>

To me, amlogic,meson is redundant.  Probably should be amlogic,vpu.txt?

Kevin

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

[Kevin Hilman]

On Wed, Nov 23, 2016 at 9:03 PM, Sekhar Nori  wrote:
> 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.

I don't have a strong preference for the enabled/disabled by default,
but I think the error messages are not error messages.  It seems
perfectly reasonable for boards to accept the reset (or bootloader)
configuration of these registers, and not consider that an error.

Kevin

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

[Kevin Hilman]

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.

Kevin

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

[Kevin Hilman]

Bartosz Golaszewski  writes:

> 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: Kevin Hilman 

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

[Kevin Hilman]

Bartosz Golaszewski  writes:

> 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: Kevin Hilman 

[PATCH v2 4/5] ARM: dts: da850-lcdk: enable mstpri and ddrctl nodes

[Kevin Hilman]

Laurent Pinchart  writes:

> Hi Bartosz,
>
> Thank you for the patch.
>
> On Monday 31 Oct 2016 15:45:37 Bartosz Golaszewski wrote:
>> Enable the MSTPRI configuration and DDR2/mDDR memory controller
>> nodes on da850-lcdk. This is needed in order to adjust the memory
>> throughput constraints for better tilcdc support.
>
> Is there a reason not to enable these unconditionally in da850.dtsi (or 
> rather 
> not disabling them) instead of handling it per board ?

Right.  They should be enabled by default in DT.  The drivers already
have board-specific compatible checks.

Kevin

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

[Kevin Hilman]

Sekhar Nori  writes:

> 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.

I'm OK with that for now.  It makes the most sense to use sycon only for
the register (ranges) that will actually be shared.

Kevin

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

[Kevin Hilman]

Bartosz Golaszewski  writes:

> 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 

[...]

> + for (; setting->name; setting++) {
> + knob = da8xx_ddrctl_match_knob(setting);
> + if (!knob) {
> + dev_warn(dev,
> +  "no such config option: %s\n", setting->name);
> + continue;
> + }
> +
> + if (knob->reg > (res->end - res->start - sizeof(u32))) {

Why the "- sizeof(u32)"?  Shouldn't this just be resource_size(res)?
(c.f. linux/ioport.h)

> + dev_warn(dev,
> +  "register offset of '%s' exceeds mapped memory 
> size\n",
> +  knob->name);
> + continue;
> + }
> +
> + reg = __raw_readl(ddrctl + knob->reg);
> + reg &= knob->mask;
> + reg |= setting->val << knob->shift;
> +
> + dev_dbg(dev, "writing 0x%08x to %s\n", reg, setting->name);
> +
> + __raw_writel(reg, ddrctl + knob->reg);

Can you use the normal readl/writel here?  Or explain why the raw ones
are needed?

> + }
> +
> + return 0;
> +}
> +

Otherwise, looks good to me.  With the changes above, feel free to add

Reviewed-by: Kevin Hilman 

Kevin

[RFC v2] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Kevin Hilman  writes:

> Bartosz Golaszewski  writes:
>
>> 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 +++
>>  drivers/memory/Kconfig |   8 +
>>  drivers/memory/Makefile|   1 +
>>  drivers/memory/da8xx-ddrctl.c  | 175 
>> +
>>  4 files changed, 204 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>>  create mode 100644 drivers/memory/da8xx-ddrctl.c
>>
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> new file mode 100644
>> index 000..7e271dd
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> @@ -0,0 +1,20 @@
>> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory 
>> controller
>> +
>> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs 
>> features
>> +a set of registers which allow to tweak the controller's behavior.
>> +
>> +Documentation:
>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>> +
>> +Required properties:
>> +
>> +- compatible:   "ti,da850-ddr-controller" - for da850 SoC based 
>> boards
>> +- reg:  a tuple containing the base address of the 
>> memory
>> +controller and the size of the memory area to map
>> +
>> +Example for da850 shown below.
>> +
>> +ddrctl {
>> +compatible = "ti,da850-ddr-controller";
>> +reg = <0xB000 0x100>;
>> +};
>
> Axel's series for the USB PHY reminded me that the PHY also has some
> config registers in this same area, and his series creates a syscon for
> a similar range of registers.
>
> Could you create a syscon for the SYSCFG0 registers, which would then
> be used by ths driver and your other drivers/bus driver?  Then the
> binding  would just reference the sysconf via phandle, and your driver
> can use syscon_regmap_lookup_by_phandle()

Nevermind. I though that the config register in this driver was also in
SYSCFG0, but I see now that it's in the reg region of the DDR controller
itself, so no syscon is needed.

Kevin

[RFC v2] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Bartosz Golaszewski  writes:

> 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 +++
>  drivers/memory/Kconfig |   8 +
>  drivers/memory/Makefile|   1 +
>  drivers/memory/da8xx-ddrctl.c  | 175 
> +
>  4 files changed, 204 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>  create mode 100644 drivers/memory/da8xx-ddrctl.c
>
> diff --git 
> a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt 
> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> new file mode 100644
> index 000..7e271dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
> @@ -0,0 +1,20 @@
> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory 
> controller
> +
> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs 
> features
> +a set of registers which allow to tweak the controller's behavior.
> +
> +Documentation:
> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
> +
> +Required properties:
> +
> +- compatible:"ti,da850-ddr-controller" - for da850 SoC based 
> boards
> +- reg:   a tuple containing the base address of the 
> memory
> + controller and the size of the memory area to map
> +
> +Example for da850 shown below.
> +
> +ddrctl {
> + compatible = "ti,da850-ddr-controller";
> + reg = <0xB000 0x100>;
> +};

Axel's series for the USB PHY reminded me that the PHY also has some
config registers in this same area, and his series creates a syscon for
a similar range of registers.

Could you create a syscon for the SYSCFG0 registers, which would then
be used by ths driver and your other drivers/bus driver?  Then the
binding  would just reference the sysconf via phandle, and your driver
can use syscon_regmap_lookup_by_phandle()

> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 4b4c0c3..ec80e35 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -134,6 +134,14 @@ config MTK_SMI
> mainly help enable/disable iommu and control the power domain and
> clocks for each local arbiter.
>  
> +config DA8XX_DDRCTL
> + bool "Texas Instruments da8xx DDR2/mDDR driver"
> + depends on ARCH_DAVINCI_DA8XX
> + help
> +   This driver is for the DDR2/mDDR Memory Controller present on
> +   Texas Instruments da8xx SoCs. It's used to tweak various memory
> +   controller configuration options.
> +
>  source "drivers/memory/samsung/Kconfig"
>  source "drivers/memory/tegra/Kconfig"
>  
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index b20ae38..e88097fb 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_MVEBU_DEVBUS)  += mvebu-devbus.o
>  obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o
>  obj-$(CONFIG_JZ4780_NEMC)+= jz4780-nemc.o
>  obj-$(CONFIG_MTK_SMI)+= mtk-smi.o
> +obj-$(CONFIG_DA8XX_DDRCTL)   += da8xx-ddrctl.o
>  
>  obj-$(CONFIG_SAMSUNG_MC) += samsung/
>  obj-$(CONFIG_TEGRA_MC)   += tegra/
> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
> new file mode 100644
> index 000..66022df
> --- /dev/null
> +++ b/drivers/memory/da8xx-ddrctl.c
> @@ -0,0 +1,175 @@
> +/*
> + * TI da8xx DDR2/mDDR controller driver
> + *
> + * Copyright (C) 2016 BayLibre SAS
> + *
> + * Author:
> + *   Bartosz Golaszewski 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * REVISIT: Linux doesn't have a good framework for the kind of performance
> + * knobs this driver controls. We can't use device tree properties as it 
> deals
> + * with hardware configuration rather than description. We also don't want to
> + * commit to maintaining some random sysfs attributes.
> + *
> + * For now we just hardcode the register values for the boards that need
> + * some changes (as is the case for the LCD controller on da850-lcdk - the
> + * first board we support here). When linux gets an appropriate framework,
> + * we'll easily convert the driver to it.
> + */
> +
> +struct da8xx_ddrctl_config_knob {
> + const char *name;
> + u32 reg;
> + u32 mask;
> + u32 offset;

nit: call this shift instead, which will also map well onto the regmap
accessors (which you'll use when switching to syscon.)

> +};
> +
> +static const struct da8xx_ddrctl_config_knob 

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

On Mon, Oct 24, 2016 at 11:41 AM, Kevin Hilman  wrote:
> Mark Rutland  writes:
>
>> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>>> Hi Mark,
>>>
>>> Mark Rutland  writes:
>>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, 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;
>>> >> + u32 regprop[2], base, memsize, reg;
>>> >> + struct device_node *node, *parent;
>>> >> + void __iomem *ddrctl;
>>> >> + const char *board;
>>> >> + struct device *dev;
>>> >> + int ret;
>>> >> +
>>> >> + dev = >dev;
>>> >> + node = dev->of_node;
>>> >> +
>>> >> + /* Find the board name. */
>>> >> + for (parent = node;
>>> >> +  !of_node_is_root(parent);
>>> >> +  parent = of_get_parent(parent));
>>> >> +
>>> >> + ret = of_property_read_string(parent, "compatible", );
>>> >> + if (ret) {
>>> >> + dev_err(dev, "unable to read the soc model\n");
>>> >> + return ret;
>>> >> + }
>>> >
>>> > I can see that you want to expose sysfs knobs for this, but is it really
>>> > necessary to match boards like this? It's very fragile, and commits us
>>> > to maintaining a database of board data (i.e. a board file).
>>> >
>>> > I am very much not keen on that.
>>>
>>> The original proposal[1] was to create DT properties reflecting the
>>> various knobs in the DDR Controller, but that was frowned upon since
>>> that was more HW configuration than hardware description.
>>>
>>> That resulted in this approach which keeps the HW configuration values
>>> in the driver, and selectable based on DT compatible.
>>>
>>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>>> you comment on your preference?
>>
>> From my PoV, it really depends on *why* we need this. What does the
>> tuning gain us, and is it specific to a given use-case?
>
> This is essentially a bus controller which allows you to set relative
> priorities of the various bus masters (CPU data, CPU instructions, DMA
> channels, ethernet MAC, SATA, display controller, etc. etc.)

Scratch that... I got this one confused with a different drivers/bus
driver Bartosz is also working on. :(

This one is just for the mechanism that controls how long old
(low-priority) xfers in the DDR command FIFO are allowed to sit around
before they will be flushed.

The use-case is the same though.  The display controller doesn't work
at higher resolutions without tweaking this setting.

The question remains though: as a system-wide setting, should this be
configured via DT (either by a DT property, or based on a compatible
string in the driver) or should the driver provide an API to tweak it.

Kevin

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Mark Rutland  writes:

> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>> Hi Mark,
>> 
>> Mark Rutland  writes:
>> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, 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;
>> >> + u32 regprop[2], base, memsize, reg;
>> >> + struct device_node *node, *parent;
>> >> + void __iomem *ddrctl;
>> >> + const char *board;
>> >> + struct device *dev;
>> >> + int ret;
>> >> +
>> >> + dev = >dev;
>> >> + node = dev->of_node;
>> >> +
>> >> + /* Find the board name. */
>> >> + for (parent = node;
>> >> +  !of_node_is_root(parent);
>> >> +  parent = of_get_parent(parent));
>> >> +
>> >> + ret = of_property_read_string(parent, "compatible", );
>> >> + if (ret) {
>> >> + dev_err(dev, "unable to read the soc model\n");
>> >> + return ret;
>> >> + }
>> >
>> > I can see that you want to expose sysfs knobs for this, but is it really
>> > necessary to match boards like this? It's very fragile, and commits us
>> > to maintaining a database of board data (i.e. a board file).
>> >
>> > I am very much not keen on that.
>> 
>> The original proposal[1] was to create DT properties reflecting the
>> various knobs in the DDR Controller, but that was frowned upon since
>> that was more HW configuration than hardware description.
>> 
>> That resulted in this approach which keeps the HW configuration values
>> in the driver, and selectable based on DT compatible.
>> 
>> IMO, neither aproach is pretty.  From a DT maintainer perspective, can
>> you comment on your preference?
>
> From my PoV, it really depends on *why* we need this. What does the
> tuning gain us, and is it specific to a given use-case?

This is essentially a bus controller which allows you to set relative
priorities of the various bus masters (CPU data, CPU instructions, DMA
channels, ethernet MAC, SATA, display controller, etc. etc.)

The reason behind this work in the first place is a specific
use-case. Namely, a display controller on this SoC needs its bus
priority to be adjusted in order to work reliably at certain
resolutions

The vendor BSPs for this chip just setup hard-coded values in the board
files (davinci, still hasn't fully migrated to DT) but we're trying to
figure out a better way.

The first approach was exposing DT knobs for all the priorities.  The
second approach was the other extermem allowing SoC or board-specific
hard-coded values.

Another possible approach would be getting rid of the hard-coded values
and exporting an API from the driver to set the priorities of the
available masters at run-time.  I'm not awarye any current need of
changing things at run-time, but it seems potentially useful as well.

Based on all this discussion, I'm starting to lean towards the runtime
API approach, but am hoping for some suggestions.

Kevin

[RFC] ARM: memory: da8xx-ddrctl: new driver

[Kevin Hilman]

Hi Mark,

Mark Rutland  writes:

> On Mon, Oct 24, 2016 at 06:46:36PM +0200, 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 +++
>>  drivers/memory/Kconfig |   8 +
>>  drivers/memory/Makefile|   1 +
>>  drivers/memory/da8xx-ddrctl.c  | 187 
>> +
>>  4 files changed, 216 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>>  create mode 100644 drivers/memory/da8xx-ddrctl.c
>> 
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> new file mode 100644
>> index 000..f0eda59
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/memory-controllers/ti-da8xx-ddrctl.txt
>> @@ -0,0 +1,20 @@
>> +* Device tree bindings for Texas Instruments da8xx DDR2/mDDR memory 
>> controller
>> +
>> +The DDR2/mDDR memory controller present on Texas Instruments da8xx SoCs 
>> memory
>> +maps a set of registers which allow to tweak the controller's behavior.
>
> This is a description of the *driver*. The device itself doesn't map
> some registers, it features them. Please descrive the *device*.
>
>> +
>> +Documentation:
>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>> +
>> +Required properties:
>> +
>> +- compatible:   "ti,da850-ddrctl" - for da850 SoC based boards
>
> Perhaps:
>
>   "ti,da850-ddr-controller"
>
>> +static int da8xx_ddrctl_probe(struct platform_device *pdev)
>> +{
>> +const struct da8xx_ddrctl_config_knob *knob;
>> +const struct da8xx_ddrctl_setting *setting;
>> +u32 regprop[2], base, memsize, reg;
>> +struct device_node *node, *parent;
>> +void __iomem *ddrctl;
>> +const char *board;
>> +struct device *dev;
>> +int ret;
>> +
>> +dev = >dev;
>> +node = dev->of_node;
>> +
>> +/* Find the board name. */
>> +for (parent = node;
>> + !of_node_is_root(parent);
>> + parent = of_get_parent(parent));
>> +
>> +ret = of_property_read_string(parent, "compatible", );
>> +if (ret) {
>> +dev_err(dev, "unable to read the soc model\n");
>> +return ret;
>> +}
>
> I can see that you want to expose sysfs knobs for this, but is it really
> necessary to match boards like this? It's very fragile, and commits us
> to maintaining a database of board data (i.e. a board file).
>
> I am very much not keen on that.

The original proposal[1] was to create DT properties reflecting the
various knobs in the DDR Controller, but that was frowned upon since
that was more HW configuration than hardware description.

That resulted in this approach which keeps the HW configuration values
in the driver, and selectable based on DT compatible.

IMO, neither aproach is pretty.  From a DT maintainer perspective, can
you comment on your preference?

Thanks,

Kevin

[1] https://marc.info/?l=linux-arm-kernel=147672200715076=2

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

[Kevin Hilman]

Hi Laurent,

Laurent Pinchart  writes:

> On Thursday 20 Oct 2016 09:57:51 Kevin Hilman wrote:
>> Laurent Pinchart  writes:
>> > On Wednesday 19 Oct 2016 10:26:57 Bartosz Golaszewski wrote:
>> >> 2016-10-18 22:49 GMT+02:00 Laurent Pinchart:
>> >>> On Monday 17 Oct 2016 18:30:49 Bartosz Golaszewski wrote:
>> >>>> Create the driver for the da8xx System Configuration and implement
>> >>>> support for writing to the three Master Priority registers.
>> >>>> 
>> >>>> Signed-off-by: Bartosz Golaszewski 
>> >> 
>> >> [snip]
>> >> 
>> >>>> +
>> >>>> +Documentation:
>> >>>> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>> >>>> +
>> >>>> +Required properties:
>> >>>> +
>> >>>> +- compatible:"ti,da850-syscfg"
>> >>> 
>> >>> Don't you need a reg property ?
>> >> 
>> >> Yes, Kevin already pointed that out. I'll add it in v2. Same for [1/3].
>> >> 
>> >>>> +Optional properties:
>> >>>> +
>> >>>> +The below properties are used to specify the priority of master
>> >>>> peripherals.
>> >>>> +They must be between 0-7 where 0 is the highest priority and 7 is the
>> >>>> lowest.
>> >>>> +
>> >>>> +- ti,pri-arm-i:  ARM_I port priority.
>> >>>> +
>> >>>> +- ti,pri-arm-d:  ARM_D port priority.
>> >>>> +
>> >>>> +- ti,pri-upp:uPP port priority.
>> >>>> +
>> >>>> +- ti,pri-sata:   SATA port priority.
>> >>>> +
>> >>>> +- ti,pri-pru0:   PRU0 port priority.
>> >>>> +
>> >>>> +- ti,pri-pru1:   PRU1 port priority.
>> >>>> +
>> >>>> +- ti,pri-edma30tc0:  EDMA3_0_TC0 port priority.
>> >>>> +
>> >>>> +- ti,pri-edma30tc1:  EDMA3_0_TC1 port priority.
>> >>>> +
>> >>>> +- ti,pri-edma31tc0:  EDMA3_1_TC0 port priority.
>> >>>> +
>> >>>> +- ti,pri-vpif-dma-0: VPIF DMA0 port priority.
>> >>>> +
>> >>>> +- ti,pri-vpif-dma-1: VPIF DMA1 port priority.
>> >>>> +
>> >>>> +- ti,pri-emac:   EMAC port priority.
>> >>>> +
>> >>>> +- ti,pri-usb0cfg:USB0 CFG port priority.
>> >>>> +
>> >>>> +- ti,pri-usb0cdma:   USB0 CDMA port priority.
>> >>>> +
>> >>>> +- ti,pri-uhpi:   HPI port priority.
>> >>>> +
>> >>>> +- ti,pri-usb1:   USB1 port priority.
>> >>>> +
>> >>>> +- ti,pri-lcdc:   LCDC port priority.
>> >>> 
>> >>> I'm afraid this looks more like system configuration than hardware
>> >>> description to me.
>> >> 
>> >> While you're certainly right, this approach is already implemented in
>> >> several other memory and bus drivers and it was also suggested by
>> >> Sekhar in one of the tilcdc rev1 threads. There's also no real
>> >> alternative that I know of.
>> > 
>> > The fact that other drivers get it wrong is no excuse for copying them :-)
>> 
>> What exactly is "wrong" with the way other drivers are doing it?
>> 
>> I'm sure there may be other ideas, and possibly some better ones, but
>> that doesn't make it wrong, and doesn't change he fact that the kernel
>> has existing drivers SoC-bus-specific system performance knobs like
>> this.
>
> It's not the drivers I'm concerned about, but the DT bindings.

I see, thanks for the clarification.

> The proposed DT binding contains a large number of properties that
> don't describe the hardware but contain configuration data.

I agree that there ought to be some more generic way for devices to
request performance constraints from their busses at runtime based on
their current operating critera, but unfortunately that doesn't exist
yet.

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.

Thanks again for the review,

Kevin

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

[Kevin Hilman]

+Arnd, Olof

Laurent Pinchart  writes:

> Hi Bartosz,
>
> On Wednesday 19 Oct 2016 10:26:57 Bartosz Golaszewski wrote:
>> 2016-10-18 22:49 GMT+02:00 Laurent Pinchart:
>> > On Monday 17 Oct 2016 18:30:49 Bartosz Golaszewski wrote:
>> >> Create the driver for the da8xx System Configuration and implement
>> >> support for writing to the three Master Priority registers.
>> >> 
>> >> Signed-off-by: Bartosz Golaszewski 
>> 
>> [snip]
>> 
>> >> +
>> >> +Documentation:
>> >> +OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf
>> >> +
>> >> +Required properties:
>> >> +
>> >> +- compatible:"ti,da850-syscfg"
>> > 
>> > Don't you need a reg property ?
>> 
>> Yes, Kevin already pointed that out. I'll add it in v2. Same for [1/3].
>> 
>> >> +Optional properties:
>> >> +
>> >> +The below properties are used to specify the priority of master
>> >> peripherals.
>> >> +They must be between 0-7 where 0 is the highest priority and 7 is the
>> >> lowest.
>> >> +
>> >> +- ti,pri-arm-i:  ARM_I port priority.
>> >> +
>> >> +- ti,pri-arm-d:  ARM_D port priority.
>> >> +
>> >> +- ti,pri-upp:uPP port priority.
>> >> +
>> >> +- ti,pri-sata:   SATA port priority.
>> >> +
>> >> +- ti,pri-pru0:   PRU0 port priority.
>> >> +
>> >> +- ti,pri-pru1:   PRU1 port priority.
>> >> +
>> >> +- ti,pri-edma30tc0:  EDMA3_0_TC0 port priority.
>> >> +
>> >> +- ti,pri-edma30tc1:  EDMA3_0_TC1 port priority.
>> >> +
>> >> +- ti,pri-edma31tc0:  EDMA3_1_TC0 port priority.
>> >> +
>> >> +- ti,pri-vpif-dma-0: VPIF DMA0 port priority.
>> >> +
>> >> +- ti,pri-vpif-dma-1: VPIF DMA1 port priority.
>> >> +
>> >> +- ti,pri-emac:   EMAC port priority.
>> >> +
>> >> +- ti,pri-usb0cfg:USB0 CFG port priority.
>> >> +
>> >> +- ti,pri-usb0cdma:   USB0 CDMA port priority.
>> >> +
>> >> +- ti,pri-uhpi:   HPI port priority.
>> >> +
>> >> +- ti,pri-usb1:   USB1 port priority.
>> >> +
>> >> +- ti,pri-lcdc:   LCDC port priority.
>> > 
>> > I'm afraid this looks more like system configuration than hardware
>> > description to me.
>> 
>> While you're certainly right, this approach is already implemented in
>> several other memory and bus drivers and it was also suggested by
>> Sekhar in one of the tilcdc rev1 threads. There's also no real
>> alternative that I know of.
>
> The fact that other drivers get it wrong is no excuse for copying them :-)

What exactly is "wrong" with the way other drivers are doing it?

I'm sure there may be other ideas, and possibly some better ones, but
that doesn't make it wrong, and doesn't change he fact that the kernel
has existing drivers SoC-bus-specific system performance knobs like
this.

>> > There was a BoF session about how to support this kind of performance
>> > knobs at ELCE last week:
>> > https://openiotelceurope2016.sched.org/event/7rss/bof-linux-device-perfor
>> > mance-framework-michael-turquette-baylibre :-)
>>
>> Unfortunately it was just a discussion about potential approaches -
>> there's no code yet.
>
> Patches are welcome ;-)

Any generic perf framework will have to build on the HW-specifics of
individual busses, so IMO, the lack of a generic performance
framework/knobs should not be a reason to block the inclusion of any
bus-specific knobs.

I guess this ultimately would go though arm-soc, so I've added Arnd &
Olof to the thread.

Kevin

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

[Kevin Hilman]

Bartosz Golaszewski  writes:

> Create the driver for the da8xx System Configuration and implement
> support for writing to the three Master Priority registers.
>
> Signed-off-by: Bartosz Golaszewski 

[...]

> +#define DA8XX_IO_PHYS0x01c0ul
> +#define DA8XX_SYSCFG0_BASE   (DA8XX_IO_PHYS + 0x14000)

The base addr should come from DT.

Kevin

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

[Kevin Hilman]

Bartosz Golaszewski  writes:

> 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 

[...]

> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
> new file mode 100644
> index 000..dcd0a61
> --- /dev/null
> +++ b/drivers/memory/da8xx-ddrctl.c
> @@ -0,0 +1,77 @@
> +/*
> + * TI da8xx DDR2/mDDR controller driver
> + *
> + * Copyright (C) 2016 BayLibre SAS
> + *
> + * Author:
> + *   Bartosz Golaszewski 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DA8XX_DDR_CTL_BASE   0xB000

This base addr should be the reg property of the node.

> +#define DA8XX_PBBPR_OFFSET   0x0020
> +#define DA8XX_PBBPR_REG(p)   ((p) + DA8XX_PBBPR_OFFSET)

Kevin

[PATCH 0/3] drm/tilcdc: Some fixes for LCDC rev1

[Kevin Hilman]

Hi Jyri,

Jyri Sarha  writes:

> On 08/23/16 15:56, Karl Beldan wrote:
>> Hi,
>> 
>> I found some missing bits for rev1 of the LCDC and here are some of the
>> changes I am using to use the DRM driver on an LCDCK (which has a rev1).
>> 1/3 seems required by rev1 of the IP and without it my the LCDC on my
>> LCDK keeps can't sync, 2/3 is required by the driver logic, while 3/3
>> is less of a requirement.
>
> I'll drop 3/3, because my recent patches should take care of that:
> http://www.spinics.net/lists/devicetree/msg138885.html
>
> However, there will be v2 round of those patches. Only the DT binding
> and its default value should change.
>
>> Although 1,2/3 would have fitted drm-fixes I based them on drm-next as
>> 2/3 would conflict with the recent changes in drm-next.
>> 
>
> I'll pick these two up for my next pull request. Thanks!
>

Can you share a branch which has your current series and these fixes
included, so we can use that as a baseline for further development?

Thanks,

Kevin

[RESEND PATCH v3 2/2] PM / Domains: Remove redundant wrapper functions for system PM

[Kevin Hilman]

Ulf Hansson  writes:

> Due to the previous changes to genpd, which removed the suspend_power_off
> flag, several of the system PM callbacks is no longer doing any additional
> checks but only invoking a corresponding pm_generic_* helper function.
>
> To clean up the code let's remove these wrapper functions as they have
> become redundant. Instead assign the system PM callbacks directly to
> the pm_generic_*() helper function.
>
> While changing this, it became clear that some of the current system PM
> callbacks in genpd, invokes the wrong callback at the driver level. For
> example, genpd's ->restore() callback invokes pm_generic_resume(), while
> it should be pm_generic_restore(). Let's fix this as well.
>
> Signed-off-by: Ulf Hansson 

Reviewed-by: Kevin Hilman 
Acked-by: Kevin Hilman 

Kevin

[RESEND PATCH v3 1/2] PM / Domains: Allow genpd to power on during the system PM phases

[Kevin Hilman]

Ulf Hansson  writes:

> If the PM domain is powered off when the first device starts its system PM
> prepare phase, genpd prevents any further attempts to power on the PM
> domain during the following system PM phases. Not until the system PM
> complete phase is finalized for all devices in the PM domain, genpd again
> allows it to be powered on.
>
> This behaviour needs to be changed, as a subsystem/driver for a device in
> the same PM domain may still need to be able to serve requests in some of
> the system PM phases. Accordingly, it may need to runtime resume its
> device and thus also request the corresponding PM domain to be powered on.
>
> To deal with these scenarios, let's make the device operational in the
> system PM prepare phase by runtime resuming it, no matter if the PM domain
> is powered on or off. Changing this also enables us to remove genpd's
> suspend_power_off flag, as it's being used to track this condition.
> Additionally, we must allow the PM domain to be powered on via runtime PM
> during the system PM phases.
>
> This change also requires a fix in the AMD ACP (Audio CoProcessor) drm
> driver. It registers a genpd to model the ACP as a PM domain, but
> unfortunately it's also abuses genpd's "internal" suspend_power_off flag
> to deal with a corner case at system PM resume.
>
> More precisely, the so called SMU block powers on the ACP at system PM
> resume, unconditionally if it's being used or not. This may lead to that
> genpd's internal status of the power state, may not correctly reflect the
> power state of the HW after a system PM resume.
>
> Because of changing the behaviour of genpd, by runtime resuming devices in
> the prepare phase, the AMD ACP drm driver no longer have to deal with this
> corner case. So let's just drop the related code in this driver.
>
> Cc: David Airlie 
> Cc: Alex Deucher 
> Cc: Christian König 
> Cc: Maruthi Srinivas Bayyavarapu 
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: Ulf Hansson 
> ---
>
> Changes in v3:
>   - Updated changelog.
>
> Changes in v2:
>   - Updated changelog.
>   - Added a fix in the AMD ACP (Audio CoProcessor) drm driver, which
>   registers a genpd. The fix removes the usage of genpd's internal
>   suspend_power_off flag as it's not needed after this change. Because of
>   this change I am also requesting an ack from the drm driver maintainer.
>
>
> ---
>  drivers/base/power/domain.c | 84 
> -----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 23 -
>  include/linux/pm_domain.h   |  1 -


For the PM core part:

Reviewed-by: Kevin Hilman 
Acked-by: Kevin Hilman 

For the AMD DRM driver, the changes look right too, but I'm not
confident enough about the intent of that to be sure.

Kevin

[PATCH v2 1/2] PM / Domains: Allow genpd to power on during the system PM phase

[Kevin Hilman]

Ulf Hansson  writes:

> On 11 May 2016 at 23:25, Rafael J. Wysocki  wrote:
>> On Wed, May 11, 2016 at 10:00 AM, Ulf Hansson  
>> wrote:
>>> If the PM domain is powered off when the first device in the domain starts
>>> its system PM prepare phase, genpd prevents any further attempts to power
>>> on the PM domain during the system PM phase. This constraint affects not
>>> only the current device which is being prepared, but all devices within
>>> the same PM domain.
>>
>> What exactly do you mean by "the system PM phase"?
>
> In between system PM device prepare and system PM device complete.
>
> Do you want me to clarify this in a better way in the change log?

It might be more clear if you something like "during system suspend/resume"

Kevin

[RFT PATCH] drm/exynos: Enable DP clock to fix display on Exynos5250 and other

[Kevin Hilman]

Krzysztof Kozlowski  writes:

> 2015-04-30 2:31 GMT+09:00 Kevin Hilman :
>> Krzysztof Kozlowski  writes:
>>
>>> After adding display power domain for Exynos5250 in commit
>>> 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250") the
>>> display on Chromebook Snow and others stopped working after boot.
>>>
>>> The reason for this suggested Andrzej Hajda: the DP clock was disabled.
>>> This clock is required by Display Port and is enabled by bootloader.
>>> However when FIMD driver probing was deferred, the display power domain
>>> was turned off. This effectively reset the value of DP clock enable
>>> register.
>>>
>>> When exynos-dp is later probed, the clock is not enabled and display is
>>> not properly configured:
>>>
>>> exynos-dp 145b.dp-controller: Timeout of video streamclk ok
>>> exynos-dp 145b.dp-controller: unable to config video
>>>
>>> Signed-off-by: Krzysztof Kozlowski 
>>> Reported-by: Javier Martinez Canillas 
>>> Fixes: 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250")
>>> Cc: 
>>>
>>> ---
>>>
>>> This should fix issue reported by Javier [1][2].
>>>
>>> Tested on Chromebook Snow (Exynos 5250). More testing would be great,
>>> especially on other Exynos 5xxx products.
>>
>> I hoped to try this on my exynos5 boards, but it doesn't seem to apply
>> to linux-next or to Linus' master branch.
>>
>> Are there some other dependencies here?
>
> It is already applied:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=1c363c7cccf64128087002b0779986ad16aff6dc

Er, yup.  That would explain it. ;)

Sorry for the noise,

Kevin

[RFT PATCH] drm/exynos: Enable DP clock to fix display on Exynos5250 and other

[Kevin Hilman]

Krzysztof Kozlowski  writes:

> After adding display power domain for Exynos5250 in commit
> 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250") the
> display on Chromebook Snow and others stopped working after boot.
>
> The reason for this suggested Andrzej Hajda: the DP clock was disabled.
> This clock is required by Display Port and is enabled by bootloader.
> However when FIMD driver probing was deferred, the display power domain
> was turned off. This effectively reset the value of DP clock enable
> register.
>
> When exynos-dp is later probed, the clock is not enabled and display is
> not properly configured:
>
> exynos-dp 145b.dp-controller: Timeout of video streamclk ok
> exynos-dp 145b.dp-controller: unable to config video
>
> Signed-off-by: Krzysztof Kozlowski 
> Reported-by: Javier Martinez Canillas 
> Fixes: 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250")
> Cc: 
>
> ---
>
> This should fix issue reported by Javier [1][2].
>
> Tested on Chromebook Snow (Exynos 5250). More testing would be great,
> especially on other Exynos 5xxx products.

I hoped to try this on my exynos5 boards, but it doesn't seem to apply
to linux-next or to Linus' master branch.

Are there some other dependencies here?

Kevin

[RFC PATCH 1/1] drm/exynos: Move platform drivers registration to module init

[Kevin Hilman]

Javier Martinez Canillas  writes:

> Hello,
>
> For completeness I'll comment what we talked with Kevin on IRC
> since probably this is the same issue that Paolo is facing.
>
> On 11/20/2014 05:41 PM, Kevin Hilman wrote:
>> Peach # setenv preboot "usb start; sleep 1"setenv bootargs
>> console=tty1 console=ttySAC3,115200 debug earlyprintk rw
>> root=/dev/mmcblk1p3 rootwait rootfstype=ext3
>
> My kernel command line is almost the same with the difference that I'm using
> clk_ignore_unused and I just checked that not passing that parameter, makes
> linux-next to hang showing the same output log that Kevin reported.

Ah!  Good find.  I confirm that adding clk_ignore_unused is getting me
booting again, but of course that is just masking a problem, so I hope
someone can shed light on which clock isn't being correctly managed.

Might I also suggest that folks have their default command-line to *not*
use clk_ignore_unused, since it's primary job is to workaround clock
bugs.

Kevin

[RFC PATCH 1/1] drm/exynos: Move platform drivers registration to module init

[Kevin Hilman]

Hi Vivek,

Vivek Gautam  writes:

[...]

> I tested linux-next on Exynos5800 peach-pi board with linux-next and and the 
> two
> patches $Subject and [0].
>
> Below is my git hash:
> 4d9e6ee drm/exynos: Move platform drivers registration to module init
> 4545ed4 POSTED: arm: dts: Exynos5: Use pmu_system_controller phandle for dp 
> phy
> 36391a5 Add linux-next specific files for 20141119
> 9b1ced1 Merge branch 'akpm/master'
> 282497e mm: add strictlimit knob
>
> With this display works for me.
> Without $Subject patch i get lookup in drm.

The current linux-next (next-20141120) is still not fully booting for
me.  I do see the penguins come up on the display, but it doesn't finish
booting.   Full boot log below.

I'm building using exynos_defconfig with CONFIG_SND_SOC_SNOW=n due to
other errors previously reported.  (Vivek, BTW, I'm curious how your peach-pi
is booting with the audio support enabled.)

Here's how I'm creating the tree, which appears to be the same as what
you guys are doing, but just to be thorough:

$ git reset --hard next/master
HEAD is now at 5b83d7ad9106 Add linux-next specific files for 20141120

$ pwclient git-am 5197701
Applying patch #5197701 using 'git am'
Description: [v2,2/2] arm: dts: Exynos5: Use pmu_system_controller phandle for 
dp phy
Applying: arm: dts: Exynos5: Use pmu_system_controller phandle for dp phy

$ pwclient git-am 5328881
Applying patch #5328881 using 'git am'
Description: [RFC,1/1] drm/exynos: Move platform drivers registration to module 
init
Applying: drm/exynos: Move platform drivers registration to module init

$ git log --oneline -n5
b16800da58a3 drm/exynos: Move platform drivers registration to module init
87541edf8a17 arm: dts: Exynos5: Use pmu_system_controller phandle for dp phy
5b83d7ad9106 Add linux-next specific files for 20141120
fd7e97b09f45 Merge branch 'akpm/master'
11729160b2d7 mm: add strictlimit knob

Kevin


[1] 
Connected to chromebook2 console [channel connected] (~$quit to exit)
(user:khilman) is already connected


# PYBOOT: console: connected.
~$hardreset

Command(chromebook2 console)> hardreset
(user:khilman) Reboot chromebook2
Reboot: chromebook2 ; phidget 4 2 :  ('off', 1, 'on')


U-Boot 2013.04-gb98ed09 (Apr 30 2014 - 16:57:31) for Peach

CPU:Exynos5422 at 1800MHz

Board: Google Peach Pi, rev 13.6
I2C:   ready
DRAM:  3.5 GiB
PMIC max77802-pmic initialized
CPU:Exynos5422 at 1800MHz
TPS65090 PMIC EC init
MMC:   EXYNOS DWMMC: 0, EXYNOS DWMMC: 1
SF: Detected W25Q32DW with page size 4 KiB, total 32 MiB
In:cros-ec-keyb
Out:   serial
Err:   lcd
SF: Detected W25Q32DW with page size 4 KiB, total 32 MiB
ELOG: Event(17) added with size 13
Net:   No ethernet found.
Hit any key to stop autoboot:  2 

# PYBOOT: u-boot: taking control.
 0 
Exynos DP init done
Peach # 
Peach setenv stdout serial
# setenv stdout serialversion

Peach # versionsetenv preboot "usb start; sleep 1"


U-Boot 2013.04-gb98ed09 (Apr 30 2014 - 16:57:31) for Peach
armv7a-cros-linux-gnueabi-gcc.real (4.7.2_cos_gg_c8f69e0) 4.7.x-google 20130114 
(prerelease)
GNU ld (binutils-2.22_cos_gg_2) 2.22
Peach # setenv preboot "usb start; sleep 1"setenv bootargs console=tty1 
console=ttySAC3,115200 debug earlyprintk rw root=/dev/mmcblk1p3 rootwait 
rootfstype=ext3

Peach # setenv bootargs console=tty1 console=ttySAC3,115200 debug earlyprintk 
rw root=/dev/mmcblk1p3 rootwait rootfstype=ext3if test -n ${initenv}; then run 
initenv; fi

Peach # if test -n ${initenv}; then run initenv; fiif test -n ${preboot}; then 
run preboot; fi

Peach # if test -n ${preboot}; then run preboot; fisetenv autoload no; setenv 
autoboot no

(Re)start USB...
USB0:   Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.00
scanning bus 0 for devices... 2 USB Device(s) found
USB1:   Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.00
scanning bus 1 for devices... 1 USB Device(s) found
   scanning usb for storage devices... 0 Storage Device(s) found
   scanning usb for ethernet devices... 1 Ethernet Device(s) found
Peach # dhcp
dhcp
Waiting for Ethernet connection... done.
BOOTP broadcast 1
BOOTP broadcast 2
DHCP client bound to address 192.168.1.110
Peach # setenv serverip 192.168.1.2
setenv serverip 192.168.1.2
Peach # if test -n ${netargs}; then run netargs; fi
if test -n ${netargs}; then run netargs; fi
Peach # tftp 0x4100 192.168.1.2:tmp/chromebook2-3T8ptb/uImage-48m8WK
tftp 0x4100 192.168.1.2:tmp/chromebook2-3T8ptb/uImage-48m8WK
Using asx0 device
TFTP from server 192.168.1.2; our IP address is 192.168.1.110
Filename 'tmp/chromebook2-3T8ptb/uImage-48m8WK'.
Load address: 0x4100
Loading: *#
 #
 #
 ##
 1.2 MiB/s
done
Bytes transferred = 3473930 (35020a hex)
Peach # printenv bootargs
printenv bootargs
bootargs=console=tty1 

[RFC PATCH 1/1] drm/exynos: Move platform drivers registration to module init

[Kevin Hilman]

Kevin Hilman  writes:

> Javier Martinez Canillas  writes:
>
>> [adding Paolo and Vivek as cc]
>>
>> Hello,
>>
>> On 11/18/2014 07:41 PM, Kevin Hilman wrote:
>>> 
>>> It fixes the DRM deadlock, issue for me on exynos5800-peach-pi, but then
>>> it proceeds to panic in the workqueue code called by the asoc max98090
>>> codec[1].
>>> 
>>> If I then disable CONFIG_SND_SOC_SNOW, I can get it to boot to a shell,
>>> but I still don't have display output.
>>> 
>>
>> Paolo Pisati pointed out in another thread that he needed the patch
>> "[PATCH v2 2/2] arm: dts: Exynos5: Use pmu_system_controller phandle for dp 
>> phy"
>> is also needed to get display working for exynos on linux-next.
>>
>> I've pinged Kukjin to apply this as a -rc fix since is needed after
>> a5ec598 ("phy: exynos-dp-video: Use syscon support to control pmu register")
>> landed in 3.18 which broke the Exynos Display Port PHY:
>>
>> exynos-dp-video-phy 10040728.video-phy: Failed to lookup PMU regmap
>>
>> I've an Exynos5800 Peach Pi now so I wanted to test display on it. Just 
>> $subject
>> and [0] should be enough to have display working on Peach Pi 
>
> Yes, with those two patches, peach-pi display working on v3.18-rc5 for me.
>
>> with linux-next but
>> it fails to me with:
>>
>> exynos-mipi-video-phy 10040714.video-phy: can't request region for resource 
>> [mem 0x10040714-0x1004071f]
>>
>> The same issue was reported by Paolo a couple of days ago [1].
>
> For me, with linux-next, I'm still getting the DRM deadlock.  Trying
> your patch that moves things to module_init gets past the deadlock, but
> still doesn't boot unless I disable CONFIG_SND_SOC_SNOW.  Doing that I
> see the same video-phy error though.

Another interesting data point is that the 2 patches which get things
working on v3.18-rc5, when applied on Kukjin's for-next branch, result
in a kernel that boots (which is better than linux-next), but without
a working display. :(

Kevin

[RFC PATCH 1/1] drm/exynos: Move platform drivers registration to module init

[Kevin Hilman]

Javier Martinez Canillas  writes:

> [adding Paolo and Vivek as cc]
>
> Hello,
>
> On 11/18/2014 07:41 PM, Kevin Hilman wrote:
>> 
>> It fixes the DRM deadlock, issue for me on exynos5800-peach-pi, but then
>> it proceeds to panic in the workqueue code called by the asoc max98090
>> codec[1].
>> 
>> If I then disable CONFIG_SND_SOC_SNOW, I can get it to boot to a shell,
>> but I still don't have display output.
>> 
>
> Paolo Pisati pointed out in another thread that he needed the patch
> "[PATCH v2 2/2] arm: dts: Exynos5: Use pmu_system_controller phandle for dp 
> phy"
> is also needed to get display working for exynos on linux-next.
>
> I've pinged Kukjin to apply this as a -rc fix since is needed after
> a5ec598 ("phy: exynos-dp-video: Use syscon support to control pmu register")
> landed in 3.18 which broke the Exynos Display Port PHY:
>
> exynos-dp-video-phy 10040728.video-phy: Failed to lookup PMU regmap
>
> I've an Exynos5800 Peach Pi now so I wanted to test display on it. Just 
> $subject
> and [0] should be enough to have display working on Peach Pi 

Yes, with those two patches, peach-pi display working on v3.18-rc5 for me.

> with linux-next but
> it fails to me with:
>
> exynos-mipi-video-phy 10040714.video-phy: can't request region for resource 
> [mem 0x10040714-0x1004071f]
>
> The same issue was reported by Paolo a couple of days ago [1].

For me, with linux-next, I'm still getting the DRM deadlock.  Trying
your patch that moves things to module_init gets past the deadlock, but
still doesn't boot unless I disable CONFIG_SND_SOC_SNOW.  Doing that I
see the same video-phy error though.

Kevin

[PATCH V8 00/14] drm/exynos: few patches to enhance bridge chip support

[Kevin Hilman]

Paolo Pisati  writes:

> On Wed, Nov 19, 2014 at 10:35:40AM +0100, Javier Martinez Canillas wrote:
>> Hello Ajay,
>> 
>> On 11/18/2014 07:20 AM, Ajay kumar wrote:
>> > On Sat, Nov 15, 2014 at 3:24 PM, Ajay Kumar  
>> > wrote:
>> >> This series is based on master branch of Linus tree at:
>> >> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>> 
>> I applied your series on top of 3.18-rc5 + linux-next's commit:
>> 
>> 0ef76ae ("ARM: exynos_defconfig: Enable options for display panel support").
>> 
>> I think you should had mentioned what other patches are needed as a
>> dependency since I spent quite a bit of time figuring out why the
>> ps8622 bridge driver probe was always deferred due of_drm_find_panel()
>> failing and then I noticed that a patch from linux-next was needed:
>> 
>> e35e305 ("drm/panel: simple: Add AUO B116XW03 panel support")
>> 
>> With that commit the ps8622 drm bridge driver probe succeed but I still
>> don't have display working on an Exynos5240 Peach Pit, the kernel log shows:
>> 
>> platform 145b.dp-controller: Driver exynos-dp requests probe deferral
>> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops)
>> exynos-drm exynos-drm: failed to bind 145b.dp-controller (ops 
>> exynos_dp_ops): -517
>> exynos-drm exynos-drm: master bind failed: -517
>> platform 145b.dp-controller: Driver exynos-dp requests probe deferral
>
> do you have this in your dmesg?
>
> ...
> exynos-dp-video-phy 10040728.video-phy: Failed to lookup PMU regmap
> ...
>
> to get a working framebuffer out of linus 318rc4 on my peach pi, i had
> to cherry pick these 3 patches:
>
> arm: dts: Exynos5: Use pmu_system_controller phandle for dp phy
> drm/exynos: dp: Remove support for unused dptx-phy

Excellent, thank you for pointing these out.  I've been struggling to
get the display working on v3.18-rc and had missed these.

> ARM: exynos_defconfig: Enable options for display panel support
>
> Check if you have those.

With the 3 patches you mention, the display is working on my peach-pi as
well.

Thanks,

Kevin

[RFC PATCH 1/1] drm/exynos: Move platform drivers registration to module init

[Kevin Hilman]

Gustavo Padovan  writes:

> 2014-11-18 Kevin Hilman :
>
>> Javier Martinez Canillas  writes:
>> 
>> > The Exynos DRM driver register its sub-devices platform drivers in
>> > the probe function but after commit 43c0767 ("of/platform: Move
>> > platform devices under /sys/devices/platform"), this is causing
>> > a deadlock in __driver_attach(). Fix this by moving the platform
>> > drivers registration to exynos_drm_init().
>> >
>> > Suggested-by: Andrzej Hajda 
>> > Signed-off-by: Javier Martinez Canillas > > collabora.co.uk>
>> > ---
>> >
>> > This issue was reported by both Krzysztof Kozlowski [0] and Kevin Hilman 
>> > [1].
>> >
>> > Inki Dae said that he will fix it property by separating the Exynos DRM
>> > driver in different sub-modules but I post this patch as RFC anyways so
>> > others can test if this fixes their boot issue.
>> 
>> It fixes the DRM deadlock, issue for me on exynos5800-peach-pi, but then
>> it proceeds to panic in the workqueue code called by the asoc max98090
>> codec[1].
>> 
>> If I then disable CONFIG_SND_SOC_SNOW, I can get it to boot to a shell,
>> but I still don't have display output.
>> 
>> Is anyone at Samsung testing linux-next?  If so, on what platforms?  It
>> would really be nice if your linux-next work was tested on these
>> publically-available 542x/5800 platforms (peach-pi, peach-pit,
>> odroid-xu3) which would also allow lots of others to help you test and
>> validate.
>
> It would also be good to add drm-exynos-next to the daily linux-next build.
> Currently drm-exynos-next is ahead of linux-next. This patch from Javier for
> example only applies on linux-next.

Which tree is the drm-exynos-next branch in?

Kevin

[RFC PATCH 1/1] drm/exynos: Move platform drivers registration to module init

[Kevin Hilman]

Javier Martinez Canillas  writes:

> The Exynos DRM driver register its sub-devices platform drivers in
> the probe function but after commit 43c0767 ("of/platform: Move
> platform devices under /sys/devices/platform"), this is causing
> a deadlock in __driver_attach(). Fix this by moving the platform
> drivers registration to exynos_drm_init().
>
> Suggested-by: Andrzej Hajda 
> Signed-off-by: Javier Martinez Canillas 
> ---
>
> This issue was reported by both Krzysztof Kozlowski [0] and Kevin Hilman [1].
>
> Inki Dae said that he will fix it property by separating the Exynos DRM
> driver in different sub-modules but I post this patch as RFC anyways so
> others can test if this fixes their boot issue.

It fixes the DRM deadlock, issue for me on exynos5800-peach-pi, but then
it proceeds to panic in the workqueue code called by the asoc max98090
codec[1].

If I then disable CONFIG_SND_SOC_SNOW, I can get it to boot to a shell,
but I still don't have display output.

Is anyone at Samsung testing linux-next?  If so, on what platforms?  It
would really be nice if your linux-next work was tested on these
publically-available 542x/5800 platforms (peach-pi, peach-pit,
odroid-xu3) which would also allow lots of others to help you test and
validate.

Kevin

[RFC 1/2] PM / Domains: Power on domain early during system resume

[Kevin Hilman]

Andrzej Hajda  writes:

> On 10/30/2014 08:36 AM, Krzysztof Kozlowski wrote:
>> On śro, 2014-10-29 at 10:46 -0700, Kevin Hilman wrote:
>>> Krzysztof Kozlowski  writes:
>>>
>>>> When resuming the system the power domain has to be powered on early so
>>>> any runtime PM aware devices could resume.
>>>>
>>>> This fixes following scenario reproduced on Exynos DRM:
>>>> 1. Power domain is off before suspending the system.
>>>> 2. System is suspended to RAM.
>>>> 3. Resuming starts. The Exynos DRM driver resume callback is called.
>>>> 4. The Exynos DRM driver calls drm_helper_resume_force_mode which turns
>>>>the screen on by calling exynos_dsi_dpms with DRM_MODE_DPMS_ON.
>>> Dumb Q: if the device (and power domain) were off before (and during)
>>> suspend, why are they being resumed?
>>>
>>> Shouldn't the resume path restore things to the same state they were
>>> before suspend?
>> One could expect that... but the Exynos DRM driver behaves differently
>> (and some other drivers also). In resume method it calls
>> drm_helper_resume_force_mode() which forces restoring mode setting
>> configuration. Apparently setting a mode needs DPMS on:
>> static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>> {
>>  ...
>>  exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>>  ...
>>
>> The previous DPMS status (status during suspend) is completely ignored
>> here.
>
> Suspend callback switches off all connectors (thus all other devs in
> their pipeline) by calling dpms_off,
> in restore callback all devs are restored to their previous state by
> calling appropriate dpms.
> So I guess drm_helper_resume_force_mode() call at the end of resume is
> incorrect.

Though I'm not terribly familiar with DRM, it seems incorrect because I
expect resume to restore the state of things when suspend happened, not
forcibly resume everything.

> On the other side it is present in many other drivers, so I am also
> little bit confused.

Many other DRM drivers?  or other drivers too?

Kevin

[RFC 1/2] PM / Domains: Power on domain early during system resume

[Kevin Hilman]

Krzysztof Kozlowski  writes:

> When resuming the system the power domain has to be powered on early so
> any runtime PM aware devices could resume.
>
> This fixes following scenario reproduced on Exynos DRM:
> 1. Power domain is off before suspending the system.
> 2. System is suspended to RAM.
> 3. Resuming starts. The Exynos DRM driver resume callback is called.
> 4. The Exynos DRM driver calls drm_helper_resume_force_mode which turns
>the screen on by calling exynos_dsi_dpms with DRM_MODE_DPMS_ON.

Dumb Q: if the device (and power domain) were off before (and during)
suspend, why are they being resumed?

Shouldn't the resume path restore things to the same state they were
before suspend?

Kevin