Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <837bcf1d-e59f-5990-29e2-2b64d3dfaaa1@ti.com>
Date:   Wed, 19 Oct 2022 22:44:19 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: [RFC PATCH v5 5/6] drm/tidss: Add IO CTRL and Power support for
 OLDI TX in am625
Content-Language: en-US
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
        Jyri Sarha <jyri.sarha@iki.fi>,
        Rob Herring <robh+dt@kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC:     Nishanth Menon <nm@ti.com>, Vignesh Raghavendra <vigneshr@ti.com>,
        Rahul T R <r-ravikumar@ti.com>,
        DRI Development List <dri-devel@lists.freedesktop.org>,
        Devicetree List <devicetree@vger.kernel.org>,
        Linux Kernel List <linux-kernel@vger.kernel.org>
References: <20220928175223.15225-1-a-bhatia1@ti.com>
 <20220928175223.15225-6-a-bhatia1@ti.com>
 <708ae70e-dc1b-1079-8442-06cbea228e99@ideasonboard.com>
From:   Aradhya Bhatia <a-bhatia1@ti.com>
In-Reply-To: <708ae70e-dc1b-1079-8442-06cbea228e99@ideasonboard.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Tomi

On 12-Oct-22 17:59, Tomi Valkeinen wrote:
> On 28/09/2022 20:52, Aradhya Bhatia wrote:
>> The ctrl mmr module of the AM625 is different from the AM65X SoC. Thus
>> the ctrl mmr registers that supported the OLDI TX power have become
>> different in AM625 SoC.
>>
>> Add IO CTRL support and control the OLDI TX power for AM625.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c      | 55 ++++++++++++++++++------
>>   drivers/gpu/drm/tidss/tidss_dispc_regs.h |  6 +++
>>   2 files changed, 49 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 88008ad39b55..68444e0cd8d7 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -921,21 +921,52 @@ int dispc_vp_bus_check(struct dispc_device 
>> *dispc, u32 hw_videoport,
>>   static void dispc_oldi_tx_power(struct dispc_device *dispc, bool power)
>>   {
>> -    u32 val = power ? 0 : OLDI_PWRDN_TX;
>> +    u32 val;
>>       if (WARN_ON(!dispc->oldi_io_ctrl))
>>           return;
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> -    regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
>> -               OLDI_PWRDN_TX, val);
>> +    if (dispc->feat->subrev == DISPC_AM65X) {
>> +        val = power ? 0 : OLDI_PWRDN_TX;
>> +
>> +        regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT0_IO_CTRL,
>> +                   OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT1_IO_CTRL,
>> +                   OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT2_IO_CTRL,
>> +                   OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, OLDI_DAT3_IO_CTRL,
>> +                   OLDI_PWRDN_TX, val);
>> +        regmap_update_bits(dispc->oldi_io_ctrl, OLDI_CLK_IO_CTRL,
>> +                   OLDI_PWRDN_TX, val);
>> +
>> +    } else if (dispc->feat->subrev == DISPC_AM625) {
>> +        if (power) {
>> +            switch (dispc->oldi_mode) {
>> +            case OLDI_SINGLE_LINK_SINGLE_MODE:
>> +                /* Power down OLDI TX 1 */
>> +                val = OLDI1_PWRDN_TX;
>> +                break;
>> +
>> +            case OLDI_SINGLE_LINK_CLONE_MODE:
>> +            case OLDI_DUAL_LINK_MODE:
>> +                /* No Power down */
>> +                val = 0;
>> +                break;
>> +
>> +            default:
>> +                /* Power down both the OLDI TXes */
>> +                val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
>> +                break;
>> +            }
>> +        } else {
>> +            /* Power down both the OLDI TXes */
>> +            val = OLDI0_PWRDN_TX | OLDI1_PWRDN_TX;
>> +        }
> 
> Ugh, I hate power-down bits. So you "enable" it to disable it =). What's 
> the default value or the register here? Or will this always be called? 
> I.e. if we only use DPI, do we power down the OLDIs somewhere (or does 
> it matter)?
> 

The power bits are defaulted to keep the OLDI TXes powered off, and they
have to be turned ON.

This function is also called to power off the OLDI TXes, from
dispc_vp_unprepare. And that happens with "power" variable as false. So
the else condition above is indeed required.

You are right about the other scenario though. If DPI is only used, the
mode will be set to OLDI_MODE_OFF and in that case, the dispc_vp_prepare
function will NOT be called for the OLDI VP thereby rendering the
"default" clause of the switch statement, for powering down the OLDIs,
essentially useless. I just put it there because of convention.

>> +
>> +        regmap_update_bits(dispc->oldi_io_ctrl, OLDI_PD_CTRL,
>> +                   OLDI0_PWRDN_TX | OLDI1_PWRDN_TX, val);
>> +    }
>>   }
>>   static void dispc_set_num_datalines(struct dispc_device *dispc,
>> @@ -2831,7 +2862,7 @@ int dispc_init(struct tidss_device *tidss)
>>           dispc->vp_data[i].gamma_table = gamma_table;
>>       }
>> -    if (feat->subrev == DISPC_AM65X) {
>> +    if (feat->subrev == DISPC_AM65X || feat->subrev == DISPC_AM625) {
>>           r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
>>           if (r)
>>               return r;
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc_regs.h 
>> b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> index 13feedfe5d6d..510bee70b3b8 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc_regs.h
>> @@ -238,6 +238,12 @@ enum dispc_common_regs {
>>   #define OLDI_DAT3_IO_CTRL            0x0C
>>   #define OLDI_CLK_IO_CTRL            0x10
>> +/* Only for AM625 OLDI TX */
>> +#define OLDI_PD_CTRL                0x100
>> +#define OLDI_LB_CTRL                0x104
>> +
>>   #define OLDI_PWRDN_TX                BIT(8)
>> +#define OLDI0_PWRDN_TX                BIT(0)
>> +#define OLDI1_PWRDN_TX                BIT(1)
> 
> Maybe these (the new and old ones) should be platform-prefixed. And 
> organized so that the register and its bits are together.
Okay, will do.


Regards
Aradhya