| Message ID | 20210708010839.692242-2-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix and enable camcc-sm8250 | expand |
On Wed 07 Jul 20:08 CDT 2021, Bryan O'Donoghue wrote: > The current implementation omits the necessary mmcx supply, which means if > you are running the code for this block and a prior clock driver, like say > the videocc hasn't run, then a reset will be generated the first time we > touch these registers. > > Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250") > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> camcc isn't enabled in the upstream dts yet and I expect that we're going to conclude on defining these GDSCs as subdomains of the cc's power-domain in time for v5.15. I don't mind if Stephen picks this to make sure the driver is functional, but I will hold off on the dts change. Regards, Bjorn > --- > drivers/clk/qcom/camcc-sm8250.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/clk/qcom/camcc-sm8250.c b/drivers/clk/qcom/camcc-sm8250.c > index 439eaafdcc86..c51112546bfc 100644 > --- a/drivers/clk/qcom/camcc-sm8250.c > +++ b/drivers/clk/qcom/camcc-sm8250.c > @@ -2212,6 +2212,7 @@ static struct gdsc bps_gdsc = { > }, > .flags = HW_CTRL | POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > + .supply = "mmcx", > }; > > static struct gdsc ipe_0_gdsc = { > @@ -2221,6 +2222,7 @@ static struct gdsc ipe_0_gdsc = { > }, > .flags = HW_CTRL | POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > + .supply = "mmcx", > }; > > static struct gdsc sbi_gdsc = { > @@ -2230,6 +2232,7 @@ static struct gdsc sbi_gdsc = { > }, > .flags = HW_CTRL | POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > + .supply = "mmcx", > }; > > static struct gdsc ife_0_gdsc = { > @@ -2239,6 +2242,7 @@ static struct gdsc ife_0_gdsc = { > }, > .flags = POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > + .supply = "mmcx", > }; > > static struct gdsc ife_1_gdsc = { > @@ -2248,6 +2252,7 @@ static struct gdsc ife_1_gdsc = { > }, > .flags = POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > + .supply = "mmcx", > }; > > static struct gdsc titan_top_gdsc = { > @@ -2257,6 +2262,7 @@ static struct gdsc titan_top_gdsc = { > }, > .flags = POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > + .supply = "mmcx", > }; > > static struct clk_regmap *cam_cc_sm8250_clocks[] = { > -- > 2.30.1 >
Quoting Bjorn Andersson (2021-07-07 21:03:06) > On Wed 07 Jul 20:08 CDT 2021, Bryan O'Donoghue wrote: > > > The current implementation omits the necessary mmcx supply, which means if > > you are running the code for this block and a prior clock driver, like say > > the videocc hasn't run, then a reset will be generated the first time we > > touch these registers. > > > > Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250") > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > camcc isn't enabled in the upstream dts yet and I expect that we're > going to conclude on defining these GDSCs as subdomains of the cc's > power-domain in time for v5.15. > > I don't mind if Stephen picks this to make sure the driver is > functional, but I will hold off on the dts change. > Seems like this is superseded by the GDSC patches that use subdomains instead of a fake supply?
On 27/07/2021 19:29, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-07-07 21:03:06) >> On Wed 07 Jul 20:08 CDT 2021, Bryan O'Donoghue wrote: >> >>> The current implementation omits the necessary mmcx supply, which means if >>> you are running the code for this block and a prior clock driver, like say >>> the videocc hasn't run, then a reset will be generated the first time we >>> touch these registers. >>> >>> Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250") >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> >> camcc isn't enabled in the upstream dts yet and I expect that we're >> going to conclude on defining these GDSCs as subdomains of the cc's >> power-domain in time for v5.15. >> >> I don't mind if Stephen picks this to make sure the driver is >> functional, but I will hold off on the dts change. >> > > Seems like this is superseded by the GDSC patches that use subdomains > instead of a fake supply? > yep
diff --git a/drivers/clk/qcom/camcc-sm8250.c b/drivers/clk/qcom/camcc-sm8250.c index 439eaafdcc86..c51112546bfc 100644 --- a/drivers/clk/qcom/camcc-sm8250.c +++ b/drivers/clk/qcom/camcc-sm8250.c @@ -2212,6 +2212,7 @@ static struct gdsc bps_gdsc = { }, .flags = HW_CTRL | POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, + .supply = "mmcx", }; static struct gdsc ipe_0_gdsc = { @@ -2221,6 +2222,7 @@ static struct gdsc ipe_0_gdsc = { }, .flags = HW_CTRL | POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, + .supply = "mmcx", }; static struct gdsc sbi_gdsc = { @@ -2230,6 +2232,7 @@ static struct gdsc sbi_gdsc = { }, .flags = HW_CTRL | POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, + .supply = "mmcx", }; static struct gdsc ife_0_gdsc = { @@ -2239,6 +2242,7 @@ static struct gdsc ife_0_gdsc = { }, .flags = POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, + .supply = "mmcx", }; static struct gdsc ife_1_gdsc = { @@ -2248,6 +2252,7 @@ static struct gdsc ife_1_gdsc = { }, .flags = POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, + .supply = "mmcx", }; static struct gdsc titan_top_gdsc = { @@ -2257,6 +2262,7 @@ static struct gdsc titan_top_gdsc = { }, .flags = POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, + .supply = "mmcx", }; static struct clk_regmap *cam_cc_sm8250_clocks[] = {
The current implementation omits the necessary mmcx supply, which means if you are running the code for this block and a prior clock driver, like say the videocc hasn't run, then a reset will be generated the first time we touch these registers. Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250") Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/clk/qcom/camcc-sm8250.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.30.1