[v4,1/3] dt-bindings: phy: Add starfive,jh7110-dphy-rx

  StarFive SoCs like the jh7110 use a MIPI D-PHY RX controller based on
a M31 IP. Add a binding for it.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 .../bindings/phy/starfive,jh7110-dphy-rx.yaml | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
  

On 12/04/2023 10:45, Changhuang Liang wrote:
> StarFive SoCs like the jh7110 use a MIPI D-PHY RX controller based on
> a M31 IP. Add a binding for it.

So this is D-PHY? Or the other patch is D-PHY? The naming is quite
confusing and your commit msgs are not helping here.

Also the power domain phandle here adds to the confusion.


> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  .../bindings/phy/starfive,jh7110-dphy-rx.yaml | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml b/Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
> new file mode 100644
> index 000000000000..5fb2f14af816
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/starfive,jh7110-dphy-rx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive SoC MIPI D-PHY Rx Controller
> +
> +maintainers:
> +  - Jack Zhu <jack.zhu@starfivetech.com>
> +  - Changhuang Liang <changhuang.liang@starfivetech.com>
> +
> +description:
> +  The StarFive SoC uses the MIPI CSI D-PHY based on M31 IP to transfer
> +  CSI camera data.
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-dphy-rx
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: config clock
> +      - description: reference clock
> +      - description: escape mode transmit clock
> +
> +  clock-names:
> +    items:
> +      - const: cfg
> +      - const: ref
> +      - const: tx
> +
> +  resets:
> +    items:
> +      - description: DPHY_HW reset
> +      - description: DPHY_B09_ALWAYS_ON reset
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  lane_maps:

Why did this appear? Underscores are not allowed. It looks like you
re-implement some standard property.

> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description:
> +      D-PHY rx controller physical lanes and logic lanes mapping table.
> +    items:
> +      - description: logic lane index point to physical lane clock lane 0
> +      - description: logic lane index point to physical lane data lane 0
> +      - description: logic lane index point to physical lane data lane 1
> +      - description: logic lane index point to physical lane data lane 2
> +      - description: logic lane index point to physical lane data lane 3
> +      - description: logic lane index point to physical lane clock lane 1
> +


Best regards,
Krzysztof
  

On 2023/4/12 19:34, Krzysztof Kozlowski wrote:
> On 12/04/2023 10:45, Changhuang Liang wrote:
>> StarFive SoCs like the jh7110 use a MIPI D-PHY RX controller based on
>> a M31 IP. Add a binding for it.
> 
> So this is D-PHY? Or the other patch is D-PHY? The naming is quite
> confusing and your commit msgs are not helping here.
> 
> Also the power domain phandle here adds to the confusion.
> 

Yes, this is DPHY, DPHY has rx and tx, and last version we are discussing that 
use power domain replace syscon:
https://lore.kernel.org/all/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/

>>
>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>> ---
>>  .../bindings/phy/starfive,jh7110-dphy-rx.yaml | 85 +++++++++++++++++++
>>  1 file changed, 85 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
>>
[...]
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  lane_maps:
> 
> Why did this appear? Underscores are not allowed. It looks like you
> re-implement some standard property.
> 

Will change to lane-maps.
Yes, according to Vinod advice, lane mapping table use device tree
to parse makes sense.

>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    description:
>> +      D-PHY rx controller physical lanes and logic lanes mapping table.
>> +    items:
>> +      - description: logic lane index point to physical lane clock lane 0
>> +      - description: logic lane index point to physical lane data lane 0
>> +      - description: logic lane index point to physical lane data lane 1
>> +      - description: logic lane index point to physical lane data lane 2
>> +      - description: logic lane index point to physical lane data lane 3
>> +      - description: logic lane index point to physical lane clock lane 1
>> +
> 
> 
> Best regards,
> Krzysztof
>
  

On 12/04/2023 14:42, Changhuang Liang wrote:
> 
> 
> On 2023/4/12 19:34, Krzysztof Kozlowski wrote:
>> On 12/04/2023 10:45, Changhuang Liang wrote:
>>> StarFive SoCs like the jh7110 use a MIPI D-PHY RX controller based on
>>> a M31 IP. Add a binding for it.
>>
>> So this is D-PHY? Or the other patch is D-PHY? The naming is quite
>> confusing and your commit msgs are not helping here.
>>
>> Also the power domain phandle here adds to the confusion.
>>
> 
> Yes, this is DPHY, DPHY has rx and tx, and last version we are discussing that 
> use power domain replace syscon:
> https://lore.kernel.org/all/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/

The other patch - DPHY PMU - is confusing. Instead of writing short
commits, explain more.

> 
>>>
>>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>>> ---
>>>  .../bindings/phy/starfive,jh7110-dphy-rx.yaml | 85 +++++++++++++++++++
>>>  1 file changed, 85 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
>>>
> [...]
>>> +
>>> +  power-domains:
>>> +    maxItems: 1
>>> +
>>> +  lane_maps:
>>
>> Why did this appear? Underscores are not allowed. It looks like you
>> re-implement some standard property.
>>
> 
> Will change to lane-maps.
> Yes, according to Vinod advice, lane mapping table use device tree
> to parse makes sense.

Hm, I have a feeling that I saw such property, so you should dig into
existing and in-flight bindings.

Best regards,
Krzysztof
  

On 2023/4/13 0:55, Krzysztof Kozlowski wrote:
> On 12/04/2023 14:42, Changhuang Liang wrote:
>>
>>
>> On 2023/4/12 19:34, Krzysztof Kozlowski wrote:
>>> On 12/04/2023 10:45, Changhuang Liang wrote:
>>>> StarFive SoCs like the jh7110 use a MIPI D-PHY RX controller based on
>>>> a M31 IP. Add a binding for it.
>>>
>>> So this is D-PHY? Or the other patch is D-PHY? The naming is quite
>>> confusing and your commit msgs are not helping here.
>>>
>>> Also the power domain phandle here adds to the confusion.
>>>
>>
>> Yes, this is DPHY, DPHY has rx and tx, and last version we are discussing that 
>> use power domain replace syscon:
>> https://lore.kernel.org/all/5dc4ddc2-9d15-ebb2-38bc-8a544ca67e0d@starfivetech.com/
> 
> The other patch - DPHY PMU - is confusing. Instead of writing short
> commits, explain more.
> 

OK, I will add more commit message in DPHY PMU dt-binding patch, for example:

dt-bindings: power: Add JH7110 DPHY PMU support.

Add DPHY PMU for StarFive JH7110 SoC, it can be used to turn on/off DPHY rx/tx
power switch, and it don't need the reg and interrupt properties.

I think this commit message will helpful for you to distinguish them.

>>
>>>>
>>>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>>>> ---
>>>>  .../bindings/phy/starfive,jh7110-dphy-rx.yaml | 85 +++++++++++++++++++
>>>>  1 file changed, 85 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
>>>>
>> [...]
>>>> +
>>>> +  power-domains:
>>>> +    maxItems: 1
>>>> +
>>>> +  lane_maps:
>>>
>>> Why did this appear? Underscores are not allowed. It looks like you
>>> re-implement some standard property.
>>>
>>
>> Will change to lane-maps.
>> Yes, according to Vinod advice, lane mapping table use device tree
>> to parse makes sense.
> 
> Hm, I have a feeling that I saw such property, so you should dig into
> existing and in-flight bindings.
> 
> Best regards,
> Krzysztof
>
  

On 2023/4/13 0:55, Krzysztof Kozlowski wrote:
> On 12/04/2023 14:42, Changhuang Liang wrote:
[...]
>>>>
>>>> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>>>> ---
>>>>  .../bindings/phy/starfive,jh7110-dphy-rx.yaml | 85 +++++++++++++++++++
>>>>  1 file changed, 85 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/phy/starfive,jh7110-dphy-rx.yaml
>>>>
>> [...]
>>>> +
>>>> +  power-domains:
>>>> +    maxItems: 1
>>>> +
>>>> +  lane_maps:
>>>
>>> Why did this appear? Underscores are not allowed. It looks like you
>>> re-implement some standard property.
>>>
>>
>> Will change to lane-maps.
>> Yes, according to Vinod advice, lane mapping table use device tree
>> to parse makes sense.
> 
> Hm, I have a feeling that I saw such property, so you should dig into
> existing and in-flight bindings.
> 
> Best regards,
> Krzysztof
> 

A standard property? Like "clocks" or "resets"?
  

On 13/04/2023 04:34, Changhuang Liang wrote:
>>>>> +  lane_maps:
>>>>
>>>> Why did this appear? Underscores are not allowed. It looks like you
>>>> re-implement some standard property.
>>>>
>>>
>>> Will change to lane-maps.
>>> Yes, according to Vinod advice, lane mapping table use device tree
>>> to parse makes sense.
>>
>> Hm, I have a feeling that I saw such property, so you should dig into
>> existing and in-flight bindings.
>>
>> Best regards,
>> Krzysztof
>>
> 
> A standard property? Like "clocks" or "resets"?

Like lane-polarities now submitted to one MIPI.

Anyway it does not look like a property of a board. You said it is fixed
per SoC, so it should be implied from the compatible. Otherwise please
explain in description and provide some rationale.

Best regards,
Krzysztof
  

On 2023/4/13 16:41, Krzysztof Kozlowski wrote:
> On 13/04/2023 04:34, Changhuang Liang wrote:
>>>>>> +  lane_maps:
>>>>>
>>>>> Why did this appear? Underscores are not allowed. It looks like you
>>>>> re-implement some standard property.
>>>>>
>>>>
>>>> Will change to lane-maps.
>>>> Yes, according to Vinod advice, lane mapping table use device tree
>>>> to parse makes sense.
>>>
>>> Hm, I have a feeling that I saw such property, so you should dig into
>>> existing and in-flight bindings.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> A standard property? Like "clocks" or "resets"?
> 
> Like lane-polarities now submitted to one MIPI.
> 
> Anyway it does not look like a property of a board. You said it is fixed
> per SoC, so it should be implied from the compatible. Otherwise please
> explain in description and provide some rationale.
> 
> Best regards,
> Krzysztof
> 

This property is the only one used for this IP, I have compared this IP with
other DPHY rx module, DPHY modules form the other manufacturers not have this
configure.
And we also have a SoC called JH7100. It DPHY rx module is the same as JH7110.
But we don't do the upstream work on it. If it use this lane-maps will be 
configure as "lane_maps = /bits/ 8 <0 1 2 3 4 5>;".
So what do you think? Can this configure be implemented into a property?
  

On 13/04/2023 11:02, Changhuang Liang wrote:
> 
> 
> On 2023/4/13 16:41, Krzysztof Kozlowski wrote:
>> On 13/04/2023 04:34, Changhuang Liang wrote:
>>>>>>> +  lane_maps:
>>>>>>
>>>>>> Why did this appear? Underscores are not allowed. It looks like you
>>>>>> re-implement some standard property.
>>>>>>
>>>>>
>>>>> Will change to lane-maps.
>>>>> Yes, according to Vinod advice, lane mapping table use device tree
>>>>> to parse makes sense.
>>>>
>>>> Hm, I have a feeling that I saw such property, so you should dig into
>>>> existing and in-flight bindings.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> A standard property? Like "clocks" or "resets"?
>>
>> Like lane-polarities now submitted to one MIPI.
>>
>> Anyway it does not look like a property of a board. You said it is fixed
>> per SoC, so it should be implied from the compatible. Otherwise please
>> explain in description and provide some rationale.
>>
>> Best regards,
>> Krzysztof
>>
> 
> This property is the only one used for this IP, I have compared this IP with
> other DPHY rx module, DPHY modules form the other manufacturers not have this
> configure.
> And we also have a SoC called JH7100. It DPHY rx module is the same as JH7110.
> But we don't do the upstream work on it. If it use this lane-maps will be 
> configure as "lane_maps = /bits/ 8 <0 1 2 3 4 5>;".

And JH7100 is different SoC, so you have different compatible. Again -
is this board specific? If not, looks like SoC specific, thus imply it
from compatible.


Best regards,
Krzysztof
  

On 2023/4/17 1:29, Krzysztof Kozlowski wrote:
> On 13/04/2023 11:02, Changhuang Liang wrote:
>>
>>
>> On 2023/4/13 16:41, Krzysztof Kozlowski wrote:
>>> On 13/04/2023 04:34, Changhuang Liang wrote:
>>>>>>>> +  lane_maps:
>>>>>>>
>>>>>>> Why did this appear? Underscores are not allowed. It looks like you
>>>>>>> re-implement some standard property.
>>>>>>>
>>>>>>
>>>>>> Will change to lane-maps.
>>>>>> Yes, according to Vinod advice, lane mapping table use device tree
>>>>>> to parse makes sense.
>>>>>
>>>>> Hm, I have a feeling that I saw such property, so you should dig into
>>>>> existing and in-flight bindings.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> A standard property? Like "clocks" or "resets"?
>>>
>>> Like lane-polarities now submitted to one MIPI.
>>>
>>> Anyway it does not look like a property of a board. You said it is fixed
>>> per SoC, so it should be implied from the compatible. Otherwise please
>>> explain in description and provide some rationale.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> This property is the only one used for this IP, I have compared this IP with
>> other DPHY rx module, DPHY modules form the other manufacturers not have this
>> configure.
>> And we also have a SoC called JH7100. It DPHY rx module is the same as JH7110.
>> But we don't do the upstream work on it. If it use this lane-maps will be 
>> configure as "lane_maps = /bits/ 8 <0 1 2 3 4 5>;".
> 
> And JH7100 is different SoC, so you have different compatible. Again -
> is this board specific? If not, looks like SoC specific, thus imply it
> from compatible.
> 
> 
> Best regards,
> Krzysztof
> 

Hi, Vinod

I agree with Krzysztof. What about your comments?

Best regards,
Changhuang
  

On Thu, Apr 13, 2023 at 10:41:23AM +0200, Krzysztof Kozlowski wrote:
> On 13/04/2023 04:34, Changhuang Liang wrote:
> >>>>> +  lane_maps:
> >>>>
> >>>> Why did this appear? Underscores are not allowed. It looks like you
> >>>> re-implement some standard property.
> >>>>
> >>>
> >>> Will change to lane-maps.
> >>> Yes, according to Vinod advice, lane mapping table use device tree
> >>> to parse makes sense.
> >>
> >> Hm, I have a feeling that I saw such property, so you should dig into
> >> existing and in-flight bindings.
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > 
> > A standard property? Like "clocks" or "resets"?
> 
> Like lane-polarities now submitted to one MIPI.
> 

data-lanes perhaps?

Rob
  

On Tue, Apr 18, 2023 at 1:42 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Apr 13, 2023 at 10:41:23AM +0200, Krzysztof Kozlowski wrote:
> > On 13/04/2023 04:34, Changhuang Liang wrote:
> > >>>>> +  lane_maps:
> > >>>>
> > >>>> Why did this appear? Underscores are not allowed. It looks like you
> > >>>> re-implement some standard property.
> > >>>>
> > >>>
> > >>> Will change to lane-maps.
> > >>> Yes, according to Vinod advice, lane mapping table use device tree
> > >>> to parse makes sense.
> > >>
> > >> Hm, I have a feeling that I saw such property, so you should dig into
> > >> existing and in-flight bindings.
> > >>
> > >> Best regards,
> > >> Krzysztof
> > >>
> > >
> > > A standard property? Like "clocks" or "resets"?
> >
> > Like lane-polarities now submitted to one MIPI.
> >
>
> data-lanes perhaps?

Except that is for the controller's endpoint rather than the phy.
Presumably if the controller knows the mapping, then it can tell the
phy if it needs the information. IOW, don't just copy 'data-lanes' to
the phy. Follow the normal patterns.

Rob
  

On 2023/4/19 2:46, Rob Herring wrote:
> On Tue, Apr 18, 2023 at 1:42 PM Rob Herring <robh@kernel.org> wrote:
>>
>> On Thu, Apr 13, 2023 at 10:41:23AM +0200, Krzysztof Kozlowski wrote:
>>> On 13/04/2023 04:34, Changhuang Liang wrote:
>>>>>>>> +  lane_maps:
>>>>>>>
>>>>>>> Why did this appear? Underscores are not allowed. It looks like you
>>>>>>> re-implement some standard property.
>>>>>>>
>>>>>>
>>>>>> Will change to lane-maps.
>>>>>> Yes, according to Vinod advice, lane mapping table use device tree
>>>>>> to parse makes sense.
>>>>>
>>>>> Hm, I have a feeling that I saw such property, so you should dig into
>>>>> existing and in-flight bindings.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> A standard property? Like "clocks" or "resets"?
>>>
>>> Like lane-polarities now submitted to one MIPI.
>>>
>>
>> data-lanes perhaps?
> 
> Except that is for the controller's endpoint rather than the phy.
> Presumably if the controller knows the mapping, then it can tell the
> phy if it needs the information. IOW, don't just copy 'data-lanes' to
> the phy. Follow the normal patterns.
> 
> Rob

I am not sure if phy can fetch from the other controller's endpoint. In 
addition, like our JH7110 SoC, it have data-lanes configure (data-lanes = <1 2>)
in csi2rx controller, but this data-lanes configure is not appropriate to phy, 
maybe they are independent.
  

On 2023/4/17 1:29, Krzysztof Kozlowski wrote:
>>>> A standard property? Like "clocks" or "resets"?
>>>
>>> Like lane-polarities now submitted to one MIPI.
>>>
>>> Anyway it does not look like a property of a board. You said it is fixed
>>> per SoC, so it should be implied from the compatible. Otherwise please
>>> explain in description and provide some rationale.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> This property is the only one used for this IP, I have compared this IP with
>> other DPHY rx module, DPHY modules form the other manufacturers not have this
>> configure.
>> And we also have a SoC called JH7100. It DPHY rx module is the same as JH7110.
>> But we don't do the upstream work on it. If it use this lane-maps will be 
>> configure as "lane_maps = /bits/ 8 <0 1 2 3 4 5>;".
> 
> And JH7100 is different SoC, so you have different compatible. Again -
> is this board specific? If not, looks like SoC specific, thus imply it
> from compatible.
> 
> 
> Best regards,
> Krzysztof
> 

Vinod,

Hi, could you give me some suggestions about Krzysztof's comment? Thanks for
your time.

Best regards,
Changhuang