linux-kernel - Re: [PATCH v14 1/2] drm: add kms driver for loongson display controller

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <588a03c7-ae1f-f449-752d-aa94cc1ab491@xen0n.name>
Date:   Mon, 22 May 2023 17:05:30 +0800
From:   WANG Xuerui <kernel@...0n.name>
To:     Sui Jingfeng <15330273260@....cn>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Sui Jingfeng <suijingfeng@...ngson.cn>,
        Li Yi <liyi@...ngson.cn>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian Koenig <christian.koenig@....com>,
        Emil Velikov <emil.l.velikov@...il.com>
Cc:     linaro-mm-sig@...ts.linaro.org, loongson-kernel@...ts.loongnix.cn,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Javier Martinez Canillas <javierm@...hat.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Liu Peibao <liupeibao@...ngson.cn>, linux-media@...r.kernel.org
Subject: Re: [PATCH v14 1/2] drm: add kms driver for loongson display
 controller

On 2023/5/22 16:51, Sui Jingfeng wrote:
> Hi,
> 
> On 2023/5/21 20:21, WANG Xuerui wrote:
>>
>>> <snip>
>>> +
>>> +static void lsdc_crtc0_soft_reset(struct lsdc_crtc *lcrtc)
>>> +{
>>> +    struct lsdc_device *ldev = lcrtc->ldev;
>>> +    u32 val;
>>> +
>>> +    val = lsdc_rreg32(ldev, LSDC_CRTC0_CFG_REG);
>>> +
>>> +    val &= CFG_VALID_BITS_MASK;
>>> +
>>> +    /* soft reset bit, active low */
>>> +    val &= ~CFG_RESET_N;
>>> +
>>> +    val &= ~CFG_PIX_FMT_MASK;
>>> +
>>> +    lsdc_wreg32(ldev, LSDC_CRTC0_CFG_REG, val);
>>> +
>>> +    udelay(5);
>>> +
>>> +    val |= CFG_RESET_N | LSDC_PF_XRGB8888 | CFG_OUTPUT_ENABLE;
>>> +
>>> +    lsdc_wreg32(ldev, LSDC_CRTC0_CFG_REG, val);
>>> +
>>> +    mdelay(20);
>>> +}
>>> +
>>> +static void lsdc_crtc1_soft_reset(struct lsdc_crtc *lcrtc)
>>> +{
>>> +    struct lsdc_device *ldev = lcrtc->ldev;
>>> +    u32 val;
>>> +
>>> +    val = lsdc_rreg32(ldev, LSDC_CRTC1_CFG_REG);
>>> +
>>> +    val &= CFG_VALID_BITS_MASK;
>>> +
>>> +    /* soft reset bit, active low */
>>> +    val &= ~CFG_RESET_N;
>>> +
>>> +    val &= ~CFG_PIX_FMT_MASK;
>>> +
>>> +    lsdc_wreg32(ldev, LSDC_CRTC1_CFG_REG, val);
>>> +
>>> +    udelay(5);
>>> +
>>> +    val |= CFG_RESET_N | LSDC_PF_XRGB8888 | CFG_OUTPUT_ENABLE;
>>> +
>>> +    lsdc_wreg32(ldev, LSDC_CRTC1_CFG_REG, val);
>>> +
>>> +    msleep(20);
>>
>> So many magic sleeps without documentation?
>>
> It is just that you should wait the device for a while before it can 
> reaction when doing the soft reset.
> 
> I think this is engineering...

As an engineer myself, I fully concur with this, but I mainly wanted 
some explanation as to "why 5 there? why 20 here? why 9 there?" -- where 
did all the discrete values come from, implied by HDL or found out by 
experimentations? Can these be extracted to properly named constants? 
Can some of the values get coalesced into one without harming 
functionality? Can some of them get shorter? -- questions like this.

> <snip>
>>> +
>>> +/* All loongson display controller support scanout position hardware */
>>
>> Commit message implies only 7A2000+ LSDC IPs have the "scanout 
>> position recorders". Either that part or this code would need tweaking... 
> 
> Both LS7A2000 and LS7A1000 have the scanout position recorders hardware.
> 
> Preciously, datasheet of LS7A1000 didn't told us if it support this 
> feature.
> 
> I will adjust the commit message at next version, the code doesn't need 
> change.

That's fine, the intent is always making the code more approachable and 
maintainable. Thanks.

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/