| Message ID | 20230308155838.1094920-8-abel.vesa@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add dedicated Qcom ICE driver | expand |
On 08/03/2023 16:58, Abel Vesa wrote: > Drop all properties related to ICE from every UFS and SDCC node, > for all platforms, and add dedicated ICE nodes for each platform. > On most platforms, there is only one ICE instance, used by either > UFS or SDCC, but there are some platforms that have two separate > instances and, therefore, two separate nodes are added. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > > Changes since v1: > * Made changes for all platforms that use ICE, as a single patch since > most changes look really similar. > > arch/arm64/boot/dts/qcom/sdm630.dtsi | 18 +++++++++----- > arch/arm64/boot/dts/qcom/sdm670.dtsi | 15 +++++++---- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 21 +++++++++------- > arch/arm64/boot/dts/qcom/sm6115.dtsi | 37 +++++++++++++++++----------- > arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 ++++++++++++++--------- > arch/arm64/boot/dts/qcom/sm8150.dtsi | 21 +++++++++------- > arch/arm64/boot/dts/qcom/sm8450.dtsi | 22 ++++++++++------- > 7 files changed, 102 insertions(+), 63 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi > index 5827cda270a0..2aed49104d9d 100644 > --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi > @@ -1330,9 +1330,8 @@ opp-200000000 { > sdhc_1: mmc@c0c4000 { > compatible = "qcom,sdm630-sdhci", "qcom,sdhci-msm-v5"; > reg = <0x0c0c4000 0x1000>, > - <0x0c0c5000 0x1000>, > - <0x0c0c8000 0x8000>; > - reg-names = "hc", "cqhci", "ice"; > + <0x0c0c5000 0x1000>; > + reg-names = "hc", "cqhci"; I believe this will break the ICE on these platforms without valid reason. The commit msg does not explain why you do it or why this is necessary. We already we received comment that we keep breaking Qualcomm platforms all the time and need to keep them in some shape. Also, patchset is non-applicable in current set (breaks users) and neither commit nor cover letter mentions it. Best regards, Krzysztof
On Thu, Mar 09, 2023 at 11:31:46AM +0100, Krzysztof Kozlowski wrote: > On 08/03/2023 16:58, Abel Vesa wrote: > > Drop all properties related to ICE from every UFS and SDCC node, > > for all platforms, and add dedicated ICE nodes for each platform. > > On most platforms, there is only one ICE instance, used by either > > UFS or SDCC, but there are some platforms that have two separate > > instances and, therefore, two separate nodes are added. > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > > > > Changes since v1: > > * Made changes for all platforms that use ICE, as a single patch since > > most changes look really similar. > > > > arch/arm64/boot/dts/qcom/sdm630.dtsi | 18 +++++++++----- > > arch/arm64/boot/dts/qcom/sdm670.dtsi | 15 +++++++---- > > arch/arm64/boot/dts/qcom/sdm845.dtsi | 21 +++++++++------- > > arch/arm64/boot/dts/qcom/sm6115.dtsi | 37 +++++++++++++++++----------- > > arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 ++++++++++++++--------- > > arch/arm64/boot/dts/qcom/sm8150.dtsi | 21 +++++++++------- > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 22 ++++++++++------- > > 7 files changed, 102 insertions(+), 63 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi > > index 5827cda270a0..2aed49104d9d 100644 > > --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi > > @@ -1330,9 +1330,8 @@ opp-200000000 { > > sdhc_1: mmc@c0c4000 { > > compatible = "qcom,sdm630-sdhci", "qcom,sdhci-msm-v5"; > > reg = <0x0c0c4000 0x1000>, > > - <0x0c0c5000 0x1000>, > > - <0x0c0c8000 0x8000>; > > - reg-names = "hc", "cqhci", "ice"; > > + <0x0c0c5000 0x1000>; > > + reg-names = "hc", "cqhci"; > > I believe this will break the ICE on these platforms without valid > reason. The commit msg does not explain why you do it or why this is > necessary. > > We already we received comment that we keep breaking Qualcomm platforms > all the time and need to keep them in some shape. > > Also, patchset is non-applicable in current set (breaks users) and > neither commit nor cover letter mentions it. > FWIW, I tested this patchset on SDA845, and ICE continues to work fine. (Though if I understand the patchset correctly, the ICE clock is no longer turned off when the UFS host controller is suspended. That isn't ideal as it wastes power. I would like that to be fixed.) Anyway, when you say "break the ICE", do you really mean "make an incompatible change to the device-tree bindings"? I'd think there would be no problem with that as long as everything is updated at once, which this patchset does. I've heard before that some people consider the device-tree bindings to be a stable UAPI. That doesn't make sense to me. Actually, my original ICE patches ran into this issue too, and the resolution was simply that the Qualcomm platforms maintainer (Bjorn) decided to take the patches anyway. I never heard any complaints afterwards. Maybe the same is fine here too? - Eric
On 09/03/2023 19:31, Eric Biggers wrote: > On Thu, Mar 09, 2023 at 11:31:46AM +0100, Krzysztof Kozlowski wrote: >> On 08/03/2023 16:58, Abel Vesa wrote: >>> Drop all properties related to ICE from every UFS and SDCC node, >>> for all platforms, and add dedicated ICE nodes for each platform. >>> On most platforms, there is only one ICE instance, used by either >>> UFS or SDCC, but there are some platforms that have two separate >>> instances and, therefore, two separate nodes are added. >>> >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>> --- >>> >>> Changes since v1: >>> * Made changes for all platforms that use ICE, as a single patch since >>> most changes look really similar. >>> >>> arch/arm64/boot/dts/qcom/sdm630.dtsi | 18 +++++++++----- >>> arch/arm64/boot/dts/qcom/sdm670.dtsi | 15 +++++++---- >>> arch/arm64/boot/dts/qcom/sdm845.dtsi | 21 +++++++++------- >>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 37 +++++++++++++++++----------- >>> arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 ++++++++++++++--------- >>> arch/arm64/boot/dts/qcom/sm8150.dtsi | 21 +++++++++------- >>> arch/arm64/boot/dts/qcom/sm8450.dtsi | 22 ++++++++++------- >>> 7 files changed, 102 insertions(+), 63 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi >>> index 5827cda270a0..2aed49104d9d 100644 >>> --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi >>> @@ -1330,9 +1330,8 @@ opp-200000000 { >>> sdhc_1: mmc@c0c4000 { >>> compatible = "qcom,sdm630-sdhci", "qcom,sdhci-msm-v5"; >>> reg = <0x0c0c4000 0x1000>, >>> - <0x0c0c5000 0x1000>, >>> - <0x0c0c8000 0x8000>; >>> - reg-names = "hc", "cqhci", "ice"; >>> + <0x0c0c5000 0x1000>; >>> + reg-names = "hc", "cqhci"; >> >> I believe this will break the ICE on these platforms without valid >> reason. The commit msg does not explain why you do it or why this is >> necessary. >> >> We already we received comment that we keep breaking Qualcomm platforms >> all the time and need to keep them in some shape. >> >> Also, patchset is non-applicable in current set (breaks users) and >> neither commit nor cover letter mentions it. >> > > FWIW, I tested this patchset on SDA845, and ICE continues to work fine. Really? I clearly see of_find_device_by_node -> "return NULL" and all old code gone, so ABI is broken. Are you sure you applied patch 1-6 and ICE was working? > > (Though if I understand the patchset correctly, the ICE clock is no longer > turned off when the UFS host controller is suspended. That isn't ideal as it > wastes power. I would like that to be fixed.) > > Anyway, when you say "break the ICE", do you really mean "make an incompatible > change to the device-tree bindings"? It breaks existing users of DTS and kernel. > > I'd think there would be no problem with that as long as everything is updated > at once, which this patchset does. Which is obviously not possible. DTS always goes separate branch, always. It cannot be combined with code into the same branch! So how do you even imagine this? > > I've heard before that some people consider the device-tree bindings to be a > stable UAPI. That doesn't make sense to me. It is stable ABI. Bindings and DTS are used by other firmwares, bootloaders and systems. The kernel *must* work with old and out of tree DTS. Even if this does not make sense to you, these are the realities, practice and current rules. > Actually, my original ICE patches > ran into this issue too, and the resolution was simply that the Qualcomm > platforms maintainer (Bjorn) decided to take the patches anyway. I never heard > any complaints afterwards. Maybe the same is fine here too? No, it is not fine. The patchset breaks ABI, breaks existing kernel with old DTS and breaks other projects using DTS and bindings. Best regards, Krzysztof
On 23-03-10 09:13:29, Krzysztof Kozlowski wrote: > On 09/03/2023 19:31, Eric Biggers wrote: > > On Thu, Mar 09, 2023 at 11:31:46AM +0100, Krzysztof Kozlowski wrote: > >> On 08/03/2023 16:58, Abel Vesa wrote: > >>> Drop all properties related to ICE from every UFS and SDCC node, > >>> for all platforms, and add dedicated ICE nodes for each platform. > >>> On most platforms, there is only one ICE instance, used by either > >>> UFS or SDCC, but there are some platforms that have two separate > >>> instances and, therefore, two separate nodes are added. > >>> > >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > >>> --- > >>> > >>> Changes since v1: > >>> * Made changes for all platforms that use ICE, as a single patch since > >>> most changes look really similar. > >>> > >>> arch/arm64/boot/dts/qcom/sdm630.dtsi | 18 +++++++++----- > >>> arch/arm64/boot/dts/qcom/sdm670.dtsi | 15 +++++++---- > >>> arch/arm64/boot/dts/qcom/sdm845.dtsi | 21 +++++++++------- > >>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 37 +++++++++++++++++----------- > >>> arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 ++++++++++++++--------- > >>> arch/arm64/boot/dts/qcom/sm8150.dtsi | 21 +++++++++------- > >>> arch/arm64/boot/dts/qcom/sm8450.dtsi | 22 ++++++++++------- > >>> 7 files changed, 102 insertions(+), 63 deletions(-) > >>> > >>> diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi > >>> index 5827cda270a0..2aed49104d9d 100644 > >>> --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi > >>> +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi > >>> @@ -1330,9 +1330,8 @@ opp-200000000 { > >>> sdhc_1: mmc@c0c4000 { > >>> compatible = "qcom,sdm630-sdhci", "qcom,sdhci-msm-v5"; > >>> reg = <0x0c0c4000 0x1000>, > >>> - <0x0c0c5000 0x1000>, > >>> - <0x0c0c8000 0x8000>; > >>> - reg-names = "hc", "cqhci", "ice"; > >>> + <0x0c0c5000 0x1000>; > >>> + reg-names = "hc", "cqhci"; > >> > >> I believe this will break the ICE on these platforms without valid > >> reason. The commit msg does not explain why you do it or why this is > >> necessary. > >> > >> We already we received comment that we keep breaking Qualcomm platforms > >> all the time and need to keep them in some shape. > >> > >> Also, patchset is non-applicable in current set (breaks users) and > >> neither commit nor cover letter mentions it. > >> > > > > FWIW, I tested this patchset on SDA845, and ICE continues to work fine. > > Really? I clearly see of_find_device_by_node -> "return NULL" and all > old code gone, so ABI is broken. Are you sure you applied patch 1-6 and > ICE was working? of_qcom_ice_get will return the ICE instance if the consumer node has a qcom,ice property with a phandle for the ICE devicetree node. It will return NULL otherwise. SDA845 has such ICE node added by this patch, therefore, it will work. All platforms that have such node will work functionally like before. But I'll take care of the legacy approach as well in v3 (see below). > > > > > (Though if I understand the patchset correctly, the ICE clock is no longer > > turned off when the UFS host controller is suspended. That isn't ideal as it > > wastes power. I would like that to be fixed.) > > > > Anyway, when you say "break the ICE", do you really mean "make an incompatible > > change to the device-tree bindings"? > > It breaks existing users of DTS and kernel. I assume you mean it breaks if someone is using old approach DTS with a kernel that would have ICE driver merged. Yes, that it does. And for that, in the v3, I'll make of_qcom_ice_get check if there is a reg entry with name "ice" and create an ICE instance but for the same dev as the consumer driver. OTOH, if there is no reg entry called "ice", it will look up a device based on phande of qcom,ice property. This will allow legacy style DTS to work fine, while using the unified driver as a library, in that case. For newer platforms, the recommended approach will be to add a new ICE node and use qcom,ice property. > > > > > I'd think there would be no problem with that as long as everything is updated > > at once, which this patchset does. > > Which is obviously not possible. DTS always goes separate branch, > always. It cannot be combined with code into the same branch! So how do > you even imagine this? > > > > > I've heard before that some people consider the device-tree bindings to be a > > stable UAPI. That doesn't make sense to me. > > It is stable ABI. Bindings and DTS are used by other firmwares, > bootloaders and systems. The kernel *must* work with old and out of tree > DTS. > > Even if this does not make sense to you, these are the realities, > practice and current rules. > > > > Actually, my original ICE patches > > ran into this issue too, and the resolution was simply that the Qualcomm > > platforms maintainer (Bjorn) decided to take the patches anyway. I never heard > > any complaints afterwards. Maybe the same is fine here too? > > > No, it is not fine. The patchset breaks ABI, breaks existing kernel with > old DTS and breaks other projects using DTS and bindings. > > Best regards, > Krzysztof >
On 10/03/2023 10:21, Abel Vesa wrote: >>>>> compatible = "qcom,sdm630-sdhci", "qcom,sdhci-msm-v5"; >>>>> reg = <0x0c0c4000 0x1000>, >>>>> - <0x0c0c5000 0x1000>, >>>>> - <0x0c0c8000 0x8000>; >>>>> - reg-names = "hc", "cqhci", "ice"; >>>>> + <0x0c0c5000 0x1000>; >>>>> + reg-names = "hc", "cqhci"; >>>> >>>> I believe this will break the ICE on these platforms without valid >>>> reason. The commit msg does not explain why you do it or why this is >>>> necessary. >>>> >>>> We already we received comment that we keep breaking Qualcomm platforms >>>> all the time and need to keep them in some shape. >>>> >>>> Also, patchset is non-applicable in current set (breaks users) and >>>> neither commit nor cover letter mentions it. >>>> >>> >>> FWIW, I tested this patchset on SDA845, and ICE continues to work fine. >> >> Really? I clearly see of_find_device_by_node -> "return NULL" and all >> old code gone, so ABI is broken. Are you sure you applied patch 1-6 and >> ICE was working? > > of_qcom_ice_get will return the ICE instance if the consumer node has a > qcom,ice property with a phandle for the ICE devicetree node. When patches 1-6 are applied, there is no qcom,ice property in DTS. Thus I don't consider the test as correct... Even if we skip entire ABI discussion the patchset is non-bisectable thus the test was failing to detect even that. > It will > return NULL otherwise. SDA845 has such ICE node added by this patch, > therefore, it will work. All platforms that have such node will work > functionally like before. But I'll take care of the legacy approach as > well in v3 (see below). At point of patch 6 none of nodes have it. That's the entire point of bisectability. What's more, if you reverse code and makes DTS patches before driver hoping to fix bisectability - do you see ICE working on existing platforms? I don't think it so... > >> >>> >>> (Though if I understand the patchset correctly, the ICE clock is no longer >>> turned off when the UFS host controller is suspended. That isn't ideal as it >>> wastes power. I would like that to be fixed.) >>> >>> Anyway, when you say "break the ICE", do you really mean "make an incompatible >>> change to the device-tree bindings"? >> >> It breaks existing users of DTS and kernel. > > I assume you mean it breaks if someone is using old approach DTS with a > kernel that would have ICE driver merged. Yes, that it does. And for > that, in the v3, I'll make of_qcom_ice_get check if there is a reg entry > with name "ice" and create an ICE instance but for the same dev as the > consumer driver. OTOH, if there is no reg entry called "ice", it will > look up a device based on phande of qcom,ice property. This will allow > legacy style DTS to work fine, while using the unified driver as a > library, in that case. For newer platforms, the recommended approach > will be to add a new ICE node and use qcom,ice property. For the driver this sounds good. I still think that existing (older) DTS should not have regs removed, because this affects other users of kernel DTS. Best regards, Krzysztof
On 23-03-10 10:27:12, Krzysztof Kozlowski wrote: > On 10/03/2023 10:21, Abel Vesa wrote: > >>>>> compatible = "qcom,sdm630-sdhci", "qcom,sdhci-msm-v5"; > >>>>> reg = <0x0c0c4000 0x1000>, > >>>>> - <0x0c0c5000 0x1000>, > >>>>> - <0x0c0c8000 0x8000>; > >>>>> - reg-names = "hc", "cqhci", "ice"; > >>>>> + <0x0c0c5000 0x1000>; > >>>>> + reg-names = "hc", "cqhci"; > >>>> > >>>> I believe this will break the ICE on these platforms without valid > >>>> reason. The commit msg does not explain why you do it or why this is > >>>> necessary. > >>>> > >>>> We already we received comment that we keep breaking Qualcomm platforms > >>>> all the time and need to keep them in some shape. > >>>> > >>>> Also, patchset is non-applicable in current set (breaks users) and > >>>> neither commit nor cover letter mentions it. > >>>> > >>> > >>> FWIW, I tested this patchset on SDA845, and ICE continues to work fine. > >> > >> Really? I clearly see of_find_device_by_node -> "return NULL" and all > >> old code gone, so ABI is broken. Are you sure you applied patch 1-6 and > >> ICE was working? > > > > of_qcom_ice_get will return the ICE instance if the consumer node has a > > qcom,ice property with a phandle for the ICE devicetree node. > > When patches 1-6 are applied, there is no qcom,ice property in DTS. Thus > I don't consider the test as correct... Even if we skip entire ABI > discussion the patchset is non-bisectable thus the test was failing to > detect even that. Yeah, but that could've been solved easily like I explained yesterday on irc. But that's not worth getting into anymore as I'll keep legacy working as I explained. > > > It will > > return NULL otherwise. SDA845 has such ICE node added by this patch, > > therefore, it will work. All platforms that have such node will work > > functionally like before. But I'll take care of the legacy approach as > > well in v3 (see below). > > At point of patch 6 none of nodes have it. That's the entire point of > bisectability. > > What's more, if you reverse code and makes DTS patches before driver > hoping to fix bisectability - do you see ICE working on existing > platforms? I don't think it so... > > > > >> > >>> > >>> (Though if I understand the patchset correctly, the ICE clock is no longer > >>> turned off when the UFS host controller is suspended. That isn't ideal as it > >>> wastes power. I would like that to be fixed.) > >>> > >>> Anyway, when you say "break the ICE", do you really mean "make an incompatible > >>> change to the device-tree bindings"? > >> > >> It breaks existing users of DTS and kernel. > > > > I assume you mean it breaks if someone is using old approach DTS with a > > kernel that would have ICE driver merged. Yes, that it does. And for > > that, in the v3, I'll make of_qcom_ice_get check if there is a reg entry > > with name "ice" and create an ICE instance but for the same dev as the > > consumer driver. OTOH, if there is no reg entry called "ice", it will > > look up a device based on phande of qcom,ice property. This will allow > > legacy style DTS to work fine, while using the unified driver as a > > library, in that case. For newer platforms, the recommended approach > > will be to add a new ICE node and use qcom,ice property. > > For the driver this sounds good. I still think that existing (older) DTS > should not have regs removed, because this affects other users of kernel > DTS. Yes, that's what I meant, the already supported platforms will remain with ice reg in the consumer node. > > > Best regards, > Krzysztof >
On 10/03/2023 10:34, Abel Vesa wrote: >>> I assume you mean it breaks if someone is using old approach DTS with a >>> kernel that would have ICE driver merged. Yes, that it does. And for >>> that, in the v3, I'll make of_qcom_ice_get check if there is a reg entry >>> with name "ice" and create an ICE instance but for the same dev as the >>> consumer driver. OTOH, if there is no reg entry called "ice", it will >>> look up a device based on phande of qcom,ice property. This will allow >>> legacy style DTS to work fine, while using the unified driver as a >>> library, in that case. For newer platforms, the recommended approach >>> will be to add a new ICE node and use qcom,ice property. >> >> For the driver this sounds good. I still think that existing (older) DTS >> should not have regs removed, because this affects other users of kernel >> DTS. > > Yes, that's what I meant, the already supported platforms will remain > with ice reg in the consumer node. ... unless you plan to add to them UFS ICE, which would be nice feature thus justifying DTS re-shuffle :) Best regards, Krzysztof
On 23-03-10 10:37:42, Krzysztof Kozlowski wrote: > On 10/03/2023 10:34, Abel Vesa wrote: > >>> I assume you mean it breaks if someone is using old approach DTS with a > >>> kernel that would have ICE driver merged. Yes, that it does. And for > >>> that, in the v3, I'll make of_qcom_ice_get check if there is a reg entry > >>> with name "ice" and create an ICE instance but for the same dev as the > >>> consumer driver. OTOH, if there is no reg entry called "ice", it will > >>> look up a device based on phande of qcom,ice property. This will allow > >>> legacy style DTS to work fine, while using the unified driver as a > >>> library, in that case. For newer platforms, the recommended approach > >>> will be to add a new ICE node and use qcom,ice property. > >> > >> For the driver this sounds good. I still think that existing (older) DTS > >> should not have regs removed, because this affects other users of kernel > >> DTS. > > > > Yes, that's what I meant, the already supported platforms will remain > > with ice reg in the consumer node. > > ... unless you plan to add to them UFS ICE, which would be nice feature > thus justifying DTS re-shuffle :) By supported I meant the ones that have ICE support already :) > > Best regards, > Krzysztof >
On Fri, Mar 10, 2023 at 09:13:29AM +0100, Krzysztof Kozlowski wrote: > > FWIW, I tested this patchset on SDA845, and ICE continues to work fine. > > Really? Yes. > I clearly see of_find_device_by_node -> "return NULL" and all old code gone, > so ABI is broken. Are you sure you applied patch 1-6 and ICE was working? Yes. But I applied the whole series. It sounds like you are talking about the case where *only* patches 1-6 are applied? I did not test that, sorry. > > (Though if I understand the patchset correctly, the ICE clock is no longer > > turned off when the UFS host controller is suspended. That isn't ideal as it > > wastes power. I would like that to be fixed.) > > > > Anyway, when you say "break the ICE", do you really mean "make an incompatible > > change to the device-tree bindings"? > > It breaks existing users of DTS and kernel. > > > > > I'd think there would be no problem with that as long as everything is updated > > at once, which this patchset does. > > Which is obviously not possible. DTS always goes separate branch, > always. It cannot be combined with code into the same branch! So how do > you even imagine this? It is not obvious. DTS and drivers are both in the kernel source tree. It's natural to think they can be updated together as per Documentation/process/stable-api-nonsense.rst. I'm *not* saying they can be -- you're the expert here, and it sounds like DTS are considered a stable UAPI. I'm merely saying that it's not obvious... especially when my personal experience is that reviewers explicitly preferred a patch breaking DTS compatibility to a patch not breaking DTS compatibility (https://lore.kernel.org/linux-scsi/20200710072013.177481-1-ebiggers@kernel.org/T/#u) Anyway, based on the follow-up replies, it sounds like Abel has a plan to keep compatibility with old DTS, so hopefully that works and isn't too much trouble. - Eric
diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi index 5827cda270a0..2aed49104d9d 100644 --- a/arch/arm64/boot/dts/qcom/sdm630.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi @@ -1330,9 +1330,8 @@ opp-200000000 { sdhc_1: mmc@c0c4000 { compatible = "qcom,sdm630-sdhci", "qcom,sdhci-msm-v5"; reg = <0x0c0c4000 0x1000>, - <0x0c0c5000 0x1000>, - <0x0c0c8000 0x8000>; - reg-names = "hc", "cqhci", "ice"; + <0x0c0c5000 0x1000>; + reg-names = "hc", "cqhci"; interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; @@ -1340,9 +1339,8 @@ sdhc_1: mmc@c0c4000 { clocks = <&gcc GCC_SDCC1_AHB_CLK>, <&gcc GCC_SDCC1_APPS_CLK>, - <&xo_board>, - <&gcc GCC_SDCC1_ICE_CORE_CLK>; - clock-names = "iface", "core", "xo", "ice"; + <&xo_board>; + clock-names = "iface", "core", "xo"; interconnects = <&a2noc 2 &a2noc 10>, <&gnoc 0 &cnoc 27>; @@ -1353,6 +1351,8 @@ sdhc_1: mmc@c0c4000 { pinctrl-1 = <&sdc1_state_off>; power-domains = <&rpmpd SDM660_VDDCX>; + qcom,ice = <&ice>; + bus-width = <8>; non-removable; @@ -1382,6 +1382,12 @@ opp-384000000 { }; }; + ice: inline-crypto-engine@c0c8000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0x0c0c8000 0x8000>; + clocks = <&gcc GCC_SDCC1_ICE_CORE_CLK>; + }; + usb2: usb@c2f8800 { compatible = "qcom,sdm660-dwc3", "qcom,dwc3"; reg = <0x0c2f8800 0x400>; diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi index 02f14692dd9d..7c1c01a8fdae 100644 --- a/arch/arm64/boot/dts/qcom/sdm670.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi @@ -416,9 +416,8 @@ qusb2_hstx_trim: hstx-trim@1eb { sdhc_1: mmc@7c4000 { compatible = "qcom,sdm670-sdhci", "qcom,sdhci-msm-v5"; reg = <0 0x007c4000 0 0x1000>, - <0 0x007c5000 0 0x1000>, - <0 0x007c8000 0 0x8000>; - reg-names = "hc", "cqhci", "ice"; + <0 0x007c5000 0 0x1000>; + reg-names = "hc", "cqhci"; interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>; @@ -427,9 +426,8 @@ sdhc_1: mmc@7c4000 { clocks = <&gcc GCC_SDCC1_AHB_CLK>, <&gcc GCC_SDCC1_APPS_CLK>, <&rpmhcc RPMH_CXO_CLK>, - <&gcc GCC_SDCC1_ICE_CORE_CLK>, <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>; - clock-names = "iface", "core", "xo", "ice", "bus"; + clock-names = "iface", "core", "xo", "bus"; iommus = <&apps_smmu 0x140 0xf>; @@ -440,10 +438,17 @@ sdhc_1: mmc@7c4000 { bus-width = <8>; non-removable; + qcom,ice = <&ice>; status = "disabled"; }; + ice: inline-crypto-engine@7c8000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x007c8000 0 0x8000>; + clocks = <&gcc GCC_SDCC1_ICE_CORE_CLK>; + }; + gpi_dma0: dma-controller@800000 { #dma-cells = <3>; compatible = "qcom,sdm670-gpi-dma", "qcom,sdm845-gpi-dma"; diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index 479859bd8ab3..80cf76dc612c 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -2543,9 +2543,8 @@ mmss_noc: interconnect@1740000 { ufs_mem_hc: ufshc@1d84000 { compatible = "qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; - reg = <0 0x01d84000 0 0x2500>, - <0 0x01d90000 0 0x8000>; - reg-names = "std", "ice"; + reg = <0 0x01d84000 0 0x2500>; + reg-names = "std"; interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; phys = <&ufs_mem_phy_lanes>; phy-names = "ufsphy"; @@ -2565,8 +2564,7 @@ ufs_mem_hc: ufshc@1d84000 { "ref_clk", "tx_lane0_sync_clk", "rx_lane0_sync_clk", - "rx_lane1_sync_clk", - "ice_core_clk"; + "rx_lane1_sync_clk"; clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, @@ -2575,8 +2573,7 @@ ufs_mem_hc: ufshc@1d84000 { <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, - <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>, - <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; freq-table-hz = <50000000 200000000>, <0 0>, @@ -2585,12 +2582,18 @@ ufs_mem_hc: ufshc@1d84000 { <0 0>, <0 0>, <0 0>, - <0 0>, - <0 300000000>; + <0 0>; + qcom,ice = <&ice>; status = "disabled"; }; + ice: inline-crypto-engine@1d90000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x01d90000 0 0x8000>; + clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + }; + ufs_mem_phy: phy@1d87000 { compatible = "qcom,sdm845-qmp-ufs-phy"; reg = <0 0x01d87000 0 0x18c>; diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi index 4d6ec815b78b..0ac12c839bc1 100644 --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi @@ -717,9 +717,8 @@ sram@4690000 { sdhc_1: mmc@4744000 { compatible = "qcom,sm6115-sdhci", "qcom,sdhci-msm-v5"; reg = <0x0 0x04744000 0x0 0x1000>, - <0x0 0x04745000 0x0 0x1000>, - <0x0 0x04748000 0x0 0x8000>; - reg-names = "hc", "cqhci", "ice"; + <0x0 0x04745000 0x0 0x1000>; + reg-names = "hc", "cqhci"; interrupts = <GIC_SPI 348 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 352 IRQ_TYPE_LEVEL_HIGH>; @@ -727,18 +726,24 @@ sdhc_1: mmc@4744000 { clocks = <&gcc GCC_SDCC1_AHB_CLK>, <&gcc GCC_SDCC1_APPS_CLK>, - <&rpmcc RPM_SMD_XO_CLK_SRC>, - <&gcc GCC_SDCC1_ICE_CORE_CLK>; - clock-names = "iface", "core", "xo", "ice"; + <&rpmcc RPM_SMD_XO_CLK_SRC>; + clock-names = "iface", "core", "xo"; pinctrl-0 = <&sdc1_state_on>; pinctrl-1 = <&sdc1_state_off>; pinctrl-names = "default", "sleep"; bus-width = <8>; + qcom,ice = <&sdhc_ice>; status = "disabled"; }; + sdhc_ice: inline-crypto-engine@4748000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x04748000 0 0x8000>; + clocks = <&gcc GCC_SDCC1_ICE_CORE_CLK>; + }; + sdhc_2: mmc@4784000 { compatible = "qcom,sm6115-sdhci", "qcom,sdhci-msm-v5"; reg = <0x0 0x04784000 0x0 0x1000>; @@ -784,8 +789,8 @@ opp-202000000 { ufs_mem_hc: ufs@4804000 { compatible = "qcom,sm6115-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; - reg = <0x0 0x04804000 0x0 0x3000>, <0x0 0x04810000 0x0 0x8000>; - reg-names = "std", "ice"; + reg = <0x0 0x04804000 0x0 0x3000>; + reg-names = "std"; interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>; phys = <&ufs_mem_phy_lanes>; phy-names = "ufsphy"; @@ -803,16 +808,14 @@ ufs_mem_hc: ufs@4804000 { <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, <&rpmcc RPM_SMD_XO_CLK_SRC>, <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, - <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, - <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>; clock-names = "core_clk", "bus_aggr_clk", "iface_clk", "core_clk_unipro", "ref_clk", "tx_lane0_sync_clk", - "rx_lane0_sync_clk", - "ice_core_clk"; + "rx_lane0_sync_clk"; freq-table-hz = <50000000 200000000>, <0 0>, @@ -820,12 +823,18 @@ ufs_mem_hc: ufs@4804000 { <37500000 150000000>, <0 0>, <0 0>, - <0 0>, - <75000000 300000000>; + <0 0>; + qcom,ice = <&ufs_ice>; status = "disabled"; }; + ufs_ice: inline-crypto-engine@4810000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x04810000 0 0x8000>; + clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + }; + ufs_mem_phy: phy@4807000 { compatible = "qcom,sm6115-qmp-ufs-phy"; reg = <0x0 0x04807000 0x0 0x1c4>; diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi index 1e1d366c92c1..ed28f8e3626b 100644 --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi @@ -638,9 +638,8 @@ rng: rng@793000 { sdhc_1: mmc@7c4000 { compatible = "qcom,sm6350-sdhci", "qcom,sdhci-msm-v5"; reg = <0 0x007c4000 0 0x1000>, - <0 0x007c5000 0 0x1000>, - <0 0x007c8000 0 0x8000>; - reg-names = "hc", "cqhci", "ice"; + <0 0x007c5000 0 0x1000>; + reg-names = "hc", "cqhci"; interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>; @@ -659,6 +658,7 @@ sdhc_1: mmc@7c4000 { bus-width = <8>; non-removable; supports-cqe; + qcom,ice = <&sdhc_ice>; status = "disabled"; @@ -682,6 +682,12 @@ opp-384000000 { }; }; + sdhc_ice: inline-crypto-engine@c0c8000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x007c8000 0 0x8000>; + clocks = <&gcc GCC_SDCC1_ICE_CORE_CLK>; + }; + gpi_dma0: dma-controller@800000 { compatible = "qcom,sm6350-gpi-dma"; reg = <0 0x00800000 0 0x60000>; @@ -933,9 +939,8 @@ mmss_noc: interconnect@1740000 { ufs_mem_hc: ufs@1d84000 { compatible = "qcom,sm6350-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; - reg = <0 0x01d84000 0 0x3000>, - <0 0x01d90000 0 0x8000>; - reg-names = "std", "ice"; + reg = <0 0x01d84000 0 0x3000>; + reg-names = "std"; interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; phys = <&ufs_mem_phy_lanes>; phy-names = "ufsphy"; @@ -955,8 +960,7 @@ ufs_mem_hc: ufs@1d84000 { "ref_clk", "tx_lane0_sync_clk", "rx_lane0_sync_clk", - "rx_lane1_sync_clk", - "ice_core_clk"; + "rx_lane1_sync_clk"; clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, <&gcc GCC_UFS_PHY_AHB_CLK>, @@ -964,8 +968,7 @@ ufs_mem_hc: ufs@1d84000 { <&rpmhcc RPMH_QLINK_CLK>, <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, - <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>, - <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; freq-table-hz = <50000000 200000000>, <0 0>, @@ -974,8 +977,8 @@ ufs_mem_hc: ufs@1d84000 { <75000000 300000000>, <0 0>, <0 0>, - <0 0>, <0 0>; + qcom,ice = <&ufs_ice>; status = "disabled"; }; @@ -1007,6 +1010,12 @@ ufs_mem_phy_lanes: phy@1d87400 { }; }; + ufs_ice: inline-crypto-engine@1d90000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x01d90000 0 0x8000>; + clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + }; + ipa: ipa@1e40000 { compatible = "qcom,sm6350-ipa"; diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi index fd20096cfc6e..844c7b80d205 100644 --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi @@ -1983,9 +1983,8 @@ pcie1_lane: phy@1c0e200 { ufs_mem_hc: ufshc@1d84000 { compatible = "qcom,sm8150-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; - reg = <0 0x01d84000 0 0x2500>, - <0 0x01d90000 0 0x8000>; - reg-names = "std", "ice"; + reg = <0 0x01d84000 0 0x2500>; + reg-names = "std"; interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; phys = <&ufs_mem_phy_lanes>; phy-names = "ufsphy"; @@ -2004,8 +2003,7 @@ ufs_mem_hc: ufshc@1d84000 { "ref_clk", "tx_lane0_sync_clk", "rx_lane0_sync_clk", - "rx_lane1_sync_clk", - "ice_core_clk"; + "rx_lane1_sync_clk"; clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, @@ -2014,8 +2012,7 @@ ufs_mem_hc: ufshc@1d84000 { <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, - <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>, - <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; freq-table-hz = <37500000 300000000>, <0 0>, @@ -2024,8 +2021,8 @@ ufs_mem_hc: ufshc@1d84000 { <0 0>, <0 0>, <0 0>, - <0 0>, - <0 300000000>; + <0 0>; + qcom,ice = <&ice>; status = "disabled"; }; @@ -2057,6 +2054,12 @@ ufs_mem_phy_lanes: phy@1d87400 { }; }; + ice: inline-crypto-engine@1d90000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x01d90000 0 0x8000>; + clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + }; + tcsr_mutex: hwlock@1f40000 { compatible = "qcom,tcsr-mutex"; reg = <0x0 0x01f40000 0x0 0x20000>; diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi index 1a744a33bcf4..8ebe6184a9c3 100644 --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi @@ -3989,9 +3989,8 @@ system-cache-controller@19200000 { ufs_mem_hc: ufshc@1d84000 { compatible = "qcom,sm8450-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; - reg = <0 0x01d84000 0 0x3000>, - <0 0x01d88000 0 0x8000>; - reg-names = "std", "ice"; + reg = <0 0x01d84000 0 0x3000>; + reg-names = "std"; interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>; phys = <&ufs_mem_phy_lanes>; phy-names = "ufsphy"; @@ -4015,8 +4014,7 @@ ufs_mem_hc: ufshc@1d84000 { "ref_clk", "tx_lane0_sync_clk", "rx_lane0_sync_clk", - "rx_lane1_sync_clk", - "ice_core_clk"; + "rx_lane1_sync_clk"; clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>, @@ -4025,8 +4023,7 @@ ufs_mem_hc: ufshc@1d84000 { <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, - <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>, - <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>; freq-table-hz = <75000000 300000000>, <0 0>, @@ -4035,8 +4032,9 @@ ufs_mem_hc: ufshc@1d84000 { <75000000 300000000>, <0 0>, <0 0>, - <0 0>, - <75000000 300000000>; + <0 0>; + qcom,ice = <&ice>; + status = "disabled"; }; @@ -4066,6 +4064,12 @@ ufs_mem_phy_lanes: phy@1d87400 { }; }; + ice: inline-crypto-engine@1d88000 { + compatible = "qcom,inline-crypto-engine"; + reg = <0 0x01d88000 0 0x8000>; + clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + }; + sdhc_2: mmc@8804000 { compatible = "qcom,sm8450-sdhci", "qcom,sdhci-msm-v5"; reg = <0 0x08804000 0 0x1000>;
Drop all properties related to ICE from every UFS and SDCC node, for all platforms, and add dedicated ICE nodes for each platform. On most platforms, there is only one ICE instance, used by either UFS or SDCC, but there are some platforms that have two separate instances and, therefore, two separate nodes are added. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- Changes since v1: * Made changes for all platforms that use ICE, as a single patch since most changes look really similar. arch/arm64/boot/dts/qcom/sdm630.dtsi | 18 +++++++++----- arch/arm64/boot/dts/qcom/sdm670.dtsi | 15 +++++++---- arch/arm64/boot/dts/qcom/sdm845.dtsi | 21 +++++++++------- arch/arm64/boot/dts/qcom/sm6115.dtsi | 37 +++++++++++++++++----------- arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 ++++++++++++++--------- arch/arm64/boot/dts/qcom/sm8150.dtsi | 21 +++++++++------- arch/arm64/boot/dts/qcom/sm8450.dtsi | 22 ++++++++++------- 7 files changed, 102 insertions(+), 63 deletions(-)