On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
<[email protected]> 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 <[email protected]>
> ---
> 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
>
--
With best wishes
Dmitry
Hi Dmitry,
> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
--
With best wishes
Dmitry
Hi Dmitry,
> On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > Hi Dmitry,
> >
> >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> >> <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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