Re: [PATCH] dt-bindings: soc: qcom: qcom,spm: support regulator SAW2 devics

[Christian Marangi <ansuelsmth@gmail.com>]

On Sun, Oct 02, 2022 at 03:20:57PM +0300, Dmitry Baryshkov wrote:
> On Sun, 2 Oct 2022 at 11:49, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 01/10/2022 01:14, Dmitry Baryshkov wrote:
> > > Merge qcom,saw2.txt bindings to existing qcom,spm.yaml. This fixes
> > > compatibility of qcom,spm schema with regulator SAW2 devices.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  .../devicetree/bindings/arm/msm/qcom,saw2.txt | 58 -------------------
> > >  .../bindings/soc/qcom/qcom,spm.yaml           | 44 +++++++++-----
> >
> > You need to update reference in
> > Documentation/devicetree/bindings/arm/cpus.yaml
> 
> ack
> 
> >
> > >  2 files changed, 30 insertions(+), 72 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> > > deleted file mode 100644
> > > index c0e3c3a42bea..000000000000
> > > --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> 
> [skipped]
> 
> > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,spm.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,spm.yaml
> > > index f433e6e0a19f..8fe35fde70b8 100644
> > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,spm.yaml
> > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,spm.yaml
> > > @@ -16,23 +16,33 @@ description: |
> > >
> > >  properties:
> > >    compatible:
> > > -    items:
> > > -      - enum:
> > > -          - qcom,sdm660-gold-saw2-v4.1-l2
> > > -          - qcom,sdm660-silver-saw2-v4.1-l2
> > > -          - qcom,msm8998-gold-saw2-v4.1-l2
> > > -          - qcom,msm8998-silver-saw2-v4.1-l2
> > > -          - qcom,msm8909-saw2-v3.0-cpu
> > > -          - qcom,msm8916-saw2-v3.0-cpu
> > > -          - qcom,msm8226-saw2-v2.1-cpu
> > > -          - qcom,msm8974-saw2-v2.1-cpu
> > > -          - qcom,apq8084-saw2-v2.1-cpu
> > > -          - qcom,apq8064-saw2-v1.1-cpu
> > > +    oneOf:
> > >        - const: qcom,saw2
> >
> > I understand old bindings had it, but I don't think we really want to
> > support the generic compatible on its own. Even old bindings indicated
> > that there are several differences between SAWs.
> >
> > Especially confusing is that once qcom,saw2 can be alone and in other
> > cases must be preceded by specific compatible. IOW, you allow for
> > apq8064 two cases:
> >
> > 1. qcom,apq8064-saw2-v1.1-cpu, qcom,saw2
> > 2. qcom,saw2
> >
> > I think we should instead add everywhere specific compatibles.
> 
> I see your point. Yes, it's probably worth doing that.
> 
> Robert, Christian, can you possibly check the version of the SAW2 used
> on ipq4019 and ipq8064? It can be read from the SPM block at the
> register offset 0xfd0.
>

From what I notice from QSDK it seems ipq806x doesn't have a reg to
provide version. (there are many variant of v1 revision)

Anyway these are the values from uboot

(IPQ) # md.l 0x2089FD0
02089fd0: 00000000    ....
(IPQ) # md.l 0x2099FD0
02099fd0: 00000000    ....
(IPQ) # md.l 0x2012FD0
02012fd0: 00000000    ....

> > > +      - items:
> > > +          - enum:
> > > +              - qcom,sdm660-gold-saw2-v4.1-l2
> > > +              - qcom,sdm660-silver-saw2-v4.1-l2
> > > +              - qcom,msm8998-gold-saw2-v4.1-l2
> > > +              - qcom,msm8998-silver-saw2-v4.1-l2
> > > +              - qcom,msm8909-saw2-v3.0-cpu
> > > +              - qcom,msm8916-saw2-v3.0-cpu
> > > +              - qcom,msm8226-saw2-v2.1-cpu
> > > +              - qcom,msm8974-saw2-v2.1-cpu
> > > +              - qcom,apq8084-saw2-v2.1-cpu
> > > +              - qcom,apq8064-saw2-v1.1-cpu
> > > +          - const: qcom,saw2
> > >
> > >    reg:
> > > -    description: Base address and size of the SPM register region
> > > -    maxItems: 1
> > > +    description: Base address and size of the SPM register region. An optional
> > > +      second element specifies the base address and size of the alias register
> > > +      region.
> > > +    minItems: 1
> > > +    maxItems: 2
> >
> > And it seems second region is not present on some variants?
> 
> The second region is a bit of a puzzle for me as it doesn't seem to be
> used at all.
> 
> -- 
> With best wishes
> Dmitry

-- 
	Ansuel