| Message ID | 20230830-fp5-initial-v1-2-5a954519bbad@fairphone.com |
|---|---|
| State | New |
| 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
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
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. Regards Luca > > -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 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.
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
diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c index 14814cba2dd6..739dc502b39e 100644 --- a/drivers/nvmem/qfprom.c +++ b/drivers/nvmem/qfprom.c @@ -423,7 +423,7 @@ static int qfprom_probe(struct platform_device *pdev) if (IS_ERR(priv->vcc)) return PTR_ERR(priv->vcc); - priv->secclk = devm_clk_get(dev, "core"); + priv->secclk = devm_clk_get_optional(dev, "core"); if (IS_ERR(priv->secclk)) return dev_err_probe(dev, PTR_ERR(priv->secclk), "Error getting clock\n");
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(-)