Re: [PATCH v10 0/4] clk: meson: add a sub EMMC clock controller support

[Liang Yang <liang.yang@amlogic.com>]

Hi Neil,

On 2022/1/25 22:54, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi Liang,
> 
> On 21/01/2022 08:45, Liang Yang wrote:
>> This driver will add a MMC clock controller driver support.
>> The original idea about adding a clock controller is during the
>> discussion in the NAND driver mainline effort[1].
>>
>> This driver is tested in the S400 board (AXG platform) with NAND driver.
> 
> Thanks a lot for providing a fixed and updated version of this serie.
> 
> After some chat with Jerome and Kevin, it seems the way the eMMC clock reuse
> for NAND was designed nearly 4 years doesn't look accurate anymore.
> 
> Having a separate clk driver designed to replace the eMMC node when NAND is
> used on the board seems over engineered.
> 
> Actually having the clock code you add in this serie _but_ directly in
> the NAND looks far better, and more coherent since having Linux runtime
> detection of eMMC vs NAND will never happen and even this serie required
> some DT modification from the bootloader.
> 
> I'll let Jerome or Kevin add more details if they want, but I think you should resurrect
> the work you pushed in [1] & [2] but:
> - passing the eMMC clk registers as a third "reg" cell
Does it just need to define a 'reg' resource in NFC node and not 
'syscon' here?
> - passing the same "clocks" phandle as the eMMC node
> - adding the eMMC clock code in the NAND driver directly
> 
> I'm open to discussions if you consider the current approach is still superior.

I don't have persuasive ideas, but really it shares the common clock 
implementation for both NFC and EMMC. and we don't need to paste the 
same code in NFC and EMMC.

> 
> Thanks,
> Neil
> 
> [1] https://lore.kernel.org/r/20220106033130.37623-1-liang.yang@amlogic.com
> [2] https://lore.kernel.org/r/20220106032504.23310-1-liang.yang@amlogic.com
> 
>>
>> Changes since v9 [10]
>>   - use clk_parent_data instead of parent_names
>>
>> Changes since v8 [9]
>>   - use MESON_SCLK_ONE_BASED instead of CLK_DIVIDER_ONE_BASED
>>   - use struct_size to caculate onecell_data
>>   - add clk-phase-delay.h
>>   - define CLK_DELAY_STEP_PS_GX and CLK_DELAY_STEP_PS_AXG
>>
>> Changes since v7 [8]
>>   - move meson_clk_get_phase_delay_data() from header to driver
>>   - CONFIG sclk-div with COMMON_CLK_AMLOGIC instead of COMMON_CLK_AMLOGIC_AUDIO
>>   - remove onecell date and ID for internal MUX clk
>>   - use helper for functions for ONE_BASED in sclk-div
>>   - add ONE_BASED support for duty cycle
>>
>> Changes since v6 [7]:
>>   - add one based support for sclk divier
>>   - alloc sclk in probe for multiple instance
>>   - fix coding styles
>>
>> Changes since v5 [6]:
>>   - remove divider ops with .init and use sclk_div instead
>>   - drop CLK_DIVIDER_ROUND_CLOSEST in mux and div
>>   - drop the useless type cast
>>
>> Changes since v4 [5]:
>>   - use struct parm in phase delay driver
>>   - remove 0 delay releted part in phase delay driver
>>   - don't rebuild the parent name once again
>>   - add divider ops with .init
>>
>> Changes since v3 [4]:
>>   - separate clk-phase-delay driver
>>   - replace clk_get_rate() with clk_hw_get_rate()
>>   - collect Rob's R-Y
>>   - drop 'meson-' prefix from compatible string
>>
>>   Changes since v2 [3]:
>>   - squash dt-binding clock-id patch
>>   - update license
>>   - fix alignment
>>   - construct a clk register helper() function
>>
>> Changes since v1 [2]:
>>   - implement phase clock
>>   - update compatible name
>>   - adjust file name
>>   - divider probe() into small functions, and re-use them
>>
>> [1] https://lkml.kernel.org/r/20180628090034.0637a062@xps13
>> [2] https://lkml.kernel.org/r/20180703145716.31860-1-yixun.lan@amlogic.com
>> [3] https://lkml.kernel.org/r/20180710163658.6175-1-yixun.lan@amlogic.com
>> [4] https://lkml.kernel.org/r/20180712211244.11428-1-yixun.lan@amlogic.com
>> [5] https://lkml.kernel.org/r/20180809070724.11935-4-yixun.lan@amlogic.com
>> [6] https://lkml.kernel.org/r/1539839245-13793-1-git-send-email-jianxin.pan@amlogic.com
>> [7] https://lkml.kernel.org/r/1541089855-19356-1-git-send-email-jianxin.pan@amlogic.com
>> [8] https://lkml.kernel.org/r/1544457877-51301-1-git-send-email-jianxin.pan@amlogic.com
>> [9] https://lkml.kernel.org/r/1545063850-21504-1-git-send-email-jianxin.pan@amlogic.com
>> [10] https://lore.kernel.org/all/20220113115745.45826-1-liang.yang@amlogic.com/
>> Liang Yang (4):
>>    clk: meson: add one based divider support for sclk
>>    clk: meson: add emmc sub clock phase delay driver
>>    clk: meson: add DT documentation for emmc clock controller
>>    clk: meson: add sub MMC clock controller driver
>>
>>   .../bindings/clock/amlogic,mmc-clkc.yaml      |  64 ++++
>>   drivers/clk/meson/Kconfig                     |  18 ++
>>   drivers/clk/meson/Makefile                    |   2 +
>>   drivers/clk/meson/clk-phase-delay.c           |  69 ++++
>>   drivers/clk/meson/clk-phase-delay.h           |  20 ++
>>   drivers/clk/meson/mmc-clkc.c                  | 302 ++++++++++++++++++
>>   drivers/clk/meson/sclk-div.c                  |  59 ++--
>>   drivers/clk/meson/sclk-div.h                  |   3 +
>>   include/dt-bindings/clock/amlogic,mmc-clkc.h  |  14 +
>>   9 files changed, 529 insertions(+), 22 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mmc-clkc.yaml
>>   create mode 100644 drivers/clk/meson/clk-phase-delay.c
>>   create mode 100644 drivers/clk/meson/clk-phase-delay.h
>>   create mode 100644 drivers/clk/meson/mmc-clkc.c
>>   create mode 100644 include/dt-bindings/clock/amlogic,mmc-clkc.h
>>
> 
> .