Hi Krzysztof, ---- On Thu, 26 Jan 2023 19:29:05 +0800 Krzysztof Kozlowski wrote --- > On 25/01/2023 14:40, Li Chen wrote: > > On Wed, 25 Jan 2023 20:14:16 +0800, > > > > Hi Krzysztof, > > > > Krzysztof Kozlowski wrote: > >> > >> On 25/01/2023 13:06, Li Chen wrote: > >>>>> Feel free to correct me if you think this > >>>>> is not a good idea. > >>>> > >>>> This is bad idea. Compatibles should be specific. Devices should not use > >>>> syscons to poke other registers, unless strictly necessary, but have > >>>> strictly defined MMIO address space and use it. > >>> > >>> Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data. > >>> > >>> But I have three questions: > >>> > >>> 0. why syscon + offsets is a bad idea copared to specific compatibles? > >> > >> Specific compatibles are a requirement. They are needed to match device > >> in exact way, not some generic and unspecific. The same with every other > >> interface, it must be specific to allow only correct usage. > >> > >> It's of course different with generic fallbacks, but we do not talk > >> about them here... > >> > >>> 1. when would it be a good idea to use syscon in device tree? > >> > >> When your device needs to poke one or few registers from some > >> system-controller block. > >> > >>> 2. syscon VS reg, which is preferred in device tree? > >> > >> There is no such choice. Your DTS *must* describe the hardware. The > >> hardware description is for example clock controller which has its own > >> address space. If you now do not add clock controller's address space to > >> the clock controller, it is not a proper hardware description. The same > >> with every other property. If your device has interrupts, but you do not > >> add them, it is not correct description. > > > > Got it. But Ambarella hardware design is kind of strange. I want to add mroe > > expalaination about why Ambarella's downstream kernel > > use so much syscon in device trees: > > > > For most SoCs from other vendors, they have seperate address space regions > > for different peripherals, like > > axi address space A: ENET > > axi address space B: PCIe > > axi address space B: USB > > ... > > > > Ambarella is somewhat **different**, its SoCs have two system controllers regions: > > RCT and scratchpad, take RCT for example: > > "The S6LM system software > > interacts with PLLs, PHYs and several other low-level hardware blocks using APB reset clock and test (RCT) > > registers with a system-layer application programming interface (API). > > This includes the setting of clock frequencies." > > > > There are so many peripherals registers located inside RCT and scratchpad > > (like usb/phy, gpio, sd, dac, enet, rng), and some peripherals even have no their > > own modules for register definitions. > > Then the syscon is the parent device of these peripherals and clocks. > You did not represent them as children but as siblings which does not > look correct. Ok, I will these syscon(RCT and scratchpad) as the parent node of our clocks and related peripherals. > > > > So most time(for a peripheral driver), the only differences between different > > Ambarella SoCs are just the syscon(rct or scratchpad) offsets get changed. > > > > I don't think such lazy hardware design is common in vendors other than ambarella. > > > > If I switch to SoC-specific compatibles, > > This is independent topic. SoC-specific compatibles are a requirement > but it does not affect your device hierarchy. Thanks, "requirement" makes things much more clear. So I will always use SoC-specific compatibles even if different Amarella SoCs may share the same reg offset and setting. > > and remove these syscon from device tree, > > of_device_id->data may only contain system controller(rct or scratchpad) offset for many Ambarella drivers, > > and ioremap/devm_ioremap carefully. > > I don't understand the problem. Neither the solution. > > > > > The question is: can upstream kernel accept such codes? > > > > If yes, I will switch to SoC-specific compatibles and remove syscon without hesitation. > > Sorry, none of your explanations here match your DTS. Your DTS clearly > models (for some reason there is no soc which makes even bigger confusion): > > rct_syscon > clocks > |-gclk-core > |-gclk-ddr > > but what you are saying is that there is no separate clock controller > device with its own IO address but these clocks are part of rct_syscon. > Then model it that way in DTS. The rct_syscon is then your clock > controller and all these fake gclk-core and gclk-ddr nodes should be gone. Ok, I will remove these fake nodes, and model the hardware as: rct_syscon node | clock node(pll, div, mux, composite clocks live in the same driver) | other periphal nodes Regards, Li
