| Message ID | 1709727995-19821-2-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add eFuse region for Qualcomm SoCs | expand |
On 3/6/24 13:26, Mukesh Ojha wrote: > Add the qfprom node for sm8450 SoC. > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi > index b86be34a912b..02089a388d03 100644 > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi > @@ -4575,6 +4575,13 @@ > }; > }; > > + qfprom: efuse@221c8000 { > + compatible = "qcom,sm8450-qfprom", "qcom,qfprom"; > + reg = <0 0x221c8000 0 0x1000>; Is is really only 0x1000-long? Also, is the base you put here the ECC-corrected part (if that still exists)? Konrad
Sorry for the late reply, was on vacation. On 3/6/2024 9:24 PM, Konrad Dybcio wrote: > > > On 3/6/24 13:26, Mukesh Ojha wrote: >> Add the qfprom node for sm8450 SoC. >> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi >> b/arch/arm64/boot/dts/qcom/sm8450.dtsi >> index b86be34a912b..02089a388d03 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi >> @@ -4575,6 +4575,13 @@ >> }; >> }; >> + qfprom: efuse@221c8000 { >> + compatible = "qcom,sm8450-qfprom", "qcom,qfprom"; >> + reg = <0 0x221c8000 0 0x1000>; > > Is is really only 0x1000-long? Also, is the base you put > here the ECC-corrected part (if that still exists)? No, its not. Entire fuse space is this. 0x221C0000-0x221Cbfff ECC corrected range is this 0x221C2000-0x221C3fff and High level OS does have a access to ECC range however, they are not recommended for SW usage. Above mentioned SW range(4) in the patch is one and only accessible range available out of 0-7 SW ranges(0x221C4000-0x221Cbfff with each size 0x1000) and does not have ECC fuses. All the downstream use cases are getting fulfilled with this. -Mukesh > > Konrad
On 14.03.2024 17:43, Mukesh Ojha wrote: > Sorry for the late reply, was on vacation. > > On 3/6/2024 9:24 PM, Konrad Dybcio wrote: >> >> >> On 3/6/24 13:26, Mukesh Ojha wrote: >>> Add the qfprom node for sm8450 SoC. >>> >>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>> --- >>> arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi >>> index b86be34a912b..02089a388d03 100644 >>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi >>> @@ -4575,6 +4575,13 @@ >>> }; >>> }; >>> + qfprom: efuse@221c8000 { >>> + compatible = "qcom,sm8450-qfprom", "qcom,qfprom"; >>> + reg = <0 0x221c8000 0 0x1000>; >> >> Is is really only 0x1000-long? Also, is the base you put >> here the ECC-corrected part (if that still exists)? > > No, its not. > > Entire fuse space is this. > 0x221C0000-0x221Cbfff > > ECC corrected range is this 0x221C2000-0x221C3fff and High level OS > does have a access to ECC range however, they are not recommended for > SW usage. Are you sure? Bjorn always insisted to use the corrected bit. The ancient APQ8016 TRM also mentions this: QFPROM Corrected Region - [...] This region is intended for functional use of the QFPROM stored data by software. > > Above mentioned SW range(4) in the patch is one and only accessible range available out of 0-7 SW ranges(0x221C4000-0x221Cbfff with each > size 0x1000) and does not have ECC fuses. > > All the downstream use cases are getting fulfilled with this. Right, and I don't quite get why there's an corrected region if it sits unused. Konrad
On Thu, Mar 14, 2024 at 10:13:59PM +0530, Mukesh Ojha wrote: > Sorry for the late reply, was on vacation. > > On 3/6/2024 9:24 PM, Konrad Dybcio wrote: > > > > > > On 3/6/24 13:26, Mukesh Ojha wrote: > > > Add the qfprom node for sm8450 SoC. > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > --- > > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > b/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > index b86be34a912b..02089a388d03 100644 > > > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > @@ -4575,6 +4575,13 @@ > > > }; > > > }; > > > + qfprom: efuse@221c8000 { > > > + compatible = "qcom,sm8450-qfprom", "qcom,qfprom"; > > > + reg = <0 0x221c8000 0 0x1000>; > > > > Is is really only 0x1000-long? Also, is the base you put > > here the ECC-corrected part (if that still exists)? > > No, its not. > > Entire fuse space is this. > 0x221C0000-0x221Cbfff > > ECC corrected range is this 0x221C2000-0x221C3fff and High level OS That's 0x2000. Does this then also imply that the ECC-corrected values are no longer mapped 1:1 with non-corrected, or why do they differ in size? > does have a access to ECC range however, they are not recommended for > SW usage. > > Above mentioned SW range(4) in the patch is one and only accessible range > available out of 0-7 SW ranges(0x221C4000-0x221Cbfff with each > size 0x1000) and does not have ECC fuses. > So you're saying that in contrast to other platforms, the 4th software range, dedicated for HLOS, does not have a matching ECC-corrected shadow? If that's the case, then "not recommended for SW usage" sounds wrong. > All the downstream use cases are getting fulfilled with this. > You only need ECC if you're unlucky... Regards, Bjorn
On Sun, Mar 17, 2024 at 08:21:58PM GMT, Bjorn Andersson wrote: > On Thu, Mar 14, 2024 at 10:13:59PM +0530, Mukesh Ojha wrote: > > Sorry for the late reply, was on vacation. > > > > On 3/6/2024 9:24 PM, Konrad Dybcio wrote: > > > > > > > > > On 3/6/24 13:26, Mukesh Ojha wrote: > > > > Add the qfprom node for sm8450 SoC. > > > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > > --- > > > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > > b/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > > index b86be34a912b..02089a388d03 100644 > > > > --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > > +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi > > > > @@ -4575,6 +4575,13 @@ > > > > }; > > > > }; > > > > + qfprom: efuse@221c8000 { > > > > + compatible = "qcom,sm8450-qfprom", "qcom,qfprom"; > > > > + reg = <0 0x221c8000 0 0x1000>; > > > > > > Is is really only 0x1000-long? Also, is the base you put > > > here the ECC-corrected part (if that still exists)? > > > > No, its not. > > > > Entire fuse space is this. > > 0x221C0000-0x221Cbfff > > > > ECC corrected range is this 0x221C2000-0x221C3fff and High level OS > > That's 0x2000. Does this then also imply that the ECC-corrected values > are no longer mapped 1:1 with non-corrected, or why do they differ in > size? > > > does have a access to ECC range however, they are not recommended for > > SW usage. > > > > Above mentioned SW range(4) in the patch is one and only accessible range > > available out of 0-7 SW ranges(0x221C4000-0x221Cbfff with each > > size 0x1000) and does not have ECC fuses. > > > > So you're saying that in contrast to other platforms, the 4th software > range, dedicated for HLOS, does not have a matching ECC-corrected > shadow? If that's the case, then "not recommended for SW usage" sounds > wrong. > > > All the downstream use cases are getting fulfilled with this. > > > > You only need ECC if you're unlucky... > The patch is either incorrect or the commit message is lacking answers to the questions from Konrad and myself. Would have appreciated a reply here, but either way I'm marking this as "changes requested" and dropping it from the queue. Regards, Bjorn
On 5/28/2024 9:07 PM, Bjorn Andersson wrote: > On Sun, Mar 17, 2024 at 08:21:58PM GMT, Bjorn Andersson wrote: >> On Thu, Mar 14, 2024 at 10:13:59PM +0530, Mukesh Ojha wrote: >>> Sorry for the late reply, was on vacation. >>> >>> On 3/6/2024 9:24 PM, Konrad Dybcio wrote: >>>> >>>> >>>> On 3/6/24 13:26, Mukesh Ojha wrote: >>>>> Add the qfprom node for sm8450 SoC. >>>>> >>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>>> --- >>>>> arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi >>>>> b/arch/arm64/boot/dts/qcom/sm8450.dtsi >>>>> index b86be34a912b..02089a388d03 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi >>>>> @@ -4575,6 +4575,13 @@ >>>>> }; >>>>> }; >>>>> + qfprom: efuse@221c8000 { >>>>> + compatible = "qcom,sm8450-qfprom", "qcom,qfprom"; >>>>> + reg = <0 0x221c8000 0 0x1000>; >>>> >>>> Is is really only 0x1000-long? Also, is the base you put >>>> here the ECC-corrected part (if that still exists)? >>> >>> No, its not. >>> >>> Entire fuse space is this. >>> 0x221C0000-0x221Cbfff >>> >>> ECC corrected range is this 0x221C2000-0x221C3fff and High level OS >> >> That's 0x2000. Does this then also imply that the ECC-corrected values >> are no longer mapped 1:1 with non-corrected, or why do they differ in >> size? >> >>> does have a access to ECC range however, they are not recommended for >>> SW usage. >>> >>> Above mentioned SW range(4) in the patch is one and only accessible range >>> available out of 0-7 SW ranges(0x221C4000-0x221Cbfff with each >>> size 0x1000) and does not have ECC fuses. >>> >> >> So you're saying that in contrast to other platforms, the 4th software >> range, dedicated for HLOS, does not have a matching ECC-corrected >> shadow? If that's the case, then "not recommended for SW usage" sounds >> wrong. >> >>> All the downstream use cases are getting fulfilled with this. >>> >> >> You only need ECC if you're unlucky... >> > > The patch is either incorrect or the commit message is lacking answers > to the questions from Konrad and myself. > > Would have appreciated a reply here, but either way I'm marking this as > "changes requested" and dropping it from the queue. Apology for coming late on this., it just skipped from my tracking.. Frankly, I don't have convincing answer apart from what reason are exposed to kernel to read for SoCs like sm8[456]0 , will try to get the answer soon. -Mukesh
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi index b86be34a912b..02089a388d03 100644 --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi @@ -4575,6 +4575,13 @@ }; }; + qfprom: efuse@221c8000 { + compatible = "qcom,sm8450-qfprom", "qcom,qfprom"; + reg = <0 0x221c8000 0 0x1000>; + #address-cells = <1>; + #size-cells = <1>; + }; + nsp_noc: interconnect@320c0000 { compatible = "qcom,sm8450-nsp-noc"; reg = <0 0x320c0000 0 0x10000>;
Add the qfprom node for sm8450 SoC. Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)