| Message ID | 20220218010054.315026-6-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: tc358767: Add DSI-to-DPI mode support | expand |
Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut: > The TC358767/TC358867/TC9595 are all capable of operating either from > attached Xtal or from DSI clock lane clock. In case the later is used, > all I2C accesses will fail until the DSI clock lane is running and > supplying clock to the chip. > > Move all hardware initialization to enable callback to guarantee the > DSI clock lane is running before accessing the hardware. In case Xtal > is attached to the chip, this change has no effect. I'm not sure if that last statement is correct. When the chip is bridging to eDP, the aux channel and HPD handling is needed to be functional way before the atomic enable happen. I have no idea how this would interact with the clock supplied from the DSI lanes. Maybe it doesn't work at all and proper eDP support always needs a external reference clock? I think we should make the "ref" clock a optional clock to properly describe the fact that the chip can operate without this clock in DSI input mode and then either do the chip init in the probe routine when the ref clock is present, or defer it to atomic enable when the ref clock is absent. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Maxime Ripard <maxime@cerno.tech> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > --- > V2: - Rebase on next-20220217 > --- > drivers/gpu/drm/bridge/tc358767.c | 111 +++++++++++++++++------------- > 1 file changed, 63 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 01d11adee6c74..134c4d8621236 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1234,6 +1234,63 @@ static int tc_edp_stream_disable(struct tc_data *tc) > return 0; > } > > +static int tc_hardware_init(struct tc_data *tc) > +{ > + int ret; > + > + ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); > + if (ret) { > + dev_err(tc->dev, "can not read device ID: %d\n", ret); > + return ret; > + } > + > + if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) { > + dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev); > + return -EINVAL; > + } > + > + tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ > + > + if (!tc->reset_gpio) { > + /* > + * If the reset pin isn't present, do a software reset. It isn't > + * as thorough as the hardware reset, as we can't reset the I2C > + * communication block for obvious reasons, but it's getting the > + * chip into a defined state. > + */ > + regmap_update_bits(tc->regmap, SYSRSTENB, > + ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, > + 0); > + regmap_update_bits(tc->regmap, SYSRSTENB, > + ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, > + ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP); > + usleep_range(5000, 10000); > + } > + > + if (tc->hpd_pin >= 0) { > + u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT; > + u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin); > + > + /* Set LCNT to 2ms */ > + regmap_write(tc->regmap, lcnt_reg, > + clk_get_rate(tc->refclk) * 2 / 1000); > + /* We need the "alternate" mode for HPD */ > + regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin)); > + > + if (tc->have_irq) { > + /* enable H & LC */ > + regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc); > + } > + } > + > + if (tc->have_irq) { > + /* enable SysErr */ > + regmap_write(tc->regmap, INTCTL_G, INT_SYSERR); > + } > + > + return 0; > +} > + > static void > tc_edp_bridge_atomic_enable(struct drm_bridge *bridge, > struct drm_bridge_state *old_bridge_state) > @@ -1241,6 +1298,12 @@ tc_edp_bridge_atomic_enable(struct drm_bridge *bridge, > struct tc_data *tc = bridge_to_tc(bridge); > int ret; > > + ret = tc_hardware_init(tc); > + if (ret < 0) { > + dev_err(tc->dev, "failed to initialize bridge: %d\n", ret); > + return; > + } > + > ret = tc_get_display_props(tc); > if (ret < 0) { > dev_err(tc->dev, "failed to read display props: %d\n", ret); > @@ -1660,9 +1723,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > } > > if (client->irq > 0) { > - /* enable SysErr */ > - regmap_write(tc->regmap, INTCTL_G, INT_SYSERR); > - > ret = devm_request_threaded_irq(dev, client->irq, > NULL, tc_irq_handler, > IRQF_ONESHOT, > @@ -1675,51 +1735,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > tc->have_irq = true; > } > > - ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); > - if (ret) { > - dev_err(tc->dev, "can not read device ID: %d\n", ret); > - return ret; > - } > - > - if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) { > - dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev); > - return -EINVAL; > - } > - > - tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ > - > - if (!tc->reset_gpio) { > - /* > - * If the reset pin isn't present, do a software reset. It isn't > - * as thorough as the hardware reset, as we can't reset the I2C > - * communication block for obvious reasons, but it's getting the > - * chip into a defined state. > - */ > - regmap_update_bits(tc->regmap, SYSRSTENB, > - ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, > - 0); > - regmap_update_bits(tc->regmap, SYSRSTENB, > - ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, > - ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP); > - usleep_range(5000, 10000); > - } > - > - if (tc->hpd_pin >= 0) { > - u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT; > - u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin); > - > - /* Set LCNT to 2ms */ > - regmap_write(tc->regmap, lcnt_reg, > - clk_get_rate(tc->refclk) * 2 / 1000); > - /* We need the "alternate" mode for HPD */ > - regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin)); > - > - if (tc->have_irq) { > - /* enable H & LC */ > - regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc); > - } > - } > - > ret = tc_aux_link_setup(tc); The above function is also poking i2c regs, as will any DP AUX transaction in case of eDP. Regards, Lucas > if (ret) > return ret;
On 2/18/22 18:49, Lucas Stach wrote: > Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut: >> The TC358767/TC358867/TC9595 are all capable of operating either from >> attached Xtal or from DSI clock lane clock. In case the later is used, >> all I2C accesses will fail until the DSI clock lane is running and >> supplying clock to the chip. >> >> Move all hardware initialization to enable callback to guarantee the >> DSI clock lane is running before accessing the hardware. In case Xtal >> is attached to the chip, this change has no effect. > > I'm not sure if that last statement is correct. When the chip is > bridging to eDP, the aux channel and HPD handling is needed to be > functional way before the atomic enable happen. I have no idea how this > would interact with the clock supplied from the DSI lanes. Maybe it > doesn't work at all and proper eDP support always needs a external > reference clock? The driver currently assumes the TC358767 always gets RefClk from Xtal. There is subsequent series which adds support for deriving clock which drive the TC358767 PLLs from DSI HS clock instead of Xtal in case the bridge operates in DSI-to-DPI mode. That needs additional plumbing, as the TC358767 must be able to select specific clock frequency on the DSI HS clock lane, because its PLLs need specific frequencies, see: [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock from DSI bridge If someone needs to implement DSI-to-(e)DP mode without Xtal, ugh, that would likely need to have a way to figure out the DSI HS clock frequency already in probe and then enable those DSI HS clock very early on too ? > I think we should make the "ref" clock a optional clock to properly > describe the fact that the chip can operate without this clock in DSI > input mode and then either do the chip init in the probe routine when > the ref clock is present, or defer it to atomic enable when the ref > clock is absent. See the RFC patchset above, that patchset does exactly that, it makes RefClk optional. [...]
Am Samstag, dem 19.02.2022 um 03:39 +0100 schrieb Marek Vasut: > On 2/18/22 18:49, Lucas Stach wrote: > > Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut: > > > The TC358767/TC358867/TC9595 are all capable of operating either from > > > attached Xtal or from DSI clock lane clock. In case the later is used, > > > all I2C accesses will fail until the DSI clock lane is running and > > > supplying clock to the chip. > > > > > > Move all hardware initialization to enable callback to guarantee the > > > DSI clock lane is running before accessing the hardware. In case Xtal > > > is attached to the chip, this change has no effect. > > > > I'm not sure if that last statement is correct. When the chip is > > bridging to eDP, the aux channel and HPD handling is needed to be > > functional way before the atomic enable happen. I have no idea how this > > would interact with the clock supplied from the DSI lanes. Maybe it > > doesn't work at all and proper eDP support always needs a external > > reference clock? > > The driver currently assumes the TC358767 always gets RefClk from Xtal. > > There is subsequent series which adds support for deriving clock which > drive the TC358767 PLLs from DSI HS clock instead of Xtal in case the > bridge operates in DSI-to-DPI mode. That needs additional plumbing, as > the TC358767 must be able to select specific clock frequency on the DSI > HS clock lane, because its PLLs need specific frequencies, see: > > [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock > from DSI bridge > > If someone needs to implement DSI-to-(e)DP mode without Xtal, ugh, that > would likely need to have a way to figure out the DSI HS clock frequency > already in probe and then enable those DSI HS clock very early on too ? > Probably, but that was really just something I wondered about while passing by. The real issue is that I think _this_ patch breaks eDP operation, as now the chip is initialized way too late, as the AUX channel functionality needs to be available long before the atomic bridge enable callback is called. The AUX channel is used to fetch the display EDID, so before you can even set a mode it needs to be functional. Unconditionally moving the chip init is probably (I haven't tested it yet) going to break this. Regards, Lucas > > I think we should make the "ref" clock a optional clock to properly > > describe the fact that the chip can operate without this clock in DSI > > input mode and then either do the chip init in the probe routine when > > the ref clock is present, or defer it to atomic enable when the ref > > clock is absent. > > See the RFC patchset above, that patchset does exactly that, it makes > RefClk optional. > > [...]
On 2/21/22 10:12, Lucas Stach wrote: > Am Samstag, dem 19.02.2022 um 03:39 +0100 schrieb Marek Vasut: >> On 2/18/22 18:49, Lucas Stach wrote: >>> Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut: >>>> The TC358767/TC358867/TC9595 are all capable of operating either from >>>> attached Xtal or from DSI clock lane clock. In case the later is used, >>>> all I2C accesses will fail until the DSI clock lane is running and >>>> supplying clock to the chip. >>>> >>>> Move all hardware initialization to enable callback to guarantee the >>>> DSI clock lane is running before accessing the hardware. In case Xtal >>>> is attached to the chip, this change has no effect. >>> >>> I'm not sure if that last statement is correct. When the chip is >>> bridging to eDP, the aux channel and HPD handling is needed to be >>> functional way before the atomic enable happen. I have no idea how this >>> would interact with the clock supplied from the DSI lanes. Maybe it >>> doesn't work at all and proper eDP support always needs a external >>> reference clock? >> >> The driver currently assumes the TC358767 always gets RefClk from Xtal. >> >> There is subsequent series which adds support for deriving clock which >> drive the TC358767 PLLs from DSI HS clock instead of Xtal in case the >> bridge operates in DSI-to-DPI mode. That needs additional plumbing, as >> the TC358767 must be able to select specific clock frequency on the DSI >> HS clock lane, because its PLLs need specific frequencies, see: >> >> [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock >> from DSI bridge >> >> If someone needs to implement DSI-to-(e)DP mode without Xtal, ugh, that >> would likely need to have a way to figure out the DSI HS clock frequency >> already in probe and then enable those DSI HS clock very early on too ? >> > Probably, but that was really just something I wondered about while > passing by. > > The real issue is that I think _this_ patch breaks eDP operation, as > now the chip is initialized way too late, as the AUX channel > functionality needs to be available long before the atomic bridge > enable callback is called. I don't think that's correct -- look at the tc_hardware_init() function, all it does is it reads the chip ID, optionally does software reset if there is no reset GPIO connected, and then sets up hotplug detect IRQ. There is nothing specific to bringing up the AUX channel in that function, so the AUX channel should work even before tc_hardware_init() is called. > The AUX channel is used to fetch the display > EDID, so before you can even set a mode it needs to be functional. > Unconditionally moving the chip init is probably (I haven't tested it > yet) going to break this. If you have such a setup with eDP, please test it, I don't think this is going to break it.
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 01d11adee6c74..134c4d8621236 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1234,6 +1234,63 @@ static int tc_edp_stream_disable(struct tc_data *tc) return 0; } +static int tc_hardware_init(struct tc_data *tc) +{ + int ret; + + ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); + if (ret) { + dev_err(tc->dev, "can not read device ID: %d\n", ret); + return ret; + } + + if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) { + dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev); + return -EINVAL; + } + + tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ + + if (!tc->reset_gpio) { + /* + * If the reset pin isn't present, do a software reset. It isn't + * as thorough as the hardware reset, as we can't reset the I2C + * communication block for obvious reasons, but it's getting the + * chip into a defined state. + */ + regmap_update_bits(tc->regmap, SYSRSTENB, + ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, + 0); + regmap_update_bits(tc->regmap, SYSRSTENB, + ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, + ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP); + usleep_range(5000, 10000); + } + + if (tc->hpd_pin >= 0) { + u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT; + u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin); + + /* Set LCNT to 2ms */ + regmap_write(tc->regmap, lcnt_reg, + clk_get_rate(tc->refclk) * 2 / 1000); + /* We need the "alternate" mode for HPD */ + regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin)); + + if (tc->have_irq) { + /* enable H & LC */ + regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc); + } + } + + if (tc->have_irq) { + /* enable SysErr */ + regmap_write(tc->regmap, INTCTL_G, INT_SYSERR); + } + + return 0; +} + static void tc_edp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) @@ -1241,6 +1298,12 @@ tc_edp_bridge_atomic_enable(struct drm_bridge *bridge, struct tc_data *tc = bridge_to_tc(bridge); int ret; + ret = tc_hardware_init(tc); + if (ret < 0) { + dev_err(tc->dev, "failed to initialize bridge: %d\n", ret); + return; + } + ret = tc_get_display_props(tc); if (ret < 0) { dev_err(tc->dev, "failed to read display props: %d\n", ret); @@ -1660,9 +1723,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) } if (client->irq > 0) { - /* enable SysErr */ - regmap_write(tc->regmap, INTCTL_G, INT_SYSERR); - ret = devm_request_threaded_irq(dev, client->irq, NULL, tc_irq_handler, IRQF_ONESHOT, @@ -1675,51 +1735,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) tc->have_irq = true; } - ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev); - if (ret) { - dev_err(tc->dev, "can not read device ID: %d\n", ret); - return ret; - } - - if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) { - dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev); - return -EINVAL; - } - - tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */ - - if (!tc->reset_gpio) { - /* - * If the reset pin isn't present, do a software reset. It isn't - * as thorough as the hardware reset, as we can't reset the I2C - * communication block for obvious reasons, but it's getting the - * chip into a defined state. - */ - regmap_update_bits(tc->regmap, SYSRSTENB, - ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, - 0); - regmap_update_bits(tc->regmap, SYSRSTENB, - ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP, - ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP); - usleep_range(5000, 10000); - } - - if (tc->hpd_pin >= 0) { - u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT; - u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin); - - /* Set LCNT to 2ms */ - regmap_write(tc->regmap, lcnt_reg, - clk_get_rate(tc->refclk) * 2 / 1000); - /* We need the "alternate" mode for HPD */ - regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin)); - - if (tc->have_irq) { - /* enable H & LC */ - regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc); - } - } - ret = tc_aux_link_setup(tc); if (ret) return ret;
The TC358767/TC358867/TC9595 are all capable of operating either from attached Xtal or from DSI clock lane clock. In case the later is used, all I2C accesses will fail until the DSI clock lane is running and supplying clock to the chip. Move all hardware initialization to enable callback to guarantee the DSI clock lane is running before accessing the hardware. In case Xtal is attached to the chip, this change has no effect. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Maxime Ripard <maxime@cerno.tech> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Sam Ravnborg <sam@ravnborg.org> --- V2: - Rebase on next-20220217 --- drivers/gpu/drm/bridge/tc358767.c | 111 +++++++++++++++++------------- 1 file changed, 63 insertions(+), 48 deletions(-)