| Message ID | 1432131540-2523-6-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 20, 2015 at 10:19 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > This adds the SCPSYS device node to the MT8173 dtsi file. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > index 924fdb6..12430f0 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > @@ -125,6 +125,16 @@ > <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>; > }; > > + scpsys: scpsys@10006000 { > + compatible = "mediatek,mt8173-scpsys"; > + #power-domain-cells = <1>; > + reg = <0 0x10006000 0 0x1000>; > + clocks = <&clk26m>, Why is mfg using <&clk26m> and not <&topckgen CLK_TOP_MFG_SEL>? I saw another patch set on the list today from James Liao that adds more clocks. Perhaps we can move the SCPSYS set on top of that one and include more clocks? > + <&topckgen CLK_TOP_MM_SEL>; FYI: the devicetree changes in this set depend on your other patch set starting with: https://patchwork.kernel.org/patch/6446341/ "arm64: dts: mt8173: Add clock controller device nodes" This patch isn't based on top of the other set, though, so it may lead to a small merge conflict when folding in the .dtsi device nodes (ie, placing scpsys@10006000 after pwrap@1000d000). I'm not sure how people usually resolve this or manage the ordering of co-dependent patch sets upstream. -Dan > + clock-names = "mfg", "mm"; > + infracfg = <&infracfg>; > + }; > + > sysirq: intpol-controller@10200620 { > compatible = "mediatek,mt8173-sysirq", > "mediatek,mt6577-sysirq"; > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
2015-05-21 16:32 GMT+02:00 Daniel Kurtz <djkurtz@chromium.org>: > On Wed, May 20, 2015 at 10:19 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: >> This adds the SCPSYS device node to the MT8173 dtsi file. >> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> --- >> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi >> index 924fdb6..12430f0 100644 >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi >> @@ -125,6 +125,16 @@ >> <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>; >> }; >> >> + scpsys: scpsys@10006000 { >> + compatible = "mediatek,mt8173-scpsys"; >> + #power-domain-cells = <1>; >> + reg = <0 0x10006000 0 0x1000>; >> + clocks = <&clk26m>, > > Why is mfg using <&clk26m> and not <&topckgen CLK_TOP_MFG_SEL>? > I saw another patch set on the list today from James Liao that adds more clocks. > Perhaps we can move the SCPSYS set on top of that one and include more clocks? > >> + <&topckgen CLK_TOP_MM_SEL>; > > FYI: the devicetree changes in this set depend on your other patch set > starting with: > > https://patchwork.kernel.org/patch/6446341/ > "arm64: dts: mt8173: Add clock controller device nodes" > > This patch isn't based on top of the other set, though, so it may lead > to a small merge conflict when folding in the .dtsi device nodes (ie, > placing scpsys@10006000 after pwrap@1000d000). > > I'm not sure how people usually resolve this or manage the ordering of > co-dependent patch sets upstream. At the moment I suppose that patch-sets are based on -rc1 if not stated different.
On Thu, May 21, 2015 at 10:32:40PM +0800, Daniel Kurtz wrote: > On Wed, May 20, 2015 at 10:19 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > This adds the SCPSYS device node to the MT8173 dtsi file. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > index 924fdb6..12430f0 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > @@ -125,6 +125,16 @@ > > <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>; > > }; > > > > + scpsys: scpsys@10006000 { > > + compatible = "mediatek,mt8173-scpsys"; > > + #power-domain-cells = <1>; > > + reg = <0 0x10006000 0 0x1000>; > > + clocks = <&clk26m>, > > Why is mfg using <&clk26m> and not <&topckgen CLK_TOP_MFG_SEL>? Because James Liao said to me that it is derived from clk26m and not from mfg_sel. > I saw another patch set on the list today from James Liao that adds more clocks. > Perhaps we can move the SCPSYS set on top of that one and include more clocks? > > > + <&topckgen CLK_TOP_MM_SEL>; > > FYI: the devicetree changes in this set depend on your other patch set > starting with: > > https://patchwork.kernel.org/patch/6446341/ > "arm64: dts: mt8173: Add clock controller device nodes" > > This patch isn't based on top of the other set, though, so it may lead > to a small merge conflict when folding in the .dtsi device nodes (ie, > placing scpsys@10006000 after pwrap@1000d000). > > I'm not sure how people usually resolve this or manage the ordering of > co-dependent patch sets upstream. This merge conflict should be easy to solve. However, I can rebase this on Matthias' dts branch if desired. Sascha
Hi Daniel, On Thu, 2015-05-21 at 19:49 +0200, Sascha Hauer wrote: > On Thu, May 21, 2015 at 10:32:40PM +0800, Daniel Kurtz wrote: > > > + scpsys: scpsys@10006000 { > > > + compatible = "mediatek,mt8173-scpsys"; > > > + #power-domain-cells = <1>; > > > + reg = <0 0x10006000 0 0x1000>; > > > + clocks = <&clk26m>, > > > > Why is mfg using <&clk26m> and not <&topckgen CLK_TOP_MFG_SEL>? > > Because James Liao said to me that it is derived from clk26m and not > from mfg_sel. Sascha is right. I had confirmed with our designer that MFG on MT8173 uses clk26m to check state. I also tested MFG domain power on/off with CLK_TOP_MFG_SEL off and it worked correctly. > > I saw another patch set on the list today from James Liao that adds more clocks. > > Perhaps we can move the SCPSYS set on top of that one and include more clocks? > > > > > + <&topckgen CLK_TOP_MM_SEL>; The clocks used by scpsys driver are subsystem bus clocks that need to be on before power on these domains. On MT8173, subsystem bus clocks are come from topckgen. My patch set yesterday add subsystem clocks, which are not needed by power domain on/off. So I think these 2 patch set are independent. Best regards, James
On Fri, May 22, 2015 at 10:41 AM, James Liao <jamesjj.liao@mediatek.com> wrote: > Hi Daniel, > > On Thu, 2015-05-21 at 19:49 +0200, Sascha Hauer wrote: >> On Thu, May 21, 2015 at 10:32:40PM +0800, Daniel Kurtz wrote: >> > > + scpsys: scpsys@10006000 { >> > > + compatible = "mediatek,mt8173-scpsys"; >> > > + #power-domain-cells = <1>; >> > > + reg = <0 0x10006000 0 0x1000>; >> > > + clocks = <&clk26m>, >> > >> > Why is mfg using <&clk26m> and not <&topckgen CLK_TOP_MFG_SEL>? >> >> Because James Liao said to me that it is derived from clk26m and not >> from mfg_sel. > > Sascha is right. I had confirmed with our designer that MFG on MT8173 > uses clk26m to check state. I also tested MFG domain power on/off with > CLK_TOP_MFG_SEL off and it worked correctly. Wait - the designer said to use clk26m, but you tested with CLK_TOP_MFG_SEL. Now I am even more confused. Which is the correct clock to use for the mfg power domains? > >> > I saw another patch set on the list today from James Liao that adds more clocks. >> > Perhaps we can move the SCPSYS set on top of that one and include more clocks? >> > >> > > + <&topckgen CLK_TOP_MM_SEL>; > > The clocks used by scpsys driver are subsystem bus clocks that need to > be on before power on these domains. On MT8173, subsystem bus clocks are > come from topckgen. > > My patch set yesterday add subsystem clocks, which are not needed by > power domain on/off. So I think these 2 patch set are independent. In other versions of the SCPSYS patch set, the scpsys node has additional "subsystem bus clocks". So will we need additional patches later to add back these additional clocks which have been removed from the current version of this pach? In other words, will there be a follow up patch like below, plus another patch to add these clocks to "enum clk_id": @@ -163,9 +163,12 @@ compatible = "mediatek,mt8173-scpsys"; #power-domain-cells = <1>; reg = <0 0x10006000 0 0x1000>; - clocks = <&clk26m>, - <&topckgen CLK_TOP_MM_SEL>; - clock-names = "mfg", "mm"; + clocks = <&topckgen CLK_TOP_MFG_SEL>, + <&topckgen CLK_TOP_MM_SEL>, + <&topckgen CLK_TOP_VDEC_SEL>, + <&topckgen CLK_TOP_VENC_SEL>, + <&topckgen CLK_TOP_VENC_LT_SEL>; + clock-names = "mfg", "mm", "vdec", "venc", "venc_lt"; infracfg = <&infracfg>; }; @@ -57,7 +57,10 @@ enum clk_id { MT8173_CLK_NONE, MT8173_CLK_MM, MT8173_CLK_MFG, - MT8173_CLK_MAX = MT8173_CLK_MFG, + MT8173_CLK_VDEC, + MT8173_CLK_VENC, + MT8173_CLK_VENC_LT, + MT8173_CLK_MAX = MT8173_CLK_VENC_LT, }; If so, is there a reason we cannot just include these clocks in the current version of the SCPSYS driver? Best, -Dan > > > Best regards, > > James > >
Hi Daniel, On Fri, 2015-05-22 at 12:19 +0800, Daniel Kurtz wrote: > On Fri, May 22, 2015 at 10:41 AM, James Liao <jamesjj.liao@mediatek.com> wrote: > > > > Sascha is right. I had confirmed with our designer that MFG on MT8173 > > uses clk26m to check state. I also tested MFG domain power on/off with > > CLK_TOP_MFG_SEL off and it worked correctly. > > Wait - the designer said to use clk26m, but you tested with CLK_TOP_MFG_SEL. > Now I am even more confused. > Which is the correct clock to use for the mfg power domains? I tested MFG domain power on/off with MFG_SEL "OFF". If MFG domain use MFG_SEL as its bus clock, it will be timeout while waiting SRAM ACK. But in my test, MFG power domain can power on/off without MFG_SEL. That means it doesn't need MFG_SEL while power on/off MFG domains. > > > > My patch set yesterday add subsystem clocks, which are not needed by > > power domain on/off. So I think these 2 patch set are independent. > > In other versions of the SCPSYS patch set, the scpsys node has > additional "subsystem bus clocks". That's my fault because I provided wrong information to Sascha. So in his early version of mtk-scpsys driver, the clocks setting of power domains are incorrect. The correct clock setting for each power domain should be: POWER_DOMAIN_MFG: clk26m (no need to enable topckgen clocks) POWER_DOMAIN_DISP: mm_sel POWER_DOMAIN_VDEC: mm_sel POWER_DOMAIN_VENC: mm_sel POWER_DOMAIN_VENC_LT: mm_sel POWER_DOMAIN_ISP: mm_sel > So will we need additional patches later to add back these additional > clocks which have been removed from the current version of this pach? No. Clocks provided by topckgen are enough for scpsys driver. Please see my comments below. > In other words, will there be a follow up patch like below, plus > another patch to add these clocks to "enum clk_id": > > @@ -163,9 +163,12 @@ > compatible = "mediatek,mt8173-scpsys"; > #power-domain-cells = <1>; > reg = <0 0x10006000 0 0x1000>; > - clocks = <&clk26m>, > - <&topckgen CLK_TOP_MM_SEL>; > - clock-names = "mfg", "mm"; > + clocks = <&topckgen CLK_TOP_MFG_SEL>, > + <&topckgen CLK_TOP_MM_SEL>, > + <&topckgen CLK_TOP_VDEC_SEL>, > + <&topckgen CLK_TOP_VENC_SEL>, > + <&topckgen CLK_TOP_VENC_LT_SEL>; > + clock-names = "mfg", "mm", "vdec", "venc", "venc_lt"; > infracfg = <&infracfg>; > }; These clocks come from topckgen. In our term, they are "top clocks". Subsystem clocks which mentioned in my patch set are provided by subsystems such as mmsys, vdecsys, vencsys and so on. Best regards, James
Hi James, On Fri, May 22, 2015 at 1:40 PM, James Liao <jamesjj.liao@mediatek.com> wrote: > Hi Daniel, > > On Fri, 2015-05-22 at 12:19 +0800, Daniel Kurtz wrote: >> On Fri, May 22, 2015 at 10:41 AM, James Liao <jamesjj.liao@mediatek.com> wrote: >> > >> > Sascha is right. I had confirmed with our designer that MFG on MT8173 >> > uses clk26m to check state. I also tested MFG domain power on/off with >> > CLK_TOP_MFG_SEL off and it worked correctly. >> >> Wait - the designer said to use clk26m, but you tested with CLK_TOP_MFG_SEL. >> Now I am even more confused. >> Which is the correct clock to use for the mfg power domains? > > I tested MFG domain power on/off with MFG_SEL "OFF". If MFG domain use > MFG_SEL as its bus clock, it will be timeout while waiting SRAM ACK. But > in my test, MFG power domain can power on/off without MFG_SEL. That > means it doesn't need MFG_SEL while power on/off MFG domains. > >> > >> > My patch set yesterday add subsystem clocks, which are not needed by >> > power domain on/off. So I think these 2 patch set are independent. >> >> In other versions of the SCPSYS patch set, the scpsys node has >> additional "subsystem bus clocks". > > That's my fault because I provided wrong information to Sascha. So in > his early version of mtk-scpsys driver, the clocks setting of power > domains are incorrect. The correct clock setting for each power domain > should be: > > POWER_DOMAIN_MFG: clk26m (no need to enable topckgen clocks) > POWER_DOMAIN_DISP: mm_sel > POWER_DOMAIN_VDEC: mm_sel > POWER_DOMAIN_VENC: mm_sel > POWER_DOMAIN_VENC_LT: mm_sel > POWER_DOMAIN_ISP: mm_sel > >> So will we need additional patches later to add back these additional >> clocks which have been removed from the current version of this pach? > > No. Clocks provided by topckgen are enough for scpsys driver. Please see > my comments below. > >> In other words, will there be a follow up patch like below, plus >> another patch to add these clocks to "enum clk_id": >> >> @@ -163,9 +163,12 @@ >> compatible = "mediatek,mt8173-scpsys"; >> #power-domain-cells = <1>; >> reg = <0 0x10006000 0 0x1000>; >> - clocks = <&clk26m>, >> - <&topckgen CLK_TOP_MM_SEL>; >> - clock-names = "mfg", "mm"; >> + clocks = <&topckgen CLK_TOP_MFG_SEL>, >> + <&topckgen CLK_TOP_MM_SEL>, >> + <&topckgen CLK_TOP_VDEC_SEL>, >> + <&topckgen CLK_TOP_VENC_SEL>, >> + <&topckgen CLK_TOP_VENC_LT_SEL>; >> + clock-names = "mfg", "mm", "vdec", "venc", "venc_lt"; >> infracfg = <&infracfg>; >> }; > > These clocks come from topckgen. In our term, they are "top clocks". > Subsystem clocks which mentioned in my patch set are provided by > subsystems such as mmsys, vdecsys, vencsys and so on. This is all great news! Thanks for the detailed update. Best Regards, -Daniel > > > Best regards, > > James >
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index 924fdb6..12430f0 100644 --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi @@ -125,6 +125,16 @@ <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>; }; + scpsys: scpsys@10006000 { + compatible = "mediatek,mt8173-scpsys"; + #power-domain-cells = <1>; + reg = <0 0x10006000 0 0x1000>; + clocks = <&clk26m>, + <&topckgen CLK_TOP_MM_SEL>; + clock-names = "mfg", "mm"; + infracfg = <&infracfg>; + }; + sysirq: intpol-controller@10200620 { compatible = "mediatek,mt8173-sysirq", "mediatek,mt6577-sysirq";
This adds the SCPSYS device node to the MT8173 dtsi file. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+)