| Message ID | 20220812075603.59375-7-samuel@sholland.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | phy: allwinner: phy-sun6i-mipi-dphy: Add the A100 DPHY | expand |
Hi Samuel, On Fri 12 Aug 22, 02:56, Samuel Holland wrote: > The A100 variant of the DPHY requires configuring the analog registers > before setting the global enable bit. Since this order also works on the > other variants, always use it, to minimize the differences between them. Did you get a chance to actually test this with either DSI/CSI-2 hardware? I vaguely remember that the order mattered. Do you have an idea of what the Allwinner BSP does too? Otherwise I could give it a try, at least with my MIPI CSI-2 setup that uses the driver. Cheers, Pau > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c > index 625c6e1e9990..9698d68d0db7 100644 > --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c > +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c > @@ -183,10 +183,6 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) > SUN6I_DPHY_TX_TIME4_HS_TX_ANA0(3) | > SUN6I_DPHY_TX_TIME4_HS_TX_ANA1(3)); > > - regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, > - SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | > - SUN6I_DPHY_GCTL_EN); > - > regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG, > SUN6I_DPHY_ANA0_REG_PWS | > SUN6I_DPHY_ANA0_REG_DMPC | > @@ -244,6 +240,10 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) > SUN6I_DPHY_ANA2_EN_P2S_CPU_MASK, > SUN6I_DPHY_ANA2_EN_P2S_CPU(lanes_mask)); > > + regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, > + SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | > + SUN6I_DPHY_GCTL_EN); > + > return 0; > } > > -- > 2.35.1 >
Hi Paul, On 8/12/22 7:03 AM, Paul Kocialkowski wrote: > On Fri 12 Aug 22, 02:56, Samuel Holland wrote: >> The A100 variant of the DPHY requires configuring the analog registers >> before setting the global enable bit. Since this order also works on the >> other variants, always use it, to minimize the differences between them. > > Did you get a chance to actually test this with either DSI/CSI-2 hardware? I have tested DSI output with the Clockwork DevTerm (D1 SoC) and Pine64 PinePhone (A64 SoC). I do not have any MIPI CSI hardware to test with. > I vaguely remember that the order mattered. Do you have an idea of what the > Allwinner BSP does too? The Allwinner BSP makes the same change as this commit in its "lowlevel_v2x" copy of the code, which is used for R40 and T7 (original DPHY) and A100 and D1 (updated DPHY). It does not make the change in "lowlevel_sun50iw1" (A64 SoC, original DPHY), but I tested A64 with this change, and it works fine. > Otherwise I could give it a try, at least with my MIPI CSI-2 setup > that uses the driver. This commit only changes sun6i_dphy_tx_power_on(). The code for RX is unchanged -- in fact, it already sets SUN6I_DPHY_GCTL_REG last. Regards, Samuel >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> >> drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c >> index 625c6e1e9990..9698d68d0db7 100644 >> --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c >> +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c >> @@ -183,10 +183,6 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) >> SUN6I_DPHY_TX_TIME4_HS_TX_ANA0(3) | >> SUN6I_DPHY_TX_TIME4_HS_TX_ANA1(3)); >> >> - regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, >> - SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | >> - SUN6I_DPHY_GCTL_EN); >> - >> regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG, >> SUN6I_DPHY_ANA0_REG_PWS | >> SUN6I_DPHY_ANA0_REG_DMPC | >> @@ -244,6 +240,10 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) >> SUN6I_DPHY_ANA2_EN_P2S_CPU_MASK, >> SUN6I_DPHY_ANA2_EN_P2S_CPU(lanes_mask)); >> >> + regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, >> + SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | >> + SUN6I_DPHY_GCTL_EN); >> + >> return 0; >> } >> >> -- >> 2.35.1 >> >
Hi Samuel, On Fri 12 Aug 22, 17:31, Samuel Holland wrote: > Hi Paul, > > On 8/12/22 7:03 AM, Paul Kocialkowski wrote: > > On Fri 12 Aug 22, 02:56, Samuel Holland wrote: > >> The A100 variant of the DPHY requires configuring the analog registers > >> before setting the global enable bit. Since this order also works on the > >> other variants, always use it, to minimize the differences between them. > > > > Did you get a chance to actually test this with either DSI/CSI-2 hardware? > > I have tested DSI output with the Clockwork DevTerm (D1 SoC) and Pine64 > PinePhone (A64 SoC). I do not have any MIPI CSI hardware to test with. Sounds good to me then! > > I vaguely remember that the order mattered. Do you have an idea of what the > > Allwinner BSP does too? > > The Allwinner BSP makes the same change as this commit in its "lowlevel_v2x" > copy of the code, which is used for R40 and T7 (original DPHY) and A100 and D1 > (updated DPHY). It does not make the change in "lowlevel_sun50iw1" (A64 SoC, > original DPHY), but I tested A64 with this change, and it works fine. Great, thanks for details. > > Otherwise I could give it a try, at least with my MIPI CSI-2 setup > > that uses the driver. > > This commit only changes sun6i_dphy_tx_power_on(). The code for RX is unchanged > -- in fact, it already sets SUN6I_DPHY_GCTL_REG last. Ah yes you're right, actually I remember being tempted to change this too when adding the rx path, but didn't have hardware to easily test. Thanks for the details, this is: Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cheers, Paul > Regards, > Samuel > > >> Signed-off-by: Samuel Holland <samuel@sholland.org> > >> --- > >> > >> drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c > >> index 625c6e1e9990..9698d68d0db7 100644 > >> --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c > >> +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c > >> @@ -183,10 +183,6 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) > >> SUN6I_DPHY_TX_TIME4_HS_TX_ANA0(3) | > >> SUN6I_DPHY_TX_TIME4_HS_TX_ANA1(3)); > >> > >> - regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, > >> - SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | > >> - SUN6I_DPHY_GCTL_EN); > >> - > >> regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG, > >> SUN6I_DPHY_ANA0_REG_PWS | > >> SUN6I_DPHY_ANA0_REG_DMPC | > >> @@ -244,6 +240,10 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) > >> SUN6I_DPHY_ANA2_EN_P2S_CPU_MASK, > >> SUN6I_DPHY_ANA2_EN_P2S_CPU(lanes_mask)); > >> > >> + regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, > >> + SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | > >> + SUN6I_DPHY_GCTL_EN); > >> + > >> return 0; > >> } > >> > >> -- > >> 2.35.1 > >> > > >
diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c index 625c6e1e9990..9698d68d0db7 100644 --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c @@ -183,10 +183,6 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) SUN6I_DPHY_TX_TIME4_HS_TX_ANA0(3) | SUN6I_DPHY_TX_TIME4_HS_TX_ANA1(3)); - regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, - SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | - SUN6I_DPHY_GCTL_EN); - regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG, SUN6I_DPHY_ANA0_REG_PWS | SUN6I_DPHY_ANA0_REG_DMPC | @@ -244,6 +240,10 @@ static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy) SUN6I_DPHY_ANA2_EN_P2S_CPU_MASK, SUN6I_DPHY_ANA2_EN_P2S_CPU(lanes_mask)); + regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, + SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) | + SUN6I_DPHY_GCTL_EN); + return 0; }
The A100 variant of the DPHY requires configuring the analog registers before setting the global enable bit. Since this order also works on the other variants, always use it, to minimize the differences between them. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)