| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 19 Oct 2022 22:44:19 +0530
From: Aradhya Bhatia <a-bhatia1@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Jyri Sarha <jyri.sarha@....fi>,
Rob Herring <robh+dt@...nel.org>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
CC: Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
Rahul T R <r-ravikumar@...com>,
DRI Development List <dri-devel@...ts.freedesktop.org>,
Devicetree List <devicetree@...r.kernel.org>,
Linux Kernel List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v5 5/6] drm/tidss: Add IO CTRL and Power support for
OLDI TX in am625
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@...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
Powered by blists - more mailing lists