| Message ID | 20210616224743.5109-4-chun-jie.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Mediatek MT8195 clock support | expand |
On Thu, Jun 17, 2021 at 7:01 AM Chun-Jie Chen <chun-jie.chen@mediatek.com> wrote: > > On MT8195, tuner_en_reg is moved to register offest 0x0. > If we only judge by tuner_en_reg, it may lead to wrong address. > Add tuner_en_bit to the check condition. And it has been confirmed, > on all the MediaTek SoCs, bit0 of offset 0x0 is always occupied by > clock square control. > > Signed-off-by: Chun-Jie Chen <chun-jie.chen@mediatek.com> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> Though you might want to consider converting these types of checks into feature flags.
On 30/06/2021 09:31, Chen-Yu Tsai wrote: > On Thu, Jun 17, 2021 at 7:01 AM Chun-Jie Chen > <chun-jie.chen@mediatek.com> wrote: >> >> On MT8195, tuner_en_reg is moved to register offest 0x0. >> If we only judge by tuner_en_reg, it may lead to wrong address. >> Add tuner_en_bit to the check condition. And it has been confirmed, >> on all the MediaTek SoCs, bit0 of offset 0x0 is always occupied by >> clock square control. >> >> Signed-off-by: Chun-Jie Chen <chun-jie.chen@mediatek.com> > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > Though you might want to consider converting these types of checks into feature > flags. > Yes I think adding a feature flag is the way to go. Luckily there are only a few SoCs that will need updates at the same time. Regards, Matthias
On Wed, Jun 30, 2021 at 6:53 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: > > > > On 30/06/2021 09:31, Chen-Yu Tsai wrote: > > On Thu, Jun 17, 2021 at 7:01 AM Chun-Jie Chen > > <chun-jie.chen@mediatek.com> wrote: > >> > >> On MT8195, tuner_en_reg is moved to register offest 0x0. > >> If we only judge by tuner_en_reg, it may lead to wrong address. > >> Add tuner_en_bit to the check condition. And it has been confirmed, > >> on all the MediaTek SoCs, bit0 of offset 0x0 is always occupied by > >> clock square control. > >> > >> Signed-off-by: Chun-Jie Chen <chun-jie.chen@mediatek.com> > > > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > > > Though you might want to consider converting these types of checks into feature > > flags. > > > > Yes I think adding a feature flag is the way to go. Luckily there are only a few > SoCs that will need updates at the same time. I also see that the different clock modules are tied together using only clock names written in the drivers, instead of clock references in the device tree. Unfortunately reworking this would likely require a lot more work. I previously did a bit of internal reworking for the sunxi drivers. While not the same, I think the plumbing required is comparable. ChenYu
On 30/06/2021 13:09, Chen-Yu Tsai wrote: > On Wed, Jun 30, 2021 at 6:53 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: >> >> >> >> On 30/06/2021 09:31, Chen-Yu Tsai wrote: >>> On Thu, Jun 17, 2021 at 7:01 AM Chun-Jie Chen >>> <chun-jie.chen@mediatek.com> wrote: >>>> >>>> On MT8195, tuner_en_reg is moved to register offest 0x0. >>>> If we only judge by tuner_en_reg, it may lead to wrong address. >>>> Add tuner_en_bit to the check condition. And it has been confirmed, >>>> on all the MediaTek SoCs, bit0 of offset 0x0 is always occupied by >>>> clock square control. >>>> >>>> Signed-off-by: Chun-Jie Chen <chun-jie.chen@mediatek.com> >>> >>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> >>> >>> Though you might want to consider converting these types of checks into feature >>> flags. >>> >> >> Yes I think adding a feature flag is the way to go. Luckily there are only a few >> SoCs that will need updates at the same time. > > I also see that the different clock modules are tied together using only clock > names written in the drivers, instead of clock references in the device tree. > Not sure I understand what you mean. Do you refer to something like [1]? That's because the clock is probed by the DRM driver, as they share the same compatible and IP block. Regards, Matthias [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/mediatek/clk-mt8173-mm.c?h=v5.13#n139 > Unfortunately reworking this would likely require a lot more work. I previously > did a bit of internal reworking for the sunxi drivers. While not the same, I > think the plumbing required is comparable. > > ChenYu >
"On Wed, Jun 30, 2021 at 7:43 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: > On 30/06/2021 13:09, Chen-Yu Tsai wrote: > > On Wed, Jun 30, 2021 at 6:53 PM Matthias Brugger <matthias.bgg@gmail.com> wrote: > >> On 30/06/2021 09:31, Chen-Yu Tsai wrote: > >>> On Thu, Jun 17, 2021 at 7:01 AM Chun-Jie Chen > >>> <chun-jie.chen@mediatek.com> wrote: > >>>> > >>>> On MT8195, tuner_en_reg is moved to register offest 0x0. > >>>> If we only judge by tuner_en_reg, it may lead to wrong address. > >>>> Add tuner_en_bit to the check condition. And it has been confirmed, > >>>> on all the MediaTek SoCs, bit0 of offset 0x0 is always occupied by > >>>> clock square control. > >>>> > >>>> Signed-off-by: Chun-Jie Chen <chun-jie.chen@mediatek.com> > >>> > >>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > >>> > >>> Though you might want to consider converting these types of checks into feature > >>> flags. > >>> > >> > >> Yes I think adding a feature flag is the way to go. Luckily there are only a few > >> SoCs that will need updates at the same time. > > > > I also see that the different clock modules are tied together using only clock > > names written in the drivers, instead of clock references in the device tree. > > > > Not sure I understand what you mean. Do you refer to something like [1]? That's > because the clock is probed by the DRM driver, as they share the same compatible > and IP block. In the example driver you mentioned, most of the registered clocks have the same parent clock, "mm_sel". This clock is from another hardware block, "topckgen" [1]. The two are linked together by looking up the clock name. The link should be explicitly described in the device tree, instead of implicitly by some name found in two drivers. The consuming driver can fetch the clock name via of_clk_get_parent_name(), or be migrated to use `struct clk_parent_data`, which allows specifying local (to the DT node) clock names or clk indices as parent clk references. What's more confusing is that the mmsys node actually has "assigned-clocks" properties [2] referencing the "mm_sel" clock, but not "clock" properties referencing the same clock. On the surface this looks like the hardware is trying to configure clocks that it doesn't use. Also, Maxime Ripard made the argument before that "assigned-clock-rates" doesn't give any real guarantees that the clock rate won't change. A better method is to request and "lock" the clock rate in the consuming driver. So overall I think there are many improvements that can be made to the Mediatek clk drivers. They aren't real blockers to new drivers though, and I think each would take some effort and coordination across all the SoCs. Regards ChenYu [1] https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mt8173.c#L545 [2] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L996 > Regards, > Matthias > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/mediatek/clk-mt8173-mm.c?h=v5.13#n139 > > > Unfortunately reworking this would likely require a lot more work. I previously > > did a bit of internal reworking for the sunxi drivers. While not the same, I > > think the plumbing required is comparable. > > > > ChenYu > >
diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c index 7fb001a4e7d8..99ada6e06697 100644 --- a/drivers/clk/mediatek/clk-pll.c +++ b/drivers/clk/mediatek/clk-pll.c @@ -332,7 +332,7 @@ static struct clk *mtk_clk_register_pll(const struct mtk_pll_data *data, pll->pcw_chg_addr = pll->base_addr + REG_CON1; if (data->tuner_reg) pll->tuner_addr = base + data->tuner_reg; - if (data->tuner_en_reg) + if (data->tuner_en_reg || data->tuner_en_bit) pll->tuner_en_addr = base + data->tuner_en_reg; if (data->en_reg) pll->en_addr = base + data->en_reg;
On MT8195, tuner_en_reg is moved to register offest 0x0. If we only judge by tuner_en_reg, it may lead to wrong address. Add tuner_en_bit to the check condition. And it has been confirmed, on all the MediaTek SoCs, bit0 of offset 0x0 is always occupied by clock square control. Signed-off-by: Chun-Jie Chen <chun-jie.chen@mediatek.com> --- drivers/clk/mediatek/clk-pll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)