[2/2] dt-bindings: pwm: mediatek: Add compatible string for MT7986

  Add new compatible string for MT7986 PWM.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
 1 file changed, 1 insertion(+)
  

On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
> Add new compatible string for MT7986 PWM.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> @@ -8,6 +8,7 @@ Required properties:
>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.

This version of the PWM h/w is not compatible with any of the existing 
chips? If it is, it should have a fallback compatible.

>     - "mediatek,mt8183-pwm": found on mt8183 SoC.
>     - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC.
>     - "mediatek,mt8365-pwm": found on mt8365 SoC.
> -- 
> 2.38.1
> 
>
  

On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
> > Add new compatible string for MT7986 PWM.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > index 554c96b6d0c3e0..6f4e60c9e18b81 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > @@ -8,6 +8,7 @@ Required properties:
> >     - "mediatek,mt7623-pwm": found on mt7623 SoC.
> >     - "mediatek,mt7628-pwm": found on mt7628 SoC.
> >     - "mediatek,mt7629-pwm": found on mt7629 SoC.
> > +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
> 
> This version of the PWM h/w is not compatible with any of the existing 
> chips? If it is, it should have a fallback compatible.

No, it is unique because it comes with just 2 PWM channels.
Otherwise the driver behaves just like for MT8183 (4 channels) or
MT8365 (3 channels) which also got distinct compatible strings.

> 
> >     - "mediatek,mt8183-pwm": found on mt8183 SoC.
> >     - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC.
> >     - "mediatek,mt8365-pwm": found on mt8365 SoC.
> > -- 
> > 2.38.1
> > 
> > 
>
  

On 21/10/2022 18:58, Daniel Golle wrote:
> On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
>> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
>>> Add new compatible string for MT7986 PWM.
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
>>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>> @@ -8,6 +8,7 @@ Required properties:
>>>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
>>>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
>>>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
>>> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
>>
>> This version of the PWM h/w is not compatible with any of the existing 
>> chips? If it is, it should have a fallback compatible.
> 
> No, it is unique because it comes with just 2 PWM channels.
> Otherwise the driver behaves just like for MT8183 (4 channels) or
> MT8365 (3 channels) which also got distinct compatible strings.

Then something would be here compatible. E.g. If you bound MT8183 with
mt7986-pwm compatible, would you get working device with two channels?

If so, they are compatible.

Best regards,
Krzysztof
  

Hi Krzysztof,

On Sat, Oct 22, 2022 at 12:35:25PM -0400, Krzysztof Kozlowski wrote:
> On 21/10/2022 18:58, Daniel Golle wrote:
> > On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
> >> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
> >>> Add new compatible string for MT7986 PWM.
> >>>
> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
> >>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>> @@ -8,6 +8,7 @@ Required properties:
> >>>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
> >>>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
> >>>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
> >>> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
> >>
> >> This version of the PWM h/w is not compatible with any of the existing 
> >> chips? If it is, it should have a fallback compatible.
> > 
> > No, it is unique because it comes with just 2 PWM channels.
> > Otherwise the driver behaves just like for MT8183 (4 channels) or
> > MT8365 (3 channels) which also got distinct compatible strings.
> 
> Then something would be here compatible. E.g. If you bound MT8183 with
> mt7986-pwm compatible, would you get working device with two channels?

Yes, but I'd see another 2 channels which do not work, accessing them
may even cause problems (I haven't tried that) as it means accessing
an undocumented memory range of the SoC which we in general we
shouldn't be messing around with.

Also note that this case is the same as MT8183 vs. MT8365, they got
distinct compatible strings and also for those two the only difference
is the number of channels.

> 
> If so, they are compatible.

By that definition you should remove the additional compatible for
MT8365 or rather, it should have been rejected for the same argument.

I'm talking about
commit fe00faee8060402a3d85aed95775e16838a6dad2
commit 394b517585da9fbb2eea2f2103ff47d37321e976


Cheers


Daniel
  

On 23/10/2022 08:24, Daniel Golle wrote:
> Hi Krzysztof,
> 
> On Sat, Oct 22, 2022 at 12:35:25PM -0400, Krzysztof Kozlowski wrote:
>> On 21/10/2022 18:58, Daniel Golle wrote:
>>> On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
>>>> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
>>>>> Add new compatible string for MT7986 PWM.
>>>>>
>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>> @@ -8,6 +8,7 @@ Required properties:
>>>>>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
>>>>>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
>>>>>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
>>>>> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
>>>>
>>>> This version of the PWM h/w is not compatible with any of the existing 
>>>> chips? If it is, it should have a fallback compatible.
>>>
>>> No, it is unique because it comes with just 2 PWM channels.
>>> Otherwise the driver behaves just like for MT8183 (4 channels) or
>>> MT8365 (3 channels) which also got distinct compatible strings.
>>
>> Then something would be here compatible. E.g. If you bound MT8183 with
>> mt7986-pwm compatible, would you get working device with two channels?
> 
> Yes, but I'd see another 2 channels which do not work, accessing them
> may even cause problems (I haven't tried that) as it means accessing
> an undocumented memory range of the SoC which we in general we
> shouldn't be messing around with.

Why on MT8183 there would be undocumented memory? Where is undocumented
memory?

> 
> Also note that this case is the same as MT8183 vs. MT8365, they got
> distinct compatible strings and also for those two the only difference
> is the number of channels.

So why they are not made compatible?

> 
>>
>> If so, they are compatible.
> 
> By that definition you should remove the additional compatible for
> MT8365 or rather, it should have been rejected for the same argument.
> 
> I'm talking about
> commit fe00faee8060402a3d85aed95775e16838a6dad2
> commit 394b517585da9fbb2eea2f2103ff47d37321e976

This is a pattern spreading in several Mediatek bindings and we already
commented on new patches. I don't know why people working on Mediatek do
not mark pieces compatible.

Best regards,
Krzysztof
  

On Sun, Oct 23, 2022 at 08:39:34AM -0400, Krzysztof Kozlowski wrote:
> On 23/10/2022 08:24, Daniel Golle wrote:
> > Hi Krzysztof,
> > 
> > On Sat, Oct 22, 2022 at 12:35:25PM -0400, Krzysztof Kozlowski wrote:
> >> On 21/10/2022 18:58, Daniel Golle wrote:
> >>> On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
> >>>> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
> >>>>> Add new compatible string for MT7986 PWM.
> >>>>>
> >>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>>>> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
> >>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>>>> @@ -8,6 +8,7 @@ Required properties:
> >>>>>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
> >>>>>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
> >>>>>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
> >>>>> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
> >>>>
> >>>> This version of the PWM h/w is not compatible with any of the existing 
> >>>> chips? If it is, it should have a fallback compatible.
> >>>
> >>> No, it is unique because it comes with just 2 PWM channels.
> >>> Otherwise the driver behaves just like for MT8183 (4 channels) or
> >>> MT8365 (3 channels) which also got distinct compatible strings.
> >>
> >> Then something would be here compatible. E.g. If you bound MT8183 with
> >> mt7986-pwm compatible, would you get working device with two channels?
> > 
> > Yes, but I'd see another 2 channels which do not work, accessing them
> > may even cause problems (I haven't tried that) as it means accessing
> > an undocumented memory range of the SoC which we in general we
> > shouldn't be messing around with.
> 
> Why on MT8183 there would be undocumented memory? Where is undocumented
> memory?

So we were talking about using the MT8183 compatible for MT7986 SoC, as
the PWM units are identical apart from the number of channels they
offer:

MT7986 got 2 PWM channels. The MMIO registers used for those two
channels start at offsets 0x10 (pwm0) and 0x50 (pwm1)

MT8183 got 4 PWM channels. The MMIO registers used for those four
channels start of offsets 0x10 (pwm0), 0x50 (pwm1), 0x90 (pwm2) and
0xd0 (pwm3).

Hence, when using the MT8183 compatible with MT7986, the driver will
access offsets 0x90 and 0xd0 in case the users enables the (bogus)
outputs pwm2 and pwm3. These offsets, however, are not mentioned in the
datasheet, so it has to be considered that writing things to these
undocumented offsets could cause unknown behavior.

I hope it's more clear now what I mean.

> 
> > 
> > Also note that this case is the same as MT8183 vs. MT8365, they got
> > distinct compatible strings and also for those two the only difference
> > is the number of channels.
> 
> So why they are not made compatible?

My guess is that it's for this very reason:
To correctly communicate the capabilities (in this case: number of
channels) to the driver and not have bogus pwmX show up in the OS
which then causes undocumented MMIO register access in case anyone
tries to actually use them.

> 
> > 
> >>
> >> If so, they are compatible.
> > 
> > By that definition you should remove the additional compatible for
> > MT8365 or rather, it should have been rejected for the same argument.
> > 
> > I'm talking about
> > commit fe00faee8060402a3d85aed95775e16838a6dad2
> > commit 394b517585da9fbb2eea2f2103ff47d37321e976
> 
> This is a pattern spreading in several Mediatek bindings and we already
> commented on new patches. I don't know why people working on Mediatek do
> not mark pieces compatible.

Others will have to answer that for you.

To me this looks a bit like a structural shortcoming of the PWM controller
bindings: if there was a way to tell the driver "hey, this is like MT8183,
but it got only two channels" that would solve it nicely.
This could either be done using child-nodes for each PWM channel or by
simply adding a 'nr-pwms' property.



Cheers


Daniel
  

On 23/10/2022 11:01, Daniel Golle wrote:
> On Sun, Oct 23, 2022 at 08:39:34AM -0400, Krzysztof Kozlowski wrote:
>> On 23/10/2022 08:24, Daniel Golle wrote:
>>> Hi Krzysztof,
>>>
>>> On Sat, Oct 22, 2022 at 12:35:25PM -0400, Krzysztof Kozlowski wrote:
>>>> On 21/10/2022 18:58, Daniel Golle wrote:
>>>>> On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
>>>>>> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
>>>>>>> Add new compatible string for MT7986 PWM.
>>>>>>>
>>>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>>>> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
>>>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>>>> @@ -8,6 +8,7 @@ Required properties:
>>>>>>>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
>>>>>>>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
>>>>>>>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
>>>>>>> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
>>>>>>
>>>>>> This version of the PWM h/w is not compatible with any of the existing 
>>>>>> chips? If it is, it should have a fallback compatible.
>>>>>
>>>>> No, it is unique because it comes with just 2 PWM channels.
>>>>> Otherwise the driver behaves just like for MT8183 (4 channels) or
>>>>> MT8365 (3 channels) which also got distinct compatible strings.
>>>>
>>>> Then something would be here compatible. E.g. If you bound MT8183 with
>>>> mt7986-pwm compatible, would you get working device with two channels?
>>>
>>> Yes, but I'd see another 2 channels which do not work, accessing them
>>> may even cause problems (I haven't tried that) as it means accessing
>>> an undocumented memory range of the SoC which we in general we
>>> shouldn't be messing around with.
>>
>> Why on MT8183 there would be undocumented memory? Where is undocumented
>> memory?
> 
> So we were talking about using the MT8183 compatible for MT7986 SoC, as
> the PWM units are identical apart from the number of channels they
> offer:

No, we talk about MT8183 with mt7986-pwm compatible. Read again my message.

> 
> MT7986 got 2 PWM channels. The MMIO registers used for those two
> channels start at offsets 0x10 (pwm0) and 0x50 (pwm1)
> 
> MT8183 got 4 PWM channels. The MMIO registers used for those four
> channels start of offsets 0x10 (pwm0), 0x50 (pwm1), 0x90 (pwm2) and
> 0xd0 (pwm3).
> 
> Hence, when using the MT8183 compatible with MT7986, the driver will
> access offsets 0x90 and 0xd0 in case the users enables the (bogus)
> outputs pwm2 and pwm3. These offsets, however, are not mentioned in the
> datasheet, so it has to be considered that writing things to these
> undocumented offsets could cause unknown behavior.
> 
> I hope it's more clear now what I mean.

But even your case is not correct. On MT7986 the device would still have
2 channels, how the heck he would get 4? Driver binds to ,t7986-pwm
compatible, which defines 2 channels.

> 
>>
>>>
>>> Also note that this case is the same as MT8183 vs. MT8365, they got
>>> distinct compatible strings and also for those two the only difference
>>> is the number of channels.
>>
>> So why they are not made compatible?
> 
> My guess is that it's for this very reason:
> To correctly communicate the capabilities (in this case: number of
> channels) to the driver and not have bogus pwmX show up in the OS
> which then causes undocumented MMIO register access in case anyone
> tries to actually use them.

No, that's not correct reason. There would be no wrong MMIO access and
capabilities would be still correctly communicated.

> 
>>
>>>
>>>>
>>>> If so, they are compatible.
>>>
>>> By that definition you should remove the additional compatible for
>>> MT8365 or rather, it should have been rejected for the same argument.
>>>
>>> I'm talking about
>>> commit fe00faee8060402a3d85aed95775e16838a6dad2
>>> commit 394b517585da9fbb2eea2f2103ff47d37321e976
>>
>> This is a pattern spreading in several Mediatek bindings and we already
>> commented on new patches. I don't know why people working on Mediatek do
>> not mark pieces compatible.
> 
> Others will have to answer that for you.
> 
> To me this looks a bit like a structural shortcoming of the PWM controller
> bindings: if there was a way to tell the driver "hey, this is like MT8183,
> but it got only two channels" that would solve it nicely.
> This could either be done using child-nodes for each PWM channel or by
> simply adding a 'nr-pwms' property.

No, it's rather someone did not think about Devicetree compatibles or
did not care to design the Mediatek bindings and just copy-paste
existing pattern...

Best regards,
Krzysztof
  

On Sun, Oct 23, 2022 at 11:29:45AM -0400, Krzysztof Kozlowski wrote:
> On 23/10/2022 11:01, Daniel Golle wrote:
> > On Sun, Oct 23, 2022 at 08:39:34AM -0400, Krzysztof Kozlowski wrote:
> >> On 23/10/2022 08:24, Daniel Golle wrote:
> >>> Hi Krzysztof,
> >>>
> >>> On Sat, Oct 22, 2022 at 12:35:25PM -0400, Krzysztof Kozlowski wrote:
> >>>> On 21/10/2022 18:58, Daniel Golle wrote:
> >>>>> On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
> >>>>>> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
> >>>>>>> Add new compatible string for MT7986 PWM.
> >>>>>>>
> >>>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> >>>>>>> ---
> >>>>>>>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
> >>>>>>>  1 file changed, 1 insertion(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>>>>>> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> >>>>>>> @@ -8,6 +8,7 @@ Required properties:
> >>>>>>>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
> >>>>>>>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
> >>>>>>>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
> >>>>>>> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
> >>>>>>
> >>>>>> This version of the PWM h/w is not compatible with any of the existing 
> >>>>>> chips? If it is, it should have a fallback compatible.
> >>>>>
> >>>>> No, it is unique because it comes with just 2 PWM channels.
> >>>>> Otherwise the driver behaves just like for MT8183 (4 channels) or
> >>>>> MT8365 (3 channels) which also got distinct compatible strings.
> >>>>
> >>>> Then something would be here compatible. E.g. If you bound MT8183 with
> >>>> mt7986-pwm compatible, would you get working device with two channels?
> >>>
> >>> Yes, but I'd see another 2 channels which do not work, accessing them
> >>> may even cause problems (I haven't tried that) as it means accessing
> >>> an undocumented memory range of the SoC which we in general we
> >>> shouldn't be messing around with.
> >>
> >> Why on MT8183 there would be undocumented memory? Where is undocumented
> >> memory?
> > 
> > So we were talking about using the MT8183 compatible for MT7986 SoC, as
> > the PWM units are identical apart from the number of channels they
> > offer:
> 
> No, we talk about MT8183 with mt7986-pwm compatible. Read again my message.

Ok, that was not clear to me. I understood the other way around, to use
`mediatek,mt8183-pwm` on MT7986 (and not even introduce an additional
compatible for MT7986 in the driver, but only list it as compatibly with
existing MT8183 in binding docs).

So: The to-be newly introduced `mediatek,mt7986-pwm` should work on
MT8183 as well, but in that case only two of the four channels would be
accessible.

> 
> > 
> > MT7986 got 2 PWM channels. The MMIO registers used for those two
> > channels start at offsets 0x10 (pwm0) and 0x50 (pwm1)
> > 
> > MT8183 got 4 PWM channels. The MMIO registers used for those four
> > channels start of offsets 0x10 (pwm0), 0x50 (pwm1), 0x90 (pwm2) and
> > 0xd0 (pwm3).
> > 
> > Hence, when using the MT8183 compatible with MT7986, the driver will
> > access offsets 0x90 and 0xd0 in case the users enables the (bogus)
> > outputs pwm2 and pwm3. These offsets, however, are not mentioned in the
> > datasheet, so it has to be considered that writing things to these
> > undocumented offsets could cause unknown behavior.
> > 
> > I hope it's more clear now what I mean.
> 
> But even your case is not correct. On MT7986 the device would still have
> 2 channels, how the heck he would get 4? Driver binds to ,t7986-pwm
> compatible, which defines 2 channels.

If we do introduce that new compatible, then yes.
I understood you were suggesting to use `mediatek,mt8183` on MT7986
hardware which would have resulted in such behavior.

> 
> > 
> >>
> >>>
> >>> Also note that this case is the same as MT8183 vs. MT8365, they got
> >>> distinct compatible strings and also for those two the only difference
> >>> is the number of channels.
> >>
> >> So why they are not made compatible?
> > 
> > My guess is that it's for this very reason:
> > To correctly communicate the capabilities (in this case: number of
> > channels) to the driver and not have bogus pwmX show up in the OS
> > which then causes undocumented MMIO register access in case anyone
> > tries to actually use them.
> 
> No, that's not correct reason. There would be no wrong MMIO access and
> capabilities would be still correctly communicated.

So what exactly do you mean by "made compatible"?
In the driver? In DTS files? In DT-bindings?

> 
> > 
> >>
> >>>
> >>>>
> >>>> If so, they are compatible.
> >>>
> >>> By that definition you should remove the additional compatible for
> >>> MT8365 or rather, it should have been rejected for the same argument.
> >>>
> >>> I'm talking about
> >>> commit fe00faee8060402a3d85aed95775e16838a6dad2
> >>> commit 394b517585da9fbb2eea2f2103ff47d37321e976
> >>
> >> This is a pattern spreading in several Mediatek bindings and we already
> >> commented on new patches. I don't know why people working on Mediatek do
> >> not mark pieces compatible.
> > 
> > Others will have to answer that for you.
> > 
> > To me this looks a bit like a structural shortcoming of the PWM controller
> > bindings: if there was a way to tell the driver "hey, this is like MT8183,
> > but it got only two channels" that would solve it nicely.
> > This could either be done using child-nodes for each PWM channel or by
> > simply adding a 'nr-pwms' property.
> 
> No, it's rather someone did not think about Devicetree compatibles or
> did not care to design the Mediatek bindings and just copy-paste
> existing pattern...

So in that case, maybe those existing patterns should be cleaned up?
Can you suggest a change? (even informal, I can wrap it up as patch).

To me, code is almost always better documentation than actual
documentation, which is usually time consuming to read, hard to
understand (compared to reading code or existing examples), and it's
commonly full of errors. I hardly waste my time with it, if there is
code or existing examples, I will always prefer do read and understand
that.


Cheers


Daniel
  

On 23/10/2022 11:45, Daniel Golle wrote:
> On Sun, Oct 23, 2022 at 11:29:45AM -0400, Krzysztof Kozlowski wrote:
>> On 23/10/2022 11:01, Daniel Golle wrote:
>>> On Sun, Oct 23, 2022 at 08:39:34AM -0400, Krzysztof Kozlowski wrote:
>>>> On 23/10/2022 08:24, Daniel Golle wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On Sat, Oct 22, 2022 at 12:35:25PM -0400, Krzysztof Kozlowski wrote:
>>>>>> On 21/10/2022 18:58, Daniel Golle wrote:
>>>>>>> On Fri, Oct 21, 2022 at 05:23:38PM -0500, Rob Herring wrote:
>>>>>>>> On Fri, Oct 21, 2022 at 04:25:18PM +0100, Daniel Golle wrote:
>>>>>>>>> Add new compatible string for MT7986 PWM.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>>>>>> ---
>>>>>>>>>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
>>>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>>>>>> index 554c96b6d0c3e0..6f4e60c9e18b81 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
>>>>>>>>> @@ -8,6 +8,7 @@ Required properties:
>>>>>>>>>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
>>>>>>>>>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
>>>>>>>>>     - "mediatek,mt7629-pwm": found on mt7629 SoC.
>>>>>>>>> +   - "mediatek,mt7986-pwm": found on mt7986 SoC.
>>>>>>>>
>>>>>>>> This version of the PWM h/w is not compatible with any of the existing 
>>>>>>>> chips? If it is, it should have a fallback compatible.
>>>>>>>
>>>>>>> No, it is unique because it comes with just 2 PWM channels.
>>>>>>> Otherwise the driver behaves just like for MT8183 (4 channels) or
>>>>>>> MT8365 (3 channels) which also got distinct compatible strings.
>>>>>>
>>>>>> Then something would be here compatible. E.g. If you bound MT8183 with
>>>>>> mt7986-pwm compatible, would you get working device with two channels?
>>>>>
>>>>> Yes, but I'd see another 2 channels which do not work, accessing them
>>>>> may even cause problems (I haven't tried that) as it means accessing
>>>>> an undocumented memory range of the SoC which we in general we
>>>>> shouldn't be messing around with.
>>>>
>>>> Why on MT8183 there would be undocumented memory? Where is undocumented
>>>> memory?
>>>
>>> So we were talking about using the MT8183 compatible for MT7986 SoC, as
>>> the PWM units are identical apart from the number of channels they
>>> offer:
>>
>> No, we talk about MT8183 with mt7986-pwm compatible. Read again my message.
> 
> Ok, that was not clear to me. I understood the other way around, to use
> `mediatek,mt8183-pwm` on MT7986 (and not even introduce an additional
> compatible for MT7986 in the driver, but only list it as compatibly with
> existing MT8183 in binding docs).

You need dedicated compatible (which will be used by the driver) because
these are two different devices.

https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42


> So: The to-be newly introduced `mediatek,mt7986-pwm` should work on
> MT8183 as well, but in that case only two of the four channels would be
> accessible.
> 
>>
>>>
>>> MT7986 got 2 PWM channels. The MMIO registers used for those two
>>> channels start at offsets 0x10 (pwm0) and 0x50 (pwm1)
>>>
>>> MT8183 got 4 PWM channels. The MMIO registers used for those four
>>> channels start of offsets 0x10 (pwm0), 0x50 (pwm1), 0x90 (pwm2) and
>>> 0xd0 (pwm3).
>>>
>>> Hence, when using the MT8183 compatible with MT7986, the driver will
>>> access offsets 0x90 and 0xd0 in case the users enables the (bogus)
>>> outputs pwm2 and pwm3. These offsets, however, are not mentioned in the
>>> datasheet, so it has to be considered that writing things to these
>>> undocumented offsets could cause unknown behavior.
>>>
>>> I hope it's more clear now what I mean.
>>
>> But even your case is not correct. On MT7986 the device would still have
>> 2 channels, how the heck he would get 4? Driver binds to ,t7986-pwm
>> compatible, which defines 2 channels.
> 
> If we do introduce that new compatible, then yes.

No one here argued against introducing new compatible. The question was
why these devices are not compatible with each other? Why new compatible
is not made a part of family of existing compatibles.


> I understood you were suggesting to use `mediatek,mt8183` on MT7986
> hardware which would have resulted in such behavior.
> 
>>
>>>
>>>>
>>>>>
>>>>> Also note that this case is the same as MT8183 vs. MT8365, they got
>>>>> distinct compatible strings and also for those two the only difference
>>>>> is the number of channels.
>>>>
>>>> So why they are not made compatible?
>>>
>>> My guess is that it's for this very reason:
>>> To correctly communicate the capabilities (in this case: number of
>>> channels) to the driver and not have bogus pwmX show up in the OS
>>> which then causes undocumented MMIO register access in case anyone
>>> tries to actually use them.
>>
>> No, that's not correct reason. There would be no wrong MMIO access and
>> capabilities would be still correctly communicated.
> 
> So what exactly do you mean by "made compatible"?
> In the driver? In DTS files? In DT-bindings?

I review bindings, so by "devices being made compatible" I meant exactly
what Devicetree specification asks:

"The property value consists of a concatenated list of null terminated
strings, from most specific to most general. They allow a device to
express its compatibility with a family of similar devices, potentially
allowing a single device driver to match against several devices."

Which means that devices share some common characteristics, like
programming model, and differ by some other pieces. For device drivers,
usually this also means that a device can use any of the compatibles to
bind and function properly, within the limits/features of that compatible.

>>>>>> If so, they are compatible.
>>>>>
>>>>> By that definition you should remove the additional compatible for
>>>>> MT8365 or rather, it should have been rejected for the same argument.
>>>>>
>>>>> I'm talking about
>>>>> commit fe00faee8060402a3d85aed95775e16838a6dad2
>>>>> commit 394b517585da9fbb2eea2f2103ff47d37321e976
>>>>
>>>> This is a pattern spreading in several Mediatek bindings and we already
>>>> commented on new patches. I don't know why people working on Mediatek do
>>>> not mark pieces compatible.
>>>
>>> Others will have to answer that for you.
>>>
>>> To me this looks a bit like a structural shortcoming of the PWM controller
>>> bindings: if there was a way to tell the driver "hey, this is like MT8183,
>>> but it got only two channels" that would solve it nicely.
>>> This could either be done using child-nodes for each PWM channel or by
>>> simply adding a 'nr-pwms' property.
>>
>> No, it's rather someone did not think about Devicetree compatibles or
>> did not care to design the Mediatek bindings and just copy-paste
>> existing pattern...
> 
> So in that case, maybe those existing patterns should be cleaned up?
> Can you suggest a change? (even informal, I can wrap it up as patch).

Someone knowing the devices should reorganize compatibles to come up
with family models, e.g. expressed in DT schema like:

oneOf:
 - const: mediatek,mt7986-pwm
 - items:
     - enum:
         - mediatek,mt8183-pwm
     - const: mediatek,mt7986-pwm

and so on.

> To me, code is almost always better documentation than actual
> documentation, which is usually time consuming to read, hard to
> understand (compared to reading code or existing examples), and it's
> commonly full of errors. I hardly waste my time with it, if there is
> code or existing examples, I will always prefer do read and understand
> that.

Example schema expresses it:

https://elixir.bootlin.com/linux/v6.0-rc1/source/Documentation/devicetree/bindings/example-schema.yaml#L43



Best regards,
Krzysztof