| Message ID | 20230609054141.18938-1-quic_ppareek@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sa8775p: Add interconnect to SMMU | expand |
On 9.06.2023 07:41, Parikshit Pareek wrote: > Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be > This series introduce the due support for associated interconnect, and > setting of the due interconnect-bandwidth. Setting due interconnect > bandwidth is needed to avoid the issues like [1], caused by not having > due clock votes(indirectly dependent upon interconnect bandwidth). [1] ??? Konrad > > Parikshit Pareek (3): > dt-bindings: arm-smmu: Add interconnect for qcom SMMUs > arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU > iommu/arm-smmu-qcom: Add support for the interconnect > > .../devicetree/bindings/iommu/arm,smmu.yaml | 22 +++++++++++++++++++ > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 ++++ > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 16 ++++++++++++++ > 3 files changed, 42 insertions(+) >
On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: > > > On 9.06.2023 07:41, Parikshit Pareek wrote: > > Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be > > This series introduce the due support for associated interconnect, and > > setting of the due interconnect-bandwidth. Setting due interconnect > > bandwidth is needed to avoid the issues like [1], caused by not having > > due clock votes(indirectly dependent upon interconnect bandwidth). > > [1] ??? My bad. Intended to mention following: https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ Regards, Parikshit Pareek > > Konrad > > > > Parikshit Pareek (3): > > dt-bindings: arm-smmu: Add interconnect for qcom SMMUs > > arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU > > iommu/arm-smmu-qcom: Add support for the interconnect > > > > .../devicetree/bindings/iommu/arm,smmu.yaml | 22 +++++++++++++++++++ > > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 ++++ > > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 16 ++++++++++++++ > > 3 files changed, 42 insertions(+) > >
On 2023-06-09 13:56, Parikshit Pareek wrote: > On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: >> >> >> On 9.06.2023 07:41, Parikshit Pareek wrote: >>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be >>> This series introduce the due support for associated interconnect, and >>> setting of the due interconnect-bandwidth. Setting due interconnect >>> bandwidth is needed to avoid the issues like [1], caused by not having >>> due clock votes(indirectly dependent upon interconnect bandwidth). >> >> [1] ??? > > My bad. Intended to mention following: > https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs? Thanks, Robin.
On 9.06.2023 16:45, Robin Murphy wrote: > On 2023-06-09 13:56, Parikshit Pareek wrote: >> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: >>> >>> >>> On 9.06.2023 07:41, Parikshit Pareek wrote: >>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be >>>> This series introduce the due support for associated interconnect, and >>>> setting of the due interconnect-bandwidth. Setting due interconnect >>>> bandwidth is needed to avoid the issues like [1], caused by not having >>>> due clock votes(indirectly dependent upon interconnect bandwidth). >>> >>> [1] ??? >> >> My bad. Intended to mention following: >> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ > > This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs? On Qualcomm platforms, particularly so with the more recent ones, some clocks are managed by various remote cores. Half of what the interconnect infra does on these SoCs is telling one such core to change the internally managed clock's rate based on the requested bw. Konrad > > Thanks, > Robin.
On Fri, 9 Jun 2023 at 17:52, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 9.06.2023 16:45, Robin Murphy wrote: > > On 2023-06-09 13:56, Parikshit Pareek wrote: > >> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: > >>> > >>> > >>> On 9.06.2023 07:41, Parikshit Pareek wrote: > >>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be > >>>> This series introduce the due support for associated interconnect, and > >>>> setting of the due interconnect-bandwidth. Setting due interconnect > >>>> bandwidth is needed to avoid the issues like [1], caused by not having > >>>> due clock votes(indirectly dependent upon interconnect bandwidth). > >>> > >>> [1] ??? > >> > >> My bad. Intended to mention following: > >> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ > > > > This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs? > On Qualcomm platforms, particularly so with the more recent ones, some > clocks are managed by various remote cores. Half of what the interconnect > infra does on these SoCs is telling one such core to change the internally > managed clock's rate based on the requested bw. But enabling PCIe interconnect to keep SMMU working sounds strange to me too. Does the fault come from some outstanding PCIe transaction?
On 2023-06-09 15:52, Konrad Dybcio wrote: > > > On 9.06.2023 16:45, Robin Murphy wrote: >> On 2023-06-09 13:56, Parikshit Pareek wrote: >>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: >>>> >>>> >>>> On 9.06.2023 07:41, Parikshit Pareek wrote: >>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be >>>>> This series introduce the due support for associated interconnect, and >>>>> setting of the due interconnect-bandwidth. Setting due interconnect >>>>> bandwidth is needed to avoid the issues like [1], caused by not having >>>>> due clock votes(indirectly dependent upon interconnect bandwidth). >>>> >>>> [1] ??? >>> >>> My bad. Intended to mention following: >>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ >> >> This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs? > On Qualcomm platforms, particularly so with the more recent ones, some > clocks are managed by various remote cores. Half of what the interconnect > infra does on these SoCs is telling one such core to change the internally > managed clock's rate based on the requested bw. That much I get, it just seems like an arse-backwards design decision if it's really necessary to pretend the SMMU needs to access memory in order for the CPU to be able to access the SMMU. The respective SMMU interfaces are functionally independent of each other - even if it is the case in the integration that both interfaces and/or the internal TCU clock do happen to be driven synchronously from the same parent clock - and in any sane interconnect the CPU->SMMU and SMMU->RAM routes would be completely different and not intersect at all. Thanks, Robin.
On 9.06.2023 17:07, Robin Murphy wrote: > On 2023-06-09 15:52, Konrad Dybcio wrote: >> >> >> On 9.06.2023 16:45, Robin Murphy wrote: >>> On 2023-06-09 13:56, Parikshit Pareek wrote: >>>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: >>>>> >>>>> >>>>> On 9.06.2023 07:41, Parikshit Pareek wrote: >>>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be >>>>>> This series introduce the due support for associated interconnect, and >>>>>> setting of the due interconnect-bandwidth. Setting due interconnect >>>>>> bandwidth is needed to avoid the issues like [1], caused by not having >>>>>> due clock votes(indirectly dependent upon interconnect bandwidth). >>>>> >>>>> [1] ??? >>>> >>>> My bad. Intended to mention following: >>>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ >>> >>> This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs? >> On Qualcomm platforms, particularly so with the more recent ones, some >> clocks are managed by various remote cores. Half of what the interconnect >> infra does on these SoCs is telling one such core to change the internally >> managed clock's rate based on the requested bw. > > That much I get, it just seems like an arse-backwards design decision if it's really necessary to pretend the SMMU needs to access memory in order for the CPU to be able to access the SMMU. The respective SMMU interfaces are functionally independent of each other - even if it is the case in the integration that both interfaces and/or the internal TCU clock do happen to be driven synchronously from the same parent clock - and in any sane interconnect the CPU->SMMU and SMMU->RAM routes would be completely different and not intersect at all. Well, it's not the first time we stumble into a.. peculiar.. design decision on these SoCs.. That said, we can't do much about it now.. On older SoCs, some interconnect paths were strongly associated with specific TBUs which were responsible for specific SID ranges.. In this specific case, it looks like SIDs 0x000-0x3ff should correspond to PCIE0 and 0x400-0x7ff to PCIE1. But the line isn't drawn very clearly this time around, so maybe there's some internal spaghetti. Konrad > > Thanks, > Robin.
On 2023-06-09 15:56, Dmitry Baryshkov wrote: > On Fri, 9 Jun 2023 at 17:52, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> >> >> On 9.06.2023 16:45, Robin Murphy wrote: >>> On 2023-06-09 13:56, Parikshit Pareek wrote: >>>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: >>>>> >>>>> >>>>> On 9.06.2023 07:41, Parikshit Pareek wrote: >>>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be >>>>>> This series introduce the due support for associated interconnect, and >>>>>> setting of the due interconnect-bandwidth. Setting due interconnect >>>>>> bandwidth is needed to avoid the issues like [1], caused by not having >>>>>> due clock votes(indirectly dependent upon interconnect bandwidth). >>>>> >>>>> [1] ??? >>>> >>>> My bad. Intended to mention following: >>>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ >>> >>> This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs? >> On Qualcomm platforms, particularly so with the more recent ones, some >> clocks are managed by various remote cores. Half of what the interconnect >> infra does on these SoCs is telling one such core to change the internally >> managed clock's rate based on the requested bw. > > But enabling PCIe interconnect to keep SMMU working sounds strange to > me too. Does the fault come from some outstanding PCIe transaction? The "Injecting instruction/data abort to VM 3" message from the hypervisor implies that it is the access to SMMU_CR0 from arm_smmu_shutdown() that's blown up. I can even believe that the SMMU shares some clocks with the PCIe interconnect, given that its TBU must be *in* that path from PCIe to memory, at least. However I would instinctively expect the abstraction layers above to have some notion of distinct votes for "CPU wants to access SMMU" vs. "SMMU/PCIe wants to access RAM", given that the latter is liable to need to enable more than the former if the clock/power gating is as fine-grained as previous SoCs seem to have been. But maybe my hunch is wrong and this time everything's just in one big clock domain. I don't know. I'm just here to ask questions to establish whether this really is the most correct abstraction or just a lazy bodge to avoid doing the proper thing in some other driver. Thanks, Robin.
Hi, On 6/9/2023 9:09 PM, Robin Murphy wrote: > On 2023-06-09 15:56, Dmitry Baryshkov wrote: >> On Fri, 9 Jun 2023 at 17:52, Konrad Dybcio <konrad.dybcio@linaro.org> >> wrote: >>> >>> >>> >>> On 9.06.2023 16:45, Robin Murphy wrote: >>>> On 2023-06-09 13:56, Parikshit Pareek wrote: >>>>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote: >>>>>> >>>>>> >>>>>> On 9.06.2023 07:41, Parikshit Pareek wrote: >>>>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth >>>>>>> to be >>>>>>> This series introduce the due support for associated >>>>>>> interconnect, and >>>>>>> setting of the due interconnect-bandwidth. Setting due interconnect >>>>>>> bandwidth is needed to avoid the issues like [1], caused by not >>>>>>> having >>>>>>> due clock votes(indirectly dependent upon interconnect bandwidth). >>>>>> >>>>>> [1] ??? >>>>> >>>>> My bad. Intended to mention following: >>>>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/ >>>> >>>> This sounds super-dodgy - do you really have to rely on >>>> configuration of the interconnect path from the SMMU's pagetable >>>> walker to RAM to keep a completely different interconnect path >>>> clocked for the CPU to access SMMU registers? You can't just request >>>> the programming interface clock directly like on other SoCs? >>> On Qualcomm platforms, particularly so with the more recent ones, some >>> clocks are managed by various remote cores. Half of what the >>> interconnect >>> infra does on these SoCs is telling one such core to change the >>> internally >>> managed clock's rate based on the requested bw. >> >> But enabling PCIe interconnect to keep SMMU working sounds strange to >> me too. Does the fault come from some outstanding PCIe transaction? > > The "Injecting instruction/data abort to VM 3" message from the > hypervisor implies that it is the access to SMMU_CR0 from > arm_smmu_shutdown() that's blown up. I can even believe that the SMMU > shares some clocks with the PCIe interconnect, given that its TBU must > be *in* that path from PCIe to memory, at least. However I would > instinctively expect the abstraction layers above to have some notion of > distinct votes for "CPU wants to access SMMU" vs. "SMMU/PCIe wants to > access RAM", given that the latter is liable to need to enable more than > the former if the clock/power gating is as fine-grained as previous SoCs > seem to have been. But maybe my hunch is wrong and this time > everything's just in one big clock domain. I don't know. I'm just here > to ask questions to establish whether this really is the most correct > abstraction or just a lazy bodge to avoid doing the proper thing in some > other driver. > > Thanks, > Robin. For this platform to access the SMMU_CR0 we need to have pcie_tcu_clk enabled and in order to do so we have to have interconnect vote from MASTER_PCIE_[0/1] -> SLAVE_ANOC_PCIE_GEM_NOC so that AOP/RPMH can enable aggre_noc_pcie_sf_clk_src which in turns enables bulk of clocks of which pcie_tcu_clk is one. --- |RAM| ------------ ----- ----------- ---------- | GEMNOC |<----| TBU |----| PCIE ANOC |<----| pcie_0/1 | ------------ ----- ----------- ---------- ^ ^ ^ | | | | v v --- ----------------- |CPU| |PCIE TCU (smmuv2)| --- ----------------- I think this should be the right driver to implement this to have a sync with vote/unvote of the clock while the smmu register is being accessed in arm_smmu_shutdown() right ! -Shazad
On Fri, Jun 09, 2023 at 11:11:39AM +0530, Parikshit Pareek wrote: > Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be > This series introduce the due support for associated interconnect, and > setting of the due interconnect-bandwidth. Setting due interconnect > bandwidth is needed to avoid the issues like [1], caused by not having > due clock votes(indirectly dependent upon interconnect bandwidth). > As discussed offline, once you enable the PCIe RC driver which votes for this interconnect path (pcie-mem) like other platforms [1], then you do not need this series. This interconnect path belongs to the PCIe RC controller. So it is the responsibility of the PCIe RC driver to vote for this path and that's what the driver is already doing. - Mani [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp.dtsi#n1767 > Parikshit Pareek (3): > dt-bindings: arm-smmu: Add interconnect for qcom SMMUs > arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU > iommu/arm-smmu-qcom: Add support for the interconnect > > .../devicetree/bindings/iommu/arm,smmu.yaml | 22 +++++++++++++++++++ > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 ++++ > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 16 ++++++++++++++ > 3 files changed, 42 insertions(+) > > -- > 2.17.1 >