| Message ID | 20210126083746.3238-2-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: clear bus clash/parity interrupt before the mask is enabled | expand |
On 26-01-21, 16:37, Bard Liao wrote: > The SoundWire specification allows a Slave device to report a bus clash > with the in-band interrupt mechanism when it detects a conflict while > driving a bitSlot it owns. This can be a symptom of an electrical conflict > or a programming error, and it's vital to detect reliably. > > Unfortunately, on some platforms, bus clashes are randomly reported by > Slave devices after a bus reset, with an interrupt status set even before > the bus clash interrupt is enabled. These initial spurious interrupts are > not relevant and should optionally be filtered out, while leaving the > interrupt mechanism enabled to detect 'true' issues. > > This patch suggests the addition of a Master level quirk to discard such > interrupts. The quirk should in theory have been added at the Slave level, > but since the problem was detected with different generations of Slave > devices it's hard to point to a specific IP. The problem might also be > board-dependent and hence dealing with a Master quirk is simpler. > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reviewed-by: Rander Wang <rander.wang@linux.intel.com> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/bus.c | 10 ++++++++++ > include/linux/soundwire/sdw.h | 4 ++++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index 6e1c988f3845..d394905936e4 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -1240,6 +1240,7 @@ static int sdw_slave_set_frequency(struct sdw_slave *slave) > static int sdw_initialize_slave(struct sdw_slave *slave) > { > struct sdw_slave_prop *prop = &slave->prop; > + int status; > int ret; > u8 val; > > @@ -1247,6 +1248,15 @@ static int sdw_initialize_slave(struct sdw_slave *slave) > if (ret < 0) > return ret; > > + if (slave->bus->prop.quirks & SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH) { > + /* Clear bus clash interrupt before enabling interrupt mask */ > + status = sdw_read_no_pm(slave, SDW_SCP_INT1); > + if (status & SDW_SCP_INT1_BUS_CLASH) { > + dev_warn(&slave->dev, "Bus clash detected before INT mask is enabled\n"); > + sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_BUS_CLASH); > + } > + } > + > /* > * Set SCP_INT1_MASK register, typically bus clash and > * implementation-defined interrupt mask. The Parity detection > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index f0b01b728640..a2766c3b603d 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -405,6 +405,7 @@ struct sdw_slave_prop { > * command > * @mclk_freq: clock reference passed to SoundWire Master, in Hz. > * @hw_disabled: if true, the Master is not functional, typically due to pin-mux > + * @quirks: bitmask identifying optional behavior beyond the scope of the MIPI specification > */ > struct sdw_master_prop { > u32 revision; > @@ -421,8 +422,11 @@ struct sdw_master_prop { > u32 err_threshold; > u32 mclk_freq; > bool hw_disabled; > + u32 quirks; Can we do u64 here please.. I dont know where we would end up.. but would hate if we start running out of space .. > }; > > +#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH BIT(0) > + > int sdw_master_read_prop(struct sdw_bus *bus); > int sdw_slave_read_prop(struct sdw_slave *slave); > > -- > 2.17.1
On 01-02-21, 15:58, Vinod Koul wrote: > On 26-01-21, 16:37, Bard Liao wrote: > > struct sdw_master_prop { > > u32 revision; > > @@ -421,8 +422,11 @@ struct sdw_master_prop { > > u32 err_threshold; > > u32 mclk_freq; > > bool hw_disabled; > > + u32 quirks; > > Can we do u64 here please.. I dont know where we would end up.. but > would hate if we start running out of space .. Also, is the sdw_master_prop right place for a 'quirk' property. I think we can use sdw_master_device or sdw_bus as this seems like a bus quirk..? -- ~Vinod
On 2/1/21 4:38 AM, Vinod Koul wrote: > On 01-02-21, 15:58, Vinod Koul wrote: >> On 26-01-21, 16:37, Bard Liao wrote: > >>> struct sdw_master_prop { >>> u32 revision; >>> @@ -421,8 +422,11 @@ struct sdw_master_prop { >>> u32 err_threshold; >>> u32 mclk_freq; >>> bool hw_disabled; >>> + u32 quirks; >> >> Can we do u64 here please.. I dont know where we would end up.. but >> would hate if we start running out of space .. No objection. > Also, is the sdw_master_prop right place for a 'quirk' property. I think > we can use sdw_master_device or sdw_bus as this seems like a bus > quirk..? It's already part of sdw_bus struct sdw_bus { struct device *dev; struct sdw_master_device *md; unsigned int link_id; int id; struct list_head slaves; DECLARE_BITMAP(assigned, SDW_MAX_DEVICES); struct mutex bus_lock; struct mutex msg_lock; int (*compute_params)(struct sdw_bus *bus); const struct sdw_master_ops *ops; const struct sdw_master_port_ops *port_ops; struct sdw_bus_params params; struct sdw_master_prop prop; The quirks could be set by a firmware property, and it seems logical to add them at the same place where we already have properties defined in firmware, no? That way all the standard, vendor-specific and quirks are read or added in the same place. the sdw_master_device isn't a good place for quirks IMHO, it's a very shallow software-only layer without any existing ties to the hardware definition.
On 01-02-21, 10:18, Pierre-Louis Bossart wrote: > On 2/1/21 4:38 AM, Vinod Koul wrote: > > On 01-02-21, 15:58, Vinod Koul wrote: > > > On 26-01-21, 16:37, Bard Liao wrote: > > > > > > struct sdw_master_prop { > > > > u32 revision; > > > > @@ -421,8 +422,11 @@ struct sdw_master_prop { > > > > u32 err_threshold; > > > > u32 mclk_freq; > > > > bool hw_disabled; > > > > + u32 quirks; > > > > > > Can we do u64 here please.. I dont know where we would end up.. but > > > would hate if we start running out of space .. > No objection. > > > Also, is the sdw_master_prop right place for a 'quirk' property. I think > > we can use sdw_master_device or sdw_bus as this seems like a bus > > quirk..? > > It's already part of sdw_bus Right, but the point is that the properties were mostly derived from DiSco, so am bit skeptical about it adding it there.. > struct sdw_bus { > struct device *dev; > struct sdw_master_device *md; > unsigned int link_id; > int id; > struct list_head slaves; > DECLARE_BITMAP(assigned, SDW_MAX_DEVICES); > struct mutex bus_lock; > struct mutex msg_lock; > int (*compute_params)(struct sdw_bus *bus); > const struct sdw_master_ops *ops; > const struct sdw_master_port_ops *port_ops; > struct sdw_bus_params params; > struct sdw_master_prop prop; > > The quirks could be set by a firmware property, and it seems logical to add > them at the same place where we already have properties defined in firmware, > no? That way all the standard, vendor-specific and quirks are read or added > in the same place. Or they could be set by driver as well based on device id, compatible and so on.. It maybe fw property as well, so limiting to property may not be best idea IMO. > the sdw_master_device isn't a good place for quirks IMHO, it's a very > shallow software-only layer without any existing ties to the hardware > definition. This one I would agree.
On 2/1/21 10:39 PM, Vinod Koul wrote: > On 01-02-21, 10:18, Pierre-Louis Bossart wrote: >> On 2/1/21 4:38 AM, Vinod Koul wrote: >>> On 01-02-21, 15:58, Vinod Koul wrote: >>>> On 26-01-21, 16:37, Bard Liao wrote: >>> >>>>> struct sdw_master_prop { >>>>> u32 revision; >>>>> @@ -421,8 +422,11 @@ struct sdw_master_prop { >>>>> u32 err_threshold; >>>>> u32 mclk_freq; >>>>> bool hw_disabled; >>>>> + u32 quirks; >>>> >>>> Can we do u64 here please.. I dont know where we would end up.. but >>>> would hate if we start running out of space .. >> No objection. >> >>> Also, is the sdw_master_prop right place for a 'quirk' property. I think >>> we can use sdw_master_device or sdw_bus as this seems like a bus >>> quirk..? >> >> It's already part of sdw_bus > > Right, but the point is that the properties were mostly derived from > DiSco, so am bit skeptical about it adding it there.. Oh, I am planning to contribute such quirks as MIPI DisCo properties for the next revision of the document (along with a capability to disable a link). This was not intended to remain Linux- or Intel-specific. >> struct sdw_bus { >> struct device *dev; >> struct sdw_master_device *md; >> unsigned int link_id; >> int id; >> struct list_head slaves; >> DECLARE_BITMAP(assigned, SDW_MAX_DEVICES); >> struct mutex bus_lock; >> struct mutex msg_lock; >> int (*compute_params)(struct sdw_bus *bus); >> const struct sdw_master_ops *ops; >> const struct sdw_master_port_ops *port_ops; >> struct sdw_bus_params params; >> struct sdw_master_prop prop; >> >> The quirks could be set by a firmware property, and it seems logical to add >> them at the same place where we already have properties defined in firmware, >> no? That way all the standard, vendor-specific and quirks are read or added >> in the same place. > > Or they could be set by driver as well based on device id, compatible > and so on.. It maybe fw property as well, so limiting to property may not > be best idea IMO. Not following, sorry. I didn't mean that only firmware can set them. All of these sdw_master_prop can come from firmware or be hard-coded by a driver for a specific implementation. The point is that they need to be provided to the bus core so that it knows what to do. If you look at DisCo today, we ignore the settings for the Slave (unfortunately all wrong in BIOS) so the Slave properties are hard-coded in drivers, but do use most of the firmware information for the Master, so it's really firmware and/or driver that can set these properties. >> the sdw_master_device isn't a good place for quirks IMHO, it's a very >> shallow software-only layer without any existing ties to the hardware >> definition. > > This one I would agree. >
On 02-02-21, 10:52, Pierre-Louis Bossart wrote: > > > On 2/1/21 10:39 PM, Vinod Koul wrote: > > On 01-02-21, 10:18, Pierre-Louis Bossart wrote: > > > On 2/1/21 4:38 AM, Vinod Koul wrote: > > > > On 01-02-21, 15:58, Vinod Koul wrote: > > > > > On 26-01-21, 16:37, Bard Liao wrote: > > > > > > > > > > struct sdw_master_prop { > > > > > > u32 revision; > > > > > > @@ -421,8 +422,11 @@ struct sdw_master_prop { > > > > > > u32 err_threshold; > > > > > > u32 mclk_freq; > > > > > > bool hw_disabled; > > > > > > + u32 quirks; > > > > > > > > > > Can we do u64 here please.. I dont know where we would end up.. but > > > > > would hate if we start running out of space .. > > > No objection. > > > > > > > Also, is the sdw_master_prop right place for a 'quirk' property. I think > > > > we can use sdw_master_device or sdw_bus as this seems like a bus > > > > quirk..? > > > > > > It's already part of sdw_bus > > > > Right, but the point is that the properties were mostly derived from > > DiSco, so am bit skeptical about it adding it there.. > > Oh, I am planning to contribute such quirks as MIPI DisCo properties for the > next revision of the document (along with a capability to disable a link). > This was not intended to remain Linux- or Intel-specific. Okay lets keep it in properties then
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 6e1c988f3845..d394905936e4 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -1240,6 +1240,7 @@ static int sdw_slave_set_frequency(struct sdw_slave *slave) static int sdw_initialize_slave(struct sdw_slave *slave) { struct sdw_slave_prop *prop = &slave->prop; + int status; int ret; u8 val; @@ -1247,6 +1248,15 @@ static int sdw_initialize_slave(struct sdw_slave *slave) if (ret < 0) return ret; + if (slave->bus->prop.quirks & SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH) { + /* Clear bus clash interrupt before enabling interrupt mask */ + status = sdw_read_no_pm(slave, SDW_SCP_INT1); + if (status & SDW_SCP_INT1_BUS_CLASH) { + dev_warn(&slave->dev, "Bus clash detected before INT mask is enabled\n"); + sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_BUS_CLASH); + } + } + /* * Set SCP_INT1_MASK register, typically bus clash and * implementation-defined interrupt mask. The Parity detection diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index f0b01b728640..a2766c3b603d 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -405,6 +405,7 @@ struct sdw_slave_prop { * command * @mclk_freq: clock reference passed to SoundWire Master, in Hz. * @hw_disabled: if true, the Master is not functional, typically due to pin-mux + * @quirks: bitmask identifying optional behavior beyond the scope of the MIPI specification */ struct sdw_master_prop { u32 revision; @@ -421,8 +422,11 @@ struct sdw_master_prop { u32 err_threshold; u32 mclk_freq; bool hw_disabled; + u32 quirks; }; +#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH BIT(0) + int sdw_master_read_prop(struct sdw_bus *bus); int sdw_slave_read_prop(struct sdw_slave *slave);