| Message ID | 20210503145503.1477-4-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Added Audio and HDMI power domain for Amlogic SoC | expand |
Hi Anand, On Mon, May 3, 2021 at 4:58 PM Anand Moon <linux.amoon@gmail.com> wrote: > > As per the S922X datasheet add hdmi power domain > controller for Meson g12a and g12b SoCs. > > Cc: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > drivers/soc/amlogic/meson-ee-pwrc.c | 5 +++++ > include/dt-bindings/power/meson-g12a-power.h | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c > index 2e07ddf2d6a6..ec402c4ab931 100644 > --- a/drivers/soc/amlogic/meson-ee-pwrc.c > +++ b/drivers/soc/amlogic/meson-ee-pwrc.c > @@ -154,6 +154,10 @@ static struct meson_ee_pwrc_mem_domain gxbb_pwrc_mem_vpu[] = { > VPU_HHI_MEMPD(HHI_MEM_PD_REG0), > }; > > +static struct meson_ee_pwrc_mem_domain meson_pwrc_mem_hdmi[] = { > + { HHI_MEM_PD_REG0, GENMASK(15, 8) }, > +}; > + the VPU power domain already includes: VPU_HHI_MEMPD(HHI_MEM_PD_REG0), whereas VPU_HHI_MEMPD is bits[15:8] Having two power domains which are managing the same registers sounds like it'll be causing some trouble So for now this is (as I am not even sure what the goal here is): NACKed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Best regards, Martin
Hi Martin, On Mon, 3 May 2021 at 20:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Mon, May 3, 2021 at 4:58 PM Anand Moon <linux.amoon@gmail.com> wrote: > > > > As per the S922X datasheet add hdmi power domain > > controller for Meson g12a and g12b SoCs. > > > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > drivers/soc/amlogic/meson-ee-pwrc.c | 5 +++++ > > include/dt-bindings/power/meson-g12a-power.h | 1 + > > 2 files changed, 6 insertions(+) > > > > diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c > > index 2e07ddf2d6a6..ec402c4ab931 100644 > > --- a/drivers/soc/amlogic/meson-ee-pwrc.c > > +++ b/drivers/soc/amlogic/meson-ee-pwrc.c > > @@ -154,6 +154,10 @@ static struct meson_ee_pwrc_mem_domain gxbb_pwrc_mem_vpu[] = { > > VPU_HHI_MEMPD(HHI_MEM_PD_REG0), > > }; > > > > +static struct meson_ee_pwrc_mem_domain meson_pwrc_mem_hdmi[] = { > > + { HHI_MEM_PD_REG0, GENMASK(15, 8) }, > > +}; > > + > the VPU power domain already includes: > VPU_HHI_MEMPD(HHI_MEM_PD_REG0), > whereas VPU_HHI_MEMPD is bits[15:8] > > Having two power domains which are managing the same registers sounds > like it'll be causing some trouble > So for now this is (as I am not even sure what the goal here is): > NACKed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Ok, thanks. On the line of Ethernet PD, I tried to add this accordingly. whenever I try something new it fails. Please ignore this series. > Best regards, > Martin Thanks -Anand
Hi Anand, On Mon, May 3, 2021 at 5:29 PM Anand Moon <linux.amoon@gmail.com> wrote: [...] > > > +static struct meson_ee_pwrc_mem_domain meson_pwrc_mem_hdmi[] = { > > > + { HHI_MEM_PD_REG0, GENMASK(15, 8) }, > > > +}; > > > + > > the VPU power domain already includes: > > VPU_HHI_MEMPD(HHI_MEM_PD_REG0), > > whereas VPU_HHI_MEMPD is bits[15:8] > > > > Having two power domains which are managing the same registers sounds > > like it'll be causing some trouble > > So for now this is (as I am not even sure what the goal here is): > > NACKed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > Ok, thanks. On the line of Ethernet PD, I tried to add this accordingly. From what I understand the VPU power domain is special because the display pipeline consists of multiple components (HDMI, VPU, ...) that's why the handling currently is special > whenever I try something new it fails. Please ignore this series. if the VPU and HDMI power domains were separate (from hardware perspective, not from driver perspective) then your change is a good step forward. in that case VPU_HHI_MEMPD would need to be removed from wherever it's currently used -> that means we need to also decide if we want to break compatibility with older (before this series) .dtbs Best regards, Martin
hi Martin On Mon, 3 May 2021 at 21:05, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Mon, May 3, 2021 at 5:29 PM Anand Moon <linux.amoon@gmail.com> wrote: > [...] > > > > +static struct meson_ee_pwrc_mem_domain meson_pwrc_mem_hdmi[] = { > > > > + { HHI_MEM_PD_REG0, GENMASK(15, 8) }, > > > > +}; > > > > + > > > the VPU power domain already includes: > > > VPU_HHI_MEMPD(HHI_MEM_PD_REG0), > > > whereas VPU_HHI_MEMPD is bits[15:8] > > > > > > Having two power domains which are managing the same registers sounds > > > like it'll be causing some trouble > > > So for now this is (as I am not even sure what the goal here is): > > > NACKed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > > > > Ok, thanks. On the line of Ethernet PD, I tried to add this accordingly. > From what I understand the VPU power domain is special because the > display pipeline consists of multiple components (HDMI, VPU, ...) > that's why the handling currently is special > > > whenever I try something new it fails. Please ignore this series. > if the VPU and HDMI power domains were separate (from hardware > perspective, not from driver perspective) then your change is a good > step forward. > in that case VPU_HHI_MEMPD would need to be removed from wherever it's > currently used -> that means we need to also decide if we want to > break compatibility with older (before this series) .dtbs > > As per the datasheet S922X Datasheet, HDMI and VPU are different reg controller and they are independent of each other. *HHI_MEM_PD_REG0 0x40* 17~16 R/W 0x3 DDR memory PD *15~8 R/W 0xFF HDMI memory PD* 7~6 R/W 0x3 Reserved 5~4 R/W 0x3 Audio mem PD 3~2 R/W 0x3 Ethernet memory PD 1~0 R/W 0x3 resved Note: HDMI and AUDIO and Ethernet are also independent of each other. *HHI_VPU_MEM_PD_REG0 0x41 * 31~30 R/W 0x3 sharp 29~28 R/W 0x3 Deinterlacer – di_post: 11 = power down. 00 = normal operation 27~26 R/W 0x3 Deinterlacer – di_pre 25~24 R/W 0x3 Vi_di_scaler 23~22 R/W 0x3 afbc_dec1 21~20 R/W 0x3 Srscl super scaler 19~18 R/W 0x3 Vdin1 memory 17~16 R/W 0x3 Vdin0 memory 15~14 R/W 0x3 Osd_scaler memory 13~12 R/W 0x3 Scaler memory 11~10 R/W 0x3 Vpp output fifo 9~8 R/W 0x3 Color management module 7~6 R/W 0x3 Vd2 memory 5~4 R/W 0x3 Vd1 memory 3~2 R/W 0x3 Osd2 memory 1~0 R/W 0x3 Osd1 memory Below is the output on Odroid N2. [alarm@archl-on2 ~]$ sudo cat /sys/kernel/debug/pm_genpd/pm_genpd_summary domain status children performance /device runtime status ---------------------------------------------------------------------------------------------- HDMI on 0 /devices/platform/soc/ff600000.bus/ff600000.hdmi-tx unsupported 0 AUDIO on 0 /devices/platform/sound unsupported 0 ETH on 0 /devices/platform/soc/ff3f0000.ethernet active 0 VPU on 0 /devices/platform/soc/ff900000.vpu unsupported 0 HDMI power domain is ON. Audio is wrongly mapped. > Best regards, > Martin -Anand
Hi Anand, On Mon, May 3, 2021 at 6:37 PM Anand Moon <linux.amoon@gmail.com> wrote: [...] > > > whenever I try something new it fails. Please ignore this series. > > if the VPU and HDMI power domains were separate (from hardware > > perspective, not from driver perspective) then your change is a good > > step forward. > > in that case VPU_HHI_MEMPD would need to be removed from wherever it's > > currently used -> that means we need to also decide if we want to > > break compatibility with older (before this series) .dtbs > > > > > > As per the datasheet S922X Datasheet, HDMI and VPU are different > reg controller and they are independent of each other. [...] > Note: HDMI and AUDIO and Ethernet are also independent of each other. let me say it this way: I've seen cases where the information from the datasheet is not correct Also to me it doesn't explicitly state that the bits are independent of each other (at the same time it also doesn't state that they belong together). In the same datasheet you also find the HHI_HDMI_PLL_CNTL0 register hdmi_dpll_M, hdmi_dpll_N and hdmi_dpll_od are listed in there. The PLL output depends on hdmi_dpll_M and hdmi_dpll_N while hdmi_dpll_od is taking the output of the two and dividing it. This relation is nowhere described in the datasheet either so you "just have to know it". Unfortunately I don't know of any good way to check the relationship of the power domain registers other than someone from Amlogic explaining to us how it works internally. [...] > Below is the output on Odroid N2. > > [alarm@archl-on2 ~]$ sudo cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > domain status children > performance > /device runtime status > ---------------------------------------------------------------------------------------------- > HDMI on > 0 > /devices/platform/soc/ff600000.bus/ff600000.hdmi-tx unsupported > 0 > AUDIO on > 0 > /devices/platform/sound unsupported > 0 > ETH on > 0 > /devices/platform/soc/ff3f0000.ethernet active > 0 > VPU on > 0 > /devices/platform/soc/ff900000.vpu unsupported > 0 This describes what Linux sees (based on how you configured the device-tree). The output confirms what you are expecting to see (I think), but based on that we can't tell what's right or wrong in terms of the actual hardware. To make another example: I could edit meson-g12b-odroid-n2.dtsi and change the vin-supply of "VDDAO_3V3" to &usb_pwr_en Then /sys/kernel/debug/regulator/regulator_summary would show that VDDAO_3V3 is taking the voltage from USB_PWR_EN as input. But from a hardware (schematics) perspective this is not correct. Since the schematics describe the relation (input, output) between the regulators we know how they are connected to each other. If this relation was not described in the schematics then we'd be in the same situation as with the power domains. Best regards, Martin
Hi Martin On Mon, 3 May 2021 at 23:22, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Mon, May 3, 2021 at 6:37 PM Anand Moon <linux.amoon@gmail.com> wrote: > [...] > > > > whenever I try something new it fails. Please ignore this series. > > > if the VPU and HDMI power domains were separate (from hardware > > > perspective, not from driver perspective) then your change is a good > > > step forward. > > > in that case VPU_HHI_MEMPD would need to be removed from wherever it's > > > currently used -> that means we need to also decide if we want to > > > break compatibility with older (before this series) .dtbs > > > > > > > > > > As per the datasheet S922X Datasheet, HDMI and VPU are different > > reg controller and they are independent of each other. > [...] > > Note: HDMI and AUDIO and Ethernet are also independent of each other. > let me say it this way: I've seen cases where the information from the > datasheet is not correct > > Also to me it doesn't explicitly state that the bits are independent > of each other (at the same time it also doesn't state that they belong > together). > > In the same datasheet you also find the HHI_HDMI_PLL_CNTL0 register > hdmi_dpll_M, hdmi_dpll_N and hdmi_dpll_od are listed in there. > The PLL output depends on hdmi_dpll_M and hdmi_dpll_N while > hdmi_dpll_od is taking the output of the two and dividing it. > This relation is nowhere described in the datasheet either so you > "just have to know it". > > Unfortunately I don't know of any good way to check the relationship > of the power domain registers other than someone from Amlogic > explaining to us how it works internally. > I should have sought more details on this feature before posting something. Thanks for the detailed explanation of this feature, So in order for this feature to work the final state should be *active* for each power domain. > [...] > > Below is the output on Odroid N2. > > > > [alarm@archl-on2 ~]$ sudo cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > > domain status children > > performance > > /device runtime status > > ---------------------------------------------------------------------------------------------- > > HDMI on > > 0 > > /devices/platform/soc/ff600000.bus/ff600000.hdmi-tx unsupported > > 0 > > AUDIO on > > 0 > > /devices/platform/sound unsupported > > 0 > > ETH on > > 0 > > /devices/platform/soc/ff3f0000.ethernet active > > 0 > > VPU on > > 0 > > /devices/platform/soc/ff900000.vpu unsupported > > 0 > This describes what Linux sees (based on how you configured the device-tree). > The output confirms what you are expecting to see (I think), but based > on that we can't tell what's right or wrong in terms of the actual > hardware. > > To make another example: I could edit meson-g12b-odroid-n2.dtsi and > change the vin-supply of "VDDAO_3V3" to &usb_pwr_en > Then /sys/kernel/debug/regulator/regulator_summary would show that > VDDAO_3V3 is taking the voltage from USB_PWR_EN as input. > But from a hardware (schematics) perspective this is not correct. > Since the schematics describe the relation (input, output) between the > regulators we know how they are connected to each other. > If this relation was not described in the schematics then we'd be in > the same situation as with the power do > On Amlogic SBC, I think VDDAO_3V3 and VDDIO_AO1V8 share the power with SD card and USB. so can we add these as optional regulators to enable/disable within the usb driver if needed? I am facing some issues to fix the power on Odroid C1+ and C2 board as well. > Best regards, > Martin Thanks -Anand
diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c index 2e07ddf2d6a6..ec402c4ab931 100644 --- a/drivers/soc/amlogic/meson-ee-pwrc.c +++ b/drivers/soc/amlogic/meson-ee-pwrc.c @@ -154,6 +154,10 @@ static struct meson_ee_pwrc_mem_domain gxbb_pwrc_mem_vpu[] = { VPU_HHI_MEMPD(HHI_MEM_PD_REG0), }; +static struct meson_ee_pwrc_mem_domain meson_pwrc_mem_hdmi[] = { + { HHI_MEM_PD_REG0, GENMASK(15, 8) }, +}; + static struct meson_ee_pwrc_mem_domain meson_pwrc_mem_audio[] = { { HHI_MEM_PD_REG0, GENMASK(5, 4) }, }; @@ -256,6 +260,7 @@ static struct meson_ee_pwrc_domain_desc axg_pwrc_domains[] = { static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = { [PWRC_G12A_VPU_ID] = VPU_PD("VPU", &gx_pwrc_vpu, g12a_pwrc_mem_vpu, pwrc_ee_get_power, 11, 2), + [PWRC_G12A_HDMI_ID] = MEM_PD("HDMI", meson_pwrc_mem_hdmi), [PWRC_G12A_AUDIO_ID] = MEM_PD("AUDIO", meson_pwrc_mem_audio), [PWRC_G12A_ETH_ID] = MEM_PD("ETH", meson_pwrc_mem_eth), }; diff --git a/include/dt-bindings/power/meson-g12a-power.h b/include/dt-bindings/power/meson-g12a-power.h index 1cf20e4e412e..900924d17798 100644 --- a/include/dt-bindings/power/meson-g12a-power.h +++ b/include/dt-bindings/power/meson-g12a-power.h @@ -10,5 +10,6 @@ #define PWRC_G12A_VPU_ID 0 #define PWRC_G12A_ETH_ID 1 #define PWRC_G12A_AUDIO_ID 2 +#define PWRC_G12A_HDMI_ID 3 #endif
As per the S922X datasheet add hdmi power domain controller for Meson g12a and g12b SoCs. Cc: Neil Armstrong <narmstrong@baylibre.com> Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- drivers/soc/amlogic/meson-ee-pwrc.c | 5 +++++ include/dt-bindings/power/meson-g12a-power.h | 1 + 2 files changed, 6 insertions(+)