| Message ID | 20230830-fp5-initial-v1-0-5a954519bbad@fairphone.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Initial support for the Fairphone 5 smartphone | expand |
On 30/08/2023 11:58, Luca Weiss wrote: > On some platforms like sc7280 on non-ChromeOS devices the core clock > cannot be touched by Linux so we cannot provide it. Mark it as optional > as accessing qfprom works without it. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > drivers/nvmem/qfprom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This should not be between DTS patches. DTS goes different tree than other driver changes. Please split your patchset and do not mix drivers and DTS. Best regards, Krzysztof
On 30/08/2023 11:58, Luca Weiss wrote: > Like other Qualcomm PMICs the PM7250B can be used on different addresses > on the SPMI bus. Use similar defines like the PMK8350 to make this > possible. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > index e8540c36bd99..3514de536baa 100644 > --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > @@ -7,6 +7,15 @@ > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/spmi/spmi.h> > > +/* This PMIC can be configured to be at different SIDs */ > +#ifndef PM7250B_SID > + #define PM7250B_SID 2 > +#endif Why do you send the same patch as v1, without any reference to previous discussions? You got here feedback already. https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ Best regards, Krzysztof
On 30/08/2023 11:58, Luca Weiss wrote: > Allow the gpio-reserved-ranges property on SC7280 TLMM. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 30/08/2023 11:58, Luca Weiss wrote: > Add SoC ID table entries for Qualcomm QCM6490. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 30.08.2023 11:58, Luca Weiss wrote: > Add the ID for the Qualcomm QCM6490 SoC. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
Hi, On Wed, Aug 30, 2023 at 2:58 AM Luca Weiss <luca.weiss@fairphone.com> wrote: > > On some platforms like sc7280 on non-ChromeOS devices the core clock > cannot be touched by Linux so we cannot provide it. Mark it as optional > as accessing qfprom works without it. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > drivers/nvmem/qfprom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Are you actually testing burning fuses from the OS, or are you just using the nvmem in "read-only" mode? From comments in the bindings, if you're trying to burn the fuses then the clock is required. If things are in read-only mode then the clock isn't required. When I compare to the driver, it seems like the driver assumes that if more than one memory region is provided then you must be supporting burning fuses. The bindings agree that having 4 memory regions specified means that the nvmem supports burning and 1 memory region specified means read-only. The extra 3 memory regions in the nvmem are all about fuse burning, I believe. So maybe the right fix here is to just change your dts to specify one memory region? -Doug
Hi, On Wed, Aug 30, 2023 at 7:43 AM Luca Weiss <luca.weiss@fairphone.com> wrote: > > On Wed Aug 30, 2023 at 4:30 PM CEST, Doug Anderson wrote: > > Hi, > > > > On Wed, Aug 30, 2023 at 2:58 AM Luca Weiss <luca.weiss@fairphone.com> wrote: > > > > > > On some platforms like sc7280 on non-ChromeOS devices the core clock > > > cannot be touched by Linux so we cannot provide it. Mark it as optional > > > as accessing qfprom works without it. > > > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > > --- > > > drivers/nvmem/qfprom.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Are you actually testing burning fuses from the OS, or are you just > > using the nvmem in "read-only" mode? From comments in the bindings, if > > you're trying to burn the fuses then the clock is required. If things > > are in read-only mode then the clock isn't required. > > Hi Doug, > > I definitely don't plan on burning any fuses on this phone. Not even > sure that's allowed by the TZ / boot stack. > > > > > When I compare to the driver, it seems like the driver assumes that if > > more than one memory region is provided then you must be supporting > > burning fuses. The bindings agree that having 4 memory regions > > specified means that the nvmem supports burning and 1 memory region > > specified means read-only. The extra 3 memory regions in the nvmem are > > all about fuse burning, I believe. > > > > So maybe the right fix here is to just change your dts to specify one > > memory region? > > I got feedback from Konrad that this here would be the preferred > approach compared to having a different dts for ChromeOS vs non-ChromeOS > devices. I don't feel strongly to either, for me it's also okay to > remove the extra memory regions and only have the main one used on > regular qcom devices. > > Let me know what you think. I don't hate the idea of leaving the extra memory regions in the dts. They do describe the hardware, after all, even if the main OS can't actually access those memory regions. ...though the same could also be said about the clock you've removed. Said another way: if you want to fully describe the hardware then the dts should have the extra memory regions and the clock. If you are OK w/ just describing the hardware in the way that the OS has access to then the dts should not have the extra memory regions and not have the clock. Does that sound right? If somehow you do end up with something like your patch, though, you're still missing a bit. Specifically, you don't want to "enable writing" a few lines below if you didn't get the clock, right? -Doug
On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: > On 30/08/2023 11:58, Luca Weiss wrote: > > Like other Qualcomm PMICs the PM7250B can be used on different addresses > > on the SPMI bus. Use similar defines like the PMK8350 to make this > > possible. > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > --- > > arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > index e8540c36bd99..3514de536baa 100644 > > --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > @@ -7,6 +7,15 @@ > > #include <dt-bindings/interrupt-controller/irq.h> > > #include <dt-bindings/spmi/spmi.h> > > > > +/* This PMIC can be configured to be at different SIDs */ > > +#ifndef PM7250B_SID > > + #define PM7250B_SID 2 > > +#endif > > Why do you send the same patch as v1, without any reference to previous > discussions? > > You got here feedback already. > > https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ Hi Krzysztof, I did mention that original patch in the cover letter of this series. I'm definitely aware of the discussion earlier this year there but also tried to get an update lately if there's any update with no response. If you want to block this patch, I'll have to remove pm7250b from the device dts, so we'll lose some functionality. Not sure what other approaches there could be. Regards Luca > > Best regards, > Krzysztof
On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: > > On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: > > On 30/08/2023 11:58, Luca Weiss wrote: > > > Like other Qualcomm PMICs the PM7250B can be used on different addresses > > > on the SPMI bus. Use similar defines like the PMK8350 to make this > > > possible. > > > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > > --- > > > arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > > index e8540c36bd99..3514de536baa 100644 > > > --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > > @@ -7,6 +7,15 @@ > > > #include <dt-bindings/interrupt-controller/irq.h> > > > #include <dt-bindings/spmi/spmi.h> > > > > > > +/* This PMIC can be configured to be at different SIDs */ > > > +#ifndef PM7250B_SID > > > + #define PM7250B_SID 2 > > > +#endif > > > > Why do you send the same patch as v1, without any reference to previous > > discussions? > > > > You got here feedback already. > > > > https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ > > Hi Krzysztof, > > I did mention that original patch in the cover letter of this series. > I'm definitely aware of the discussion earlier this year there but also > tried to get an update lately if there's any update with no response. I think the overall consensus was that my proposal is too complicated for the DT files. > > If you want to block this patch, I'll have to remove pm7250b from the > device dts, so we'll lose some functionality. Not sure what other > approaches there could be. > > Regards > Luca > > > > > Best regards, > > Krzysztof >
On 31/08/2023 13:33, Dmitry Baryshkov wrote: > On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: >> >> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: >>> On 30/08/2023 11:58, Luca Weiss wrote: >>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses >>>> on the SPMI bus. Use similar defines like the PMK8350 to make this >>>> possible. >>>> >>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>>> --- >>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- >>>> 1 file changed, 16 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>> index e8540c36bd99..3514de536baa 100644 >>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>> @@ -7,6 +7,15 @@ >>>> #include <dt-bindings/interrupt-controller/irq.h> >>>> #include <dt-bindings/spmi/spmi.h> >>>> >>>> +/* This PMIC can be configured to be at different SIDs */ >>>> +#ifndef PM7250B_SID >>>> + #define PM7250B_SID 2 >>>> +#endif >>> >>> Why do you send the same patch as v1, without any reference to previous >>> discussions? >>> >>> You got here feedback already. >>> >>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ >> >> Hi Krzysztof, >> >> I did mention that original patch in the cover letter of this series. >> I'm definitely aware of the discussion earlier this year there but also >> tried to get an update lately if there's any update with no response. > > I think the overall consensus was that my proposal is too complicated > for the DT files. I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and customize per address? No. I definitely do not agree to these ifndef->define. Maybe using just define would work (so drop ifndef->define), because this makes it obvious and fail-safe if included in wrong place... except that it is still not the define we expect. This is not the coding style present in other DTSes. The true problem how these SPMI bindings were created. Requiring SID address in every child is clearly redundant and I think we do not follow such approach anywhere else. Best regards, Krzysztof
On Thu, 31 Aug 2023 at 14:54, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 31/08/2023 13:33, Dmitry Baryshkov wrote: > > On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: > >> > >> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: > >>> On 30/08/2023 11:58, Luca Weiss wrote: > >>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses > >>>> on the SPMI bus. Use similar defines like the PMK8350 to make this > >>>> possible. > >>>> > >>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > >>>> --- > >>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > >>>> 1 file changed, 16 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>> index e8540c36bd99..3514de536baa 100644 > >>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>> @@ -7,6 +7,15 @@ > >>>> #include <dt-bindings/interrupt-controller/irq.h> > >>>> #include <dt-bindings/spmi/spmi.h> > >>>> > >>>> +/* This PMIC can be configured to be at different SIDs */ > >>>> +#ifndef PM7250B_SID > >>>> + #define PM7250B_SID 2 > >>>> +#endif > >>> > >>> Why do you send the same patch as v1, without any reference to previous > >>> discussions? > >>> > >>> You got here feedback already. > >>> > >>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ > >> > >> Hi Krzysztof, > >> > >> I did mention that original patch in the cover letter of this series. > >> I'm definitely aware of the discussion earlier this year there but also > >> tried to get an update lately if there's any update with no response. > > > > I think the overall consensus was that my proposal is too complicated > > for the DT files. > > I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and > customize per address? No. At the same time, we do keep SoC files separate from the board files. Yes, I'm slightly exaggerating here. I think that for PMIC files it makes sense to extract common parts if that eases reuse of the common parts. > > I definitely do not agree to these ifndef->define. Maybe using just > define would work (so drop ifndef->define), because this makes it > obvious and fail-safe if included in wrong place... except that it is > still not the define we expect. This is not the coding style present in > other DTSes. > > The true problem how these SPMI bindings were created. Requiring SID > address in every child is clearly redundant and I think we do not follow > such approach anywhere else. > > Best regards, > Krzysztof >
On Thu Aug 31, 2023 at 1:54 PM CEST, Krzysztof Kozlowski wrote: > On 31/08/2023 13:33, Dmitry Baryshkov wrote: > > On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: > >> > >> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: > >>> On 30/08/2023 11:58, Luca Weiss wrote: > >>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses > >>>> on the SPMI bus. Use similar defines like the PMK8350 to make this > >>>> possible. > >>>> > >>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > >>>> --- > >>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > >>>> 1 file changed, 16 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>> index e8540c36bd99..3514de536baa 100644 > >>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>> @@ -7,6 +7,15 @@ > >>>> #include <dt-bindings/interrupt-controller/irq.h> > >>>> #include <dt-bindings/spmi/spmi.h> > >>>> > >>>> +/* This PMIC can be configured to be at different SIDs */ > >>>> +#ifndef PM7250B_SID > >>>> + #define PM7250B_SID 2 > >>>> +#endif > >>> > >>> Why do you send the same patch as v1, without any reference to previous > >>> discussions? > >>> > >>> You got here feedback already. > >>> > >>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ > >> > >> Hi Krzysztof, > >> > >> I did mention that original patch in the cover letter of this series. > >> I'm definitely aware of the discussion earlier this year there but also > >> tried to get an update lately if there's any update with no response. > > > > I think the overall consensus was that my proposal is too complicated > > for the DT files. > > I proposed to duplicate the entries. If you mean creating a pm7250b-8.dtsi with pm7250b copy-pasted but the SID changed from 2 & 3 to 8 & 9, I can do that if that's the way forward. If this was done, I'd also say then that pm7250b.dtsi should be renamed to e.g. pm7250b-2.dtsi since it's currently sitting on SID 2 & 3. > Do you keep QUP nodes in DTSI and customize per address? No. > > I definitely do not agree to these ifndef->define. Maybe using just > define would work (so drop ifndef->define), because this makes it > obvious and fail-safe if included in wrong place... except that it is > still not the define we expect. This is not the coding style present in > other DTSes. I really don't mind either way, I'd just like to have some way for now. > > The true problem how these SPMI bindings were created. Requiring SID > address in every child is clearly redundant and I think we do not follow > such approach anywhere else. Is this something that could be fixed long term? Especially since Qualcomm is reconfiguring PMICs on different addresses nowadays maybe there's more or a push to do this? Regards Luca > > Best regards, > Krzysztof
On Wed Aug 30, 2023 at 4:57 PM CEST, Doug Anderson wrote: > Hi, > > On Wed, Aug 30, 2023 at 7:43 AM Luca Weiss <luca.weiss@fairphone.com> wrote: > > > > On Wed Aug 30, 2023 at 4:30 PM CEST, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Aug 30, 2023 at 2:58 AM Luca Weiss <luca.weiss@fairphone.com> wrote: > > > > > > > > On some platforms like sc7280 on non-ChromeOS devices the core clock > > > > cannot be touched by Linux so we cannot provide it. Mark it as optional > > > > as accessing qfprom works without it. > > > > > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > > > --- > > > > drivers/nvmem/qfprom.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > Are you actually testing burning fuses from the OS, or are you just > > > using the nvmem in "read-only" mode? From comments in the bindings, if > > > you're trying to burn the fuses then the clock is required. If things > > > are in read-only mode then the clock isn't required. > > > > Hi Doug, > > > > I definitely don't plan on burning any fuses on this phone. Not even > > sure that's allowed by the TZ / boot stack. > > > > > > > > When I compare to the driver, it seems like the driver assumes that if > > > more than one memory region is provided then you must be supporting > > > burning fuses. The bindings agree that having 4 memory regions > > > specified means that the nvmem supports burning and 1 memory region > > > specified means read-only. The extra 3 memory regions in the nvmem are > > > all about fuse burning, I believe. > > > > > > So maybe the right fix here is to just change your dts to specify one > > > memory region? > > > > I got feedback from Konrad that this here would be the preferred > > approach compared to having a different dts for ChromeOS vs non-ChromeOS > > devices. I don't feel strongly to either, for me it's also okay to > > remove the extra memory regions and only have the main one used on > > regular qcom devices. > > > > Let me know what you think. > > I don't hate the idea of leaving the extra memory regions in the dts. > They do describe the hardware, after all, even if the main OS can't > actually access those memory regions. ...though the same could also be > said about the clock you've removed. Said another way: if you want to > fully describe the hardware then the dts should have the extra memory > regions and the clock. If you are OK w/ just describing the hardware > in the way that the OS has access to then the dts should not have the > extra memory regions and not have the clock. Does that sound right? Not sure which of those memory regions are actually accessible on this board, but honestly I don't even want to try accessing it. Blowing fuses is not my wish there ;) On downstream the node is just described like the following: qfprom: qfprom@780000 { compatible = "qcom,qfprom"; reg = <0x780000 0x7000>; ... }; So we have 0x780000 - 0x786fff here. In sc7280.dtsi we have the following: qfprom: efuse@784000 { compatible = "qcom,sc7280-qfprom", "qcom,qfprom"; reg = <0 0x00784000 0 0xa20>, <0 0x00780000 0 0xa20>, <0 0x00782000 0 0x120>, <0 0x00786000 0 0x1fff>; ... }; So I guess this: * 0x780000 - 0x780a1f * 0x782000 - 0x78211f * 0x784000 - 0x784a1f * 0x786000 - 0x787ffe So at least the last memory region seems to be partially out of range according to downstream. So after reading all of this I tried running this commmand on the phone and the phone reboots into 900e mode. $ cat /sys/devices/platform/soc@0/784000.efuse/qfprom0/nvmem I guess normally this should work? So if I interpret this correctly, the Linux driver thinks it can access more than it can/should. But also should probably try this command on another chipset to see if it works on any really? Regards Luca > > If somehow you do end up with something like your patch, though, > you're still missing a bit. Specifically, you don't want to "enable > writing" a few lines below if you didn't get the clock, right? > > -Doug
Hi, On Fri, Sep 1, 2023 at 7:54 AM Luca Weiss <luca.weiss@fairphone.com> wrote: > > > > > So maybe the right fix here is to just change your dts to specify one > > > > memory region? > > > > > > I got feedback from Konrad that this here would be the preferred > > > approach compared to having a different dts for ChromeOS vs non-ChromeOS > > > devices. I don't feel strongly to either, for me it's also okay to > > > remove the extra memory regions and only have the main one used on > > > regular qcom devices. > > > > > > Let me know what you think. > > > > I don't hate the idea of leaving the extra memory regions in the dts. > > They do describe the hardware, after all, even if the main OS can't > > actually access those memory regions. ...though the same could also be > > said about the clock you've removed. Said another way: if you want to > > fully describe the hardware then the dts should have the extra memory > > regions and the clock. If you are OK w/ just describing the hardware > > in the way that the OS has access to then the dts should not have the > > extra memory regions and not have the clock. Does that sound right? > > Not sure which of those memory regions are actually accessible on this > board, but honestly I don't even want to try accessing it. Blowing fuses > is not my wish there ;) > > On downstream the node is just described like the following: > > qfprom: qfprom@780000 { > compatible = "qcom,qfprom"; > reg = <0x780000 0x7000>; > ... > }; > > So we have 0x780000 - 0x786fff here. > > In sc7280.dtsi we have the following: > > qfprom: efuse@784000 { > compatible = "qcom,sc7280-qfprom", "qcom,qfprom"; > reg = <0 0x00784000 0 0xa20>, > <0 0x00780000 0 0xa20>, > <0 0x00782000 0 0x120>, > <0 0x00786000 0 0x1fff>; > ... > }; > > So I guess this: > * 0x780000 - 0x780a1f > * 0x782000 - 0x78211f > * 0x784000 - 0x784a1f > * 0x786000 - 0x787ffe > > So at least the last memory region seems to be partially out of range > according to downstream. From the other discussion, it sounds as if you _can_ leave the clock in the device tree and then use "clk_get_optional" here. IMO then, the right answer is to use "clk_get_optional" but then also modify the check below so that instead of: /* Only enable writing if we have SoC data. */ if (priv->soc_data) econfig.reg_write = qfprom_reg_write; It is: /* Only enable writing if we have SoC data and a valid clock */ if (priv->soc_data && priv->secclk) econfig.reg_write = qfprom_reg_write; Does that work for you? > So after reading all of this I tried running this commmand on the phone > and the phone reboots into 900e mode. > > $ cat /sys/devices/platform/soc@0/784000.efuse/qfprom0/nvmem > > I guess normally this should work? So if I interpret this correctly, the > Linux driver thinks it can access more than it can/should. But also > should probably try this command on another chipset to see if it works > on any really? Presumably your firmware needs a different "sc7280_qfprom_keepout". If that's true then I guess you'll have to undergo negotiations with the DT bindings folks and the nvmem maintainer to figure out how to specify that your firmware protects different things than the ChromeOS firmware? -Doug
On 8/30/2023 7:43 AM, Luca Weiss wrote: > On Wed Aug 30, 2023 at 4:30 PM CEST, Doug Anderson wrote: >> Hi, >> >> On Wed, Aug 30, 2023 at 2:58 AM Luca Weiss <luca.weiss@fairphone.com> wrote: >>> >>> On some platforms like sc7280 on non-ChromeOS devices the core clock >>> cannot be touched by Linux so we cannot provide it. Mark it as optional >>> as accessing qfprom works without it. >>> >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>> --- >>> drivers/nvmem/qfprom.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Are you actually testing burning fuses from the OS, or are you just >> using the nvmem in "read-only" mode? From comments in the bindings, if >> you're trying to burn the fuses then the clock is required. If things >> are in read-only mode then the clock isn't required. > > Hi Doug, > > I definitely don't plan on burning any fuses on this phone. Not even > sure that's allowed by the TZ / boot stack. > >> >> When I compare to the driver, it seems like the driver assumes that if >> more than one memory region is provided then you must be supporting >> burning fuses. The bindings agree that having 4 memory regions >> specified means that the nvmem supports burning and 1 memory region >> specified means read-only. The extra 3 memory regions in the nvmem are >> all about fuse burning, I believe. >> >> So maybe the right fix here is to just change your dts to specify one >> memory region? > > I got feedback from Konrad that this here would be the preferred > approach compared to having a different dts for ChromeOS vs non-ChromeOS > devices. I don't feel strongly to either, for me it's also okay to > remove the extra memory regions and only have the main one used on > regular qcom devices. > > Let me know what you think. I would prefer to re-use the sc7280 DT as well. Thank you for your patches. We plan to use your patches for platform on the same part.
On 1.09.2023 17:08, Doug Anderson wrote: > Hi, > > On Fri, Sep 1, 2023 at 7:54 AM Luca Weiss <luca.weiss@fairphone.com> wrote: >> >>>>> So maybe the right fix here is to just change your dts to specify one >>>>> memory region? >>>> >>>> I got feedback from Konrad that this here would be the preferred >>>> approach compared to having a different dts for ChromeOS vs non-ChromeOS >>>> devices. I don't feel strongly to either, for me it's also okay to >>>> remove the extra memory regions and only have the main one used on >>>> regular qcom devices. >>>> >>>> Let me know what you think. >>> >>> I don't hate the idea of leaving the extra memory regions in the dts. >>> They do describe the hardware, after all, even if the main OS can't >>> actually access those memory regions. ...though the same could also be >>> said about the clock you've removed. Said another way: if you want to >>> fully describe the hardware then the dts should have the extra memory >>> regions and the clock. If you are OK w/ just describing the hardware >>> in the way that the OS has access to then the dts should not have the >>> extra memory regions and not have the clock. Does that sound right? >> >> Not sure which of those memory regions are actually accessible on this >> board, but honestly I don't even want to try accessing it. Blowing fuses >> is not my wish there ;) >> >> On downstream the node is just described like the following: >> >> qfprom: qfprom@780000 { >> compatible = "qcom,qfprom"; >> reg = <0x780000 0x7000>; >> ... >> }; >> >> So we have 0x780000 - 0x786fff here. >> >> In sc7280.dtsi we have the following: >> >> qfprom: efuse@784000 { >> compatible = "qcom,sc7280-qfprom", "qcom,qfprom"; >> reg = <0 0x00784000 0 0xa20>, >> <0 0x00780000 0 0xa20>, >> <0 0x00782000 0 0x120>, >> <0 0x00786000 0 0x1fff>; >> ... >> }; >> >> So I guess this: >> * 0x780000 - 0x780a1f >> * 0x782000 - 0x78211f >> * 0x784000 - 0x784a1f >> * 0x786000 - 0x787ffe >> >> So at least the last memory region seems to be partially out of range >> according to downstream. > > From the other discussion, it sounds as if you _can_ leave the clock > in the device tree and then use "clk_get_optional" here. IMO then, the > right answer is to use "clk_get_optional" but then also modify the > check below so that instead of: > > /* Only enable writing if we have SoC data. */ > if (priv->soc_data) > econfig.reg_write = qfprom_reg_write; > > It is: > > /* Only enable writing if we have SoC data and a valid clock */ > if (priv->soc_data && priv->secclk) > econfig.reg_write = qfprom_reg_write; > > > Does that work for you? > > >> So after reading all of this I tried running this commmand on the phone >> and the phone reboots into 900e mode. >> >> $ cat /sys/devices/platform/soc@0/784000.efuse/qfprom0/nvmem >> >> I guess normally this should work? So if I interpret this correctly, the >> Linux driver thinks it can access more than it can/should. But also >> should probably try this command on another chipset to see if it works >> on any really? > > Presumably your firmware needs a different "sc7280_qfprom_keepout". If > that's true then I guess you'll have to undergo negotiations with the > DT bindings folks and the nvmem maintainer to figure out how to > specify that your firmware protects different things than the ChromeOS > firmware? Luca, if you feel like wasting some time, you can probably bruteforce this. I assume this keepout thing could be expanded in a generic way and made into a dt property. Other than that, I think it'd be fine to skip that for now, as it sounds like it's functional so long as you don't intentionally access forbidden regs. Konrad
On Fri Sep 1, 2023 at 5:08 PM CEST, Doug Anderson wrote: > Hi, > > On Fri, Sep 1, 2023 at 7:54 AM Luca Weiss <luca.weiss@fairphone.com> wrote: > > > > > > > So maybe the right fix here is to just change your dts to specify one > > > > > memory region? > > > > > > > > I got feedback from Konrad that this here would be the preferred > > > > approach compared to having a different dts for ChromeOS vs non-ChromeOS > > > > devices. I don't feel strongly to either, for me it's also okay to > > > > remove the extra memory regions and only have the main one used on > > > > regular qcom devices. > > > > > > > > Let me know what you think. > > > > > > I don't hate the idea of leaving the extra memory regions in the dts. > > > They do describe the hardware, after all, even if the main OS can't > > > actually access those memory regions. ...though the same could also be > > > said about the clock you've removed. Said another way: if you want to > > > fully describe the hardware then the dts should have the extra memory > > > regions and the clock. If you are OK w/ just describing the hardware > > > in the way that the OS has access to then the dts should not have the > > > extra memory regions and not have the clock. Does that sound right? > > > > Not sure which of those memory regions are actually accessible on this > > board, but honestly I don't even want to try accessing it. Blowing fuses > > is not my wish there ;) > > > > On downstream the node is just described like the following: > > > > qfprom: qfprom@780000 { > > compatible = "qcom,qfprom"; > > reg = <0x780000 0x7000>; > > ... > > }; > > > > So we have 0x780000 - 0x786fff here. > > > > In sc7280.dtsi we have the following: > > > > qfprom: efuse@784000 { > > compatible = "qcom,sc7280-qfprom", "qcom,qfprom"; > > reg = <0 0x00784000 0 0xa20>, > > <0 0x00780000 0 0xa20>, > > <0 0x00782000 0 0x120>, > > <0 0x00786000 0 0x1fff>; > > ... > > }; > > > > So I guess this: > > * 0x780000 - 0x780a1f > > * 0x782000 - 0x78211f > > * 0x784000 - 0x784a1f > > * 0x786000 - 0x787ffe > > > > So at least the last memory region seems to be partially out of range > > according to downstream. > > From the other discussion, it sounds as if you _can_ leave the clock > in the device tree and then use "clk_get_optional" here. IMO then, the > right answer is to use "clk_get_optional" but then also modify the > check below so that instead of: > > /* Only enable writing if we have SoC data. */ > if (priv->soc_data) > econfig.reg_write = qfprom_reg_write; > > It is: > > /* Only enable writing if we have SoC data and a valid clock */ > if (priv->soc_data && priv->secclk) > econfig.reg_write = qfprom_reg_write; > > > Does that work for you? Thanks Doug, this feels like a good solution. I'll update v2 with that. > > > > So after reading all of this I tried running this commmand on the phone > > and the phone reboots into 900e mode. > > > > $ cat /sys/devices/platform/soc@0/784000.efuse/qfprom0/nvmem > > > > I guess normally this should work? So if I interpret this correctly, the > > Linux driver thinks it can access more than it can/should. But also > > should probably try this command on another chipset to see if it works > > on any really? > > Presumably your firmware needs a different "sc7280_qfprom_keepout". If > that's true then I guess you'll have to undergo negotiations with the > DT bindings folks and the nvmem maintainer to figure out how to > specify that your firmware protects different things than the ChromeOS > firmware? Right. But based on Konrad's reply here, I think I'll skip this for now since generally qfprom seems to be fine with the current data, at least for the purposes it's used for on my device. Regards Luca > > > -Doug
On Thu Aug 31, 2023 at 2:27 PM CEST, Dmitry Baryshkov wrote: > On Thu, 31 Aug 2023 at 14:54, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 31/08/2023 13:33, Dmitry Baryshkov wrote: > > > On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: > > >> > > >> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: > > >>> On 30/08/2023 11:58, Luca Weiss wrote: > > >>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses > > >>>> on the SPMI bus. Use similar defines like the PMK8350 to make this > > >>>> possible. > > >>>> > > >>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > >>>> --- > > >>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > > >>>> 1 file changed, 16 insertions(+), 7 deletions(-) > > >>>> > > >>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > >>>> index e8540c36bd99..3514de536baa 100644 > > >>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > >>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > >>>> @@ -7,6 +7,15 @@ > > >>>> #include <dt-bindings/interrupt-controller/irq.h> > > >>>> #include <dt-bindings/spmi/spmi.h> > > >>>> > > >>>> +/* This PMIC can be configured to be at different SIDs */ > > >>>> +#ifndef PM7250B_SID > > >>>> + #define PM7250B_SID 2 > > >>>> +#endif > > >>> > > >>> Why do you send the same patch as v1, without any reference to previous > > >>> discussions? > > >>> > > >>> You got here feedback already. > > >>> > > >>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ > > >> > > >> Hi Krzysztof, > > >> > > >> I did mention that original patch in the cover letter of this series. > > >> I'm definitely aware of the discussion earlier this year there but also > > >> tried to get an update lately if there's any update with no response. > > > > > > I think the overall consensus was that my proposal is too complicated > > > for the DT files. > > > > I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and > > customize per address? No. > > At the same time, we do keep SoC files separate from the board files. > Yes, I'm slightly exaggerating here. > > I think that for PMIC files it makes sense to extract common parts if > that eases reuse of the common parts. Hi all, what can I do for v2 now? 1. Keep this patch as-is, and keep pm7250b in device dts. 2. Drop pm7250b patch and drop from device dts, until _someone_ figures out a solution talking to the PMIC on different SID. 3. Something else like copy-pasting pm7250b.dtsi to pm7250-8.dtsi and changing the SID there, and using that in device dts. Please let me know what to do. Regards Luca > > > > > I definitely do not agree to these ifndef->define. Maybe using just > > define would work (so drop ifndef->define), because this makes it > > obvious and fail-safe if included in wrong place... except that it is > > still not the define we expect. This is not the coding style present in > > other DTSes. > > > > The true problem how these SPMI bindings were created. Requiring SID > > address in every child is clearly redundant and I think we do not follow > > such approach anywhere else. > > > > Best regards, > > Krzysztof > >
On Tue Sep 5, 2023 at 10:30 AM CEST, Luca Weiss wrote: > On Thu Aug 31, 2023 at 2:27 PM CEST, Dmitry Baryshkov wrote: > > On Thu, 31 Aug 2023 at 14:54, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > On 31/08/2023 13:33, Dmitry Baryshkov wrote: > > > > On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: > > > >> > > > >> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: > > > >>> On 30/08/2023 11:58, Luca Weiss wrote: > > > >>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses > > > >>>> on the SPMI bus. Use similar defines like the PMK8350 to make this > > > >>>> possible. > > > >>>> > > > >>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > > >>>> --- > > > >>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > > > >>>> 1 file changed, 16 insertions(+), 7 deletions(-) > > > >>>> > > > >>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > > >>>> index e8540c36bd99..3514de536baa 100644 > > > >>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > > >>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > > > >>>> @@ -7,6 +7,15 @@ > > > >>>> #include <dt-bindings/interrupt-controller/irq.h> > > > >>>> #include <dt-bindings/spmi/spmi.h> > > > >>>> > > > >>>> +/* This PMIC can be configured to be at different SIDs */ > > > >>>> +#ifndef PM7250B_SID > > > >>>> + #define PM7250B_SID 2 > > > >>>> +#endif > > > >>> > > > >>> Why do you send the same patch as v1, without any reference to previous > > > >>> discussions? > > > >>> > > > >>> You got here feedback already. > > > >>> > > > >>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ > > > >> > > > >> Hi Krzysztof, > > > >> > > > >> I did mention that original patch in the cover letter of this series. > > > >> I'm definitely aware of the discussion earlier this year there but also > > > >> tried to get an update lately if there's any update with no response. > > > > > > > > I think the overall consensus was that my proposal is too complicated > > > > for the DT files. > > > > > > I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and > > > customize per address? No. > > > > At the same time, we do keep SoC files separate from the board files. > > Yes, I'm slightly exaggerating here. > > > > I think that for PMIC files it makes sense to extract common parts if > > that eases reuse of the common parts. > > Hi all, > > what can I do for v2 now? > > 1. Keep this patch as-is, and keep pm7250b in device dts. > > 2. Drop pm7250b patch and drop from device dts, until _someone_ figures > out a solution talking to the PMIC on different SID. > > 3. Something else like copy-pasting pm7250b.dtsi to pm7250-8.dtsi and > changing the SID there, and using that in device dts. > > Please let me know what to do. > > Regards > Luca Hi, if there's no feedback I'll keep this patch in v2 of this series and we can continue to discuss there (if necessary). Regards Luca > > > > > > > > > I definitely do not agree to these ifndef->define. Maybe using just > > > define would work (so drop ifndef->define), because this makes it > > > obvious and fail-safe if included in wrong place... except that it is > > > still not the define we expect. This is not the coding style present in > > > other DTSes. > > > > > > The true problem how these SPMI bindings were created. Requiring SID > > > address in every child is clearly redundant and I think we do not follow > > > such approach anywhere else. > > > > > > Best regards, > > > Krzysztof > > >
On 11/09/2023 10:34, Luca Weiss wrote: > On Tue Sep 5, 2023 at 10:30 AM CEST, Luca Weiss wrote: >> On Thu Aug 31, 2023 at 2:27 PM CEST, Dmitry Baryshkov wrote: >>> On Thu, 31 Aug 2023 at 14:54, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 31/08/2023 13:33, Dmitry Baryshkov wrote: >>>>> On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: >>>>>> >>>>>> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: >>>>>>> On 30/08/2023 11:58, Luca Weiss wrote: >>>>>>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses >>>>>>>> on the SPMI bus. Use similar defines like the PMK8350 to make this >>>>>>>> possible. >>>>>>>> >>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>>>>>>> --- >>>>>>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- >>>>>>>> 1 file changed, 16 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>> index e8540c36bd99..3514de536baa 100644 >>>>>>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>> @@ -7,6 +7,15 @@ >>>>>>>> #include <dt-bindings/interrupt-controller/irq.h> >>>>>>>> #include <dt-bindings/spmi/spmi.h> >>>>>>>> >>>>>>>> +/* This PMIC can be configured to be at different SIDs */ >>>>>>>> +#ifndef PM7250B_SID >>>>>>>> + #define PM7250B_SID 2 >>>>>>>> +#endif >>>>>>> >>>>>>> Why do you send the same patch as v1, without any reference to previous >>>>>>> discussions? >>>>>>> >>>>>>> You got here feedback already. >>>>>>> >>>>>>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ >>>>>> >>>>>> Hi Krzysztof, >>>>>> >>>>>> I did mention that original patch in the cover letter of this series. >>>>>> I'm definitely aware of the discussion earlier this year there but also >>>>>> tried to get an update lately if there's any update with no response. >>>>> >>>>> I think the overall consensus was that my proposal is too complicated >>>>> for the DT files. >>>> >>>> I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and >>>> customize per address? No. >>> >>> At the same time, we do keep SoC files separate from the board files. >>> Yes, I'm slightly exaggerating here. >>> >>> I think that for PMIC files it makes sense to extract common parts if >>> that eases reuse of the common parts. >> >> Hi all, >> >> what can I do for v2 now? >> >> 1. Keep this patch as-is, and keep pm7250b in device dts. This was NAKed by me. What Qualcomm SoC maintainers decide (or not decide) about other options, should not cause the wrong solution to be re-posted... >> >> 2. Drop pm7250b patch and drop from device dts, until _someone_ figures >> out a solution talking to the PMIC on different SID. >> >> 3. Something else like copy-pasting pm7250b.dtsi to pm7250-8.dtsi and >> changing the SID there, and using that in device dts. >> >> Please let me know what to do. >> >> Regards >> Luca > > Hi, > > if there's no feedback I'll keep this patch in v2 of this series and we > can continue to discuss there (if necessary). Sorry, I still do not agree and there were no arguments convincing me to change the mind. I gave you the solution from my perspective. Why do you decided to ignore it and send it as is? Best regards, Krzysztof
On Mon Sep 11, 2023 at 11:44 AM CEST, Krzysztof Kozlowski wrote: > On 11/09/2023 10:34, Luca Weiss wrote: > > On Tue Sep 5, 2023 at 10:30 AM CEST, Luca Weiss wrote: > >> On Thu Aug 31, 2023 at 2:27 PM CEST, Dmitry Baryshkov wrote: > >>> On Thu, 31 Aug 2023 at 14:54, Krzysztof Kozlowski > >>> <krzysztof.kozlowski@linaro.org> wrote: > >>>> > >>>> On 31/08/2023 13:33, Dmitry Baryshkov wrote: > >>>>> On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: > >>>>>> > >>>>>> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: > >>>>>>> On 30/08/2023 11:58, Luca Weiss wrote: > >>>>>>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses > >>>>>>>> on the SPMI bus. Use similar defines like the PMK8350 to make this > >>>>>>>> possible. > >>>>>>>> > >>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > >>>>>>>> --- > >>>>>>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- > >>>>>>>> 1 file changed, 16 insertions(+), 7 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>>>>>> index e8540c36bd99..3514de536baa 100644 > >>>>>>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>>>>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi > >>>>>>>> @@ -7,6 +7,15 @@ > >>>>>>>> #include <dt-bindings/interrupt-controller/irq.h> > >>>>>>>> #include <dt-bindings/spmi/spmi.h> > >>>>>>>> > >>>>>>>> +/* This PMIC can be configured to be at different SIDs */ > >>>>>>>> +#ifndef PM7250B_SID > >>>>>>>> + #define PM7250B_SID 2 > >>>>>>>> +#endif > >>>>>>> > >>>>>>> Why do you send the same patch as v1, without any reference to previous > >>>>>>> discussions? > >>>>>>> > >>>>>>> You got here feedback already. > >>>>>>> > >>>>>>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ > >>>>>> > >>>>>> Hi Krzysztof, > >>>>>> > >>>>>> I did mention that original patch in the cover letter of this series. > >>>>>> I'm definitely aware of the discussion earlier this year there but also > >>>>>> tried to get an update lately if there's any update with no response. > >>>>> > >>>>> I think the overall consensus was that my proposal is too complicated > >>>>> for the DT files. > >>>> > >>>> I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and > >>>> customize per address? No. > >>> > >>> At the same time, we do keep SoC files separate from the board files. > >>> Yes, I'm slightly exaggerating here. > >>> > >>> I think that for PMIC files it makes sense to extract common parts if > >>> that eases reuse of the common parts. > >> > >> Hi all, > >> > >> what can I do for v2 now? > >> > >> 1. Keep this patch as-is, and keep pm7250b in device dts. > > This was NAKed by me. What Qualcomm SoC maintainers decide (or not > decide) about other options, should not cause the wrong solution to be > re-posted... > > >> > >> 2. Drop pm7250b patch and drop from device dts, until _someone_ figures > >> out a solution talking to the PMIC on different SID. > >> > >> 3. Something else like copy-pasting pm7250b.dtsi to pm7250-8.dtsi and > >> changing the SID there, and using that in device dts. @Konrad, @Bjorn: Can you give any feedback here what's preferable? Otherwise I'm just blocked on this series. > >> > >> Please let me know what to do. > >> > >> Regards > >> Luca > > > > Hi, > > > > if there's no feedback I'll keep this patch in v2 of this series and we > > can continue to discuss there (if necessary). > > Sorry, I still do not agree and there were no arguments convincing me to > change the mind. > > I gave you the solution from my perspective. Why do you decided to > ignore it and send it as is? I get it that you are not final decider for qcom dts changes but it's quite difficult for someone sending patches to not get any feedback what other change to replace this is appropriate. I doubt it's a good idea to just implement some random pm7250-8.dtsi or whatever to potentially immediately get a response that that way is also bad. That's why I'm trying to get some info before working on something and sending it. Hopefully Bjorn or Konrad can add their thoughts above. Also I don't recall me ever reading a "solution" from your side but maybe I need to dig through the old emails again. Regards Luca > > > Best regards, > Krzysztof
On 11/09/2023 11:59, Luca Weiss wrote: > On Mon Sep 11, 2023 at 11:44 AM CEST, Krzysztof Kozlowski wrote: >> On 11/09/2023 10:34, Luca Weiss wrote: >>> On Tue Sep 5, 2023 at 10:30 AM CEST, Luca Weiss wrote: >>>> On Thu Aug 31, 2023 at 2:27 PM CEST, Dmitry Baryshkov wrote: >>>>> On Thu, 31 Aug 2023 at 14:54, Krzysztof Kozlowski >>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>> >>>>>> On 31/08/2023 13:33, Dmitry Baryshkov wrote: >>>>>>> On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: >>>>>>>> >>>>>>>> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: >>>>>>>>> On 30/08/2023 11:58, Luca Weiss wrote: >>>>>>>>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses >>>>>>>>>> on the SPMI bus. Use similar defines like the PMK8350 to make this >>>>>>>>>> possible. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>>>>>>>>> --- >>>>>>>>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- >>>>>>>>>> 1 file changed, 16 insertions(+), 7 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>>>> index e8540c36bd99..3514de536baa 100644 >>>>>>>>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>>>> @@ -7,6 +7,15 @@ >>>>>>>>>> #include <dt-bindings/interrupt-controller/irq.h> >>>>>>>>>> #include <dt-bindings/spmi/spmi.h> >>>>>>>>>> >>>>>>>>>> +/* This PMIC can be configured to be at different SIDs */ >>>>>>>>>> +#ifndef PM7250B_SID >>>>>>>>>> + #define PM7250B_SID 2 >>>>>>>>>> +#endif >>>>>>>>> >>>>>>>>> Why do you send the same patch as v1, without any reference to previous >>>>>>>>> discussions? >>>>>>>>> >>>>>>>>> You got here feedback already. >>>>>>>>> >>>>>>>>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ >>>>>>>> >>>>>>>> Hi Krzysztof, >>>>>>>> >>>>>>>> I did mention that original patch in the cover letter of this series. >>>>>>>> I'm definitely aware of the discussion earlier this year there but also >>>>>>>> tried to get an update lately if there's any update with no response. >>>>>>> >>>>>>> I think the overall consensus was that my proposal is too complicated >>>>>>> for the DT files. >>>>>> >>>>>> I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and >>>>>> customize per address? No. >>>>> >>>>> At the same time, we do keep SoC files separate from the board files. >>>>> Yes, I'm slightly exaggerating here. >>>>> >>>>> I think that for PMIC files it makes sense to extract common parts if >>>>> that eases reuse of the common parts. >>>> >>>> Hi all, >>>> >>>> what can I do for v2 now? >>>> >>>> 1. Keep this patch as-is, and keep pm7250b in device dts. >> >> This was NAKed by me. What Qualcomm SoC maintainers decide (or not >> decide) about other options, should not cause the wrong solution to be >> re-posted... >> >>>> >>>> 2. Drop pm7250b patch and drop from device dts, until _someone_ figures >>>> out a solution talking to the PMIC on different SID. >>>> >>>> 3. Something else like copy-pasting pm7250b.dtsi to pm7250-8.dtsi and >>>> changing the SID there, and using that in device dts. > > @Konrad, @Bjorn: Can you give any feedback here what's preferable? > Otherwise I'm just blocked on this series. > >>>> >>>> Please let me know what to do. >>>> >>>> Regards >>>> Luca >>> >>> Hi, >>> >>> if there's no feedback I'll keep this patch in v2 of this series and we >>> can continue to discuss there (if necessary). >> >> Sorry, I still do not agree and there were no arguments convincing me to >> change the mind. >> >> I gave you the solution from my perspective. Why do you decided to >> ignore it and send it as is? > > I get it that you are not final decider for qcom dts changes but it's > quite difficult for someone sending patches to not get any feedback what > other change to replace this is appropriate. I doubt it's a good idea to > just implement some random pm7250-8.dtsi or whatever to potentially > immediately get a response that that way is also bad. > > That's why I'm trying to get some info before working on something and > sending it. Hopefully Bjorn or Konrad can add their thoughts above. I understand, and it is frustrating. If such case happens the solution in upstream is not sending the same NAKed version but send something else. > > Also I don't recall me ever reading a "solution" from your side but > maybe I need to dig through the old emails again. Here: "I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and customize per address? No." Dmitry responded that having PMICs extracted help re-using. He is right. But here you hit the limit of such re-usage. Best regards, Krzysztof
On 11.09.2023 13:15, Krzysztof Kozlowski wrote: > On 11/09/2023 11:59, Luca Weiss wrote: >> On Mon Sep 11, 2023 at 11:44 AM CEST, Krzysztof Kozlowski wrote: >>> On 11/09/2023 10:34, Luca Weiss wrote: >>>> On Tue Sep 5, 2023 at 10:30 AM CEST, Luca Weiss wrote: >>>>> On Thu Aug 31, 2023 at 2:27 PM CEST, Dmitry Baryshkov wrote: >>>>>> On Thu, 31 Aug 2023 at 14:54, Krzysztof Kozlowski >>>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>>> >>>>>>> On 31/08/2023 13:33, Dmitry Baryshkov wrote: >>>>>>>> On Thu, 31 Aug 2023 at 13:13, Luca Weiss <luca.weiss@fairphone.com> wrote: >>>>>>>>> >>>>>>>>> On Wed Aug 30, 2023 at 12:06 PM CEST, Krzysztof Kozlowski wrote: >>>>>>>>>> On 30/08/2023 11:58, Luca Weiss wrote: >>>>>>>>>>> Like other Qualcomm PMICs the PM7250B can be used on different addresses >>>>>>>>>>> on the SPMI bus. Use similar defines like the PMK8350 to make this >>>>>>>>>>> possible. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>>>>>>>>>> --- >>>>>>>>>>> arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 ++++++++++++++++------- >>>>>>>>>>> 1 file changed, 16 insertions(+), 7 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/pm7250b.dtsi b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>>>>> index e8540c36bd99..3514de536baa 100644 >>>>>>>>>>> --- a/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/pm7250b.dtsi >>>>>>>>>>> @@ -7,6 +7,15 @@ >>>>>>>>>>> #include <dt-bindings/interrupt-controller/irq.h> >>>>>>>>>>> #include <dt-bindings/spmi/spmi.h> >>>>>>>>>>> >>>>>>>>>>> +/* This PMIC can be configured to be at different SIDs */ >>>>>>>>>>> +#ifndef PM7250B_SID >>>>>>>>>>> + #define PM7250B_SID 2 >>>>>>>>>>> +#endif >>>>>>>>>> >>>>>>>>>> Why do you send the same patch as v1, without any reference to previous >>>>>>>>>> discussions? >>>>>>>>>> >>>>>>>>>> You got here feedback already. >>>>>>>>>> >>>>>>>>>> https://lore.kernel.org/linux-arm-msm/f52524da-719b-790f-ad2c-0c3f313d9fe9@linaro.org/ >>>>>>>>> >>>>>>>>> Hi Krzysztof, >>>>>>>>> >>>>>>>>> I did mention that original patch in the cover letter of this series. >>>>>>>>> I'm definitely aware of the discussion earlier this year there but also >>>>>>>>> tried to get an update lately if there's any update with no response. >>>>>>>> >>>>>>>> I think the overall consensus was that my proposal is too complicated >>>>>>>> for the DT files. >>>>>>> >>>>>>> I proposed to duplicate the entries. Do you keep QUP nodes in DTSI and >>>>>>> customize per address? No. >>>>>> >>>>>> At the same time, we do keep SoC files separate from the board files. >>>>>> Yes, I'm slightly exaggerating here. >>>>>> >>>>>> I think that for PMIC files it makes sense to extract common parts if >>>>>> that eases reuse of the common parts. >>>>> >>>>> Hi all, >>>>> >>>>> what can I do for v2 now? >>>>> >>>>> 1. Keep this patch as-is, and keep pm7250b in device dts. >>> >>> This was NAKed by me. What Qualcomm SoC maintainers decide (or not >>> decide) about other options, should not cause the wrong solution to be >>> re-posted... >>> >>>>> >>>>> 2. Drop pm7250b patch and drop from device dts, until _someone_ figures >>>>> out a solution talking to the PMIC on different SID. >>>>> >>>>> 3. Something else like copy-pasting pm7250b.dtsi to pm7250-8.dtsi and >>>>> changing the SID there, and using that in device dts. >> >> @Konrad, @Bjorn: Can you give any feedback here what's preferable? >> Otherwise I'm just blocked on this series. I'm sure Krzysztof will disagree, but all of the solutions (which are either duplicate the dt, add ifdefs or skip adding this pmic) are equally band-aid-class.. A bright future where this PMIC thing is handled on the driver side that will hopefully come soon(tm) should resolve such problems.. From my side, ifdef is the least burdensome, even if ugly.. Konrad
On Wed, 30 Aug 2023 11:58:25 +0200, Luca Weiss wrote: > Add support to boot up mainline kernel on the QCM6490-based Fairphone 5 > smartphone. > > These patches only cover a part of the functionality brought up on > mainline so far, with the rest needing larger dts and driver changes or > depend on patches that are not yet merged. I will work on sending those > once these base patches here have settled. > > [...] Applied, thanks! [07/11] dt-bindings: arm: qcom,ids: Add SoC ID for QCM6490 commit: ccfb4d8b606302d857a03ea29039e21029311335 [08/11] soc: qcom: socinfo: Add SoC ID for QCM6490 commit: 59872d59d164ec67f295d6f96fe818b92973ee40 Best regards,
Add support to boot up mainline kernel on the QCM6490-based Fairphone 5 smartphone. These patches only cover a part of the functionality brought up on mainline so far, with the rest needing larger dts and driver changes or depend on patches that are not yet merged. I will work on sending those once these base patches here have settled. Since QCM6490, like SC7280 are 'yupik' in the vendor-provided kernel, we can base the dts on it and leverage existing support. Though current sc7280 support mostly assumes ChromeOS devices which have a different TrustZone setup, so we need to move some ChromeOS-specific bits to the sc7280-chrome-common.dtsi file to make it boot on a standard TZ board. Depends on (just for the #include in sc7280.dtsi): https://lore.kernel.org/linux-arm-msm/20230818-qcom-vmid-defines-v1-1-45b610c96b13@fairphone.com/ The pm7250b patch has been picked up from this series: https://lore.kernel.org/linux-arm-msm/20230407-pm7250b-sid-v1-2-fc648478cc25@fairphone.com/ Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> --- Luca Weiss (11): arm64: dts: qcom: sc7280: Mark some nodes as 'reserved' nvmem: qfprom: Mark core clk as optional arm64: dts: qcom: sc7280: Move qfprom clock to chrome-common arm64: dts: qcom: pm7250b: make SID configurable arm64: dts: qcom: pm8350c: Add flash led node dt-bindings: pinctrl: qcom,sc7280: Allow gpio-reserved-ranges dt-bindings: arm: qcom,ids: Add SoC ID for QCM6490 soc: qcom: socinfo: Add SoC ID for QCM6490 cpufreq: Add QCM6490 to cpufreq-dt-platdev blocklist dt-bindings: arm: qcom: Add QCM6490 Fairphone 5 arm64: dts: qcom: qcm6490: Add device-tree for Fairphone 5 Documentation/devicetree/bindings/arm/qcom.yaml | 6 + .../bindings/pinctrl/qcom,sc7280-pinctrl.yaml | 4 + arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/pm7250b.dtsi | 23 +- arch/arm64/boot/dts/qcom/pm8350c.dtsi | 6 + arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts | 659 +++++++++++++++++++++ arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 17 + arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 +- drivers/cpufreq/cpufreq-dt-platdev.c | 1 + drivers/nvmem/qfprom.c | 2 +- drivers/soc/qcom/socinfo.c | 1 + include/dt-bindings/arm/qcom,ids.h | 1 + 12 files changed, 717 insertions(+), 11 deletions(-) --- base-commit: 0255bba921438ea1e45d3f0873c3e8c5a1e03876 change-id: 20230818-fp5-initial-b6c8210ba9c8 Best regards,