diff mbox series

drm/bridge: it6505: Add lock for it6505 i2c bank

Message ID	20230324072958.2993946-1-hsinyi@chromium.org
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Hsin-Yi Wang <hsinyi@chromium.org> To: Robert Foss <rfoss@kernel.org>, Douglas Anderson <dianders@chromium.org> Cc: Andrzej Hajda <andrzej.hajda@intel.com>, Neil Armstrong <neil.armstrong@linaro.org>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Jonas Karlman <jonas@kwiboo.se>, Jernej Skrabec <jernej.skrabec@gmail.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kenneth.hung@ite.corp-partner.google.com, xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com> Subject: [PATCH] drm/bridge: it6505: Add lock for it6505 i2c bank Date: Fri, 24 Mar 2023 15:29:58 +0800 Message-Id: <20230324072958.2993946-1-hsinyi@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	drm/bridge: it6505: Add lock for it6505 i2c bank \| drm/bridge: it6505: Add lock for it6505 i2c bank

Commit Message

Hsin-Yi Wang March 24, 2023, 7:29 a.m. UTC

  From: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>

When the i2c bank register (REG_BANK_SEL) is set to 1,
only the registers belong to bank 1 can be written.
There will be a race condition when a process is writing
bank 0 registers while another process set the bank to 1.
Add a mutex to handle regmap read/write locking for
registers in multiple i2c bank. Since the driver now
owns the lock, there's no need to use regmap API's lock.

Signed-off-by: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 72 ++++++++++++++++++++++-------
 1 file changed, 55 insertions(+), 17 deletions(-)

Comments

Andrzej Hajda March 24, 2023, 12:18 p.m. UTC | #1

On 24.03.2023 08:29, Hsin-Yi Wang wrote:
> From: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
>
> When the i2c bank register (REG_BANK_SEL) is set to 1,
> only the registers belong to bank 1 can be written.
> There will be a race condition when a process is writing
> bank 0 registers while another process set the bank to 1.
> Add a mutex to handle regmap read/write locking for
> registers in multiple i2c bank. Since the driver now
> owns the lock, there's no need to use regmap API's lock.
>
> Signed-off-by: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>   drivers/gpu/drm/bridge/ite-it6505.c | 72 ++++++++++++++++++++++-------
>   1 file changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index bc451b2a77c28..1a8dcc49fc1ee 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -258,12 +258,12 @@
>   #define REG_AUD_INFOFRAM_SUM 0xFB
>   
>   /* the following six registers are in bank1 */
> -#define REG_DRV_0_DB_800_MV 0x7E
> -#define REG_PRE_0_DB_800_MV 0x7F
> -#define REG_PRE_3P5_DB_800_MV 0x81
> -#define REG_SSC_CTRL0 0x88
> -#define REG_SSC_CTRL1 0x89
> -#define REG_SSC_CTRL2 0x8A
> +#define REG_DRV_0_DB_800_MV 0x17E
> +#define REG_PRE_0_DB_800_MV 0x17F
> +#define REG_PRE_3P5_DB_800_MV 0x181
> +#define REG_SSC_CTRL0 0x188
> +#define REG_SSC_CTRL1 0x189
> +#define REG_SSC_CTRL2 0x18A
>   
>   #define RBR DP_LINK_BW_1_62
>   #define HBR DP_LINK_BW_2_7
> @@ -414,12 +414,14 @@ struct it6505 {
>   	struct mutex extcon_lock;
>   	struct mutex mode_lock; /* used to bridge_detect */
>   	struct mutex aux_lock; /* used to aux data transfers */
> +	struct mutex bank_lock; /* used to protect i2c bank access */
>   	struct regmap *regmap;
>   	struct drm_display_mode source_output_mode;
>   	struct drm_display_mode video_info;
>   	struct notifier_block event_nb;
>   	struct extcon_dev *extcon;
>   	struct work_struct extcon_wq;
> +	int bank_state;   /* 1 indicates bank 1, 0 indicates bank 0 */
>   	int extcon_state;
>   	enum drm_connector_status connector_status;
>   	enum link_train_status link_state;
> @@ -502,8 +504,22 @@ static const struct regmap_config it6505_regmap_config = {
>   	.val_bits = 8,
>   	.volatile_table = &it6505_bridge_volatile_table,
>   	.cache_type = REGCACHE_NONE,
> +	.disable_locking = true,
> +	.can_sleep = true,
>   };
>   
> +static int it6505_config_bank(struct it6505 *it6505, unsigned int reg_addr)
> +{
> +	int err = 0, target = !!(reg_addr > 0xff);
> +
> +	if (target != it6505->bank_state) {

It would be better to return if equal, this way you can avoid indentation.

> +		err = regmap_write(it6505->regmap, REG_BANK_SEL, target);
> +		if (!err)
> +			it6505->bank_state = target;
> +	}
> +	return err;
> +}
> +
>   static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
>   {
>   	unsigned int value;
> @@ -513,7 +529,10 @@ static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
>   	if (!it6505->powered)
>   		return -ENODEV;
>   
> -	err = regmap_read(it6505->regmap, reg_addr, &value);
> +	mutex_lock(&it6505->bank_lock);
> +	err = it6505_config_bank(it6505, reg_addr);
> +	err |= regmap_read(it6505->regmap, reg_addr & 0xff, &value);

Shoudn't be rather if (!err) err = regmap_read(...) ?

> +	mutex_unlock(&it6505->bank_lock);
>   	if (err < 0) {
>   		dev_err(dev, "read failed reg[0x%x] err: %d", reg_addr, err);
>   		return err;
> @@ -531,8 +550,10 @@ static int it6505_write(struct it6505 *it6505, unsigned int reg_addr,
>   	if (!it6505->powered)
>   		return -ENODEV;
>   
> -	err = regmap_write(it6505->regmap, reg_addr, reg_val);
> -
> +	mutex_lock(&it6505->bank_lock);
> +	err = it6505_config_bank(it6505, reg_addr);
> +	err |= regmap_write(it6505->regmap, reg_addr & 0xff, reg_val);
> +	mutex_unlock(&it6505->bank_lock);
>   	if (err < 0) {
>   		dev_err(dev, "write failed reg[0x%x] = 0x%x err = %d",
>   			reg_addr, reg_val, err);
> @@ -551,7 +572,10 @@ static int it6505_set_bits(struct it6505 *it6505, unsigned int reg,
>   	if (!it6505->powered)
>   		return -ENODEV;
>   
> -	err = regmap_update_bits(it6505->regmap, reg, mask, value);
> +	mutex_lock(&it6505->bank_lock);
> +	err = it6505_config_bank(it6505, reg);
> +	err |= regmap_update_bits(it6505->regmap, reg & 0xff, mask, value);
> +	mutex_unlock(&it6505->bank_lock);
>   	if (err < 0) {
>   		dev_err(dev, "write reg[0x%x] = 0x%x mask = 0x%x failed err %d",
>   			reg, value, mask, err);
> @@ -892,7 +916,10 @@ static void it6505_aux_reset(struct it6505 *it6505)
>   
>   static void it6505_reset_logic(struct it6505 *it6505)
>   {
> +	mutex_lock(&it6505->bank_lock);
> +	it6505_config_bank(it6505, REG_RESET_CTRL);
>   	regmap_write(it6505->regmap, REG_RESET_CTRL, ALL_LOGIC_RESET);
> +	mutex_unlock(&it6505->bank_lock);

Why not call it6505_write ?
>   	usleep_range(1000, 1500);
>   }
>   
> @@ -972,9 +999,14 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>   	it6505_write(it6505, REG_AUX_ADR_16_19,
>   		     ((address >> 16) & 0x0F) | ((size - 1) << 4));
>   
> -	if (cmd == CMD_AUX_NATIVE_WRITE)
> +	if (cmd == CMD_AUX_NATIVE_WRITE) {
> +		mutex_lock(&it6505->bank_lock);
> +		it6505_config_bank(it6505, REG_AUX_OUT_DATA0);
>   		regmap_bulk_write(it6505->regmap, REG_AUX_OUT_DATA0, buffer,
>   				  size);
> +		mutex_unlock(&it6505->bank_lock);
> +	}
> +
>   
>   	/* Aux Fire */
>   	it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
> @@ -1197,9 +1229,12 @@ static int it6505_send_video_infoframe(struct it6505 *it6505,
>   	if (err)
>   		return err;
>   
> -	err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> +	mutex_lock(&it6505->bank_lock);
> +	err = it6505_config_bank(it6505, REG_AVI_INFO_DB1);
> +	err |= regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
>   				buffer + HDMI_INFOFRAME_HEADER_SIZE,
>   				frame->length);
> +	mutex_unlock(&it6505->bank_lock);

Common code with it6505_aux_operation, maybe it6505_bulk_write ?

Have you checked if regmap does not support banking? IMO it should be 
implemented there.

Regards
Andrzej

>   	if (err)
>   		return err;
>   
> @@ -1267,7 +1302,6 @@ static void it6505_init(struct it6505 *it6505)
>   	it6505_write(it6505, REG_TIME_STMP_CTRL,
>   		     EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
>   	it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
> -	it6505_write(it6505, REG_BANK_SEL, 0x01);
>   	it6505_write(it6505, REG_DRV_0_DB_800_MV,
>   		     afe_setting_table[it6505->afe_setting][0]);
>   	it6505_write(it6505, REG_PRE_0_DB_800_MV,
> @@ -1277,7 +1311,6 @@ static void it6505_init(struct it6505 *it6505)
>   	it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
>   	it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
>   	it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> -	it6505_write(it6505, REG_BANK_SEL, 0x00);
>   }
>   
>   static void it6505_video_disable(struct it6505 *it6505)
> @@ -1506,11 +1539,9 @@ static void it6505_setup_ssc(struct it6505 *it6505)
>   	it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
>   			it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
>   	if (it6505->enable_ssc) {
> -		it6505_write(it6505, REG_BANK_SEL, 0x01);
>   		it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
>   		it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
>   		it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> -		it6505_write(it6505, REG_BANK_SEL, 0x00);
>   		it6505_write(it6505, REG_SP_CTRL0, 0x07);
>   		it6505_write(it6505, REG_IP_CTRL1, 0x29);
>   		it6505_write(it6505, REG_IP_CTRL2, 0x03);
> @@ -1983,8 +2014,11 @@ static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
>   	it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ,
>   			HDCP_EN_M0_READ);
>   
> -	err = regmap_bulk_read(it6505->regmap, REG_M0_0_7,
> +	mutex_lock(&it6505->bank_lock);
> +	err = it6505_config_bank(it6505, REG_M0_0_7);
> +	err |= regmap_bulk_read(it6505->regmap, REG_M0_0_7,
>   			       sha1_input + msg_count, 8);
> +	mutex_unlock(&it6505->bank_lock);
>   
>   	it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ, 0x00);
>   
> @@ -2577,6 +2611,9 @@ static int it6505_poweron(struct it6505 *it6505)
>   	}
>   
>   	it6505->powered = true;
> +	mutex_lock(&it6505->bank_lock);
> +	it6505->bank_state = 0;
> +	mutex_unlock(&it6505->bank_lock);
>   	it6505_reset_logic(it6505);
>   	it6505_int_mask_enable(it6505);
>   	it6505_init(it6505);
> @@ -3359,6 +3396,7 @@ static int it6505_i2c_probe(struct i2c_client *client)
>   	mutex_init(&it6505->extcon_lock);
>   	mutex_init(&it6505->mode_lock);
>   	mutex_init(&it6505->aux_lock);
> +	mutex_init(&it6505->bank_lock);
>   
>   	it6505->bridge.of_node = client->dev.of_node;
>   	it6505->connector_status = connector_status_disconnected;

Hsin-Yi Wang March 24, 2023, 3:34 p.m. UTC | #2

On Fri, Mar 24, 2023 at 8:18 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>
>
>
> On 24.03.2023 08:29, Hsin-Yi Wang wrote:
> > From: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> >
> > When the i2c bank register (REG_BANK_SEL) is set to 1,
> > only the registers belong to bank 1 can be written.
> > There will be a race condition when a process is writing
> > bank 0 registers while another process set the bank to 1.
> > Add a mutex to handle regmap read/write locking for
> > registers in multiple i2c bank. Since the driver now
> > owns the lock, there's no need to use regmap API's lock.
> >
> > Signed-off-by: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> >   drivers/gpu/drm/bridge/ite-it6505.c | 72 ++++++++++++++++++++++-------
> >   1 file changed, 55 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> > index bc451b2a77c28..1a8dcc49fc1ee 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > @@ -258,12 +258,12 @@
> >   #define REG_AUD_INFOFRAM_SUM 0xFB
> >
> >   /* the following six registers are in bank1 */
> > -#define REG_DRV_0_DB_800_MV 0x7E
> > -#define REG_PRE_0_DB_800_MV 0x7F
> > -#define REG_PRE_3P5_DB_800_MV 0x81
> > -#define REG_SSC_CTRL0 0x88
> > -#define REG_SSC_CTRL1 0x89
> > -#define REG_SSC_CTRL2 0x8A
> > +#define REG_DRV_0_DB_800_MV 0x17E
> > +#define REG_PRE_0_DB_800_MV 0x17F
> > +#define REG_PRE_3P5_DB_800_MV 0x181
> > +#define REG_SSC_CTRL0 0x188
> > +#define REG_SSC_CTRL1 0x189
> > +#define REG_SSC_CTRL2 0x18A
> >
> >   #define RBR DP_LINK_BW_1_62
> >   #define HBR DP_LINK_BW_2_7
> > @@ -414,12 +414,14 @@ struct it6505 {
> >       struct mutex extcon_lock;
> >       struct mutex mode_lock; /* used to bridge_detect */
> >       struct mutex aux_lock; /* used to aux data transfers */
> > +     struct mutex bank_lock; /* used to protect i2c bank access */
> >       struct regmap *regmap;
> >       struct drm_display_mode source_output_mode;
> >       struct drm_display_mode video_info;
> >       struct notifier_block event_nb;
> >       struct extcon_dev *extcon;
> >       struct work_struct extcon_wq;
> > +     int bank_state;   /* 1 indicates bank 1, 0 indicates bank 0 */
> >       int extcon_state;
> >       enum drm_connector_status connector_status;
> >       enum link_train_status link_state;
> > @@ -502,8 +504,22 @@ static const struct regmap_config it6505_regmap_config = {
> >       .val_bits = 8,
> >       .volatile_table = &it6505_bridge_volatile_table,
> >       .cache_type = REGCACHE_NONE,
> > +     .disable_locking = true,
> > +     .can_sleep = true,
> >   };
> >
> > +static int it6505_config_bank(struct it6505 *it6505, unsigned int reg_addr)
> > +{
> > +     int err = 0, target = !!(reg_addr > 0xff);
> > +
> > +     if (target != it6505->bank_state) {
>
> It would be better to return if equal, this way you can avoid indentation.
>
Will fix in v2.
> > +             err = regmap_write(it6505->regmap, REG_BANK_SEL, target);
> > +             if (!err)
> > +                     it6505->bank_state = target;
> > +     }
> > +     return err;
> > +}
> > +
> >   static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> >   {
> >       unsigned int value;
> > @@ -513,7 +529,10 @@ static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> >       if (!it6505->powered)
> >               return -ENODEV;
> >
> > -     err = regmap_read(it6505->regmap, reg_addr, &value);
> > +     mutex_lock(&it6505->bank_lock);
> > +     err = it6505_config_bank(it6505, reg_addr);
> > +     err |= regmap_read(it6505->regmap, reg_addr & 0xff, &value);
>
> Shoudn't be rather if (!err) err = regmap_read(...) ?
>
Will fix in v2.
> > +     mutex_unlock(&it6505->bank_lock);
> >       if (err < 0) {
> >               dev_err(dev, "read failed reg[0x%x] err: %d", reg_addr, err);
> >               return err;
> > @@ -531,8 +550,10 @@ static int it6505_write(struct it6505 *it6505, unsigned int reg_addr,
> >       if (!it6505->powered)
> >               return -ENODEV;
> >
> > -     err = regmap_write(it6505->regmap, reg_addr, reg_val);
> > -
> > +     mutex_lock(&it6505->bank_lock);
> > +     err = it6505_config_bank(it6505, reg_addr);
> > +     err |= regmap_write(it6505->regmap, reg_addr & 0xff, reg_val);
> > +     mutex_unlock(&it6505->bank_lock);
> >       if (err < 0) {
> >               dev_err(dev, "write failed reg[0x%x] = 0x%x err = %d",
> >                       reg_addr, reg_val, err);
> > @@ -551,7 +572,10 @@ static int it6505_set_bits(struct it6505 *it6505, unsigned int reg,
> >       if (!it6505->powered)
> >               return -ENODEV;
> >
> > -     err = regmap_update_bits(it6505->regmap, reg, mask, value);
> > +     mutex_lock(&it6505->bank_lock);
> > +     err = it6505_config_bank(it6505, reg);
> > +     err |= regmap_update_bits(it6505->regmap, reg & 0xff, mask, value);
> > +     mutex_unlock(&it6505->bank_lock);
> >       if (err < 0) {
> >               dev_err(dev, "write reg[0x%x] = 0x%x mask = 0x%x failed err %d",
> >                       reg, value, mask, err);
> > @@ -892,7 +916,10 @@ static void it6505_aux_reset(struct it6505 *it6505)
> >
> >   static void it6505_reset_logic(struct it6505 *it6505)
> >   {
> > +     mutex_lock(&it6505->bank_lock);
> > +     it6505_config_bank(it6505, REG_RESET_CTRL);
> >       regmap_write(it6505->regmap, REG_RESET_CTRL, ALL_LOGIC_RESET);
> > +     mutex_unlock(&it6505->bank_lock);
>
> Why not call it6505_write ?
Will fix in v2.
> >       usleep_range(1000, 1500);
> >   }
> >
> > @@ -972,9 +999,14 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
> >       it6505_write(it6505, REG_AUX_ADR_16_19,
> >                    ((address >> 16) & 0x0F) | ((size - 1) << 4));
> >
> > -     if (cmd == CMD_AUX_NATIVE_WRITE)
> > +     if (cmd == CMD_AUX_NATIVE_WRITE) {
> > +             mutex_lock(&it6505->bank_lock);
> > +             it6505_config_bank(it6505, REG_AUX_OUT_DATA0);
> >               regmap_bulk_write(it6505->regmap, REG_AUX_OUT_DATA0, buffer,
> >                                 size);
> > +             mutex_unlock(&it6505->bank_lock);
> > +     }
> > +
> >
> >       /* Aux Fire */
> >       it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
> > @@ -1197,9 +1229,12 @@ static int it6505_send_video_infoframe(struct it6505 *it6505,
> >       if (err)
> >               return err;
> >
> > -     err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> > +     mutex_lock(&it6505->bank_lock);
> > +     err = it6505_config_bank(it6505, REG_AVI_INFO_DB1);
> > +     err |= regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> >                               buffer + HDMI_INFOFRAME_HEADER_SIZE,
> >                               frame->length);
> > +     mutex_unlock(&it6505->bank_lock);
>
> Common code with it6505_aux_operation, maybe it6505_bulk_write ?
Will fix in v2.
>
> Have you checked if regmap does not support banking? IMO it should be
> implemented there.

Thanks for the suggestion. Do you mean regmap_field*? I think the
banking in that series of API is different to the use case here. Since
we would need to additionally set REG_BANK_SEL to 0 or 1, we still
need a similar approach to this patch.
We can also use regmap_multi_reg_write() to carry REG_BANK_SEL and the
reg we want to write but that would have set REG_BANK_SEL multiple
times unnecessarily (eg. a consequent write on 2 regs on bank 0
doesn't need to write REG_BANK_SEL to 0 again on the second one.)


>
> Regards
> Andrzej
>
> >       if (err)
> >               return err;
> >
> > @@ -1267,7 +1302,6 @@ static void it6505_init(struct it6505 *it6505)
> >       it6505_write(it6505, REG_TIME_STMP_CTRL,
> >                    EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
> >       it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
> > -     it6505_write(it6505, REG_BANK_SEL, 0x01);
> >       it6505_write(it6505, REG_DRV_0_DB_800_MV,
> >                    afe_setting_table[it6505->afe_setting][0]);
> >       it6505_write(it6505, REG_PRE_0_DB_800_MV,
> > @@ -1277,7 +1311,6 @@ static void it6505_init(struct it6505 *it6505)
> >       it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
> >       it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
> >       it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> > -     it6505_write(it6505, REG_BANK_SEL, 0x00);
> >   }
> >
> >   static void it6505_video_disable(struct it6505 *it6505)
> > @@ -1506,11 +1539,9 @@ static void it6505_setup_ssc(struct it6505 *it6505)
> >       it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
> >                       it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
> >       if (it6505->enable_ssc) {
> > -             it6505_write(it6505, REG_BANK_SEL, 0x01);
> >               it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
> >               it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
> >               it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> > -             it6505_write(it6505, REG_BANK_SEL, 0x00);
> >               it6505_write(it6505, REG_SP_CTRL0, 0x07);
> >               it6505_write(it6505, REG_IP_CTRL1, 0x29);
> >               it6505_write(it6505, REG_IP_CTRL2, 0x03);
> > @@ -1983,8 +2014,11 @@ static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
> >       it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ,
> >                       HDCP_EN_M0_READ);
> >
> > -     err = regmap_bulk_read(it6505->regmap, REG_M0_0_7,
> > +     mutex_lock(&it6505->bank_lock);
> > +     err = it6505_config_bank(it6505, REG_M0_0_7);
> > +     err |= regmap_bulk_read(it6505->regmap, REG_M0_0_7,
> >                              sha1_input + msg_count, 8);
> > +     mutex_unlock(&it6505->bank_lock);
> >
> >       it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ, 0x00);
> >
> > @@ -2577,6 +2611,9 @@ static int it6505_poweron(struct it6505 *it6505)
> >       }
> >
> >       it6505->powered = true;
> > +     mutex_lock(&it6505->bank_lock);
> > +     it6505->bank_state = 0;
> > +     mutex_unlock(&it6505->bank_lock);
> >       it6505_reset_logic(it6505);
> >       it6505_int_mask_enable(it6505);
> >       it6505_init(it6505);
> > @@ -3359,6 +3396,7 @@ static int it6505_i2c_probe(struct i2c_client *client)
> >       mutex_init(&it6505->extcon_lock);
> >       mutex_init(&it6505->mode_lock);
> >       mutex_init(&it6505->aux_lock);
> > +     mutex_init(&it6505->bank_lock);
> >
> >       it6505->bridge.of_node = client->dev.of_node;
> >       it6505->connector_status = connector_status_disconnected;
>

Hsin-Yi Wang March 25, 2023, 6:17 a.m. UTC | #3

On Fri, Mar 24, 2023 at 11:34 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Fri, Mar 24, 2023 at 8:18 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> >
> >
> >
> > On 24.03.2023 08:29, Hsin-Yi Wang wrote:
> > > From: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> > >
> > > When the i2c bank register (REG_BANK_SEL) is set to 1,
> > > only the registers belong to bank 1 can be written.
> > > There will be a race condition when a process is writing
> > > bank 0 registers while another process set the bank to 1.
> > > Add a mutex to handle regmap read/write locking for
> > > registers in multiple i2c bank. Since the driver now
> > > owns the lock, there's no need to use regmap API's lock.
> > >
> > > Signed-off-by: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > ---
> > >   drivers/gpu/drm/bridge/ite-it6505.c | 72 ++++++++++++++++++++++-------
> > >   1 file changed, 55 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> > > index bc451b2a77c28..1a8dcc49fc1ee 100644
> > > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > > @@ -258,12 +258,12 @@
> > >   #define REG_AUD_INFOFRAM_SUM 0xFB
> > >
> > >   /* the following six registers are in bank1 */
> > > -#define REG_DRV_0_DB_800_MV 0x7E
> > > -#define REG_PRE_0_DB_800_MV 0x7F
> > > -#define REG_PRE_3P5_DB_800_MV 0x81
> > > -#define REG_SSC_CTRL0 0x88
> > > -#define REG_SSC_CTRL1 0x89
> > > -#define REG_SSC_CTRL2 0x8A
> > > +#define REG_DRV_0_DB_800_MV 0x17E
> > > +#define REG_PRE_0_DB_800_MV 0x17F
> > > +#define REG_PRE_3P5_DB_800_MV 0x181
> > > +#define REG_SSC_CTRL0 0x188
> > > +#define REG_SSC_CTRL1 0x189
> > > +#define REG_SSC_CTRL2 0x18A
> > >
> > >   #define RBR DP_LINK_BW_1_62
> > >   #define HBR DP_LINK_BW_2_7
> > > @@ -414,12 +414,14 @@ struct it6505 {
> > >       struct mutex extcon_lock;
> > >       struct mutex mode_lock; /* used to bridge_detect */
> > >       struct mutex aux_lock; /* used to aux data transfers */
> > > +     struct mutex bank_lock; /* used to protect i2c bank access */
> > >       struct regmap *regmap;
> > >       struct drm_display_mode source_output_mode;
> > >       struct drm_display_mode video_info;
> > >       struct notifier_block event_nb;
> > >       struct extcon_dev *extcon;
> > >       struct work_struct extcon_wq;
> > > +     int bank_state;   /* 1 indicates bank 1, 0 indicates bank 0 */
> > >       int extcon_state;
> > >       enum drm_connector_status connector_status;
> > >       enum link_train_status link_state;
> > > @@ -502,8 +504,22 @@ static const struct regmap_config it6505_regmap_config = {
> > >       .val_bits = 8,
> > >       .volatile_table = &it6505_bridge_volatile_table,
> > >       .cache_type = REGCACHE_NONE,
> > > +     .disable_locking = true,
> > > +     .can_sleep = true,
> > >   };
> > >
> > > +static int it6505_config_bank(struct it6505 *it6505, unsigned int reg_addr)
> > > +{
> > > +     int err = 0, target = !!(reg_addr > 0xff);
> > > +
> > > +     if (target != it6505->bank_state) {
> >
> > It would be better to return if equal, this way you can avoid indentation.
> >
> Will fix in v2.
> > > +             err = regmap_write(it6505->regmap, REG_BANK_SEL, target);
> > > +             if (!err)
> > > +                     it6505->bank_state = target;
> > > +     }
> > > +     return err;
> > > +}
> > > +
> > >   static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> > >   {
> > >       unsigned int value;
> > > @@ -513,7 +529,10 @@ static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> > >       if (!it6505->powered)
> > >               return -ENODEV;
> > >
> > > -     err = regmap_read(it6505->regmap, reg_addr, &value);
> > > +     mutex_lock(&it6505->bank_lock);
> > > +     err = it6505_config_bank(it6505, reg_addr);
> > > +     err |= regmap_read(it6505->regmap, reg_addr & 0xff, &value);
> >
> > Shoudn't be rather if (!err) err = regmap_read(...) ?
> >
> Will fix in v2.
> > > +     mutex_unlock(&it6505->bank_lock);
> > >       if (err < 0) {
> > >               dev_err(dev, "read failed reg[0x%x] err: %d", reg_addr, err);
> > >               return err;
> > > @@ -531,8 +550,10 @@ static int it6505_write(struct it6505 *it6505, unsigned int reg_addr,
> > >       if (!it6505->powered)
> > >               return -ENODEV;
> > >
> > > -     err = regmap_write(it6505->regmap, reg_addr, reg_val);
> > > -
> > > +     mutex_lock(&it6505->bank_lock);
> > > +     err = it6505_config_bank(it6505, reg_addr);
> > > +     err |= regmap_write(it6505->regmap, reg_addr & 0xff, reg_val);
> > > +     mutex_unlock(&it6505->bank_lock);
> > >       if (err < 0) {
> > >               dev_err(dev, "write failed reg[0x%x] = 0x%x err = %d",
> > >                       reg_addr, reg_val, err);
> > > @@ -551,7 +572,10 @@ static int it6505_set_bits(struct it6505 *it6505, unsigned int reg,
> > >       if (!it6505->powered)
> > >               return -ENODEV;
> > >
> > > -     err = regmap_update_bits(it6505->regmap, reg, mask, value);
> > > +     mutex_lock(&it6505->bank_lock);
> > > +     err = it6505_config_bank(it6505, reg);
> > > +     err |= regmap_update_bits(it6505->regmap, reg & 0xff, mask, value);
> > > +     mutex_unlock(&it6505->bank_lock);
> > >       if (err < 0) {
> > >               dev_err(dev, "write reg[0x%x] = 0x%x mask = 0x%x failed err %d",
> > >                       reg, value, mask, err);
> > > @@ -892,7 +916,10 @@ static void it6505_aux_reset(struct it6505 *it6505)
> > >
> > >   static void it6505_reset_logic(struct it6505 *it6505)
> > >   {
> > > +     mutex_lock(&it6505->bank_lock);
> > > +     it6505_config_bank(it6505, REG_RESET_CTRL);
> > >       regmap_write(it6505->regmap, REG_RESET_CTRL, ALL_LOGIC_RESET);
> > > +     mutex_unlock(&it6505->bank_lock);
> >
> > Why not call it6505_write ?
> Will fix in v2.
> > >       usleep_range(1000, 1500);
> > >   }
> > >
> > > @@ -972,9 +999,14 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
> > >       it6505_write(it6505, REG_AUX_ADR_16_19,
> > >                    ((address >> 16) & 0x0F) | ((size - 1) << 4));
> > >
> > > -     if (cmd == CMD_AUX_NATIVE_WRITE)
> > > +     if (cmd == CMD_AUX_NATIVE_WRITE) {
> > > +             mutex_lock(&it6505->bank_lock);
> > > +             it6505_config_bank(it6505, REG_AUX_OUT_DATA0);
> > >               regmap_bulk_write(it6505->regmap, REG_AUX_OUT_DATA0, buffer,
> > >                                 size);
> > > +             mutex_unlock(&it6505->bank_lock);
> > > +     }
> > > +
> > >
> > >       /* Aux Fire */
> > >       it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
> > > @@ -1197,9 +1229,12 @@ static int it6505_send_video_infoframe(struct it6505 *it6505,
> > >       if (err)
> > >               return err;
> > >
> > > -     err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> > > +     mutex_lock(&it6505->bank_lock);
> > > +     err = it6505_config_bank(it6505, REG_AVI_INFO_DB1);
> > > +     err |= regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> > >                               buffer + HDMI_INFOFRAME_HEADER_SIZE,
> > >                               frame->length);
> > > +     mutex_unlock(&it6505->bank_lock);
> >
> > Common code with it6505_aux_operation, maybe it6505_bulk_write ?
> Will fix in v2.
> >
> > Have you checked if regmap does not support banking? IMO it should be
> > implemented there.
>
> Thanks for the suggestion. Do you mean regmap_field*? I think the
> banking in that series of API is different to the use case here. Since
> we would need to additionally set REG_BANK_SEL to 0 or 1, we still
> need a similar approach to this patch.
> We can also use regmap_multi_reg_write() to carry REG_BANK_SEL and the
> reg we want to write but that would have set REG_BANK_SEL multiple
> times unnecessarily (eg. a consequent write on 2 regs on bank 0
> doesn't need to write REG_BANK_SEL to 0 again on the second one.)
>
We should set selector_reg which should solve the issue.

>
> >
> > Regards
> > Andrzej
> >
> > >       if (err)
> > >               return err;
> > >
> > > @@ -1267,7 +1302,6 @@ static void it6505_init(struct it6505 *it6505)
> > >       it6505_write(it6505, REG_TIME_STMP_CTRL,
> > >                    EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
> > >       it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
> > > -     it6505_write(it6505, REG_BANK_SEL, 0x01);
> > >       it6505_write(it6505, REG_DRV_0_DB_800_MV,
> > >                    afe_setting_table[it6505->afe_setting][0]);
> > >       it6505_write(it6505, REG_PRE_0_DB_800_MV,
> > > @@ -1277,7 +1311,6 @@ static void it6505_init(struct it6505 *it6505)
> > >       it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
> > >       it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
> > >       it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> > > -     it6505_write(it6505, REG_BANK_SEL, 0x00);
> > >   }
> > >
> > >   static void it6505_video_disable(struct it6505 *it6505)
> > > @@ -1506,11 +1539,9 @@ static void it6505_setup_ssc(struct it6505 *it6505)
> > >       it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
> > >                       it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
> > >       if (it6505->enable_ssc) {
> > > -             it6505_write(it6505, REG_BANK_SEL, 0x01);
> > >               it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
> > >               it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
> > >               it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> > > -             it6505_write(it6505, REG_BANK_SEL, 0x00);
> > >               it6505_write(it6505, REG_SP_CTRL0, 0x07);
> > >               it6505_write(it6505, REG_IP_CTRL1, 0x29);
> > >               it6505_write(it6505, REG_IP_CTRL2, 0x03);
> > > @@ -1983,8 +2014,11 @@ static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
> > >       it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ,
> > >                       HDCP_EN_M0_READ);
> > >
> > > -     err = regmap_bulk_read(it6505->regmap, REG_M0_0_7,
> > > +     mutex_lock(&it6505->bank_lock);
> > > +     err = it6505_config_bank(it6505, REG_M0_0_7);
> > > +     err |= regmap_bulk_read(it6505->regmap, REG_M0_0_7,
> > >                              sha1_input + msg_count, 8);
> > > +     mutex_unlock(&it6505->bank_lock);
> > >
> > >       it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ, 0x00);
> > >
> > > @@ -2577,6 +2611,9 @@ static int it6505_poweron(struct it6505 *it6505)
> > >       }
> > >
> > >       it6505->powered = true;
> > > +     mutex_lock(&it6505->bank_lock);
> > > +     it6505->bank_state = 0;
> > > +     mutex_unlock(&it6505->bank_lock);
> > >       it6505_reset_logic(it6505);
> > >       it6505_int_mask_enable(it6505);
> > >       it6505_init(it6505);
> > > @@ -3359,6 +3396,7 @@ static int it6505_i2c_probe(struct i2c_client *client)
> > >       mutex_init(&it6505->extcon_lock);
> > >       mutex_init(&it6505->mode_lock);
> > >       mutex_init(&it6505->aux_lock);
> > > +     mutex_init(&it6505->bank_lock);
> > >
> > >       it6505->bridge.of_node = client->dev.of_node;
> > >       it6505->connector_status = connector_status_disconnected;
> >

Hsin-Yi Wang March 27, 2023, 5:04 a.m. UTC | #4

On Sat, Mar 25, 2023 at 2:17 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Fri, Mar 24, 2023 at 11:34 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > On Fri, Mar 24, 2023 at 8:18 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> > >
> > >
> > >
> > > On 24.03.2023 08:29, Hsin-Yi Wang wrote:
> > > > From: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> > > >
> > > > When the i2c bank register (REG_BANK_SEL) is set to 1,
> > > > only the registers belong to bank 1 can be written.
> > > > There will be a race condition when a process is writing
> > > > bank 0 registers while another process set the bank to 1.
> > > > Add a mutex to handle regmap read/write locking for
> > > > registers in multiple i2c bank. Since the driver now
> > > > owns the lock, there's no need to use regmap API's lock.
> > > >
> > > > Signed-off-by: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > > ---
> > > >   drivers/gpu/drm/bridge/ite-it6505.c | 72 ++++++++++++++++++++++-------
> > > >   1 file changed, 55 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> > > > index bc451b2a77c28..1a8dcc49fc1ee 100644
> > > > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > > > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > > > @@ -258,12 +258,12 @@
> > > >   #define REG_AUD_INFOFRAM_SUM 0xFB
> > > >
> > > >   /* the following six registers are in bank1 */
> > > > -#define REG_DRV_0_DB_800_MV 0x7E
> > > > -#define REG_PRE_0_DB_800_MV 0x7F
> > > > -#define REG_PRE_3P5_DB_800_MV 0x81
> > > > -#define REG_SSC_CTRL0 0x88
> > > > -#define REG_SSC_CTRL1 0x89
> > > > -#define REG_SSC_CTRL2 0x8A
> > > > +#define REG_DRV_0_DB_800_MV 0x17E
> > > > +#define REG_PRE_0_DB_800_MV 0x17F
> > > > +#define REG_PRE_3P5_DB_800_MV 0x181
> > > > +#define REG_SSC_CTRL0 0x188
> > > > +#define REG_SSC_CTRL1 0x189
> > > > +#define REG_SSC_CTRL2 0x18A
> > > >
> > > >   #define RBR DP_LINK_BW_1_62
> > > >   #define HBR DP_LINK_BW_2_7
> > > > @@ -414,12 +414,14 @@ struct it6505 {
> > > >       struct mutex extcon_lock;
> > > >       struct mutex mode_lock; /* used to bridge_detect */
> > > >       struct mutex aux_lock; /* used to aux data transfers */
> > > > +     struct mutex bank_lock; /* used to protect i2c bank access */
> > > >       struct regmap *regmap;
> > > >       struct drm_display_mode source_output_mode;
> > > >       struct drm_display_mode video_info;
> > > >       struct notifier_block event_nb;
> > > >       struct extcon_dev *extcon;
> > > >       struct work_struct extcon_wq;
> > > > +     int bank_state;   /* 1 indicates bank 1, 0 indicates bank 0 */
> > > >       int extcon_state;
> > > >       enum drm_connector_status connector_status;
> > > >       enum link_train_status link_state;
> > > > @@ -502,8 +504,22 @@ static const struct regmap_config it6505_regmap_config = {
> > > >       .val_bits = 8,
> > > >       .volatile_table = &it6505_bridge_volatile_table,
> > > >       .cache_type = REGCACHE_NONE,
> > > > +     .disable_locking = true,
> > > > +     .can_sleep = true,
> > > >   };
> > > >
> > > > +static int it6505_config_bank(struct it6505 *it6505, unsigned int reg_addr)
> > > > +{
> > > > +     int err = 0, target = !!(reg_addr > 0xff);
> > > > +
> > > > +     if (target != it6505->bank_state) {
> > >
> > > It would be better to return if equal, this way you can avoid indentation.
> > >
> > Will fix in v2.
> > > > +             err = regmap_write(it6505->regmap, REG_BANK_SEL, target);
> > > > +             if (!err)
> > > > +                     it6505->bank_state = target;
> > > > +     }
> > > > +     return err;
> > > > +}
> > > > +
> > > >   static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> > > >   {
> > > >       unsigned int value;
> > > > @@ -513,7 +529,10 @@ static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
> > > >       if (!it6505->powered)
> > > >               return -ENODEV;
> > > >
> > > > -     err = regmap_read(it6505->regmap, reg_addr, &value);
> > > > +     mutex_lock(&it6505->bank_lock);
> > > > +     err = it6505_config_bank(it6505, reg_addr);
> > > > +     err |= regmap_read(it6505->regmap, reg_addr & 0xff, &value);
> > >
> > > Shoudn't be rather if (!err) err = regmap_read(...) ?
> > >
> > Will fix in v2.
> > > > +     mutex_unlock(&it6505->bank_lock);
> > > >       if (err < 0) {
> > > >               dev_err(dev, "read failed reg[0x%x] err: %d", reg_addr, err);
> > > >               return err;
> > > > @@ -531,8 +550,10 @@ static int it6505_write(struct it6505 *it6505, unsigned int reg_addr,
> > > >       if (!it6505->powered)
> > > >               return -ENODEV;
> > > >
> > > > -     err = regmap_write(it6505->regmap, reg_addr, reg_val);
> > > > -
> > > > +     mutex_lock(&it6505->bank_lock);
> > > > +     err = it6505_config_bank(it6505, reg_addr);
> > > > +     err |= regmap_write(it6505->regmap, reg_addr & 0xff, reg_val);
> > > > +     mutex_unlock(&it6505->bank_lock);
> > > >       if (err < 0) {
> > > >               dev_err(dev, "write failed reg[0x%x] = 0x%x err = %d",
> > > >                       reg_addr, reg_val, err);
> > > > @@ -551,7 +572,10 @@ static int it6505_set_bits(struct it6505 *it6505, unsigned int reg,
> > > >       if (!it6505->powered)
> > > >               return -ENODEV;
> > > >
> > > > -     err = regmap_update_bits(it6505->regmap, reg, mask, value);
> > > > +     mutex_lock(&it6505->bank_lock);
> > > > +     err = it6505_config_bank(it6505, reg);
> > > > +     err |= regmap_update_bits(it6505->regmap, reg & 0xff, mask, value);
> > > > +     mutex_unlock(&it6505->bank_lock);
> > > >       if (err < 0) {
> > > >               dev_err(dev, "write reg[0x%x] = 0x%x mask = 0x%x failed err %d",
> > > >                       reg, value, mask, err);
> > > > @@ -892,7 +916,10 @@ static void it6505_aux_reset(struct it6505 *it6505)
> > > >
> > > >   static void it6505_reset_logic(struct it6505 *it6505)
> > > >   {
> > > > +     mutex_lock(&it6505->bank_lock);
> > > > +     it6505_config_bank(it6505, REG_RESET_CTRL);
> > > >       regmap_write(it6505->regmap, REG_RESET_CTRL, ALL_LOGIC_RESET);
> > > > +     mutex_unlock(&it6505->bank_lock);
> > >
> > > Why not call it6505_write ?
> > Will fix in v2.
> > > >       usleep_range(1000, 1500);
> > > >   }
> > > >
> > > > @@ -972,9 +999,14 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
> > > >       it6505_write(it6505, REG_AUX_ADR_16_19,
> > > >                    ((address >> 16) & 0x0F) | ((size - 1) << 4));
> > > >
> > > > -     if (cmd == CMD_AUX_NATIVE_WRITE)
> > > > +     if (cmd == CMD_AUX_NATIVE_WRITE) {
> > > > +             mutex_lock(&it6505->bank_lock);
> > > > +             it6505_config_bank(it6505, REG_AUX_OUT_DATA0);
> > > >               regmap_bulk_write(it6505->regmap, REG_AUX_OUT_DATA0, buffer,
> > > >                                 size);
> > > > +             mutex_unlock(&it6505->bank_lock);
> > > > +     }
> > > > +
> > > >
> > > >       /* Aux Fire */
> > > >       it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
> > > > @@ -1197,9 +1229,12 @@ static int it6505_send_video_infoframe(struct it6505 *it6505,
> > > >       if (err)
> > > >               return err;
> > > >
> > > > -     err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> > > > +     mutex_lock(&it6505->bank_lock);
> > > > +     err = it6505_config_bank(it6505, REG_AVI_INFO_DB1);
> > > > +     err |= regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> > > >                               buffer + HDMI_INFOFRAME_HEADER_SIZE,
> > > >                               frame->length);
> > > > +     mutex_unlock(&it6505->bank_lock);
> > >
> > > Common code with it6505_aux_operation, maybe it6505_bulk_write ?
> > Will fix in v2.
> > >
> > > Have you checked if regmap does not support banking? IMO it should be
> > > implemented there.
> >
> > Thanks for the suggestion. Do you mean regmap_field*? I think the
> > banking in that series of API is different to the use case here. Since
> > we would need to additionally set REG_BANK_SEL to 0 or 1, we still
> > need a similar approach to this patch.
> > We can also use regmap_multi_reg_write() to carry REG_BANK_SEL and the
> > reg we want to write but that would have set REG_BANK_SEL multiple
> > times unnecessarily (eg. a consequent write on 2 regs on bank 0
> > doesn't need to write REG_BANK_SEL to 0 again on the second one.)
> >
> We should set selector_reg which should solve the issue.
>
Revised in https://patchwork.kernel.org/project/dri-devel/patch/20230327044804.3657551-1-hsinyi@chromium.org/.

Thanks.
> >
> > >
> > > Regards
> > > Andrzej
> > >
> > > >       if (err)
> > > >               return err;
> > > >
> > > > @@ -1267,7 +1302,6 @@ static void it6505_init(struct it6505 *it6505)
> > > >       it6505_write(it6505, REG_TIME_STMP_CTRL,
> > > >                    EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
> > > >       it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
> > > > -     it6505_write(it6505, REG_BANK_SEL, 0x01);
> > > >       it6505_write(it6505, REG_DRV_0_DB_800_MV,
> > > >                    afe_setting_table[it6505->afe_setting][0]);
> > > >       it6505_write(it6505, REG_PRE_0_DB_800_MV,
> > > > @@ -1277,7 +1311,6 @@ static void it6505_init(struct it6505 *it6505)
> > > >       it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
> > > >       it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
> > > >       it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> > > > -     it6505_write(it6505, REG_BANK_SEL, 0x00);
> > > >   }
> > > >
> > > >   static void it6505_video_disable(struct it6505 *it6505)
> > > > @@ -1506,11 +1539,9 @@ static void it6505_setup_ssc(struct it6505 *it6505)
> > > >       it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
> > > >                       it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
> > > >       if (it6505->enable_ssc) {
> > > > -             it6505_write(it6505, REG_BANK_SEL, 0x01);
> > > >               it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
> > > >               it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
> > > >               it6505_write(it6505, REG_SSC_CTRL2, 0x42);
> > > > -             it6505_write(it6505, REG_BANK_SEL, 0x00);
> > > >               it6505_write(it6505, REG_SP_CTRL0, 0x07);
> > > >               it6505_write(it6505, REG_IP_CTRL1, 0x29);
> > > >               it6505_write(it6505, REG_IP_CTRL2, 0x03);
> > > > @@ -1983,8 +2014,11 @@ static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
> > > >       it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ,
> > > >                       HDCP_EN_M0_READ);
> > > >
> > > > -     err = regmap_bulk_read(it6505->regmap, REG_M0_0_7,
> > > > +     mutex_lock(&it6505->bank_lock);
> > > > +     err = it6505_config_bank(it6505, REG_M0_0_7);
> > > > +     err |= regmap_bulk_read(it6505->regmap, REG_M0_0_7,
> > > >                              sha1_input + msg_count, 8);
> > > > +     mutex_unlock(&it6505->bank_lock);
> > > >
> > > >       it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ, 0x00);
> > > >
> > > > @@ -2577,6 +2611,9 @@ static int it6505_poweron(struct it6505 *it6505)
> > > >       }
> > > >
> > > >       it6505->powered = true;
> > > > +     mutex_lock(&it6505->bank_lock);
> > > > +     it6505->bank_state = 0;
> > > > +     mutex_unlock(&it6505->bank_lock);
> > > >       it6505_reset_logic(it6505);
> > > >       it6505_int_mask_enable(it6505);
> > > >       it6505_init(it6505);
> > > > @@ -3359,6 +3396,7 @@ static int it6505_i2c_probe(struct i2c_client *client)
> > > >       mutex_init(&it6505->extcon_lock);
> > > >       mutex_init(&it6505->mode_lock);
> > > >       mutex_init(&it6505->aux_lock);
> > > > +     mutex_init(&it6505->bank_lock);
> > > >
> > > >       it6505->bridge.of_node = client->dev.of_node;
> > > >       it6505->connector_status = connector_status_disconnected;
> > >

diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index bc451b2a77c28..1a8dcc49fc1ee 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -258,12 +258,12 @@ 
 #define REG_AUD_INFOFRAM_SUM 0xFB
 
 /* the following six registers are in bank1 */
-#define REG_DRV_0_DB_800_MV 0x7E
-#define REG_PRE_0_DB_800_MV 0x7F
-#define REG_PRE_3P5_DB_800_MV 0x81
-#define REG_SSC_CTRL0 0x88
-#define REG_SSC_CTRL1 0x89
-#define REG_SSC_CTRL2 0x8A
+#define REG_DRV_0_DB_800_MV 0x17E
+#define REG_PRE_0_DB_800_MV 0x17F
+#define REG_PRE_3P5_DB_800_MV 0x181
+#define REG_SSC_CTRL0 0x188
+#define REG_SSC_CTRL1 0x189
+#define REG_SSC_CTRL2 0x18A
 
 #define RBR DP_LINK_BW_1_62
 #define HBR DP_LINK_BW_2_7
@@ -414,12 +414,14 @@  struct it6505 {
 	struct mutex extcon_lock;
 	struct mutex mode_lock; /* used to bridge_detect */
 	struct mutex aux_lock; /* used to aux data transfers */
+	struct mutex bank_lock; /* used to protect i2c bank access */
 	struct regmap *regmap;
 	struct drm_display_mode source_output_mode;
 	struct drm_display_mode video_info;
 	struct notifier_block event_nb;
 	struct extcon_dev *extcon;
 	struct work_struct extcon_wq;
+	int bank_state;   /* 1 indicates bank 1, 0 indicates bank 0 */
 	int extcon_state;
 	enum drm_connector_status connector_status;
 	enum link_train_status link_state;
@@ -502,8 +504,22 @@  static const struct regmap_config it6505_regmap_config = {
 	.val_bits = 8,
 	.volatile_table = &it6505_bridge_volatile_table,
 	.cache_type = REGCACHE_NONE,
+	.disable_locking = true,
+	.can_sleep = true,
 };
 
+static int it6505_config_bank(struct it6505 *it6505, unsigned int reg_addr)
+{
+	int err = 0, target = !!(reg_addr > 0xff);
+
+	if (target != it6505->bank_state) {
+		err = regmap_write(it6505->regmap, REG_BANK_SEL, target);
+		if (!err)
+			it6505->bank_state = target;
+	}
+	return err;
+}
+
 static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
 {
 	unsigned int value;
@@ -513,7 +529,10 @@  static int it6505_read(struct it6505 *it6505, unsigned int reg_addr)
 	if (!it6505->powered)
 		return -ENODEV;
 
-	err = regmap_read(it6505->regmap, reg_addr, &value);
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, reg_addr);
+	err |= regmap_read(it6505->regmap, reg_addr & 0xff, &value);
+	mutex_unlock(&it6505->bank_lock);
 	if (err < 0) {
 		dev_err(dev, "read failed reg[0x%x] err: %d", reg_addr, err);
 		return err;
@@ -531,8 +550,10 @@  static int it6505_write(struct it6505 *it6505, unsigned int reg_addr,
 	if (!it6505->powered)
 		return -ENODEV;
 
-	err = regmap_write(it6505->regmap, reg_addr, reg_val);
-
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, reg_addr);
+	err |= regmap_write(it6505->regmap, reg_addr & 0xff, reg_val);
+	mutex_unlock(&it6505->bank_lock);
 	if (err < 0) {
 		dev_err(dev, "write failed reg[0x%x] = 0x%x err = %d",
 			reg_addr, reg_val, err);
@@ -551,7 +572,10 @@  static int it6505_set_bits(struct it6505 *it6505, unsigned int reg,
 	if (!it6505->powered)
 		return -ENODEV;
 
-	err = regmap_update_bits(it6505->regmap, reg, mask, value);
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, reg);
+	err |= regmap_update_bits(it6505->regmap, reg & 0xff, mask, value);
+	mutex_unlock(&it6505->bank_lock);
 	if (err < 0) {
 		dev_err(dev, "write reg[0x%x] = 0x%x mask = 0x%x failed err %d",
 			reg, value, mask, err);
@@ -892,7 +916,10 @@  static void it6505_aux_reset(struct it6505 *it6505)
 
 static void it6505_reset_logic(struct it6505 *it6505)
 {
+	mutex_lock(&it6505->bank_lock);
+	it6505_config_bank(it6505, REG_RESET_CTRL);
 	regmap_write(it6505->regmap, REG_RESET_CTRL, ALL_LOGIC_RESET);
+	mutex_unlock(&it6505->bank_lock);
 	usleep_range(1000, 1500);
 }
 
@@ -972,9 +999,14 @@  static ssize_t it6505_aux_operation(struct it6505 *it6505,
 	it6505_write(it6505, REG_AUX_ADR_16_19,
 		     ((address >> 16) & 0x0F) | ((size - 1) << 4));
 
-	if (cmd == CMD_AUX_NATIVE_WRITE)
+	if (cmd == CMD_AUX_NATIVE_WRITE) {
+		mutex_lock(&it6505->bank_lock);
+		it6505_config_bank(it6505, REG_AUX_OUT_DATA0);
 		regmap_bulk_write(it6505->regmap, REG_AUX_OUT_DATA0, buffer,
 				  size);
+		mutex_unlock(&it6505->bank_lock);
+	}
+
 
 	/* Aux Fire */
 	it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
@@ -1197,9 +1229,12 @@  static int it6505_send_video_infoframe(struct it6505 *it6505,
 	if (err)
 		return err;
 
-	err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, REG_AVI_INFO_DB1);
+	err |= regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
 				buffer + HDMI_INFOFRAME_HEADER_SIZE,
 				frame->length);
+	mutex_unlock(&it6505->bank_lock);
 	if (err)
 		return err;
 
@@ -1267,7 +1302,6 @@  static void it6505_init(struct it6505 *it6505)
 	it6505_write(it6505, REG_TIME_STMP_CTRL,
 		     EN_SSC_GAT | EN_ENHANCE_VID_STMP | EN_ENHANCE_AUD_STMP);
 	it6505_write(it6505, REG_INFOFRAME_CTRL, 0x00);
-	it6505_write(it6505, REG_BANK_SEL, 0x01);
 	it6505_write(it6505, REG_DRV_0_DB_800_MV,
 		     afe_setting_table[it6505->afe_setting][0]);
 	it6505_write(it6505, REG_PRE_0_DB_800_MV,
@@ -1277,7 +1311,6 @@  static void it6505_init(struct it6505 *it6505)
 	it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
 	it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
 	it6505_write(it6505, REG_SSC_CTRL2, 0x42);
-	it6505_write(it6505, REG_BANK_SEL, 0x00);
 }
 
 static void it6505_video_disable(struct it6505 *it6505)
@@ -1506,11 +1539,9 @@  static void it6505_setup_ssc(struct it6505 *it6505)
 	it6505_set_bits(it6505, REG_TRAIN_CTRL0, SPREAD_AMP_5,
 			it6505->enable_ssc ? SPREAD_AMP_5 : 0x00);
 	if (it6505->enable_ssc) {
-		it6505_write(it6505, REG_BANK_SEL, 0x01);
 		it6505_write(it6505, REG_SSC_CTRL0, 0x9E);
 		it6505_write(it6505, REG_SSC_CTRL1, 0x1C);
 		it6505_write(it6505, REG_SSC_CTRL2, 0x42);
-		it6505_write(it6505, REG_BANK_SEL, 0x00);
 		it6505_write(it6505, REG_SP_CTRL0, 0x07);
 		it6505_write(it6505, REG_IP_CTRL1, 0x29);
 		it6505_write(it6505, REG_IP_CTRL2, 0x03);
@@ -1983,8 +2014,11 @@  static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
 	it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ,
 			HDCP_EN_M0_READ);
 
-	err = regmap_bulk_read(it6505->regmap, REG_M0_0_7,
+	mutex_lock(&it6505->bank_lock);
+	err = it6505_config_bank(it6505, REG_M0_0_7);
+	err |= regmap_bulk_read(it6505->regmap, REG_M0_0_7,
 			       sha1_input + msg_count, 8);
+	mutex_unlock(&it6505->bank_lock);
 
 	it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ, 0x00);
 
@@ -2577,6 +2611,9 @@  static int it6505_poweron(struct it6505 *it6505)
 	}
 
 	it6505->powered = true;
+	mutex_lock(&it6505->bank_lock);
+	it6505->bank_state = 0;
+	mutex_unlock(&it6505->bank_lock);
 	it6505_reset_logic(it6505);
 	it6505_int_mask_enable(it6505);
 	it6505_init(it6505);
@@ -3359,6 +3396,7 @@  static int it6505_i2c_probe(struct i2c_client *client)
 	mutex_init(&it6505->extcon_lock);
 	mutex_init(&it6505->mode_lock);
 	mutex_init(&it6505->aux_lock);
+	mutex_init(&it6505->bank_lock);
 
 	it6505->bridge.of_node = client->dev.of_node;
 	it6505->connector_status = connector_status_disconnected;