| Message ID | 20250502-wdt_reset_reason-v3-4-b2dc7ace38ca@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support to read the restart reason from IMEM | expand |
On 02/05/2025 15:17, Kathiravan Thirumoorthy wrote: > > +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, > + const struct qcom_wdt_match_data *data) > +{ > + struct regmap *imem; > + unsigned int val; > + int ret; > + > + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); And how are you handling proper probe ordering? Use phandles and define this as an ABI. Best regards, Krzysztof
On 5/2/2025 7:03 PM, Guenter Roeck wrote: >> static int qcom_wdt_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -273,8 +304,13 @@ static int qcom_wdt_probe(struct platform_device >> *pdev) >> wdt->wdd.parent = dev; >> wdt->layout = data->offset; >> - if (readl(wdt_addr(wdt, WDT_STS)) & 1) >> - wdt->wdd.bootstatus = WDIOF_CARDRESET; >> + ret = qcom_wdt_get_restart_reason(wdt, data); >> + if (ret == -ENODEV) { >> + if (readl(wdt_addr(wdt, WDT_STS)) & 1) >> + wdt->wdd.bootstatus = WDIOF_CARDRESET; >> + } else if (ret) { >> + return ret; >> + } > > Seems odd to me that there is now a function > qcom_wdt_get_restart_reason() > but it doesn't handle all means to get the restart reason. Maybe I > missed it, > but what is the reason for that ? Why not move reading WDT_STS into > qcom_wdt_get_restart_reason() as well ? No specific reason as such. I was little hesitant use "goto" statements and such as. So I thought this would be little cleaner approach. Please let me know if I have consolidate everything under qcom_wdt_get_restart_reason(). > > Guenter > > >> /* >> * If 'timeout-sec' unspecified in devicetree, assume a 30 second
On 5/2/2025 8:24 PM, Dmitry Baryshkov wrote: >> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >> + const struct qcom_wdt_match_data *data) >> +{ >> + struct regmap *imem; >> + unsigned int val; >> + int ret; >> + >> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >> + if (IS_ERR(imem)) >> + return PTR_ERR(imem); > Why? Just pass the syscon directly via DT. Ack. As replied to Konrad, will rework this.
On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: > > On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>> + const struct qcom_wdt_match_data *data) >>> +{ >>> + struct regmap *imem; >>> + unsigned int val; >>> + int ret; >>> + >>> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g. >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi >> >> That way all platform specifics will live in the DT, requiring no >> hardcode-y driver changes on similar platforms > > > Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value. > > I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below > > imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>; > > and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required. Let's call the property qcom,restart-reason and only pass the register value Because we may have any number of crazy combinations of various restart reasons, we can go two paths: 1. promise really really really hard we won't be too crazy with the number of possible values and put them in the driver 2. go all out on DT properties (such as `bootstatus-overheat`, `bootstatus-fanfault` etc. I'd much prefer to go with 1 really.. If we used nvmem, we could have a map of cell names to restart reasons, but we've already established IMEM is volatile and we shouldn't mess up the convention just because that subsystem has nicer APIs.. Unless we rename the subsystem to `fuses`, `magic-values` or something.. +Srini? :P Konrad
On 5/3/2025 3:53 AM, Konrad Dybcio wrote: > On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: >> On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>>> + const struct qcom_wdt_match_data *data) >>>> +{ >>>> + struct regmap *imem; >>>> + unsigned int val; >>>> + int ret; >>>> + >>>> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g. >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi >>> >>> That way all platform specifics will live in the DT, requiring no >>> hardcode-y driver changes on similar platforms >> >> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value. >> >> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below >> >> imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>; >> >> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required. > Let's call the property qcom,restart-reason and only pass the register value > > Because we may have any number of crazy combinations of various restart > reasons, we can go two paths: > > 1. promise really really really hard we won't be too crazy with the number > of possible values and put them in the driver > 2. go all out on DT properties (such as `bootstatus-overheat`, > `bootstatus-fanfault` etc. Thanks Konrad for the suggestions and the offline discussions. @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons. Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this. > > I'd much prefer to go with 1 really.. If we used nvmem, we could have a map > of cell names to restart reasons, but we've already established IMEM is > volatile and we shouldn't mess up the convention just because that > subsystem has nicer APIs.. > > Unless we rename the subsystem to `fuses`, `magic-values` or something.. > +Srini? :P > > Konrad
On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote: > > On 5/3/2025 3:53 AM, Konrad Dybcio wrote: >> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: >>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>>>> + const struct qcom_wdt_match_data *data) >>>>> +{ >>>>> + struct regmap *imem; >>>>> + unsigned int val; >>>>> + int ret; >>>>> + >>>>> + imem = >>>>> syscon_regmap_lookup_by_compatible(data->imem_compatible); >>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see >>>> e.g. >>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in >>>> x1e80100.dtsi >>>> >>>> That way all platform specifics will live in the DT, requiring no >>>> hardcode-y driver changes on similar platforms >>> >>> Thanks. I thought about this API but it didn't strike that I can use >>> the args to fetch and match the value. >>> >>> I need a suggestion here. There is a plan to extend this feature to >>> other IPQ targets and also support WDIOF_POWERUNDER and >>> WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support >>> and for other IPQ platforms, we are exploring how to integrate >>> WDIOF_OVERHEAT. In any case, can I define the DT entry like below >>> >>> imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power >>> Under value> <Overheat value>>; >>> >>> and store these in values args[1], args[2] and args[3] respectively >>> and use it for manipulation? If any of the platform doesn't support >>> all 3, I can update the bindings and define the number of args as >>> required. >> Let's call the property qcom,restart-reason and only pass the >> register value >> >> Because we may have any number of crazy combinations of various restart >> reasons, we can go two paths: >> >> 1. promise really really really hard we won't be too crazy with the >> number >> of possible values and put them in the driver >> 2. go all out on DT properties (such as `bootstatus-overheat`, >> `bootstatus-fanfault` etc. > > > Thanks Konrad for the suggestions and the offline discussions. > > @Guenter, I need a suggestion here. Currently as part of this series, > we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, > WDIOF_OVERHEAT reasons. > > Once this is done, we do have the custom reason codes like Kernel > Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and > few many. Is it okay to expose these values also via the bootstatus > sysFS by extending the current list of reasons? Since these are > outside the scope of watchdog, need your thoughts on this. Konrad / Guenter, We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus. If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data. imem,phandle = <&phandle <imem_offset> <value>>; Kindly share your thoughts and inputs on this to proceed further. > > >> >> I'd much prefer to go with 1 really.. If we used nvmem, we could have >> a map >> of cell names to restart reasons, but we've already established IMEM is >> volatile and we shouldn't mess up the convention just because that >> subsystem has nicer APIs.. >> >> Unless we rename the subsystem to `fuses`, `magic-values` or something.. >> +Srini? :P >> >> Konrad
On 5/14/25 06:15, Kathiravan Thirumoorthy wrote: > > On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote: >> >> On 5/3/2025 3:53 AM, Konrad Dybcio wrote: >>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: >>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>>>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>>>>> + const struct qcom_wdt_match_data *data) >>>>>> +{ >>>>>> + struct regmap *imem; >>>>>> + unsigned int val; >>>>>> + int ret; >>>>>> + >>>>>> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g. >>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi >>>>> >>>>> That way all platform specifics will live in the DT, requiring no >>>>> hardcode-y driver changes on similar platforms >>>> >>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value. >>>> >>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below >>>> >>>> imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>; >>>> >>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required. >>> Let's call the property qcom,restart-reason and only pass the register value >>> >>> Because we may have any number of crazy combinations of various restart >>> reasons, we can go two paths: >>> >>> 1. promise really really really hard we won't be too crazy with the number >>> of possible values and put them in the driver >>> 2. go all out on DT properties (such as `bootstatus-overheat`, >>> `bootstatus-fanfault` etc. >> >> >> Thanks Konrad for the suggestions and the offline discussions. >> >> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons. >> >> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this. > > > Konrad / Guenter, > > We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus. > Correct. The watchdog subsystem can only handle watchdog triggered reboots/resets. > If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data. > > imem,phandle = <&phandle <imem_offset> <value>>; > > Kindly share your thoughts and inputs on this to proceed further. > Sounds good to me. Guenter > >> >> >>> >>> I'd much prefer to go with 1 really.. If we used nvmem, we could have a map >>> of cell names to restart reasons, but we've already established IMEM is >>> volatile and we shouldn't mess up the convention just because that >>> subsystem has nicer APIs.. >>> >>> Unless we rename the subsystem to `fuses`, `magic-values` or something.. >>> +Srini? :P >>> >>> Konrad
On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote: > > On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote: >> >> On 5/3/2025 3:53 AM, Konrad Dybcio wrote: >>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: >>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>>>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>>>>> + const struct qcom_wdt_match_data *data) >>>>>> +{ >>>>>> + struct regmap *imem; >>>>>> + unsigned int val; >>>>>> + int ret; >>>>>> + >>>>>> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g. >>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi >>>>> >>>>> That way all platform specifics will live in the DT, requiring no >>>>> hardcode-y driver changes on similar platforms >>>> >>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value. >>>> >>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below >>>> >>>> imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>; >>>> >>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required. >>> Let's call the property qcom,restart-reason and only pass the register value >>> >>> Because we may have any number of crazy combinations of various restart >>> reasons, we can go two paths: >>> >>> 1. promise really really really hard we won't be too crazy with the number >>> of possible values and put them in the driver >>> 2. go all out on DT properties (such as `bootstatus-overheat`, >>> `bootstatus-fanfault` etc. >> >> >> Thanks Konrad for the suggestions and the offline discussions. >> >> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons. >> >> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this. > > > Konrad / Guenter, > > We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus. > > If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data. > > imem,phandle = <&phandle <imem_offset> <value>>; the part before the comma is a vendor prefix, so that must be qcom,xyz what are your plans for the other reboot reasons? are we scrapping them? Konrad
On 5/16/2025 4:48 PM, Konrad Dybcio wrote: > On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote: >> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote: >>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote: >>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: >>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>>>>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>>>>>> + const struct qcom_wdt_match_data *data) >>>>>>> +{ >>>>>>> + struct regmap *imem; >>>>>>> + unsigned int val; >>>>>>> + int ret; >>>>>>> + >>>>>>> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g. >>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi >>>>>> >>>>>> That way all platform specifics will live in the DT, requiring no >>>>>> hardcode-y driver changes on similar platforms >>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value. >>>>> >>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below >>>>> >>>>> imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>; >>>>> >>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required. >>>> Let's call the property qcom,restart-reason and only pass the register value >>>> >>>> Because we may have any number of crazy combinations of various restart >>>> reasons, we can go two paths: >>>> >>>> 1. promise really really really hard we won't be too crazy with the number >>>> of possible values and put them in the driver >>>> 2. go all out on DT properties (such as `bootstatus-overheat`, >>>> `bootstatus-fanfault` etc. >>> >>> Thanks Konrad for the suggestions and the offline discussions. >>> >>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons. >>> >>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this. >> >> Konrad / Guenter, >> >> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus. >> >> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data. >> >> imem,phandle = <&phandle <imem_offset> <value>>; > the part before the comma is a vendor prefix, so that must be qcom,xyz Sure, will name it as qcom,imem-phandle. Hope this name is fine. > > what are your plans for the other reboot reasons? are we scrapping them? No, we are not scrapping it. We are exploring further on where to put this. May be we can put those logic in some simple driver named as ipq-restart-reason.c under drivers/soc/qcom/? > > Konrad
On 5/16/25 2:52 PM, Kathiravan Thirumoorthy wrote: > > On 5/16/2025 4:48 PM, Konrad Dybcio wrote: >> On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote: >>> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote: >>>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote: >>>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: >>>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>>>>>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>>>>>>> + const struct qcom_wdt_match_data *data) >>>>>>>> +{ >>>>>>>> + struct regmap *imem; >>>>>>>> + unsigned int val; >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >>>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g. >>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi >>>>>>> >>>>>>> That way all platform specifics will live in the DT, requiring no >>>>>>> hardcode-y driver changes on similar platforms >>>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value. >>>>>> >>>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below >>>>>> >>>>>> imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>; >>>>>> >>>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required. >>>>> Let's call the property qcom,restart-reason and only pass the register value >>>>> >>>>> Because we may have any number of crazy combinations of various restart >>>>> reasons, we can go two paths: >>>>> >>>>> 1. promise really really really hard we won't be too crazy with the number >>>>> of possible values and put them in the driver >>>>> 2. go all out on DT properties (such as `bootstatus-overheat`, >>>>> `bootstatus-fanfault` etc. >>>> >>>> Thanks Konrad for the suggestions and the offline discussions. >>>> >>>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons. >>>> >>>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this. >>> >>> Konrad / Guenter, >>> >>> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus. >>> >>> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data. >>> >>> imem,phandle = <&phandle <imem_offset> <value>>; >> the part before the comma is a vendor prefix, so that must be qcom,xyz > > > Sure, will name it as qcom,imem-phandle. Hope this name is fine. just qcom,imem is fine, phandle is a datatype described in dt-bindings >> what are your plans for the other reboot reasons? are we scrapping them? > > > No, we are not scrapping it. We are exploring further on where to put this. May be we can put those logic in some simple driver named as ipq-restart-reason.c under drivers/soc/qcom/? I see drivers/power/reset/at91-reset.c does something like this Konrad
On 5/16/2025 10:05 PM, Konrad Dybcio wrote: > On 5/16/25 2:52 PM, Kathiravan Thirumoorthy wrote: >> On 5/16/2025 4:48 PM, Konrad Dybcio wrote: >>> On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote: >>>> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote: >>>>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote: >>>>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote: >>>>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote: >>>>>>>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>>>>>>>> + const struct qcom_wdt_match_data *data) >>>>>>>>> +{ >>>>>>>>> + struct regmap *imem; >>>>>>>>> + unsigned int val; >>>>>>>>> + int ret; >>>>>>>>> + >>>>>>>>> + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); >>>>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g. >>>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi >>>>>>>> >>>>>>>> That way all platform specifics will live in the DT, requiring no >>>>>>>> hardcode-y driver changes on similar platforms >>>>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value. >>>>>>> >>>>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below >>>>>>> >>>>>>> imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>; >>>>>>> >>>>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required. >>>>>> Let's call the property qcom,restart-reason and only pass the register value >>>>>> >>>>>> Because we may have any number of crazy combinations of various restart >>>>>> reasons, we can go two paths: >>>>>> >>>>>> 1. promise really really really hard we won't be too crazy with the number >>>>>> of possible values and put them in the driver >>>>>> 2. go all out on DT properties (such as `bootstatus-overheat`, >>>>>> `bootstatus-fanfault` etc. >>>>> Thanks Konrad for the suggestions and the offline discussions. >>>>> >>>>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons. >>>>> >>>>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this. >>>> Konrad / Guenter, >>>> >>>> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus. >>>> >>>> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data. >>>> >>>> imem,phandle = <&phandle <imem_offset> <value>>; >>> the part before the comma is a vendor prefix, so that must be qcom,xyz >> >> Sure, will name it as qcom,imem-phandle. Hope this name is fine. > just qcom,imem is fine, phandle is a datatype described in dt-bindings Sure thanks. > >>> what are your plans for the other reboot reasons? are we scrapping them? >> >> No, we are not scrapping it. We are exploring further on where to put this. May be we can put those logic in some simple driver named as ipq-restart-reason.c under drivers/soc/qcom/? > I see drivers/power/reset/at91-reset.c does something like this Thanks for the reference. I will submit the patches in a couple of weeks. > > Konrad
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c index dfaac5995c84c1f377023e6e62770c5548528a4c..f2cb8bfdf53a5090bcfff6ea3a23005b629ef948 100644 --- a/drivers/watchdog/qcom-wdt.c +++ b/drivers/watchdog/qcom-wdt.c @@ -7,9 +7,11 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/watchdog.h> enum wdt_reg { @@ -42,6 +44,9 @@ struct qcom_wdt_match_data { const u32 *offset; bool pretimeout; u32 max_tick_count; + const char *imem_compatible; + unsigned int restart_reason_offset; + unsigned int non_secure_wdt_val; }; struct qcom_wdt { @@ -185,6 +190,9 @@ static const struct qcom_wdt_match_data match_data_ipq5424 = { .offset = reg_offset_data_kpss, .pretimeout = true, .max_tick_count = 0xFFFFFU, + .imem_compatible = "qcom,ipq5424-imem", + .restart_reason_offset = 0x7b0, + .non_secure_wdt_val = 0x5, }; static const struct qcom_wdt_match_data match_data_kpss = { @@ -193,6 +201,29 @@ static const struct qcom_wdt_match_data match_data_kpss = { .max_tick_count = 0xFFFFFU, }; +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, + const struct qcom_wdt_match_data *data) +{ + struct regmap *imem; + unsigned int val; + int ret; + + imem = syscon_regmap_lookup_by_compatible(data->imem_compatible); + if (IS_ERR(imem)) + return PTR_ERR(imem); + + ret = regmap_read(imem, data->restart_reason_offset, &val); + if (ret) { + dev_err(wdt->wdd.parent, "failed to read the restart reason info\n"); + return ret; + } + + if (val == data->non_secure_wdt_val) + wdt->wdd.bootstatus = WDIOF_CARDRESET; + + return 0; +} + static int qcom_wdt_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -273,8 +304,13 @@ static int qcom_wdt_probe(struct platform_device *pdev) wdt->wdd.parent = dev; wdt->layout = data->offset; - if (readl(wdt_addr(wdt, WDT_STS)) & 1) - wdt->wdd.bootstatus = WDIOF_CARDRESET; + ret = qcom_wdt_get_restart_reason(wdt, data); + if (ret == -ENODEV) { + if (readl(wdt_addr(wdt, WDT_STS)) & 1) + wdt->wdd.bootstatus = WDIOF_CARDRESET; + } else if (ret) { + return ret; + } /* * If 'timeout-sec' unspecified in devicetree, assume a 30 second
When the system boots up after a watchdog reset, the EXPIRED_STATUS bit in the WDT_STS register is cleared. To identify if the system was restarted due to WDT expiry, XBL update the information in the IMEM region. Update the driver to read the restart reason from IMEM and populate the bootstatus accordingly. With the CONFIG_WATCHDOG_SYSFS enabled, user can extract the information as below: cat /sys/devices/platform/soc@0/f410000.watchdog/watchdog/watchdog0/bootstatus 32 For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new function qcom_wdt_get_restart_reason() to read the restart reason from IMEM. Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com> --- Changes in v3: - Split the introduction of device data into separate patch - s/bootloaders/XBL - for clarity of which bootloader is involved - Mention the sysfs path on to extract this information - s/compatible/imem_compatible in the device data structure to avoid the confusion / better naming Changes in v2: - Use the syscon API to access the IMEM region - Handle the error cases returned by qcom_wdt_get_restart_reason - Define device specific data to retrieve the IMEM compatible, offset and the value for non secure WDT, which allows to extend the support for other SoCs --- drivers/watchdog/qcom-wdt.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)