Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 16:53:48 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Johan Hovold <johan+linaro@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        manivannan.sadhasivam@linaro.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer
Message-ID: <Ys2K/BH/kAeTBz5t@hovoldconsulting.com>
References: <20220707160858.3178771-1-bjorn.andersson@linaro.org>
 <Ysvlqw/+eMk5XLRY@hovoldconsulting.com>
 <YszcSgnSrbsncw0J@ripper>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YszcSgnSrbsncw0J@ripper>
Precedence: bulk

On Mon, Jul 11, 2022 at 07:28:26PM -0700, Bjorn Andersson wrote:
> On Mon 11 Jul 01:56 PDT 2022, Johan Hovold wrote:
> 
> > On Thu, Jul 07, 2022 at 09:08:58AM -0700, Bjorn Andersson wrote:
> > > The timer node needs ranges specified to map the 1-cell children to the
> > > 2-cell address range used in /soc. This addition never made it into the
> > > patch that was posted and merged, so add it now.
> > > 
> > > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > index 2bdb42c88311..37a4cd6f85b6 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > @@ -1667,6 +1667,7 @@ timer@17c20000 {
> > >  			reg = <0x0 0x17c20000 0x0 0x1000>;
> > >  			#address-cells = <1>;
> > >  			#size-cells = <1>;
> > > +			ranges = <0 0 0 0x20000000>;
> > 
> > While addressing the current issue, this looks odd to me. Why not use a
> > non-zero parent bus address here instead?
> > 
> 
> I guess we could express the frames relative the timer range, but that
> would imply that anyone porting downstream dts snippets would have to
> translate these addresses - or more likely would end up just copying the
> existing cases.
> 
> > And please use hex notation consistently for the addresses.
> 
> That seems like a reasonable ask, I can fix that up. But on both
> accounts this matches what I merged for all the other platforms in:
> 
> 458ebdbb8e5d ("arm64: dts: qcom: timer should use only 32-bit size")
> 
> 
> So I guess we'll also need to go back and fix up the style of all the
> other platforms - just because we're not allowed to express the frames
> in 64-bits according to the binding...

Would have been easier to just amend the binding. I don't think that
#size-cells = 1 constraint is set in stone as it was added when
converting to DT schema.

I also don't think you need to fixup the hex notation elsewhere, it's
quite inconsistent currently, but no need to make it worse.

But you probably should amend the commit message and mention that this
fixes time keeping. I had recently noticed that something was off
(journals rotating, and erratic cursor blinking) but didn't realise that
timers were broken until you posted this.

Johan