Re: [PATCH v11 04/16] dt-bindings: display: mediatek: dsc: add yaml for mt8195 SoC binding

Re: [PATCH v11 04/16] dt-bindings: display: mediatek: dsc: add yaml for mt8195 SoC binding

[Chun-Kuang Hu]

Hi, Jason:

jason-jh.lin <jason-jh.lin@mediatek.com> 於 2021年9月21日 週二 下午11:52寫道：
>
> 1. Add mediatek,dsc.yaml to describe DSC module in details.
> 2. Add mt8195 SoC binding to mediatek,dsc.yaml.

Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>

>
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> ---
>  .../display/mediatek/mediatek,dsc.yaml        | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml
> new file mode 100644
> index 000000000000..1ec083eff824
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsc.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,dsc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: mediatek display DSC controller
> +
> +maintainers:
> +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> +  - Philipp Zabel <p.zabel@pengutronix.de>
> +
> +description: |
> +  The DSC standard is a specification of the algorithms used for
> +  compressing and decompressing image display streams, including
> +  the specification of the syntax and semantics of the compressed
> +  video bit stream. DSC is designed for real-time systems with
> +  real-time compression, transmission, decompression and Display.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: mediatek,mt8195-disp-dsc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: DSC Wrapper Clock
> +
> +  power-domains:
> +    description: A phandle and PM domain specifier as defined by bindings of
> +      the power controller specified by phandle. See
> +      Documentation/devicetree/bindings/power/power-domain.yaml for details.
> +
> +  mediatek,gce-client-reg:
> +    description:
> +      The register of client driver can be configured by gce with 4 arguments
> +      defined in this property, such as phandle of gce, subsys id,
> +      register offset and size.
> +      Each subsys id is mapping to a base address of display function blocks
> +      register which is defined in the gce header
> +      include/dt-bindings/gce/<chip>-gce.h.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - power-domains
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    dsc0: disp_dsc_wrap@1c009000 {
> +        compatible = "mediatek,mt8195-disp-dsc";
> +        reg = <0 0x1c009000 0 0x1000>;
> +        interrupts = <GIC_SPI 645 IRQ_TYPE_LEVEL_HIGH 0>;
> +        power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS0>;
> +        clocks = <&vdosys0 CLK_VDO0_DSC_WRAP0>;
> +        mediatek,gce-client-reg = <&gce1 SUBSYS_1c00XXXX 0x9000 0x1000>;
> +    };
> --
> 2.18.0
>

Re: [PATCH v11 05/16] dt-bindings: display: mediatek: merge: add additional prop for mt8195

[Chun-Kuang Hu]

Hi, Jason:

jason-jh.lin <jason-jh.lin@mediatek.com> 於 2021年9月21日 週二 下午11:52寫道：
>
> add MERGE additional properties description for mt8195:
> 1. async clock
> 2. fifo setting enable
> 3. reset controller
>
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> ---
>  .../display/mediatek/mediatek,merge.yaml      | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
> index 75beeb207ceb..542dd7137d3b 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
> @@ -38,6 +38,19 @@ properties:
>    clocks:
>      items:
>        - description: MERGE Clock
> +      - description: MERGE Async Clock
> +          Controlling the synchronous process between MERGE and other display
> +          function blocks cross clock domain.
> +
> +  mediatek,merge-fifo-en:
> +    description:
> +      The setting of merge fifo is mainly provided for the display latency
> +      buffer to ensure that the back-end panel display data will not be
> +      underrun, a little more data is needed in the fifo.
> +      According to the merge fifo settings, when the water level is detected
> +      to be insufficient, it will trigger RDMA sending ultra and preulra
> +      command to SMI to speed up the data rate.
> +    type: boolean
>
>    mediatek,gce-client-reg:
>      description:
> @@ -50,6 +63,11 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1
>
> +  resets:
> +    description: reset controller
> +      See Documentation/devicetree/bindings/reset/reset.txt for details.
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -67,3 +85,16 @@ examples:
>          power-domains = <&spm MT8173_POWER_DOMAIN_MM>;
>          clocks = <&mmsys CLK_MM_DISP_MERGE>;
>      };
> +
> +    merge5: disp_vpp_merge5@1c110000 {
> +        compatible = "mediatek,mt8195-disp-merge";
> +        reg = <0 0x1c110000 0 0x1000>;
> +        interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH 0>;
> +        clocks = <&vdosys1 CLK_VDO1_VPP_MERGE4>,
> +                 <&vdosys1 CLK_VDO1_MERGE4_DL_ASYNC>;
> +        clock-names = "merge","merge_async";

Define clock-names first.

Regards,
Chun-Kuang.

> +        power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS1>;
> +        mediatek,gce-client-reg = <&gce1 SUBSYS_1c11XXXX 0x0000 0x1000>;
> +        mediatek,merge-fifo-en = <1>;
> +        resets = <&vdosys1 MT8195_VDOSYS1_SW0_RST_B_MERGE4_DL_ASYNC>;
> +    };
> --
> 2.18.0
>

Re: [PATCH v11 07/16] dt-bindings: arm: mediatek: move common module from display folder

[Chun-Kuang Hu]

Hi, Jason:

jason-jh.lin <jason-jh.lin@mediatek.com> 於 2021年9月21日 週二 下午11:52寫道：
>
> AAL, COLOR, CCORR, MUTEX, WDMA could be used by other modules,
> such as MDP, so move their binding document into the common folder.

Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>

>
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> ---
>  .../{display => arm}/mediatek/mediatek,aal.yaml      |  9 ++++-----
>  .../{display => arm}/mediatek/mediatek,ccorr.yaml    |  9 ++++-----
>  .../{display => arm}/mediatek/mediatek,color.yaml    | 11 +++++------
>  .../{display => arm}/mediatek/mediatek,mutex.yaml    | 12 +++++-------
>  .../{display => arm}/mediatek/mediatek,wdma.yaml     |  9 ++++-----
>  5 files changed, 22 insertions(+), 28 deletions(-)
>  rename Documentation/devicetree/bindings/{display => arm}/mediatek/mediatek,aal.yaml (88%)
>  rename Documentation/devicetree/bindings/{display => arm}/mediatek/mediatek,ccorr.yaml (87%)
>  rename Documentation/devicetree/bindings/{display => arm}/mediatek/mediatek,color.yaml (86%)
>  rename Documentation/devicetree/bindings/{display => arm}/mediatek/mediatek,mutex.yaml (85%)
>  rename Documentation/devicetree/bindings/{display => arm}/mediatek/mediatek,wdma.yaml (90%)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,aal.yaml
> similarity index 88%
> rename from Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> rename to Documentation/devicetree/bindings/arm/mediatek/mediatek,aal.yaml
> index 311bbf05a967..ab6eb9b550a4 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,aal.yaml
> @@ -1,17 +1,16 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/display/mediatek/mediatek,aal.yaml#
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,aal.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Mediatek display adaptive ambient light processor
> +title: MediaTek adaptive ambient light processor
>
>  maintainers:
> -  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> -  - Philipp Zabel <p.zabel@pengutronix.de>
> +  - Matthias Brugger <matthias.bgg@gmail.com>
>
>  description: |
> -  Mediatek display adaptive ambient light processor, namely AAL,
> +  MediaTek adaptive ambient light processor, namely AAL,
>    is responsible for backlight power saving and sunlight visibility improving.
>    AAL device node must be siblings to the central MMSYS_CONFIG node.
>    For a description of the MMSYS_CONFIG binding, see
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,ccorr.yaml
> similarity index 87%
> rename from Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> rename to Documentation/devicetree/bindings/arm/mediatek/mediatek,ccorr.yaml
> index 60752ce45d49..de86e9ae35f3 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,ccorr.yaml
> @@ -1,17 +1,16 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/display/mediatek/mediatek,ccorr.yaml#
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,ccorr.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Mediatek display color correction
> +title: MediaTek color correction
>
>  maintainers:
> -  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> -  - Philipp Zabel <p.zabel@pengutronix.de>
> +  - Matthias Brugger <matthias.bgg@gmail.com>
>
>  description: |
> -  Mediatek display color correction, namely CCORR, reproduces correct color
> +  MediaTek color correction, namely CCORR, reproduces correct color
>    on panels with different color gamut.
>    CCORR device node must be siblings to the central MMSYS_CONFIG node.
>    For a description of the MMSYS_CONFIG binding, see
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,color.yaml
> similarity index 86%
> rename from Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> rename to Documentation/devicetree/bindings/arm/mediatek/mediatek,color.yaml
> index f6636869909c..73be301b50d2 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,color.yaml
> @@ -1,18 +1,17 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/display/mediatek/mediatek,color.yaml#
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,color.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Mediatek display color processor
> +title: MediaTek color processor
>
>  maintainers:
> -  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> -  - Philipp Zabel <p.zabel@pengutronix.de>
> +  - Matthias Brugger <matthias.bgg@gmail.com>
>
>  description: |
> -  Mediatek display color processor, namely COLOR, provides hue, luma and
> -  saturation adjustments to get better picture quality and to have one panel
> +  MediaTek color processor, namely COLOR, provides hue, luma and saturation
> +  adjustments to get better picture quality and to have one panel
>    resemble the other in their output characteristics.
>    COLOR device node must be siblings to the central MMSYS_CONFIG node.
>    For a description of the MMSYS_CONFIG binding, see
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,mutex.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mutex.yaml
> similarity index 85%
> rename from Documentation/devicetree/bindings/display/mediatek/mediatek,mutex.yaml
> rename to Documentation/devicetree/bindings/arm/mediatek/mediatek,mutex.yaml
> index 6eca525eced0..713c7485e11a 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,mutex.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mutex.yaml
> @@ -1,19 +1,17 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/display/mediatek/mediatek,mutex.yaml#
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,mutex.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Mediatek mutex
> +title: MediaTek mutex
>
>  maintainers:
> -  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> -  - Philipp Zabel <p.zabel@pengutronix.de>
> +  - Matthias Brugger <matthias.bgg@gmail.com>
>
>  description: |
> -  Mediatek mutex, namely MUTEX, is used to send the triggers signals called
> -  Start Of Frame (SOF) / End Of Frame (EOF) to each sub-modules on the display
> -  data path or MDP data path.
> +  MediaTek mutex, namely MUTEX, is used to send the triggers signals called
> +  Start Of Frame(SOF) / End Of Frame(EOF) to each sub-modules on the data path.
>    In some SoC, such as mt2701, MUTEX could be a hardware mutex which protects
>    the shadow register.
>    MUTEX device node must be siblings to the central MMSYS_CONFIG node.
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,wdma.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,wdma.yaml
> similarity index 90%
> rename from Documentation/devicetree/bindings/display/mediatek/mediatek,wdma.yaml
> rename to Documentation/devicetree/bindings/arm/mediatek/mediatek,wdma.yaml
> index 25f9a63fe7af..5222535d98c6 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,wdma.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,wdma.yaml
> @@ -1,17 +1,16 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/display/mediatek/mediatek,wdma.yaml#
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,wdma.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Mediatek display WDMA
> +title: MediaTek WDMA
>
>  maintainers:
> -  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> -  - Philipp Zabel <p.zabel@pengutronix.de>
> +  - Matthias Brugger <matthias.bgg@gmail.com>
>
>  description: |
> -  Mediatek display WDMA stands for Write Direct Memory Access.
> +  MediaTek WDMA stands for Write Direct Memory Access.
>    It can write the data in display pipeline into DRAM.
>    WDMA device node must be siblings to the central MMSYS_CONFIG node.
>    For a description of the MMSYS_CONFIG binding, see
> --
> 2.18.0
>

Re: [PATCH v11 09/16] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

[AngeloGioacchino Del Regno]

> Add mt8195 vdosys0 clock driver name and routing table to
> the driver data of mtk-mmsys.
> 
> Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com>
> ---
> This patch is base on [1]
> [1] soc: mediatek: mmsys: add mt8192 mmsys support
> - https://patchwork.kernel.org/project/linux-mediatek/list/?series=524857
> 
> The vdosys1 impelmentation patch [2]
> [2] soc: mediatek: add mtk-mmsys support for mt8195 vdosys1
> - https://patchwork.kernel.org/project/linux-mediatek/patch/20210906071539.12953-7-nancy.lin@mediatek.com/
> ---

Hello Jason,
thanks for the patch! However, there are a few things to improve:


>   drivers/soc/mediatek/mt8195-mmsys.h    | 114 +++++++++++++++++++++++++
>   drivers/soc/mediatek/mtk-mmsys.c       |  11 +++
>   include/linux/soc/mediatek/mtk-mmsys.h |   9 ++
>   3 files changed, 134 insertions(+)
>   create mode 100644 drivers/soc/mediatek/mt8195-mmsys.h
> 
> diff --git a/drivers/soc/mediatek/mt8195-mmsys.h b/drivers/soc/mediatek/mt8195-mmsys.h
> new file mode 100644
> index 000000000000..0c97a5f016c1
> --- /dev/null
> +++ b/drivers/soc/mediatek/mt8195-mmsys.h
> @@ -0,0 +1,114 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __SOC_MEDIATEK_MT8195_MMSYS_H
> +#define __SOC_MEDIATEK_MT8195_MMSYS_H
> +
> +#define MT8195_VDO0_OVL_MOUT_EN					0xf14
> +#define MT8195_MOUT_DISP_OVL0_TO_DISP_RDMA0			BIT(0)
> +#define MT8195_MOUT_DISP_OVL0_TO_DISP_WDMA0			BIT(1)
> +#define MT8195_MOUT_DISP_OVL0_TO_DISP_OVL1			BIT(2)
> +#define MT8195_MOUT_DISP_OVL1_TO_DISP_RDMA1			BIT(4)
> +#define MT8195_MOUT_DISP_OVL1_TO_DISP_WDMA1			BIT(5)
> +#define MT8195_MOUT_DISP_OVL1_TO_DISP_OVL0			BIT(6)
> +
> +#define MT8195_VDO0_SEL_IN					0xf34
> +#define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT		(0 << 0)

Bitshifting 0 by 0 bits == 0, so this is simply 0.

> +#define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1		(1 << 0)

I would write 0x1 here

> +#define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0		(2 << 0)

....and 0x2 here: bitshifting of 0 bits makes little sense.

> +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0		(0 << 4)

Bitshifting 0 by 4 bits is still 0, so this is again 0.
This is repeated too many times, so I will not list it for all of the occurrences.

> +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE		(1 << 4)

This is BIT(4).

> +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1		(0 << 5) > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE		(1 << 5)

...and this is BIT(5)

> +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_VPP_MERGE		(0 << 8)
> +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_DSC_WRAP1_OUT		(1 << 8)

BIT(8)

> +#define MT8195_SEL_IN_SINB_VIRTUAL0_FROM_DSC_WRAP0_OUT		(0 << 9)
> +#define MT8195_SEL_IN_DP_INTF0_FROM_DSC_WRAP1_OUT		(0 << 12)
> +#define MT8195_SEL_IN_DP_INTF0_FROM_VPP_MERGE			(1 << 12)

BIT(12)

> +#define MT8195_SEL_IN_DP_INTF0_FROM_VDO1_VIRTUAL0		(2 << 12)

BIT(13)

... and please, use the BIT(nr) macro for all these bit definitions, it's way more
readable like that.

Regards,
- Angelo

Re: [PATCH v11 09/16] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

[Jason-JH Lin]

Hi Angelo,

Thanks for the reviews.


On Thu, 2021-10-14 at 16:05 +0200, AngeloGioacchino Del Regno wrote:
> > Add mt8195 vdosys0 clock driver name and routing table to
> > the driver data of mtk-mmsys.
> > 

[snip]

> >  
> > ---
> 
> Hello Jason,
> thanks for the patch! However, there are a few things to improve:
> 

[snip]

> > +#define MT8195_VDO0_SEL_IN					0xf34
> > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT		(0 <<
> > 0)
> 
> Bitshifting 0 by 0 bits == 0, so this is simply 0.
> 
> > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1		(1 <<
> > 0)
> 
> I would write 0x1 here
> 
> > +#define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0		(2 <<
> > 0)
> 
> ....and 0x2 here: bitshifting of 0 bits makes little sense.
> 
> > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0		
> > (0 << 4)
> 
> Bitshifting 0 by 4 bits is still 0, so this is again 0.
> This is repeated too many times, so I will not list it for all of the
> occurrences.
> 
> > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE		(1 <<
> > 4)
> 
> This is BIT(4).
> 
> > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1		
> > (0 << 5) > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE	
> > 	(1 << 5)
> 
> ...and this is BIT(5)
> 
> > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_VPP_MERGE		(0 <<
> > 8)
> > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_DSC_WRAP1_OUT		
> > (1 << 8)
> 
> BIT(8)
> 
> > +#define MT8195_SEL_IN_SINB_VIRTUAL0_FROM_DSC_WRAP0_OUT		
> > (0 << 9)
> > +#define MT8195_SEL_IN_DP_INTF0_FROM_DSC_WRAP1_OUT		(0 <<
> > 12)
> > +#define MT8195_SEL_IN_DP_INTF0_FROM_VPP_MERGE			
> > (1 << 12)
> 
> BIT(12)
> 
> > +#define MT8195_SEL_IN_DP_INTF0_FROM_VDO1_VIRTUAL0		(2 <<
> > 12)
> 
> BIT(13)
> 
> ... and please, use the BIT(nr) macro for all these bit definitions,
> it's way more
> readable like that.
> 
> Regards,
> - Angelo

Because the HW register design of MT8195_VDO0_SEL_IN 0xf34 is like
this:

bit[1:0] as MT8195_SEL_IN_VPP_MERGE and
  value: 0 as MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT
  value: 1 as MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1
  value: 2 as MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0
bit[4:4] as MT8195_SEL_IN_DSC_WRAP0_IN and
  value 0 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0
  value 1 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE
bit[5:5] as MT8195_SEL_IN_DSC_WRAP1_IN and
  value 0 as
MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1
  value 1 as
MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE
and so on...

I think using BIT(nr) macro directly is not easy to debug.


Is it better to define another MACRO like this?

#define BIT_VAL(val, bit)  ((val) << (bit))
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  BIT_VAL(0, 4)
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  BIT_VAL(1, 4)
...

or

#define MT8195_SEL_IN_DSC_WRAP0_IN (4)
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  (0
<< MT8195_SEL_IN_DSC_WRAP0_IN)
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  (1 <<
MT8195_SEL_IN_DSC_WRAP0_IN)
...

What do you think?


Regards,
Jason-JH Lin <jason-jh.lin@mediatek.com>

Re: [PATCH v11 09/16] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

[Fei Shao]

On Fri, Oct 22, 2021 at 6:13 PM Jason-JH Lin <jason-jh.lin@mediatek.com> wrote:
>
> Hi Angelo,
>
> Thanks for the reviews.
>
>
> On Thu, 2021-10-14 at 16:05 +0200, AngeloGioacchino Del Regno wrote:
> > > Add mt8195 vdosys0 clock driver name and routing table to
> > > the driver data of mtk-mmsys.
> > >
>
> [snip]
>
> > >
> > > ---
> >
> > Hello Jason,
> > thanks for the patch! However, there are a few things to improve:
> >
>
> [snip]
>
> > > +#define MT8195_VDO0_SEL_IN                                 0xf34
> > > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT         (0 <<
> > > 0)
> >
> > Bitshifting 0 by 0 bits == 0, so this is simply 0.
> >
> > > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1          (1 <<
> > > 0)
> >
> > I would write 0x1 here
> >
> > > +#define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0         (2 <<
> > > 0)
> >
> > ....and 0x2 here: bitshifting of 0 bits makes little sense.
> >
> > > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0
> > > (0 << 4)
> >
> > Bitshifting 0 by 4 bits is still 0, so this is again 0.
> > This is repeated too many times, so I will not list it for all of the
> > occurrences.
> >
> > > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE          (1 <<
> > > 4)
> >
> > This is BIT(4).
> >
> > > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1
> > > (0 << 5) > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE
> > >     (1 << 5)
> >
> > ...and this is BIT(5)
> >
> > > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_VPP_MERGE         (0 <<
> > > 8)
> > > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_DSC_WRAP1_OUT
> > > (1 << 8)
> >
> > BIT(8)
> >
> > > +#define MT8195_SEL_IN_SINB_VIRTUAL0_FROM_DSC_WRAP0_OUT
> > > (0 << 9)
> > > +#define MT8195_SEL_IN_DP_INTF0_FROM_DSC_WRAP1_OUT          (0 <<
> > > 12)
> > > +#define MT8195_SEL_IN_DP_INTF0_FROM_VPP_MERGE
> > > (1 << 12)
> >
> > BIT(12)
> >
> > > +#define MT8195_SEL_IN_DP_INTF0_FROM_VDO1_VIRTUAL0          (2 <<
> > > 12)
> >
> > BIT(13)
> >
> > ... and please, use the BIT(nr) macro for all these bit definitions,
> > it's way more
> > readable like that.
> >
> > Regards,
> > - Angelo
>
> Because the HW register design of MT8195_VDO0_SEL_IN 0xf34 is like
> this:
>
> bit[1:0] as MT8195_SEL_IN_VPP_MERGE and
>   value: 0 as MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT
>   value: 1 as MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1
>   value: 2 as MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0
> bit[4:4] as MT8195_SEL_IN_DSC_WRAP0_IN and
>   value 0 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0
>   value 1 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE
> bit[5:5] as MT8195_SEL_IN_DSC_WRAP1_IN and
>   value 0 as
> MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1
>   value 1 as
> MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE
> and so on...
>
> I think using BIT(nr) macro directly is not easy to debug.
>
>
> Is it better to define another MACRO like this?
>
> #define BIT_VAL(val, bit)  ((val) << (bit))
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  BIT_VAL(0, 4)
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  BIT_VAL(1, 4)
> ...
>
> or
>
> #define MT8195_SEL_IN_DSC_WRAP0_IN (4)
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  (0
> << MT8195_SEL_IN_DSC_WRAP0_IN)
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  (1 <<
> MT8195_SEL_IN_DSC_WRAP0_IN)
> ...
>
> What do you think?

Hi Jason,

If that's the case you can still use BIT(nr) for the definitions and
describe their usage in the comment, so both code readability and the
ease of maintenance are preserved, and people can easily tell if there
are duplicated/missing definitions while reading through the code.
Adding informative comments is never a bad thing.

I would do something like this (and further split the definitions into
sections by their functionalities with blank lines for visual
comfort):

/*
 * MT8195_VDO0_SEL_IN[1:0]: VPP_MERGE
 *   0x0 : DSC_WRAP0_OUT
 *   0x1 : DISP_DITHER1
 *   0x10: VDO1_VIRTUAL0
 */
#define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT           0
#define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1            BIT(0)
#define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0           BIT(1)

/*
 * MT8195_VDO0_SEL_IN[4:4]: DSC_WRAP0_IN
 *   0x0: DISP_DITHER0
 *   0x1: VPP_MERGE
 */
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0         0
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE            BIT(4)
... and so on.

Regards,
Fei

>
>
> Regards,
> Jason-JH Lin <jason-jh.lin@mediatek.com>
>

Re: [PATCH v11 09/16] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

[Jason-JH Lin]

Hi Fei,

Thanks for the reviews.

On Mon, 2021-10-25 at 13:05 +0800, Fei Shao wrote:
> On Fri, Oct 22, 2021 at 6:13 PM Jason-JH Lin <
> jason-jh.lin@mediatek.com> wrote:
> > 
> > Hi Angelo,
> > 
> > Thanks for the reviews.
> > 
> > 
> > On Thu, 2021-10-14 at 16:05 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > > Add mt8195 vdosys0 clock driver name and routing table to
> > > > the driver data of mtk-mmsys.
> > > > 
> > 
> > [snip]
> > 
> > > > 
> > > > ---
> > > 
> > > Hello Jason,
> > > thanks for the patch! However, there are a few things to improve:
> > > 
> > 
> > [snip]
> > 
> > > > +#define
> > > > MT8195_VDO0_SEL_IN                                 0xf34
> > > > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT         (0
> > > > <<
> > > > 0)
> > > 
> > > Bitshifting 0 by 0 bits == 0, so this is simply 0.
> > > 
> > > > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1          (1
> > > > <<
> > > > 0)
> > > 
> > > I would write 0x1 here
> > > 
> > > > +#define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0         (2
> > > > <<
> > > > 0)
> > > 
> > > ....and 0x2 here: bitshifting of 0 bits makes little sense.
> > > 
> > > > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0
> > > > (0 << 4)
> > > 
> > > Bitshifting 0 by 4 bits is still 0, so this is again 0.
> > > This is repeated too many times, so I will not list it for all of
> > > the
> > > occurrences.
> > > 
> > > > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE          (1
> > > > <<
> > > > 4)
> > > 
> > > This is BIT(4).
> > > 
> > > > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1
> > > > (0 << 5) > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE
> > > >     (1 << 5)
> > > 
> > > ...and this is BIT(5)
> > > 
> > > > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_VPP_MERGE         (0
> > > > <<
> > > > 8)
> > > > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_DSC_WRAP1_OUT
> > > > (1 << 8)
> > > 
> > > BIT(8)
> > > 
> > > > +#define MT8195_SEL_IN_SINB_VIRTUAL0_FROM_DSC_WRAP0_OUT
> > > > (0 << 9)
> > > > +#define MT8195_SEL_IN_DP_INTF0_FROM_DSC_WRAP1_OUT          (0
> > > > <<
> > > > 12)
> > > > +#define MT8195_SEL_IN_DP_INTF0_FROM_VPP_MERGE
> > > > (1 << 12)
> > > 
> > > BIT(12)
> > > 
> > > > +#define MT8195_SEL_IN_DP_INTF0_FROM_VDO1_VIRTUAL0          (2
> > > > <<
> > > > 12)
> > > 
> > > BIT(13)
> > > 
> > > ... and please, use the BIT(nr) macro for all these bit
> > > definitions,
> > > it's way more
> > > readable like that.
> > > 
> > > Regards,
> > > - Angelo
> > 
> > Because the HW register design of MT8195_VDO0_SEL_IN 0xf34 is like
> > this:
> > 
> > bit[1:0] as MT8195_SEL_IN_VPP_MERGE and
> >   value: 0 as MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT
> >   value: 1 as MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1
> >   value: 2 as MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0
> > bit[4:4] as MT8195_SEL_IN_DSC_WRAP0_IN and
> >   value 0 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0
> >   value 1 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE
> > bit[5:5] as MT8195_SEL_IN_DSC_WRAP1_IN and
> >   value 0 as
> > MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1
> >   value 1 as
> > MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE
> > and so on...
> > 
> > I think using BIT(nr) macro directly is not easy to debug.
> > 
> > 
> > Is it better to define another MACRO like this?
> > 
> > #define BIT_VAL(val, bit)  ((val) << (bit))
> > #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  BIT_VAL(0, 4)
> > #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  BIT_VAL(1, 4)
> > ...
> > 
> > or
> > 
> > #define MT8195_SEL_IN_DSC_WRAP0_IN (4)
> > #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  (0
> > << MT8195_SEL_IN_DSC_WRAP0_IN)
> > #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  (1 <<
> > MT8195_SEL_IN_DSC_WRAP0_IN)
> > ...
> > 
> > What do you think?
> 
> Hi Jason,
> 
> If that's the case you can still use BIT(nr) for the definitions and
> describe their usage in the comment, so both code readability and the
> ease of maintenance are preserved, and people can easily tell if
> there
> are duplicated/missing definitions while reading through the code.
> Adding informative comments is never a bad thing.
> 
> I would do something like this (and further split the definitions
> into
> sections by their functionalities with blank lines for visual
> comfort):
> 
> /*
>  * MT8195_VDO0_SEL_IN[1:0]: VPP_MERGE
>  *   0x0 : DSC_WRAP0_OUT
>  *   0x1 : DISP_DITHER1
>  *   0x10: VDO1_VIRTUAL0
>  */
> #define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT           0
> #define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1            BIT(0)
> #define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0           BIT(1)
> 
> /*
>  * MT8195_VDO0_SEL_IN[4:4]: DSC_WRAP0_IN
>  *   0x0: DISP_DITHER0
>  *   0x1: VPP_MERGE
>  */
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0         0
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE            BIT(4)
> ... and so on.
> 
> Regards,
> Fei
> 

OK, I'll fix it.

> > 
> > 
> > Regards,
> > Jason-JH Lin <jason-jh.lin@mediatek.com>
> > 
-- 
Jason-JH Lin <jason-jh.lin@mediatek.com>