| Message ID | 20221210200353.418391-1-konrad.dybcio@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | MSM8996 eMMC boot fix | expand |
On Sat, 10 Dec 2022 at 23:04, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > In a very unfortunate turn of events, enabling interconnect on non-UFS > devices (or more precisely devices-with-UFS-clocks-disabled-from- > bootloader) crashes the device, as a memory read to an unlocked peripheral > is attempted. This series tries to fix that with the least amount of > casualties.. I worked around the icc issues by disabling sync_state. But your change is definitely better. I'll have to check if there are any other omissions in the driver. Thanks for finding this! > > Konrad Dybcio (5): > dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC > interconnect: qcom: msm8996: Provide UFS clocks to A2NoC > interconnect: qcom: msm8996: Fix regmap max_register values > interconnect: qcom: rpm: Use _optional func for provider clocks > arm64: dts: qcom: msm8996: Add additional A2NoC clocks > > .../bindings/interconnect/qcom,rpm.yaml | 24 ++++++++++++++++++- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 +++-- > drivers/interconnect/qcom/icc-rpm.c | 2 +- > drivers/interconnect/qcom/msm8996.c | 19 +++++++++++---- > 4 files changed, 42 insertions(+), 9 deletions(-) > > -- > 2.38.1 >
On 10.12.2022 21:55, Dmitry Baryshkov wrote: > On Sat, 10 Dec 2022 at 23:04, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> In a very unfortunate turn of events, enabling interconnect on non-UFS >> devices (or more precisely devices-with-UFS-clocks-disabled-from- >> bootloader) crashes the device, as a memory read to an unlocked peripheral >> is attempted. This series tries to fix that with the least amount of >> casualties.. > > I worked around the icc issues by disabling sync_state. But your > change is definitely better. I'll have to check if there are any other > omissions in the driver. > Thanks for finding this! sync_state failing is the greatest indicator of the icc driver being incomplete/broken, I've learnt that the hard way multiple times! :P Konrad > >> >> Konrad Dybcio (5): >> dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC >> interconnect: qcom: msm8996: Provide UFS clocks to A2NoC >> interconnect: qcom: msm8996: Fix regmap max_register values >> interconnect: qcom: rpm: Use _optional func for provider clocks >> arm64: dts: qcom: msm8996: Add additional A2NoC clocks >> >> .../bindings/interconnect/qcom,rpm.yaml | 24 ++++++++++++++++++- >> arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 +++-- >> drivers/interconnect/qcom/icc-rpm.c | 2 +- >> drivers/interconnect/qcom/msm8996.c | 19 +++++++++++---- >> 4 files changed, 42 insertions(+), 9 deletions(-) >> >> -- >> 2.38.1 >> > >
On 10/12/2022 22:03, Konrad Dybcio wrote: > In a very unfortunate turn of events, enabling interconnect on non-UFS > devices (or more precisely devices-with-UFS-clocks-disabled-from- > bootloader) crashes the device, as a memory read to an unlocked peripheral > is attempted. This series tries to fix that with the least amount of > casualties.. > > Konrad Dybcio (5): > dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC > interconnect: qcom: msm8996: Provide UFS clocks to A2NoC > interconnect: qcom: msm8996: Fix regmap max_register values > interconnect: qcom: rpm: Use _optional func for provider clocks > arm64: dts: qcom: msm8996: Add additional A2NoC clocks > > .../bindings/interconnect/qcom,rpm.yaml | 24 ++++++++++++++++++- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 +++-- > drivers/interconnect/qcom/icc-rpm.c | 2 +- > drivers/interconnect/qcom/msm8996.c | 19 +++++++++++---- > 4 files changed, 42 insertions(+), 9 deletions(-) > For the whole series: Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> #db820c The series indeed seems to fix the rare boot hang we observed on the dragonboard.
On 10/12/2022 22:03, Konrad Dybcio wrote: > In a very unfortunate turn of events, enabling interconnect on non-UFS > devices (or more precisely devices-with-UFS-clocks-disabled-from- > bootloader) crashes the device, as a memory read to an unlocked peripheral > is attempted. This series tries to fix that with the least amount of > casualties.. Just to note. I had occasional boot issues with UFS on msm8996 even after these patches being applied. It seems I was able to fix them by enabling interconnect support in the UFS-qcom driver. > > Konrad Dybcio (5): > dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC > interconnect: qcom: msm8996: Provide UFS clocks to A2NoC > interconnect: qcom: msm8996: Fix regmap max_register values > interconnect: qcom: rpm: Use _optional func for provider clocks > arm64: dts: qcom: msm8996: Add additional A2NoC clocks > > .../bindings/interconnect/qcom,rpm.yaml | 24 ++++++++++++++++++- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 +++-- > drivers/interconnect/qcom/icc-rpm.c | 2 +- > drivers/interconnect/qcom/msm8996.c | 19 +++++++++++---- > 4 files changed, 42 insertions(+), 9 deletions(-) >
On 23.12.2022 01:19, Dmitry Baryshkov wrote: > On 10/12/2022 22:03, Konrad Dybcio wrote: >> In a very unfortunate turn of events, enabling interconnect on non-UFS >> devices (or more precisely devices-with-UFS-clocks-disabled-from- >> bootloader) crashes the device, as a memory read to an unlocked peripheral >> is attempted. This series tries to fix that with the least amount of >> casualties.. > > Just to note. I had occasional boot issues with UFS on msm8996 even after these patches being applied. It seems I was able to fix them by enabling interconnect support in the UFS-qcom driver. > To be fair, we may be missing some more things (I have no concrete evidence, maybe things go south just because my ports of icc drivers all turn out to be garbage..): - icc is not aware of what hardware is on from the bootloader and if we don't add interconnects= *everywhere*, *including* where downstream made (in their case) educated assumptions, things start falling apart real quick, as random bits of hw may stop working if they get a zero vote - on other platforms, this is even a bigger mess, because some hardware *"unvotes"* on reset, such as MDSS or PCIe.. - if some but not all connections are described, requested bus rate may be lower than expected, with effects ranging from subpar performance to things simply not working because of too much traffic - it's really hard to test smd rpm icc drivers other than dumping reg writes and comparing them with downstream, sometimes things "work" by luck, other times it breaks booting.. Konrad >> >> Konrad Dybcio (5): >> dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC >> interconnect: qcom: msm8996: Provide UFS clocks to A2NoC >> interconnect: qcom: msm8996: Fix regmap max_register values >> interconnect: qcom: rpm: Use _optional func for provider clocks >> arm64: dts: qcom: msm8996: Add additional A2NoC clocks >> >> .../bindings/interconnect/qcom,rpm.yaml | 24 ++++++++++++++++++- >> arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 +++-- >> drivers/interconnect/qcom/icc-rpm.c | 2 +- >> drivers/interconnect/qcom/msm8996.c | 19 +++++++++++---- >> 4 files changed, 42 insertions(+), 9 deletions(-) >> >
On 23/12/2022 02:54, Konrad Dybcio wrote: > > > On 23.12.2022 01:19, Dmitry Baryshkov wrote: >> On 10/12/2022 22:03, Konrad Dybcio wrote: >>> In a very unfortunate turn of events, enabling interconnect on non-UFS >>> devices (or more precisely devices-with-UFS-clocks-disabled-from- >>> bootloader) crashes the device, as a memory read to an unlocked peripheral >>> is attempted. This series tries to fix that with the least amount of >>> casualties.. >> >> Just to note. I had occasional boot issues with UFS on msm8996 even after these patches being applied. It seems I was able to fix them by enabling interconnect support in the UFS-qcom driver. >> > To be fair, we may be missing some more things (I have no concrete > evidence, maybe things go south just because my ports of icc drivers > all turn out to be garbage..): > > - icc is not aware of what hardware is on from the bootloader > and if we don't add interconnects= *everywhere*, *including* where > downstream made (in their case) educated assumptions, things start > falling apart real quick, as random bits of hw may stop working if > they get a zero vote I think most if not all drivers unvote their resources without picking up bootloader configuration. This includes icc, clocks, power-domains, etc. > - on other platforms, this is even a bigger mess, because > some hardware *"unvotes"* on reset, such as MDSS or PCIe.. > > - if some but not all connections are described, requested bus > rate may be lower than expected, with effects ranging from subpar > performance to things simply not working because of too much traffic > > - it's really hard to test smd rpm icc drivers other than dumping > reg writes and comparing them with downstream, sometimes things > "work" by luck, other times it breaks booting.. Yeah, I have been debugging 8996 boot issue for quite some time... > > Konrad >>> >>> Konrad Dybcio (5): >>> dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC >>> interconnect: qcom: msm8996: Provide UFS clocks to A2NoC >>> interconnect: qcom: msm8996: Fix regmap max_register values >>> interconnect: qcom: rpm: Use _optional func for provider clocks >>> arm64: dts: qcom: msm8996: Add additional A2NoC clocks >>> >>> .../bindings/interconnect/qcom,rpm.yaml | 24 ++++++++++++++++++- >>> arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 +++-- >>> drivers/interconnect/qcom/icc-rpm.c | 2 +- >>> drivers/interconnect/qcom/msm8996.c | 19 +++++++++++---- >>> 4 files changed, 42 insertions(+), 9 deletions(-) >>> >>
On Sat, 10 Dec 2022 21:03:48 +0100, Konrad Dybcio wrote: > In a very unfortunate turn of events, enabling interconnect on non-UFS > devices (or more precisely devices-with-UFS-clocks-disabled-from- > bootloader) crashes the device, as a memory read to an unlocked peripheral > is attempted. This series tries to fix that with the least amount of > casualties.. > > Konrad Dybcio (5): > dt-bindings: interconnect: Add UFS clocks to MSM8996 A2NoC > interconnect: qcom: msm8996: Provide UFS clocks to A2NoC > interconnect: qcom: msm8996: Fix regmap max_register values > interconnect: qcom: rpm: Use _optional func for provider clocks > arm64: dts: qcom: msm8996: Add additional A2NoC clocks > > [...] Applied, thanks! [5/5] arm64: dts: qcom: msm8996: Add additional A2NoC clocks commit: 67fb53745e0b38275fa0b422b6a3c6c1c028c9a2 Best regards,