From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Update the data structures for ucsi_connector_capability and
ucsi_connector_status to UCSIv3.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Connector status has several unaligned bitfields (16-bit) that result in
difficult to maintain macros. It may be better if we simply re-define
these structs as u8[] and add bit range macros to access and cast these
values.
i.e.
struct ucsi_connector_status {
u8 raw_data[18];
...
\#define UCSI_CONSTAT_CONNECTOR_STATUS FIELD(u16, 15, 0)
\#define UCSI_CONSTAT_BCD_PD_VER_OPER_MODE FIELD(u16, 85, 70)
}
GET_UCSI_FIELD(con->status, UCSI_CONSTAT_CONNECTOR_STATUS);
SET_UCSI_FIELD(con->status, UCSI_CONSTAT_CONNECTOR_STATUS, 0);
I didn't find a clear example of an existing mechanism to do this. Would
love some pointers here if it already exists and some feedback from the
maintainer if this is a direction you want to go.
(no changes since v1)
drivers/usb/typec/ucsi/ucsi.h | 50 ++++++++++++++++++++++++++++++++---
1 file changed, 46 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index bec920fa6b8a..94b373378f63 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -3,6 +3,7 @@
#ifndef __DRIVER_USB_TYPEC_UCSI_H
#define __DRIVER_USB_TYPEC_UCSI_H
+#include <asm-generic/unaligned.h>
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/power_supply.h>
@@ -214,9 +215,29 @@ struct ucsi_connector_capability {
#define UCSI_CONCAP_OPMODE_USB2 BIT(5)
#define UCSI_CONCAP_OPMODE_USB3 BIT(6)
#define UCSI_CONCAP_OPMODE_ALT_MODE BIT(7)
- u8 flags;
+ u32 flags;
#define UCSI_CONCAP_FLAG_PROVIDER BIT(0)
#define UCSI_CONCAP_FLAG_CONSUMER BIT(1)
+#define UCSI_CONCAP_FLAG_SWAP_TO_DFP BIT(2)
+#define UCSI_CONCAP_FLAG_SWAP_TO_UFP BIT(3)
+#define UCSI_CONCAP_FLAG_SWAP_TO_SRC BIT(4)
+#define UCSI_CONCAP_FLAG_SWAP_TO_SINK BIT(5)
+#define UCSI_CONCAP_FLAG_EX_OP_MODE(_f_) \
+ (((_f_) & GENMASK(13, 6)) >> 6)
+#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN2 BIT(0)
+#define UCSI_CONCAP_EX_OP_MODE_EPR_SRC BIT(1)
+#define UCSI_CONCAP_EX_OP_MODE_EPR_SINK BIT(2)
+#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN3 BIT(3)
+#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN4 BIT(4)
+#define UCSI_CONCAP_FLAG_MISC_CAPS(_f_) \
+ (((_f_) & GENMASK(17, 14)) >> 14)
+#define UCSI_CONCAP_MISC_CAP_FW_UPDATE BIT(0)
+#define UCSI_CONCAP_MISC_CAP_SECURITY BIT(1)
+#define UCSI_CONCAP_FLAG_REV_CURR_PROT_SUPPORT BIT(18)
+#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) \
+ (((_f_) & GENMASK(20, 19)) >> 19)
+#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \
+ (UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) << 8)
} __packed;
struct ucsi_altmode {
@@ -276,15 +297,36 @@ struct ucsi_connector_status {
#define UCSI_CONSTAT_PARTNER_TYPE_DEBUG 5
#define UCSI_CONSTAT_PARTNER_TYPE_AUDIO 6
u32 request_data_obj;
- u8 pwr_status;
-#define UCSI_CONSTAT_BC_STATUS(_p_) ((_p_) & GENMASK(2, 0))
+
+ u8 pwr_status[3];
+#define UCSI_CONSTAT_BC_STATUS(_p_) ((_p_[0]) & GENMASK(1, 0))
#define UCSI_CONSTAT_BC_NOT_CHARGING 0
#define UCSI_CONSTAT_BC_NOMINAL_CHARGING 1
#define UCSI_CONSTAT_BC_SLOW_CHARGING 2
#define UCSI_CONSTAT_BC_TRICKLE_CHARGING 3
-#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_) (((_p_) & GENMASK(6, 3)) >> 3)
+#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_) (((_p_[0]) & GENMASK(5, 2)) >> 2)
#define UCSI_CONSTAT_CAP_PWR_LOWERED 0
#define UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT 1
+#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_) \
+ ((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
+#define UCSI_CONSTAT_ORIENTATION(_p_) (((_p_[2]) & GENMASK(6, 6)) >> 6)
+#define UCSI_CONSTAT_ORIENTATION_DIRECT 0
+#define UCSI_CONSTAT_ORIENTATION_FLIPPED 1
+#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_) (((_p_[2]) & GENMASK(7, 7)) >> 7)
+#define UCSI_CONSTAT_SINK_PATH_DISABLED 0
+#define UCSI_CONSTAT_SINK_PATH_ENABLED 1
+ u8 pwr_readings[9];
+#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_) ((_p_[0]) & 0x1)
+#define UCSI_CONSTAT_PWR_READING_VALID(_p_) (((_p_[0]) & GENMASK(1, 1)) >> 1)
+#define UCSI_CONSTAT_CURRENT_SCALE(_p_) (((_p_[0]) & GENMASK(4, 2)) >> 2)
+#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
+ ((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
+#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
+ ((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
+#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
+ ((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
+#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
+ ((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
} __packed;
/* -------------------------------------------------------------------------- */
--
2.43.0.429.g432eaa2c6b-googHi Abhishek, > +#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \ > + (UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) << 8) Can you replace this with a common HEADER_REV_AS_BCD macro that can be used for both GET_CONNECTOR_CAPABILTY and GET_CABLE_PROPERTY? Also, the USB PD major revision value in the message header is one less than the revision (PD Spec section 6.2.1.1.5). So, we need to add 1 before shifting. Thanks, Jameson
On Thu, Feb 8, 2024 at 11:48 AM Jameson Thies <jthies@google.com> wrote: > > Hi Abhishek, > > > +#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \ > > + (UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) << 8) > > Can you replace this with a common HEADER_REV_AS_BCD macro that can be > used for both GET_CONNECTOR_CAPABILTY and GET_CABLE_PROPERTY? > Also, the USB PD major revision value in the message header is one less than the > revision (PD Spec section 6.2.1.1.5). So, we need to add 1 before shifting. Jameson and I talked briefly and I discovered that PD assigns the following values for the major rev: * 00 = 1 * 01 = 2 * 10 = 3 * 11 = Reserved/Invalid From PD 3 onwards, there's a new Get_Revision message that can be queried from UCSI using GET_PD_MESSAGE. In future patches adding support for Discover Identity (also using GET_PD_MESSAGE), we will need to check this major revision to see whether we should also query Get Revision. Since this code is incorrect, I will send up a PATCH v4 with the correct BCD version as Jameson suggested. I'll also fix up some of the minor nits in that patch series. > > Thanks, > Jameson
On Wed, Jan 24, 2024 at 04:44:53PM -0800, Abhishek Pandit-Subedi wrote: > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index bec920fa6b8a..94b373378f63 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -3,6 +3,7 @@ > #ifndef __DRIVER_USB_TYPEC_UCSI_H > #define __DRIVER_USB_TYPEC_UCSI_H > > +#include <asm-generic/unaligned.h> Do you really need to include a asm/ include file? This feels very wrong. It's also in the wrong place, AND why "asm-generic"? That also feels wrong. thanks, greg k-h
On Thu, Jan 25, 2024 at 3:03 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 24, 2024 at 04:44:53PM -0800, Abhishek Pandit-Subedi wrote: > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > index bec920fa6b8a..94b373378f63 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.h > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > @@ -3,6 +3,7 @@ > > #ifndef __DRIVER_USB_TYPEC_UCSI_H > > #define __DRIVER_USB_TYPEC_UCSI_H > > > > +#include <asm-generic/unaligned.h> > > Do you really need to include a asm/ include file? This feels very > wrong. I didn't see any header in include/linux that already had these unaligned access functions so I opted to include asm-generic/unaligned.h. Is there a reason not to use an asm/ include file? > > It's also in the wrong place, AND why "asm-generic"? That also feels > wrong. asm-generic is definitely wrong; I misunderstood how these headers are supposed to be used (should be just asm/unaligned.h). For ordering, I took a look at some other files and it looks like <asm/...> goes below the <linux/...> includes. This also probably deserves documenting in the style guide. > > thanks, > > greg k-h
On Thu, Jan 25, 2024 at 04:21:47PM -0800, Abhishek Pandit-Subedi wrote: > On Thu, Jan 25, 2024 at 3:03 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Jan 24, 2024 at 04:44:53PM -0800, Abhishek Pandit-Subedi wrote: > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > > index bec920fa6b8a..94b373378f63 100644 > > > --- a/drivers/usb/typec/ucsi/ucsi.h > > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > > @@ -3,6 +3,7 @@ > > > #ifndef __DRIVER_USB_TYPEC_UCSI_H > > > #define __DRIVER_USB_TYPEC_UCSI_H > > > > > > +#include <asm-generic/unaligned.h> > > > > Do you really need to include a asm/ include file? This feels very > > wrong. > > I didn't see any header in include/linux that already had these > unaligned access functions so I opted to include > asm-generic/unaligned.h. Is there a reason not to use an asm/ include > file? Yes, you should never need to include a asm/ file, unless you are arch-specific code. But the big issue is that you don't really need this, right? > > It's also in the wrong place, AND why "asm-generic"? That also feels > > wrong. > > asm-generic is definitely wrong; I misunderstood how these headers are > supposed to be used (should be just asm/unaligned.h). Why? What are you requiring this .h file for? > For ordering, I took a look at some other files and it looks like > <asm/...> goes below the <linux/...> includes. This also probably > deserves documenting in the style guide. It is somewhere, checkpatch should complain about it. thanks, greg k-h
On Thu, Jan 25, 2024 at 5:50 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Jan 25, 2024 at 04:21:47PM -0800, Abhishek Pandit-Subedi wrote: > > On Thu, Jan 25, 2024 at 3:03 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Jan 24, 2024 at 04:44:53PM -0800, Abhishek Pandit-Subedi wrote: > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > > > index bec920fa6b8a..94b373378f63 100644 > > > > --- a/drivers/usb/typec/ucsi/ucsi.h > > > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > > > @@ -3,6 +3,7 @@ > > > > #ifndef __DRIVER_USB_TYPEC_UCSI_H > > > > #define __DRIVER_USB_TYPEC_UCSI_H > > > > > > > > +#include <asm-generic/unaligned.h> > > > > > > Do you really need to include a asm/ include file? This feels very > > > wrong. > > > > I didn't see any header in include/linux that already had these > > unaligned access functions so I opted to include > > asm-generic/unaligned.h. Is there a reason not to use an asm/ include > > file? > > Yes, you should never need to include a asm/ file, unless you are > arch-specific code. > > But the big issue is that you don't really need this, right? The UCSI struct definitions have lots of unaligned bit ranges (and I will be refactoring <linux/bitfield.h> to support this but that's coming later). As an example, the GET_CONNECTOR_STATUS data structure has unaligned fields from bit 88-145. Rather than define my own macro, it was suggested I use the get_unaligned_le32 functions (see https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5195032/3..4/drivers/usb/typec/ucsi/ucsi.h#b183). I did a quick ripgrep on the drivers folder -- it looks like the "You should never need to include a asm/ file unless you are arch specific" isn't being followed for this file: $ (cd drivers && rg -g '*.h' "unaligned\.h" -l) | wc -l 22 The unaligned access functions (get_unaligned_le16, get_unaligned_le32, etc) are really useful and widely used. Maybe they SHOULD be exposed from a <linux/unaligned.h> since they are so useful? I can send a follow-on patch that creates <linux/unaligned.h> (that simply just includes <asm/unaligned.h>) and moves all includes of <asm/unaligned.h> outside of "arch" to the linux header instead (this will also create a checkpatch warning now as you are expecting). > > > > It's also in the wrong place, AND why "asm-generic"? That also feels > > > wrong. > > > > asm-generic is definitely wrong; I misunderstood how these headers are > > supposed to be used (should be just asm/unaligned.h). > > Why? What are you requiring this .h file for? > > > For ordering, I took a look at some other files and it looks like > > <asm/...> goes below the <linux/...> includes. This also probably > > deserves documenting in the style guide. > > It is somewhere, checkpatch should complain about it. Checkpatch only complains if there exists a <linux/foo.h> and you call <asm/foo.h>: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v6.8-rc1#n5882 That's the only relevant check I found when searched for "asm" in checkpatch.pl > > thanks, > > greg k-h
On Fri, Jan 26, 2024 at 10:08:16AM -0800, Abhishek Pandit-Subedi wrote: > On Thu, Jan 25, 2024 at 5:50 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Jan 25, 2024 at 04:21:47PM -0800, Abhishek Pandit-Subedi wrote: > > > On Thu, Jan 25, 2024 at 3:03 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Wed, Jan 24, 2024 at 04:44:53PM -0800, Abhishek Pandit-Subedi wrote: > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > > > > index bec920fa6b8a..94b373378f63 100644 > > > > > --- a/drivers/usb/typec/ucsi/ucsi.h > > > > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > > > > @@ -3,6 +3,7 @@ > > > > > #ifndef __DRIVER_USB_TYPEC_UCSI_H > > > > > #define __DRIVER_USB_TYPEC_UCSI_H > > > > > > > > > > +#include <asm-generic/unaligned.h> > > > > > > > > Do you really need to include a asm/ include file? This feels very > > > > wrong. > > > > > > I didn't see any header in include/linux that already had these > > > unaligned access functions so I opted to include > > > asm-generic/unaligned.h. Is there a reason not to use an asm/ include > > > file? > > > > Yes, you should never need to include a asm/ file, unless you are > > arch-specific code. > > > > But the big issue is that you don't really need this, right? > > The UCSI struct definitions have lots of unaligned bit ranges (and I > will be refactoring <linux/bitfield.h> to support this but that's > coming later). As an example, the GET_CONNECTOR_STATUS data structure > has unaligned fields from bit 88-145. > Rather than define my own macro, it was suggested I use the > get_unaligned_le32 functions (see > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5195032/3..4/drivers/usb/typec/ucsi/ucsi.h#b183). > > I did a quick ripgrep on the drivers folder -- it looks like the "You > should never need to include a asm/ file unless you are arch specific" > isn't being followed for this file: > $ (cd drivers && rg -g '*.h' "unaligned\.h" -l) | wc -l > 22 > > The unaligned access functions (get_unaligned_le16, > get_unaligned_le32, etc) are really useful and widely used. Maybe they > SHOULD be exposed from a <linux/unaligned.h> since they are so useful? > I can send a follow-on patch that creates <linux/unaligned.h> (that > simply just includes <asm/unaligned.h>) and moves all includes of > <asm/unaligned.h> outside of "arch" to the linux header instead (this > will also create a checkpatch warning now as you are expecting). This is being worked on, see: https://lore.kernel.org/r/20231212024920.GG1674809@ZenIV thanks, greg k-h
On Fri, Jan 26, 2024 at 10:30 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Jan 26, 2024 at 10:08:16AM -0800, Abhishek Pandit-Subedi wrote: > > On Thu, Jan 25, 2024 at 5:50 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, Jan 25, 2024 at 04:21:47PM -0800, Abhishek Pandit-Subedi wrote: > > > > On Thu, Jan 25, 2024 at 3:03 PM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Wed, Jan 24, 2024 at 04:44:53PM -0800, Abhishek Pandit-Subedi wrote: > > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > > > > > index bec920fa6b8a..94b373378f63 100644 > > > > > > --- a/drivers/usb/typec/ucsi/ucsi.h > > > > > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > > > > > @@ -3,6 +3,7 @@ > > > > > > #ifndef __DRIVER_USB_TYPEC_UCSI_H > > > > > > #define __DRIVER_USB_TYPEC_UCSI_H > > > > > > > > > > > > +#include <asm-generic/unaligned.h> > > > > > > > > > > Do you really need to include a asm/ include file? This feels very > > > > > wrong. > > > > > > > > I didn't see any header in include/linux that already had these > > > > unaligned access functions so I opted to include > > > > asm-generic/unaligned.h. Is there a reason not to use an asm/ include > > > > file? > > > > > > Yes, you should never need to include a asm/ file, unless you are > > > arch-specific code. > > > > > > But the big issue is that you don't really need this, right? > > > > The UCSI struct definitions have lots of unaligned bit ranges (and I > > will be refactoring <linux/bitfield.h> to support this but that's > > coming later). As an example, the GET_CONNECTOR_STATUS data structure > > has unaligned fields from bit 88-145. > > Rather than define my own macro, it was suggested I use the > > get_unaligned_le32 functions (see > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5195032/3..4/drivers/usb/typec/ucsi/ucsi.h#b183). > > > > I did a quick ripgrep on the drivers folder -- it looks like the "You > > should never need to include a asm/ file unless you are arch specific" > > isn't being followed for this file: > > $ (cd drivers && rg -g '*.h' "unaligned\.h" -l) | wc -l > > 22 > > > > The unaligned access functions (get_unaligned_le16, > > get_unaligned_le32, etc) are really useful and widely used. Maybe they > > SHOULD be exposed from a <linux/unaligned.h> since they are so useful? > > I can send a follow-on patch that creates <linux/unaligned.h> (that > > simply just includes <asm/unaligned.h>) and moves all includes of > > <asm/unaligned.h> outside of "arch" to the linux header instead (this > > will also create a checkpatch warning now as you are expecting). > > This is being worked on, see: > https://lore.kernel.org/r/20231212024920.GG1674809@ZenIV > > thanks, > > greg k-h Thanks, I see the move here: https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=headers.unaligned&id=3169da8e80dfca2bcbfb6e998e2f36bcdcd5895a I'm not sure how the logistics of this is going to work but I assume it's ok to merge with <asm/unaligned.h> for now and let the later merge from viro fix this? (+Viro as FYI) I'll send up Patch 3 of this series with the fixes discussed (use asm/unaligned.h and reorder includes) Abhishek
© 2016 - 2025 Red Hat, Inc.