• Douglas Anderson's avatar
    drm/msm/dp: Clean up handling of DP AUX interrupts · b20566cd
    Douglas Anderson authored
    The DP AUX interrupt handling was a bit of a mess.
    * There were two functions (one for "native" transfers and one for
      "i2c" transfers) that were quite similar. It was hard to say how
      many of the differences between the two functions were on purpose
      and how many of them were just an accident of how they were coded.
    * Each function sometimes used "else if" to test for error bits and
      sometimes didn't and again it was hard to say if this was on purpose
      or just an accident.
    * The two functions wouldn't notice whether "unknown" bits were
      set. For instance, there seems to be a bit "DP_INTR_PLL_UNLOCKED"
      and if it was set there would be no indication.
    * The two functions wouldn't notice if more than one error was set.
    
    Let's fix this by being more consistent / explicit about what we're
    doing.
    
    By design this could cause different handling for AUX transfers,
    though I'm not actually aware of any bug fixed as a result of
    this patch (this patch was created because we simply noticed how odd
    the old code was by code inspection). Specific notes here:
    1. In the old native transfer case if we got "done + wrong address"
       we'd ignore the "wrong address" (because of the "else if"). Now we
       won't.
    2. In the old native transfer case if we got "done + timeout" we'd
       ignore the "timeout" (because of the "else if"). Now we won't.
    3. In the old native transfer case we'd see "nack_defer" and translate
       it to the error number for "nack". This differed from the i2c
       transfer case where "nack_defer" was given the error number for
       "nack_defer". This 100% can't matter because the only user of this
       error number treats "nack defer" the same as "nack", so it's clear
       that the difference between the "native" and "i2c" was pointless
       here.
    4. In the old i2c transfer case if we got "done" plus any error
       besides "nack" or "defer" then we'd ignore the error. Now we don't.
    5. If there is more than one error signaled by the hardware it's
       possible that we'll report a different one than we used to. I don't
       know if this matters. If someone is aware of a case this matters we
       should document it and change the code to make it explicit.
    6. One quirk we keep (I don't know if this is important) is that in
       the i2c transfer case if we see "done + defer" we report that as a
       "nack". That seemed too intentional in the old code to just drop.
    
    After this change we will add extra logging, including:
    * A warning if we see more than one error bit set.
    * A warning if we see an unexpected interrupt.
    * A warning if we get an AUX transfer interrupt when shouldn't.
    
    It actually turns out that as a result of this change then at boot we
    sometimes see an error:
      [drm:dp_aux_isr] *ERROR* Unexpected DP AUX IRQ 0x01000000 when not busy
    That means that, during init, we are seeing DP_INTR_PLL_UNLOCKED. For
    now I'm going to say that leaving this error reported in the logs is
    OK-ish and hopefully it will encourage someone to track down what's
    going on at init time.
    
    One last note here is that this change renames one of the interrupt
    bits. The bit named "i2c done" clearly was used for native transfers
    being done too, so I renamed it to indicate this.
    Signed-off-by: default avatarDouglas Anderson <dianders@chromium.org>
    Tested-by: default avatarKuogee Hsieh <quic_khsieh@quicinc.com>
    Reviewed-by: default avatarKuogee Hsieh <quic_khsieh@quicinc.com>
    Patchwork: https://patchwork.freedesktop.org/patch/520658/
    Link: https://lore.kernel.org/r/20230126170745.v2.1.I90ffed3ddd21e818ae534f820cb4d6d8638859ab@changeidSigned-off-by: default avatarDmitry Baryshkov <dmitry.baryshkov@linaro.org>
    b20566cd
dp_catalog.c 33.1 KB