[PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Bjorn Andersson]

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>;
 
 			frame@17c21000 {
 				frame-number = <0>;
-- 
2.35.1

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Manivannan Sadhasivam]

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>;

Even though this looks correct, I'm wondering why other SoCs are defining the
child addresses in 2 cells. I don't think the timer frames can go beyond 32bit
address space. Should we fix them too?

But for this patch,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

>  
>  			frame@17c21000 {
>  				frame-number = <0>;
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Bjorn Andersson]

On Fri 08 Jul 23:59 PDT 2022, Manivannan Sadhasivam 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>;
> 
> Even though this looks correct, I'm wondering why other SoCs are defining the
> child addresses in 2 cells. I don't think the timer frames can go beyond 32bit
> address space. Should we fix them too?
> 

Neither addresses nor sizes used for mmio need more than 32 bits, but
ranges and dma-ranges are expressed in #size-cells. So unless these
cells are 2 there's no way for us to express the fact that (most of) our
busses uses 36 address bits.

And we need to be able to do this, because the SMMU on multiple
platforms claims that the bus is 40 address bits, so we end up running
into issues with the upper 4 bits of IOVAs being truncated.

Regards,
Bjorn

> But for this patch,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Thanks,
> Mani
> 
> >  
> >  			frame@17c21000 {
> >  				frame-number = <0>;
> > -- 
> > 2.35.1
> > 
> 
> -- 
> ??????????????????????????? ????????????????????????

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Johan Hovold]

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?

And please use hex notation consistently for the addresses.

>  
>  			frame@17c21000 {
>  				frame-number = <0>;

Johan

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Bjorn Andersson]

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...

Regards,
Bjorn

> 
> >  
> >  			frame@17c21000 {
> >  				frame-number = <0>;
> 
> Johan

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Johan Hovold]

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

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Johan Hovold]

On Tue, Jul 12, 2022 at 04:53:48PM +0200, Johan Hovold wrote:
> 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.

Ok, maybe it was done on purpose since Rob later merged

	c5c689d3221e ("dt-bindings: timer: Use non-empty ranges in example")

which suggests we should use a non-empty ranges as I mentioned above.

Any comments, Rob?

> 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

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Bjorn Andersson]

On Tue 12 Jul 09:53 CDT 2022, Johan Hovold wrote:

> 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.
> 

Rob disagrees with this idea:
https://lore.kernel.org/all/CAL_JsqJMMCBnukFZLJ8X14s1PwqT=VEwKjDVj8mm4h55pZpcuw@mail.gmail.com/

> 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.
> 

I'll prefix the 0s with some 0x in this case and we can care for the
other platforms some other day...

> 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.
> 

That sounds like a good idea, thanks for the suggestion.

Regards,
Bjorn

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Johan Hovold]

On Tue, Jul 12, 2022 at 08:53:34PM -0500, Bjorn Andersson wrote:
> On Tue 12 Jul 09:53 CDT 2022, Johan Hovold wrote:
> > On Mon, Jul 11, 2022 at 07:28:26PM -0700, Bjorn Andersson wrote:
> > > On Mon 11 Jul 01:56 PDT 2022, Johan Hovold wrote:

> > > > 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.
> 
> Rob disagrees with this idea:
> https://lore.kernel.org/all/CAL_JsqJMMCBnukFZLJ8X14s1PwqT=VEwKjDVj8mm4h55pZpcuw@mail.gmail.com/

Ok, thanks for the link. I'd still prefer a non-zero parent-bus address
in that ranges, but we can change that later too.

> > 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.
> > 
> 
> That sounds like a good idea, thanks for the suggestion.

I guess you're posting a v2, but otherwise feel free to add my 

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

when applying.

Johan

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Bjorn Andersson]

On Thu 07 Jul 11:08 CDT 2022, 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>

Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>

> ---
>  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>;
>  
>  			frame@17c21000 {
>  				frame-number = <0>;
> -- 
> 2.35.1
> 

Re: [PATCH] arm64: dts: qcom: sc8280xp: Add lost ranges for timer

[Bjorn Andersson]

On Thu, 7 Jul 2022 09:08:58 -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.
> 
> 

Applied, thanks!

[1/1] arm64: dts: qcom: sc8280xp: Add lost ranges for timer
      commit: 769fe42092a68dc34c1897673e781489428a108d

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>