Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Fri, Aug 7, 2015 at 4:05 PM, Daniel Kurtz wrote:
> On Fri, Aug 7, 2015 at 10:20 AM, James Liao wrote:
>> Hi Sascha,
>>
>> On Thu, 2015-08-06 at 12:20 +0200, Sascha Hauer wrote:
>>> On Thu, Aug 06, 2015 at 05:13:21PM +0800, Daniel Kurtz wrote:
>>> > On Thu, Aug 6, 2015 at 5:00 PM, James Liao
>>> > wrote:
>>> > > Hi Sascha,
>>> > >
>>> > > On Thu, 2015-08-06 at 10:53 +0200, Sascha Hauer wrote:
>>> > >> On Thu, Aug 06, 2015 at 04:23:51PM +0800, James Liao wrote:
>>> > >> > On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
>>> > >> > > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
>>> > >> > > > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
>>> > >> > > > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m",
>>> > >> > > > 400 * MHZ),
>>> > >> > > > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m",
>>> > >> > > > "clk26m", 125 * MHZ),
>>> > >> > > > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 *
>>> > >> > > > MHZ),
>>> > >> > > > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 *
>>> > >> > > > MHZ),
>>> > >> > > > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5
>>> > >> > > > * MHZ),
>>> > >> > > > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll",
>>> > >> > > > 51.975 * MHZ),
>>> > >> > >
>>> > >> > > I would expect 51975 * KHZ here to avoid fractional numbers.
>>> > >> > > Probably
>>> > >> > > gcc calculates that during compile time so this will work as
>>> > >> > > expected,
>>> > >> > > still I'm not sure this is good style to use fractional numbers
>>> > >> > > here.
>>> > >> >
>>> > >> > As I know all constants will be calculated in compile time, so there
>>> > >> > should be no difference between 51.975 * MHZ and 51975 * KHz.
>>> > >> >
>>> > >> > > Anyway, on my system lvdspll is running at 150MHz. Are you sure
>>> > >> > > there is
>>> > >> > > a clock derived from this running at 148.5MHz? Is it really
>>> > >> > > correct to
>>> > >> > > use a fixed clock here or should it rather be lvdspll directly?
>>> > >> >
>>> > >> > Here is the clock hierarchy between lvdspll and lvds_pxl:
>>> > >> >
>>> > >> > AD_VPLL_DPIX_CK lvds_pxl
>>> > >> > -
>>> > >> >||->||-->|
>>> > >> >|| | cksys | |
>>> > >> > LVDSPLL -->| LVDSTX | | buffer | |
>>> > >> > MMSYS
>>> > >> >|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
>>> > >> >||->||-->|
>>> > >> >
>>> > >> > -
>>> > >> >
>>> > >> > Some clocks and blocks are not modeled into CCF. But we prefer to
>>> > >> > enable
>>> > >> > lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and
>>> > >> > lvds_cts)
>>> > >> > as a fixed-rate clock with a source from lvdspll.
>>> > >> >
>>> > >> > The frequency of these fixed-rate clocks (such as 148.5 MHz) are
>>> > >> > typical
>>> > >> > rate. In fact, we don't care about the actual rate of these clocks.
>>> > >> > We
>>> > >> > just care about the enable / disable sequence of them.
>>> > >>
>>> > >> Please either use the real rate or 0 (along with a explaining why).
>>> > >> Using
>>> > >> a frequency with four to five significant digits makes me think that
>>> > >> the
>>> > >> actual rate is very important.
>>> > >
>>> > > Oops, your suggestion is much different from Daniel's.
>>> > >
>>> > > Daniel, could you help to comment about how we model these clocks?
>>> >
>>> > First of all, for clocks where the rate doesn't matter, it doesn't
>>> > matters to what rate we set the clock.
>>> >
>>> > As for the color of our shed, "the designer says these are the typical
>>> > rates" sounds good enough to me for a "real rate", so I prefer using
>>> > the rates in James' patch.
>>> >
>>> > If not sure what Sascha's concern is, but if he insists on 0, I'm fine
>>> > with that too.
>>>
>>> I only find it confusing. I'd expect either the correct rate or an
>>> obviously dummy rate. Whatever we choose a comment explaining the
>>> background would really help here. Otherwise we won't know later
>>> whether this 148.5 MHz rate was introduced because a) The consumers
>>> depend on this rate being reported, b) It really is the correct rate or
>>> c) we don't care about the rate.
>>
>> So the proper setting should be:
>>
>> clk_name, parent,rate
>> -
>> "clkph_mck_o", "clk26m", 0
>> "usb_syspll_125m", "clk26m", 125 * MHZ
>> "dsi0_dig","clk26m", 0
>> "dsi1_dig","clk26m", 0
>> "lvds_pxl","lvdspll", 0
>> "lvds_cts","lvdspll", 0
>>
>> usb_syspll_125m will keep in 125 MHz event in different products.Others
>> may be changed by DRAM or display settings.
>>
>> Daniel, do you think it's OK to mod
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Fri, Aug 7, 2015 at 10:20 AM, James Liao wrote:
> Hi Sascha,
>
> On Thu, 2015-08-06 at 12:20 +0200, Sascha Hauer wrote:
>> On Thu, Aug 06, 2015 at 05:13:21PM +0800, Daniel Kurtz wrote:
>> > On Thu, Aug 6, 2015 at 5:00 PM, James Liao
>> > wrote:
>> > > Hi Sascha,
>> > >
>> > > On Thu, 2015-08-06 at 10:53 +0200, Sascha Hauer wrote:
>> > >> On Thu, Aug 06, 2015 at 04:23:51PM +0800, James Liao wrote:
>> > >> > On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
>> > >> > > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
>> > >> > > > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
>> > >> > > > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m",
>> > >> > > > 400 * MHZ),
>> > >> > > > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m",
>> > >> > > > "clk26m", 125 * MHZ),
>> > >> > > > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 *
>> > >> > > > MHZ),
>> > >> > > > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 *
>> > >> > > > MHZ),
>> > >> > > > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5
>> > >> > > > * MHZ),
>> > >> > > > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975
>> > >> > > > * MHZ),
>> > >> > >
>> > >> > > I would expect 51975 * KHZ here to avoid fractional numbers.
>> > >> > > Probably
>> > >> > > gcc calculates that during compile time so this will work as
>> > >> > > expected,
>> > >> > > still I'm not sure this is good style to use fractional numbers
>> > >> > > here.
>> > >> >
>> > >> > As I know all constants will be calculated in compile time, so there
>> > >> > should be no difference between 51.975 * MHZ and 51975 * KHz.
>> > >> >
>> > >> > > Anyway, on my system lvdspll is running at 150MHz. Are you sure
>> > >> > > there is
>> > >> > > a clock derived from this running at 148.5MHz? Is it really correct
>> > >> > > to
>> > >> > > use a fixed clock here or should it rather be lvdspll directly?
>> > >> >
>> > >> > Here is the clock hierarchy between lvdspll and lvds_pxl:
>> > >> >
>> > >> > AD_VPLL_DPIX_CK lvds_pxl -
>> > >> >||->||-->|
>> > >> >|| | cksys | |
>> > >> > LVDSPLL -->| LVDSTX | | buffer | |
>> > >> > MMSYS
>> > >> >|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
>> > >> >||->||-->|
>> > >> > -
>> > >> >
>> > >> > Some clocks and blocks are not modeled into CCF. But we prefer to
>> > >> > enable
>> > >> > lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and lvds_cts)
>> > >> > as a fixed-rate clock with a source from lvdspll.
>> > >> >
>> > >> > The frequency of these fixed-rate clocks (such as 148.5 MHz) are
>> > >> > typical
>> > >> > rate. In fact, we don't care about the actual rate of these clocks. We
>> > >> > just care about the enable / disable sequence of them.
>> > >>
>> > >> Please either use the real rate or 0 (along with a explaining why).
>> > >> Using
>> > >> a frequency with four to five significant digits makes me think that the
>> > >> actual rate is very important.
>> > >
>> > > Oops, your suggestion is much different from Daniel's.
>> > >
>> > > Daniel, could you help to comment about how we model these clocks?
>> >
>> > First of all, for clocks where the rate doesn't matter, it doesn't
>> > matters to what rate we set the clock.
>> >
>> > As for the color of our shed, "the designer says these are the typical
>> > rates" sounds good enough to me for a "real rate", so I prefer using
>> > the rates in James' patch.
>> >
>> > If not sure what Sascha's concern is, but if he insists on 0, I'm fine
>> > with that too.
>>
>> I only find it confusing. I'd expect either the correct rate or an
>> obviously dummy rate. Whatever we choose a comment explaining the
>> background would really help here. Otherwise we won't know later
>> whether this 148.5 MHz rate was introduced because a) The consumers
>> depend on this rate being reported, b) It really is the correct rate or
>> c) we don't care about the rate.
>
> So the proper setting should be:
>
> clk_name, parent,rate
> -
> "clkph_mck_o", "clk26m", 0
> "usb_syspll_125m", "clk26m", 125 * MHZ
> "dsi0_dig","clk26m", 0
> "dsi1_dig","clk26m", 0
> "lvds_pxl","lvdspll", 0
> "lvds_cts","lvdspll", 0
>
> usb_syspll_125m will keep in 125 MHz event in different products.Others
> may be changed by DRAM or display settings.
>
> Daniel, do you think it's OK to model these clocks like above?
Yes, I am fine with this.
>
>
> Best regards,
>
> James
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.or
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
Hi Sascha,
On Thu, 2015-08-06 at 12:20 +0200, Sascha Hauer wrote:
> On Thu, Aug 06, 2015 at 05:13:21PM +0800, Daniel Kurtz wrote:
> > On Thu, Aug 6, 2015 at 5:00 PM, James Liao
> > wrote:
> > > Hi Sascha,
> > >
> > > On Thu, 2015-08-06 at 10:53 +0200, Sascha Hauer wrote:
> > >> On Thu, Aug 06, 2015 at 04:23:51PM +0800, James Liao wrote:
> > >> > On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
> > >> > > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> > >> > > > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> > >> > > > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m",
> > >> > > > 400 * MHZ),
> > >> > > > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m",
> > >> > > > "clk26m", 125 * MHZ),
> > >> > > > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 *
> > >> > > > MHZ),
> > >> > > > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 *
> > >> > > > MHZ),
> > >> > > > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 *
> > >> > > > MHZ),
> > >> > > > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975
> > >> > > > * MHZ),
> > >> > >
> > >> > > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> > >> > > gcc calculates that during compile time so this will work as
> > >> > > expected,
> > >> > > still I'm not sure this is good style to use fractional numbers here.
> > >> >
> > >> > As I know all constants will be calculated in compile time, so there
> > >> > should be no difference between 51.975 * MHZ and 51975 * KHz.
> > >> >
> > >> > > Anyway, on my system lvdspll is running at 150MHz. Are you sure
> > >> > > there is
> > >> > > a clock derived from this running at 148.5MHz? Is it really correct
> > >> > > to
> > >> > > use a fixed clock here or should it rather be lvdspll directly?
> > >> >
> > >> > Here is the clock hierarchy between lvdspll and lvds_pxl:
> > >> >
> > >> > AD_VPLL_DPIX_CK lvds_pxl -
> > >> >||->||-->|
> > >> >|| | cksys | |
> > >> > LVDSPLL -->| LVDSTX | | buffer | | MMSYS
> > >> >|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
> > >> >||->||-->|
> > >> > -
> > >> >
> > >> > Some clocks and blocks are not modeled into CCF. But we prefer to
> > >> > enable
> > >> > lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and lvds_cts)
> > >> > as a fixed-rate clock with a source from lvdspll.
> > >> >
> > >> > The frequency of these fixed-rate clocks (such as 148.5 MHz) are
> > >> > typical
> > >> > rate. In fact, we don't care about the actual rate of these clocks. We
> > >> > just care about the enable / disable sequence of them.
> > >>
> > >> Please either use the real rate or 0 (along with a explaining why). Using
> > >> a frequency with four to five significant digits makes me think that the
> > >> actual rate is very important.
> > >
> > > Oops, your suggestion is much different from Daniel's.
> > >
> > > Daniel, could you help to comment about how we model these clocks?
> >
> > First of all, for clocks where the rate doesn't matter, it doesn't
> > matters to what rate we set the clock.
> >
> > As for the color of our shed, "the designer says these are the typical
> > rates" sounds good enough to me for a "real rate", so I prefer using
> > the rates in James' patch.
> >
> > If not sure what Sascha's concern is, but if he insists on 0, I'm fine
> > with that too.
>
> I only find it confusing. I'd expect either the correct rate or an
> obviously dummy rate. Whatever we choose a comment explaining the
> background would really help here. Otherwise we won't know later
> whether this 148.5 MHz rate was introduced because a) The consumers
> depend on this rate being reported, b) It really is the correct rate or
> c) we don't care about the rate.
So the proper setting should be:
clk_name, parent,rate
-
"clkph_mck_o", "clk26m", 0
"usb_syspll_125m", "clk26m", 125 * MHZ
"dsi0_dig","clk26m", 0
"dsi1_dig","clk26m", 0
"lvds_pxl","lvdspll", 0
"lvds_cts","lvdspll", 0
usb_syspll_125m will keep in 125 MHz event in different products.Others
may be changed by DRAM or display settings.
Daniel, do you think it's OK to model these clocks like above?
Best regards,
James
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Thu, Aug 06, 2015 at 05:13:21PM +0800, Daniel Kurtz wrote:
> On Thu, Aug 6, 2015 at 5:00 PM, James Liao wrote:
> > Hi Sascha,
> >
> > On Thu, 2015-08-06 at 10:53 +0200, Sascha Hauer wrote:
> >> On Thu, Aug 06, 2015 at 04:23:51PM +0800, James Liao wrote:
> >> > On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
> >> > > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> >> > > > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> >> > > > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400
> >> > > > * MHZ),
> >> > > > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m",
> >> > > > "clk26m", 125 * MHZ),
> >> > > > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
> >> > > > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
> >> > > > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 *
> >> > > > MHZ),
> >> > > > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 *
> >> > > > MHZ),
> >> > >
> >> > > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> >> > > gcc calculates that during compile time so this will work as expected,
> >> > > still I'm not sure this is good style to use fractional numbers here.
> >> >
> >> > As I know all constants will be calculated in compile time, so there
> >> > should be no difference between 51.975 * MHZ and 51975 * KHz.
> >> >
> >> > > Anyway, on my system lvdspll is running at 150MHz. Are you sure there
> >> > > is
> >> > > a clock derived from this running at 148.5MHz? Is it really correct to
> >> > > use a fixed clock here or should it rather be lvdspll directly?
> >> >
> >> > Here is the clock hierarchy between lvdspll and lvds_pxl:
> >> >
> >> > AD_VPLL_DPIX_CK lvds_pxl -
> >> >||->||-->|
> >> >|| | cksys | |
> >> > LVDSPLL -->| LVDSTX | | buffer | | MMSYS
> >> >|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
> >> >||->||-->|
> >> > -
> >> >
> >> > Some clocks and blocks are not modeled into CCF. But we prefer to enable
> >> > lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and lvds_cts)
> >> > as a fixed-rate clock with a source from lvdspll.
> >> >
> >> > The frequency of these fixed-rate clocks (such as 148.5 MHz) are typical
> >> > rate. In fact, we don't care about the actual rate of these clocks. We
> >> > just care about the enable / disable sequence of them.
> >>
> >> Please either use the real rate or 0 (along with a explaining why). Using
> >> a frequency with four to five significant digits makes me think that the
> >> actual rate is very important.
> >
> > Oops, your suggestion is much different from Daniel's.
> >
> > Daniel, could you help to comment about how we model these clocks?
>
> First of all, for clocks where the rate doesn't matter, it doesn't
> matters to what rate we set the clock.
>
> As for the color of our shed, "the designer says these are the typical
> rates" sounds good enough to me for a "real rate", so I prefer using
> the rates in James' patch.
>
> If not sure what Sascha's concern is, but if he insists on 0, I'm fine
> with that too.
I only find it confusing. I'd expect either the correct rate or an
obviously dummy rate. Whatever we choose a comment explaining the
background would really help here. Otherwise we won't know later
whether this 148.5 MHz rate was introduced because a) The consumers
depend on this rate being reported, b) It really is the correct rate or
c) we don't care about the rate.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Thu, Aug 6, 2015 at 5:00 PM, James Liao wrote:
> Hi Sascha,
>
> On Thu, 2015-08-06 at 10:53 +0200, Sascha Hauer wrote:
>> On Thu, Aug 06, 2015 at 04:23:51PM +0800, James Liao wrote:
>> > On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
>> > > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
>> > > > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
>> > > > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 *
>> > > > MHZ),
>> > > > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m",
>> > > > "clk26m", 125 * MHZ),
>> > > > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
>> > > > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
>> > > > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 *
>> > > > MHZ),
>> > > > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 *
>> > > > MHZ),
>> > >
>> > > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
>> > > gcc calculates that during compile time so this will work as expected,
>> > > still I'm not sure this is good style to use fractional numbers here.
>> >
>> > As I know all constants will be calculated in compile time, so there
>> > should be no difference between 51.975 * MHZ and 51975 * KHz.
>> >
>> > > Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
>> > > a clock derived from this running at 148.5MHz? Is it really correct to
>> > > use a fixed clock here or should it rather be lvdspll directly?
>> >
>> > Here is the clock hierarchy between lvdspll and lvds_pxl:
>> >
>> > AD_VPLL_DPIX_CK lvds_pxl -
>> >||->||-->|
>> >|| | cksys | |
>> > LVDSPLL -->| LVDSTX | | buffer | | MMSYS
>> >|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
>> >||->||-->|
>> > -
>> >
>> > Some clocks and blocks are not modeled into CCF. But we prefer to enable
>> > lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and lvds_cts)
>> > as a fixed-rate clock with a source from lvdspll.
>> >
>> > The frequency of these fixed-rate clocks (such as 148.5 MHz) are typical
>> > rate. In fact, we don't care about the actual rate of these clocks. We
>> > just care about the enable / disable sequence of them.
>>
>> Please either use the real rate or 0 (along with a explaining why). Using
>> a frequency with four to five significant digits makes me think that the
>> actual rate is very important.
>
> Oops, your suggestion is much different from Daniel's.
>
> Daniel, could you help to comment about how we model these clocks?
First of all, for clocks where the rate doesn't matter, it doesn't
matters to what rate we set the clock.
As for the color of our shed, "the designer says these are the typical
rates" sounds good enough to me for a "real rate", so I prefer using
the rates in James' patch.
If not sure what Sascha's concern is, but if he insists on 0, I'm fine
with that too.
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
Hi Sascha,
On Thu, 2015-08-06 at 10:53 +0200, Sascha Hauer wrote:
> On Thu, Aug 06, 2015 at 04:23:51PM +0800, James Liao wrote:
> > On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
> > > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> > > > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> > > > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 *
> > > > MHZ),
> > > > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m",
> > > > 125 * MHZ),
> > > > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
> > > > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
> > > > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
> > > > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 *
> > > > MHZ),
> > >
> > > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> > > gcc calculates that during compile time so this will work as expected,
> > > still I'm not sure this is good style to use fractional numbers here.
> >
> > As I know all constants will be calculated in compile time, so there
> > should be no difference between 51.975 * MHZ and 51975 * KHz.
> >
> > > Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
> > > a clock derived from this running at 148.5MHz? Is it really correct to
> > > use a fixed clock here or should it rather be lvdspll directly?
> >
> > Here is the clock hierarchy between lvdspll and lvds_pxl:
> >
> > AD_VPLL_DPIX_CK lvds_pxl -
> >||->||-->|
> >|| | cksys | |
> > LVDSPLL -->| LVDSTX | | buffer | | MMSYS
> >|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
> >||->||-->|
> > -
> >
> > Some clocks and blocks are not modeled into CCF. But we prefer to enable
> > lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and lvds_cts)
> > as a fixed-rate clock with a source from lvdspll.
> >
> > The frequency of these fixed-rate clocks (such as 148.5 MHz) are typical
> > rate. In fact, we don't care about the actual rate of these clocks. We
> > just care about the enable / disable sequence of them.
>
> Please either use the real rate or 0 (along with a explaining why). Using
> a frequency with four to five significant digits makes me think that the
> actual rate is very important.
Oops, your suggestion is much different from Daniel's.
Daniel, could you help to comment about how we model these clocks?
Best regards,
James
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Thu, Aug 06, 2015 at 04:23:51PM +0800, James Liao wrote:
> Hi Sascha,
>
> On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
> > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> > > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> > > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 * MHZ),
> > > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m", 125 *
> > > MHZ),
> > > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
> > > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
> > > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
> > > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
> >
> > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> > gcc calculates that during compile time so this will work as expected,
> > still I'm not sure this is good style to use fractional numbers here.
>
> As I know all constants will be calculated in compile time, so there
> should be no difference between 51.975 * MHZ and 51975 * KHz.
>
> > Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
> > a clock derived from this running at 148.5MHz? Is it really correct to
> > use a fixed clock here or should it rather be lvdspll directly?
>
> Here is the clock hierarchy between lvdspll and lvds_pxl:
>
> AD_VPLL_DPIX_CK lvds_pxl -
>||->||-->|
>|| | cksys | |
> LVDSPLL -->| LVDSTX | | buffer | | MMSYS
>|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
>||->||-->|
> -
>
> Some clocks and blocks are not modeled into CCF. But we prefer to enable
> lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and lvds_cts)
> as a fixed-rate clock with a source from lvdspll.
>
> The frequency of these fixed-rate clocks (such as 148.5 MHz) are typical
> rate. In fact, we don't care about the actual rate of these clocks. We
> just care about the enable / disable sequence of them.
Please either use the real rate or 0 (along with a explaining why). Using
a frequency with four to five significant digits makes me think that the
actual rate is very important.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
Hi Sascha,
On Wed, 2015-08-05 at 08:46 +0200, Sascha Hauer wrote:
> On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> > static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> > FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 * MHZ),
> > FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m", 125 *
> > MHZ),
> > + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
> > + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
> > + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
> > + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
>
> I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> gcc calculates that during compile time so this will work as expected,
> still I'm not sure this is good style to use fractional numbers here.
As I know all constants will be calculated in compile time, so there
should be no difference between 51.975 * MHZ and 51975 * KHz.
> Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
> a clock derived from this running at 148.5MHz? Is it really correct to
> use a fixed clock here or should it rather be lvdspll directly?
Here is the clock hierarchy between lvdspll and lvds_pxl:
AD_VPLL_DPIX_CK lvds_pxl -
||->||-->|
|| | cksys | |
LVDSPLL -->| LVDSTX | | buffer | | MMSYS
|| AD_LVDSTX_CLKDIG_CTS | test | lvds_cts |
||->||-->|
-
Some clocks and blocks are not modeled into CCF. But we prefer to enable
lvdspll before enabling lvds_pxl. So I modeled lvds_pxl (and lvds_cts)
as a fixed-rate clock with a source from lvdspll.
The frequency of these fixed-rate clocks (such as 148.5 MHz) are typical
rate. In fact, we don't care about the actual rate of these clocks. We
just care about the enable / disable sequence of them.
Best regards,
James
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Wed, Aug 5, 2015 at 3:50 PM, Sascha Hauer wrote:
> On Wed, Aug 05, 2015 at 03:41:49PM +0800, Daniel Kurtz wrote:
>> On Wed, Aug 5, 2015 at 3:36 PM, Sascha Hauer wrote:
>> > On Wed, Aug 05, 2015 at 03:26:29PM +0800, Daniel Kurtz wrote:
>> >> On Wed, Aug 5, 2015 at 2:46 PM, Sascha Hauer
>> >> wrote:
>> >> > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
>> >> >> Most multimedia subsystem clocks will be accessed by multiple
>> >> >> drivers, so it's a better way to manage these clocks in CCF.
>> >> >> This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
>> >> >> subsystems.
>> >> >>
>> >> >> Signed-off-by: James Liao
>> >> >> ---
>> >> >> drivers/clk/mediatek/clk-mt8173.c | 267
>> >> >> +
>> >> >> include/dt-bindings/clock/mt8173-clk.h | 97 +++-
>> >> >> 2 files changed, 361 insertions(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/clk/mediatek/clk-mt8173.c
>> >> >> b/drivers/clk/mediatek/clk-mt8173.c
>> >> >> index f37ace6..05335e5 100644
>> >> >> --- a/drivers/clk/mediatek/clk-mt8173.c
>> >> >> +++ b/drivers/clk/mediatek/clk-mt8173.c
>> >> >> @@ -25,6 +25,10 @@ static DEFINE_SPINLOCK(mt8173_clk_lock);
>> >> >> static const struct mtk_fixed_clk fixed_clks[] __initconst = {
>> >> >> FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 *
>> >> >> MHZ),
>> >> >> FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m",
>> >> >> 125 * MHZ),
>> >> >> + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
>> >> >> + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
>> >> >> + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
>> >> >> + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
>> >> >
>> >> > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
>> >> > gcc calculates that during compile time so this will work as expected,
>> >> > still I'm not sure this is good style to use fractional numbers here.
>> >>
>> >> I thought this looked a bit strange too, but for what its worth, these
>> >> two evaluate correctly:
>> >>
>> >> localhost ~ # cat /sys/kernel/debug/clk/clk_summary | grep lvds
>> >> lvdspll 00 14878
>> >>0 0
>> >>lvds_pxl 00 14850
>> >>0 0
>> >>lvds_cts 0051975000
>> >>0 0
>> >>
>> >> >
>> >> > Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
>> >> > a clock derived from this running at 148.5MHz? Is it really correct to
>> >> > use a fixed clock here or should it rather be lvdspll directly?
>> >>
>> >> I agree it does look strange to have a 51.975 MHz and 148.5 MHz clocks
>> >> with a 150 MHz PLL as their parent... However, I'm not sure how much
>> >> this matters? I think the idea here was that these frequencies are
>> >> best effort "nominal" clock values provided by Mediatek "designers".
>> >> The important point is that for the hardware to generate these either
>> >> of these clocks, lvdspll must be enabled.
>> >
>> > This assumes that the lvdspll always runs at frequency the designers
>> > thought that might be a good value. Should we really provide wrong clock
>> > values when on some board for whatever reason the lvdspll is configured
>> > for a different frequency?
>>
>> Do you have an alternative suggestion for estimating the frequency of
>> a non-software controllable or measurable hardware clock?
>
> What's the problem with using lvdspll directly? The consumers of these
> clocks should be able to cope with the deviation between nomial
> frequency and actual frequency.
Oh, I'm totally fine with that too. Although I think the way James
has it better models the hardware.
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Wed, Aug 05, 2015 at 03:41:49PM +0800, Daniel Kurtz wrote:
> On Wed, Aug 5, 2015 at 3:36 PM, Sascha Hauer wrote:
> > On Wed, Aug 05, 2015 at 03:26:29PM +0800, Daniel Kurtz wrote:
> >> On Wed, Aug 5, 2015 at 2:46 PM, Sascha Hauer
> >> wrote:
> >> > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> >> >> Most multimedia subsystem clocks will be accessed by multiple
> >> >> drivers, so it's a better way to manage these clocks in CCF.
> >> >> This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
> >> >> subsystems.
> >> >>
> >> >> Signed-off-by: James Liao
> >> >> ---
> >> >> drivers/clk/mediatek/clk-mt8173.c | 267
> >> >> +
> >> >> include/dt-bindings/clock/mt8173-clk.h | 97 +++-
> >> >> 2 files changed, 361 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/clk/mediatek/clk-mt8173.c
> >> >> b/drivers/clk/mediatek/clk-mt8173.c
> >> >> index f37ace6..05335e5 100644
> >> >> --- a/drivers/clk/mediatek/clk-mt8173.c
> >> >> +++ b/drivers/clk/mediatek/clk-mt8173.c
> >> >> @@ -25,6 +25,10 @@ static DEFINE_SPINLOCK(mt8173_clk_lock);
> >> >> static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> >> >> FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 *
> >> >> MHZ),
> >> >> FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m",
> >> >> 125 * MHZ),
> >> >> + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
> >> >> + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
> >> >> + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
> >> >> + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
> >> >
> >> > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> >> > gcc calculates that during compile time so this will work as expected,
> >> > still I'm not sure this is good style to use fractional numbers here.
> >>
> >> I thought this looked a bit strange too, but for what its worth, these
> >> two evaluate correctly:
> >>
> >> localhost ~ # cat /sys/kernel/debug/clk/clk_summary | grep lvds
> >> lvdspll 00 14878
> >>0 0
> >>lvds_pxl 00 14850
> >>0 0
> >>lvds_cts 0051975000
> >>0 0
> >>
> >> >
> >> > Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
> >> > a clock derived from this running at 148.5MHz? Is it really correct to
> >> > use a fixed clock here or should it rather be lvdspll directly?
> >>
> >> I agree it does look strange to have a 51.975 MHz and 148.5 MHz clocks
> >> with a 150 MHz PLL as their parent... However, I'm not sure how much
> >> this matters? I think the idea here was that these frequencies are
> >> best effort "nominal" clock values provided by Mediatek "designers".
> >> The important point is that for the hardware to generate these either
> >> of these clocks, lvdspll must be enabled.
> >
> > This assumes that the lvdspll always runs at frequency the designers
> > thought that might be a good value. Should we really provide wrong clock
> > values when on some board for whatever reason the lvdspll is configured
> > for a different frequency?
>
> Do you have an alternative suggestion for estimating the frequency of
> a non-software controllable or measurable hardware clock?
What's the problem with using lvdspll directly? The consumers of these
clocks should be able to cope with the deviation between nomial
frequency and actual frequency.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Wed, Aug 5, 2015 at 3:36 PM, Sascha Hauer wrote:
> On Wed, Aug 05, 2015 at 03:26:29PM +0800, Daniel Kurtz wrote:
>> On Wed, Aug 5, 2015 at 2:46 PM, Sascha Hauer wrote:
>> > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
>> >> Most multimedia subsystem clocks will be accessed by multiple
>> >> drivers, so it's a better way to manage these clocks in CCF.
>> >> This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
>> >> subsystems.
>> >>
>> >> Signed-off-by: James Liao
>> >> ---
>> >> drivers/clk/mediatek/clk-mt8173.c | 267
>> >> +
>> >> include/dt-bindings/clock/mt8173-clk.h | 97 +++-
>> >> 2 files changed, 361 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/clk/mediatek/clk-mt8173.c
>> >> b/drivers/clk/mediatek/clk-mt8173.c
>> >> index f37ace6..05335e5 100644
>> >> --- a/drivers/clk/mediatek/clk-mt8173.c
>> >> +++ b/drivers/clk/mediatek/clk-mt8173.c
>> >> @@ -25,6 +25,10 @@ static DEFINE_SPINLOCK(mt8173_clk_lock);
>> >> static const struct mtk_fixed_clk fixed_clks[] __initconst = {
>> >> FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 * MHZ),
>> >> FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m", 125
>> >> * MHZ),
>> >> + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
>> >> + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
>> >> + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
>> >> + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
>> >
>> > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
>> > gcc calculates that during compile time so this will work as expected,
>> > still I'm not sure this is good style to use fractional numbers here.
>>
>> I thought this looked a bit strange too, but for what its worth, these
>> two evaluate correctly:
>>
>> localhost ~ # cat /sys/kernel/debug/clk/clk_summary | grep lvds
>> lvdspll 00 14878
>>0 0
>>lvds_pxl 00 14850
>>0 0
>>lvds_cts 0051975000
>>0 0
>>
>> >
>> > Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
>> > a clock derived from this running at 148.5MHz? Is it really correct to
>> > use a fixed clock here or should it rather be lvdspll directly?
>>
>> I agree it does look strange to have a 51.975 MHz and 148.5 MHz clocks
>> with a 150 MHz PLL as their parent... However, I'm not sure how much
>> this matters? I think the idea here was that these frequencies are
>> best effort "nominal" clock values provided by Mediatek "designers".
>> The important point is that for the hardware to generate these either
>> of these clocks, lvdspll must be enabled.
>
> This assumes that the lvdspll always runs at frequency the designers
> thought that might be a good value. Should we really provide wrong clock
> values when on some board for whatever reason the lvdspll is configured
> for a different frequency?
Do you have an alternative suggestion for estimating the frequency of
a non-software controllable or measurable hardware clock?
Or can we land this series and update the frequency via some future
patch if and when that other board arrives which may have a different
frequency - and there is some reason to care what that frequency
actually is?
-Dan
>
> sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Wed, Aug 05, 2015 at 03:26:29PM +0800, Daniel Kurtz wrote:
> On Wed, Aug 5, 2015 at 2:46 PM, Sascha Hauer wrote:
> > On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> >> Most multimedia subsystem clocks will be accessed by multiple
> >> drivers, so it's a better way to manage these clocks in CCF.
> >> This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
> >> subsystems.
> >>
> >> Signed-off-by: James Liao
> >> ---
> >> drivers/clk/mediatek/clk-mt8173.c | 267
> >> +
> >> include/dt-bindings/clock/mt8173-clk.h | 97 +++-
> >> 2 files changed, 361 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/clk/mediatek/clk-mt8173.c
> >> b/drivers/clk/mediatek/clk-mt8173.c
> >> index f37ace6..05335e5 100644
> >> --- a/drivers/clk/mediatek/clk-mt8173.c
> >> +++ b/drivers/clk/mediatek/clk-mt8173.c
> >> @@ -25,6 +25,10 @@ static DEFINE_SPINLOCK(mt8173_clk_lock);
> >> static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> >> FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 * MHZ),
> >> FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m", 125
> >> * MHZ),
> >> + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
> >> + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
> >> + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
> >> + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
> >
> > I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> > gcc calculates that during compile time so this will work as expected,
> > still I'm not sure this is good style to use fractional numbers here.
>
> I thought this looked a bit strange too, but for what its worth, these
> two evaluate correctly:
>
> localhost ~ # cat /sys/kernel/debug/clk/clk_summary | grep lvds
> lvdspll 00 14878
>0 0
>lvds_pxl 00 14850
>0 0
>lvds_cts 0051975000
>0 0
>
> >
> > Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
> > a clock derived from this running at 148.5MHz? Is it really correct to
> > use a fixed clock here or should it rather be lvdspll directly?
>
> I agree it does look strange to have a 51.975 MHz and 148.5 MHz clocks
> with a 150 MHz PLL as their parent... However, I'm not sure how much
> this matters? I think the idea here was that these frequencies are
> best effort "nominal" clock values provided by Mediatek "designers".
> The important point is that for the hardware to generate these either
> of these clocks, lvdspll must be enabled.
This assumes that the lvdspll always runs at frequency the designers
thought that might be a good value. Should we really provide wrong clock
values when on some board for whatever reason the lvdspll is configured
for a different frequency?
sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Wed, Aug 5, 2015 at 2:46 PM, Sascha Hauer wrote:
> On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
>> Most multimedia subsystem clocks will be accessed by multiple
>> drivers, so it's a better way to manage these clocks in CCF.
>> This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
>> subsystems.
>>
>> Signed-off-by: James Liao
>> ---
>> drivers/clk/mediatek/clk-mt8173.c | 267
>> +
>> include/dt-bindings/clock/mt8173-clk.h | 97 +++-
>> 2 files changed, 361 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8173.c
>> b/drivers/clk/mediatek/clk-mt8173.c
>> index f37ace6..05335e5 100644
>> --- a/drivers/clk/mediatek/clk-mt8173.c
>> +++ b/drivers/clk/mediatek/clk-mt8173.c
>> @@ -25,6 +25,10 @@ static DEFINE_SPINLOCK(mt8173_clk_lock);
>> static const struct mtk_fixed_clk fixed_clks[] __initconst = {
>> FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 * MHZ),
>> FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m", 125 *
>> MHZ),
>> + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
>> + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
>> + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
>> + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
>
> I would expect 51975 * KHZ here to avoid fractional numbers. Probably
> gcc calculates that during compile time so this will work as expected,
> still I'm not sure this is good style to use fractional numbers here.
I thought this looked a bit strange too, but for what its worth, these
two evaluate correctly:
localhost ~ # cat /sys/kernel/debug/clk/clk_summary | grep lvds
lvdspll 00 14878
0 0
lvds_pxl 00 14850
0 0
lvds_cts 0051975000
0 0
>
> Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
> a clock derived from this running at 148.5MHz? Is it really correct to
> use a fixed clock here or should it rather be lvdspll directly?
I agree it does look strange to have a 51.975 MHz and 148.5 MHz clocks
with a 150 MHz PLL as their parent... However, I'm not sure how much
this matters? I think the idea here was that these frequencies are
best effort "nominal" clock values provided by Mediatek "designers".
The important point is that for the hardware to generate these either
of these clocks, lvdspll must be enabled.
-Dan
> Sascha
>
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
On Tue, Aug 04, 2015 at 04:16:56PM +0800, James Liao wrote:
> Most multimedia subsystem clocks will be accessed by multiple
> drivers, so it's a better way to manage these clocks in CCF.
> This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
> subsystems.
>
> Signed-off-by: James Liao
> ---
> drivers/clk/mediatek/clk-mt8173.c | 267
> +
> include/dt-bindings/clock/mt8173-clk.h | 97 +++-
> 2 files changed, 361 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8173.c
> b/drivers/clk/mediatek/clk-mt8173.c
> index f37ace6..05335e5 100644
> --- a/drivers/clk/mediatek/clk-mt8173.c
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -25,6 +25,10 @@ static DEFINE_SPINLOCK(mt8173_clk_lock);
> static const struct mtk_fixed_clk fixed_clks[] __initconst = {
> FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 * MHZ),
> FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m", 125 *
> MHZ),
> + FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
> + FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
> + FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
> + FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
I would expect 51975 * KHZ here to avoid fractional numbers. Probably
gcc calculates that during compile time so this will work as expected,
still I'm not sure this is good style to use fractional numbers here.
Anyway, on my system lvdspll is running at 150MHz. Are you sure there is
a clock derived from this running at 148.5MHz? Is it really correct to
use a fixed clock here or should it rather be lvdspll directly?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v6 7/9] clk: mediatek: Add subsystem clocks of MT8173
Most multimedia subsystem clocks will be accessed by multiple
drivers, so it's a better way to manage these clocks in CCF.
This patch adds clock support for MM, IMG, VDEC, VENC and VENC_LT
subsystems.
Signed-off-by: James Liao
---
drivers/clk/mediatek/clk-mt8173.c | 267 +
include/dt-bindings/clock/mt8173-clk.h | 97 +++-
2 files changed, 361 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/mediatek/clk-mt8173.c
b/drivers/clk/mediatek/clk-mt8173.c
index f37ace6..05335e5 100644
--- a/drivers/clk/mediatek/clk-mt8173.c
+++ b/drivers/clk/mediatek/clk-mt8173.c
@@ -25,6 +25,10 @@ static DEFINE_SPINLOCK(mt8173_clk_lock);
static const struct mtk_fixed_clk fixed_clks[] __initconst = {
FIXED_CLK(CLK_TOP_CLKPH_MCK_O, "clkph_mck_o", "clk26m", 400 * MHZ),
FIXED_CLK(CLK_TOP_USB_SYSPLL_125M, "usb_syspll_125m", "clk26m", 125 *
MHZ),
+ FIXED_CLK(CLK_TOP_DSI0_DIG, "dsi0_dig", "clk26m", 130 * MHZ),
+ FIXED_CLK(CLK_TOP_DSI1_DIG, "dsi1_dig", "clk26m", 130 * MHZ),
+ FIXED_CLK(CLK_TOP_LVDS_PXL, "lvds_pxl", "lvdspll", 148.5 * MHZ),
+ FIXED_CLK(CLK_TOP_LVDS_CTS, "lvds_cts", "lvdspll", 51.975 * MHZ),
};
static const struct mtk_fixed_factor top_divs[] __initconst = {
@@ -697,6 +701,183 @@ static const struct mtk_composite peri_clks[] __initconst
= {
MUX(CLK_PERI_UART3_SEL, "uart3_ck_sel", uart_ck_sel_parents, 0x40c, 3,
1),
};
+static const struct mtk_gate_regs cg_regs_4_8_0 __initconst = {
+ .set_ofs = 0x0004,
+ .clr_ofs = 0x0008,
+ .sta_ofs = 0x,
+};
+
+#define GATE_IMG(_id, _name, _parent, _shift) {\
+ .id = _id, \
+ .name = _name, \
+ .parent_name = _parent, \
+ .regs = &cg_regs_4_8_0, \
+ .shift = _shift,\
+ .ops = &mtk_clk_gate_ops_setclr,\
+ }
+
+static const struct mtk_gate img_clks[] __initconst = {
+ GATE_IMG(CLK_IMG_LARB2_SMI, "img_larb2_smi", "mm_sel", 0),
+ GATE_IMG(CLK_IMG_CAM_SMI, "img_cam_smi", "mm_sel", 5),
+ GATE_IMG(CLK_IMG_CAM_CAM, "img_cam_cam", "mm_sel", 6),
+ GATE_IMG(CLK_IMG_SEN_TG, "img_sen_tg", "camtg_sel", 7),
+ GATE_IMG(CLK_IMG_SEN_CAM, "img_sen_cam", "mm_sel", 8),
+ GATE_IMG(CLK_IMG_CAM_SV, "img_cam_sv", "mm_sel", 9),
+ GATE_IMG(CLK_IMG_FD, "img_fd", "mm_sel", 11),
+};
+
+static const struct mtk_gate_regs mm0_cg_regs __initconst = {
+ .set_ofs = 0x0104,
+ .clr_ofs = 0x0108,
+ .sta_ofs = 0x0100,
+};
+
+static const struct mtk_gate_regs mm1_cg_regs __initconst = {
+ .set_ofs = 0x0114,
+ .clr_ofs = 0x0118,
+ .sta_ofs = 0x0110,
+};
+
+#define GATE_MM0(_id, _name, _parent, _shift) {\
+ .id = _id, \
+ .name = _name, \
+ .parent_name = _parent, \
+ .regs = &mm0_cg_regs, \
+ .shift = _shift,\
+ .ops = &mtk_clk_gate_ops_setclr,\
+ }
+
+#define GATE_MM1(_id, _name, _parent, _shift) {\
+ .id = _id, \
+ .name = _name, \
+ .parent_name = _parent, \
+ .regs = &mm1_cg_regs, \
+ .shift = _shift,\
+ .ops = &mtk_clk_gate_ops_setclr,\
+ }
+
+static const struct mtk_gate mm_clks[] __initconst = {
+ /* MM0 */
+ GATE_MM0(CLK_MM_SMI_COMMON, "mm_smi_common", "mm_sel", 0),
+ GATE_MM0(CLK_MM_SMI_LARB0, "mm_smi_larb0", "mm_sel", 1),
+ GATE_MM0(CLK_MM_CAM_MDP, "mm_cam_mdp", "mm_sel", 2),
+ GATE_MM0(CLK_MM_MDP_RDMA0, "mm_mdp_rdma0", "mm_sel", 3),
+ GATE_MM0(CLK_MM_MDP_RDMA1, "mm_mdp_rdma1", "mm_sel", 4),
+ GATE_MM0(CLK_MM_MDP_RSZ0, "mm_mdp_rsz0", "mm_sel", 5),
+ GATE_MM0(CLK_MM_MDP_RSZ1, "mm_mdp_rsz1", "mm_sel", 6),
+ GATE_MM0(CLK_MM_MDP_RSZ2, "mm_mdp_rsz2", "mm_sel", 7),
+ GATE_MM0(CLK_MM_MDP_TDSHP0, "mm_mdp_tdshp0", "mm_sel", 8),
+ GATE_MM0(CLK_MM_MDP_TDSHP1, "mm_mdp_tdshp1", "mm_sel", 9),
+ GATE_MM0(CLK_MM_MDP_WDMA, "mm_mdp_wdma", "mm_sel", 11),
+ GATE_MM0(CLK_MM_MDP_WROT0, "mm_mdp_wrot0", "mm_sel", 12),
+ GATE_MM0(CLK_MM_MDP_WROT1, "mm_mdp_wrot1", "mm_sel", 13),
+ GATE_MM0(CLK_MM_FAKE_ENG, "mm_fake_eng", "mm_sel", 14),
+ GATE_MM0(CLK_MM_MUTEX_32K, "mm_mutex_32k", "rtc_sel", 15),
+ GATE_MM0(CLK_MM_DISP_OVL0, "mm_disp_ovl0", "mm_sel", 16),
+ GATE_MM0(CLK_MM_DISP_OVL1, "mm_disp_ovl1", "m
