[PATCH] ARM: dra7xx: hwmod: drop .modulemode from pcie1/2 hwmods
Grygorii.Strashko@linaro.org
grygorii.strashko at linaro.org
Thu Feb 12 00:35:44 PST 2015
On 02/12/2015 02:43 PM, Kishon Vijay Abraham I wrote:
> On Monday 09 February 2015 08:22 PM, Grygorii.Strashko at linaro.org wrote:
>> On 02/09/2015 09:24 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Monday 09 February 2015 03:58 PM, Grygorii.Strashko at linaro.org wrote:
>>>> Hi Kishon,
>>>> On 02/09/2015 04:50 PM, Kishon Vijay Abraham I wrote:
>>>>> On Tuesday 03 February 2015 09:21 PM, grygorii.strashko at linaro.org
>>>>> wrote:
>>>>>> From: Grygorii Strashko <grygorii.strashko at linaro.org>
>>>>>>
>>>>>> Now DRA7xx pcie1/2 hwmods define PRCM configuration as following:
>>>>>> .clkctrl_offs = DRA7XX_CM_PCIE_CLKSTCTRL_OFFSET,
>>>>>> .rstctrl_offs = DRA7XX_RM_L3INIT_RSTCTRL_OFFSET,
>>>>>> .modulemode = MODULEMODE_SWCTRL,
>>>>>> which is completely wrong because DRA7XX_CM_PCIE_CLKSTCTRL_OFFSET
>>>>>> is clockdomain ctrl register and NOT module ctrl register.
>>>>>> And they have diffrent allowed values for bits[0,1]:
>>>>>> CLKTRCTRL MODULEMODE
>>>>>> 0x0: NO_SLEEP 0x0: Module is disabled by SW.
>>>>>> 0x1: SW_SLEEP 0x1: Module is managed automatically by HW
>>>>>> 0x2: SW_WKUP 0x2: Module is explicitly enabled.
>>>>>> 0x3: HW_AUTO 0x3: Reserved
>>>>>>
>>>>>> As result, following message can be seen during suspend:
>>>>>> "omap_hwmod: pcie1: _wait_target_disable failed"
>>>>>>
>>>>>> Fix it by removing .modulemode from pcie1/2 hwmods and, in that
>>>>>> way, prevent clockdomain ctrl register writing from HWMOD core.
>>>>>
>>>>> Looks correct except for one change.
>>>>>
>>>>> Acked-by: Kishon Vijay Abraham I <kishon at ti.com>
>>>>>>
>>>>>> Signed-off-by: Grygorii Strashko <Grygorii.Strashko at linaro.org>
>>>>>> ---
>>>>>>
>>>>>> More over, it looks like pcie1/2 hwmods are fake and have to be
>>>>>> dropped at all.
>>>>>> The real HWMODs are PCIESS1/2.
>>>>>
>>>>> Not sure I get this. You mean "dra7xx_pcie1_hwmod" should be
>>>>> replaced with
>>>>> "dra7xx_pciess1_hwmod"? Or you mean an entire new hwmod is missing?
>>>>>
>>>>> Please note we still have to enable the clock domain and main clock.
>>>>> We've also
>>>>> purposefully omitted sysconfig from hwmod data since pcie reset
>>>>> (RM_PCIESS_RSTCTRL) should be done before accessing the syconfig
>>>>> register and
>>>>> the infrastructure for that is currently not present.
>>>>
>>>> What I'm trying to say is that now PM control data mixed between
>>>> "pcieX" and "pcieX-phy" hwmods.
>>>> After this patch "pcieX" hwmods will actually do nothing (I assume
>>>> that "pciex-phy" will be
>>>> enabled before "pcieX"), and probably can be removed if "pcie_clkdm"
>>>> could be attached to "pcieX-phy" hwmod
>>>> instead.
>>>>
>>>> More over, now, "pcie_clkdm" is connected to "pcieX" hwmod while
>>>> MODULEMODE register is controlled
>>>> by "pciex-phy" hwmod, so when pciess is going to be enabled the
>>>> "l3init_clkdm" will be waken-up by
>>>> hwmode core and not "pcie_clkdm" - as I can remember this is not good
>>>> (we should alway wake-up clockdomain
>>>> and keep it in SWSUP mode while changing MODULEMODE and SYSC
>>>> registers).
>>>>
>>>> static struct omap_hwmod dra7xx_pcie1_hwmod = {
>>>> .name = "pcie1",
>>>> .class = &dra7xx_pcie_hwmod_class,
>>>> .clkdm_name = "pcie_clkdm",
>>>> .main_clk = "l4_root_clk_div",
>>>>
>>>> static struct omap_hwmod dra7xx_pcie1_phy_hwmod = {
>>>> .name = "pcie1-phy",
>>>> .class = &dra7xx_pcie_phy_hwmod_class,
>>>> .clkdm_name = "l3init_clkdm",
>>>> .main_clk = "l4_root_clk_div",
>>>>
>>>> So, in my opinion, some rework may be needed here.
>>>> Am I right?
>>>
>>> you are right. We should have a single hwmod like dra7xx_pciess1_hwmod
>>> whose
>>> clkdm should be "pcie_clkdm" and whose clkctrl_offs should be
>>> DRA7XX_CM_L3INIT_PCIESS1_CLKCTRL_OFFSET (for controlling MODULEMODE).
>>> PCIE PHY
>>> shouldn't have a hwmod entry at all.
>>>
>>> Thanks
>>> Kishon
>>>
>>
>> Could this patch be applied any way? It fixes real issue for me.
>
> A proper fix should look something like below IMO
>
Looks good for me and seems working.
>
> From 1c177d37ac46885a4dc17bacec33071ac23c56da Mon Sep 17 00:00:00 2001
> From: Kishon Vijay Abraham I <kishon at ti.com>
> Date: Thu, 12 Feb 2015 11:55:08 +0530
> Subject: [RFC PATCH] ARM: DRA7: hwmod_data: Fix hwmod data for pcie
>
> Fixed hwmod data for pcie by having the correct module mode offset.
> Previously this module mode offset was part of pcie PHY which was wrong.
> Now this module mode offset was moved to pcie hwmod and removed the
> hwmod data
> for pcie phy. While at that renamed pcie_hwmod to pciess_hwmod in order
> to match with the name given in TRM.
>
> This helps to get rid of the following warning
> "omap_hwmod: pcie1: _wait_target_disable failed"
>
> [Grygorii.Strashko at linaro.org: Found the issue that actually caused
> "omap_hwmod: pcie1: _wait_target_disable failed"]
> Signed-off-by: Grygorii Strashko <Grygorii.Strashko at linaro.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon at ti.com>
> ---
> this patch was created on 3.14 kernel after applying reset framework
> patches
> required for testing PCIe. I can port this to mainline if this patch is
> fine.
>
> arch/arm/boot/dts/dra7.dtsi | 2 -
> arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 107
> +++++++----------------------
> 2 files changed, 26 insertions(+), 83 deletions(-)
>
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index a4b1337..d7a1ff9 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -1364,7 +1364,6 @@
> "wkupclk", "refclk",
> "div-clk", "phy-div";
> #phy-cells = <0>;
> - ti,hwmods = "pcie1-phy";
> };
>
> pcie2_phy: pciephy at 4a095000 {
> @@ -1383,7 +1382,6 @@
> "wkupclk", "refclk",
> "div-clk", "phy-div";
> #phy-cells = <0>;
> - ti,hwmods = "pcie2-phy";
> };
> };
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> index 5ae755c..56f2c58 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> @@ -1968,25 +1968,26 @@ static struct omap_hwmod dra7xx_ocp2scp3_hwmod = {
> *
> */
>
> -static struct omap_hwmod_class dra7xx_pcie_hwmod_class = {
> +static struct omap_hwmod_class dra7xx_pciess_hwmod_class = {
> .name = "pcie",
> };
>
> /* pcie1 */
> -static struct omap_hwmod_rst_info dra7xx_pcie1_resets[] = {
> +static struct omap_hwmod_rst_info dra7xx_pciess1_resets[] = {
> { .name = "pcie", .rst_shift = 0 },
> };
>
> -static struct omap_hwmod dra7xx_pcie1_hwmod = {
> +static struct omap_hwmod dra7xx_pciess1_hwmod = {
> .name = "pcie1",
> - .class = &dra7xx_pcie_hwmod_class,
> + .class = &dra7xx_pciess_hwmod_class,
> .clkdm_name = "pcie_clkdm",
> - .rst_lines = dra7xx_pcie1_resets,
> - .rst_lines_cnt = ARRAY_SIZE(dra7xx_pcie1_resets),
> + .rst_lines = dra7xx_pciess1_resets,
> + .rst_lines_cnt = ARRAY_SIZE(dra7xx_pciess1_resets),
> .main_clk = "l4_root_clk_div",
> .prcm = {
> .omap4 = {
> - .clkctrl_offs = DRA7XX_CM_PCIE_CLKSTCTRL_OFFSET,
> + .clkctrl_offs = DRA7XX_CM_L3INIT_PCIESS1_CLKCTRL_OFFSET,
> + .context_offs = DRA7XX_RM_L3INIT_PCIESS1_CONTEXT_OFFSET,
> .rstctrl_offs = DRA7XX_RM_L3INIT_RSTCTRL_OFFSET,
> .modulemode = MODULEMODE_SWCTRL,
> },
> @@ -1994,60 +1995,22 @@ static struct omap_hwmod dra7xx_pcie1_hwmod = {
> };
>
> /* pcie2 */
> -static struct omap_hwmod_rst_info dra7xx_pcie2_resets[] = {
> +static struct omap_hwmod_rst_info dra7xx_pciess2_resets[] = {
> { .name = "pcie", .rst_shift = 1 },
> };
>
> -static struct omap_hwmod dra7xx_pcie2_hwmod = {
> +static struct omap_hwmod dra7xx_pciess2_hwmod = {
> .name = "pcie2",
> - .class = &dra7xx_pcie_hwmod_class,
> + .class = &dra7xx_pciess_hwmod_class,
> .clkdm_name = "pcie_clkdm",
> - .rst_lines = dra7xx_pcie2_resets,
> - .rst_lines_cnt = ARRAY_SIZE(dra7xx_pcie2_resets),
> - .main_clk = "l4_root_clk_div",
> - .prcm = {
> - .omap4 = {
> - .clkctrl_offs = DRA7XX_CM_PCIE_CLKSTCTRL_OFFSET,
> - .rstctrl_offs = DRA7XX_RM_L3INIT_RSTCTRL_OFFSET,
> - .modulemode = MODULEMODE_SWCTRL,
> - },
> - },
> -};
> -
> -/*
> - * 'PCIE PHY' class
> - *
> - */
> -
> -static struct omap_hwmod_class dra7xx_pcie_phy_hwmod_class = {
> - .name = "pcie-phy",
> -};
> -
> -/* pcie1 phy */
> -static struct omap_hwmod dra7xx_pcie1_phy_hwmod = {
> - .name = "pcie1-phy",
> - .class = &dra7xx_pcie_phy_hwmod_class,
> - .clkdm_name = "l3init_clkdm",
> - .main_clk = "l4_root_clk_div",
> - .prcm = {
> - .omap4 = {
> - .clkctrl_offs = DRA7XX_CM_L3INIT_PCIESS1_CLKCTRL_OFFSET,
> - .context_offs = DRA7XX_RM_L3INIT_PCIESS1_CONTEXT_OFFSET,
> - .modulemode = MODULEMODE_SWCTRL,
> - },
> - },
> -};
> -
> -/* pcie2 phy */
> -static struct omap_hwmod dra7xx_pcie2_phy_hwmod = {
> - .name = "pcie2-phy",
> - .class = &dra7xx_pcie_phy_hwmod_class,
> - .clkdm_name = "l3init_clkdm",
> + .rst_lines = dra7xx_pciess2_resets,
> + .rst_lines_cnt = ARRAY_SIZE(dra7xx_pciess2_resets),
> .main_clk = "l4_root_clk_div",
> .prcm = {
> .omap4 = {
> .clkctrl_offs = DRA7XX_CM_L3INIT_PCIESS2_CLKCTRL_OFFSET,
> .context_offs = DRA7XX_RM_L3INIT_PCIESS2_CONTEXT_OFFSET,
> + .rstctrl_offs = DRA7XX_RM_L3INIT_RSTCTRL_OFFSET,
> .modulemode = MODULEMODE_SWCTRL,
> },
> },
> @@ -3621,49 +3584,33 @@ static struct omap_hwmod_ocp_if
> dra7xx_l4_cfg__ocp2scp3 = {
> };
>
> /* l3_main_1 -> pcie1 */
> -static struct omap_hwmod_ocp_if dra7xx_l3_main_1__pcie1 = {
> +static struct omap_hwmod_ocp_if dra7xx_l3_main_1__pciess1 = {
> .master = &dra7xx_l3_main_1_hwmod,
> - .slave = &dra7xx_pcie1_hwmod,
> + .slave = &dra7xx_pciess1_hwmod,
> .clk = "l3_iclk_div",
> .user = OCP_USER_MPU | OCP_USER_SDMA,
> };
>
> /* l4_cfg -> pcie1 */
> -static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie1 = {
> +static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pciess1 = {
> .master = &dra7xx_l4_cfg_hwmod,
> - .slave = &dra7xx_pcie1_hwmod,
> + .slave = &dra7xx_pciess1_hwmod,
> .clk = "l4_root_clk_div",
> .user = OCP_USER_MPU | OCP_USER_SDMA,
> };
>
> /* l3_main_1 -> pcie2 */
> -static struct omap_hwmod_ocp_if dra7xx_l3_main_1__pcie2 = {
> +static struct omap_hwmod_ocp_if dra7xx_l3_main_1__pciess2 = {
> .master = &dra7xx_l3_main_1_hwmod,
> - .slave = &dra7xx_pcie2_hwmod,
> + .slave = &dra7xx_pciess2_hwmod,
> .clk = "l3_iclk_div",
> .user = OCP_USER_MPU | OCP_USER_SDMA,
> };
>
> /* l4_cfg -> pcie2 */
> -static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie2 = {
> - .master = &dra7xx_l4_cfg_hwmod,
> - .slave = &dra7xx_pcie2_hwmod,
> - .clk = "l4_root_clk_div",
> - .user = OCP_USER_MPU | OCP_USER_SDMA,
> -};
> -
> -/* l4_cfg -> pcie1 phy */
> -static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie1_phy = {
> - .master = &dra7xx_l4_cfg_hwmod,
> - .slave = &dra7xx_pcie1_phy_hwmod,
> - .clk = "l4_root_clk_div",
> - .user = OCP_USER_MPU | OCP_USER_SDMA,
> -};
> -
> -/* l4_cfg -> pcie2 phy */
> -static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie2_phy = {
> +static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pciess2 = {
> .master = &dra7xx_l4_cfg_hwmod,
> - .slave = &dra7xx_pcie2_phy_hwmod,
> + .slave = &dra7xx_pciess2_hwmod,
> .clk = "l4_root_clk_div",
> .user = OCP_USER_MPU | OCP_USER_SDMA,
> };
> @@ -4153,12 +4100,10 @@ static struct omap_hwmod_ocp_if
> *dra7xx_hwmod_ocp_ifs[] __initdata = {
> &dra7xx_l4_cfg__mpu,
> &dra7xx_l4_cfg__ocp2scp1,
> &dra7xx_l4_cfg__ocp2scp3,
> - &dra7xx_l3_main_1__pcie1,
> - &dra7xx_l4_cfg__pcie1,
> - &dra7xx_l3_main_1__pcie2,
> - &dra7xx_l4_cfg__pcie2,
> - &dra7xx_l4_cfg__pcie1_phy,
> - &dra7xx_l4_cfg__pcie2_phy,
> + &dra7xx_l3_main_1__pciess1,
> + &dra7xx_l4_cfg__pciess1,
> + &dra7xx_l3_main_1__pciess2,
> + &dra7xx_l4_cfg__pciess2,
> &dra7xx_l4_cfg__pruss1, /* AM57xx only */
> &dra7xx_l4_cfg__pruss2, /* AM57xx only */
> &dra7xx_l3_main_1__qspi,
--
regards,
-grygorii
More information about the linux-arm-kernel
mailing list