[PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Julien Stephan]

From: Florian Sylvestre <fsylvestre@baylibre.com>

This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
some Mediatek soc, such as the mt8365

Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml

diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
new file mode 100644
index 000000000000..5aa8c0b41cdf
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek Sensor Interface MIPI CSI CD-PHY
+
+maintainers:
+  - Julien Stephan <jstephan@baylibre.com>
+  - Andy Hsieh <andy.hsieh@mediatek.com>
+
+description:
+  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
+  receivers. The number of PHYs depends on the SoC model.
+  Depending on the soc model, each PHYs can support CDPHY or DPHY only
+
+properties:
+  compatible:
+    enum:
+      - mediatek,phy-mipi-csi-0-5
+
+  reg:
+    maxItems: 1
+
+  '#phy-cells':
+    const: 0
+
+  mediatek,is_cdphy:
+    description:
+      Specify if the current phy support CDPHY configuration
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - '#phy-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      mipi_rx_csi0: mipi_rx_csi0@11c10000 {
+        compatible = "mediatek,phy-mipi-csi-0-5";
+        reg = <0 0x11C10000 0 0x2000>;
+        status = "okay";
+        mediatek,is_cdphy;
+        #phy-cells = <0>;
+      };
+
+      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
+        compatible = "mediatek,phy-mipi-csi-0-5";
+        reg = <0 0x11C12000 0 0x2000>;
+        status = "disabled";
+        #phy-cells = <0>;
+      };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 6d54f3193075..44f0ff11e984 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13098,6 +13098,12 @@ F:	Documentation/devicetree/bindings/media/mediatek-vpu.txt
 F:	drivers/media/platform/mediatek/vcodec/
 F:	drivers/media/platform/mediatek/vpu/
 
+MEDIATEK MIPI-CSI CDPHY DRIVER
+M:	Julien Stephan <jstephan@baylibre.com>
+M:	Andy Hsieh <andy.hsieh@mediatek.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
+
 MEDIATEK MMC/SD/SDIO DRIVER
 M:	Chaotian Jing <chaotian.jing@mediatek.com>
 S:	Maintained
-- 
2.40.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[AngeloGioacchino Del Regno]

Il 15/05/23 11:05, Julien Stephan ha scritto:
> From: Florian Sylvestre <fsylvestre@baylibre.com>
> 
> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
> some Mediatek soc, such as the mt8365
> 
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
>   .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>   MAINTAINERS                                   |  6 ++
>   2 files changed, 68 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> new file mode 100644
> index 000000000000..5aa8c0b41cdf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> +
> +maintainers:
> +  - Julien Stephan <jstephan@baylibre.com>
> +  - Andy Hsieh <andy.hsieh@mediatek.com>
> +
> +description:
> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
> +  receivers. The number of PHYs depends on the SoC model.
> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,phy-mipi-csi-0-5

mediatek,mtXXXX-csi-rx sounds great, doesn't it?

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#phy-cells':
> +    const: 0
> +
> +  mediatek,is_cdphy:

No underscores please: mediatek,is-cdphy

> +    description:
> +      Specify if the current phy support CDPHY configuration

Description fits in one line.

     description: Specify if the current phy support CDPHY configuration

> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#phy-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {

csi0_rx: phy@11c10000

> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C10000 0 0x2000>;
> +        status = "okay";
> +        mediatek,is_cdphy;
> +        #phy-cells = <0>;
> +      };
> +
> +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {

csi1_rx: phy@11c20000

> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C12000 0 0x2000>;
> +        status = "disabled";
> +        #phy-cells = <0>;
> +      };
> +    };
> +...

Regards,
Angelo


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 15/05/2023 11:05, Julien Stephan wrote:
> From: Florian Sylvestre <fsylvestre@baylibre.com>
> 
> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
> some Mediatek soc, such as the mt8365
> 
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>

What are the changes? IOW: changelog here or in cover letter.

Subject: you have some multiple spaces.

Subject: drop driver. Bindings are not for drivers.

> ---
>  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> new file mode 100644
> index 000000000000..5aa8c0b41cdf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> +
> +maintainers:
> +  - Julien Stephan <jstephan@baylibre.com>
> +  - Andy Hsieh <andy.hsieh@mediatek.com>
> +
> +description:
> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
> +  receivers. The number of PHYs depends on the SoC model.
> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,phy-mipi-csi-0-5

SoC based compatibles. 0-5 is odd.

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#phy-cells':
> +    const: 0
> +
> +  mediatek,is_cdphy:

No underscores in node names.

> +    description:
> +      Specify if the current phy support CDPHY configuration

Why this cannot be implied from compatible? Add specific compatibles.


> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#phy-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {

No underscores in node names. How this is v2?

> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C10000 0 0x2000>;
> +        status = "okay";

Drop

> +        mediatek,is_cdphy;
> +        #phy-cells = <0>;
> +      };
> +
> +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
> +        compatible = "mediatek,phy-mipi-csi-0-5";
> +        reg = <0 0x11C12000 0 0x2000>;
> +        status = "disabled";

???


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Julien Stephan]

On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
> On 15/05/2023 11:05, Julien Stephan wrote:
> > From: Florian Sylvestre <fsylvestre@baylibre.com>
> >
> > This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
> > some Mediatek soc, such as the mt8365
> >
> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>
> What are the changes? IOW: changelog here or in cover letter.
>
Hi Krzysztof,
I added a changelog in the cover letter, but I will try to be more
descritpive next time. Changes from v1 are mainly style issues fixed
(mostly from your first review)

> Subject: you have some multiple spaces.
>
> Subject: drop driver. Bindings are not for drivers.
>
> > ---
> >  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
> >  MAINTAINERS                                   |  6 ++
> >  2 files changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> > new file mode 100644
> > index 000000000000..5aa8c0b41cdf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> > +
> > +maintainers:
> > +  - Julien Stephan <jstephan@baylibre.com>
> > +  - Andy Hsieh <andy.hsieh@mediatek.com>
> > +
> > +description:
> > +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
> > +  receivers. The number of PHYs depends on the SoC model.
> > +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,phy-mipi-csi-0-5
>
> SoC based compatibles. 0-5 is odd.
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#phy-cells':
> > +    const: 0
> > +
> > +  mediatek,is_cdphy:
>
> No underscores in node names.
>
> > +    description:
> > +      Specify if the current phy support CDPHY configuration
>
> Why this cannot be implied from compatible? Add specific compatibles.
>
>
This cannot be implied by compatible because the number of phys depends
on the soc and each phy can be either D-PHY only or CD-PHY capable.
For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY

Regards,
Julien
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#phy-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {
>
> No underscores in node names. How this is v2?
>
> > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > +        reg = <0 0x11C10000 0 0x2000>;
> > +        status = "okay";
>
> Drop
>
> > +        mediatek,is_cdphy;
> > +        #phy-cells = <0>;
> > +      };
> > +
> > +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
> > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > +        reg = <0 0x11C12000 0 0x2000>;
> > +        status = "disabled";
>
> ???
>
>
> Best regards,
> Krzysztof
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 16/05/2023 11:41, Julien Stephan wrote:
> On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
>> On 15/05/2023 11:05, Julien Stephan wrote:
>>> From: Florian Sylvestre <fsylvestre@baylibre.com>
>>>
>>> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
>>> some Mediatek soc, such as the mt8365
>>>
>>> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
>>> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>>
>> What are the changes? IOW: changelog here or in cover letter.
>>
> Hi Krzysztof,
> I added a changelog in the cover letter, but I will try to be more
> descritpive next time. Changes from v1 are mainly style issues fixed
> (mostly from your first review)

What do you mean by "in cover letter"? There is no cover letter.

> 
>> Subject: you have some multiple spaces.
>>
>> Subject: drop driver. Bindings are not for drivers.
>>
>>> ---
>>>  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>>>  MAINTAINERS                                   |  6 ++
>>>  2 files changed, 68 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>> new file mode 100644
>>> index 000000000000..5aa8c0b41cdf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>> @@ -0,0 +1,62 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
>>> +
>>> +maintainers:
>>> +  - Julien Stephan <jstephan@baylibre.com>
>>> +  - Andy Hsieh <andy.hsieh@mediatek.com>
>>> +
>>> +description:
>>> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
>>> +  receivers. The number of PHYs depends on the SoC model.
>>> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - mediatek,phy-mipi-csi-0-5
>>
>> SoC based compatibles. 0-5 is odd.
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  '#phy-cells':
>>> +    const: 0
>>> +
>>> +  mediatek,is_cdphy:
>>
>> No underscores in node names.
>>
>>> +    description:
>>> +      Specify if the current phy support CDPHY configuration
>>
>> Why this cannot be implied from compatible? Add specific compatibles.
>>
>>
> This cannot be implied by compatible because the number of phys depends
> on the soc and each phy can be either D-PHY only or CD-PHY capable.
> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY

So it is SoC specific so why it cannot be implied by compatible? I don't
understand. You will have SoC specific compatibles, right? or you just
ignored my comments here?

>>

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Kevin Hilman]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 16/05/2023 11:41, Julien Stephan wrote:
>> On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
>>> On 15/05/2023 11:05, Julien Stephan wrote:
>>>> From: Florian Sylvestre <fsylvestre@baylibre.com>
>>>>
>>>> This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded in
>>>> some Mediatek soc, such as the mt8365
>>>>
>>>> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
>>>> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>>>
>>> What are the changes? IOW: changelog here or in cover letter.
>>>
>> Hi Krzysztof,
>> I added a changelog in the cover letter, but I will try to be more
>> descritpive next time. Changes from v1 are mainly style issues fixed
>> (mostly from your first review)
>
> What do you mean by "in cover letter"? There is no cover letter.

Julien, your cover letter[1] was sent to a a different list of
recipients than the patches, and most important for this thread, it was
*not* sent to the devictree list. So I'm guessing that's why Krzysztof
doesn't see it in his devicetree review queue.  Generally, you should
have the same list of recipients for the cover letter as the patches
since reviewers/maintainers generally filter mail based on which mailing
lists are in to/cc.

>> 
>>> Subject: you have some multiple spaces.
>>>
>>> Subject: drop driver. Bindings are not for drivers.
>>>
>>>> ---
>>>>  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62 +++++++++++++++++++
>>>>  MAINTAINERS                                   |  6 ++
>>>>  2 files changed, 68 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>> new file mode 100644
>>>> index 000000000000..5aa8c0b41cdf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
>>>> @@ -0,0 +1,62 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Mediatek Sensor Interface MIPI CSI CD-PHY
>>>> +
>>>> +maintainers:
>>>> +  - Julien Stephan <jstephan@baylibre.com>
>>>> +  - Andy Hsieh <andy.hsieh@mediatek.com>
>>>> +
>>>> +description:
>>>> +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF CSI-2
>>>> +  receivers. The number of PHYs depends on the SoC model.
>>>> +  Depending on the soc model, each PHYs can support CDPHY or DPHY only
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - mediatek,phy-mipi-csi-0-5
>>>
>>> SoC based compatibles. 0-5 is odd.
>>>
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  '#phy-cells':
>>>> +    const: 0
>>>> +
>>>> +  mediatek,is_cdphy:
>>>
>>> No underscores in node names.
>>>
>>>> +    description:
>>>> +      Specify if the current phy support CDPHY configuration
>>>
>>> Why this cannot be implied from compatible? Add specific compatibles.
>>>
>>>
>> This cannot be implied by compatible because the number of phys depends
>> on the soc and each phy can be either D-PHY only or CD-PHY capable.
>> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY
>
> So it is SoC specific so why it cannot be implied by compatible? I don't
> understand. You will have SoC specific compatibles, right? or you just
> ignored my comments here?

Julien, I think you had SoC specific compatibles in an earlier version
but then changed it to be generic based on reviewer feedback.  However,
that earlier version of the driver was trying to do a bunch of SoC
specific logic internally and support multiple SoCs.  You've now greatly
simplified the driver, with only a few SoC specific decisions needed.
These can be implied by the driver based SoC specific compatible, as
Krzysztof suggests, so you should just go back to having SoC specific
compatibles.

Kevin

[1] https://lore.kernel.org/linux-mediatek/20230515090551.1251389-1-jstephan@baylibre.com/#r

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 16/05/2023 19:00, Kevin Hilman wrote:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - mediatek,phy-mipi-csi-0-5
>>>>
>>>> SoC based compatibles. 0-5 is odd.
>>>>
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  '#phy-cells':
>>>>> +    const: 0
>>>>> +
>>>>> +  mediatek,is_cdphy:
>>>>
>>>> No underscores in node names.
>>>>
>>>>> +    description:
>>>>> +      Specify if the current phy support CDPHY configuration
>>>>
>>>> Why this cannot be implied from compatible? Add specific compatibles.
>>>>
>>>>
>>> This cannot be implied by compatible because the number of phys depends
>>> on the soc and each phy can be either D-PHY only or CD-PHY capable.
>>> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY
>>
>> So it is SoC specific so why it cannot be implied by compatible? I don't
>> understand. You will have SoC specific compatibles, right? or you just
>> ignored my comments here?
> 
> Julien, I think you had SoC specific compatibles in an earlier version
> but then changed it to be generic based on reviewer feedback.  However,
> that earlier version of the driver was trying to do a bunch of SoC
> specific logic internally and support multiple SoCs.  You've now greatly
> simplified the driver, with only a few SoC specific decisions needed.
> These can be implied by the driver based SoC specific compatible, as
> Krzysztof suggests, so you should just go back to having SoC specific
> compatibles.
> 

Yes. If there is common part, e.g. several SoCs use the same device with
same programming model, then the generic recommendation is to have
SoC-based fallback (used also in the driver) and SoC-specific compatibles.

Second accepted solution is to have generic fallback which does not use
SoC in the compatible (and of course mandatory SoC-specific comaptibles).

Third is to use versioned IP blocks.

The second case also would work, if it is applicable to you (you really
have fallback matching all devices). Third solution depends on your
versioning and Rob expressed dislike about it many times.

We had many discussions on mailing lists, thus simplifying the review -
I recommend the first choice. For a better recommendation you should say
a bit more about the block in different SoCs.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Kevin Hilman]

Hi Krzysztof, 

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 16/05/2023 19:00, Kevin Hilman wrote:
>>>>>> +  compatible:
>>>>>> +    enum:
>>>>>> +      - mediatek,phy-mipi-csi-0-5
>>>>>
>>>>> SoC based compatibles. 0-5 is odd.
>>>>>
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  '#phy-cells':
>>>>>> +    const: 0
>>>>>> +
>>>>>> +  mediatek,is_cdphy:
>>>>>
>>>>> No underscores in node names.
>>>>>
>>>>>> +    description:
>>>>>> +      Specify if the current phy support CDPHY configuration
>>>>>
>>>>> Why this cannot be implied from compatible? Add specific compatibles.
>>>>>
>>>>>
>>>> This cannot be implied by compatible because the number of phys depends
>>>> on the soc and each phy can be either D-PHY only or CD-PHY capable.
>>>> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and CSI0 is CD-PHY
>>>
>>> So it is SoC specific so why it cannot be implied by compatible? I don't
>>> understand. You will have SoC specific compatibles, right? or you just
>>> ignored my comments here?
>> 
>> Julien, I think you had SoC specific compatibles in an earlier version
>> but then changed it to be generic based on reviewer feedback.  However,
>> that earlier version of the driver was trying to do a bunch of SoC
>> specific logic internally and support multiple SoCs.  You've now greatly
>> simplified the driver, with only a few SoC specific decisions needed.
>> These can be implied by the driver based SoC specific compatible, as
>> Krzysztof suggests, so you should just go back to having SoC specific
>> compatibles.
>> 
>
> Yes. If there is common part, e.g. several SoCs use the same device with
> same programming model, then the generic recommendation is to have
> SoC-based fallback (used also in the driver) and SoC-specific compatibles.
>
> Second accepted solution is to have generic fallback which does not use
> SoC in the compatible (and of course mandatory SoC-specific comaptibles).
>
> Third is to use versioned IP blocks.
>
> The second case also would work, if it is applicable to you (you really
> have fallback matching all devices). Third solution depends on your
> versioning and Rob expressed dislike about it many times.
>
> We had many discussions on mailing lists, thus simplifying the review -
> I recommend the first choice. For a better recommendation you should say
> a bit more about the block in different SoCs.

I'll try to say a bit more about the PHY block, but in fact, it's not
just about differences between SoCs. On the same SoC, 2 different PHYs
may have different features/capabilities.

For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
(used as the example in the binding patch[1].)  On another related SoC,
there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.

So that's why it seems (at least to me) that while we need SoC
compatible, it's not enough.  We also need properties to describe
PHY-specific features (e.g. C-D PHY)

Of course, we could rely only on SoC-specific compatibles describe this.
But then driver will need an SoC-specific table with the number of PHYs
and per-PHY features for each SoC encoded in the driver.  Since the
driver otherwise doesn't (and shouldn't, IMHO) need to know how many
PHYs are on each SoC, I suggested to Julien that perhaps the additional
propery was the better solution.  

To me it seems redundant to have the driver encode PHYs-per-SoC info,
when the per-SoC DT is going to have the same info, so my suggestion was
to simplify the driver and have this kind of hardware description in the
DT, and keep the driver simple, but we are definitely open to learning
the "right way" of doing this.

Thanks for your review and guidance,

Kevin

[1] https://lore.kernel.org/linux-mediatek/20230515090551.1251389-2-jstephan@baylibre.com/

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 16/05/2023 23:31, Kevin Hilman wrote:

>> Third is to use versioned IP blocks.
>>
>> The second case also would work, if it is applicable to you (you really
>> have fallback matching all devices). Third solution depends on your
>> versioning and Rob expressed dislike about it many times.
>>
>> We had many discussions on mailing lists, thus simplifying the review -
>> I recommend the first choice. For a better recommendation you should say
>> a bit more about the block in different SoCs.
> 
> I'll try to say a bit more about the PHY block, but in fact, it's not
> just about differences between SoCs. On the same SoC, 2 different PHYs
> may have different features/capabilities.
> 
> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
> (used as the example in the binding patch[1].)  On another related SoC,
> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
> 
> So that's why it seems (at least to me) that while we need SoC
> compatible, it's not enough.  We also need properties to describe
> PHY-specific features (e.g. C-D PHY)

I recall the same or very similar case... It bugs me now, but
unfortunately I cannot find it.

> 
> Of course, we could rely only on SoC-specific compatibles describe this.
> But then driver will need an SoC-specific table with the number of PHYs
> and per-PHY features for each SoC encoded in the driver.  Since the
> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
> PHYs are on each SoC, I suggested to Julien that perhaps the additional
> propery was the better solution.

Phys were modeled as separate device instances, so you would need
difference in compatible to figure out which phy is it.

Other way could be to create device for all phys and use phy-cells=1.
Whether it makes sense, depends on the actual datasheet - maybe the
split phy per device is artificial? There is one PHY block with two
address ranges for each PHY - CSI0 and CSI1 - but it is actually one
block? You should carefully check this because once design is chosen,
you won't be able to go back to other and it might be a problem (e.g.
there is some top-level block for powering on all CSI instances).


> 
> To me it seems redundant to have the driver encode PHYs-per-SoC info,
> when the per-SoC DT is going to have the same info, so my suggestion was
> to simplify the driver and have this kind of hardware description in the
> DT, and keep the driver simple, but we are definitely open to learning
> the "right way" of doing this.

The property then is reasonable. It should not be bool, though, because
it does not scale. There can be next block which supports only D-PHY on
CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
configurations.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Kevin Hilman]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 16/05/2023 23:31, Kevin Hilman wrote:
>
>>> Third is to use versioned IP blocks.
>>>
>>> The second case also would work, if it is applicable to you (you really
>>> have fallback matching all devices). Third solution depends on your
>>> versioning and Rob expressed dislike about it many times.
>>>
>>> We had many discussions on mailing lists, thus simplifying the review -
>>> I recommend the first choice. For a better recommendation you should say
>>> a bit more about the block in different SoCs.
>> 
>> I'll try to say a bit more about the PHY block, but in fact, it's not
>> just about differences between SoCs. On the same SoC, 2 different PHYs
>> may have different features/capabilities.
>> 
>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
>> (used as the example in the binding patch[1].)  On another related SoC,
>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
>> 
>> So that's why it seems (at least to me) that while we need SoC
>> compatible, it's not enough.  We also need properties to describe
>> PHY-specific features (e.g. C-D PHY)
>
> I recall the same or very similar case... It bugs me now, but
> unfortunately I cannot find it.
>
>> 
>> Of course, we could rely only on SoC-specific compatibles describe this.
>> But then driver will need an SoC-specific table with the number of PHYs
>> and per-PHY features for each SoC encoded in the driver.  Since the
>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
>> propery was the better solution.
>
> Phys were modeled as separate device instances, so you would need
> difference in compatible to figure out which phy is it.
>
> Other way could be to create device for all phys and use phy-cells=1.
> Whether it makes sense, depends on the actual datasheet - maybe the
> split phy per device is artificial? There is one PHY block with two
> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
> block? You should carefully check this because once design is chosen,
> you won't be able to go back to other and it might be a problem (e.g.
> there is some top-level block for powering on all CSI instances).

We're pretty sure these are multiple instances of the IP block as they
can operate completely independently. 

>> 
>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
>> when the per-SoC DT is going to have the same info, so my suggestion was
>> to simplify the driver and have this kind of hardware description in the
>> DT, and keep the driver simple, but we are definitely open to learning
>> the "right way" of doing this.
>
> The property then is reasonable. It should not be bool, though, because
> it does not scale. There can be next block which supports only D-PHY on
> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
> configurations.

OK, looks like include/dt-bindings/phy/phy.y already has

  #define PHY_TYPE_DPHY		10
  #define PHY_TYPE_CPHY		11

we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?

Kevin

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Krzysztof Kozlowski]

On 22/05/2023 21:15, Kevin Hilman wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> 
>> On 16/05/2023 23:31, Kevin Hilman wrote:
>>
>>>> Third is to use versioned IP blocks.
>>>>
>>>> The second case also would work, if it is applicable to you (you really
>>>> have fallback matching all devices). Third solution depends on your
>>>> versioning and Rob expressed dislike about it many times.
>>>>
>>>> We had many discussions on mailing lists, thus simplifying the review -
>>>> I recommend the first choice. For a better recommendation you should say
>>>> a bit more about the block in different SoCs.
>>>
>>> I'll try to say a bit more about the PHY block, but in fact, it's not
>>> just about differences between SoCs. On the same SoC, 2 different PHYs
>>> may have different features/capabilities.
>>>
>>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
>>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
>>> (used as the example in the binding patch[1].)  On another related SoC,
>>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
>>>
>>> So that's why it seems (at least to me) that while we need SoC
>>> compatible, it's not enough.  We also need properties to describe
>>> PHY-specific features (e.g. C-D PHY)
>>
>> I recall the same or very similar case... It bugs me now, but
>> unfortunately I cannot find it.
>>
>>>
>>> Of course, we could rely only on SoC-specific compatibles describe this.
>>> But then driver will need an SoC-specific table with the number of PHYs
>>> and per-PHY features for each SoC encoded in the driver.  Since the
>>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
>>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
>>> propery was the better solution.
>>
>> Phys were modeled as separate device instances, so you would need
>> difference in compatible to figure out which phy is it.
>>
>> Other way could be to create device for all phys and use phy-cells=1.
>> Whether it makes sense, depends on the actual datasheet - maybe the
>> split phy per device is artificial? There is one PHY block with two
>> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
>> block? You should carefully check this because once design is chosen,
>> you won't be able to go back to other and it might be a problem (e.g.
>> there is some top-level block for powering on all CSI instances).
> 
> We're pretty sure these are multiple instances of the IP block as they
> can operate completely independently. 
> 
>>>
>>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
>>> when the per-SoC DT is going to have the same info, so my suggestion was
>>> to simplify the driver and have this kind of hardware description in the
>>> DT, and keep the driver simple, but we are definitely open to learning
>>> the "right way" of doing this.
>>
>> The property then is reasonable. It should not be bool, though, because
>> it does not scale. There can be next block which supports only D-PHY on
>> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
>> configurations.
> 
> OK, looks like include/dt-bindings/phy/phy.y already has
> 
>   #define PHY_TYPE_DPHY		10
>   #define PHY_TYPE_CPHY		11
> 
> we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?

Yes. Currently it is usually used as phy-cells argument (after the phy
number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as
property in provider. In both cases they have a bit different meaning
than yours. You want to list all supported modes or narrow/restrict
them. Maybe hisilicon,fixed-mode fits your purpose?

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Julien Stephan]

On Tue, May 30, 2023 at 10:53:31AM +0200, Krzysztof Kozlowski wrote:
> On 22/05/2023 21:15, Kevin Hilman wrote:
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> >
> >> On 16/05/2023 23:31, Kevin Hilman wrote:
> >>
> >>>> Third is to use versioned IP blocks.
> >>>>
> >>>> The second case also would work, if it is applicable to you (you really
> >>>> have fallback matching all devices). Third solution depends on your
> >>>> versioning and Rob expressed dislike about it many times.
> >>>>
> >>>> We had many discussions on mailing lists, thus simplifying the review -
> >>>> I recommend the first choice. For a better recommendation you should say
> >>>> a bit more about the block in different SoCs.
> >>>
> >>> I'll try to say a bit more about the PHY block, but in fact, it's not
> >>> just about differences between SoCs. On the same SoC, 2 different PHYs
> >>> may have different features/capabilities.
> >>>
> >>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
> >>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
> >>> (used as the example in the binding patch[1].)  On another related SoC,
> >>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
> >>>
> >>> So that's why it seems (at least to me) that while we need SoC
> >>> compatible, it's not enough.  We also need properties to describe
> >>> PHY-specific features (e.g. C-D PHY)
> >>
> >> I recall the same or very similar case... It bugs me now, but
> >> unfortunately I cannot find it.
> >>
> >>>
> >>> Of course, we could rely only on SoC-specific compatibles describe this.
> >>> But then driver will need an SoC-specific table with the number of PHYs
> >>> and per-PHY features for each SoC encoded in the driver.  Since the
> >>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
> >>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
> >>> propery was the better solution.
> >>
> >> Phys were modeled as separate device instances, so you would need
> >> difference in compatible to figure out which phy is it.
> >>
> >> Other way could be to create device for all phys and use phy-cells=1.
> >> Whether it makes sense, depends on the actual datasheet - maybe the
> >> split phy per device is artificial? There is one PHY block with two
> >> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
> >> block? You should carefully check this because once design is chosen,
> >> you won't be able to go back to other and it might be a problem (e.g.
> >> there is some top-level block for powering on all CSI instances).
> >
> > We're pretty sure these are multiple instances of the IP block as they
> > can operate completely independently.
> >
> >>>
> >>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
> >>> when the per-SoC DT is going to have the same info, so my suggestion was
> >>> to simplify the driver and have this kind of hardware description in the
> >>> DT, and keep the driver simple, but we are definitely open to learning
> >>> the "right way" of doing this.
> >>
> >> The property then is reasonable. It should not be bool, though, because
> >> it does not scale. There can be next block which supports only D-PHY on
> >> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
> >> configurations.
> >
> > OK, looks like include/dt-bindings/phy/phy.y already has
> >
> >   #define PHY_TYPE_DPHY		10
> >   #define PHY_TYPE_CPHY		11
> >
> > we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?
>
> Yes. Currently it is usually used as phy-cells argument (after the phy
> number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as
> property in provider. In both cases they have a bit different meaning
> than yours. You want to list all supported modes or narrow/restrict
> them. Maybe hisilicon,fixed-mode fits your purpose?
>

Hi Krzysztof,

Thanks for the suggestion, using something like hisilicon,fixed-node
looks like a good fit.
With mediatek,fixed-node, by default CSI node will be considered as
CD-PHY capable (unless the fixed-mode property is set.) so I won't need
anymore the new define PHY_TYPE_CDPHY introduced in v3.

Also introducing mediatek,fixed-mode suggests that PHYs not declaring
the fixed mode property support mode selection, so I suggest to add a
phy argument (#phy-cells = <1>) to select the mode (D or C mode).
Exactly what is done by the hsilicon driver.

How does that sound to you?

Best,
Julien
> Best regards,
> Krzysztof
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Rob Herring]

On Mon, Jun 05, 2023 at 10:44:22AM +0200, Julien Stephan wrote:
> On Tue, May 30, 2023 at 10:53:31AM +0200, Krzysztof Kozlowski wrote:
> > On 22/05/2023 21:15, Kevin Hilman wrote:
> > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> > >
> > >> On 16/05/2023 23:31, Kevin Hilman wrote:
> > >>
> > >>>> Third is to use versioned IP blocks.
> > >>>>
> > >>>> The second case also would work, if it is applicable to you (you really
> > >>>> have fallback matching all devices). Third solution depends on your
> > >>>> versioning and Rob expressed dislike about it many times.
> > >>>>
> > >>>> We had many discussions on mailing lists, thus simplifying the review -
> > >>>> I recommend the first choice. For a better recommendation you should say
> > >>>> a bit more about the block in different SoCs.
> > >>>
> > >>> I'll try to say a bit more about the PHY block, but in fact, it's not
> > >>> just about differences between SoCs. On the same SoC, 2 different PHYs
> > >>> may have different features/capabilities.
> > >>>
> > >>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
> > >>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
> > >>> (used as the example in the binding patch[1].)  On another related SoC,
> > >>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
> > >>>
> > >>> So that's why it seems (at least to me) that while we need SoC
> > >>> compatible, it's not enough.  We also need properties to describe
> > >>> PHY-specific features (e.g. C-D PHY)
> > >>
> > >> I recall the same or very similar case... It bugs me now, but
> > >> unfortunately I cannot find it.
> > >>
> > >>>
> > >>> Of course, we could rely only on SoC-specific compatibles describe this.
> > >>> But then driver will need an SoC-specific table with the number of PHYs
> > >>> and per-PHY features for each SoC encoded in the driver.  Since the
> > >>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
> > >>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
> > >>> propery was the better solution.
> > >>
> > >> Phys were modeled as separate device instances, so you would need
> > >> difference in compatible to figure out which phy is it.
> > >>
> > >> Other way could be to create device for all phys and use phy-cells=1.
> > >> Whether it makes sense, depends on the actual datasheet - maybe the
> > >> split phy per device is artificial? There is one PHY block with two
> > >> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
> > >> block? You should carefully check this because once design is chosen,
> > >> you won't be able to go back to other and it might be a problem (e.g.
> > >> there is some top-level block for powering on all CSI instances).
> > >
> > > We're pretty sure these are multiple instances of the IP block as they
> > > can operate completely independently.
> > >
> > >>>
> > >>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
> > >>> when the per-SoC DT is going to have the same info, so my suggestion was
> > >>> to simplify the driver and have this kind of hardware description in the
> > >>> DT, and keep the driver simple, but we are definitely open to learning
> > >>> the "right way" of doing this.
> > >>
> > >> The property then is reasonable. It should not be bool, though, because
> > >> it does not scale. There can be next block which supports only D-PHY on
> > >> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
> > >> configurations.
> > >
> > > OK, looks like include/dt-bindings/phy/phy.y already has
> > >
> > >   #define PHY_TYPE_DPHY		10
> > >   #define PHY_TYPE_CPHY		11
> > >
> > > we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?

No, because these defines are the 1 mode used for a specific connection, 
not the set of supported modes.

> >
> > Yes. Currently it is usually used as phy-cells argument (after the phy
> > number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as
> > property in provider. In both cases they have a bit different meaning
> > than yours. You want to list all supported modes or narrow/restrict
> > them. Maybe hisilicon,fixed-mode fits your purpose?

There is also 'phy-type' to set the mode/type in the provider.

If we need to list all possible modes, then you need to justify why 
that's needed and then define a property which is a mask of the 
existing type defines.

> >
> 
> Hi Krzysztof,
> 
> Thanks for the suggestion, using something like hisilicon,fixed-node
> looks like a good fit.
> With mediatek,fixed-node, by default CSI node will be considered as
> CD-PHY capable (unless the fixed-mode property is set.) so I won't need
> anymore the new define PHY_TYPE_CDPHY introduced in v3.
> 
> Also introducing mediatek,fixed-mode suggests that PHYs not declaring
> the fixed mode property support mode selection, so I suggest to add a
> phy argument (#phy-cells = <1>) to select the mode (D or C mode).
> Exactly what is done by the hsilicon driver.
> 
> How does that sound to you?

I don't follow the need for fixed-mode, but agree you should use a phy 
arg. Can't you just assume the D-PHY only instance will only have D-PHY 
for the arg?

Rob

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Chunfeng Yun (云春峰)]

On Tue, 2023-05-16 at 11:41 +0200, Julien Stephan wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, May 16, 2023 at 10:07:47AM +0200, Krzysztof Kozlowski wrote:
> > On 15/05/2023 11:05, Julien Stephan wrote:
> > > From: Florian Sylvestre <fsylvestre@baylibre.com>
> > > 
> > > This adds the bindings, for the MIPI CD-PHY module v 0.5 embedded
> > > in
> > > some Mediatek soc, such as the mt8365
> > > 
> > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> > 
> > What are the changes? IOW: changelog here or in cover letter.
> > 
> 
> Hi Krzysztof,
> I added a changelog in the cover letter, but I will try to be more
> descritpive next time. Changes from v1 are mainly style issues fixed
> (mostly from your first review)
> 
> > Subject: you have some multiple spaces.
> > 
> > Subject: drop driver. Bindings are not for drivers.
> > 
> > > ---
> > >  .../phy/mediatek,phy-mipi-csi-0-5.yaml        | 62
> > > +++++++++++++++++++
> > >  MAINTAINERS                                   |  6 ++
> > >  2 files changed, 68 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-
> > > 5.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/phy/mediatek,phy-
> > > mipi-csi-0-5.yaml
> > > b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-
> > > 5.yaml
> > > new file mode 100644
> > > index 000000000000..5aa8c0b41cdf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/mediatek,phy-mipi-
> > > csi-0-5.yaml
> > > @@ -0,0 +1,62 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-Only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: 
> > > https://urldefense.com/v3/__http://devicetree.org/schemas/phy/mediatek,phy-mipi-csi-0-5.yaml*__;Iw!!CTRNKA9wMg0ARbw!kUABjbQO6-_jrANKiqoLPxZkh_0LAplQwlgCG9lhmCKm2ec8pCItYUB3-zQ2PGBnlAcgsBhwrOgi5qeilrddZ5s$
> > > +$schema: 
> > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!kUABjbQO6-_jrANKiqoLPxZkh_0LAplQwlgCG9lhmCKm2ec8pCItYUB3-zQ2PGBnlAcgsBhwrOgi5qeiABoSu-M$
> > > +
> > > +title: Mediatek Sensor Interface MIPI CSI CD-PHY
> > > +
> > > +maintainers:
> > > +  - Julien Stephan <jstephan@baylibre.com>
> > > +  - Andy Hsieh <andy.hsieh@mediatek.com>
> > > +
> > > +description:
> > > +  The SENINF CD-PHY is a set of CD-PHY connected to the SENINF
> > > CSI-2
> > > +  receivers. The number of PHYs depends on the SoC model.
> > > +  Depending on the soc model, each PHYs can support CDPHY or
> > > DPHY only
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - mediatek,phy-mipi-csi-0-5
> > 
> > SoC based compatibles. 0-5 is odd.
> > 
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  '#phy-cells':
> > > +    const: 0
> > > +
> > > +  mediatek,is_cdphy:
> > 
> > No underscores in node names.
> > 
> > > +    description:
> > > +      Specify if the current phy support CDPHY configuration
> > 
> > Why this cannot be implied from compatible? Add specific
> > compatibles.
> > 
> > 
> 
> This cannot be implied by compatible because the number of phys
> depends
> on the soc and each phy can be either D-PHY only or CD-PHY capable.
> For example mt8365 has 2 phy: CSI0 and CSI1. CSI1 is DPHY only and
> CSI0 is CD-PHY

Do the user need to know which type phy he will use?

> 
> Regards,
> Julien
> > > +    type: boolean
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - '#phy-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    soc {
> > > +      #address-cells = <2>;
> > > +      #size-cells = <2>;
> > > +
> > > +      mipi_rx_csi0: mipi_rx_csi0@11c10000 {
> > 
> > No underscores in node names. How this is v2?
> > 
> > > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > > +        reg = <0 0x11C10000 0 0x2000>;
> > > +        status = "okay";
> > 
> > Drop
> > 
> > > +        mediatek,is_cdphy;
> > > +        #phy-cells = <0>;
> > > +      };
> > > +
> > > +      mipi_rx_csi1: mipi-rx-csi1@11c12000 {
> > > +        compatible = "mediatek,phy-mipi-csi-0-5";
> > > +        reg = <0 0x11C12000 0 0x2000>;
> > > +        status = "disabled";
> > 
> > ???
> > 
> > 
> > Best regards,
> > Krzysztof
> > 
-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[Julien Stephan]

From: Phi-bang Nguyen <pnguyen@baylibre.com>

This is a new driver that supports the MIPI CSI CD-PHY version 0.5

The number of PHYs depend on the soc.

Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
[Julien Stephan: use GENMASK]
[Julien Stephan: refactor code]
[Julien Stephan: update device tree support and probe function]
Co-developed-by: Julien Stephan <jstephan@baylibre.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 MAINTAINERS                                   |   1 +
 drivers/phy/mediatek/Kconfig                  |   8 +
 drivers/phy/mediatek/Makefile                 |   2 +
 .../mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h    |  58 ++++
 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c   | 257 ++++++++++++++++++
 5 files changed, 326 insertions(+)
 create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
 create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 44f0ff11e984..fc2766cb50d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13103,6 +13103,7 @@ M:	Julien Stephan <jstephan@baylibre.com>
 M:	Andy Hsieh <andy.hsieh@mediatek.com>
 S:	Supported
 F:	Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
+F:	drivers/phy/mediatek/phy-mtk-mipi-csi-0-5*
 
 MEDIATEK MMC/SD/SDIO DRIVER
 M:	Chaotian Jing <chaotian.jing@mediatek.com>
diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
index 3125ecb5d119..452bc7ac5ce5 100644
--- a/drivers/phy/mediatek/Kconfig
+++ b/drivers/phy/mediatek/Kconfig
@@ -74,3 +74,11 @@ config PHY_MTK_DP
 	select GENERIC_PHY
 	help
 	  Support DisplayPort PHY for MediaTek SoCs.
+
+config PHY_MTK_MIPI_CSI_0_5
+	tristate "MediaTek CSI CD-PHY v 0.5 Driver"
+	depends on ARCH_MEDIATEK && OF
+	select GENERIC_PHY
+	help
+	  Enable this to support the MIPI CSI CD-PHY receiver version 0.5.
+	  The driver supports multiple CSI cdphy ports simultaneously.
diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
index fb1f8edaffa7..8eb7b8747c67 100644
--- a/drivers/phy/mediatek/Makefile
+++ b/drivers/phy/mediatek/Makefile
@@ -18,3 +18,5 @@ phy-mtk-mipi-dsi-drv-y			:= phy-mtk-mipi-dsi.o
 phy-mtk-mipi-dsi-drv-y			+= phy-mtk-mipi-dsi-mt8173.o
 phy-mtk-mipi-dsi-drv-y			+= phy-mtk-mipi-dsi-mt8183.o
 obj-$(CONFIG_PHY_MTK_MIPI_DSI)		+= phy-mtk-mipi-dsi-drv.o
+
+obj-$(CONFIG_PHY_MTK_MIPI_CSI_0_5)	+= phy-mtk-mipi-csi-0-5.o
diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
new file mode 100644
index 000000000000..e9a7f1ab3e2f
--- /dev/null
+++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__
+#define __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__
+
+/*
+ * CSI1 and CSI2 are identical, and similar to CSI0. All CSIx macros are
+ * applicable to the three PHYs. Where differences exist, they are denoted by
+ * macro names using CSI0 and CSI1, the latter being applicable to CSI1 and
+ * CSI2 alike.
+ */
+
+#define MIPI_RX_ANA00_CSIxA			0x0000
+#define RG_CSI0A_CPHY_EN			BIT(0)
+#define RG_CSIxA_EQ_PROTECT_EN			BIT(1)
+#define RG_CSIxA_BG_LPF_EN			BIT(2)
+#define RG_CSIxA_BG_CORE_EN			BIT(3)
+#define RG_CSIxA_DPHY_L0_CKMODE_EN		BIT(5)
+#define RG_CSIxA_DPHY_L0_CKSEL			BIT(6)
+#define RG_CSIxA_DPHY_L1_CKMODE_EN		BIT(8)
+#define RG_CSIxA_DPHY_L1_CKSEL			BIT(9)
+#define RG_CSIxA_DPHY_L2_CKMODE_EN		BIT(11)
+#define RG_CSIxA_DPHY_L2_CKSEL			BIT(12)
+
+#define MIPI_RX_ANA18_CSIxA			0x0018
+#define RG_CSI0A_L0_T0AB_EQ_IS			GENMASK(5, 4)
+#define RG_CSI0A_L0_T0AB_EQ_BW			GENMASK(7, 6)
+#define RG_CSI0A_L1_T1AB_EQ_IS			GENMASK(21, 20)
+#define RG_CSI0A_L1_T1AB_EQ_BW			GENMASK(23, 22)
+#define RG_CSI0A_L2_T1BC_EQ_IS			GENMASK(21, 20)
+#define RG_CSI0A_L2_T1BC_EQ_BW			GENMASK(23, 22)
+#define RG_CSI1A_L0_EQ_IS			GENMASK(5, 4)
+#define RG_CSI1A_L0_EQ_BW			GENMASK(7, 6)
+#define RG_CSI1A_L1_EQ_IS			GENMASK(21, 20)
+#define RG_CSI1A_L1_EQ_BW			GENMASK(23, 22)
+#define RG_CSI1A_L2_EQ_IS			GENMASK(5, 4)
+#define RG_CSI1A_L2_EQ_BW			GENMASK(7, 6)
+
+#define MIPI_RX_ANA1C_CSIxA			0x001c
+#define MIPI_RX_ANA20_CSI0A			0x0020
+
+#define MIPI_RX_ANA24_CSIxA			0x0024
+#define RG_CSIxA_RESERVE			GENMASK(31, 24)
+
+#define MIPI_RX_ANA40_CSIxA			0x0040
+#define RG_CSIxA_CPHY_FMCK_SEL			GENMASK(1, 0)
+#define RG_CSIxA_ASYNC_OPTION			GENMASK(7, 4)
+#define RG_CSIxA_CPHY_SPARE			GENMASK(31, 16)
+
+#define MIPI_RX_WRAPPER80_CSIxA			0x0080
+#define CSR_CSI_RST_MODE			GENMASK(17, 16)
+
+#define MIPI_RX_ANAA8_CSIxA			0x00a8
+#define RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT	BIT(0)
+#define RG_CSIxA_DPHY_L1_BYTECK_INVERT		BIT(1)
+#define RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT	BIT(2)
+
+#endif
diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
new file mode 100644
index 000000000000..ae2d3dc9631d
--- /dev/null
+++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "phy-mtk-io.h"
+#include "phy-mtk-mipi-csi-0-5-rx-reg.h"
+
+#define CSIxB_OFFSET		0x1000
+
+struct mtk_mipi_dphy;
+
+struct mtk_mipi_dphy_port {
+	struct device *dev;
+	void __iomem *base;
+	struct phy *phy;
+	bool is_cdphy;
+};
+
+static int mtk_mipi_phy_power_on(struct phy *phy)
+{
+	struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
+	void __iomem *base = port->base;
+
+	/*
+	 * Only DPHY is supported for now,
+	 * so set analog phy mode to DPHY in CDPHY compatible PHYs
+	 */
+	if (port->is_cdphy) {
+		mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+				     RG_CSI0A_CPHY_EN, 0);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+				     RG_CSI0A_CPHY_EN, 0);
+	}
+
+	/*
+	 * Lane configuration:
+	 *
+	 * Only 4 data + 1 clock is supported for now with the following mapping:
+	 *
+	 * CSIxA_LNR0 --> D2
+	 * CSIxA_LNR1 --> D0
+	 * CSIxA_LNR2 --> C
+	 * CSIxB_LNR0 --> D1
+	 * CSIxB_LNR1 --> D3
+	 */
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L0_CKSEL, 1);
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L1_CKSEL, 1);
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L2_CKMODE_EN, 1);
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L2_CKSEL, 1);
+
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L0_CKSEL, 1);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L1_CKSEL, 1);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L2_CKMODE_EN, 0);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_DPHY_L2_CKSEL, 1);
+
+	/* Byte clock invert */
+	mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
+			     RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
+	mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
+			     RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
+	mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
+			     RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
+
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
+			     RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
+			     RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
+			     RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
+
+	/* Start ANA EQ tuning */
+	if (port->is_cdphy) {
+		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI0A_L0_T0AB_EQ_IS, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI0A_L0_T0AB_EQ_BW, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI0A_L1_T1AB_EQ_IS, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI0A_L1_T1AB_EQ_BW, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
+				     RG_CSI0A_L2_T1BC_EQ_IS, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
+				     RG_CSI0A_L2_T1BC_EQ_BW, 1);
+
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI0A_L0_T0AB_EQ_IS, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI0A_L0_T0AB_EQ_BW, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI0A_L1_T1AB_EQ_IS, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI0A_L1_T1AB_EQ_BW, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
+				     RG_CSI0A_L2_T1BC_EQ_IS, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
+				     RG_CSI0A_L2_T1BC_EQ_BW, 1);
+	} else {
+		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L0_EQ_IS, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L0_EQ_BW, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L1_EQ_IS, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L1_EQ_BW, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI1A_L2_EQ_IS, 1);
+		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI1A_L2_EQ_BW, 1);
+
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L0_EQ_IS, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L0_EQ_BW, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L1_EQ_IS, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+				     RG_CSI1A_L1_EQ_BW, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI1A_L2_EQ_IS, 1);
+		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+				     RG_CSI1A_L2_EQ_BW, 1);
+	}
+
+	/* End ANA EQ tuning */
+	mtk_phy_set_bits(base + MIPI_RX_ANA40_CSIxA, 0x90);
+
+	mtk_phy_update_field(base + MIPI_RX_ANA24_CSIxA,
+			     RG_CSIxA_RESERVE, 0x40);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA24_CSIxA,
+			     RG_CSIxA_RESERVE, 0x40);
+	mtk_phy_update_field(base + MIPI_RX_WRAPPER80_CSIxA,
+			     CSR_CSI_RST_MODE, 0);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_WRAPPER80_CSIxA,
+			     CSR_CSI_RST_MODE, 0);
+	/* ANA power on */
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_CORE_EN, 1);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_CORE_EN, 1);
+	usleep_range(20, 40);
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_LPF_EN, 1);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_LPF_EN, 1);
+
+	return 0;
+}
+
+static int mtk_mipi_phy_power_off(struct phy *phy)
+{
+	struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
+	void __iomem *base = port->base;
+
+	/* Disable MIPI BG. */
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_CORE_EN, 0);
+	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_LPF_EN, 0);
+
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_CORE_EN, 0);
+	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+			     RG_CSIxA_BG_LPF_EN, 0);
+
+	return 0;
+}
+
+static const struct phy_ops mtk_dphy_ops = {
+	.power_on	= mtk_mipi_phy_power_on,
+	.power_off	= mtk_mipi_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int mtk_mipi_dphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy_provider *phy_provider;
+	struct mtk_mipi_dphy_port *port;
+	struct phy *phy;
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, port);
+
+	port->dev = dev;
+
+	port->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(port->base))
+		return PTR_ERR(port->base);
+
+	port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");
+
+	phy = devm_phy_create(dev, NULL, &mtk_dphy_ops);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "Failed to create PHY: %ld\n", PTR_ERR(phy));
+		return PTR_ERR(phy);
+	}
+
+	port->phy = phy;
+	phy_set_drvdata(phy, port);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider)) {
+		dev_err(dev, "Failed to register PHY provider: %ld\n",
+			PTR_ERR(phy_provider));
+		return PTR_ERR(phy_provider);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id mtk_mipi_dphy_of_match[] = {
+	{.compatible = "mediatek,phy-mipi-csi-0-5"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_mipi_dphy_of_match);
+
+static struct platform_driver mipi_dphy_pdrv = {
+	.probe = mtk_mipi_dphy_probe,
+	.driver	= {
+		.name	= "mtk-mipi-csi-0-5",
+		.of_match_table = mtk_mipi_dphy_of_match,
+	},
+};
+
+module_platform_driver(mipi_dphy_pdrv);
+
+MODULE_DESCRIPTION("MTK mipi csi cdphy driver");
+MODULE_AUTHOR("Louis Kuo <louis.kuo@mediatek.com>");
+MODULE_LICENSE("GPL");
-- 
2.40.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[AngeloGioacchino Del Regno]

Il 15/05/23 11:05, Julien Stephan ha scritto:
> From: Phi-bang Nguyen <pnguyen@baylibre.com>
> 
> This is a new driver that supports the MIPI CSI CD-PHY version 0.5
> 
> The number of PHYs depend on the soc.
> 
> Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
> Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
> [Julien Stephan: use GENMASK]
> [Julien Stephan: refactor code]
> [Julien Stephan: update device tree support and probe function]
> Co-developed-by: Julien Stephan <jstephan@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
>   MAINTAINERS                                   |   1 +
>   drivers/phy/mediatek/Kconfig                  |   8 +
>   drivers/phy/mediatek/Makefile                 |   2 +
>   .../mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h    |  58 ++++
>   drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c   | 257 ++++++++++++++++++
>   5 files changed, 326 insertions(+)
>   create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
>   create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 44f0ff11e984..fc2766cb50d3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13103,6 +13103,7 @@ M:	Julien Stephan <jstephan@baylibre.com>
>   M:	Andy Hsieh <andy.hsieh@mediatek.com>
>   S:	Supported
>   F:	Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> +F:	drivers/phy/mediatek/phy-mtk-mipi-csi-0-5*
>   
>   MEDIATEK MMC/SD/SDIO DRIVER
>   M:	Chaotian Jing <chaotian.jing@mediatek.com>
> diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
> index 3125ecb5d119..452bc7ac5ce5 100644
> --- a/drivers/phy/mediatek/Kconfig
> +++ b/drivers/phy/mediatek/Kconfig
> @@ -74,3 +74,11 @@ config PHY_MTK_DP
>   	select GENERIC_PHY
>   	help
>   	  Support DisplayPort PHY for MediaTek SoCs.
> +
> +config PHY_MTK_MIPI_CSI_0_5
> +	tristate "MediaTek CSI CD-PHY v 0.5 Driver"

"MediaTek CSI CD-PHY v0.5 Driver"

> +	depends on ARCH_MEDIATEK && OF
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support the MIPI CSI CD-PHY receiver version 0.5.
> +	  The driver supports multiple CSI cdphy ports simultaneously.
> diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
> index fb1f8edaffa7..8eb7b8747c67 100644
> --- a/drivers/phy/mediatek/Makefile
> +++ b/drivers/phy/mediatek/Makefile
> @@ -18,3 +18,5 @@ phy-mtk-mipi-dsi-drv-y			:= phy-mtk-mipi-dsi.o
>   phy-mtk-mipi-dsi-drv-y			+= phy-mtk-mipi-dsi-mt8173.o
>   phy-mtk-mipi-dsi-drv-y			+= phy-mtk-mipi-dsi-mt8183.o
>   obj-$(CONFIG_PHY_MTK_MIPI_DSI)		+= phy-mtk-mipi-dsi-drv.o
> +
> +obj-$(CONFIG_PHY_MTK_MIPI_CSI_0_5)	+= phy-mtk-mipi-csi-0-5.o
> diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
> new file mode 100644
> index 000000000000..e9a7f1ab3e2f
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__
> +#define __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__

What about....

__PHY_MTK_MIPI_CSI_V_0_5_RX_REG_H ?

> +
> +/*
> + * CSI1 and CSI2 are identical, and similar to CSI0. All CSIx macros are
> + * applicable to the three PHYs. Where differences exist, they are denoted by
> + * macro names using CSI0 and CSI1, the latter being applicable to CSI1 and
> + * CSI2 alike.
> + */
> +
> +#define MIPI_RX_ANA00_CSIxA			0x0000

I would rename all those from "CSIx" to "CSIX" (so, just toupper('x')).

> +#define RG_CSI0A_CPHY_EN			BIT(0)

..snip..

> +
> +#endif
> diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
> new file mode 100644
> index 000000000000..ae2d3dc9631d
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "phy-mtk-io.h"
> +#include "phy-mtk-mipi-csi-0-5-rx-reg.h"
> +
> +#define CSIxB_OFFSET		0x1000

What if we grab two (or three?) iospaces from devicetree?

- base (global)
- csi_a
- csi_b

That would make it possible to maybe eventually extend this driver to more
versions (older or newer) of the CSI PHY IP without putting fixes offsets
inside of platform data structures and such.

> +
> +struct mtk_mipi_dphy;
> +
> +struct mtk_mipi_dphy_port {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct phy *phy;
> +	bool is_cdphy;
> +};
> +
> +static int mtk_mipi_phy_power_on(struct phy *phy)
> +{
> +	struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
> +	void __iomem *base = port->base;
> +
> +	/*
> +	 * Only DPHY is supported for now,
> +	 * so set analog phy mode to DPHY in CDPHY compatible PHYs
> +	 */
> +	if (port->is_cdphy) {
> +		mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +				     RG_CSI0A_CPHY_EN, 0);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +				     RG_CSI0A_CPHY_EN, 0);
> +	}
> +
> +	/*
> +	 * Lane configuration:
> +	 *
> +	 * Only 4 data + 1 clock is supported for now with the following mapping:
> +	 *
> +	 * CSIxA_LNR0 --> D2
> +	 * CSIxA_LNR1 --> D0
> +	 * CSIxA_LNR2 --> C
> +	 * CSIxB_LNR0 --> D1
> +	 * CSIxB_LNR1 --> D3
> +	 */
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L0_CKSEL, 1);
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L1_CKSEL, 1);
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L2_CKMODE_EN, 1);
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L2_CKSEL, 1);
> +
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L0_CKSEL, 1);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L1_CKSEL, 1);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L2_CKMODE_EN, 0);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_DPHY_L2_CKSEL, 1);
> +
> +	/* Byte clock invert */
> +	mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
> +			     RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
> +	mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
> +			     RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
> +	mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
> +			     RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
> +
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
> +			     RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
> +			     RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
> +			     RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
> +
> +	/* Start ANA EQ tuning */
> +	if (port->is_cdphy) {

statid void mtk_phy_csi_analog_eq_tune(struct mtk_mipi_dphy_port *port)
{
	if (port->is_cdphy)
		mtk_phy_csi_dphy_ana_eq_tune(...)
	else
		mtk_phy_csi_cphy_ana_eq_tune(...)

	/* CPHY/DPHY common "end of tuning" sequence below */
	mtk_phy_update_field( ... stuff ...);
}

...then all those calls will also fit in one line due to the reduced
indentation, improving readability and reducing line count.

> +		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI0A_L0_T0AB_EQ_IS, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI0A_L0_T0AB_EQ_BW, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI0A_L1_T1AB_EQ_IS, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI0A_L1_T1AB_EQ_BW, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
> +				     RG_CSI0A_L2_T1BC_EQ_IS, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
> +				     RG_CSI0A_L2_T1BC_EQ_BW, 1);
> +
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI0A_L0_T0AB_EQ_IS, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI0A_L0_T0AB_EQ_BW, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI0A_L1_T1AB_EQ_IS, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI0A_L1_T1AB_EQ_BW, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
> +				     RG_CSI0A_L2_T1BC_EQ_IS, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
> +				     RG_CSI0A_L2_T1BC_EQ_BW, 1);
> +	} else {
> +		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L0_EQ_IS, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L0_EQ_BW, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L1_EQ_IS, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L1_EQ_BW, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI1A_L2_EQ_IS, 1);
> +		mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI1A_L2_EQ_BW, 1);
> +
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L0_EQ_IS, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L0_EQ_BW, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L1_EQ_IS, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> +				     RG_CSI1A_L1_EQ_BW, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI1A_L2_EQ_IS, 1);
> +		mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> +				     RG_CSI1A_L2_EQ_BW, 1);
> +	}
> +
> +	/* End ANA EQ tuning */
> +	mtk_phy_set_bits(base + MIPI_RX_ANA40_CSIxA, 0x90);
> +
> +	mtk_phy_update_field(base + MIPI_RX_ANA24_CSIxA,
> +			     RG_CSIxA_RESERVE, 0x40);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA24_CSIxA,
> +			     RG_CSIxA_RESERVE, 0x40);
> +	mtk_phy_update_field(base + MIPI_RX_WRAPPER80_CSIxA,
> +			     CSR_CSI_RST_MODE, 0);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_WRAPPER80_CSIxA,
> +			     CSR_CSI_RST_MODE, 0);
> +	/* ANA power on */
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_CORE_EN, 1);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_CORE_EN, 1);
> +	usleep_range(20, 40);
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_LPF_EN, 1);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_LPF_EN, 1);
> +
> +	return 0;
> +}
> +
> +static int mtk_mipi_phy_power_off(struct phy *phy)
> +{
> +	struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
> +	void __iomem *base = port->base;
> +
> +	/* Disable MIPI BG. */
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_CORE_EN, 0);
> +	mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_LPF_EN, 0);
> +
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_CORE_EN, 0);
> +	mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> +			     RG_CSIxA_BG_LPF_EN, 0);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops mtk_dphy_ops = {
> +	.power_on	= mtk_mipi_phy_power_on,
> +	.power_off	= mtk_mipi_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int mtk_mipi_dphy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	struct mtk_mipi_dphy_port *port;
> +	struct phy *phy;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, port);
> +
> +	port->dev = dev;
> +
> +	port->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(port->base))
> +		return PTR_ERR(port->base);
> +
> +	port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");

This driver doesn't support C-PHY mode, so you either add support for that, or in
my opinion you should simply refuse to probe it, as it is *dysfunctional* for the
unsupported case (and might even introduce unstabilities).

	/* At the moment, only D-PHY mode is supported */
	if (!port->is_cdphy)
		return -EINVAL;

Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy"
is fine.

> +
> +	phy = devm_phy_create(dev, NULL, &mtk_dphy_ops);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "Failed to create PHY: %ld\n", PTR_ERR(phy));
> +		return PTR_ERR(phy);
> +	}
> +
> +	port->phy = phy;
> +	phy_set_drvdata(phy, port);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		dev_err(dev, "Failed to register PHY provider: %ld\n",
> +			PTR_ERR(phy_provider));
> +		return PTR_ERR(phy_provider);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mtk_mipi_dphy_of_match[] = {
> +	{.compatible = "mediatek,phy-mipi-csi-0-5"},

leave spaces.

	{ .comp... " },

...and always end with

	{ /* sentinel */ }


Also, please follow what the other PHY drivers do and use a SoC model,
example:

"mediatek,mt7777-csi-phy", or "mediatek,mt8888-csi-rx"

where the latter would make more sense imo.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_mipi_dphy_of_match);
> +
> +static struct platform_driver mipi_dphy_pdrv = {
> +	.probe = mtk_mipi_dphy_probe,
> +	.driver	= {
> +		.name	= "mtk-mipi-csi-0-5",
> +		.of_match_table = mtk_mipi_dphy_of_match,
> +	},
> +};
> +

remove extra blank line here.

> +module_platform_driver(mipi_dphy_pdrv);
> +
> +MODULE_DESCRIPTION("MTK mipi csi cdphy driver");

"MediaTek MIPI CSI CDPHY Driver"

> +MODULE_AUTHOR("Louis Kuo <louis.kuo@mediatek.com>");
> +MODULE_LICENSE("GPL");

Regards,
Angelo


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[Julien Stephan]

On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/05/23 11:05, Julien Stephan ha scritto:
 ..snip..
> > +	port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");
>
> This driver doesn't support C-PHY mode, so you either add support for that, or in
> my opinion you should simply refuse to probe it, as it is *dysfunctional* for the
> unsupported case (and might even introduce unstabilities).
>
> 	/* At the moment, only D-PHY mode is supported */
> 	if (!port->is_cdphy)
> 		return -EINVAL;
>
> Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy"
> is fine.
>
Hi Angelo,
You are right this driver does not support C-PHY mode, but some of the
PHYs themselves support BOTH C-PHY AND D-PHY. The idea of `is_cdphy` variable
is to know if the CSI port supports BOTH C-PHY AND D-PHY or only DPHY.
For example mt8365 has 2 PHYs: CSI0 and CSI1. CSI1 support only D-PHY,
while CSI0 can be configured in C-PHY or D-PHY. Registers for CD-PHY and
D-PHY are almost identical, except that CD-PHY compatible has some extra
bitfields to configure properly the mode and the lanes (because supporting
trios for CD-PHY).
If C-PHY support is eventually added into the driver, I think we will need
another variable such as `mode` to know the mode. I was also thinking
of adding a phy argument to determine if the mode is C-PHY or D-PHY.

So here, I don't want to stop the probe if `is_cdphy` variable is set to
true. Does it make sense ?

Regards
Julien

.. snip..
> > +
> > +	phy = devm_phy_create(dev, NULL, &mtk_dphy_ops);
> > +	if (IS_ERR(phy)) {
> > +		dev_err(dev, "Failed to create PHY: %ld\n", PTR_ERR(phy));
> > +		return PTR_ERR(phy);
> > +	}
>
> Regards,
> Angelo
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[AngeloGioacchino Del Regno]

Il 15/05/23 15:36, Julien Stephan ha scritto:
> On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 15/05/23 11:05, Julien Stephan ha scritto:
>   ..snip..
>>> +	port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");
>>
>> This driver doesn't support C-PHY mode, so you either add support for that, or in
>> my opinion you should simply refuse to probe it, as it is *dysfunctional* for the
>> unsupported case (and might even introduce unstabilities).
>>
>> 	/* At the moment, only D-PHY mode is supported */
>> 	if (!port->is_cdphy)
>> 		return -EINVAL;
>>
>> Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy"
>> is fine.
>>
> Hi Angelo,
> You are right this driver does not support C-PHY mode, but some of the
> PHYs themselves support BOTH C-PHY AND D-PHY. The idea of `is_cdphy` variable
> is to know if the CSI port supports BOTH C-PHY AND D-PHY or only DPHY.
> For example mt8365 has 2 PHYs: CSI0 and CSI1. CSI1 support only D-PHY,
> while CSI0 can be configured in C-PHY or D-PHY. Registers for CD-PHY and
> D-PHY are almost identical, except that CD-PHY compatible has some extra
> bitfields to configure properly the mode and the lanes (because supporting
> trios for CD-PHY).
> If C-PHY support is eventually added into the driver, I think we will need
> another variable such as `mode` to know the mode. I was also thinking
> of adding a phy argument to determine if the mode is C-PHY or D-PHY.
> 
> So here, I don't want to stop the probe if `is_cdphy` variable is set to
> true. Does it make sense ?
> 

Comments in the code convinced me that the other PHYs providing only C or D PHY
support weren't compatible at all with this driver.

I got it now - but at this point can you please add a comment in the code actually
clarifying that this driver supports both PHYs providing *only* D-PHY and ones
providing selectable C-or-D PHY?

That clarified, it would not make sense to stop probing if it's not a CDPHY because
as you said there might be a D-only PHY that would be actually supported here.

Regards,
Angelo



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[Julien Stephan]

On Mon, May 15, 2023 at 04:22:38PM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/05/23 15:36, Julien Stephan ha scritto:
> > On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
> > > Il 15/05/23 11:05, Julien Stephan ha scritto:
> >   ..snip..
> > > > +	port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");
> > >
> > > This driver doesn't support C-PHY mode, so you either add support for that, or in
> > > my opinion you should simply refuse to probe it, as it is *dysfunctional* for the
> > > unsupported case (and might even introduce unstabilities).
> > >
> > > 	/* At the moment, only D-PHY mode is supported */
> > > 	if (!port->is_cdphy)
> > > 		return -EINVAL;
> > >
> > > Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy"
> > > is fine.
> > >
> > Hi Angelo,
> > You are right this driver does not support C-PHY mode, but some of the
> > PHYs themselves support BOTH C-PHY AND D-PHY. The idea of `is_cdphy` variable
> > is to know if the CSI port supports BOTH C-PHY AND D-PHY or only DPHY.
> > For example mt8365 has 2 PHYs: CSI0 and CSI1. CSI1 support only D-PHY,
> > while CSI0 can be configured in C-PHY or D-PHY. Registers for CD-PHY and
> > D-PHY are almost identical, except that CD-PHY compatible has some extra
> > bitfields to configure properly the mode and the lanes (because supporting
> > trios for CD-PHY).
> > If C-PHY support is eventually added into the driver, I think we will need
> > another variable such as `mode` to know the mode. I was also thinking
> > of adding a phy argument to determine if the mode is C-PHY or D-PHY.
> >
> > So here, I don't want to stop the probe if `is_cdphy` variable is set to
> > true. Does it make sense ?
> >
>
> Comments in the code convinced me that the other PHYs providing only C or D PHY
> support weren't compatible at all with this driver.
>
> I got it now - but at this point can you please add a comment in the code actually
> clarifying that this driver supports both PHYs providing *only* D-PHY and ones
> providing selectable C-or-D PHY?
>
> That clarified, it would not make sense to stop probing if it's not a CDPHY because
> as you said there might be a D-only PHY that would be actually supported here.
>
> Regards,
> Angelo
>
>
Ok, I will add a comment in the code to make it more clear.

Regards
Julien

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[Julien Stephan]

On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
> > +#define CSIxB_OFFSET		0x1000
>
> What if we grab two (or three?) iospaces from devicetree?
>
> - base (global)
> - csi_a
> - csi_b
>
> That would make it possible to maybe eventually extend this driver to more
> versions (older or newer) of the CSI PHY IP without putting fixes offsets
> inside of platform data structures and such.
>
Hi Angelo,
The register bank of the CSI port is divided into 2:
* from base address to base + 0x1000 (port A)
* from base + 0x1000 to base +0x2000 (port B)
Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
1 clock) and use either port A or port B.

For example  mt8365 has CSI0 that can be used either in 4D1C mode or in
2 * 2D1C and CSI1 which can use only 4D1C mode

2D1C mode can not be tested and is not implemented in the driver so
I guess adding csi_a and csi_b reg value may be confusing?

What do you think?

Regards,
Julien

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[AngeloGioacchino Del Regno]

Il 15/05/23 16:07, Julien Stephan ha scritto:
> On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
>>> +#define CSIxB_OFFSET		0x1000
>>
>> What if we grab two (or three?) iospaces from devicetree?
>>
>> - base (global)
>> - csi_a
>> - csi_b
>>
>> That would make it possible to maybe eventually extend this driver to more
>> versions (older or newer) of the CSI PHY IP without putting fixes offsets
>> inside of platform data structures and such.
>>
> Hi Angelo,
> The register bank of the CSI port is divided into 2:
> * from base address to base + 0x1000 (port A)
> * from base + 0x1000 to base +0x2000 (port B)
> Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
> the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
> 1 clock) and use either port A or port B.
> 
> For example  mt8365 has CSI0 that can be used either in 4D1C mode or in
> 2 * 2D1C and CSI1 which can use only 4D1C mode
> 
> 2D1C mode can not be tested and is not implemented in the driver so
> I guess adding csi_a and csi_b reg value may be confusing?
> 
> What do you think?

Ok so we're talking about two data lanes per CSI port... it may still be
beneficial to split the two register regions as

reg-names = "csi-a", "csi-b"; (whoops, I actually used underscores before,
                                and that was a mistake, sorry!)

....but that would be actually good only if we are expecting to get a CSI
PHY in the future with four data lanes per port.

If you do *not* expect at all such a CSI PHY, or you do *not* expect such
a PHY to ever be compatible with this driver (read as: if you expect such
a PHY to be literally completely different from this one), then it would
not change much to have the registers split in two.

Another case in which it would make sense is if we were to get a PHY that
provides more than two CSI ports: in that case, we'd avoid platform data
machinery to check the number of actual ports in the IP, as we would be
just checking how many register regions we were given from the devicetree,
meaning that if we got "csi-a", "csi-b", "csi-c", "csi-d", we have four
ports.

Besides, another thing to think about is... yes you cannot test nor implement
2D1C mode in your submission, but this doesn't mean that others won't ever be
interested in this and that other people won't be actually implementing that;
Providing them with the right initial driver structure will surely make things
easier, encouraging other people from the community to spend their precious
time on the topic.

Regards,
Angelo


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[Julien Stephan]

On Mon, May 15, 2023 at 04:32:42PM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/05/23 16:07, Julien Stephan ha scritto:
> > On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
> > > > +#define CSIxB_OFFSET		0x1000
> > >
> > > What if we grab two (or three?) iospaces from devicetree?
> > >
> > > - base (global)
> > > - csi_a
> > > - csi_b
> > >
> > > That would make it possible to maybe eventually extend this driver to more
> > > versions (older or newer) of the CSI PHY IP without putting fixes offsets
> > > inside of platform data structures and such.
> > >
> > Hi Angelo,
> > The register bank of the CSI port is divided into 2:
> > * from base address to base + 0x1000 (port A)
> > * from base + 0x1000 to base +0x2000 (port B)
> > Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
> > the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
> > 1 clock) and use either port A or port B.
> >
> > For example  mt8365 has CSI0 that can be used either in 4D1C mode or in
> > 2 * 2D1C and CSI1 which can use only 4D1C mode
> >
> > 2D1C mode can not be tested and is not implemented in the driver so
> > I guess adding csi_a and csi_b reg value may be confusing?
> >
> > What do you think?
>
> Ok so we're talking about two data lanes per CSI port... it may still be
> beneficial to split the two register regions as
>
> reg-names = "csi-a", "csi-b"; (whoops, I actually used underscores before,
>                                and that was a mistake, sorry!)
>
> ....but that would be actually good only if we are expecting to get a CSI
> PHY in the future with four data lanes per port.
>
> If you do *not* expect at all such a CSI PHY, or you do *not* expect such
> a PHY to ever be compatible with this driver (read as: if you expect such
> a PHY to be literally completely different from this one), then it would
> not change much to have the registers split in two.
>
> Another case in which it would make sense is if we were to get a PHY that
> provides more than two CSI ports: in that case, we'd avoid platform data
> machinery to check the number of actual ports in the IP, as we would be
> just checking how many register regions we were given from the devicetree,
> meaning that if we got "csi-a", "csi-b", "csi-c", "csi-d", we have four
> ports.
>
> Besides, another thing to think about is... yes you cannot test nor implement
> 2D1C mode in your submission, but this doesn't mean that others won't ever be
> interested in this and that other people won't be actually implementing that;
> Providing them with the right initial driver structure will surely make things
> easier, encouraging other people from the community to spend their precious
> time on the topic.
>
Hi Angelo,
Ok, I see your point, but for future potential upgrade to support A/B
ports I was thinking of something else: adding independent nodes for csixA
and csixB such as:

csi0_rx: phy@11c10000 {
  reg = <0 0x11C10000 0 0x2000>;
  mediatek,mode = <4D1c>;
  ...
};

csi0a_rx: phy@11c10000 {
  reg = <0 0x11C10000 0 0x1000>;
  mediatek,mode = <2D1c>;
  ...
};
csi0b_rx: phy@11c11000 {
  reg = <0 0x11C11000 0 0x1000>;
  mediatek,mode = <2D1c>;
  ...
};

giving the correct register range. One thing I did not mention is that if
csi0_rx is used csi0a_rx and csi0b_rx cannot be used (they share same
physical lanes as csio_rx), but csi0a_rx and csi0b_rx can be used simultaneously.
So platform device will enable only the node(s) it needs and enabling
csi0_rx and csioa/b_rx will fail because they share the same register
region and map will fail and it does not have any sense because you
either have a camera using the whole port or sub port but you cannot have
both plugged in. What do you think about it?

> > > > +#define CSIxB_OFFSET		0x1000
Maybe moving this declaration in phy-mtk-mipi-csi-0-5-rx-reg.h would be
better?

Regards,
Julien
> Regards,
> Angelo
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[AngeloGioacchino Del Regno]

Il 16/05/23 11:30, Julien Stephan ha scritto:
> On Mon, May 15, 2023 at 04:32:42PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 15/05/23 16:07, Julien Stephan ha scritto:
>>> On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
>>>>> +#define CSIxB_OFFSET		0x1000
>>>>
>>>> What if we grab two (or three?) iospaces from devicetree?
>>>>
>>>> - base (global)
>>>> - csi_a
>>>> - csi_b
>>>>
>>>> That would make it possible to maybe eventually extend this driver to more
>>>> versions (older or newer) of the CSI PHY IP without putting fixes offsets
>>>> inside of platform data structures and such.
>>>>
>>> Hi Angelo,
>>> The register bank of the CSI port is divided into 2:
>>> * from base address to base + 0x1000 (port A)
>>> * from base + 0x1000 to base +0x2000 (port B)
>>> Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
>>> the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
>>> 1 clock) and use either port A or port B.
>>>
>>> For example  mt8365 has CSI0 that can be used either in 4D1C mode or in
>>> 2 * 2D1C and CSI1 which can use only 4D1C mode
>>>
>>> 2D1C mode can not be tested and is not implemented in the driver so
>>> I guess adding csi_a and csi_b reg value may be confusing?
>>>
>>> What do you think?
>>
>> Ok so we're talking about two data lanes per CSI port... it may still be
>> beneficial to split the two register regions as
>>
>> reg-names = "csi-a", "csi-b"; (whoops, I actually used underscores before,
>>                                 and that was a mistake, sorry!)
>>
>> ....but that would be actually good only if we are expecting to get a CSI
>> PHY in the future with four data lanes per port.
>>
>> If you do *not* expect at all such a CSI PHY, or you do *not* expect such
>> a PHY to ever be compatible with this driver (read as: if you expect such
>> a PHY to be literally completely different from this one), then it would
>> not change much to have the registers split in two.
>>
>> Another case in which it would make sense is if we were to get a PHY that
>> provides more than two CSI ports: in that case, we'd avoid platform data
>> machinery to check the number of actual ports in the IP, as we would be
>> just checking how many register regions we were given from the devicetree,
>> meaning that if we got "csi-a", "csi-b", "csi-c", "csi-d", we have four
>> ports.
>>
>> Besides, another thing to think about is... yes you cannot test nor implement
>> 2D1C mode in your submission, but this doesn't mean that others won't ever be
>> interested in this and that other people won't be actually implementing that;
>> Providing them with the right initial driver structure will surely make things
>> easier, encouraging other people from the community to spend their precious
>> time on the topic.
>>
> Hi Angelo,
> Ok, I see your point, but for future potential upgrade to support A/B
> ports I was thinking of something else: adding independent nodes for csixA
> and csixB such as:
> 
> csi0_rx: phy@11c10000 {
>    reg = <0 0x11C10000 0 0x2000>;
>    mediatek,mode = <4D1c>;
>    ...
> };
> 
> csi0a_rx: phy@11c10000 {
>    reg = <0 0x11C10000 0 0x1000>;
>    mediatek,mode = <2D1c>;
>    ...
> };
> csi0b_rx: phy@11c11000 {
>    reg = <0 0x11C11000 0 0x1000>;
>    mediatek,mode = <2D1c>;
>    ...
> };
> 
> giving the correct register range. One thing I did not mention is that if
> csi0_rx is used csi0a_rx and csi0b_rx cannot be used (they share same
> physical lanes as csio_rx), but csi0a_rx and csi0b_rx can be used simultaneously.
> So platform device will enable only the node(s) it needs and enabling
> csi0_rx and csioa/b_rx will fail because they share the same register
> region and map will fail and it does not have any sense because you
> either have a camera using the whole port or sub port but you cannot have
> both plugged in. What do you think about it?
> 

Your description of the hardware makes me even more confident in pushing for
having one single node with multiple iospaces.

You could have a node such as:

csi0_rx: phy@11c10000 {
	compatible = ....
	reg = <0 0x11c10000 0 0x1000>, <0 0x11c20000 0 0x1000>;
	reg-names = "csi-a", "csi-b";
	/* 4 means 4D1C */
	num-lanes = <4>;
		or
	/* 2 means 2D1C */
	num-lanes = <2>;
};

You would then reference the csi0_rx node as:

	/* PHY is configured as 4 lanes (4D1C) */
	something = <&csi0_rx 0>;

	or

	/* First two lanes (CSI0 PORT-A) */
	something = <&csi0_rx 0>;
	/* Second two lanes (CSI0 PORT-B) */
	something = <&csi0_rx 1>;

Preferrably, you should (or shall?) use a graph to describe such connections,
anyway.

This is because overriding the number of lanes on a per-board basis becomes
*otherwise* difficult, in the sense of human readability issues, other than
duplicated nodes being a real issue.

Regards,
Angelo


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy