diff mbox series

[05/16] dt-bindings: clock: qcom,mmcc-msm8998: drop core_bi_pll_test_se

Message ID 20221228133243.3052132-6-dmitry.baryshkov@linaro.org (mailing list archive)
State Superseded
Headers show
Series clk: qcom: get rid of core_bi_pll_test_se | expand

Commit Message

Dmitry Baryshkov Dec. 28, 2022, 1:32 p.m. UTC
The test clock apparently it's not used by anyone upstream. Remove it.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 Documentation/devicetree/bindings/clock/qcom,mmcc.yaml | 2 --
 1 file changed, 2 deletions(-)

Comments

Jeffrey Hugo Jan. 3, 2023, 3:38 p.m. UTC | #1
On Wed, Dec 28, 2022 at 6:33 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> The test clock apparently it's not used by anyone upstream. Remove it.

IMO, NACK,

This is not a valid justification.

The DT is supposed to describe the hardware, and should be complete in
that regard.  This clock exists in the hardware, so it should be
described.

DT is supposed to be separate from Linux, that is it doesn't matter
that Linux doesn't consume this clock.  Maybe FreeBSD does, or some
other OS.  Linux doesn't own Device Tree any more than it owns BIOS or
ACPI.

Also, I'm listed as a maintainer for this binding, yet this series is
not addressed to me.  Seems like you might need to review how you are
composing your patches.
Dmitry Baryshkov Jan. 3, 2023, 4:09 p.m. UTC | #2
On 03/01/2023 17:38, Jeffrey Hugo wrote:
> On Wed, Dec 28, 2022 at 6:33 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> The test clock apparently it's not used by anyone upstream. Remove it.
> 
> IMO, NACK,
> 
> This is not a valid justification.
> 
> The DT is supposed to describe the hardware, and should be complete in
> that regard.  This clock exists in the hardware, so it should be
> described.

Most of Qualcomm clock controllers can input clocks from 
core_bi_pll_test_se. But we are listing them only for a small number of 
them. And even on these platforms nobody provides this clock.

Maybe you shed some light here, what is the source of this clock? Who 
provides the clock, e.g. on msm8998 platform?

> 
> DT is supposed to be separate from Linux, that is it doesn't matter
> that Linux doesn't consume this clock.  Maybe FreeBSD does, or some
> other OS.  Linux doesn't own Device Tree any more than it owns BIOS or
> ACPI.
> 
> Also, I'm listed as a maintainer for this binding, yet this series is
> not addressed to me.  Seems like you might need to review how you are
> composing your patches.

Ack, excuse me. I missed the 'in file' maintainers.
Jeffrey Hugo Jan. 3, 2023, 4:31 p.m. UTC | #3
On Tue, Jan 3, 2023 at 9:09 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 03/01/2023 17:38, Jeffrey Hugo wrote:
> > On Wed, Dec 28, 2022 at 6:33 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >>
> >> The test clock apparently it's not used by anyone upstream. Remove it.
> >
> > IMO, NACK,
> >
> > This is not a valid justification.
> >
> > The DT is supposed to describe the hardware, and should be complete in
> > that regard.  This clock exists in the hardware, so it should be
> > described.
>
> Most of Qualcomm clock controllers can input clocks from
> core_bi_pll_test_se. But we are listing them only for a small number of
> them. And even on these platforms nobody provides this clock.

IMO the Qcom bindings could use some more rigor, I just don't have the
cycles to help there.  The ones I've looked at appear to be written
from the perspective of "what does the linux driver need" and not
"what do we have in the schematic".  Often "what does the linux driver
need" changes over time, which means the binding needs to evolve,
which breaks the interface.  It's entirely valid to not use something
in the Linux driver, especially as the platform implementation is
probably minimal during early bringup, but such things are expected to
be implemented eventually.

There is a huge set of existing platforms where we probably can't go
back and fix them since the binding is already defined, but going
forward, new platforms can do better.

>
> Maybe you shed some light here, what is the source of this clock? Who
> provides the clock, e.g. on msm8998 platform?

It is an external input to the SoC, similar to CXO.

On the laptops, TP88 (test point) on the main motherboard is routed to
the SoC pin.  I don't have schematics for every platform in the wild,
so I can't say if that is the norm.

-Jeff
Dmitry Baryshkov Jan. 4, 2023, 10:45 a.m. UTC | #4
On 03/01/2023 18:31, Jeffrey Hugo wrote:
> On Tue, Jan 3, 2023 at 9:09 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 03/01/2023 17:38, Jeffrey Hugo wrote:
>>> On Wed, Dec 28, 2022 at 6:33 AM Dmitry Baryshkov
>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>> The test clock apparently it's not used by anyone upstream. Remove it.
>>>
>>> IMO, NACK,
>>>
>>> This is not a valid justification.
>>>
>>> The DT is supposed to describe the hardware, and should be complete in
>>> that regard.  This clock exists in the hardware, so it should be
>>> described.
>>
>> Most of Qualcomm clock controllers can input clocks from
>> core_bi_pll_test_se. But we are listing them only for a small number of
>> them. And even on these platforms nobody provides this clock.
> 
> IMO the Qcom bindings could use some more rigor, I just don't have the
> cycles to help there.  The ones I've looked at appear to be written
> from the perspective of "what does the linux driver need" and not
> "what do we have in the schematic".  Often "what does the linux driver
> need" changes over time, which means the binding needs to evolve,
> which breaks the interface.  It's entirely valid to not use something
> in the Linux driver, especially as the platform implementation is
> probably minimal during early bringup, but such things are expected to
> be implemented eventually.

Well, the problem is that not all of us have access to lowlevel 
documentation, thus we have to resort to the information provided by the 
vendor kernel. Sometimes our approach to platform implementation changes.

> 
> There is a huge set of existing platforms where we probably can't go
> back and fix them since the binding is already defined, but going
> forward, new platforms can do better.

Bindings can change (especially if the change is backwards-compatible). 
We are finishing one of such migrations (to use DT to bind parent clocks).

If you have anything particular in mind, please don't hesitate to 
describe your ideas.

> 
>>
>> Maybe you shed some light here, what is the source of this clock? Who
>> provides the clock, e.g. on msm8998 platform?
> 
> It is an external input to the SoC, similar to CXO.
> 
> On the laptops, TP88 (test point) on the main motherboard is routed to
> the SoC pin.  I don't have schematics for every platform in the wild,
> so I can't say if that is the norm.

Ack, externally supplied clock. That's great. Thank you.

Let's leave the question of having core_bi_pll clock to subsystem 
(Bjorn and Stephen) and bindings (Rob, Krzysztof) maintainers.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
index e6d17426e903..cf04d791093f 100644
--- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
@@ -229,7 +229,6 @@  allOf:
             - description: HDMI phy PLL clock
             - description: DisplayPort phy PLL link clock
             - description: DisplayPort phy PLL vco clock
-            - description: Test clock
 
         clock-names:
           items:
@@ -242,7 +241,6 @@  allOf:
             - const: hdmipll
             - const: dplink
             - const: dpvco
-            - const: core_bi_pll_test_se
 
   - if:
       properties: