| Message ID | 20221207135922.314827-1-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Qcom: LLCC/EDAC: Fix base address used for LLCC banks | expand |
Hi Manivannan, On Wed Dec 7, 2022 at 2:59 PM CET, Manivannan Sadhasivam wrote: > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for > accessing the (Control and Status Regsiters) CSRs of each LLCC bank. > This offset only works for some SoCs like SDM845 for which driver support > was initially added. > > But the later SoCs use different register stride that vary between the > banks with holes in-between. So it is not possible to use a single register > stride for accessing the CSRs of each bank. By doing so could result in a > crash with the current drivers. So far this crash is not reported since > EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the > driver extensively by triggering the EDAC IRQ (that's where each bank > CSRs are accessed). > > For fixing this issue, let's obtain the base address of each LLCC bank from > devicetree and get rid of the fixed stride. > > This series affects multiple platforms but I have only tested this on > SM8250 and SM8450. Testing on other platforms is welcomed. If you can tell me *how* I can test it, I'd be happy to test the series on sm6350, like how to trigger the EDAC IRQ. So far without any extra patches I don't even see the driver probing, with this in kconfig +CONFIG_EDAC=y +CONFIG_EDAC_QCOM=y I do have /sys/bus/platform/drivers/qcom_llcc_edac at runtime but nothing in there (except bind, uevent and unbind), and also nothing interesting in dmesg with "llcc", with edac there's just this message: [ 0.064800] EDAC MC: Ver: 3.0.0 From what I'm seeing now the edac driver is only registered if the interrupt is specified but it doesn't seem like sm6350 (=lagoon) has this irq? Downstream dts is just this: cache-controller@9200000 { compatible = "lagoon-llcc-v1"; reg = <0x9200000 0x50000> , <0x9600000 0x50000>; reg-names = "llcc_base", "llcc_broadcast_base"; cap-based-alloc-and-pwr-collapse; }; From looking at the downstream code, perhaps it's using the polling mode there? /* Request for ecc irq */ ecc_irq = llcc_driv_data->ecc_irq; if (ecc_irq < 0) { dev_info(dev, "No ECC IRQ; defaulting to polling mode\n"); Let me know what you think. Regards Luca > > Thanks, > Mani > > Manivannan Sadhasivam (12): > dt-bindings: arm: msm: Update the maintainers for LLCC > dt-bindings: arm: msm: Fix register regions used for LLCC banks > arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks > arm64: dts: qcom: sc7180: Fix the base addresses of LLCC banks > arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks > arm64: dts: qcom: sc8280xp: Fix the base addresses of LLCC banks > arm64: dts: qcom: sm8150: Fix the base addresses of LLCC banks > arm64: dts: qcom: sm8250: Fix the base addresses of LLCC banks > arm64: dts: qcom: sm8350: Fix the base addresses of LLCC banks > arm64: dts: qcom: sm8450: Fix the base addresses of LLCC banks > arm64: dts: qcom: sm6350: Fix the base addresses of LLCC banks > qcom: llcc/edac: Fix the base address used for accessing LLCC banks > > .../bindings/arm/msm/qcom,llcc.yaml | 128 ++++++++++++++++-- > arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 5 +- > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 10 +- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 7 +- > arch/arm64/boot/dts/qcom/sm6350.dtsi | 2 +- > arch/arm64/boot/dts/qcom/sm8150.dtsi | 7 +- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 7 +- > arch/arm64/boot/dts/qcom/sm8350.dtsi | 7 +- > arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +- > drivers/edac/qcom_edac.c | 14 +- > drivers/soc/qcom/llcc-qcom.c | 64 +++++---- > include/linux/soc/qcom/llcc-qcom.h | 4 +- > 13 files changed, 197 insertions(+), 67 deletions(-) > > -- > 2.25.1
Hi Luca, On Thu, Dec 08, 2022 at 10:16:27AM +0100, Luca Weiss wrote: > Hi Manivannan, > > On Wed Dec 7, 2022 at 2:59 PM CET, Manivannan Sadhasivam wrote: > > The Qualcomm LLCC/EDAC drivers were using a fixed register stride for > > accessing the (Control and Status Regsiters) CSRs of each LLCC bank. > > This offset only works for some SoCs like SDM845 for which driver support > > was initially added. > > > > But the later SoCs use different register stride that vary between the > > banks with holes in-between. So it is not possible to use a single register > > stride for accessing the CSRs of each bank. By doing so could result in a > > crash with the current drivers. So far this crash is not reported since > > EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the > > driver extensively by triggering the EDAC IRQ (that's where each bank > > CSRs are accessed). > > > > For fixing this issue, let's obtain the base address of each LLCC bank from > > devicetree and get rid of the fixed stride. > > > > This series affects multiple platforms but I have only tested this on > > SM8250 and SM8450. Testing on other platforms is welcomed. > > If you can tell me *how* I can test it, I'd be happy to test the series > on sm6350, like how to trigger the EDAC IRQ. > I suppose there is no manual way to trigger EDAC IRQ on Qcom platforms. For testing the series, I manually called the EDAC IRQ handler to verify that it doesn't crash reading the registers. > So far without any extra patches I don't even see the driver probing, > with this in kconfig > > +CONFIG_EDAC=y > +CONFIG_EDAC_QCOM=y > > I do have /sys/bus/platform/drivers/qcom_llcc_edac at runtime but > nothing in there (except bind, uevent and unbind), and also nothing > interesting in dmesg with "llcc", with edac there's just this message: > > [ 0.064800] EDAC MC: Ver: 3.0.0 > > From what I'm seeing now the edac driver is only registered if the > interrupt is specified but it doesn't seem like sm6350 (=lagoon) has > this irq? Downstream dts is just this: > Right. The upstream EDAC driver only works in IRQ mode. So you need the interrupts property in LLCC devicetree node for probing. > cache-controller@9200000 { > compatible = "lagoon-llcc-v1"; > reg = <0x9200000 0x50000> , <0x9600000 0x50000>; > reg-names = "llcc_base", "llcc_broadcast_base"; > cap-based-alloc-and-pwr-collapse; > }; > > From looking at the downstream code, perhaps it's using the polling mode > there? > > /* Request for ecc irq */ > ecc_irq = llcc_driv_data->ecc_irq; > if (ecc_irq < 0) { > dev_info(dev, "No ECC IRQ; defaulting to polling mode\n"); > In the next version, I will add polling support so that you can test the series on your platform without any hacks. Thanks, Mani > Let me know what you think. > > Regards > Luca > > > > > Thanks, > > Mani > > > > Manivannan Sadhasivam (12): > > dt-bindings: arm: msm: Update the maintainers for LLCC > > dt-bindings: arm: msm: Fix register regions used for LLCC banks > > arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sc7180: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sc8280xp: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sm8150: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sm8250: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sm8350: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sm8450: Fix the base addresses of LLCC banks > > arm64: dts: qcom: sm6350: Fix the base addresses of LLCC banks > > qcom: llcc/edac: Fix the base address used for accessing LLCC banks > > > > .../bindings/arm/msm/qcom,llcc.yaml | 128 ++++++++++++++++-- > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 5 +- > > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 10 +- > > arch/arm64/boot/dts/qcom/sdm845.dtsi | 7 +- > > arch/arm64/boot/dts/qcom/sm6350.dtsi | 2 +- > > arch/arm64/boot/dts/qcom/sm8150.dtsi | 7 +- > > arch/arm64/boot/dts/qcom/sm8250.dtsi | 7 +- > > arch/arm64/boot/dts/qcom/sm8350.dtsi | 7 +- > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +- > > drivers/edac/qcom_edac.c | 14 +- > > drivers/soc/qcom/llcc-qcom.c | 64 +++++---- > > include/linux/soc/qcom/llcc-qcom.h | 4 +- > > 13 files changed, 197 insertions(+), 67 deletions(-) > > > > -- > > 2.25.1 >
The Qualcomm LLCC/EDAC drivers were using a fixed register stride for accessing the (Control and Status Regsiters) CSRs of each LLCC bank. This offset only works for some SoCs like SDM845 for which driver support was initially added. But the later SoCs use different register stride that vary between the banks with holes in-between. So it is not possible to use a single register stride for accessing the CSRs of each bank. By doing so could result in a crash with the current drivers. So far this crash is not reported since EDAC_QCOM driver is not enabled in ARM64 defconfig and no one tested the driver extensively by triggering the EDAC IRQ (that's where each bank CSRs are accessed). For fixing this issue, let's obtain the base address of each LLCC bank from devicetree and get rid of the fixed stride. This series affects multiple platforms but I have only tested this on SM8250 and SM8450. Testing on other platforms is welcomed. Thanks, Mani Manivannan Sadhasivam (12): dt-bindings: arm: msm: Update the maintainers for LLCC dt-bindings: arm: msm: Fix register regions used for LLCC banks arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks arm64: dts: qcom: sc7180: Fix the base addresses of LLCC banks arm64: dts: qcom: sc7280: Fix the base addresses of LLCC banks arm64: dts: qcom: sc8280xp: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8150: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8250: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8350: Fix the base addresses of LLCC banks arm64: dts: qcom: sm8450: Fix the base addresses of LLCC banks arm64: dts: qcom: sm6350: Fix the base addresses of LLCC banks qcom: llcc/edac: Fix the base address used for accessing LLCC banks .../bindings/arm/msm/qcom,llcc.yaml | 128 ++++++++++++++++-- arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc7280.dtsi | 5 +- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 10 +- arch/arm64/boot/dts/qcom/sdm845.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm6350.dtsi | 2 +- arch/arm64/boot/dts/qcom/sm8150.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm8250.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm8350.dtsi | 7 +- arch/arm64/boot/dts/qcom/sm8450.dtsi | 7 +- drivers/edac/qcom_edac.c | 14 +- drivers/soc/qcom/llcc-qcom.c | 64 +++++---- include/linux/soc/qcom/llcc-qcom.h | 4 +- 13 files changed, 197 insertions(+), 67 deletions(-)