| Message ID | 1488904944-14285-2-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Neil, On 07-03-2017 16:42, Neil Armstrong wrote: > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > In preparation for adding PHY operations to handle RX SENSE and HPD, > group all the PHY interrupt setup code in a single location and extract > it to a separate function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 50 ++++++++++++++----------------- > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 026a0dc..1ed8bc1 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -1496,7 +1496,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) > } > > /* Wait until we are registered to enable interrupts */ > -static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi) > +static void dw_hdmi_fb_registered(struct dw_hdmi *hdmi) > { > hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL, > HDMI_PHY_I2CM_INT_ADDR); > @@ -1504,15 +1504,6 @@ static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi) > hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL | > HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL, > HDMI_PHY_I2CM_CTLINT_ADDR); > - > - /* enable cable hot plug irq */ > - hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > - > - /* Clear Hotplug interrupts */ > - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > - HDMI_IH_PHY_STAT0); > - > - return 0; > } > > static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) > @@ -1630,6 +1621,26 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi) > hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > } > > +static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi) > +{ > + /* > + * Configure the PHY RX SENSE and HPD interrupts polarities and clear > + * any pending interrupt. > + */ > + hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > + HDMI_IH_PHY_STAT0); > + > + /* Enable cable hot plug irq. */ > + hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > + > + /* Clear and unmute interrupts. */ > + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > + HDMI_IH_PHY_STAT0); > + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > + HDMI_IH_MUTE_PHY_STAT0); > +} > + > static enum drm_connector_status > dw_hdmi_connector_detect(struct drm_connector *connector, bool force) > { > @@ -2141,29 +2152,14 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > hdmi->ddc = NULL; > } > > - /* > - * Configure registers related to HDMI interrupt > - * generation before registering IRQ. > - */ I've seen the databook and HPD interrupts are enabled per default (actually I think most of the interrupts are). I'm thinking whether we could get spurious interrupts because of not configuring HPD before registering IRQ (probably thats why the comment was there). How did you test this? I think that if you insert the cable before loading modules then you can try to emulate this condition. What do you think? Best regards, Jose Miguel Abreu > - hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > - > - /* Clear Hotplug interrupts */ > - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > - HDMI_IH_PHY_STAT0); > - > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > #ifdef CONFIG_OF > hdmi->bridge.of_node = pdev->dev.of_node; > #endif > > - ret = dw_hdmi_fb_registered(hdmi); > - if (ret) > - goto err_iahb; > - > - /* Unmute interrupts */ > - hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > - HDMI_IH_MUTE_PHY_STAT0); > + dw_hdmi_fb_registered(hdmi); > + dw_hdmi_phy_setup_hpd(hdmi); > > memset(&pdevinfo, 0, sizeof(pdevinfo)); > pdevinfo.parent = dev;
Hi Jose, On 03/07/2017 06:12 PM, Jose Abreu wrote: > Hi Neil, > > > On 07-03-2017 16:42, Neil Armstrong wrote: >> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> >> In preparation for adding PHY operations to handle RX SENSE and HPD, >> group all the PHY interrupt setup code in a single location and extract >> it to a separate function. >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 50 ++++++++++++++----------------- >> 1 file changed, 23 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> index 026a0dc..1ed8bc1 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -1496,7 +1496,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) >> } >> >> /* Wait until we are registered to enable interrupts */ >> -static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi) >> +static void dw_hdmi_fb_registered(struct dw_hdmi *hdmi) >> { >> hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL, >> HDMI_PHY_I2CM_INT_ADDR); >> @@ -1504,15 +1504,6 @@ static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi) >> hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL | >> HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL, >> HDMI_PHY_I2CM_CTLINT_ADDR); >> - >> - /* enable cable hot plug irq */ >> - hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); >> - >> - /* Clear Hotplug interrupts */ >> - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, >> - HDMI_IH_PHY_STAT0); >> - >> - return 0; >> } >> >> static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) >> @@ -1630,6 +1621,26 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi) >> hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); >> } >> >> +static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi) >> +{ >> + /* >> + * Configure the PHY RX SENSE and HPD interrupts polarities and clear >> + * any pending interrupt. >> + */ >> + hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); >> + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, >> + HDMI_IH_PHY_STAT0); >> + >> + /* Enable cable hot plug irq. */ >> + hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); >> + >> + /* Clear and unmute interrupts. */ >> + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, >> + HDMI_IH_PHY_STAT0); >> + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), >> + HDMI_IH_MUTE_PHY_STAT0); >> +} >> + >> static enum drm_connector_status >> dw_hdmi_connector_detect(struct drm_connector *connector, bool force) >> { >> @@ -2141,29 +2152,14 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> hdmi->ddc = NULL; >> } >> >> - /* >> - * Configure registers related to HDMI interrupt >> - * generation before registering IRQ. >> - */ > > > I've seen the databook and HPD interrupts are enabled per default > (actually I think most of the interrupts are). I'm thinking > whether we could get spurious interrupts because of not > configuring HPD before registering IRQ (probably thats why the > comment was there). How did you test this? I think that if you > insert the cable before loading modules then you can try to > emulate this condition. What do you think? In fact the comment was wrong because it happened after the IRQ register, and right before the IRQ register the initialize_hdmi_ih_mutes() is already called to mute all the IRQ sources. The previous rework moved the IRQ code, so this cleanup is welcome. But indeed, I did not check this condition, I can try this. Thanks, Neil > > Best regards, > Jose Miguel Abreu > >> - hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); >> - >> - /* Clear Hotplug interrupts */ >> - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, >> - HDMI_IH_PHY_STAT0); >> - >> hdmi->bridge.driver_private = hdmi; >> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; >> #ifdef CONFIG_OF >> hdmi->bridge.of_node = pdev->dev.of_node; >> #endif >> >> - ret = dw_hdmi_fb_registered(hdmi); >> - if (ret) >> - goto err_iahb; >> - >> - /* Unmute interrupts */ >> - hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), >> - HDMI_IH_MUTE_PHY_STAT0); >> + dw_hdmi_fb_registered(hdmi); >> + dw_hdmi_phy_setup_hpd(hdmi); >> >> memset(&pdevinfo, 0, sizeof(pdevinfo)); >> pdevinfo.parent = dev;
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 026a0dc..1ed8bc1 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1496,7 +1496,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) } /* Wait until we are registered to enable interrupts */ -static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi) +static void dw_hdmi_fb_registered(struct dw_hdmi *hdmi) { hdmi_writeb(hdmi, HDMI_PHY_I2CM_INT_ADDR_DONE_POL, HDMI_PHY_I2CM_INT_ADDR); @@ -1504,15 +1504,6 @@ static int dw_hdmi_fb_registered(struct dw_hdmi *hdmi) hdmi_writeb(hdmi, HDMI_PHY_I2CM_CTLINT_ADDR_NAC_POL | HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL, HDMI_PHY_I2CM_CTLINT_ADDR); - - /* enable cable hot plug irq */ - hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); - - /* Clear Hotplug interrupts */ - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, - HDMI_IH_PHY_STAT0); - - return 0; } static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) @@ -1630,6 +1621,26 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi) hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); } +static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi) +{ + /* + * Configure the PHY RX SENSE and HPD interrupts polarities and clear + * any pending interrupt. + */ + hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, + HDMI_IH_PHY_STAT0); + + /* Enable cable hot plug irq. */ + hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); + + /* Clear and unmute interrupts. */ + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, + HDMI_IH_PHY_STAT0); + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), + HDMI_IH_MUTE_PHY_STAT0); +} + static enum drm_connector_status dw_hdmi_connector_detect(struct drm_connector *connector, bool force) { @@ -2141,29 +2152,14 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) hdmi->ddc = NULL; } - /* - * Configure registers related to HDMI interrupt - * generation before registering IRQ. - */ - hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); - - /* Clear Hotplug interrupts */ - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, - HDMI_IH_PHY_STAT0); - hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; #ifdef CONFIG_OF hdmi->bridge.of_node = pdev->dev.of_node; #endif - ret = dw_hdmi_fb_registered(hdmi); - if (ret) - goto err_iahb; - - /* Unmute interrupts */ - hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), - HDMI_IH_MUTE_PHY_STAT0); + dw_hdmi_fb_registered(hdmi); + dw_hdmi_phy_setup_hpd(hdmi); memset(&pdevinfo, 0, sizeof(pdevinfo)); pdevinfo.parent = dev;