On 26/05/2023 22:28, Konrad Dybcio wrote:
On 26.05.2023 23:17, Bryan O'Donoghue wrote:
On 26/05/2023 21:57, Konrad Dybcio wrote:
This code contains a whole bunch of hacky counting logic that should have
been substituted with _byname, but now we're stuck with indices to keep
compatibility with old DTs :/
If CAMSS_GDSC (talking about pre-TITAN hw) was a parent of all the other
CAMSS-related GDSCs, we could make it their parent in the clock driver
and call it a day.
I mean, it wouldn't make much sense from a hw design POV if that weren't the case..
Hmm looks like its already there.
static struct gdsc vfe0_gdsc = {
.gdscr = 0x3664,
.cxcs = (unsigned int []){ 0x36a8 },
.cxc_count = 1,
.pd = {
.name = "vfe0",
},
.parent = &camss_gdsc.pd,
.pwrsts = PWRSTS_OFF_ON,
};
static struct gdsc vfe1_gdsc = {
.gdscr = 0x3674,
.cxcs = (unsigned int []){ 0x36ac },
.cxc_count = 1,
.pd = {
.name = "vfe1",
},
.parent = &camss_gdsc.pd,
.pwrsts = PWRSTS_OFF_ON,
};
I feel this is probably a problem in the description of dependencies for the CSIPHY in the dts for the 8996..
I.e. the CSIPHY requires some clocks and power-rails to be switched on ah..
static const struct resources csiphy_res_8x96[] = {
/* CSIPHY0 */
{
.regulators = {},
.clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer" },
should probably look something like
static const struct resources csiphy_res_8x96[] = {
/* CSIPHY0 */
{
.regulators = {},
.clock = { "top_ahb", "ispif_ahb", "ahb", "csiphy0_timer", "vfe0"},
But basically yeah, we haven't modeled the dependency to the CAMSS_GDSC via the VFEx
I have little idea how CAMSS is tied together, but the VFEn clocks
are assigned in vfe_res_8x96.clock and ispif_res_8x96.clock_for_reset.
FWIW the ancient msm-3.18 doesn't reference the VFE clocks in CSIPHY.
Anyway, looks like the issue here is that we're not toggling the
GDSC early enough in cases where something that's not VFE needs it.
Hmm wait - why haven't we included the CAMSS_GDSC in the dtsi for the 8996 ?
Since both VFE GDSCs are children of CAMSS_GDSC and (as mentioned in the
commit message) the power sequencing used to be different, it just seems
to me like we've been piggybacking on lucky ordering since the introduction
of 8996 support.
For comparison, 8916 doesn't define it because it doesn't have it and newer
SoCs use TITAN.
SDM630 doesn't define it, but nobody touched it since like 2021 (except
Dmitry's fixups when he got his hands on the inforce baord) so it's
probably broken as well..
Konrad
---
bod
Hmm, so what I suggested is what Yassine has i.e. adds <&mmcc
CAMSS_GDSC>; to the top-level camss node.
Without testing, this _looks_ right to me. I just think, like Conor
flagged we don't need to add a dependency on the power-domain names.
I don't quite know whats the reference for downstream you are looking at
but, just generally it is possible to waggle the CSIPHY, VFE, IFE - my
guess would be that CAMX @ the time of 8996 always had the CAMSS_GDSC or
the VFEx - and hence the VFE -> CAMSS_GDSC on by the time the CSIPHY cod
ran.
Anyway my comments here still stand.
- I don't think we need to nor should be counting power-domain names
- I do think we should be adding CAMSS_GDSC to the 8996 top-level CAMSS node
Yassine ? Can you take a stab at that ?
---
bod
