| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20221008125436.ndj2nwesx5lgppsf@mobilestation>
Date: Sat, 8 Oct 2022 15:54:36 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Patrick Rudolph <patrick.rudolph@...ements.com>
Cc: Peter Rosin <peda@...ntia.se>, robh@...nel.org,
laurent.pinchart@...asonboard.com, wsa@...nel.org,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [v9 3/4] i2c: muxes: pca954x: Configure MAX7357 in enhanced mode
On Fri, Oct 07, 2022 at 09:53:52AM +0200, Patrick Rudolph wrote:
> The MAX7357 and MAX7358 have 6 additional registers called enhanced mode
> in the following paragraphs. While the MAX7357 exposes those registers
> without a special sequence, the MAX7358 requires an unlock sequence.
> The enhanced mode allows to configure optional features which are nice to
> have on an I2C mux, but are not mandatory for it's general operation.
>
> As I don't have a MAX7358 for testing the special unlock sequence the
> enhanced mode isn't used on the MAX7358, but it could be added later
> if required.
Not that hard to do. Just place four I2C_SMBUS_QUICK messages in a
single transfer:
S Addr Wr [A] Sr Addr Rd [A] Sr Addr Wr [A] Sr Addr Rd [A] P
it can be easily done by means of the i2c_transfer() method called
with four i2c_msg instances (Wr/Rd/Wr/Rd with zero length) passed.
See the way the quicks smbus-messages are implemented in
i2c_smbus_xfer_emulated((). Just place four if them in single array
and pass to the i2c_transfer() method.
Note some drivers may unsupport the I2C-level transfers. Also note
some adapters may unsupport the zero-length I2C-transfers. AFAIR we
had such problem with the Synopsys DW I2C controller.
>
> The MAX7357 enhanced mode is used to configure the chip to
> - Disable interrupts on bus locked up detection
> - Enable bus locked-up clearing
> - Disconnect only locked bus instead of all channels
>
> This configuration protects the I2C tree from total failure and attempts
> to unbrick the faulty bus. It's unclear why this isn't the default
> configuration.
>
> Tested using the MAX7357 and verified that the stalled bus is disconnected
> while the other channels remain operational.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@...ements.com>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 46 ++++++++++++++++++++++++++++-
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 4b63b1eb669e..992976fa6798 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -57,6 +57,37 @@
>
> #define PCA954X_IRQ_OFFSET 4
>
> +/*
> + * The MAX7357 and MAX7358 have 6 additional registers called enhanced mode
> + * in the following paragraphs. While the MAX7357 exposes those registers
> + * without a special sequence, the MAX7358 requires an unlock sequence.
> + *
> + * The first enhanced mode register called CONF allows to configure
> + * additional features.
> + */
> +#define MAX7357_REG_SWITCH 0
> +#define MAX7357_REG_CONF 1
> +#define MAX7357_CONF_INT_ENABLE BIT(0)
> +#define MAX7357_CONF_FLUSH_OUT BIT(1)
> +#define MAX7357_CONF_RELEASE_INT BIT(2)
> +#define MAX7357_CONF_LOCK_UP_CLEAR_ON_READ BIT(3)
> +#define MAX7357_CONF_DISCON_SINGLE_CHAN BIT(4)
> +#define MAX7357_CONF_BUS_LOCKUP_DETECTION_DISABLE BIT(5)
> +#define MAX7357_CONF_ENABLE_BASIC_MODE BIT(6)
> +#define MAX7357_CONF_PRECONNECT_TEST BIT(7)
> +
> +/*
> + * On boot the MAX735x behave like a regular MUX. Apply a fixed
> + * default configuration on MAX7357 that:
> + * - disables interrupts
> + * - sents automatically flush-out sequence on locked-up channels
> + when a lock-up condition is detected
> + * - isolates only the locked channel instead of all channels
> + * - doesn't disable bus lock-up detection.
> + */
> +#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
> + MAX7357_CONF_DISCON_SINGLE_CHAN)
Moving the macro definition fully to the new line might look a bit
nicer:
+#define MAX7357_CONF_DEFAULTS \
(MAX7357_CONF_FLUSH_OUT | MAX7357_CONF_DISCON_SINGLE_CHAN)
> +
> enum pca_type {
> max_7367,
> max_7368,
> @@ -82,6 +113,7 @@ struct chip_desc {
> u8 nchans;
> u8 enable; /* used for muxes only */
> u8 has_irq;
> + u8 maxim_enhanced_mode;
So long name.( What about a shorter version, i.e. max(im)?_enh ?
BTW how to differentiate the devices with the enhanced mode
enabled/disabled by default?
-Sergey
> enum muxtype {
> pca954x_ismux = 0,
> pca954x_isswi
> @@ -113,6 +145,7 @@ static const struct chip_desc chips[] = {
> [max_7357] = {
> .nchans = 8,
> .muxtype = pca954x_isswi,
> + .maxim_enhanced_mode = 1,
> .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
> },
> [max_7358] = {
> @@ -452,6 +485,7 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
>
> static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> {
> + struct i2c_adapter *adap = client->adapter;
> int ret;
>
> if (data->idle_state >= 0)
> @@ -459,7 +493,17 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> else
> data->last_chan = 0; /* Disconnect multiplexer */
>
> - ret = i2c_smbus_write_byte(client, data->last_chan);
> + if (data->chip->maxim_enhanced_mode) {
> + if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
> + ret = i2c_smbus_write_byte_data(client, data->last_chan,
> + MAX7357_CONF_DEFAULTS);
> + } else {
> + dev_warn(&client->dev, "Didn't configure enhanced defaults\n");
> + ret = i2c_smbus_write_byte(client, data->last_chan);
> + }
> + } else {
> + ret = i2c_smbus_write_byte(client, data->last_chan);
> + }
> if (ret < 0)
> data->last_chan = 0;
>
> --
> 2.37.3
>
Powered by blists - more mailing lists