| Message ID | 20200828072101.3781956-2-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: use FIELD_{GET|PREP} in subsystem | expand |
Hi Vinod, This change to use FIELD_PREP/GET looks good, the code is indeed a lot clearer, but ... > +#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) > +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) > +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) > +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) > +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) > +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr) ...our CI stopped on a compilation error with these macros. You will need the patch1 attached. Patch 9 also introduces conflicts with the multi-link code (fix in patch2), so would you mind if we go first with the multi-link code, or defer patch9 for now? Our validation for CML w/ RT700 is at: https://github.com/thesofproject/linux/pull/2404 We will also test on machines that are not in the CI farm and provide feedback. Thanks -Pierre
On 28-08-20, 11:03, Pierre-Louis Bossart wrote: > Hi Vinod, > This change to use FIELD_PREP/GET looks good, the code is indeed a lot > clearer, but ... > > > +#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) > > +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) > > +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) > > +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) > > +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) > > +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr) > > ...our CI stopped on a compilation error with these macros. You will need > the patch1 attached. Aha, that looks wrong indeed, somehow my test & build for aarch64 and build for x86 didnt flag this, neither this kbuild-bot! Have fixed it up now > > Patch 9 also introduces conflicts with the multi-link code (fix in patch2), > so would you mind if we go first with the multi-link code, or defer patch9 > for now? We can fix the series required while applying.. > > Our validation for CML w/ RT700 is at: > https://github.com/thesofproject/linux/pull/2404 > > We will also test on machines that are not in the CI farm and provide > feedback. > > Thanks > -Pierre > > >From 3aba5a7229c904664dacf1843f2e925585d4bd3e Mon Sep 17 00:00:00 2001 > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Date: Fri, 28 Aug 2020 10:45:22 -0500 > Subject: [PATCH 1/2] fixup! soundwire: define and use addr bit masks > > s/addr/adr > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > include/linux/soundwire/sdw.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index 892bf4718bc3..ebfabab63ec9 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -472,12 +472,12 @@ struct sdw_slave_id { > #define SDW_PART_ID_MASK GENMASK(23, 8) > #define SDW_CLASS_ID_MASK GENMASK(7, 0) > > -#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) > -#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) > -#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) > -#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) > -#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) > -#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr) > +#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, adr) > +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, adr) > +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, adr) > +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, adr) > +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, adr) > +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, adr) > > /** > * struct sdw_slave_intr_status - Slave interrupt status > -- > 2.25.1 > > >From f0280ed5dbe284df628e58c5afa1e61452cd5cb8 Mon Sep 17 00:00:00 2001 > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Date: Fri, 28 Aug 2020 10:51:52 -0500 > Subject: [PATCH 2/2] soundwire: intel: use FIELD_PREP macro > > Follow upstream changes. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/intel.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > index 566c7a99a5c1..20f111ce8a7a 100644 > --- a/drivers/soundwire/intel.c > +++ b/drivers/soundwire/intel.c > @@ -381,10 +381,11 @@ static int intel_link_power_up(struct sdw_intel *sdw) > link_control = intel_readl(shim, SDW_SHIM_LCTL); > > /* only power-up enabled links */ > - spa_mask = sdw->link_res->link_mask << > - SDW_REG_SHIFT(SDW_SHIM_LCTL_SPA_MASK); > - cpa_mask = sdw->link_res->link_mask << > - SDW_REG_SHIFT(SDW_SHIM_LCTL_CPA_MASK); > + spa_mask = FIELD_PREP(SDW_SHIM_LCTL_SPA_MASK, > + sdw->link_res->link_mask); > + cpa_mask = FIELD_PREP(SDW_SHIM_LCTL_CPA_MASK, > + sdw->link_res->link_mask); > + > > link_control |= spa_mask; > > @@ -555,10 +556,11 @@ static int intel_link_power_down(struct sdw_intel *sdw) > link_control = intel_readl(shim, SDW_SHIM_LCTL); > > /* only power-down enabled links */ > - spa_mask = (~sdw->link_res->link_mask) << > - SDW_REG_SHIFT(SDW_SHIM_LCTL_SPA_MASK); > - cpa_mask = sdw->link_res->link_mask << > - SDW_REG_SHIFT(SDW_SHIM_LCTL_CPA_MASK); > + spa_mask = FIELD_PREP(SDW_SHIM_LCTL_SPA_MASK, > + ~sdw->link_res->link_mask); > + > + cpa_mask = FIELD_PREP(SDW_SHIM_LCTL_CPA_MASK, > + sdw->link_res->link_mask); > > link_control &= spa_mask; > > -- > 2.25.1 >
>> Patch 9 also introduces conflicts with the multi-link code (fix in patch2), >> so would you mind if we go first with the multi-link code, or defer patch9 >> for now? > > We can fix the series required while applying.. It's not an issue with git, it's rather that this series is quite invasive and if you add it first it becomes quite hard to bisect/debug. In terms of risk management, it's safer to isolate contributions by groups and not mix them.
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 76052f12c9f7..c7462c333480 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -5,6 +5,7 @@ #define __SOUNDWIRE_H #include <linux/mod_devicetable.h> +#include <linux/bitfield.h> struct sdw_bus; struct sdw_slave; @@ -455,13 +456,19 @@ struct sdw_slave_id { * * The MIPI DisCo for SoundWire defines in addition the link_id as bits 51:48 */ - -#define SDW_DISCO_LINK_ID(adr) (((adr) >> 48) & GENMASK(3, 0)) -#define SDW_VERSION(adr) (((adr) >> 44) & GENMASK(3, 0)) -#define SDW_UNIQUE_ID(adr) (((adr) >> 40) & GENMASK(3, 0)) -#define SDW_MFG_ID(adr) (((adr) >> 24) & GENMASK(15, 0)) -#define SDW_PART_ID(adr) (((adr) >> 8) & GENMASK(15, 0)) -#define SDW_CLASS_ID(adr) ((adr) & GENMASK(7, 0)) +#define SDW_DISCO_LINK_ID_MASK GENMASK(51, 48) +#define SDW_VERSION_MASK GENMASK(47, 44) +#define SDW_UNIQUE_ID_MASK GENMASK(43, 40) +#define SDW_MFG_ID_MASK GENMASK(39, 24) +#define SDW_PART_ID_MASK GENMASK(23, 8) +#define SDW_CLASS_ID_MASK GENMASK(7, 0) + +#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr) /** * struct sdw_slave_intr_status - Slave interrupt status
Soundwire addr is a 52bit value encoding link, version, unique id, mfg id, part id and class id. Define bit masks for these and use FIELD_GET() to extract these fields. Signed-off-by: Vinod Koul <vkoul@kernel.org> --- include/linux/soundwire/sdw.h | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)