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 >
