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
