Hi Chanho,
2016ë
02ì 12ì¼ 16:39ì Chanho Park ì´(ê°) ì´ ê¸:
> Hi,
>
> On Fri, Feb 12, 2016 at 1:56 PM, Inki Dae <inki.dae at samsung.com> wrote:
>> Hi Chanho,
>>
>> There was a missing one so below is my review again.
>>
>> 2016ë
02ì 11ì¼ 22:59ì Chanho Park ì´(ê°) ì´ ê¸:
>>> Hi Inki,
>>>
>>> On Thu, Feb 11, 2016 at 7:37 PM, Inki Dae <inki.dae at samsung.com> wrote:
>>>> Hi Chanho,
>>>>
>>>> 2016ë
01ì 30ì¼ 22:58ì Chanho Park ì´(ê°) ì´ ê¸:
>>>>> From: Chanho Park <chanho61.park at samsung.com>
>>>>>
>>>>> This patch adds a mic_bypass option to bypass the mic
>>>>> from display out path. The mic(Mobile image compressor) compresses
>>>>> RGB data from fimd and send the compressed data to the mipi dsi.
>>>>> The bypass option can be founded from system register and the bit
>>>>> of the option is 11.
>>>>>
>>>>> Cc: Inki Dae <inki.dae at samsung.com>
>>>>> Cc: Joonyoung Shim <jy0922.shim at samsung.com>
>>>>> Cc: Seung-Woo Kim <sw0312.kim at samsung.com>
>>>>> Signed-off-by: Chanho Park <chanho61.park at samsung.com>
>>>>> ---
>>>>> .../devicetree/bindings/display/exynos/samsung-fimd.txt | 2 ++
>>>>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 14
>>>>> ++++++++++++++
>>>>> 2 files changed, 16 insertions(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>> b/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>> index 27c3ce0..7f90c4a 100644
>>>>> --- a/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>> +++ b/Documentation/devicetree/bindings/display/exynos/samsung-fimd.txt
>>>>> @@ -45,6 +45,8 @@ Optional Properties:
>>>>> Can be used in case timings cannot be provided otherwise
>>>>> or to override timings provided by the panel.
>>>>> - samsung,sysreg: handle to syscon used to control the system registers
>>>>> +- samsung,mic-bypass: bypass mic(mobile image compressor) from display
>>>>> path.
>>
>> mic-bypass is not really common property for fimd family so it's not correct
>> for fimd device node has mic-bypass property.
>
> According to user manual of exynos, the option has been introduced
> since exynos5420.
That is why samsung-fimd.txt is not correct place. samsung-fimd.txt describes
hardware information for s3c24xx, s3c64xx also, which don't have mic ip.
> The exynos5420 and exynos5422(5800) have a fimd controller not decon
> display controller.
>
>>
>>>>> + This option is only available since exynos5420.
>>>>> - i80-if-timings: timing configuration for lcd i80 interface support.
>>>>> - cs-setup: clock cycles for the active period of address signal is
>>>>> enabled
>>>>> until chip select is enabled.
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>> index 70194d0..4fb2952 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>>> @@ -94,6 +94,7 @@ struct fimd_driver_data {
>>>>> unsigned int lcdblk_offset;
>>>>> unsigned int lcdblk_vt_shift;
>>>>> unsigned int lcdblk_bypass_shift;
>>>>> + unsigned int lcdblk_mic_bypass_shift;
>>>>>
>>>>> unsigned int has_shadowcon:1;
>>>>> unsigned int has_clksel:1;
>>>>> @@ -140,6 +141,7 @@ static struct fimd_driver_data
>>>>> exynos5_fimd_driver_data = {
>>>>> .lcdblk_offset = 0x214,
>>>>> .lcdblk_vt_shift = 24,
>>>>> .lcdblk_bypass_shift = 15,
>>>>> + .lcdblk_mic_bypass_shift = 11,
>>>>> .has_shadowcon = 1,
>>>>> .has_vidoutcon = 1,
>>>>> .has_vtsel = 1,
>>>>> @@ -162,6 +164,7 @@ struct fimd_context {
>>>>> u32 i80ifcon;
>>>>> bool i80_if;
>>>>> bool suspended;
>>>>> + bool mic_bypass;
>>>>> int pipe;
>>>>> wait_queue_head_t wait_vsync_queue;
>>>>> atomic_t wait_vsync_event;
>>>>> @@ -461,6 +464,14 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
>>>>> return;
>>>>> }
>>>>>
>>>>> + if (ctx->mic_bypass && ctx->sysreg &&
>>>>> regmap_update_bits(ctx->sysreg,
>>>>> + driver_data->lcdblk_offset,
>>>>> + 0x1 << driver_data->lcdblk_mic_bypass_shift,
>>>>> + 0x1 <<
>>>>> driver_data->lcdblk_mic_bypass_shift)) {
>>>>> + DRM_ERROR("Failed to update sysreg for bypass mic.\n");
>>>>> + return;
>>>>> + }
>>>>
>>>> It'd better to consider mic path also because mic bypass bit of lcdblk
>>>> could be true by bootloader. In this case, fimd driver wouldn't do
>>>> anything if mic_bypass property isn't declared but the mic_bypass bit of
>>>> lcdblk register is still set to 1.
>>>
>>> Actually, I wanted to set the bit on kernel side even though it's not
>>> assigned from bootloader.
>>> If the bootloader already set the bit, that means mic will be by-pass
>>> and we don't care about it from kernel side.
>>> The option is useful when I want to skip the mic on the kernel side.
>>>
>>>>
>>>> For this, I think you could check lcdblk_mic_pypass_shift to identify
>>>> whether mic path is supported or not like below,
>>>> if (ctx->lcdblk_mic_bypass_shift) {
>>>
>>> It causes all exynos5 fimd driver skips the mic from display path.
>>> How about below patch instead of this?
>>>
>>> + if (of_property_read_bool(dev->of_node, "samsung,mic-bypass") &&
>>> + ctx->driver_data->lcdblk_mic_bypass_shift)
>>> + ctx->mic_bypass = true;
>>
>> So let's bypass mic path in default like lcdblk_bypass,
>
> I'm not sure why the option is default for exynos542x fimd controller.
> The reset value of the bit is 0. That means the mic bypass is not a
> default option on chip-level.
That is because Exynos drm is not ready for mic support fully. As of now,
Exynos drm uses mic driver only for Exynos5433 SoC but not other SoC.
Do you know that? FIMDBYPASS bit of lcdblk register has 0 - Reserved not FIMD
bypass(in case of Exynos5422) in default.
>
>> /* set mic bypass selection */
>> if (ctx->has_mic_bypass && ctx->sysreg &&
>> regmap_update_bits(ctx->sysreg,
>> driver_data->lcdblk_offset,
>> 0x1 << driver_data->lcdblk_mic_bypass_shift,
>> 0x1 <<
>> driver_data->lcdblk_mic_bypass_shift)) {
>> DRM_ERROR("Failed to update sysreg for mic bypass
>> setting.\n");
>> return;
>> }
>>
>> For this, I added a new member, has_mic_by_pass, which means that lcdblk
>> register has mic_bypass bit like has_vtsel.
> Hmm. Like lcd_blk_bypass_shift, the option is configurable because
> mic_by_pass_shift is not zero.
> I think we don't need the new has_mic_bypass option.
First of all, it seems that adding mic_bypass property to fimd device node is
really wrong. If so, how can you let fimd driver know whether now SoC has mic
bypass bit?
>
>>
>> We could consider display path such as mic and mDNIe later using of_graph
>> concept below,
>> Documentation/devicetree/bindings/graph.txt
>
> Yes. To use the mic on display path, we'll define the graph in the device
> tree.
> In case of not using the mic, however, we'll not define any graphs in the dts.
However, fimd driver should know whether now SoC has mic bypass bit because
fimd driver is used for other SoC also, which have no mic ip.
Thanks,
Inki Dae
>
> I intended this patch is just providing configurable option from
> device tree to skip mic from display path.
>
