| Message ID | 20210914162825.v3.3.Ibf9b125434e3806b35f9079f6d8125578d76f138@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] drm/bridge: parade-ps8640: Improve logging at probing | expand |
Quoting Philip Chen (2021-09-14 16:28:45) > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 8d3e7a147170..dc349d729f5a 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e) [...] > + case DP_AUX_I2C_WRITE: > + case DP_AUX_I2C_READ: > + break; > + default: > + return -EINVAL; > + } > + > + ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET); > + if (ret) { > + dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret); Can we use DRM_DEV_ERROR()? > + return ret; > + } > + > + /* Assume it's good */ > + msg->reply = 0; > + > + addr_len[0] = msg->address & 0xff; > + addr_len[1] = (msg->address >> 8) & 0xff; > + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | > + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); It really feels like this out to be possible with some sort of cpu_to_le32() API. We're shoving msg->address into 3 bytes and then adding in the request and some length. So we could do something like: u32 addr_len; addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); if (len) addr_len |= FIELD_PREP(LEN_MASK, len - 1); else addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); cpu_to_le32s(&addr_len); regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len)); > + addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD : > + ((len - 1) & SWAUX_LENGTH_MASK); > + > + regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len, > + ARRAY_SIZE(addr_len)); > + > + if (len && (request == DP_AUX_NATIVE_WRITE || > + request == DP_AUX_I2C_WRITE)) { > + /* Write to the internal FIFO buffer */ > + for (i = 0; i < len; i++) { > + ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]); > + if (ret) { > + dev_err(dev, "failed to write WDATA: %d\n", DRM_DEV_ERROR? > + ret); > + return ret; > + } > + } > + } > + > + regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND); > + > + /* Zero delay loop because i2c transactions are slow already */ > + regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data, > + !(data & SWAUX_SEND), 0, 50 * 1000); > + > + regmap_read(map, PAGE0_SWAUX_STATUS, &data); > + if (ret) { > + dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret); DRM_DEV_ERROR? > + return ret; > + } > + > + switch (data & SWAUX_STATUS_MASK) { > + /* Ignore the DEFER cases as they are already handled in hardware */ > + case SWAUX_STATUS_NACK: > + case SWAUX_STATUS_I2C_NACK: > + /* > + * The programming guide is not clear about whether a I2C NACK > + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So > + * we handle both cases together. > + */ > + if (is_native_aux) > + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; > + else > + msg->reply |= DP_AUX_I2C_REPLY_NACK; > + > + len = data & SWAUX_M_MASK; > + return len; Why no 'return data & SWAUX_M_MASK;' and skip the assignment? > + case SWAUX_STATUS_ACKM: Move this up and add fallthrough? > + len = data & SWAUX_M_MASK; > + return len; > + case SWAUX_STATUS_INVALID: > + return -EOPNOTSUPP; > + case SWAUX_STATUS_TIMEOUT: > + return -ETIMEDOUT; > + } > + > + if (len && (request == DP_AUX_NATIVE_READ || > + request == DP_AUX_I2C_READ)) { > + /* Read from the internal FIFO buffer */ > + for (i = 0; i < len; i++) { > + ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data); > + buf[i] = data; Can drop a line ret = regmap_read(map, PAGE0_SWAUX_RDATA, buf + i); > + if (ret) { > + dev_err(dev, "failed to read RDATA: %d\n", > + ret); > + return ret; > + } > + } > + } > + > + return len; > +}
Hi On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-09-14 16:28:45) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 8d3e7a147170..dc349d729f5a 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e) > [...] > > + case DP_AUX_I2C_WRITE: > > + case DP_AUX_I2C_READ: > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET); > > + if (ret) { > > + dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret); > > Can we use DRM_DEV_ERROR()? Sure. > > > + return ret; > > + } > > + > > + /* Assume it's good */ > > + msg->reply = 0; > > + > > + addr_len[0] = msg->address & 0xff; > > + addr_len[1] = (msg->address >> 8) & 0xff; > > + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | > > + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); > > It really feels like this out to be possible with some sort of > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then > adding in the request and some length. So we could do something like: > > u32 addr_len; > > addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); > addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); > if (len) > addr_len |= FIELD_PREP(LEN_MASK, len - 1); > else > addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); > > cpu_to_le32s(&addr_len); > > regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len)); > Yes, thanks for the advice. Will add this change to v4. > > + addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD : > > + ((len - 1) & SWAUX_LENGTH_MASK); > > + > > + regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len, > > + ARRAY_SIZE(addr_len)); > > + > > + if (len && (request == DP_AUX_NATIVE_WRITE || > > + request == DP_AUX_I2C_WRITE)) { > > + /* Write to the internal FIFO buffer */ > > + for (i = 0; i < len; i++) { > > + ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]); > > + if (ret) { > > + dev_err(dev, "failed to write WDATA: %d\n", > > DRM_DEV_ERROR? Sure. > > > + ret); > > + return ret; > > + } > > + } > > + } > > + > > + regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND); > > + > > + /* Zero delay loop because i2c transactions are slow already */ > > + regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data, > > + !(data & SWAUX_SEND), 0, 50 * 1000); > > + > > + regmap_read(map, PAGE0_SWAUX_STATUS, &data); > > + if (ret) { > > + dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret); > > DRM_DEV_ERROR? Sure. > > > + return ret; > > + } > > + > > + switch (data & SWAUX_STATUS_MASK) { > > + /* Ignore the DEFER cases as they are already handled in hardware */ > > + case SWAUX_STATUS_NACK: > > + case SWAUX_STATUS_I2C_NACK: > > + /* > > + * The programming guide is not clear about whether a I2C NACK > > + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So > > + * we handle both cases together. > > + */ > > + if (is_native_aux) > > + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; > > + else > > + msg->reply |= DP_AUX_I2C_REPLY_NACK; > > + > > + len = data & SWAUX_M_MASK; > > + return len; > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment? I want to make it clear that we are returning the number of bytes that we have read/written instead of some error code. If you think it's not super helpful, I can just return data & SWAUX_M_MASK. > > > + case SWAUX_STATUS_ACKM: > > Move this up and add fallthrough? Thanks. Will add this change to v4. > > > + len = data & SWAUX_M_MASK; > > + return len; > > + case SWAUX_STATUS_INVALID: > > + return -EOPNOTSUPP; > > + case SWAUX_STATUS_TIMEOUT: > > + return -ETIMEDOUT; > > + } > > + > > + if (len && (request == DP_AUX_NATIVE_READ || > > + request == DP_AUX_I2C_READ)) { > > + /* Read from the internal FIFO buffer */ > > + for (i = 0; i < len; i++) { > > + ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data); > > + buf[i] = data; > > Can drop a line regmap_read() wants to read to an unsigned int, but buf is an u8 array. To be safe, I read RDATA to data and then assign data to buf[i]. As regmap_read() should always read 1 byte at a time, should I just do: regmap_read(map, PAGE0_SWAUX_RDATA, (unsigned int*)(buf + i)) > > ret = regmap_read(map, PAGE0_SWAUX_RDATA, buf + i); > > > + if (ret) { > > + dev_err(dev, "failed to read RDATA: %d\n", > > + ret); > > + return ret; > > + } > > + } > > + } > > + > > + return len; > > +}
On Wed, Sep 15, 2021 at 5:41 PM Philip Chen <philipchen@chromium.org> wrote: > As regmap_read() should always read 1 byte at a time, should I just do: > regmap_read(map, PAGE0_SWAUX_RDATA, (unsigned int*)(buf + i)) There is also regmap_bulk_read() if you need to read more data.
Hi Fabio On Wed, Sep 15, 2021 at 2:00 PM Fabio Estevam <festevam@gmail.com> wrote: > > On Wed, Sep 15, 2021 at 5:41 PM Philip Chen <philipchen@chromium.org> wrote: > > > As regmap_read() should always read 1 byte at a time, should I just do: > > regmap_read(map, PAGE0_SWAUX_RDATA, (unsigned int*)(buf + i)) > > There is also regmap_bulk_read() if you need to read more data. Thanks for the review. PAGE0_SWAUX_RDATA is a single-byte FIFO buffer. So I'll need to read one byte at a time cyclically.
Hi, On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-09-14 16:28:45) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 8d3e7a147170..dc349d729f5a 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e) > [...] > > + case DP_AUX_I2C_WRITE: > > + case DP_AUX_I2C_READ: > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET); > > + if (ret) { > > + dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret); > > Can we use DRM_DEV_ERROR()? I've never gotten clear guidance here. For instance, in some other review I suggested using the DRM wrapper and got told "no" [1]. ;-) The driver landed without the DRM_ERROR versions. I don't really care lots so it's fine with me to use use DRM_DEV_ERROR, I just wish I understood the rules... [1] https://lore.kernel.org/all/49db7ef3-fa53-a274-7c69-c2d840b13058@denx.de/ > > + return ret; > > + } > > + > > + /* Assume it's good */ > > + msg->reply = 0; > > + > > + addr_len[0] = msg->address & 0xff; > > + addr_len[1] = (msg->address >> 8) & 0xff; > > + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | > > + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); > > It really feels like this out to be possible with some sort of > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then > adding in the request and some length. So we could do something like: > > u32 addr_len; > > addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); > addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); > if (len) > addr_len |= FIELD_PREP(LEN_MASK, len - 1); > else > addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); > > cpu_to_le32s(&addr_len); > > regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len)); You're arguing that your version of the code is more efficient? Easier to understand? Something else? To me, Philip's initial version is crystal clear and easy to map to the bridge datasheet but I need to think more to confirm that your version is right. Thinking is hard and I like to avoid it when possible. In any case, it's definitely bikeshedding and I'll yield if everyone likes the other version better. ;-) > > + return ret; > > + } > > + > > + switch (data & SWAUX_STATUS_MASK) { > > + /* Ignore the DEFER cases as they are already handled in hardware */ > > + case SWAUX_STATUS_NACK: > > + case SWAUX_STATUS_I2C_NACK: > > + /* > > + * The programming guide is not clear about whether a I2C NACK > > + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So > > + * we handle both cases together. > > + */ > > + if (is_native_aux) > > + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; > > + else > > + msg->reply |= DP_AUX_I2C_REPLY_NACK; > > + > > + len = data & SWAUX_M_MASK; > > + return len; > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment? Actually, I think it's the "return" that's a bug, isn't it? If we're doing a "read" and we're returning a positive number of bytes then we need to actually _read_ them. Reading happens below, doesn't it? -Doug
Quoting Doug Anderson (2021-09-15 14:27:40) > Hi, > > On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Philip Chen (2021-09-14 16:28:45) > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > > index 8d3e7a147170..dc349d729f5a 100644 > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e) > > [...] > > > + case DP_AUX_I2C_WRITE: > > > + case DP_AUX_I2C_READ: > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET); > > > + if (ret) { > > > + dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret); > > > > Can we use DRM_DEV_ERROR()? > > I've never gotten clear guidance here. For instance, in some other > review I suggested using the DRM wrapper and got told "no" [1]. ;-) > The driver landed without the DRM_ERROR versions. I don't really care > lots so it's fine with me to use use DRM_DEV_ERROR, I just wish I > understood the rules... > > [1] https://lore.kernel.org/all/49db7ef3-fa53-a274-7c69-c2d840b13058@denx.de/ I think the rule is that the DRM specific printk stuff should be used so that they can be stuck into the drm logs. On chromeOS we also have a record of the drm logs that we can use to debug things, split away from the general kernel printk logs. So using DRM prints when there's a DRM device around is a good thing to do. > > > > > + return ret; > > > + } > > > + > > > + /* Assume it's good */ > > > + msg->reply = 0; > > > + > > > + addr_len[0] = msg->address & 0xff; > > > + addr_len[1] = (msg->address >> 8) & 0xff; > > > + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | > > > + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); > > > > It really feels like this out to be possible with some sort of > > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then > > adding in the request and some length. So we could do something like: > > > > u32 addr_len; > > > > addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); > > addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); > > if (len) > > addr_len |= FIELD_PREP(LEN_MASK, len - 1); > > else > > addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); > > > > cpu_to_le32s(&addr_len); > > > > regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len)); > > You're arguing that your version of the code is more efficient? Easier > to understand? Something else? To me, Philip's initial version is > crystal clear and easy to map to the bridge datasheet but I need to > think more to confirm that your version is right. Thinking is hard and > I like to avoid it when possible. > > In any case, it's definitely bikeshedding and I'll yield if everyone > likes the other version better. ;-) Yeah it's bikeshedding. I don't really care about this either but I find it easier to read when the assignment isn't wrapped across multiple lines. If the buffer approach is preferable then maybe use the address macros to clarify which register is being set? unsigned int base = PAGE0_SWAUX_ADDR_7_0; addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address; addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8; addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = msg->address >> 16; addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4; ... > > > > > + return ret; > > > + } > > > + > > > + switch (data & SWAUX_STATUS_MASK) { > > > + /* Ignore the DEFER cases as they are already handled in hardware */ > > > + case SWAUX_STATUS_NACK: > > > + case SWAUX_STATUS_I2C_NACK: > > > + /* > > > + * The programming guide is not clear about whether a I2C NACK > > > + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So > > > + * we handle both cases together. > > > + */ > > > + if (is_native_aux) > > > + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; > > > + else > > > + msg->reply |= DP_AUX_I2C_REPLY_NACK; > > > + > > > + len = data & SWAUX_M_MASK; > > > + return len; > > > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment? > > Actually, I think it's the "return" that's a bug, isn't it? If we're > doing a "read" and we're returning a positive number of bytes then we > need to actually _read_ them. Reading happens below, doesn't it? > Oh I missed that. We're still supposed to return data to upper layers on a NACKed read?
Hi, On Thu, Sep 16, 2021 at 1:15 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > + return ret; > > > > + } > > > > + > > > > + /* Assume it's good */ > > > > + msg->reply = 0; > > > > + > > > > + addr_len[0] = msg->address & 0xff; > > > > + addr_len[1] = (msg->address >> 8) & 0xff; > > > > + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | > > > > + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); > > > > > > It really feels like this out to be possible with some sort of > > > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then > > > adding in the request and some length. So we could do something like: > > > > > > u32 addr_len; > > > > > > addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); > > > addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); > > > if (len) > > > addr_len |= FIELD_PREP(LEN_MASK, len - 1); > > > else > > > addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); > > > > > > cpu_to_le32s(&addr_len); > > > > > > regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len)); > > > > You're arguing that your version of the code is more efficient? Easier > > to understand? Something else? To me, Philip's initial version is > > crystal clear and easy to map to the bridge datasheet but I need to > > think more to confirm that your version is right. Thinking is hard and > > I like to avoid it when possible. > > > > In any case, it's definitely bikeshedding and I'll yield if everyone > > likes the other version better. ;-) > > Yeah it's bikeshedding. I don't really care about this either but I find > it easier to read when the assignment isn't wrapped across multiple > lines. If the buffer approach is preferable then maybe use the address > macros to clarify which register is being set? > > unsigned int base = PAGE0_SWAUX_ADDR_7_0; > > addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address; > addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8; > addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = msg->address >> 16; > addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4; Sure, this looks nice to me. :-) > > > > + return ret; > > > > + } > > > > + > > > > + switch (data & SWAUX_STATUS_MASK) { > > > > + /* Ignore the DEFER cases as they are already handled in hardware */ > > > > + case SWAUX_STATUS_NACK: > > > > + case SWAUX_STATUS_I2C_NACK: > > > > + /* > > > > + * The programming guide is not clear about whether a I2C NACK > > > > + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So > > > > + * we handle both cases together. > > > > + */ > > > > + if (is_native_aux) > > > > + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; > > > > + else > > > > + msg->reply |= DP_AUX_I2C_REPLY_NACK; > > > > + > > > > + len = data & SWAUX_M_MASK; > > > > + return len; > > > > > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment? > > > > Actually, I think it's the "return" that's a bug, isn't it? If we're > > doing a "read" and we're returning a positive number of bytes then we > > need to actually _read_ them. Reading happens below, doesn't it? > > > > Oh I missed that. We're still supposed to return data to upper > layers on a NACKed read? It seems highly likely not to matter in practice, but: * The docs from parade clearly state that when a NAK is returned that the number of bytes transferred before the NAK will be provided to us. * The i2c interface specifies NAK not as a return code but as a bit in the "reply". That presumably is to let us return the number of bytes transferred before the NAK to the next level up. * If we're returning the number of bytes and it's a read then we'd better return the data too! It looks like we handled this OK in the TI bridge driver. From reading the TI docs we'll get both AUX_IRQ_STATUS_AUX_SHORT and AUX_IRQ_STATUS_NAT_I2C_FAIL in the case of a partial transfer and so we'll do the right thing. -Doug
Hi On Thu, Sep 16, 2021 at 1:31 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Sep 16, 2021 at 1:15 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > + return ret; > > > > > + } > > > > > + > > > > > + /* Assume it's good */ > > > > > + msg->reply = 0; > > > > > + > > > > > + addr_len[0] = msg->address & 0xff; > > > > > + addr_len[1] = (msg->address >> 8) & 0xff; > > > > > + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | > > > > > + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); > > > > > > > > It really feels like this out to be possible with some sort of > > > > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then > > > > adding in the request and some length. So we could do something like: > > > > > > > > u32 addr_len; > > > > > > > > addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); > > > > addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); > > > > if (len) > > > > addr_len |= FIELD_PREP(LEN_MASK, len - 1); > > > > else > > > > addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); > > > > > > > > cpu_to_le32s(&addr_len); > > > > > > > > regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len)); > > > > > > You're arguing that your version of the code is more efficient? Easier > > > to understand? Something else? To me, Philip's initial version is > > > crystal clear and easy to map to the bridge datasheet but I need to > > > think more to confirm that your version is right. Thinking is hard and > > > I like to avoid it when possible. > > > > > > In any case, it's definitely bikeshedding and I'll yield if everyone > > > likes the other version better. ;-) > > > > Yeah it's bikeshedding. I don't really care about this either but I find > > it easier to read when the assignment isn't wrapped across multiple > > lines. If the buffer approach is preferable then maybe use the address > > macros to clarify which register is being set? > > > > unsigned int base = PAGE0_SWAUX_ADDR_7_0; > > > > addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address; > > addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8; > > addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = msg->address >> 16; > > addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4; > > Sure, this looks nice to me. :-) Thanks. Implemented the fix in v4. PTAL. > > > > > > > + return ret; > > > > > + } > > > > > + > > > > > + switch (data & SWAUX_STATUS_MASK) { > > > > > + /* Ignore the DEFER cases as they are already handled in hardware */ > > > > > + case SWAUX_STATUS_NACK: > > > > > + case SWAUX_STATUS_I2C_NACK: > > > > > + /* > > > > > + * The programming guide is not clear about whether a I2C NACK > > > > > + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So > > > > > + * we handle both cases together. > > > > > + */ > > > > > + if (is_native_aux) > > > > > + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; > > > > > + else > > > > > + msg->reply |= DP_AUX_I2C_REPLY_NACK; > > > > > + > > > > > + len = data & SWAUX_M_MASK; > > > > > + return len; > > > > > > > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment? > > > > > > Actually, I think it's the "return" that's a bug, isn't it? If we're > > > doing a "read" and we're returning a positive number of bytes then we > > > need to actually _read_ them. Reading happens below, doesn't it? > > > > > > > Oh I missed that. We're still supposed to return data to upper > > layers on a NACKed read? > > It seems highly likely not to matter in practice, but: > > * The docs from parade clearly state that when a NAK is returned that > the number of bytes transferred before the NAK will be provided to us. > > * The i2c interface specifies NAK not as a return code but as a bit in > the "reply". That presumably is to let us return the number of bytes > transferred before the NAK to the next level up. > > * If we're returning the number of bytes and it's a read then we'd > better return the data too! > > It looks like we handled this OK in the TI bridge driver. From reading > the TI docs we'll get both AUX_IRQ_STATUS_AUX_SHORT and > AUX_IRQ_STATUS_NAT_I2C_FAIL in the case of a partial transfer and so > we'll do the right thing. Thanks for catching the bug. In v4, I made SWAUX_STATUS_I2C_NACK fall through to SWAUX_STATUS_ACKM and store the number of read/written bytes in len w/o returning immediately. PTAL. > > -Doug
Hi On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-09-14 16:28:45) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > > index 8d3e7a147170..dc349d729f5a 100644 > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e) > [...] > > + case DP_AUX_I2C_WRITE: > > + case DP_AUX_I2C_READ: > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET); > > + if (ret) { > > + dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret); > > Can we use DRM_DEV_ERROR()? Sure. Done in v4. PTAL. > > > + return ret; > > + } > > + > > + /* Assume it's good */ > > + msg->reply = 0; > > + > > + addr_len[0] = msg->address & 0xff; > > + addr_len[1] = (msg->address >> 8) & 0xff; > > + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | > > + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); > > It really feels like this out to be possible with some sort of > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then > adding in the request and some length. So we could do something like: > > u32 addr_len; > > addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address); > addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request); > if (len) > addr_len |= FIELD_PREP(LEN_MASK, len - 1); > else > addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD ); > > cpu_to_le32s(&addr_len); > > regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len)); > > > + addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD : > > + ((len - 1) & SWAUX_LENGTH_MASK); > > + > > + regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len, > > + ARRAY_SIZE(addr_len)); > > + > > + if (len && (request == DP_AUX_NATIVE_WRITE || > > + request == DP_AUX_I2C_WRITE)) { > > + /* Write to the internal FIFO buffer */ > > + for (i = 0; i < len; i++) { > > + ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]); > > + if (ret) { > > + dev_err(dev, "failed to write WDATA: %d\n", > > DRM_DEV_ERROR? Sure. Done in v4. PTAL. > > > + ret); > > + return ret; > > + } > > + } > > + } > > + > > + regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND); > > + > > + /* Zero delay loop because i2c transactions are slow already */ > > + regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data, > > + !(data & SWAUX_SEND), 0, 50 * 1000); > > + > > + regmap_read(map, PAGE0_SWAUX_STATUS, &data); > > + if (ret) { > > + dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret); > > DRM_DEV_ERROR? Sure. Done in v4. PTAL. > > > + return ret; > > + } > > + > > + switch (data & SWAUX_STATUS_MASK) { > > + /* Ignore the DEFER cases as they are already handled in hardware */ > > + case SWAUX_STATUS_NACK: > > + case SWAUX_STATUS_I2C_NACK: > > + /* > > + * The programming guide is not clear about whether a I2C NACK > > + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So > > + * we handle both cases together. > > + */ > > + if (is_native_aux) > > + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; > > + else > > + msg->reply |= DP_AUX_I2C_REPLY_NACK; > > + > > + len = data & SWAUX_M_MASK; > > + return len; > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment? > > > + case SWAUX_STATUS_ACKM: > > Move this up and add fallthrough? > > > + len = data & SWAUX_M_MASK; > > + return len; > > + case SWAUX_STATUS_INVALID: > > + return -EOPNOTSUPP; > > + case SWAUX_STATUS_TIMEOUT: > > + return -ETIMEDOUT; > > + } > > + > > + if (len && (request == DP_AUX_NATIVE_READ || > > + request == DP_AUX_I2C_READ)) { > > + /* Read from the internal FIFO buffer */ > > + for (i = 0; i < len; i++) { > > + ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data); > > + buf[i] = data; > > Can drop a line Sure. Done in v4. PTAL. > > ret = regmap_read(map, PAGE0_SWAUX_RDATA, buf + i); > > > + if (ret) { > > + dev_err(dev, "failed to read RDATA: %d\n", > > + ret); > > + return ret; > > + } > > + } > > + } > > + > > + return len; > > +}
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 8d3e7a147170..dc349d729f5a 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -13,11 +13,37 @@ #include <linux/regulator/consumer.h> #include <drm/drm_bridge.h> +#include <drm/drm_dp_helper.h> #include <drm/drm_mipi_dsi.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> #include <drm/drm_print.h> +#define PAGE0_AUXCH_CFG3 0x76 +#define AUXCH_CFG3_RESET 0xff +#define PAGE0_SWAUX_ADDR_7_0 0x7d +#define PAGE0_SWAUX_ADDR_15_8 0x7e +#define PAGE0_SWAUX_ADDR_23_16 0x7f +#define SWAUX_ADDR_19_16_MASK GENMASK(3, 0) +#define SWAUX_CMD_MASK GENMASK(7, 4) +#define PAGE0_SWAUX_LENGTH 0x80 +#define SWAUX_LENGTH_MASK GENMASK(3, 0) +#define SWAUX_NO_PAYLOAD BIT(7) +#define PAGE0_SWAUX_WDATA 0x81 +#define PAGE0_SWAUX_RDATA 0x82 +#define PAGE0_SWAUX_CTRL 0x83 +#define SWAUX_SEND BIT(0) +#define PAGE0_SWAUX_STATUS 0x84 +#define SWAUX_M_MASK GENMASK(4, 0) +#define SWAUX_STATUS_MASK GENMASK(7, 5) +#define SWAUX_STATUS_NACK (0x1 << 5) +#define SWAUX_STATUS_DEFER (0x2 << 5) +#define SWAUX_STATUS_ACKM (0x3 << 5) +#define SWAUX_STATUS_INVALID (0x4 << 5) +#define SWAUX_STATUS_I2C_NACK (0x5 << 5) +#define SWAUX_STATUS_I2C_DEFER (0x6 << 5) +#define SWAUX_STATUS_TIMEOUT (0x7 << 5) + #define PAGE2_GPIO_H 0xa7 #define PS_GPIO9 BIT(1) #define PAGE2_I2C_BYPASS 0xea @@ -68,6 +94,7 @@ enum ps8640_vdo_control { struct ps8640 { struct drm_bridge bridge; struct drm_bridge *panel_bridge; + struct drm_dp_aux aux; struct mipi_dsi_device *dsi; struct i2c_client *page[MAX_DEVS]; struct regmap *regmap[MAX_DEVS]; @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e) return container_of(e, struct ps8640, bridge); } +static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux) +{ + return container_of(aux, struct ps8640, aux); +} + +static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, + struct drm_dp_aux_msg *msg) +{ + struct ps8640 *ps_bridge = aux_to_ps8640(aux); + struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL]; + struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev; + + unsigned int len = msg->size; + unsigned int data; + int ret; + u8 request = msg->request & + ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE); + u8 *buf = msg->buffer; + u8 addr_len[PAGE0_SWAUX_LENGTH + 1 - PAGE0_SWAUX_ADDR_7_0]; + u8 i; + bool is_native_aux = false; + + if (len > DP_AUX_MAX_PAYLOAD_BYTES) + return -EINVAL; + + switch (request) { + case DP_AUX_NATIVE_WRITE: + case DP_AUX_NATIVE_READ: + is_native_aux = true; + fallthrough; + case DP_AUX_I2C_WRITE: + case DP_AUX_I2C_READ: + break; + default: + return -EINVAL; + } + + ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET); + if (ret) { + dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret); + return ret; + } + + /* Assume it's good */ + msg->reply = 0; + + addr_len[0] = msg->address & 0xff; + addr_len[1] = (msg->address >> 8) & 0xff; + addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) | + ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK); + addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD : + ((len - 1) & SWAUX_LENGTH_MASK); + + regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len, + ARRAY_SIZE(addr_len)); + + if (len && (request == DP_AUX_NATIVE_WRITE || + request == DP_AUX_I2C_WRITE)) { + /* Write to the internal FIFO buffer */ + for (i = 0; i < len; i++) { + ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]); + if (ret) { + dev_err(dev, "failed to write WDATA: %d\n", + ret); + return ret; + } + } + } + + regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND); + + /* Zero delay loop because i2c transactions are slow already */ + regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data, + !(data & SWAUX_SEND), 0, 50 * 1000); + + regmap_read(map, PAGE0_SWAUX_STATUS, &data); + if (ret) { + dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret); + return ret; + } + + switch (data & SWAUX_STATUS_MASK) { + /* Ignore the DEFER cases as they are already handled in hardware */ + case SWAUX_STATUS_NACK: + case SWAUX_STATUS_I2C_NACK: + /* + * The programming guide is not clear about whether a I2C NACK + * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So + * we handle both cases together. + */ + if (is_native_aux) + msg->reply |= DP_AUX_NATIVE_REPLY_NACK; + else + msg->reply |= DP_AUX_I2C_REPLY_NACK; + + len = data & SWAUX_M_MASK; + return len; + case SWAUX_STATUS_ACKM: + len = data & SWAUX_M_MASK; + return len; + case SWAUX_STATUS_INVALID: + return -EOPNOTSUPP; + case SWAUX_STATUS_TIMEOUT: + return -ETIMEDOUT; + } + + if (len && (request == DP_AUX_NATIVE_READ || + request == DP_AUX_I2C_READ)) { + /* Read from the internal FIFO buffer */ + for (i = 0; i < len; i++) { + ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data); + buf[i] = data; + if (ret) { + dev_err(dev, "failed to read RDATA: %d\n", + ret); + return ret; + } + } + } + + return len; +} + static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge, const enum ps8640_vdo_control ctrl) { @@ -286,18 +436,34 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, dsi->format = MIPI_DSI_FMT_RGB888; dsi->lanes = NUM_MIPI_LANES; ret = mipi_dsi_attach(dsi); - if (ret) + if (ret) { + dev_err(dev, "failed to attach dsi device: %d\n", ret); goto err_dsi_attach; + } + + ret = drm_dp_aux_register(&ps_bridge->aux); + if (ret) { + dev_err(dev, "failed to register DP AUX channel: %d\n", ret); + goto err_aux_register; + } /* Attach the panel-bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, &ps_bridge->bridge, flags); +err_aux_register: + mipi_dsi_detach(dsi); err_dsi_attach: mipi_dsi_device_unregister(dsi); return ret; } + +static void ps8640_bridge_detach(struct drm_bridge *bridge) +{ + drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux); +} + static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, struct drm_connector *connector) { @@ -334,6 +500,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, + .detach = ps8640_bridge_detach, .get_edid = ps8640_bridge_get_edid, .post_disable = ps8640_post_disable, .pre_enable = ps8640_pre_enable, @@ -423,6 +590,11 @@ static int ps8640_probe(struct i2c_client *client) i2c_set_clientdata(client, ps_bridge); + ps_bridge->aux.name = "parade-ps8640-aux"; + ps_bridge->aux.dev = dev; + ps_bridge->aux.transfer = ps8640_aux_transfer; + drm_dp_aux_init(&ps_bridge->aux); + drm_bridge_add(&ps_bridge->bridge); return 0;
Implement the first version of AUX support, which will be useful as we expand the driver to support varied use cases. Signed-off-by: Philip Chen <philipchen@chromium.org> --- Changes in v3: - Verify with HW and thus remove WARNING from the patch description - Fix the reg names to better match the manual - Fix aux_transfer function: - Fix the switch statement which handles aux request - Write the original (unstripped) aux request code to the register - Replace DRM_ERROR with dev_err - Remove goto and just return ret - Fix the switch statement which handles aux status - When reading returned data, read from RDATA instead of WDATA - Fix attach function: - Call mipi_dsi_detach() when aux_register fails Changes in v2: - Handle the case where an AUX transaction has no payload - Add a reg polling for p0.0x83 to confirm AUX cmd is issued and read data is returned - Replace regmap_noinc_read/write with looped regmap_read/write, as regmap_noinc_read/write doesn't read one byte at a time unless max_raw_read/write is set to 1. - Register/Unregister the AUX device explicitly when the bridge is attached/detached - Remove the use of runtime PM - Program AUX addr/cmd/len in a single regmap_bulk_write() - Add newlines for DRM_ERROR mesages drivers/gpu/drm/bridge/parade-ps8640.c | 174 ++++++++++++++++++++++++- 1 file changed, 173 insertions(+), 1 deletion(-)