* [PATCH 0/2] iommu: SMMU for SDM670 @ 2022-09-20 22:39 Richard Acayan 2022-09-20 22:39 ` [PATCH 1/2] dt-bindings: iommu: arm-smmu: add sdm670 compatible Richard Acayan 2022-09-20 22:39 ` [PATCH 2/2] iommu: arm-smmu-qcom: " Richard Acayan 0 siblings, 2 replies; 17+ messages in thread From: Richard Acayan @ 2022-09-20 22:39 UTC (permalink / raw) To: linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Konrad Dybcio, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree, Richard Acayan This adds the compatible string for the Qualcomm Snapdragon 670's SMMU. The SMMU is necessary for GENI I2C, included in initial bringup because it doesn't need non-trivial patches except for patches conventionally added before the initial device tree (global clock controller and pinctrl). Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 1 + drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + 2 files changed, 2 insertions(+) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] dt-bindings: iommu: arm-smmu: add sdm670 compatible 2022-09-20 22:39 [PATCH 0/2] iommu: SMMU for SDM670 Richard Acayan @ 2022-09-20 22:39 ` Richard Acayan 2022-09-21 7:31 ` Krzysztof Kozlowski 2022-09-20 22:39 ` [PATCH 2/2] iommu: arm-smmu-qcom: " Richard Acayan 1 sibling, 1 reply; 17+ messages in thread From: Richard Acayan @ 2022-09-20 22:39 UTC (permalink / raw) To: linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Konrad Dybcio, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree, Richard Acayan The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string in the documentation to represent its support. Signed-off-by: Richard Acayan <mailingradian@gmail.com> --- Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml index 9066e6df1ba1..5ad6dc64e1c4 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml @@ -38,6 +38,7 @@ properties: - qcom,sc7280-smmu-500 - qcom,sc8180x-smmu-500 - qcom,sc8280xp-smmu-500 + - qcom,sdm670-smmu-500 - qcom,sdm845-smmu-500 - qcom,sdx55-smmu-500 - qcom,sdx65-smmu-500 -- 2.37.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iommu: arm-smmu: add sdm670 compatible 2022-09-20 22:39 ` [PATCH 1/2] dt-bindings: iommu: arm-smmu: add sdm670 compatible Richard Acayan @ 2022-09-21 7:31 ` Krzysztof Kozlowski 0 siblings, 0 replies; 17+ messages in thread From: Krzysztof Kozlowski @ 2022-09-21 7:31 UTC (permalink / raw) To: Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Konrad Dybcio, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21/09/2022 00:39, Richard Acayan wrote: > The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string in > the documentation to represent its support. > > Signed-off-by: Richard Acayan <mailingradian@gmail.com> > --- > Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 1 + Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-20 22:39 [PATCH 0/2] iommu: SMMU for SDM670 Richard Acayan 2022-09-20 22:39 ` [PATCH 1/2] dt-bindings: iommu: arm-smmu: add sdm670 compatible Richard Acayan @ 2022-09-20 22:39 ` Richard Acayan 2022-09-20 23:45 ` Konrad Dybcio 2022-09-21 7:31 ` Krzysztof Kozlowski 1 sibling, 2 replies; 17+ messages in thread From: Richard Acayan @ 2022-09-20 22:39 UTC (permalink / raw) To: linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Konrad Dybcio, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree, Richard Acayan The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to support it. Signed-off-by: Richard Acayan <mailingradian@gmail.com> --- drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c index b2708de25ea3..bf9653b9eb89 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { { .compatible = "qcom,sc8180x-smmu-500" }, { .compatible = "qcom,sc8280xp-smmu-500" }, { .compatible = "qcom,sdm630-smmu-v2" }, + { .compatible = "qcom,sdm670-smmu-500" }, { .compatible = "qcom,sdm845-smmu-500" }, { .compatible = "qcom,sm6125-smmu-500" }, { .compatible = "qcom,sm6350-smmu-500" }, -- 2.37.3 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-20 22:39 ` [PATCH 2/2] iommu: arm-smmu-qcom: " Richard Acayan @ 2022-09-20 23:45 ` Konrad Dybcio 2022-09-21 7:31 ` Krzysztof Kozlowski 1 sibling, 0 replies; 17+ messages in thread From: Konrad Dybcio @ 2022-09-20 23:45 UTC (permalink / raw) To: Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21.09.2022 00:39, Richard Acayan wrote: > The Snapdragon 670 needs the IOMMU for GENI I2C. Way more than that, but sure :D Add a compatible string to > support it. > > Signed-off-by: Richard Acayan <mailingradian@gmail.com> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@somainline.org> Konrad > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index b2708de25ea3..bf9653b9eb89 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { > { .compatible = "qcom,sc8180x-smmu-500" }, > { .compatible = "qcom,sc8280xp-smmu-500" }, > { .compatible = "qcom,sdm630-smmu-v2" }, > + { .compatible = "qcom,sdm670-smmu-500" }, > { .compatible = "qcom,sdm845-smmu-500" }, > { .compatible = "qcom,sm6125-smmu-500" }, > { .compatible = "qcom,sm6350-smmu-500" }, ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-20 22:39 ` [PATCH 2/2] iommu: arm-smmu-qcom: " Richard Acayan 2022-09-20 23:45 ` Konrad Dybcio @ 2022-09-21 7:31 ` Krzysztof Kozlowski 2022-09-21 18:47 ` Konrad Dybcio 1 sibling, 1 reply; 17+ messages in thread From: Krzysztof Kozlowski @ 2022-09-21 7:31 UTC (permalink / raw) To: Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Konrad Dybcio, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21/09/2022 00:39, Richard Acayan wrote: > The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to > support it. > > Signed-off-by: Richard Acayan <mailingradian@gmail.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index b2708de25ea3..bf9653b9eb89 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { > { .compatible = "qcom,sc8180x-smmu-500" }, > { .compatible = "qcom,sc8280xp-smmu-500" }, > { .compatible = "qcom,sdm630-smmu-v2" }, > + { .compatible = "qcom,sdm670-smmu-500" }, Why do we keep adding compatibles to the driver for apparently compatible devices? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-21 7:31 ` Krzysztof Kozlowski @ 2022-09-21 18:47 ` Konrad Dybcio 2022-09-21 18:48 ` Konrad Dybcio 0 siblings, 1 reply; 17+ messages in thread From: Konrad Dybcio @ 2022-09-21 18:47 UTC (permalink / raw) To: Krzysztof Kozlowski, Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21.09.2022 09:31, Krzysztof Kozlowski wrote: > On 21/09/2022 00:39, Richard Acayan wrote: >> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >> support it. >> >> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >> --- >> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> index b2708de25ea3..bf9653b9eb89 100644 >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >> { .compatible = "qcom,sc8180x-smmu-500" }, >> { .compatible = "qcom,sc8280xp-smmu-500" }, >> { .compatible = "qcom,sdm630-smmu-v2" }, >> + { .compatible = "qcom,sdm670-smmu-500" }, > > Why do we keep adding compatibles to the driver for apparently > compatible devices? Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation of it, that's abstracted to us through various generations of their saddening software stack. This is also the case for many more standard components, even as far as the GIC on recent boards.. Konrad > > > Best regards, > Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-21 18:47 ` Konrad Dybcio @ 2022-09-21 18:48 ` Konrad Dybcio 2022-09-21 19:05 ` Krzysztof Kozlowski 0 siblings, 1 reply; 17+ messages in thread From: Konrad Dybcio @ 2022-09-21 18:48 UTC (permalink / raw) To: Krzysztof Kozlowski, Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21.09.2022 20:47, Konrad Dybcio wrote: > > > On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >> On 21/09/2022 00:39, Richard Acayan wrote: >>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>> support it. >>> >>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>> --- >>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> index b2708de25ea3..bf9653b9eb89 100644 >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>> { .compatible = "qcom,sc8180x-smmu-500" }, >>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>> { .compatible = "qcom,sdm630-smmu-v2" }, >>> + { .compatible = "qcom,sdm670-smmu-500" }, >> >> Why do we keep adding compatibles to the driver for apparently >> compatible devices? > > Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and s/funny/fully unfortunate typo, this is not funny, quite the contrary.. Konrad > we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation > of it, that's abstracted to us through various generations of their saddening software stack. This > is also the case for many more standard components, even as far as the GIC on recent boards.. > > Konrad >> >> >> Best regards, >> Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-21 18:48 ` Konrad Dybcio @ 2022-09-21 19:05 ` Krzysztof Kozlowski 2022-09-21 21:14 ` Konrad Dybcio 0 siblings, 1 reply; 17+ messages in thread From: Krzysztof Kozlowski @ 2022-09-21 19:05 UTC (permalink / raw) To: Konrad Dybcio, Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21/09/2022 20:48, Konrad Dybcio wrote: > > > On 21.09.2022 20:47, Konrad Dybcio wrote: >> >> >> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>> On 21/09/2022 00:39, Richard Acayan wrote: >>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>> support it. >>>> >>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>> --- >>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> index b2708de25ea3..bf9653b9eb89 100644 >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>> >>> Why do we keep adding compatibles to the driver for apparently >>> compatible devices? >> >> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and > s/funny/fully > > unfortunate typo, this is not funny, quite the contrary.. > > Konrad >> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >> of it, that's abstracted to us through various generations of their saddening software stack. This >> is also the case for many more standard components, even as far as the GIC on recent boards.. Unfortunately I don't get this explanation... you mean some other firmware requires Linux drivers to use specific compatibles instead of one fallback? All of these do not have driver data, so they are essentially compatible for Linux driver. Growing this list in the driver seems pointless. What is the benefit of growing driver with same entries, except more patches? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-21 19:05 ` Krzysztof Kozlowski @ 2022-09-21 21:14 ` Konrad Dybcio 2022-09-22 2:38 ` Richard Acayan ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Konrad Dybcio @ 2022-09-21 21:14 UTC (permalink / raw) To: Krzysztof Kozlowski, Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21.09.2022 21:05, Krzysztof Kozlowski wrote: > On 21/09/2022 20:48, Konrad Dybcio wrote: >> >> >> On 21.09.2022 20:47, Konrad Dybcio wrote: >>> >>> >>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>>> On 21/09/2022 00:39, Richard Acayan wrote: >>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>>> support it. >>>>> >>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>>> --- >>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> index b2708de25ea3..bf9653b9eb89 100644 >>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>>> >>>> Why do we keep adding compatibles to the driver for apparently >>>> compatible devices? >>> >>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and >> s/funny/fully >> >> unfortunate typo, this is not funny, quite the contrary.. >> >> Konrad >>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >>> of it, that's abstracted to us through various generations of their saddening software stack. This >>> is also the case for many more standard components, even as far as the GIC on recent boards.. > > Unfortunately I don't get this explanation... you mean some other > firmware requires Linux drivers to use specific compatibles instead of > one fallback? No, perhaps I misunderstood you. > > All of these do not have driver data, so they are essentially compatible > for Linux driver. Growing this list in the driver seems pointless. What > is the benefit of growing driver with same entries, except more patches? Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves and consumer devices (such as MDSS). The situation is more complicated, because some qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use `qcom_smmu_impl` and some others need even more quirks on top of that (that generally hurt performance or functionality, so we don't want them when they're unnecessary). If all generations of qcom SMMU implementation that bear the same name behaved anywhere near consistent, there would be no need for keeping this around, instead requiring only "qcom,broken-smmu" or something". Konrad > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-21 21:14 ` Konrad Dybcio @ 2022-09-22 2:38 ` Richard Acayan 2022-09-22 6:41 ` Krzysztof Kozlowski 2022-09-22 6:39 ` Krzysztof Kozlowski 2022-10-18 10:21 ` Dmitry Baryshkov 2 siblings, 1 reply; 17+ messages in thread From: Richard Acayan @ 2022-09-22 2:38 UTC (permalink / raw) To: Konrad Dybcio Cc: Krzysztof Kozlowski, Richard Acayan, linux-arm-msm, Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree >On 21.09.2022 21:05, Krzysztof Kozlowski wrote: >> On 21/09/2022 20:48, Konrad Dybcio wrote: >>> >>> >>> On 21.09.2022 20:47, Konrad Dybcio wrote: >>>> >>>> >>>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>>>> On 21/09/2022 00:39, Richard Acayan wrote: >>>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>>>> support it. >>>>>> >>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>>>> --- >>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> index b2708de25ea3..bf9653b9eb89 100644 >>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>>>> >>>>> Why do we keep adding compatibles to the driver for apparently >>>>> compatible devices? >>>> >>>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and >>> s/funny/fully >>> >>> unfortunate typo, this is not funny, quite the contrary.. >>> >>> Konrad >>>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >>>> of it, that's abstracted to us through various generations of their saddening software stack. This >>>> is also the case for many more standard components, even as far as the GIC on recent boards.. >> >> Unfortunately I don't get this explanation... you mean some other >> firmware requires Linux drivers to use specific compatibles instead of >> one fallback? > No, perhaps I misunderstood you. > >> >> All of these do not have driver data, so they are essentially compatible >> for Linux driver. Growing this list in the driver seems pointless. What >> is the benefit of growing driver with same entries, except more patches? > Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves >and consumer devices (such as MDSS). The situation is more complicated, because some >qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 >vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use > `qcom_smmu_impl` and some others need even more quirks on top of that (that generally >hurt performance or functionality, so we don't want them when they're unnecessary). > If all generations of qcom SMMU implementation that bear the same name behaved anywhere >near consistent, there would be no need for keeping this around, instead requiring only > "qcom,broken-smmu" or something". Hi, just stopping by to share my own thoughts. First, I don't mind if this series doesn't get applied as-is. After seeing the eMMC driver in its current state: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-msm.c?h=v6.0-rc6#n2437 I can understand that the devicetree maintainers don't want to see new SoCs touching drivers unnecessarily. Second, I don't see enough quirks to justify needing all compatible strings in the driver (2 quirky SoCs compared to 16 total not counting adreno iommu): $ grep qcom, drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c if (of_device_is_compatible(np, "qcom,msm8996-smmu-v2")) * All targets that use the qcom,adreno-smmu compatible string *should* { .compatible = "qcom,adreno" }, { .compatible = "qcom,mdp4" }, { .compatible = "qcom,mdss" }, { .compatible = "qcom,sc7180-mdss" }, { .compatible = "qcom,sc7180-mss-pil" }, { .compatible = "qcom,sc7280-mdss" }, { .compatible = "qcom,sc7280-mss-pil" }, { .compatible = "qcom,sc8180x-mdss" }, { .compatible = "qcom,sm8250-mdss" }, { .compatible = "qcom,sdm845-mdss" }, { .compatible = "qcom,sdm845-mss-pil" }, if (of_device_is_compatible(np, "qcom,sdm845-smmu-500")) { .compatible = "qcom,msm8998-smmu-v2" }, { .compatible = "qcom,qcm2290-smmu-500" }, { .compatible = "qcom,sc7180-smmu-500" }, { .compatible = "qcom,sc7280-smmu-500" }, { .compatible = "qcom,sc8180x-smmu-500" }, { .compatible = "qcom,sc8280xp-smmu-500" }, { .compatible = "qcom,sdm630-smmu-v2" }, { .compatible = "qcom,sdm670-smmu-500" }, { .compatible = "qcom,sdm845-smmu-500" }, { .compatible = "qcom,sm6125-smmu-500" }, { .compatible = "qcom,sm6350-smmu-500" }, { .compatible = "qcom,sm6375-smmu-500" }, { .compatible = "qcom,sm8150-smmu-500" }, { .compatible = "qcom,sm8250-smmu-500" }, { .compatible = "qcom,sm8350-smmu-500" }, { .compatible = "qcom,sm8450-smmu-500" }, if (of_device_is_compatible(np, "qcom,adreno-smmu")) I don't know if it's better to get myself involved in fixing this, though. There is no fallback that encompasses qcom devices but not all arm devices. Either way, I'll have to add a new compatible string to the driver. If something like this is fine for now, I'll format it properly tomorrow: --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml @@ -48,6 +48,13 @@ properties: - qcom,sm8350-smmu-500 - qcom,sm8450-smmu-500 - const: arm,mmu-500 + + - description: Qcom SoCs implementing "qcom,smmu-500" + items: + - enum: + - qcom,sdm670-smmu-500 + - const: qcom,smmu-500 + - description: Qcom Adreno GPUs implementing "arm,smmu-v2" items: - enum: --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c @@ -424,6 +424,7 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu, } static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { + { .compatible = "qcom,smmu-500" }, { .compatible = "qcom,msm8998-smmu-v2" }, { .compatible = "qcom,qcm2290-smmu-500" }, { .compatible = "qcom,sc7180-smmu-500" }, > > Konrad >> >> Best regards, >> Krzysztof >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-22 2:38 ` Richard Acayan @ 2022-09-22 6:41 ` Krzysztof Kozlowski 2022-09-22 10:48 ` Konrad Dybcio 0 siblings, 1 reply; 17+ messages in thread From: Krzysztof Kozlowski @ 2022-09-22 6:41 UTC (permalink / raw) To: Richard Acayan, Konrad Dybcio Cc: linux-arm-msm, Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 22/09/2022 04:38, Richard Acayan wrote: >> On 21.09.2022 21:05, Krzysztof Kozlowski wrote: >>> On 21/09/2022 20:48, Konrad Dybcio wrote: >>>> >>>> >>>> On 21.09.2022 20:47, Konrad Dybcio wrote: >>>>> >>>>> >>>>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>>>>> On 21/09/2022 00:39, Richard Acayan wrote: >>>>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>>>>> support it. >>>>>>> >>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>>>>> --- >>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>> index b2708de25ea3..bf9653b9eb89 100644 >>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>>>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>>>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>>>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>>>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>>>>> >>>>>> Why do we keep adding compatibles to the driver for apparently >>>>>> compatible devices? >>>>> >>>>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and >>>> s/funny/fully >>>> >>>> unfortunate typo, this is not funny, quite the contrary.. >>>> >>>> Konrad >>>>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >>>>> of it, that's abstracted to us through various generations of their saddening software stack. This >>>>> is also the case for many more standard components, even as far as the GIC on recent boards.. >>> >>> Unfortunately I don't get this explanation... you mean some other >>> firmware requires Linux drivers to use specific compatibles instead of >>> one fallback? >> No, perhaps I misunderstood you. >> >>> >>> All of these do not have driver data, so they are essentially compatible >>> for Linux driver. Growing this list in the driver seems pointless. What >>> is the benefit of growing driver with same entries, except more patches? >> Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves >> and consumer devices (such as MDSS). The situation is more complicated, because some >> qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 >> vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use >> `qcom_smmu_impl` and some others need even more quirks on top of that (that generally >> hurt performance or functionality, so we don't want them when they're unnecessary). >> If all generations of qcom SMMU implementation that bear the same name behaved anywhere >> near consistent, there would be no need for keeping this around, instead requiring only >> "qcom,broken-smmu" or something". > > Hi, just stopping by to share my own thoughts. > > First, I don't mind if this series doesn't get applied as-is. After seeing > the eMMC driver in its current state: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-msm.c?h=v6.0-rc6#n2437 > > I can understand that the devicetree maintainers don't want to see new SoCs > touching drivers unnecessarily. Second, I don't see enough quirks to > justify needing all compatible strings in the driver (2 quirky SoCs > compared to 16 total not counting adreno iommu): > > $ grep qcom, drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > if (of_device_is_compatible(np, "qcom,msm8996-smmu-v2")) > * All targets that use the qcom,adreno-smmu compatible string *should* > { .compatible = "qcom,adreno" }, > { .compatible = "qcom,mdp4" }, > { .compatible = "qcom,mdss" }, > { .compatible = "qcom,sc7180-mdss" }, > { .compatible = "qcom,sc7180-mss-pil" }, > { .compatible = "qcom,sc7280-mdss" }, > { .compatible = "qcom,sc7280-mss-pil" }, > { .compatible = "qcom,sc8180x-mdss" }, > { .compatible = "qcom,sm8250-mdss" }, > { .compatible = "qcom,sdm845-mdss" }, > { .compatible = "qcom,sdm845-mss-pil" }, > if (of_device_is_compatible(np, "qcom,sdm845-smmu-500")) > { .compatible = "qcom,msm8998-smmu-v2" }, > { .compatible = "qcom,qcm2290-smmu-500" }, > { .compatible = "qcom,sc7180-smmu-500" }, > { .compatible = "qcom,sc7280-smmu-500" }, > { .compatible = "qcom,sc8180x-smmu-500" }, > { .compatible = "qcom,sc8280xp-smmu-500" }, > { .compatible = "qcom,sdm630-smmu-v2" }, > { .compatible = "qcom,sdm670-smmu-500" }, > { .compatible = "qcom,sdm845-smmu-500" }, > { .compatible = "qcom,sm6125-smmu-500" }, > { .compatible = "qcom,sm6350-smmu-500" }, > { .compatible = "qcom,sm6375-smmu-500" }, > { .compatible = "qcom,sm8150-smmu-500" }, > { .compatible = "qcom,sm8250-smmu-500" }, > { .compatible = "qcom,sm8350-smmu-500" }, > { .compatible = "qcom,sm8450-smmu-500" }, > if (of_device_is_compatible(np, "qcom,adreno-smmu")) > > I don't know if it's better to get myself involved in fixing this, though. > There is no fallback that encompasses qcom devices but not all arm devices. > Either way, I'll have to add a new compatible string to the driver. > > If something like this is fine for now, I'll format it properly tomorrow: Please wait till we reach some conclusion otherwise your work might be wasted. > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml > @@ -48,6 +48,13 @@ properties: > - qcom,sm8350-smmu-500 > - qcom,sm8450-smmu-500 > - const: arm,mmu-500 > + > + - description: Qcom SoCs implementing "qcom,smmu-500" > + items: > + - enum: > + - qcom,sdm670-smmu-500 > + - const: qcom,smmu-500 > + Someone would have to confirm that smmu-500 is a real device spec/version. Otherwise this should be device-specific compatible (e.g. earliest in family). Best regards, Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-22 6:41 ` Krzysztof Kozlowski @ 2022-09-22 10:48 ` Konrad Dybcio 2022-09-22 12:18 ` Krzysztof Kozlowski 0 siblings, 1 reply; 17+ messages in thread From: Konrad Dybcio @ 2022-09-22 10:48 UTC (permalink / raw) To: Krzysztof Kozlowski, Richard Acayan Cc: linux-arm-msm, Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 22.09.2022 08:41, Krzysztof Kozlowski wrote: > On 22/09/2022 04:38, Richard Acayan wrote: >>> On 21.09.2022 21:05, Krzysztof Kozlowski wrote: >>>> On 21/09/2022 20:48, Konrad Dybcio wrote: >>>>> >>>>> >>>>> On 21.09.2022 20:47, Konrad Dybcio wrote: >>>>>> >>>>>> >>>>>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>>>>>> On 21/09/2022 00:39, Richard Acayan wrote: >>>>>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>>>>>> support it. >>>>>>>> >>>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>>>>>> --- >>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>> >>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> index b2708de25ea3..bf9653b9eb89 100644 >>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>>>>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>>>>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>>>>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>>>>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>>>>>> >>>>>>> Why do we keep adding compatibles to the driver for apparently >>>>>>> compatible devices? >>>>>> >>>>>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and >>>>> s/funny/fully >>>>> >>>>> unfortunate typo, this is not funny, quite the contrary.. >>>>> >>>>> Konrad >>>>>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >>>>>> of it, that's abstracted to us through various generations of their saddening software stack. This >>>>>> is also the case for many more standard components, even as far as the GIC on recent boards.. >>>> >>>> Unfortunately I don't get this explanation... you mean some other >>>> firmware requires Linux drivers to use specific compatibles instead of >>>> one fallback? >>> No, perhaps I misunderstood you. >>> >>>> >>>> All of these do not have driver data, so they are essentially compatible >>>> for Linux driver. Growing this list in the driver seems pointless. What >>>> is the benefit of growing driver with same entries, except more patches? >>> Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves >>> and consumer devices (such as MDSS). The situation is more complicated, because some >>> qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 >>> vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use >>> `qcom_smmu_impl` and some others need even more quirks on top of that (that generally >>> hurt performance or functionality, so we don't want them when they're unnecessary). >>> If all generations of qcom SMMU implementation that bear the same name behaved anywhere >>> near consistent, there would be no need for keeping this around, instead requiring only >>> "qcom,broken-smmu" or something". >> >> Hi, just stopping by to share my own thoughts. >> >> First, I don't mind if this series doesn't get applied as-is. After seeing >> the eMMC driver in its current state: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-msm.c?h=v6.0-rc6#n2437 >> >> I can understand that the devicetree maintainers don't want to see new SoCs >> touching drivers unnecessarily. Second, I don't see enough quirks to >> justify needing all compatible strings in the driver (2 quirky SoCs >> compared to 16 total not counting adreno iommu): >> >> $ grep qcom, drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >> if (of_device_is_compatible(np, "qcom,msm8996-smmu-v2")) >> * All targets that use the qcom,adreno-smmu compatible string *should* >> { .compatible = "qcom,adreno" }, >> { .compatible = "qcom,mdp4" }, >> { .compatible = "qcom,mdss" }, >> { .compatible = "qcom,sc7180-mdss" }, >> { .compatible = "qcom,sc7180-mss-pil" }, >> { .compatible = "qcom,sc7280-mdss" }, >> { .compatible = "qcom,sc7280-mss-pil" }, >> { .compatible = "qcom,sc8180x-mdss" }, >> { .compatible = "qcom,sm8250-mdss" }, >> { .compatible = "qcom,sdm845-mdss" }, >> { .compatible = "qcom,sdm845-mss-pil" }, >> if (of_device_is_compatible(np, "qcom,sdm845-smmu-500")) >> { .compatible = "qcom,msm8998-smmu-v2" }, >> { .compatible = "qcom,qcm2290-smmu-500" }, >> { .compatible = "qcom,sc7180-smmu-500" }, >> { .compatible = "qcom,sc7280-smmu-500" }, >> { .compatible = "qcom,sc8180x-smmu-500" }, >> { .compatible = "qcom,sc8280xp-smmu-500" }, >> { .compatible = "qcom,sdm630-smmu-v2" }, >> { .compatible = "qcom,sdm670-smmu-500" }, >> { .compatible = "qcom,sdm845-smmu-500" }, >> { .compatible = "qcom,sm6125-smmu-500" }, >> { .compatible = "qcom,sm6350-smmu-500" }, >> { .compatible = "qcom,sm6375-smmu-500" }, >> { .compatible = "qcom,sm8150-smmu-500" }, >> { .compatible = "qcom,sm8250-smmu-500" }, >> { .compatible = "qcom,sm8350-smmu-500" }, >> { .compatible = "qcom,sm8450-smmu-500" }, >> if (of_device_is_compatible(np, "qcom,adreno-smmu")) >> >> I don't know if it's better to get myself involved in fixing this, though. >> There is no fallback that encompasses qcom devices but not all arm devices. >> Either way, I'll have to add a new compatible string to the driver. >> >> If something like this is fine for now, I'll format it properly tomorrow: > > Please wait till we reach some conclusion otherwise your work might be > wasted. > >> >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >> @@ -48,6 +48,13 @@ properties: >> - qcom,sm8350-smmu-500 >> - qcom,sm8450-smmu-500 >> - const: arm,mmu-500 >> + >> + - description: Qcom SoCs implementing "qcom,smmu-500" >> + items: >> + - enum: >> + - qcom,sdm670-smmu-500 >> + - const: qcom,smmu-500 >> + > > Someone would have to confirm that smmu-500 is a real device > spec/version. Otherwise this should be device-specific compatible (e.g. > earliest in family). In my view it's hard to name it, downstream uses bool properties for enabling/disabling certain quirks and on different generations there are different combinations of that. Interestingly enough, I vaguely remember that the same quirk names can mean different things on different msm-X.Y versions.. Add to that, different msm-X.Y versions can have different assumptions on what's the default (aka without taking the bool properties into account) behaviour for a given compatible. So I suppose "first in the family" would be the best way to go for mainline, though there are still quite a few families: <earlier ones used qcom_iommu> - 8996 with quirks that are already accounted for (or one may also say it works by miracle, just like msm8916 - downstream adds more special handling, but looks like the fw is not as restrictive) - 8996pro + 660 + 8998 with serious unmerged ones [1] - 845 which seems to be aok - special case of chromebooks where they only have qcom TZ/XPUs and not the hypervisor to fight with, so ma-a-aybe (no downstream reference & I don't have the hw to confirm) they can get away with less things - 8[1234]50 which seem to be a mix-and-match of less serious (read: not accounting for them may hurt performance but will not make your device sepuku at SMMU probe) minor quirks [2][3][4][5] (big warning: these may be overriden somewhere in other device tree fragments, the surest option would be to take a compiled dtb and decompile it to be sure about it) - 4xxx/6xxx series that mostly align with "whatever was there on the flagship soc released a year before" Konrad [1] https://github.com/SoMainline/linux/commits/marijn/v5.19-ninges/drivers/iommu [2] https://github.com/TeamWin/android_kernel_samsung_d2q/blob/master/arch/arm64/boot/dts/qcom/msm-arm-smmu-sm8150-v2.dtsi [3] https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/tags/android-11.0.0_r0.56/qcom/msm-arm-smmu-kona.dtsi [4] https://github.com/sonyxperiadev/kernel-copyleft-dts/blob/61.0.A.11.xxx/qcom/msm-arm-smmu-lahaina.dtsi [5] https://github.com/sonyxperiadev/kernel-copyleft-dts/blob/64.0.A.2.xxx/devicetree/qcom/msm-arm-smmu-waipio.dtsi > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-22 10:48 ` Konrad Dybcio @ 2022-09-22 12:18 ` Krzysztof Kozlowski 2022-09-23 22:24 ` Richard Acayan 0 siblings, 1 reply; 17+ messages in thread From: Krzysztof Kozlowski @ 2022-09-22 12:18 UTC (permalink / raw) To: Konrad Dybcio, Richard Acayan Cc: linux-arm-msm, Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 22/09/2022 12:48, Konrad Dybcio wrote: > > > On 22.09.2022 08:41, Krzysztof Kozlowski wrote: >> On 22/09/2022 04:38, Richard Acayan wrote: >>>> On 21.09.2022 21:05, Krzysztof Kozlowski wrote: >>>>> On 21/09/2022 20:48, Konrad Dybcio wrote: >>>>>> >>>>>> >>>>>> On 21.09.2022 20:47, Konrad Dybcio wrote: >>>>>>> >>>>>>> >>>>>>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>>>>>>> On 21/09/2022 00:39, Richard Acayan wrote: >>>>>>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>>>>>>> support it. >>>>>>>>> >>>>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>>>>>>> --- >>>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>> index b2708de25ea3..bf9653b9eb89 100644 >>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>>>>>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>>>>>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>>>>>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>>>>>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>>>>>>> >>>>>>>> Why do we keep adding compatibles to the driver for apparently >>>>>>>> compatible devices? >>>>>>> >>>>>>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and >>>>>> s/funny/fully >>>>>> >>>>>> unfortunate typo, this is not funny, quite the contrary.. >>>>>> >>>>>> Konrad >>>>>>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >>>>>>> of it, that's abstracted to us through various generations of their saddening software stack. This >>>>>>> is also the case for many more standard components, even as far as the GIC on recent boards.. >>>>> >>>>> Unfortunately I don't get this explanation... you mean some other >>>>> firmware requires Linux drivers to use specific compatibles instead of >>>>> one fallback? >>>> No, perhaps I misunderstood you. >>>> >>>>> >>>>> All of these do not have driver data, so they are essentially compatible >>>>> for Linux driver. Growing this list in the driver seems pointless. What >>>>> is the benefit of growing driver with same entries, except more patches? >>>> Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves >>>> and consumer devices (such as MDSS). The situation is more complicated, because some >>>> qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 >>>> vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use >>>> `qcom_smmu_impl` and some others need even more quirks on top of that (that generally >>>> hurt performance or functionality, so we don't want them when they're unnecessary). >>>> If all generations of qcom SMMU implementation that bear the same name behaved anywhere >>>> near consistent, there would be no need for keeping this around, instead requiring only >>>> "qcom,broken-smmu" or something". >>> >>> Hi, just stopping by to share my own thoughts. >>> >>> First, I don't mind if this series doesn't get applied as-is. After seeing >>> the eMMC driver in its current state: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-msm.c?h=v6.0-rc6#n2437 >>> >>> I can understand that the devicetree maintainers don't want to see new SoCs >>> touching drivers unnecessarily. Second, I don't see enough quirks to >>> justify needing all compatible strings in the driver (2 quirky SoCs >>> compared to 16 total not counting adreno iommu): >>> >>> $ grep qcom, drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>> if (of_device_is_compatible(np, "qcom,msm8996-smmu-v2")) >>> * All targets that use the qcom,adreno-smmu compatible string *should* >>> { .compatible = "qcom,adreno" }, >>> { .compatible = "qcom,mdp4" }, >>> { .compatible = "qcom,mdss" }, >>> { .compatible = "qcom,sc7180-mdss" }, >>> { .compatible = "qcom,sc7180-mss-pil" }, >>> { .compatible = "qcom,sc7280-mdss" }, >>> { .compatible = "qcom,sc7280-mss-pil" }, >>> { .compatible = "qcom,sc8180x-mdss" }, >>> { .compatible = "qcom,sm8250-mdss" }, >>> { .compatible = "qcom,sdm845-mdss" }, >>> { .compatible = "qcom,sdm845-mss-pil" }, >>> if (of_device_is_compatible(np, "qcom,sdm845-smmu-500")) >>> { .compatible = "qcom,msm8998-smmu-v2" }, >>> { .compatible = "qcom,qcm2290-smmu-500" }, >>> { .compatible = "qcom,sc7180-smmu-500" }, >>> { .compatible = "qcom,sc7280-smmu-500" }, >>> { .compatible = "qcom,sc8180x-smmu-500" }, >>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>> { .compatible = "qcom,sdm630-smmu-v2" }, >>> { .compatible = "qcom,sdm670-smmu-500" }, >>> { .compatible = "qcom,sdm845-smmu-500" }, >>> { .compatible = "qcom,sm6125-smmu-500" }, >>> { .compatible = "qcom,sm6350-smmu-500" }, >>> { .compatible = "qcom,sm6375-smmu-500" }, >>> { .compatible = "qcom,sm8150-smmu-500" }, >>> { .compatible = "qcom,sm8250-smmu-500" }, >>> { .compatible = "qcom,sm8350-smmu-500" }, >>> { .compatible = "qcom,sm8450-smmu-500" }, >>> if (of_device_is_compatible(np, "qcom,adreno-smmu")) >>> >>> I don't know if it's better to get myself involved in fixing this, though. >>> There is no fallback that encompasses qcom devices but not all arm devices. >>> Either way, I'll have to add a new compatible string to the driver. >>> >>> If something like this is fine for now, I'll format it properly tomorrow: >> >> Please wait till we reach some conclusion otherwise your work might be >> wasted. >> >>> >>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >>> @@ -48,6 +48,13 @@ properties: >>> - qcom,sm8350-smmu-500 >>> - qcom,sm8450-smmu-500 >>> - const: arm,mmu-500 >>> + >>> + - description: Qcom SoCs implementing "qcom,smmu-500" >>> + items: >>> + - enum: >>> + - qcom,sdm670-smmu-500 >>> + - const: qcom,smmu-500 >>> + >> >> Someone would have to confirm that smmu-500 is a real device >> spec/version. Otherwise this should be device-specific compatible (e.g. >> earliest in family). > In my view it's hard to name it, downstream uses bool properties for enabling/disabling > certain quirks and on different generations there are different combinations of that. > Interestingly enough, I vaguely remember that the same quirk names can mean different > things on different msm-X.Y versions.. Add to that, different msm-X.Y versions can have > different assumptions on what's the default (aka without taking the bool properties into > account) behaviour for a given compatible. Downstream does not care about ABI, coding style, reasonable approach, so it should not wonder that they code things inconsistent. > So I suppose "first in the family" would be > the best way to go for mainline, though there are still quite a few families: > > <earlier ones used qcom_iommu> > - 8996 with quirks that are already accounted for (or one may also say it works by miracle, > just like msm8916 - downstream adds more special handling, but looks like the fw is not as > restrictive) > > - 8996pro + 660 + 8998 with serious unmerged ones [1] > > - 845 which seems to be aok > > - special case of chromebooks where they only have qcom TZ/XPUs and not the hypervisor to > fight with, so ma-a-aybe (no downstream reference & I don't have the hw to confirm) they > can get away with less things > > - 8[1234]50 which seem to be a mix-and-match of less serious (read: not accounting for them > may hurt performance but will not make your device sepuku at SMMU probe) minor quirks > [2][3][4][5] (big warning: these may be overriden somewhere in other device tree fragments, > the surest option would be to take a compiled dtb and decompile it to be sure about it) > > - 4xxx/6xxx series that mostly align with "whatever was there on the flagship soc released > a year before" If the devices are really different, even though it is not visible in Linux driver, then indeed there is no point for fake compatibility. I raised the question only because the driver does not customize the variants, but that might be not enough. Linked DTSI use different quirks (assuming quirks would have same meaning...), plus they have sometimes different amount of clocks, so in total maybe it is not reasonable to make them compatible. On the other hand, maybe the programming model is very, very similar thus Linux could bind to one fallback and few different bits would be customized with specific compatible. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-22 12:18 ` Krzysztof Kozlowski @ 2022-09-23 22:24 ` Richard Acayan 0 siblings, 0 replies; 17+ messages in thread From: Richard Acayan @ 2022-09-23 22:24 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: linux-arm-msm, Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Konrad Dybcio, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree, Richard Acayan > On 22/09/2022 12:48, Konrad Dybcio wrote: >> >> >> On 22.09.2022 08:41, Krzysztof Kozlowski wrote: >>> On 22/09/2022 04:38, Richard Acayan wrote: >>>>> On 21.09.2022 21:05, Krzysztof Kozlowski wrote: >>>>>> On 21/09/2022 20:48, Konrad Dybcio wrote: >>>>>>> >>>>>>> >>>>>>> On 21.09.2022 20:47, Konrad Dybcio wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>>>>>>>> On 21/09/2022 00:39, Richard Acayan wrote: >>>>>>>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>>>>>>>> support it. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>>>>>>>> --- >>>>>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>> index b2708de25ea3..bf9653b9eb89 100644 >>>>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>>>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>>>>>>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>>>>>>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>>>>>>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>>>>>>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>>>>>>>> >>>>>>>>> Why do we keep adding compatibles to the driver for apparently >>>>>>>>> compatible devices? >>>>>>>> >>>>>>>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and >>>>>>> s/funny/fully >>>>>>> >>>>>>> unfortunate typo, this is not funny, quite the contrary.. >>>>>>> >>>>>>> Konrad >>>>>>>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >>>>>>>> of it, that's abstracted to us through various generations of their saddening software stack. This >>>>>>>> is also the case for many more standard components, even as far as the GIC on recent boards.. >>>>>> >>>>>> Unfortunately I don't get this explanation... you mean some other >>>>>> firmware requires Linux drivers to use specific compatibles instead of >>>>>> one fallback? >>>>> No, perhaps I misunderstood you. >>>>> >>>>>> >>>>>> All of these do not have driver data, so they are essentially compatible >>>>>> for Linux driver. Growing this list in the driver seems pointless. What >>>>>> is the benefit of growing driver with same entries, except more patches? >>>>> Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves >>>>> and consumer devices (such as MDSS). The situation is more complicated, because some >>>>> qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 >>>>> vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use >>>>> `qcom_smmu_impl` and some others need even more quirks on top of that (that generally >>>>> hurt performance or functionality, so we don't want them when they're unnecessary). >>>>> If all generations of qcom SMMU implementation that bear the same name behaved anywhere >>>>> near consistent, there would be no need for keeping this around, instead requiring only >>>>> "qcom,broken-smmu" or something". >>>> >>>> Hi, just stopping by to share my own thoughts. >>>> >>>> First, I don't mind if this series doesn't get applied as-is. After seeing >>>> the eMMC driver in its current state: >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-msm.c?h=v6.0-rc6#n2437 >>>> >>>> I can understand that the devicetree maintainers don't want to see new SoCs >>>> touching drivers unnecessarily. Second, I don't see enough quirks to >>>> justify needing all compatible strings in the driver (2 quirky SoCs >>>> compared to 16 total not counting adreno iommu): >>>> >>>> $ grep qcom, drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>> if (of_device_is_compatible(np, "qcom,msm8996-smmu-v2")) >>>> * All targets that use the qcom,adreno-smmu compatible string *should* >>>> { .compatible = "qcom,adreno" }, >>>> { .compatible = "qcom,mdp4" }, >>>> { .compatible = "qcom,mdss" }, >>>> { .compatible = "qcom,sc7180-mdss" }, >>>> { .compatible = "qcom,sc7180-mss-pil" }, >>>> { .compatible = "qcom,sc7280-mdss" }, >>>> { .compatible = "qcom,sc7280-mss-pil" }, >>>> { .compatible = "qcom,sc8180x-mdss" }, >>>> { .compatible = "qcom,sm8250-mdss" }, >>>> { .compatible = "qcom,sdm845-mdss" }, >>>> { .compatible = "qcom,sdm845-mss-pil" }, >>>> if (of_device_is_compatible(np, "qcom,sdm845-smmu-500")) >>>> { .compatible = "qcom,msm8998-smmu-v2" }, >>>> { .compatible = "qcom,qcm2290-smmu-500" }, >>>> { .compatible = "qcom,sc7180-smmu-500" }, >>>> { .compatible = "qcom,sc7280-smmu-500" }, >>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>> { .compatible = "qcom,sdm670-smmu-500" }, >>>> { .compatible = "qcom,sdm845-smmu-500" }, >>>> { .compatible = "qcom,sm6125-smmu-500" }, >>>> { .compatible = "qcom,sm6350-smmu-500" }, >>>> { .compatible = "qcom,sm6375-smmu-500" }, >>>> { .compatible = "qcom,sm8150-smmu-500" }, >>>> { .compatible = "qcom,sm8250-smmu-500" }, >>>> { .compatible = "qcom,sm8350-smmu-500" }, >>>> { .compatible = "qcom,sm8450-smmu-500" }, >>>> if (of_device_is_compatible(np, "qcom,adreno-smmu")) >>>> >>>> I don't know if it's better to get myself involved in fixing this, though. >>>> There is no fallback that encompasses qcom devices but not all arm devices. >>>> Either way, I'll have to add a new compatible string to the driver. >>>> >>>> If something like this is fine for now, I'll format it properly tomorrow: >>> >>> Please wait till we reach some conclusion otherwise your work might be >>> wasted. >>> >>>> >>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml >>>> @@ -48,6 +48,13 @@ properties: >>>> - qcom,sm8350-smmu-500 >>>> - qcom,sm8450-smmu-500 >>>> - const: arm,mmu-500 >>>> + >>>> + - description: Qcom SoCs implementing "qcom,smmu-500" >>>> + items: >>>> + - enum: >>>> + - qcom,sdm670-smmu-500 >>>> + - const: qcom,smmu-500 >>>> + >>> >>> Someone would have to confirm that smmu-500 is a real device >>> spec/version. Otherwise this should be device-specific compatible (e.g. >>> earliest in family). >> In my view it's hard to name it, downstream uses bool properties for enabling/disabling >> certain quirks and on different generations there are different combinations of that. >> Interestingly enough, I vaguely remember that the same quirk names can mean different >> things on different msm-X.Y versions.. Add to that, different msm-X.Y versions can have >> different assumptions on what's the default (aka without taking the bool properties into >> account) behaviour for a given compatible. > > Downstream does not care about ABI, coding style, reasonable approach, > so it should not wonder that they code things inconsistent. > > >> So I suppose "first in the family" would be >> the best way to go for mainline, though there are still quite a few families: >> >> <earlier ones used qcom_iommu> >> - 8996 with quirks that are already accounted for (or one may also say it works by miracle, >> just like msm8916 - downstream adds more special handling, but looks like the fw is not as >> restrictive) >> >> - 8996pro + 660 + 8998 with serious unmerged ones [1] >> >> - 845 which seems to be aok >> >> - special case of chromebooks where they only have qcom TZ/XPUs and not the hypervisor to >> fight with, so ma-a-aybe (no downstream reference & I don't have the hw to confirm) they >> can get away with less things >> >> - 8[1234]50 which seem to be a mix-and-match of less serious (read: not accounting for them >> may hurt performance but will not make your device sepuku at SMMU probe) minor quirks >> [2][3][4][5] (big warning: these may be overriden somewhere in other device tree fragments, >> the surest option would be to take a compiled dtb and decompile it to be sure about it) >> >> - 4xxx/6xxx series that mostly align with "whatever was there on the flagship soc released >> a year before" > > If the devices are really different, even though it is not visible in > Linux driver, then indeed there is no point for fake compatibility. I > raised the question only because the driver does not customize the > variants, but that might be not enough. If there are no problems with the original patch, could you please indicate that? I think this thread is getting a little lengthy for the other maintainers. > Linked DTSI use different quirks (assuming quirks would have same > meaning...), plus they have sometimes different amount of clocks, so in > total maybe it is not reasonable to make them compatible. On the other > hand, maybe the programming model is very, very similar thus Linux could > bind to one fallback and few different bits would be customized with > specific compatible. Not to rush you or anything. If you think this should be decided before the series goes in, it's not urgent. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-21 21:14 ` Konrad Dybcio 2022-09-22 2:38 ` Richard Acayan @ 2022-09-22 6:39 ` Krzysztof Kozlowski 2022-10-18 10:21 ` Dmitry Baryshkov 2 siblings, 0 replies; 17+ messages in thread From: Krzysztof Kozlowski @ 2022-09-22 6:39 UTC (permalink / raw) To: Konrad Dybcio, Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Dmitry Baryshkov, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 21/09/2022 23:14, Konrad Dybcio wrote: >> Unfortunately I don't get this explanation... you mean some other >> firmware requires Linux drivers to use specific compatibles instead of >> one fallback? > No, perhaps I misunderstood you. > >> >> All of these do not have driver data, so they are essentially compatible >> for Linux driver. Growing this list in the driver seems pointless. What >> is the benefit of growing driver with same entries, except more patches? > Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves > and consumer devices (such as MDSS). The situation is more complicated, because some > qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 > vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use > `qcom_smmu_impl` and some others need even more quirks on top of that (that generally > hurt performance or functionality, so we don't want them when they're unnecessary). > If all generations of qcom SMMU implementation that bear the same name behaved anywhere > near consistent, there would be no need for keeping this around, instead requiring only > "qcom,broken-smmu" or something". So where are the quirks? Again: driver data is empty. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible 2022-09-21 21:14 ` Konrad Dybcio 2022-09-22 2:38 ` Richard Acayan 2022-09-22 6:39 ` Krzysztof Kozlowski @ 2022-10-18 10:21 ` Dmitry Baryshkov 2 siblings, 0 replies; 17+ messages in thread From: Dmitry Baryshkov @ 2022-10-18 10:21 UTC (permalink / raw) To: Konrad Dybcio, Krzysztof Kozlowski, Richard Acayan, linux-arm-msm Cc: Will Deacon, Robin Murphy, Joerg Roedel, Rob Herring, Krzysztof Kozlowski, Bjorn Andersson, Sai Prakash Ranjan, Rob Clark, Sibi Sankar, Loic Poulain, Vinod Koul, iommu, devicetree On 22/09/2022 00:14, Konrad Dybcio wrote: > > > On 21.09.2022 21:05, Krzysztof Kozlowski wrote: >> On 21/09/2022 20:48, Konrad Dybcio wrote: >>> >>> >>> On 21.09.2022 20:47, Konrad Dybcio wrote: >>>> >>>> >>>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote: >>>>> On 21/09/2022 00:39, Richard Acayan wrote: >>>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to >>>>>> support it. >>>>>> >>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>>>>> --- >>>>>> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> index b2708de25ea3..bf9653b9eb89 100644 >>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c >>>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = { >>>>>> { .compatible = "qcom,sc8180x-smmu-500" }, >>>>>> { .compatible = "qcom,sc8280xp-smmu-500" }, >>>>>> { .compatible = "qcom,sdm630-smmu-v2" }, >>>>>> + { .compatible = "qcom,sdm670-smmu-500" }, >>>>> >>>>> Why do we keep adding compatibles to the driver for apparently >>>>> compatible devices? >>>> >>>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and >>> s/funny/fully >>> >>> unfortunate typo, this is not funny, quite the contrary.. >>> >>> Konrad >>>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation >>>> of it, that's abstracted to us through various generations of their saddening software stack. This >>>> is also the case for many more standard components, even as far as the GIC on recent boards.. >> >> Unfortunately I don't get this explanation... you mean some other >> firmware requires Linux drivers to use specific compatibles instead of >> one fallback? > No, perhaps I misunderstood you. > >> >> All of these do not have driver data, so they are essentially compatible >> for Linux driver. Growing this list in the driver seems pointless. What >> is the benefit of growing driver with same entries, except more patches? > Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves > and consumer devices (such as MDSS). The situation is more complicated, because some > qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998 > vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use > `qcom_smmu_impl` and some others need even more quirks on top of that (that generally > hurt performance or functionality, so we don't want them when they're unnecessary). > If all generations of qcom SMMU implementation that bear the same name behaved anywhere > near consistent, there would be no need for keeping this around, instead requiring only > "qcom,broken-smmu" or something". Excuse me for bumping this thread, I successfully forgot this message in drafts folder. Granted the driver, your pending smmu patches and the current list of quirks (which includes qcom,msm8996-smmu-v2 and qcom,sdm845-smmu-500, and Konrad's unmerged quirks for -v2), I'd second the suggestion of adding a single qcom,msm-smmu-500 (or qcom,arm-smmu-500) entry as a generic fallback. Downstream device trees support this name by using the -v500 suffix (yes, it's a light argument, but still). This way we can apply SoC-specific quirks, while still retaining the generic entry here. What's definitely has to be done is the merge of the platform data from arm-smmu-qcom-debug.c into arm-smmu-qcom.c. And having identical entries there just drives me crazy. I'll work on a patch later today. For -v2 we already have the generic compat, however granted the lack of proper quirks in place, I'd abstain from reworking this part for now. BTW: I'm trying to follow the commit a242f4297cfe ("iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c."). It adds msm8996-specific quirk, but we lack the msm8996 compat entry in the array. Does this work at all? > > Konrad >> >> Best regards, >> Krzysztof >> -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-10-18 10:21 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-20 22:39 [PATCH 0/2] iommu: SMMU for SDM670 Richard Acayan 2022-09-20 22:39 ` [PATCH 1/2] dt-bindings: iommu: arm-smmu: add sdm670 compatible Richard Acayan 2022-09-21 7:31 ` Krzysztof Kozlowski 2022-09-20 22:39 ` [PATCH 2/2] iommu: arm-smmu-qcom: " Richard Acayan 2022-09-20 23:45 ` Konrad Dybcio 2022-09-21 7:31 ` Krzysztof Kozlowski 2022-09-21 18:47 ` Konrad Dybcio 2022-09-21 18:48 ` Konrad Dybcio 2022-09-21 19:05 ` Krzysztof Kozlowski 2022-09-21 21:14 ` Konrad Dybcio 2022-09-22 2:38 ` Richard Acayan 2022-09-22 6:41 ` Krzysztof Kozlowski 2022-09-22 10:48 ` Konrad Dybcio 2022-09-22 12:18 ` Krzysztof Kozlowski 2022-09-23 22:24 ` Richard Acayan 2022-09-22 6:39 ` Krzysztof Kozlowski 2022-10-18 10:21 ` Dmitry Baryshkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).