| Message ID | 20240531090249.10293-3-quic_tdas@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for SA8775P Multimedia clock controllers | expand |
Hi Konrad, Thanks for your review. On 5/31/2024 6:52 PM, Konrad Dybcio wrote: > On 31.05.2024 11:02 AM, Taniya Das wrote: >> Update the GDSC wait_val fields as per the default hardware values as >> otherwise they would lead to GDSC FSM state to be stuck and causing >> failures to power on/off. Also add the GDSC flags as applicable and >> add support to control PCIE GDSC's using collapse vote registers. >> >> Fixes: 08c51ceb12f7 ("clk: qcom: add the GCC driver for sa8775p") >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> --- >> drivers/clk/qcom/gcc-sa8775p.c | 40 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> >> diff --git a/drivers/clk/qcom/gcc-sa8775p.c b/drivers/clk/qcom/gcc-sa8775p.c >> index 7bb7aa3a7be5..71fa95f59a0a 100644 >> --- a/drivers/clk/qcom/gcc-sa8775p.c >> +++ b/drivers/clk/qcom/gcc-sa8775p.c >> @@ -4203,74 +4203,114 @@ static struct clk_branch gcc_video_axi1_clk = { >> >> static struct gdsc pcie_0_gdsc = { >> .gdscr = 0xa9004, >> + .collapse_ctrl = 0x4b104, >> + .collapse_mask = BIT(0), >> + .en_rest_wait_val = 0x2, >> + .en_few_wait_val = 0x2, >> + .clk_dis_wait_val = 0xf, >> .pd = { >> .name = "pcie_0_gdsc", >> }, >> .pwrsts = PWRSTS_OFF_ON, >> + .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, > > I have some old dt for this platform, and it doesn't mention the downstream > counterpart flag for it (qcom,support-cfg-gdscr), so please double-check > whether you really want to poll gdcsr + 0x4. > Yes, the older code did not have the cfg-gdscr updated in the DT, but as per the latest discussions with design we have concluded to use the polling of GDSCR from the CFG register on all latest designs. We added the support in the latest DT as well to support for 'qcom,support-cfg-gdscr'. > The magic values I trust you have better sources for, the collapse off/masks > look good. > Yes, these are the Power-on Reset (PoR) values which the current GDSC driver overrides in gdsc_init(). The GDSC driver for older designs needed these overrides from SW, but the newer designs did not want to make any such changes. > Konrad
On 6/10/24 10:57, Taniya Das wrote: > Hi Konrad, > > Thanks for your review. > > On 5/31/2024 6:52 PM, Konrad Dybcio wrote: >> On 31.05.2024 11:02 AM, Taniya Das wrote: >>> Update the GDSC wait_val fields as per the default hardware values as >>> otherwise they would lead to GDSC FSM state to be stuck and causing >>> failures to power on/off. Also add the GDSC flags as applicable and >>> add support to control PCIE GDSC's using collapse vote registers. >>> >>> Fixes: 08c51ceb12f7 ("clk: qcom: add the GCC driver for sa8775p") >>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >>> --- >>> drivers/clk/qcom/gcc-sa8775p.c | 40 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 40 insertions(+) >>> >>> diff --git a/drivers/clk/qcom/gcc-sa8775p.c b/drivers/clk/qcom/gcc-sa8775p.c >>> index 7bb7aa3a7be5..71fa95f59a0a 100644 >>> --- a/drivers/clk/qcom/gcc-sa8775p.c >>> +++ b/drivers/clk/qcom/gcc-sa8775p.c >>> @@ -4203,74 +4203,114 @@ static struct clk_branch gcc_video_axi1_clk = { >>> static struct gdsc pcie_0_gdsc = { >>> .gdscr = 0xa9004, >>> + .collapse_ctrl = 0x4b104, >>> + .collapse_mask = BIT(0), >>> + .en_rest_wait_val = 0x2, >>> + .en_few_wait_val = 0x2, >>> + .clk_dis_wait_val = 0xf, >>> .pd = { >>> .name = "pcie_0_gdsc", >>> }, >>> .pwrsts = PWRSTS_OFF_ON, >>> + .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, >> >> I have some old dt for this platform, and it doesn't mention the downstream >> counterpart flag for it (qcom,support-cfg-gdscr), so please double-check >> whether you really want to poll gdcsr + 0x4. >> > > Yes, the older code did not have the cfg-gdscr updated in the DT, but as per the latest discussions with design we have concluded to use the polling of GDSCR from the CFG register on all latest designs. We added the support in the latest DT as well to support for 'qcom,support-cfg-gdscr'. > >> The magic values I trust you have better sources for, the collapse off/masks >> look good. >> > > Yes, these are the Power-on Reset (PoR) values which the current GDSC driver overrides in gdsc_init(). The GDSC driver for older designs needed these overrides from SW, but the newer designs did not want to make any such changes. (something may be wrong with your email client, I never got this mail and only noticed it on the mailling list :/) That's.. not good.. We should not be randomly overriding these configs. Do we have a timeline / last known chip where the "older designs" stopped requiring that explicit setting? Maybe we could turn it into an opt-in flag and set it for such platforms. Konrad
diff --git a/drivers/clk/qcom/gcc-sa8775p.c b/drivers/clk/qcom/gcc-sa8775p.c index 7bb7aa3a7be5..71fa95f59a0a 100644 --- a/drivers/clk/qcom/gcc-sa8775p.c +++ b/drivers/clk/qcom/gcc-sa8775p.c @@ -4203,74 +4203,114 @@ static struct clk_branch gcc_video_axi1_clk = { static struct gdsc pcie_0_gdsc = { .gdscr = 0xa9004, + .collapse_ctrl = 0x4b104, + .collapse_mask = BIT(0), + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "pcie_0_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc pcie_1_gdsc = { .gdscr = 0x77004, + .collapse_ctrl = 0x4b104, + .collapse_mask = BIT(1), + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "pcie_1_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc ufs_card_gdsc = { .gdscr = 0x81004, + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "ufs_card_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc ufs_phy_gdsc = { .gdscr = 0x83004, + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "ufs_phy_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc usb20_prim_gdsc = { .gdscr = 0x1c004, + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "usb20_prim_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc usb30_prim_gdsc = { .gdscr = 0x1b004, + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "usb30_prim_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc usb30_sec_gdsc = { .gdscr = 0x2f004, + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "usb30_sec_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc emac0_gdsc = { .gdscr = 0xb6004, + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "emac0_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct gdsc emac1_gdsc = { .gdscr = 0xb4004, + .en_rest_wait_val = 0x2, + .en_few_wait_val = 0x2, + .clk_dis_wait_val = 0xf, .pd = { .name = "emac1_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = RETAIN_FF_ENABLE | POLL_CFG_GDSCR, }; static struct clk_regmap *gcc_sa8775p_clocks[] = {
Update the GDSC wait_val fields as per the default hardware values as otherwise they would lead to GDSC FSM state to be stuck and causing failures to power on/off. Also add the GDSC flags as applicable and add support to control PCIE GDSC's using collapse vote registers. Fixes: 08c51ceb12f7 ("clk: qcom: add the GCC driver for sa8775p") Signed-off-by: Taniya Das <quic_tdas@quicinc.com> --- drivers/clk/qcom/gcc-sa8775p.c | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)