| Message ID | 20220819091344.2274-2-chunfeng.yun@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/7] dt-bindings: phy: mediatek,tphy: add support type of SGMII | expand |
On 19/08/2022 12:13, Chunfeng Yun wrote: > Add a property to set usb2 phy's pre-emphasis. > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > index 848edfb1f677..aee2f3027371 100644 > --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > @@ -219,6 +219,13 @@ patternProperties: > minimum: 1 > maximum: 15 > > + mediatek,pre-emphasis: > + description: > + The selection of pre-emphasis (U2 phy) > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 1 > + maximum: 3 Instead of hard-coding register values in bindings, you should rather describe here feature/effect. If it is in units, use unit suffixes. If it is some choice, usually string enum is appropriate. Best regards, Krzysztof
On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski wrote: > On 19/08/2022 12:13, Chunfeng Yun wrote: > > Add a property to set usb2 phy's pre-emphasis. > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 7 > > +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > index 848edfb1f677..aee2f3027371 100644 > > --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > @@ -219,6 +219,13 @@ patternProperties: > > minimum: 1 > > maximum: 15 > > > > + mediatek,pre-emphasis: > > + description: > > + The selection of pre-emphasis (U2 phy) > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 1 > > + maximum: 3 > > Instead of hard-coding register values in bindings, you should rather > describe here feature/effect. If it is in units, use unit suffixes. > If > it is some choice, usually string enum is appropriate. How about changing description as bellow: "The level of pre-emphasis, increases one level, boosts the relative amplitudes of signal's higher frequencies components about 4.16% (U2 phy)" Thanks > > Best regards, > Krzysztof
On 22/08/2022 10:07, Chunfeng Yun wrote: > On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski wrote: >> On 19/08/2022 12:13, Chunfeng Yun wrote: >>> Add a property to set usb2 phy's pre-emphasis. >>> >>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> >>> --- >>> Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 7 >>> +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>> b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>> index 848edfb1f677..aee2f3027371 100644 >>> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>> @@ -219,6 +219,13 @@ patternProperties: >>> minimum: 1 >>> maximum: 15 >>> >>> + mediatek,pre-emphasis: >>> + description: >>> + The selection of pre-emphasis (U2 phy) >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + minimum: 1 >>> + maximum: 3 >> >> Instead of hard-coding register values in bindings, you should rather >> describe here feature/effect. If it is in units, use unit suffixes. >> If >> it is some choice, usually string enum is appropriate. > How about changing description as bellow: > > "The level of pre-emphasis, increases one level, boosts the relative > amplitudes of signal's higher frequencies components about 4.16% (U2 > phy)" > Still the question is what is the unit. 4.16%? Best regards, Krzysztof
On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski wrote: > On 22/08/2022 10:07, Chunfeng Yun wrote: > > On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski wrote: > > > On 19/08/2022 12:13, Chunfeng Yun wrote: > > > > Add a property to set usb2 phy's pre-emphasis. > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > > > --- > > > > Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 7 > > > > +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > index 848edfb1f677..aee2f3027371 100644 > > > > --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > @@ -219,6 +219,13 @@ patternProperties: > > > > minimum: 1 > > > > maximum: 15 > > > > > > > > + mediatek,pre-emphasis: > > > > + description: > > > > + The selection of pre-emphasis (U2 phy) > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + minimum: 1 > > > > + maximum: 3 > > > > > > Instead of hard-coding register values in bindings, you should > > > rather > > > describe here feature/effect. If it is in units, use unit > > > suffixes. > > > If > > > it is some choice, usually string enum is appropriate. > > > > How about changing description as bellow: > > > > "The level of pre-emphasis, increases one level, boosts the > > relative > > amplitudes of signal's higher frequencies components about 4.16% > > (U2 > > phy)" > > > > Still the question is what is the unit. 4.16%? No unit, it's a level value, like an index of array. > > Best regards, > Krzysztof
On 26/08/2022 08:36, Chunfeng Yun wrote: > On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski wrote: >> On 22/08/2022 10:07, Chunfeng Yun wrote: >>> On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski wrote: >>>> On 19/08/2022 12:13, Chunfeng Yun wrote: >>>>> Add a property to set usb2 phy's pre-emphasis. >>>>> >>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> >>>>> --- >>>>> Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 7 >>>>> +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>> b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>> index 848edfb1f677..aee2f3027371 100644 >>>>> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>> @@ -219,6 +219,13 @@ patternProperties: >>>>> minimum: 1 >>>>> maximum: 15 >>>>> >>>>> + mediatek,pre-emphasis: >>>>> + description: >>>>> + The selection of pre-emphasis (U2 phy) >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + minimum: 1 >>>>> + maximum: 3 >>>> >>>> Instead of hard-coding register values in bindings, you should >>>> rather >>>> describe here feature/effect. If it is in units, use unit >>>> suffixes. >>>> If >>>> it is some choice, usually string enum is appropriate. >>> >>> How about changing description as bellow: >>> >>> "The level of pre-emphasis, increases one level, boosts the >>> relative >>> amplitudes of signal's higher frequencies components about 4.16% >>> (U2 >>> phy)" >>> >> >> Still the question is what is the unit. 4.16%? > No unit, it's a level value, like an index of array. > So a value from register/device programming? Rather a regular units should be used if that's possible. If not, this should be clearly described here, not some magical number which you encode into DTS... Best regards, Krzysztof
On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski wrote: > On 26/08/2022 08:36, Chunfeng Yun wrote: > > On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski wrote: > > > On 22/08/2022 10:07, Chunfeng Yun wrote: > > > > On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski wrote: > > > > > On 19/08/2022 12:13, Chunfeng Yun wrote: > > > > > > Add a property to set usb2 phy's pre-emphasis. > > > > > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > > > > > --- > > > > > > Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | > > > > > > 7 > > > > > > +++++++ > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > diff --git > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > > > index 848edfb1f677..aee2f3027371 100644 > > > > > > --- > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > > > +++ > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > > > > > > @@ -219,6 +219,13 @@ patternProperties: > > > > > > minimum: 1 > > > > > > maximum: 15 > > > > > > > > > > > > + mediatek,pre-emphasis: > > > > > > + description: > > > > > > + The selection of pre-emphasis (U2 phy) > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > + minimum: 1 > > > > > > + maximum: 3 > > > > > > > > > > Instead of hard-coding register values in bindings, you > > > > > should > > > > > rather > > > > > describe here feature/effect. If it is in units, use unit > > > > > suffixes. > > > > > If > > > > > it is some choice, usually string enum is appropriate. > > > > > > > > How about changing description as bellow: > > > > > > > > "The level of pre-emphasis, increases one level, boosts the > > > > relative > > > > amplitudes of signal's higher frequencies components about > > > > 4.16% > > > > (U2 > > > > phy)" > > > > > > > > > > Still the question is what is the unit. 4.16%? > > > > No unit, it's a level value, like an index of array. > > > > So a value from register/device programming? Yes > Rather a regular units > should be used if that's possible. If not, this should be clearly > described here, not some magical number which you encode into DTS... Ok, I'll add more descriptions. Thanks a lot > > Best regards, > Krzysztof
On 29/08/2022 05:37, Chunfeng Yun wrote: > On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski wrote: >> On 26/08/2022 08:36, Chunfeng Yun wrote: >>> On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski wrote: >>>> On 22/08/2022 10:07, Chunfeng Yun wrote: >>>>> On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski wrote: >>>>>> On 19/08/2022 12:13, Chunfeng Yun wrote: >>>>>>> Add a property to set usb2 phy's pre-emphasis. >>>>>>> >>>>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> >>>>>>> --- >>>>>>> Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | >>>>>>> 7 >>>>>>> +++++++ >>>>>>> 1 file changed, 7 insertions(+) >>>>>>> >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>>>> b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>>>> index 848edfb1f677..aee2f3027371 100644 >>>>>>> --- >>>>>>> a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>>>> +++ >>>>>>> b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml >>>>>>> @@ -219,6 +219,13 @@ patternProperties: >>>>>>> minimum: 1 >>>>>>> maximum: 15 >>>>>>> >>>>>>> + mediatek,pre-emphasis: >>>>>>> + description: >>>>>>> + The selection of pre-emphasis (U2 phy) >>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>> + minimum: 1 >>>>>>> + maximum: 3 >>>>>> >>>>>> Instead of hard-coding register values in bindings, you >>>>>> should >>>>>> rather >>>>>> describe here feature/effect. If it is in units, use unit >>>>>> suffixes. >>>>>> If >>>>>> it is some choice, usually string enum is appropriate. >>>>> >>>>> How about changing description as bellow: >>>>> >>>>> "The level of pre-emphasis, increases one level, boosts the >>>>> relative >>>>> amplitudes of signal's higher frequencies components about >>>>> 4.16% >>>>> (U2 >>>>> phy)" >>>>> >>>> >>>> Still the question is what is the unit. 4.16%? >>> >>> No unit, it's a level value, like an index of array. >>> >> >> So a value from register/device programming? > Yes >> Rather a regular units >> should be used if that's possible. If not, this should be clearly >> described here, not some magical number which you encode into DTS... > Ok, I'll add more descriptions. Better use logical value, e.g. https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml#L38 Best regards, Krzysztof
On Tue, 2022-08-30 at 13:08 +0300, Krzysztof Kozlowski wrote: > On 29/08/2022 05:37, Chunfeng Yun wrote: > > On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski wrote: > > > On 26/08/2022 08:36, Chunfeng Yun wrote: > > > > On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski wrote: > > > > > On 22/08/2022 10:07, Chunfeng Yun wrote: > > > > > > On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski > > > > > > wrote: > > > > > > > On 19/08/2022 12:13, Chunfeng Yun wrote: > > > > > > > > Add a property to set usb2 phy's pre-emphasis. > > > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > > > > > > > --- > > > > > > > > Documentation/devicetree/bindings/phy/mediatek,tphy.ya > > > > > > > > ml | > > > > > > > > 7 > > > > > > > > +++++++ > > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > > > diff --git > > > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.y > > > > > > > > aml > > > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.y > > > > > > > > aml > > > > > > > > index 848edfb1f677..aee2f3027371 100644 > > > > > > > > --- > > > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tphy.y > > > > > > > > aml > > > > > > > > +++ > > > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tphy.y > > > > > > > > aml > > > > > > > > @@ -219,6 +219,13 @@ patternProperties: > > > > > > > > minimum: 1 > > > > > > > > maximum: 15 > > > > > > > > > > > > > > > > + mediatek,pre-emphasis: > > > > > > > > + description: > > > > > > > > + The selection of pre-emphasis (U2 phy) > > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > > > + minimum: 1 > > > > > > > > + maximum: 3 > > > > > > > > > > > > > > Instead of hard-coding register values in bindings, you > > > > > > > should > > > > > > > rather > > > > > > > describe here feature/effect. If it is in units, use unit > > > > > > > suffixes. > > > > > > > If > > > > > > > it is some choice, usually string enum is appropriate. > > > > > > > > > > > > How about changing description as bellow: > > > > > > > > > > > > "The level of pre-emphasis, increases one level, boosts the > > > > > > relative > > > > > > amplitudes of signal's higher frequencies components about > > > > > > 4.16% > > > > > > (U2 > > > > > > phy)" > > > > > > > > > > > > > > > > Still the question is what is the unit. 4.16%? > > > > > > > > No unit, it's a level value, like an index of array. > > > > > > > > > > So a value from register/device programming? > > > > Yes > > > Rather a regular units > > > should be used if that's possible. If not, this should be clearly > > > described here, not some magical number which you encode into > > > DTS... > > > > Ok, I'll add more descriptions. > > Better use logical value, e.g. > https://urldefense.com/v3/__https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml*L38__;Iw!!CTRNKA9wMg0ARbw!1e-h0R_uwcaHKfKC9qYfaRWYeuWRq1sLCGy3yupNmkFyuW5s1nmRotL7Y0vFG9ETLLTA$ > Optional unit may be -percent or -bp, but the value 4.16% * X (X=1,2,3...)is not an exact value, they are variable in a range and dependent more factors. So I think use level value is simple enough. > > Best regards, > Krzysztof
On 31/08/2022 06:00, Chunfeng Yun wrote: > On Tue, 2022-08-30 at 13:08 +0300, Krzysztof Kozlowski wrote: >> On 29/08/2022 05:37, Chunfeng Yun wrote: >>> On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski wrote: >>>> On 26/08/2022 08:36, Chunfeng Yun wrote: >>>>> On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski wrote: >>>>>> On 22/08/2022 10:07, Chunfeng Yun wrote: >>>>>>> On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski >>>>>>> wrote: >>>>>>>> On 19/08/2022 12:13, Chunfeng Yun wrote: >>>>>>>>> Add a property to set usb2 phy's pre-emphasis. >>>>>>>>> >>>>>>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> >>>>>>>>> --- >>>>>>>>> Documentation/devicetree/bindings/phy/mediatek,tphy.ya >>>>>>>>> ml | >>>>>>>>> 7 >>>>>>>>> +++++++ >>>>>>>>> 1 file changed, 7 insertions(+) >>>>>>>>> >>>>>>>>> diff --git >>>>>>>>> a/Documentation/devicetree/bindings/phy/mediatek,tphy.y >>>>>>>>> aml >>>>>>>>> b/Documentation/devicetree/bindings/phy/mediatek,tphy.y >>>>>>>>> aml >>>>>>>>> index 848edfb1f677..aee2f3027371 100644 >>>>>>>>> --- >>>>>>>>> a/Documentation/devicetree/bindings/phy/mediatek,tphy.y >>>>>>>>> aml >>>>>>>>> +++ >>>>>>>>> b/Documentation/devicetree/bindings/phy/mediatek,tphy.y >>>>>>>>> aml >>>>>>>>> @@ -219,6 +219,13 @@ patternProperties: >>>>>>>>> minimum: 1 >>>>>>>>> maximum: 15 >>>>>>>>> >>>>>>>>> + mediatek,pre-emphasis: >>>>>>>>> + description: >>>>>>>>> + The selection of pre-emphasis (U2 phy) >>>>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>>>> + minimum: 1 >>>>>>>>> + maximum: 3 >>>>>>>> >>>>>>>> Instead of hard-coding register values in bindings, you >>>>>>>> should >>>>>>>> rather >>>>>>>> describe here feature/effect. If it is in units, use unit >>>>>>>> suffixes. >>>>>>>> If >>>>>>>> it is some choice, usually string enum is appropriate. >>>>>>> >>>>>>> How about changing description as bellow: >>>>>>> >>>>>>> "The level of pre-emphasis, increases one level, boosts the >>>>>>> relative >>>>>>> amplitudes of signal's higher frequencies components about >>>>>>> 4.16% >>>>>>> (U2 >>>>>>> phy)" >>>>>>> >>>>>> >>>>>> Still the question is what is the unit. 4.16%? >>>>> >>>>> No unit, it's a level value, like an index of array. >>>>> >>>> >>>> So a value from register/device programming? >>> >>> Yes >>>> Rather a regular units >>>> should be used if that's possible. If not, this should be clearly >>>> described here, not some magical number which you encode into >>>> DTS... >>> >>> Ok, I'll add more descriptions. >> >> Better use logical value, e.g. >> > https://urldefense.com/v3/__https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml*L38__;Iw!!CTRNKA9wMg0ARbw!1e-h0R_uwcaHKfKC9qYfaRWYeuWRq1sLCGy3yupNmkFyuW5s1nmRotL7Y0vFG9ETLLTA$ >> > Optional unit may be -percent or -bp, but the value 4.16% * X > (X=1,2,3...)is not an exact value, they are variable in a range and > dependent more factors. > So I think use level value is simple enough. Then again explain exactly what are the levels. How you wrote it last time, -bp would do the trick. Best regards, Krzysztof
On Wed, 2022-08-31 at 09:03 +0300, Krzysztof Kozlowski wrote: > On 31/08/2022 06:00, Chunfeng Yun wrote: > > On Tue, 2022-08-30 at 13:08 +0300, Krzysztof Kozlowski wrote: > > > On 29/08/2022 05:37, Chunfeng Yun wrote: > > > > On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski wrote: > > > > > On 26/08/2022 08:36, Chunfeng Yun wrote: > > > > > > On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski > > > > > > wrote: > > > > > > > On 22/08/2022 10:07, Chunfeng Yun wrote: > > > > > > > > On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski > > > > > > > > wrote: > > > > > > > > > On 19/08/2022 12:13, Chunfeng Yun wrote: > > > > > > > > > > Add a property to set usb2 phy's pre-emphasis. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun < > > > > > > > > > > chunfeng.yun@mediatek.com> > > > > > > > > > > --- > > > > > > > > > > Documentation/devicetree/bindings/phy/mediatek,tph > > > > > > > > > > y.ya > > > > > > > > > > ml | > > > > > > > > > > 7 > > > > > > > > > > +++++++ > > > > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > index 848edfb1f677..aee2f3027371 100644 > > > > > > > > > > --- > > > > > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > +++ > > > > > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > @@ -219,6 +219,13 @@ patternProperties: > > > > > > > > > > minimum: 1 > > > > > > > > > > maximum: 15 > > > > > > > > > > > > > > > > > > > > + mediatek,pre-emphasis: > > > > > > > > > > + description: > > > > > > > > > > + The selection of pre-emphasis (U2 phy) > > > > > > > > > > + $ref: > > > > > > > > > > /schemas/types.yaml#/definitions/uint32 > > > > > > > > > > + minimum: 1 > > > > > > > > > > + maximum: 3 > > > > > > > > > > > > > > > > > > Instead of hard-coding register values in bindings, > > > > > > > > > you > > > > > > > > > should > > > > > > > > > rather > > > > > > > > > describe here feature/effect. If it is in units, use > > > > > > > > > unit > > > > > > > > > suffixes. > > > > > > > > > If > > > > > > > > > it is some choice, usually string enum is > > > > > > > > > appropriate. > > > > > > > > > > > > > > > > How about changing description as bellow: > > > > > > > > > > > > > > > > "The level of pre-emphasis, increases one level, boosts > > > > > > > > the > > > > > > > > relative > > > > > > > > amplitudes of signal's higher frequencies components > > > > > > > > about > > > > > > > > 4.16% > > > > > > > > (U2 > > > > > > > > phy)" > > > > > > > > > > > > > > > > > > > > > > Still the question is what is the unit. 4.16%? > > > > > > > > > > > > No unit, it's a level value, like an index of array. > > > > > > > > > > > > > > > > So a value from register/device programming? > > > > > > > > Yes > > > > > Rather a regular units > > > > > should be used if that's possible. If not, this should be > > > > > clearly > > > > > described here, not some magical number which you encode into > > > > > DTS... > > > > > > > > Ok, I'll add more descriptions. > > > > > > Better use logical value, e.g. > > > > > > > https://urldefense.com/v3/__https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml*L38__;Iw!!CTRNKA9wMg0ARbw!1e-h0R_uwcaHKfKC9qYfaRWYeuWRq1sLCGy3yupNmkFyuW5s1nmRotL7Y0vFG9ETLLTA$ > > > > > > > Optional unit may be -percent or -bp, but the value 4.16% * X > > (X=1,2,3...)is not an exact value, they are variable in a range and > > dependent more factors. > > So I think use level value is simple enough. > > Then again explain exactly what are the levels. Ok, I've asked help from our DE to get more information and reply later, thanks a lot > How you wrote it last > time, -bp would do the trick. > > Best regards, > Krzysztof
On Wed, 2022-08-31 at 09:03 +0300, Krzysztof Kozlowski wrote: > On 31/08/2022 06:00, Chunfeng Yun wrote: > > On Tue, 2022-08-30 at 13:08 +0300, Krzysztof Kozlowski wrote: > > > On 29/08/2022 05:37, Chunfeng Yun wrote: > > > > On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski wrote: > > > > > On 26/08/2022 08:36, Chunfeng Yun wrote: > > > > > > On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski > > > > > > wrote: > > > > > > > On 22/08/2022 10:07, Chunfeng Yun wrote: > > > > > > > > On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski > > > > > > > > wrote: > > > > > > > > > On 19/08/2022 12:13, Chunfeng Yun wrote: > > > > > > > > > > Add a property to set usb2 phy's pre-emphasis. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun < > > > > > > > > > > chunfeng.yun@mediatek.com> > > > > > > > > > > --- > > > > > > > > > > Documentation/devicetree/bindings/phy/mediatek,tph > > > > > > > > > > y.ya > > > > > > > > > > ml | > > > > > > > > > > 7 > > > > > > > > > > +++++++ > > > > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > index 848edfb1f677..aee2f3027371 100644 > > > > > > > > > > --- > > > > > > > > > > a/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > +++ > > > > > > > > > > b/Documentation/devicetree/bindings/phy/mediatek,tp > > > > > > > > > > hy.y > > > > > > > > > > aml > > > > > > > > > > @@ -219,6 +219,13 @@ patternProperties: > > > > > > > > > > minimum: 1 > > > > > > > > > > maximum: 15 > > > > > > > > > > > > > > > > > > > > + mediatek,pre-emphasis: > > > > > > > > > > + description: > > > > > > > > > > + The selection of pre-emphasis (U2 phy) > > > > > > > > > > + $ref: > > > > > > > > > > /schemas/types.yaml#/definitions/uint32 > > > > > > > > > > + minimum: 1 > > > > > > > > > > + maximum: 3 > > > > > > > > > > > > > > > > > > Instead of hard-coding register values in bindings, > > > > > > > > > you > > > > > > > > > should > > > > > > > > > rather > > > > > > > > > describe here feature/effect. If it is in units, use > > > > > > > > > unit > > > > > > > > > suffixes. > > > > > > > > > If > > > > > > > > > it is some choice, usually string enum is > > > > > > > > > appropriate. > > > > > > > > > > > > > > > > How about changing description as bellow: > > > > > > > > > > > > > > > > "The level of pre-emphasis, increases one level, boosts > > > > > > > > the > > > > > > > > relative > > > > > > > > amplitudes of signal's higher frequencies components > > > > > > > > about > > > > > > > > 4.16% > > > > > > > > (U2 > > > > > > > > phy)" > > > > > > > > > > > > > > > > > > > > > > Still the question is what is the unit. 4.16%? > > > > > > > > > > > > No unit, it's a level value, like an index of array. > > > > > > > > > > > > > > > > So a value from register/device programming? > > > > > > > > Yes > > > > > Rather a regular units > > > > > should be used if that's possible. If not, this should be > > > > > clearly > > > > > described here, not some magical number which you encode into > > > > > DTS... > > > > > > > > Ok, I'll add more descriptions. > > > > > > Better use logical value, e.g. > > > > > > > https://urldefense.com/v3/__https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml*L38__;Iw!!CTRNKA9wMg0ARbw!1e-h0R_uwcaHKfKC9qYfaRWYeuWRq1sLCGy3yupNmkFyuW5s1nmRotL7Y0vFG9ETLLTA$ > > > > > > > Optional unit may be -percent or -bp, but the value 4.16% * X > > (X=1,2,3...)is not an exact value, they are variable in a range and > > dependent more factors. > > So I think use level value is simple enough. > > Then again explain exactly what are the levels. How you wrote it last > time, -bp would do the trick. There are many different methods of measuring pre-emphasis. The way used in MediaTek USB2 PHY as below: pre-emphasis level equation = Vpp/Vs -1; Vpp: peak-peak voltage of differential signal; Vs : static voltage of differential signal, normal voltage, e.g. 400mV for u2 phy; The pre-emphasis circuitry within t-phy can be dynamically programmed to three different levels of pre-emphasis. The exact value of pre-emphasis cannot be predetermined, because each device requires a percentage of pre-emphasis that is dependent on the output signal strength and transmission path characteristics. Below shows three programmable pre-emphasis levels for a differential drive signal of 400 mV. The amount of pre-emphasis changes according to the transmission path parameters. programmable level typical pre-emphasis level 1 4.16% 2 8.30% 3 12.40% The reasons that why prefer to use programmable level in dt-binding as following: 1. as you said, -bp may do the trick, but the main problem is that pre-emphasis level is variable on different SoC, and is also variable even on different pcb for the same SoC. e.g. for the programmable level 1, pre-emphasis level may be 6% on a platform, but for the programmable level 2, pre-emphasis level may be also 6% on another platform; I think use pre-emphasis level in property, e.g. 4.16%, the deviation may be bigger than 40%, may cause confusion for users, due to we can't promise that the actual measurement is about 4.16%, it may be 2%, or 5% when measured; 2. the programmable / logical level is flexible when we support more and pre-emphasis level, ans it is easy for us to tune the level due to it's continuous value. 3. all other vendor properties that can't provide exact measurable value in this dt-binding make use of logic level, I want to keep them align; Thanks a lot > > Best regards, > Krzysztof
On 09/09/2022 05:03, Chunfeng Yun wrote: > On Wed, 2022-08-31 at 09:03 +0300, Krzysztof Kozlowski wrote: >> On 31/08/2022 06:00, Chunfeng Yun wrote: >>> On Tue, 2022-08-30 at 13:08 +0300, Krzysztof Kozlowski wrote: >>>> On 29/08/2022 05:37, Chunfeng Yun wrote: >>>>> On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski wrote: >>>>>> On 26/08/2022 08:36, Chunfeng Yun wrote: >>>>>>> On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski >>>>>>> wrote: >>>>>>>> On 22/08/2022 10:07, Chunfeng Yun wrote: >>>>>>>>> On Fri, 2022-08-19 at 15:15 +0300, Krzysztof Kozlowski >>>>>>>>> wrote: >>>>>>>>>> On 19/08/2022 12:13, Chunfeng Yun wrote: >>>>>>>>>>> Add a property to set usb2 phy's pre-emphasis. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Chunfeng Yun < >>>>>>>>>>> chunfeng.yun@mediatek.com> >>>>>>>>>>> --- >>>>>>>>>>> Documentation/devicetree/bindings/phy/mediatek,tph >>>>>>>>>>> y.ya >>>>>>>>>>> ml | >>>>>>>>>>> 7 >>>>>>>>>>> +++++++ >>>>>>>>>>> 1 file changed, 7 insertions(+) >>>>>>>>>>> >>>>>>>>>>> diff --git >>>>>>>>>>> a/Documentation/devicetree/bindings/phy/mediatek,tp >>>>>>>>>>> hy.y >>>>>>>>>>> aml >>>>>>>>>>> b/Documentation/devicetree/bindings/phy/mediatek,tp >>>>>>>>>>> hy.y >>>>>>>>>>> aml >>>>>>>>>>> index 848edfb1f677..aee2f3027371 100644 >>>>>>>>>>> --- >>>>>>>>>>> a/Documentation/devicetree/bindings/phy/mediatek,tp >>>>>>>>>>> hy.y >>>>>>>>>>> aml >>>>>>>>>>> +++ >>>>>>>>>>> b/Documentation/devicetree/bindings/phy/mediatek,tp >>>>>>>>>>> hy.y >>>>>>>>>>> aml >>>>>>>>>>> @@ -219,6 +219,13 @@ patternProperties: >>>>>>>>>>> minimum: 1 >>>>>>>>>>> maximum: 15 >>>>>>>>>>> >>>>>>>>>>> + mediatek,pre-emphasis: >>>>>>>>>>> + description: >>>>>>>>>>> + The selection of pre-emphasis (U2 phy) >>>>>>>>>>> + $ref: >>>>>>>>>>> /schemas/types.yaml#/definitions/uint32 >>>>>>>>>>> + minimum: 1 >>>>>>>>>>> + maximum: 3 >>>>>>>>>> >>>>>>>>>> Instead of hard-coding register values in bindings, >>>>>>>>>> you >>>>>>>>>> should >>>>>>>>>> rather >>>>>>>>>> describe here feature/effect. If it is in units, use >>>>>>>>>> unit >>>>>>>>>> suffixes. >>>>>>>>>> If >>>>>>>>>> it is some choice, usually string enum is >>>>>>>>>> appropriate. >>>>>>>>> >>>>>>>>> How about changing description as bellow: >>>>>>>>> >>>>>>>>> "The level of pre-emphasis, increases one level, boosts >>>>>>>>> the >>>>>>>>> relative >>>>>>>>> amplitudes of signal's higher frequencies components >>>>>>>>> about >>>>>>>>> 4.16% >>>>>>>>> (U2 >>>>>>>>> phy)" >>>>>>>>> >>>>>>>> >>>>>>>> Still the question is what is the unit. 4.16%? >>>>>>> >>>>>>> No unit, it's a level value, like an index of array. >>>>>>> >>>>>> >>>>>> So a value from register/device programming? >>>>> >>>>> Yes >>>>>> Rather a regular units >>>>>> should be used if that's possible. If not, this should be >>>>>> clearly >>>>>> described here, not some magical number which you encode into >>>>>> DTS... >>>>> >>>>> Ok, I'll add more descriptions. >>>> >>>> Better use logical value, e.g. >>>> >>> >>> > https://urldefense.com/v3/__https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml*L38__;Iw!!CTRNKA9wMg0ARbw!1e-h0R_uwcaHKfKC9qYfaRWYeuWRq1sLCGy3yupNmkFyuW5s1nmRotL7Y0vFG9ETLLTA$ >>>> >>> >>> Optional unit may be -percent or -bp, but the value 4.16% * X >>> (X=1,2,3...)is not an exact value, they are variable in a range and >>> dependent more factors. >>> So I think use level value is simple enough. >> >> Then again explain exactly what are the levels. How you wrote it last >> time, -bp would do the trick. > > There are many different methods of measuring pre-emphasis. > The way used in MediaTek USB2 PHY as below: > > pre-emphasis level equation = Vpp/Vs -1; > Vpp: peak-peak voltage of differential signal; > Vs : static voltage of differential signal, normal voltage, e.g. 400mV > for u2 phy; > > The pre-emphasis circuitry within t-phy can be dynamically programmed > to three different levels of pre-emphasis. The exact value of > pre-emphasis cannot be predetermined, because each device requires > a percentage of pre-emphasis that is dependent on the output signal > strength and transmission path characteristics. > > Below shows three programmable pre-emphasis levels for a differential > drive signal of 400 mV. The amount of pre-emphasis changes according > to the transmission path parameters. > > programmable level typical pre-emphasis level > 1 4.16% > 2 8.30% > 3 12.40% > > The reasons that why prefer to use programmable level in dt-binding as > following: > 1. as you said, -bp may do the trick, but the main problem is that > pre-emphasis level is variable on different SoC, and is also > variable even on different pcb for the same SoC. e.g. for the > programmable level 1, pre-emphasis level may be 6% on a platform, > but for the programmable level 2, pre-emphasis level may be also > 6% on another platform; > I think use pre-emphasis level in property, e.g. 4.16%, the > deviation may be bigger than 40%, may cause confusion for users, > due to we can't promise that the actual measurement is about 4.16%, > it may be 2%, or 5% when measured; > 2. the programmable / logical level is flexible when we support more > and pre-emphasis level, ans it is easy for us to tune the level > due to it's continuous value. > 3. all other vendor properties that can't provide exact measurable > value in this dt-binding make use of logic level, I want to > keep them align; Hm, that's clarifies a lot. Thanks for explanation. It's ok for me. Best regards, Krzysztof
On Fri, 2022-09-09 at 10:27 +0200, Krzysztof Kozlowski wrote: > On 09/09/2022 05:03, Chunfeng Yun wrote: > > On Wed, 2022-08-31 at 09:03 +0300, Krzysztof Kozlowski wrote: > > > On 31/08/2022 06:00, Chunfeng Yun wrote: > > > > On Tue, 2022-08-30 at 13:08 +0300, Krzysztof Kozlowski wrote: > > > > > On 29/08/2022 05:37, Chunfeng Yun wrote: > > > > > > On Fri, 2022-08-26 at 09:36 +0300, Krzysztof Kozlowski > > > > > > wrote: > > > > > > > On 26/08/2022 08:36, Chunfeng Yun wrote: > > > > > > > > On Tue, 2022-08-23 at 13:24 +0300, Krzysztof Kozlowski > > > > > > > > wrote: > > > > > > > > > On 22/08/2022 10:07, Chunfeng Yun wrote: > > > > > > > > > > On Fri, 2022-08-19 at 15:15 +0300, Krzysztof > > > > > > > > > > Kozlowski > > > > > > > > > > wrote: > > > > > > > > > > > On 19/08/2022 12:13, Chunfeng Yun wrote: > > > > > > > > > > > > Add a property to set usb2 phy's pre-emphasis. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun < > > > > > > > > > > > > chunfeng.yun@mediatek.com> > > > > > > > > > > > > --- > > > > > > > > > > > > Documentation/devicetree/bindings/phy/mediatek > > > > > > > > > > > > ,tph > > > > > > > > > > > > y.ya > > > > > > > > > > > > ml | > > > > > > > > > > > > 7 > > > > > > > > > > > > +++++++ > > > > > > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > > > > > > > > > > > diff --git > > > > > > > > > > > > a/Documentation/devicetree/bindings/phy/mediate > > > > > > > > > > > > k,tp > > > > > > > > > > > > hy.y > > > > > > > > > > > > aml > > > > > > > > > > > > b/Documentation/devicetree/bindings/phy/mediate > > > > > > > > > > > > k,tp > > > > > > > > > > > > hy.y > > > > > > > > > > > > aml > > > > > > > > > > > > index 848edfb1f677..aee2f3027371 100644 > > > > > > > > > > > > --- > > > > > > > > > > > > a/Documentation/devicetree/bindings/phy/mediate > > > > > > > > > > > > k,tp > > > > > > > > > > > > hy.y > > > > > > > > > > > > aml > > > > > > > > > > > > +++ > > > > > > > > > > > > b/Documentation/devicetree/bindings/phy/mediate > > > > > > > > > > > > k,tp > > > > > > > > > > > > hy.y > > > > > > > > > > > > aml > > > > > > > > > > > > @@ -219,6 +219,13 @@ patternProperties: > > > > > > > > > > > > minimum: 1 > > > > > > > > > > > > maximum: 15 > > > > > > > > > > > > > > > > > > > > > > > > + mediatek,pre-emphasis: > > > > > > > > > > > > + description: > > > > > > > > > > > > + The selection of pre-emphasis (U2 > > > > > > > > > > > > phy) > > > > > > > > > > > > + $ref: > > > > > > > > > > > > /schemas/types.yaml#/definitions/uint32 > > > > > > > > > > > > + minimum: 1 > > > > > > > > > > > > + maximum: 3 > > > > > > > > > > > > > > > > > > > > > > Instead of hard-coding register values in > > > > > > > > > > > bindings, > > > > > > > > > > > you > > > > > > > > > > > should > > > > > > > > > > > rather > > > > > > > > > > > describe here feature/effect. If it is in units, > > > > > > > > > > > use > > > > > > > > > > > unit > > > > > > > > > > > suffixes. > > > > > > > > > > > If > > > > > > > > > > > it is some choice, usually string enum is > > > > > > > > > > > appropriate. > > > > > > > > > > > > > > > > > > > > How about changing description as bellow: > > > > > > > > > > > > > > > > > > > > "The level of pre-emphasis, increases one level, > > > > > > > > > > boosts > > > > > > > > > > the > > > > > > > > > > relative > > > > > > > > > > amplitudes of signal's higher frequencies > > > > > > > > > > components > > > > > > > > > > about > > > > > > > > > > 4.16% > > > > > > > > > > (U2 > > > > > > > > > > phy)" > > > > > > > > > > > > > > > > > > > > > > > > > > > > Still the question is what is the unit. 4.16%? > > > > > > > > > > > > > > > > No unit, it's a level value, like an index of array. > > > > > > > > > > > > > > > > > > > > > > So a value from register/device programming? > > > > > > > > > > > > Yes > > > > > > > Rather a regular units > > > > > > > should be used if that's possible. If not, this should be > > > > > > > clearly > > > > > > > described here, not some magical number which you encode > > > > > > > into > > > > > > > DTS... > > > > > > > > > > > > Ok, I'll add more descriptions. > > > > > > > > > > Better use logical value, e.g. > > > > > > > > > > > > > > > > > https://urldefense.com/v3/__https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml*L38__;Iw!!CTRNKA9wMg0ARbw!1e-h0R_uwcaHKfKC9qYfaRWYeuWRq1sLCGy3yupNmkFyuW5s1nmRotL7Y0vFG9ETLLTA$ > > > > > > > > > > > > > Optional unit may be -percent or -bp, but the value 4.16% * X > > > > (X=1,2,3...)is not an exact value, they are variable in a range > > > > and > > > > dependent more factors. > > > > So I think use level value is simple enough. > > > > > > Then again explain exactly what are the levels. How you wrote it > > > last > > > time, -bp would do the trick. > > > > There are many different methods of measuring pre-emphasis. > > The way used in MediaTek USB2 PHY as below: > > > > pre-emphasis level equation = Vpp/Vs -1; > > Vpp: peak-peak voltage of differential signal; > > Vs : static voltage of differential signal, normal voltage, e.g. > > 400mV > > for u2 phy; > > > > The pre-emphasis circuitry within t-phy can be dynamically > > programmed > > to three different levels of pre-emphasis. The exact value of > > pre-emphasis cannot be predetermined, because each device requires > > a percentage of pre-emphasis that is dependent on the output signal > > strength and transmission path characteristics. > > > > Below shows three programmable pre-emphasis levels for a > > differential > > drive signal of 400 mV. The amount of pre-emphasis changes > > according > > to the transmission path parameters. > > > > programmable level typical pre-emphasis level > > 1 4.16% > > 2 8.30% > > 3 12.40% > > > > The reasons that why prefer to use programmable level in dt-binding > > as > > following: > > 1. as you said, -bp may do the trick, but the main problem is that > > pre-emphasis level is variable on different SoC, and is also > > variable even on different pcb for the same SoC. e.g. for the > > programmable level 1, pre-emphasis level may be 6% on a > > platform, > > but for the programmable level 2, pre-emphasis level may be also > > 6% on another platform; > > I think use pre-emphasis level in property, e.g. 4.16%, the > > deviation may be bigger than 40%, may cause confusion for users, > > due to we can't promise that the actual measurement is about > > 4.16%, > > it may be 2%, or 5% when measured; > > 2. the programmable / logical level is flexible when we support > > more > > and pre-emphasis level, ans it is easy for us to tune the level > > due to it's continuous value. > > 3. all other vendor properties that can't provide exact measurable > > value in this dt-binding make use of logic level, I want to > > keep them align; > > Hm, that's clarifies a lot. Thanks for explanation. I couldn't know more about pre-emphasis without your questions, thank you very much. > It's ok for me. > > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml index 848edfb1f677..aee2f3027371 100644 --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml @@ -219,6 +219,13 @@ patternProperties: minimum: 1 maximum: 15 + mediatek,pre-emphasis: + description: + The selection of pre-emphasis (U2 phy) + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + maximum: 3 + mediatek,bc12: description: Specify the flag to enable BC1.2 if support it
Add a property to set usb2 phy's pre-emphasis. Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 7 +++++++ 1 file changed, 7 insertions(+)