Hi Marek
On Sat, 27 Nov 2021 at 03:24, Marek Vasut <marex@xxxxxxx> wrote:
>
> 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.
This puzzles me as there seem to be a couple of ambiguities over how
it would be used.
Firstly the DT binding lists the clock as a required property, but
there isn't one present if deriving the clock from the DSI clock lane.
Secondly the datasheet states that the DSI Reference Clock Source
Division Selection is done by the MODE1 pin, and switches between
HSCKBY2 divided by 7 and HSCKBY2 divided by 9. There is a stated
assumption that HSCK is either 364MHz or 468MHz, thereby ending up
with 26MHz. To do this we have to be running DSI in burst mode, but
the support for that in DRM is near zero.
Can I ask how the chip has actually been configured and run in this mode?
Thanks
Dave
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> Cc: Jonas Karlman <jonas@xxxxxxxxx>
> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> ---
> 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 23a6f90b694b..f7fbf6f673e9 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1234,11 +1234,74 @@ static int tc_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_bridge_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);
> @@ -1626,9 +1689,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,
> @@ -1641,51 +1701,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;
> --
> 2.33.0
>
