schemas: introduce assigned-clock-sscs #154
Conversation
|
I am happy to see unified bindings but this makes me feel bitter of totally desynced work from Dario's. Three months iMX8 patchsets, multiple reviews and no single comment from you till January! https://lore.kernel.org/all/[email protected]/ So you saw his work. Why you didn't say that you intend to come with competitive solution? Nothing to stop him from doing work which you made now partially obsolete (the bindings part). NXP needs to be a bit more active in discussions. |
|
I should say sorry for this. I gave R-b for most of Dario's driver patches, so I agree with his approach and I also plan to reuse his appoarch for i.MX93/i.MX91 after his patch get landed. I not intend to waste your and Dario's time. But I got a request to enable SSC for i.MX95 based SCMI CLK just three days ago, and then I started to investigate how to enable that. Since drivers/clk/clk-scmi.c is a generic clk driver, and scmi nodes mostly only has a reg property, so impossible to add non generic property(SCMI Maintainers not happy to include other properties, especially vendor specific stuff), so I think "assigned-clock-sscs" may help here. Then I wrote the patches in two days and posted out for comments. |
|
Commit 223d32eb1001 ("dt-bindings: clock: st,stm32-rcc: support spread spectrum clocking") use "st,ssc-moddepth-permyriad" (in permyriad, i.e. 0.01%) while commit 4a8bc2644ef0 ("dt-bindings: ti: dpll: add spread spectrum support") use "ti,ssc-deltam" in permille unit (10th of a percent). What about change the "The modulation percentage" in "The modulation depth", expressed in permyriad? We use a 32 bit value, so we can express it as 0.01%. |
|
It should be fine to change to modulation depth in permyriad |
|
Why not put this in a clock cell for the clock? Or carve out some bits on the existing cell? How many bits are really needed here? How is the setting determined? Is it per board? per SoC? Fixed for the consumer of the clock? |
32 bits for i.MX95. Put this in a clock cell maybe not good or carve out some bits on the existing cell. This would change clock xlate saying break ABI.
The settings are tuned by measuring using oscilloscope or else. It is per board. Spread spectrum is mostly for reduce the radiated emissions of digital yes, fixed for consumer of the clock. Some references: |
Why not good exactly?
How would it break? Old implementation should ignore trailing arguments. Are you saying it does not ignore them? |
The SSC needs three fields for each clock, using one u32 or three u32. we could not carve out 32bits from a u32 clock entry, saying clocks = <&provider 5>. There is not enough bits for SSC. Current bindings for clocks in linux kernel per my search, Putting SSC in a cell would means adding extra three u32 or one u32, this would make it a bit hard to read.
Taking current i.MX8M/iMX93 for example, SSC not supported in upstream linux as of now. Adding trailing arguments means clock provider needs update to parse the new arg and each clocks entry needs update to add new arguments to encoding SSC. For other platforms(already in tree) that would need to support SSC, similar change should be done. Not only Linux, but also other OSes that follow dt-schema. Using assigned-clock-sscs could save a lot work for people. |
|
@robherring @krzk do you have more comments? Thanks |
|
Growing cells, so the xlate, should not be an ABI break. Might be in some cases, but logically excessive arguments should be ignored, unless something uses of_phandle_args_equal(). Many Linux clock drivers use of_clk_hw_onecell_get() which ignores additional params, so they should be fine: no ABI break. There is already binding using a cells==2: interconnect for qcom, so your case with one more number would be perfectly fine. If you really need to add three numbers, this would grow for imx to cells==4 (and you said you found
No, provider ignores additional args and parsing SSC you need to add anyway. Either as clock cells or as additional property, so the amount of work would be the same. That all said, many clocks for the same clock controller would not need or not even support spread-spectrum, but clock-cells apply to entire clock controller device. Therefore I think using clock-cells might lead to rather reduced readability of such DTS and I am fine with assigned-xxx properties. Please capture pieces of this in commit msg and send a v2 of this pull req, unless @robherring has some more objections or thoughts. |
|
@krzk sorry. My bad. I not check github comments before push a new version, please ignore it. I will rethink and post a new version. |
Spread Spectrum Clocking(SSC) is common to overcome EMI issues. A spread spectrum clock is frequency modulated, so the energy is spread out over a wider bandwidth, reducing the peak radiated emissions. It includes three parameters to enable SSC: - modulation frequency:typical values range is a few tens of KHz - modulation depth: platform dependent, -0.5%~+0.5%, -2%~+2% or even 0.01% per stm32-rcc.yaml, - modulation method: center, down, up spread. To support spread spectrum clock, two options: 1. Put the spread spectrum settings in clock cells 2. Introduce assigned-clock-sscs Option1 requires growing clock cells, but many clocks for the same clock controller would not need or not even support spread-spectrum, but clock-cells apply to entire clock controller device. using clock-cells might lead to rather reduced readability of such DTS. Option2 introduces a new property with each clock takes three u32. This property only needs to be added under nodes that needs SSC. This patch use Option2 which is a cleaner way from DTS view. Signed-off-by: Peng Fan <[email protected]>
|
I updated the patch with more commit log, please help look whether it is good for you. @krzk Appreciate your help on detailed explaination above! |
To support spread spectrum clock, introduce assigned-clock-sscs, it is an uint32-matrix with format multiple elements of below
, <...>