> -----Original Message----- > From: Nikula, Jani > Sent: Tuesday, December 27, 2016 8:18 PM > To: Chauhan, Madhav <[email protected]>; intel- > [email protected] > Cc: Conselvan De Oliveira, Ander <[email protected]>; > Saarinen, Jani <[email protected]>; Konduru, Chandra > <[email protected]>; Shankar, Uma <[email protected]>; > Mukherjee, Indranil <[email protected]>; Kumar, Shobhit > <[email protected]>; Deepak M <[email protected]> > Subject: RE: [GLK MIPI DSI V2 3/9] drm/i915/glk: Add MIPIIO Enable/disable > sequence > > On Tue, 27 Dec 2016, "Chauhan, Madhav" <[email protected]> > wrote: > >> -----Original Message----- > >> From: Nikula, Jani > >> Sent: Tuesday, December 27, 2016 6:04 PM > >> To: Chauhan, Madhav <[email protected]>; intel- > >> [email protected] > >> Cc: Conselvan De Oliveira, Ander > >> <[email protected]>; > >> Saarinen, Jani <[email protected]>; Konduru, Chandra > >> <[email protected]>; Shankar, Uma > <[email protected]>; > >> Mukherjee, Indranil <[email protected]>; Kumar, Shobhit > >> <[email protected]>; Deepak M <[email protected]> > >> Subject: RE: [GLK MIPI DSI V2 3/9] drm/i915/glk: Add MIPIIO > >> Enable/disable sequence > >> > >> On Mon, 26 Dec 2016, "Chauhan, Madhav" > <[email protected]> > >> wrote: > >> >> -----Original Message----- > >> >> From: Nikula, Jani > >> >> Sent: Friday, December 23, 2016 7:40 PM > >> >> To: Chauhan, Madhav <[email protected]>; intel- > >> >> [email protected] > >> >> Cc: Conselvan De Oliveira, Ander > >> >> <[email protected]>; > >> >> Saarinen, Jani <[email protected]>; Konduru, Chandra > >> >> <[email protected]>; Shankar, Uma > >> <[email protected]>; > >> >> Mukherjee, Indranil <[email protected]>; Kumar, Shobhit > >> >> <[email protected]>; Deepak M <[email protected]>; > >> Chauhan, > >> >> Madhav <[email protected]> > >> >> Subject: Re: [GLK MIPI DSI V2 3/9] drm/i915/glk: Add MIPIIO > >> >> Enable/disable sequence > >> >> > >> >> On Thu, 15 Dec 2016, Madhav Chauhan > <[email protected]> > >> >> wrote: > >> >> > From: Deepak M <[email protected]> > >> >> > > >> >> > v2: Addressed Jani's Review comments(renamed bit field macros) > >> >> > > >> >> > Signed-off-by: Deepak M <[email protected]> > >> >> > Signed-off-by: Madhav Chauhan <[email protected]> > >> >> > --- > >> >> > drivers/gpu/drm/i915/intel_dsi.c | 134 > >> >> > +++++++++++++++++++++++++++++++++++++++ > >> >> > 1 file changed, 134 insertions(+) > >> >> > > >> >> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c > >> >> > b/drivers/gpu/drm/i915/intel_dsi.c > >> >> > index b78c686..c0697e9 100644 > >> >> > --- a/drivers/gpu/drm/i915/intel_dsi.c > >> >> > +++ b/drivers/gpu/drm/i915/intel_dsi.c > >> >> > @@ -357,6 +357,134 @@ static bool > >> >> > intel_dsi_compute_config(struct > >> >> intel_encoder *encoder, > >> >> > return true; > >> >> > } > >> >> > > >> >> > +static void intel_dsi_disable_mipiio(struct intel_encoder *encoder) { > >> >> > + struct drm_i915_private *dev_priv = to_i915(encoder- > >base.dev); > >> >> > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder- > >base); > >> >> > + enum port port; > >> >> > + u32 tmp; > >> >> > + > >> >> > + /* Put the IO into reset */ > >> >> > + tmp = I915_READ(MIPI_CTRL(PORT_A)); > >> >> > + tmp &= ~GLK_MIPIIO_RESET_RELEASED; > >> >> > + I915_WRITE(MIPI_CTRL(PORT_A), tmp); > >> >> > + > >> >> > + /* Wait for MIPI PHY status bit to unset */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + if (intel_wait_for_register(dev_priv, > >> >> > + MIPI_CTRL(port), > >> >> > + GLK_PHY_STATUS_PORT_READY, 0, > 20)) > >> >> > + DRM_ERROR("PHY is not turning OFF\n"); > >> >> > + } > >> >> > + > >> >> > + /* Clear MIPI mode */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + tmp = I915_READ(MIPI_CTRL(port)); > >> >> > + tmp &= ~GLK_MIPIIO_ENABLE; > >> >> > + I915_WRITE(MIPI_CTRL(port), tmp); > >> >> > + } > >> >> > +} > >> >> > + > >> >> > +static void intel_dsi_enable_mipiio(struct intel_encoder *encoder) { > >> >> > + struct drm_i915_private *dev_priv = to_i915(encoder- > >base.dev); > >> >> > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder- > >base); > >> >> > + enum port port; > >> >> > + u32 tmp, val; > >> >> > + > >> >> > + /* Put the IO into reset */ > >> >> > + tmp = I915_READ(MIPI_CTRL(PORT_A)); > >> >> > + tmp &= ~GLK_MIPIIO_RESET_RELEASED; > >> >> > + I915_WRITE(MIPI_CTRL(PORT_A), tmp); > >> >> > + > >> >> > + /* Program LP Wake */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + tmp = I915_READ(MIPI_CTRL(port)); > >> >> > + tmp &= ~GLK_LP_WAKE; > >> >> > + I915_WRITE(MIPI_CTRL(port), tmp); > >> >> > + } > >> >> > + > >> >> > + /* Set the MIPI mode */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + tmp = I915_READ(MIPI_CTRL(port)); > >> >> > + I915_WRITE(MIPI_CTRL(port), tmp | > GLK_MIPIIO_ENABLE); > >> >> > + } > >> >> > + > >> >> > + /* Wait for Pwr ACK */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + if (intel_wait_for_register(dev_priv, > >> >> > + MIPI_CTRL(port), > >> >> GLK_MIPIIO_PORT_POWERED, > >> >> > + GLK_MIPIIO_PORT_POWERED, 20)) > >> >> > + DRM_ERROR("Power ACK not received\n"); > >> >> > + } > >> >> > + > >> >> > + /* Wait for MIPI PHY status bit to set */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + if (intel_wait_for_register(dev_priv, > >> >> > + MIPI_CTRL(port), > >> >> GLK_MIPIIO_PORT_POWERED, > >> >> > + GLK_MIPIIO_PORT_POWERED, 20)) > >> >> > + DRM_ERROR("PHY is not ON\n"); > >> >> > + } > >> >> > + > >> >> > + /* Get IO out of reset */ > >> >> > + tmp = I915_READ(MIPI_CTRL(PORT_A)); > >> >> > + I915_WRITE(MIPI_CTRL(PORT_A), tmp | > >> >> GLK_MIPIIO_RESET_RELEASED); > >> >> > + > >> >> > + /* Get IO out of Low power state*/ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + if (!(I915_READ(MIPI_DEVICE_READY(port)) & > >> >> DEVICE_READY)) { > >> >> > + val = I915_READ(MIPI_DEVICE_READY(port)); > >> >> > + val &= ~ULPS_STATE_MASK; > >> >> > + val |= DEVICE_READY; > >> >> > + I915_WRITE(MIPI_DEVICE_READY(port), val); > >> >> > + usleep_range(10, 15); > >> >> > + } > >> >> > + > >> >> > + /* Enter ULPS */ > >> >> > + val = I915_READ(MIPI_DEVICE_READY(port)); > >> >> > + val &= ~ULPS_STATE_MASK; > >> >> > + val |= (ULPS_STATE_ENTER | DEVICE_READY); > >> >> > + I915_WRITE(MIPI_DEVICE_READY(port), val); > >> >> > + > >> >> > + /* Wait for ULPS Not active */ > >> >> > + if (intel_wait_for_register(dev_priv, > >> >> > + MIPI_CTRL(port), > GLK_ULPS_NOT_ACTIVE, > >> >> > + GLK_ULPS_NOT_ACTIVE, 20)) > >> >> > + > >> >> > + /* Exit ULPS */ > >> >> > + val = I915_READ(MIPI_DEVICE_READY(port)); > >> >> > + val &= ~ULPS_STATE_MASK; > >> >> > + val |= (ULPS_STATE_EXIT | DEVICE_READY); > >> >> > + I915_WRITE(MIPI_DEVICE_READY(port), val); > >> >> > + > >> >> > + /* Enter Normal Mode */ > >> >> > + val = I915_READ(MIPI_DEVICE_READY(port)); > >> >> > + val &= ~ULPS_STATE_MASK; > >> >> > + val |= (ULPS_STATE_NORMAL_OPERATION | > DEVICE_READY); > >> >> > + I915_WRITE(MIPI_DEVICE_READY(port), val); > >> >> > + > >> >> > + tmp = I915_READ(MIPI_CTRL(port)); > >> >> > + tmp &= ~GLK_LP_WAKE; > >> >> > + I915_WRITE(MIPI_CTRL(port), tmp); > >> >> > + } > >> >> > + > >> >> > + /* Wait for Stop state */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + if (intel_wait_for_register(dev_priv, > >> >> > + MIPI_CTRL(port), > >> >> GLK_DATA_LANE_STOP_STATE, > >> >> > + GLK_DATA_LANE_STOP_STATE, 20)) > >> >> > + DRM_ERROR("Date lane not in STOP > state\n"); > >> >> > + } > >> >> > + > >> >> > + /* Wait for AFE LATCH */ > >> >> > + for_each_dsi_port(port, intel_dsi->ports) { > >> >> > + if (intel_wait_for_register(dev_priv, > >> >> > + BXT_MIPI_PORT_CTRL(port), > AFE_LATCHOUT, > >> >> > + AFE_LATCHOUT, 20)) > >> >> > + DRM_ERROR("D-PHY not entering LP-11 > state\n"); > >> >> > + } > >> >> > +} > >> >> > + > >> >> > >> >> I'm wondering if these should just be GLK versions of > >> >> intel_dsi_device_ready and intel_dsi_clear_device_ready. It seems > >> >> totally wrong that you're doing device ready stuff twice on GLK... > >> > > >> > Agree. Don't need to call intel_dsi_device_ready for GLK, as device > >> > ready is > >> already done inside enable_io. > >> > Will do following : > >> > If(!IS_GEMINILAKE(dev_priv) > >> > intel_dsi_device_ready(encoder); > >> > > >> >> > >> >> > >> >> > static void bxt_dsi_device_ready(struct intel_encoder *encoder) { > >> >> > struct drm_i915_private *dev_priv = > >> >> > to_i915(encoder->base.dev); > >> >> @@ > >> >> > -559,6 +687,9 @@ static void intel_dsi_pre_enable(struct > >> >> > intel_encoder *encoder, > >> >> > > >> >> > intel_dsi_prepare(encoder, pipe_config); > >> >> > > >> >> > + if (IS_GEMINILAKE(dev_priv)) > >> >> > + intel_dsi_enable_mipiio(encoder); > >> >> > + > >> >> > /* Panel Enable over CRC PMIC */ > >> >> > if (intel_dsi->gpio_panel) > >> >> > gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1); > @@ - > >> >> 699,6 > >> >> > +830,9 @@ static void intel_dsi_clear_device_ready(struct > >> >> > +intel_encoder > >> >> *encoder) > >> >> > usleep_range(2000, 2500); > >> >> > } > >> >> > > >> >> > + if (IS_GEMINILAKE(dev_priv)) > >> >> > + intel_dsi_disable_mipiio(encoder); > >> >> > + > >> >> > >> >> When you're doing enable/disable of something, they should be > >> >> called from the corresponding functions in the enable/disable > >> >> paths. But this is just a general remark, if we conclude that > >> >> these should be alternative device ready/unready calls instead. > >> > > >> > Yes agree, intel_dsi_disable_mipiio should be called from > >> > intel_dsi_post_disable after intel_dsi_clear_device_ready and > >> > before > >> intel_disable_dsi_pll Will do these changes in next series. > >> > >> I meant, how about renaming intel_dsi_enable_mipiio to > >> glk_dsi_device_ready, and making intel_dsi_device_ready call > >> glk_dsi_device_ready for GLK. > >> > >> Then rename intel_dsi_clear_device_ready to > >> vlv_dsi_clear_device_ready and intel_dsi_disable_mipiio to > >> glk_dsi_clear_device_ready, and add a new > >> intel_dsi_clear_device_ready wrapper to call vlv_dsi_clear_device_ready > for VLV/CHV/BXT, and glk_dsi_clear_device_ready for GLK. > >> > >> How does that sound? > > > > That's good input. It will align the code to the platforms. > > For glk_dsi_clear_device_ready, we need to 1. Disable MIPI IO, 2. > > Enter LP Shouldn't we add 2 separate functions for 1,2 and call them inside > glk_dsi_clear_device_ready, it will make code readable/modularize as per > BSPEC?? > > Sure, abstractions like that are fine.
Thanks. Will send the patch addressing all these review comments in next series. > > BR, > Jani. > > > > >> > >> > >> BR, > >> Jani. > >> > >> >> > >> >> BR, > >> >> Jani. > >> >> > >> >> > >> >> > intel_disable_dsi_pll(encoder); } > >> >> > >> >> -- > >> >> Jani Nikula, Intel Open Source Technology Center > >> > >> -- > >> Jani Nikula, Intel Open Source Technology Center > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
