On 20/06/2022 17:50, Nícolas F. R. A. Prado wrote: > On Mon, Jun 20, 2022 at 10:50:57AM +0200, Krzysztof Kozlowski wrote: >> On 20/06/2022 08:59, Chunfeng Yun wrote: >>> On Sun, 2022-06-19 at 14:05 +0200, Krzysztof Kozlowski wrote: >>>> On 19/06/2022 09:46, Chunfeng Yun wrote: >>>>> On Fri, 2022-06-17 at 18:25 -0700, Krzysztof Kozlowski wrote: >>>>>> On 17/06/2022 15:29, Nícolas F. R. A. Prado wrote: >>>>>>> The current clock list in the binding doesn't allow for one of >>>>>>> the >>>>>>> optional clocks to be missing and a subsequent clock to be >>>>>>> present. >>>>>>> An >>>>>>> example where this is an issue is in mt8192.dtsi, which has >>>>>>> "sys_ck", >>>>>>> "ref_ck", "xhci_ck" and would cause dtbs_check warnings. >>>>>>> >>>>>>> Change the clock list in a way that allows the middle optional >>>>>>> clocks to >>>>>>> be missing, while still guaranteeing a fixed order. The >>>>>>> "ref_ck" is >>>>>>> kept >>>>>>> as a const even though it is optional for simplicity, since it >>>>>>> is >>>>>>> present in all current dts files. >>>>>>> >>>>>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx> >>>>>>> --- >>>>>>> >>>>>>> .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml | 9 >>>>>>> +++++++-- >>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/usb/mediatek,mtk- >>>>>>> xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk- >>>>>>> xhci.yaml >>>>>>> index 63cbc2b62d18..99a1b233ec90 100644 >>>>>>> --- a/Documentation/devicetree/bindings/usb/mediatek,mtk- >>>>>>> xhci.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/usb/mediatek,mtk- >>>>>>> xhci.yaml >>>>>>> @@ -80,8 +80,13 @@ properties: >>>>>>> items: >>>>>>> - const: sys_ck # required, the following ones are >>>>>>> optional >>>>>>> - const: ref_ck >>>>>>> - - const: mcu_ck >>>>>>> - - const: dma_ck >>>>>>> + - enum: >>>>>>> + - mcu_ck >>>>>>> + - dma_ck >>>>>>> + - xhci_ck >>>>>>> + - enum: >>>>>>> + - dma_ck >>>>>>> + - xhci_ck >>>>>>> - const: xhci_ck >>>>>> >>>>>> You allow now almost any order here, including incorrect like >>>>>> sys,ref,xhci,xhci,xhci. >>>>>> >>>>>> The order of clocks has to be fixed and we cannot allow >>>>>> flexibility. >>>>>> Are >>>>>> you sure that these clocks are actually optional (not wired to >>>>>> the >>>>>> device)? >>>>> >>>>> In fact, these optional clocks are fixed, due to no gates are >>>>> provided, >>>>> SW can't control them by CCF; >>>>> In this case, I usually use a fixed clock, or ignore it. >>>> >>>> But in some versions these clocks are controllable or not? >>> Some SoCs are controllable, some ones are not (fixed clock). >> >> Thanks for confirming. Then I would prefer to make these clocks required >> (not optional) and always provide them - via common clock framework or >> fixed-clock. > > Hi Krzysztof and Chunfeng, > > thank you both for the feedback. > > Since the solution I proposed in this patch is not acceptable I see two options: > 1. Split the clocks in several if blocks matched by compatibles > 2. Make the clocks required and use fixed-clock nodes for the missing clocks in > the DT > > My understanding is that 1 is the desirable solution if the clock is really > missing in some hardware variants, while 2 is desirable if all hardware variants > really receive all the clocks, only that on some variants they're fixed and not > controlable by SW. > > From what I'm reading of this discussion it seems that the latter is the case > here and thus we should go for 2. Is this correct? This is how I understood it as well, so correct from my side. > > Also Chunfeng, do you have information on whether the same is true for the MMC > HW block? I recently submitted some changes to that binding [1] but I followed > approach 1 there instead. However if all the clocks are present in the HW level > there as well it would make more sense for me to change it to follow approach 2. > > Thanks, > Nícolas > > [1] https://lore.kernel.org/all/20220617230114.2438875-1-nfraprado@xxxxxxxxxxxxx Best regards, Krzysztof
