[PATCH] ARM: dra7xx: hwmod: drop .modulemode from pcie1/2 hwmods

[grygorii.strashko@linaro.org (Grygorii.Strashko@linaro.org)]

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@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@ti.com>
>>>>>>
>>>>>> Signed-off-by: Grygorii Strashko <Grygorii.Strashko@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@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@linaro.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon@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