RE: [PATCH v5 08/10] clk: realtek: Add support for MMC-tuned PLL clocks

[Yu-Chun Lin [林祐君]]

> Quoting Yu-Chun Lin (2026-03-23 19:53:29)
> > diff --git a/drivers/clk/realtek/clk-pll-mmc.c
> > b/drivers/clk/realtek/clk-pll-mmc.c
> > new file mode 100644
> > index 000000000000..017663738c1f
> > --- /dev/null
> > +++ b/drivers/clk/realtek/clk-pll-mmc.c
> > @@ -0,0 +1,399 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2021 Realtek Semiconductor Corporation
> > + * Author: Cheng-Yu Lee <cylee12@xxxxxxxxxxx>  */
> > +
> > +#include "clk-pll.h"
> 
> Include what you use in this C file, not just this header file. That makes it
> simpler to see what is used without following include trails.

Got it. I will explicitly include the required headers directly in this .c file.

> > +
> > +#define PLL_EMMC1_OFFSET           0x0
> > +#define PLL_EMMC2_OFFSET           0x4
> > +#define PLL_EMMC3_OFFSET           0x8
> > +#define PLL_EMMC4_OFFSET           0xc
> > +#define PLL_SSC_DIG_EMMC1_OFFSET   0x0
> > +#define PLL_SSC_DIG_EMMC3_OFFSET   0xc
> > +#define PLL_SSC_DIG_EMMC4_OFFSET   0x10
> > +
> > +#define PLL_MMC_SSC_DIV_N_VAL      0x1b
> > +
> > +#define PLL_PHRT0_MASK             BIT(1)
> > +#define PLL_PHSEL_MASK             GENMASK(4, 0)
> > +#define PLL_SSCPLL_RS_MASK         GENMASK(12, 10)
> > +#define PLL_SSCPLL_ICP_MASK        GENMASK(9, 5)
> > +#define PLL_SSC_DIV_EXT_F_MASK     GENMASK(25, 13)
> > +#define PLL_PI_IBSELH_MASK         GENMASK(28, 27)
> > +#define PLL_SSC_DIV_N_MASK         GENMASK(23, 16)
> > +#define PLL_NCODE_SSC_EMMC_MASK    GENMASK(20, 13)
> > +#define PLL_FCODE_SSC_EMMC_MASK    GENMASK(12, 0)
> > +#define PLL_GRAN_EST_EM_MC_MASK    GENMASK(20, 0)
> > +#define PLL_EN_SSC_EMMC_MASK       BIT(0)
> > +#define PLL_FLAG_INITAL_EMMC_MASK  BIT(1)
> [...]
> > diff --git a/drivers/clk/realtek/clk-pll.h
> > b/drivers/clk/realtek/clk-pll.h index 2d27a44a270c..9cf219871218
> > 100644
> > --- a/drivers/clk/realtek/clk-pll.h
> > +++ b/drivers/clk/realtek/clk-pll.h
> > @@ -44,4 +44,25 @@ static inline struct clk_pll *to_clk_pll(struct
> > clk_hw *hw)  extern const struct clk_ops rtk_clk_pll_ops;  extern
> > const struct clk_ops rtk_clk_pll_ro_ops;
> >
> > +struct clk_pll_mmc {
> > +       struct clk_regmap clkr;
> > +       int pll_ofs;
> > +       int ssc_dig_ofs;
> 
> These offsets should be unsigned?
>

Yes, I will fix it.

> > +       struct clk_hw phase0_hw;
> > +       struct clk_hw phase1_hw;
> > +       u32 set_rate_val_53_97_set_ipc: 1;
> 
> bool? Doubt we care about this unless we're packing structs (which we
> shouldn't be).
> 

This member is actually redundant, so I will just remove it.

> > +};
> > +
> > +#define __clk_pll_mmc_hw(_ptr)  __clk_regmap_hw(&(_ptr)->clkr)
> > +
> > +static inline struct clk_pll_mmc *to_clk_pll_mmc(struct clk_hw *hw) {
> > +       struct clk_regmap *clkr = to_clk_regmap(hw);
> > +
> > +       return container_of(clkr, struct clk_pll_mmc, clkr); }

Best regards,
Yu-Chun