| Message ID | Y1K5ym1EL8kwzQEt@makrotopia.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] pwm: mediatek: Add support for MT7986 | expand |
Hi Daniel: On Fri, 2022-10-21 at 16:24 +0100, Daniel Golle wrote: > Add support for PWM on MT7986 which has 2 PWM channels, one of them > is > typically used for a temperature controlled fan. > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > drivers/pwm/pwm-mediatek.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > index 6901a44dc428de..2219cba033e348 100644 > --- a/drivers/pwm/pwm-mediatek.c > +++ b/drivers/pwm/pwm-mediatek.c > @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data > mt8365_pwm_data = { > .has_ck_26m_sel = true, > }; > > +static const struct pwm_mediatek_of_data mt7986_pwm_data = { > + .num_pwms = 2, > + .pwm45_fixup = false, > + .has_ck_26m_sel = true, For MT7986 SoC, I think the value of "has_ck_26m_sel" should be 'false' > +}; > + > static const struct pwm_mediatek_of_data mt8516_pwm_data = { > .num_pwms = 5, > .pwm45_fixup = false, > @@ -342,6 +348,7 @@ static const struct of_device_id > pwm_mediatek_of_match[] = { > { .compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data > }, > { .compatible = "mediatek,mt7628-pwm", .data = &mt7628_pwm_data > }, > { .compatible = "mediatek,mt7629-pwm", .data = &mt7629_pwm_data > }, > + { .compatible = "mediatek,mt7986-pwm", .data = &mt7986_pwm_data > }, > { .compatible = "mediatek,mt8183-pwm", .data = &mt8183_pwm_data > }, > { .compatible = "mediatek,mt8365-pwm", .data = &mt8365_pwm_data > }, > { .compatible = "mediatek,mt8516-pwm", .data = &mt8516_pwm_data > }, Regards, Sam
On Tue, Oct 25, 2022 at 02:35:43PM +0800, Sam Shih wrote: > Hi Daniel: > > On Fri, 2022-10-21 at 16:24 +0100, Daniel Golle wrote: > > Add support for PWM on MT7986 which has 2 PWM channels, one of them > > is > > typically used for a temperature controlled fan. > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > drivers/pwm/pwm-mediatek.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > > index 6901a44dc428de..2219cba033e348 100644 > > --- a/drivers/pwm/pwm-mediatek.c > > +++ b/drivers/pwm/pwm-mediatek.c > > @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data > > mt8365_pwm_data = { > > .has_ck_26m_sel = true, > > }; > > > > +static const struct pwm_mediatek_of_data mt7986_pwm_data = { > > + .num_pwms = 2, > > + .pwm45_fixup = false, > > + .has_ck_26m_sel = true, > > For MT7986 SoC, I think the value of "has_ck_26m_sel" should be 'false' That's a bit surprising, please explain why. Reading the commit adding .has_ck_26m_sel field: > commit 0c0ead76235db0bcfaab83f04db546995449d002 > Author: Fabien Parent <fparent@baylibre.com> > Date: Mon Oct 19 16:07:02 2020 +0200 > > pwm: mediatek: Always use bus clock > > The MediaTek PWM IP can sometimes use the 26 MHz source clock to > generate the PWM signal, but the driver currently assumes that we always > use the PWM bus clock to generate the PWM signal. > > This commit modifies the PWM driver in order to force the PWM IP to > always use the bus clock as source clock. > > I do not have the datasheet of all the MediaTek SoC, so I don't know if > the register to choose the source clock is present in all the SoCs or > only in subset. As a consequence I made this change optional by using a > platform data paremeter to says whether this register is supported or > not. On all the SoCs I don't have the datasheet (MT2712, MT7622, MT7623, > MT7628, MT7629) I kept the behavior to be the same as before this > change. > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> From MT7986 datasheet: > 0x10048210 PWM_CK_26M_SEL PWM BCLK Selection > Reset value 0x00000001 > Description > 0: Select bus CLK as BCLK > 1: Select 26M fix CLK as BCLK So after reset, the 26M clock is selected by default. In pwm-mediatek.c I read: > #define PWM_CK_26M_SEL 0x210 > ... > /* Make sure we use the bus clock and not the 26MHz clock */ > if (pc->soc->has_ck_26m_sel) > writel(0, pc->regs + PWM_CK_26M_SEL); So this PWM_CK_26M_SEL register does exist on MT7986 and has the same address as expected by the driver ($PWM_BASE + 0x210). The default value selected after reset (0x00000001) matches the problem and solution described in the commit description "pwm: mediatek: Always use bus clock". Sidenode: I've tried with both, .has_ck_26m_sel = true as well as .has_ck_26m_sel = false. Both do work, but the behavior is slightly different, again matching the commit description above. > > > +}; > > + > > static const struct pwm_mediatek_of_data mt8516_pwm_data = { > > .num_pwms = 5, > > .pwm45_fixup = false, > > @@ -342,6 +348,7 @@ static const struct of_device_id > > pwm_mediatek_of_match[] = { > > { .compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data > > }, > > { .compatible = "mediatek,mt7628-pwm", .data = &mt7628_pwm_data > > }, > > { .compatible = "mediatek,mt7629-pwm", .data = &mt7629_pwm_data > > }, > > + { .compatible = "mediatek,mt7986-pwm", .data = &mt7986_pwm_data > > }, > > { .compatible = "mediatek,mt8183-pwm", .data = &mt8183_pwm_data > > }, > > { .compatible = "mediatek,mt8365-pwm", .data = &mt8365_pwm_data > > }, > > { .compatible = "mediatek,mt8516-pwm", .data = &mt8516_pwm_data > > }, > > Regards, > Sam >
On Tue, 2022-10-25 at 12:57 +0100, Daniel Golle wrote: > On Tue, Oct 25, 2022 at 02:35:43PM +0800, Sam Shih wrote: > > Hi Daniel: > > > > On Fri, 2022-10-21 at 16:24 +0100, Daniel Golle wrote: > > > Add support for PWM on MT7986 which has 2 PWM channels, one of > > > them > > > is > > > typically used for a temperature controlled fan. > > > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > > --- > > > drivers/pwm/pwm-mediatek.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm- > > > mediatek.c > > > index 6901a44dc428de..2219cba033e348 100644 > > > --- a/drivers/pwm/pwm-mediatek.c > > > +++ b/drivers/pwm/pwm-mediatek.c > > > @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data > > > mt8365_pwm_data = { > > > .has_ck_26m_sel = true, > > > }; > > > > > > +static const struct pwm_mediatek_of_data mt7986_pwm_data = { > > > + .num_pwms = 2, > > > + .pwm45_fixup = false, > > > + .has_ck_26m_sel = true, > > > > For MT7986 SoC, I think the value of "has_ck_26m_sel" should be > > 'false' > > That's a bit surprising, please explain why. > The clock tree of MT7981/MT7986 PWM BCLK is as bellow: PLL --> topckgen fix-factors --> TOP_PWM_SEL (topckgen clock mux) --> -- > CLK_INFRA_PWM_BSEL (infra clock mux) --> PWM BCLK (gate) We do have the clock multiplexer to select the source clock for PWM_BCLK https://github.com/torvalds/linux/blob/master/drivers/clk/mediatek/clk-mt7986-infracfg.c#L63 In my knowledge, the pwm hardware of MT7981/MT7986 SoC should ignore this register directtly, but we still keep the register for backword compatibility. In fact, the MT7986 SoC is also working whether 'has_ck_26m_sel' is 'true' or 'false'. Going back to the definition of 'has_ck_26m_sel', if it means "PWM_CK_26M_SEL" register exists or not, we should use 'true', but if it means clock from 26M clock or BUS clock, we should use 'false' > Reading the commit adding .has_ck_26m_sel field: > > commit 0c0ead76235db0bcfaab83f04db546995449d002 > > Author: Fabien Parent <fparent@baylibre.com> > > Date: Mon Oct 19 16:07:02 2020 +0200 > > > > pwm: mediatek: Always use bus clock > > > > The MediaTek PWM IP can sometimes use the 26 MHz source clock to > > generate the PWM signal, but the driver currently assumes that we > > always > > use the PWM bus clock to generate the PWM signal. > > > > This commit modifies the PWM driver in order to force the PWM IP to > > always use the bus clock as source clock. > > > > I do not have the datasheet of all the MediaTek SoC, so I don't > > know if > > the register to choose the source clock is present in all the SoCs > > or > > only in subset. As a consequence I made this change optional by > > using a > > platform data paremeter to says whether this register is supported > > or > > not. On all the SoCs I don't have the datasheet (MT2712, MT7622, > > MT7623, > > MT7628, MT7629) I kept the behavior to be the same as before this > > change. > > > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > From MT7986 datasheet: > > 0x10048210 PWM_CK_26M_SEL PWM BCLK Selection > > Reset value 0x00000001 > > Description > > 0: Select bus CLK as BCLK > > 1: Select 26M fix CLK as BCLK > So after reset, the 26M clock is selected by default. > > In pwm-mediatek.c I read: > > #define PWM_CK_26M_SEL 0x210 > > ... > > /* Make sure we use the bus clock and not the 26MHz clock > > */ > > if (pc->soc->has_ck_26m_sel) > > writel(0, pc->regs + PWM_CK_26M_SEL); > > So this PWM_CK_26M_SEL register does exist on MT7986 and has the > same address as expected by the driver ($PWM_BASE + 0x210). > The default value selected after reset (0x00000001) matches the > problem and solution described in the commit description > "pwm: mediatek: Always use bus clock". > > Sidenode: I've tried with both, .has_ck_26m_sel = true as well as > .has_ck_26m_sel = false. Both do work, but the behavior is slightly > different, again matching the commit description above. What is the difference between the two? I tried to config the pwm ch0 period=1000000 and duty=500000, Modify PWM_CK_26M_SEL, and measure the output waveform, the waveform keep the same. Regards, Sam
On Wed, Oct 26, 2022 at 02:17:06PM +0800, Sam Shih wrote: > On Tue, 2022-10-25 at 12:57 +0100, Daniel Golle wrote: > > On Tue, Oct 25, 2022 at 02:35:43PM +0800, Sam Shih wrote: > > > Hi Daniel: > > > > > > On Fri, 2022-10-21 at 16:24 +0100, Daniel Golle wrote: > > > > Add support for PWM on MT7986 which has 2 PWM channels, one of > > > > them > > > > is > > > > typically used for a temperature controlled fan. > > > > > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > > > --- > > > > drivers/pwm/pwm-mediatek.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm- > > > > mediatek.c > > > > index 6901a44dc428de..2219cba033e348 100644 > > > > --- a/drivers/pwm/pwm-mediatek.c > > > > +++ b/drivers/pwm/pwm-mediatek.c > > > > @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data > > > > mt8365_pwm_data = { > > > > .has_ck_26m_sel = true, > > > > }; > > > > > > > > +static const struct pwm_mediatek_of_data mt7986_pwm_data = { > > > > + .num_pwms = 2, > > > > + .pwm45_fixup = false, > > > > + .has_ck_26m_sel = true, > > > > > > For MT7986 SoC, I think the value of "has_ck_26m_sel" should be > > > 'false' > > > > That's a bit surprising, please explain why. > > > > The clock tree of MT7981/MT7986 PWM BCLK is as bellow: > PLL --> topckgen fix-factors --> TOP_PWM_SEL (topckgen clock mux) --> > -- > > CLK_INFRA_PWM_BSEL (infra clock mux) --> PWM BCLK (gate) > > We do have the clock multiplexer to select the source clock for PWM_BCLK > https://github.com/torvalds/linux/blob/master/drivers/clk/mediatek/clk-mt7986-infracfg.c#L63 > > In my knowledge, the pwm hardware of MT7981/MT7986 SoC should ignore > this register directtly, but we still keep the register for backword > compatibility. > > In fact, the MT7986 SoC is also working whether 'has_ck_26m_sel' is > 'true' or 'false'. > > Going back to the definition of 'has_ck_26m_sel', if it means > "PWM_CK_26M_SEL" register exists or not, we should use 'true', but if > it means clock from 26M clock or BUS clock, we should use 'false' It means 'write 0 to PWM_CK_26M_SEL to make sure PWM BCLK is selected'. If the register isn't used at all on MT7986 (despite being mentioned in the datasheet) or the value written there never has any effect then there is no need to do this and has_ck_26m_sel can be false. > > > Reading the commit adding .has_ck_26m_sel field: > > > commit 0c0ead76235db0bcfaab83f04db546995449d002 > > > Author: Fabien Parent <fparent@baylibre.com> > > > Date: Mon Oct 19 16:07:02 2020 +0200 > > > > > > pwm: mediatek: Always use bus clock > > > > > > The MediaTek PWM IP can sometimes use the 26 MHz source clock to > > > generate the PWM signal, but the driver currently assumes that we > > > always > > > use the PWM bus clock to generate the PWM signal. > > > > > > This commit modifies the PWM driver in order to force the PWM IP to > > > always use the bus clock as source clock. > > > > > > I do not have the datasheet of all the MediaTek SoC, so I don't > > > know if > > > the register to choose the source clock is present in all the SoCs > > > or > > > only in subset. As a consequence I made this change optional by > > > using a > > > platform data paremeter to says whether this register is supported > > > or > > > not. On all the SoCs I don't have the datasheet (MT2712, MT7622, > > > MT7623, > > > MT7628, MT7629) I kept the behavior to be the same as before this > > > change. > > > > > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > > > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > > From MT7986 datasheet: > > > 0x10048210 PWM_CK_26M_SEL PWM BCLK Selection > > > Reset value 0x00000001 > > > Description > > > 0: Select bus CLK as BCLK > > > 1: Select 26M fix CLK as BCLK > > So after reset, the 26M clock is selected by default. > > > > In pwm-mediatek.c I read: > > > #define PWM_CK_26M_SEL 0x210 > > > ... > > > /* Make sure we use the bus clock and not the 26MHz clock > > > */ > > > if (pc->soc->has_ck_26m_sel) > > > writel(0, pc->regs + PWM_CK_26M_SEL); > > > > So this PWM_CK_26M_SEL register does exist on MT7986 and has the > > same address as expected by the driver ($PWM_BASE + 0x210). > > The default value selected after reset (0x00000001) matches the > > problem and solution described in the commit description > > "pwm: mediatek: Always use bus clock". > > > > Sidenode: I've tried with both, .has_ck_26m_sel = true as well as > > .has_ck_26m_sel = false. Both do work, but the behavior is slightly > > different, again matching the commit description above. > > What is the difference between the two? > > I tried to config the pwm ch0 period=1000000 and duty=500000, > Modify PWM_CK_26M_SEL, and measure the output waveform, the waveform > keep the same. Maybe something else already sets the PWM_CK_26M_SEL register to 0 before Linux starts (e.g. U-Boot)?
On Tue, Oct 25, 2022 at 02:35:43PM +0800, Sam Shih wrote: > Hi Daniel: > > On Fri, 2022-10-21 at 16:24 +0100, Daniel Golle wrote: > > Add support for PWM on MT7986 which has 2 PWM channels, one of them > > is > > typically used for a temperature controlled fan. > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > drivers/pwm/pwm-mediatek.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > > index 6901a44dc428de..2219cba033e348 100644 > > --- a/drivers/pwm/pwm-mediatek.c > > +++ b/drivers/pwm/pwm-mediatek.c > > @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data > > mt8365_pwm_data = { > > .has_ck_26m_sel = true, > > }; > > > > +static const struct pwm_mediatek_of_data mt7986_pwm_data = { > > + .num_pwms = 2, > > + .pwm45_fixup = false, > > + .has_ck_26m_sel = true, > > For MT7986 SoC, I think the value of "has_ck_26m_sel" should be 'false' The status of the discussion isn't clear to me. You didn't visibly agree which value is right now. Will there be a v2 of this patch? Or is it expected to be picked up as is. From my side (i.e. not having checked the hw details just judging with the PWM hat on) the patch is fine. Best regards Uwe
On Thu, Nov 17, 2022 at 12:56:24PM +0100, Uwe Kleine-König wrote: > On Tue, Oct 25, 2022 at 02:35:43PM +0800, Sam Shih wrote: > > Hi Daniel: > > > > On Fri, 2022-10-21 at 16:24 +0100, Daniel Golle wrote: > > > Add support for PWM on MT7986 which has 2 PWM channels, one of them > > > is > > > typically used for a temperature controlled fan. > > > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > > --- > > > drivers/pwm/pwm-mediatek.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > > > index 6901a44dc428de..2219cba033e348 100644 > > > --- a/drivers/pwm/pwm-mediatek.c > > > +++ b/drivers/pwm/pwm-mediatek.c > > > @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data > > > mt8365_pwm_data = { > > > .has_ck_26m_sel = true, > > > }; > > > > > > +static const struct pwm_mediatek_of_data mt7986_pwm_data = { > > > + .num_pwms = 2, > > > + .pwm45_fixup = false, > > > + .has_ck_26m_sel = true, > > > > For MT7986 SoC, I think the value of "has_ck_26m_sel" should be 'false' > > The status of the discussion isn't clear to me. You didn't visibly agree > which value is right now. Will there be a v2 of this patch? Or is it > expected to be picked up as is. The patch has been tested thoroughly and works well as-is. The CK_26M_SEL register does exist on MT7986 and, at least according to the datasheet[1], is set to the wrong value (selecting 26M clock) on reset (but probably then already set to 0 to select the bus clock, e.g. by the bootloader). So in the worst case, this is a no-op. [1]: MT7986 Reference Manual, version 1.0 released 2022-05-29, page 428 available at https://wiki.banana-pi.org/Banana_Pi_BPI-R3#Documents > > From my side (i.e. not having checked the hw details just judging with > the PWM hat on) the patch is fine. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | /
Hi Uwe, Sorry for the late reply. Since the CK_26M_SEL register does exist on MT7986, and for the other mediatek SoCs, we should update the value of the CK_26M_SEL register if it exists on the SoC, even the MT7986's PWM hardware just bypasses this setting. From this point of view, I think it is fine to mark '.has_ck_26m_sel' attribute to 'true'. For this patch: Reviewed-by: Sam Shih <sam.shih@mediatek.com> Thanks, Best Regards, Sam Shih On Thu, 2022-11-17 at 12:56 +0100, Uwe Kleine-König wrote: > On Tue, Oct 25, 2022 at 02:35:43PM +0800, Sam Shih wrote: > > Hi Daniel: > > > > On Fri, 2022-10-21 at 16:24 +0100, Daniel Golle wrote: > > > Add support for PWM on MT7986 which has 2 PWM channels, one of > > > them > > > is > > > typically used for a temperature controlled fan. > > > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > > --- > > > drivers/pwm/pwm-mediatek.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm- > > > mediatek.c > > > index 6901a44dc428de..2219cba033e348 100644 > > > --- a/drivers/pwm/pwm-mediatek.c > > > +++ b/drivers/pwm/pwm-mediatek.c > > > @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data > > > mt8365_pwm_data = { > > > .has_ck_26m_sel = true, > > > }; > > > > > > +static const struct pwm_mediatek_of_data mt7986_pwm_data = { > > > + .num_pwms = 2, > > > + .pwm45_fixup = false, > > > + .has_ck_26m_sel = true, > > > > For MT7986 SoC, I think the value of "has_ck_26m_sel" should be > > 'false' > > The status of the discussion isn't clear to me. You didn't visibly > agree > which value is right now. Will there be a v2 of this patch? Or is it > expected to be picked up as is. > > From my side (i.e. not having checked the hw details just judging > with > the PWM hat on) the patch is fine. > > Best regards > Uwe >
diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c index 6901a44dc428de..2219cba033e348 100644 --- a/drivers/pwm/pwm-mediatek.c +++ b/drivers/pwm/pwm-mediatek.c @@ -329,6 +329,12 @@ static const struct pwm_mediatek_of_data mt8365_pwm_data = { .has_ck_26m_sel = true, }; +static const struct pwm_mediatek_of_data mt7986_pwm_data = { + .num_pwms = 2, + .pwm45_fixup = false, + .has_ck_26m_sel = true, +}; + static const struct pwm_mediatek_of_data mt8516_pwm_data = { .num_pwms = 5, .pwm45_fixup = false, @@ -342,6 +348,7 @@ static const struct of_device_id pwm_mediatek_of_match[] = { { .compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data }, { .compatible = "mediatek,mt7628-pwm", .data = &mt7628_pwm_data }, { .compatible = "mediatek,mt7629-pwm", .data = &mt7629_pwm_data }, + { .compatible = "mediatek,mt7986-pwm", .data = &mt7986_pwm_data }, { .compatible = "mediatek,mt8183-pwm", .data = &mt8183_pwm_data }, { .compatible = "mediatek,mt8365-pwm", .data = &mt8365_pwm_data }, { .compatible = "mediatek,mt8516-pwm", .data = &mt8516_pwm_data },
Add support for PWM on MT7986 which has 2 PWM channels, one of them is typically used for a temperature controlled fan. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/pwm/pwm-mediatek.c | 7 +++++++ 1 file changed, 7 insertions(+)