| Message ID | 1648656179-10347-5-git-send-email-quic_sbillaka@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for the eDP panel over aux_bus | expand |
On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti <quic_sbillaka@quicinc.com> wrote: > > The interrupt register will still reflect the connect and disconnect > interrupt status without generating an actual HW interrupt. > The controller driver should not handle those masked interrupts. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > index 3c16f95..1809ce2 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) > { > struct dp_catalog_private *catalog = container_of(dp_catalog, > struct dp_catalog_private, dp_catalog); > - int isr = 0; > + int isr, mask; > > isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); > dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, > (isr & DP_DP_HPD_INT_MASK)); > + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); > > - return isr; > + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); I suspect that the logic is inverted here. Shouldn't it be: return isr & DP_DP_HPD_STATE_STATUS_MASK & mask; ? > } > > int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) > -- > 2.7.4 >
Hi Dmitry, > On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti > <quic_sbillaka@quicinc.com> wrote: > > > > The interrupt register will still reflect the connect and disconnect > > interrupt status without generating an actual HW interrupt. > > The controller driver should not handle those masked interrupts. > > > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > > --- > > drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > > b/drivers/gpu/drm/msm/dp/dp_catalog.c > > index 3c16f95..1809ce2 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct > > dp_catalog *dp_catalog) { > > struct dp_catalog_private *catalog = container_of(dp_catalog, > > struct dp_catalog_private, dp_catalog); > > - int isr = 0; > > + int isr, mask; > > > > isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); > > dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, > > (isr & DP_DP_HPD_INT_MASK)); > > + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); > > > > - return isr; > > + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); > > I suspect that the logic is inverted here. Shouldn't it be: > > return isr & DP_DP_HPD_STATE_STATUS_MASK & mask; > > ? > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the value of the read interrupt mask variable could be is 0xF. The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, bits 3:0. The HPD status is indicated via a different read-only register REG_DP_DP_HPD_INT_STATUS, bits 31:29. isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always. > > } > > > > int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) > > -- > > 2.7.4 > > > > > -- > With best wishes > Dmitry Thank you, Sankeerth
On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote: > Hi Dmitry, > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti >> <quic_sbillaka@quicinc.com> wrote: >>> >>> The interrupt register will still reflect the connect and disconnect >>> interrupt status without generating an actual HW interrupt. >>> The controller driver should not handle those masked interrupts. >>> >>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c >>> index 3c16f95..1809ce2 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct >>> dp_catalog *dp_catalog) { >>> struct dp_catalog_private *catalog = container_of(dp_catalog, >>> struct dp_catalog_private, dp_catalog); >>> - int isr = 0; >>> + int isr, mask; >>> >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, >>> (isr & DP_DP_HPD_INT_MASK)); >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); >>> >>> - return isr; >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); >> >> I suspect that the logic is inverted here. Shouldn't it be: >> >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask; >> >> ? >> > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the value of the read > interrupt mask variable could be is 0xF. > > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, bits 3:0. > The HPD status is indicated via a different read-only register REG_DP_DP_HPD_INT_STATUS, bits 31:29. I see. Maybe the following expression would be better? return isr & (mask & ~DP_DP_HPD_INT_MASK); > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always. > >>> } >>> >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) >>> -- >>> 2.7.4 >>> >> >> >> -- >> With best wishes >> Dmitry > > Thank you, > Sankeerth
Hi Dmitry, > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote: > > Hi Dmitry, > > > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti > >> <quic_sbillaka@quicinc.com> wrote: > >>> > >>> The interrupt register will still reflect the connect and disconnect > >>> interrupt status without generating an actual HW interrupt. > >>> The controller driver should not handle those masked interrupts. > >>> > >>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > >>> --- > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c > >>> index 3c16f95..1809ce2 100644 > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct > >>> dp_catalog *dp_catalog) { > >>> struct dp_catalog_private *catalog = container_of(dp_catalog, > >>> struct dp_catalog_private, dp_catalog); > >>> - int isr = 0; > >>> + int isr, mask; > >>> > >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); > >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, > >>> (isr & DP_DP_HPD_INT_MASK)); > >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); > >>> > >>> - return isr; > >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); > >> > >> I suspect that the logic is inverted here. Shouldn't it be: > >> > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask; > >> > >> ? > >> > > > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the > value > > of the read interrupt mask variable could be is 0xF. > > > > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, > bits 3:0. > > The HPD status is indicated via a different read-only register > REG_DP_DP_HPD_INT_STATUS, bits 31:29. > > I see. Maybe the following expression would be better? > > return isr & (mask & ~DP_DP_HPD_INT_MASK); > I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under the same register definition as REG_DP_DP_HPD_INT_MASK I will rearrange the definitions below. #define REG_DP_DP_HPD_INT_MASK (0x0000000C) #define DP_DP_HPD_PLUG_INT_MASK (0x00000001) #define DP_DP_IRQ_HPD_INT_MASK (0x00000002) #define DP_DP_HPD_REPLUG_INT_MASK (0x00000004) #define DP_DP_HPD_UNPLUG_INT_MASK (0x00000008) #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \ DP_DP_IRQ_HPD_INT_MASK | \ DP_DP_HPD_REPLUG_INT_MASK | \ DP_DP_HPD_UNPLUG_INT_MASK) Below are status bits from register REG_DP_DP_HPD_INT_STATUS #define DP_DP_HPD_STATE_STATUS_CONNECTED (0x40000000) #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000) #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000) #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000) DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always. For DP, we want to enable all interrupts. So the programmed mask value is 0xF. We want to return 0x40000001 for connect and 8 for disconnect For eDP, we want to disable the connect and disconnect interrupts. So, the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK) We want to return 0x40000000 (or 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect respectively. > > > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always. > > > >>> } > >>> > >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) > >>> -- > >>> 2.7.4 > >>> > >> > >> > >> -- > >> With best wishes > >> Dmitry > > > > Thank you, > > Sankeerth > > > -- > With best wishes > Dmitry Thank you, Sankeerth
On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti <sbillaka@qti.qualcomm.com> wrote: > > Hi Dmitry, > > > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote: > > > Hi Dmitry, > > > > > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti > > >> <quic_sbillaka@quicinc.com> wrote: > > >>> > > >>> The interrupt register will still reflect the connect and disconnect > > >>> interrupt status without generating an actual HW interrupt. > > >>> The controller driver should not handle those masked interrupts. > > >>> > > >>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > > >>> --- > > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- > > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c > > >>> index 3c16f95..1809ce2 100644 > > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct > > >>> dp_catalog *dp_catalog) { > > >>> struct dp_catalog_private *catalog = container_of(dp_catalog, > > >>> struct dp_catalog_private, dp_catalog); > > >>> - int isr = 0; > > >>> + int isr, mask; > > >>> > > >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); > > >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, > > >>> (isr & DP_DP_HPD_INT_MASK)); > > >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); > > >>> > > >>> - return isr; > > >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); > > >> > > >> I suspect that the logic is inverted here. Shouldn't it be: > > >> > > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask; > > >> > > >> ? > > >> > > > > > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the > > value > > > of the read interrupt mask variable could be is 0xF. > > > > > > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, > > bits 3:0. > > > The HPD status is indicated via a different read-only register > > REG_DP_DP_HPD_INT_STATUS, bits 31:29. > > > > I see. Maybe the following expression would be better? > > > > return isr & (mask & ~DP_DP_HPD_INT_MASK); Ugh, excuse me please. I meant: return isr & (mask | ~DP_DP_HPD_INT_MASK); > > > > I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under the same register definition as REG_DP_DP_HPD_INT_MASK > I will rearrange the definitions below. > > #define REG_DP_DP_HPD_INT_MASK (0x0000000C) > #define DP_DP_HPD_PLUG_INT_MASK (0x00000001) > #define DP_DP_IRQ_HPD_INT_MASK (0x00000002) > #define DP_DP_HPD_REPLUG_INT_MASK (0x00000004) > #define DP_DP_HPD_UNPLUG_INT_MASK (0x00000008) > #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \ > DP_DP_IRQ_HPD_INT_MASK | \ > DP_DP_HPD_REPLUG_INT_MASK | \ > DP_DP_HPD_UNPLUG_INT_MASK) > > Below are status bits from register REG_DP_DP_HPD_INT_STATUS > > #define DP_DP_HPD_STATE_STATUS_CONNECTED (0x40000000) > #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000) > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000) > #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000) > > DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always. > > For DP, we want to enable all interrupts. > So the programmed mask value is 0xF. We want to return 0x40000001 for connect and 8 for disconnect > > For eDP, we want to disable the connect and disconnect interrupts. So, the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK) > We want to return 0x40000000 (or 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect respectively. > > > > > > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always. > > > > > >>> } > > >>> > > >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) > > >>> -- > > >>> 2.7.4 > > >>> > > >> > > >> > > >> -- > > >> With best wishes > > >> Dmitry > > > > > > Thank you, > > > Sankeerth > > > > > > -- > > With best wishes > > Dmitry > > Thank you, > Sankeerth
> On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti > <sbillaka@qti.qualcomm.com> wrote: > > > > Hi Dmitry, > > > > > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote: > > > > Hi Dmitry, > > > > > > > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti > > > >> <quic_sbillaka@quicinc.com> wrote: > > > >>> > > > >>> The interrupt register will still reflect the connect and > > > >>> disconnect interrupt status without generating an actual HW > interrupt. > > > >>> The controller driver should not handle those masked interrupts. > > > >>> > > > >>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > > > >>> --- > > > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- > > > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > > > >>> > > > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > > > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > >>> index 3c16f95..1809ce2 100644 > > > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > > > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > > > >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct > > > >>> dp_catalog *dp_catalog) { > > > >>> struct dp_catalog_private *catalog = container_of(dp_catalog, > > > >>> struct dp_catalog_private, dp_catalog); > > > >>> - int isr = 0; > > > >>> + int isr, mask; > > > >>> > > > >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); > > > >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, > > > >>> (isr & DP_DP_HPD_INT_MASK)); > > > >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); > > > >>> > > > >>> - return isr; > > > >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); > > > >> > > > >> I suspect that the logic is inverted here. Shouldn't it be: > > > >> > > > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask; > > > >> > > > >> ? > > > >> > > > > > > > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the > > > value > > > > of the read interrupt mask variable could be is 0xF. > > > > > > > > The mask value is indicated via the register, > > > > REG_DP_DP_HPD_INT_MASK, > > > bits 3:0. > > > > The HPD status is indicated via a different read-only register > > > REG_DP_DP_HPD_INT_STATUS, bits 31:29. > > > > > > I see. Maybe the following expression would be better? > > > > > > return isr & (mask & ~DP_DP_HPD_INT_MASK); > > Ugh, excuse me please. I meant: > > return isr & (mask | ~DP_DP_HPD_INT_MASK); > Okay. I will change it. > > > > > > > I believe the confusion occurred because the > > DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under > the same register definition as REG_DP_DP_HPD_INT_MASK I will rearrange > the definitions below. > > > > #define REG_DP_DP_HPD_INT_MASK (0x0000000C) > > #define DP_DP_HPD_PLUG_INT_MASK (0x00000001) > > #define DP_DP_IRQ_HPD_INT_MASK (0x00000002) > > #define DP_DP_HPD_REPLUG_INT_MASK (0x00000004) > > #define DP_DP_HPD_UNPLUG_INT_MASK (0x00000008) > > #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \ > > DP_DP_IRQ_HPD_INT_MASK | \ > > DP_DP_HPD_REPLUG_INT_MASK | \ > > > > DP_DP_HPD_UNPLUG_INT_MASK) > > > > Below are status bits from register REG_DP_DP_HPD_INT_STATUS > > > > #define DP_DP_HPD_STATE_STATUS_CONNECTED (0x40000000) > > #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000) > > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000) > > #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000) > > > > DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits > 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always. > > > > For DP, we want to enable all interrupts. > > So the programmed mask value is 0xF. We want to return 0x40000001 for > > connect and 8 for disconnect > > > > For eDP, we want to disable the connect and disconnect interrupts. So, > > the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | > DP_DP_HPD_REPLUG_INT_MASK) We want to return 0x40000000 (or > 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect > respectively. > > > > > > > > > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always. > > > > > > > >>> } > > > >>> > > > >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog > > > >>> *dp_catalog) > > > >>> -- > > > >>> 2.7.4 > > > >>> > > > >> > > > >> > > > >> -- > > > >> With best wishes > > > >> Dmitry > > > > > > > > Thank you, > > > > Sankeerth > > > > > > > > > -- > > > With best wishes > > > Dmitry > > > > Thank you, > > Sankeerth > > > > -- > With best wishes > Dmitry
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 3c16f95..1809ce2 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); - int isr = 0; + int isr, mask; isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, (isr & DP_DP_HPD_INT_MASK)); + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); - return isr; + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); } int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
The interrupt register will still reflect the connect and disconnect interrupt status without generating an actual HW interrupt. The controller driver should not handle those masked interrupts. Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)