linux-kernel - Re: [PATCH 1/5] i2c: tegra: Add HS mode support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a40329ad49dcc0efe1a468046a0a1ab19e3ff53a.camel@nvidia.com>
Date: Wed, 15 Jan 2025 11:40:49 +0000
From: Kartik Rajput <kkartik@...dia.com>
To: "thierry.reding@...il.com" <thierry.reding@...il.com>
CC: Laxman Dewangan <ldewangan@...dia.com>, Jon Hunter <jonathanh@...dia.com>,
	Akhil R <akhilrajeev@...dia.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "robh@...nel.org" <robh@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "andi.shyti@...nel.org"
	<andi.shyti@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>, "digetx@...il.com"
	<digetx@...il.com>, "linux-tegra@...r.kernel.org"
	<linux-tegra@...r.kernel.org>
Subject: Re: [PATCH 1/5] i2c: tegra: Add HS mode support

On Wed, 2025-01-08 at 17:45 +0100, Thierry Reding wrote:
> On Wed, Jan 08, 2025 at 04:36:16PM +0530, Kartik Rajput wrote:
> > From: Akhil R <akhilrajeev@...dia.com>
> > 
> > Add support for HS (High Speed) mode tranfers, which is supported
> > by
> > Tegra194 onwards.
> > 
> > Signed-off-by: Akhil R <akhilrajeev@...dia.com>
> > Signed-off-by: Kartik Rajput <kkartik@...dia.com>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c
> > index 87976e99e6d0..7b97c6d347ee 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -91,6 +91,7 @@
> >  #define I2C_HEADER_IE_ENABLE                   BIT(17)
> >  #define I2C_HEADER_REPEAT_START                        BIT(16)
> >  #define I2C_HEADER_CONTINUE_XFER               BIT(15)
> > +#define I2C_HEADER_HS_MODE                     BIT(22)
> >  #define I2C_HEADER_SLAVE_ADDR_SHIFT            1
> >  
> >  #define I2C_BUS_CLEAR_CNFG                     0x084
> > @@ -220,10 +221,13 @@ struct tegra_i2c_hw_feature {
> >         u32 thigh_std_mode;
> >         u32 tlow_fast_fastplus_mode;
> >         u32 thigh_fast_fastplus_mode;
> > +       u32 tlow_hs_mode;
> > +       u32 thigh_hs_mode;
> >         u32 setup_hold_time_std_mode;
> >         u32 setup_hold_time_fast_fast_plus_mode;
> >         u32 setup_hold_time_hs_mode;
> >         bool has_interface_timing_reg;
> > +       bool has_hs_mode_support;
> >  };
> >  
> >  /**
> > @@ -681,6 +685,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev
> > *i2c_dev)
> >         if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
> >                 i2c_writel(i2c_dev, tsu_thd,
> > I2C_INTERFACE_TIMING_1);
> >  
> > +       /* Write HS mode registers. These will get used only for HS
> > mode*/
> > +       if (i2c_dev->hw->has_hs_mode_support) {
> > +               tlow = i2c_dev->hw->tlow_hs_mode;
> > +               thigh = i2c_dev->hw->thigh_hs_mode;
> > +               tsu_thd = i2c_dev->hw->setup_hold_time_hs_mode;
> > +
> > +               val = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH,
> > thigh) |
> > +                       FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW,
> > tlow);
> > +               i2c_writel(i2c_dev, val,
> > I2C_HS_INTERFACE_TIMING_0);
> > +               i2c_writel(i2c_dev, tsu_thd,
> > I2C_HS_INTERFACE_TIMING_1);
> > +       }
> > +
> >         clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
> >  
> >         err = clk_set_rate(i2c_dev->div_clk,
> > @@ -1178,6 +1194,9 @@ static void
> > tegra_i2c_push_packet_header(struct tegra_i2c_dev *i2c_dev,
> >         if (msg->flags & I2C_M_RD)
> >                 packet_header |= I2C_HEADER_READ;
> >  
> > +       if (i2c_dev->timings.bus_freq_hz >
> > I2C_MAX_FAST_MODE_PLUS_FREQ)
> > +               packet_header |= I2C_HEADER_HS_MODE;
> > +
> 
> Do we need any kind of checking here? What happens if the requested
> frequency is higher than fastmode+ but the device doesn't support the
> new high-speed mode? I guess maybe we don't have to care because such
> cases won't ever show up because, well, they won't work.
> 
> Still, it might be a good idea to be explicit about it and have an
> extra
> check in place (or perhaps a little higher up in the call chain) and
> return an error if we're trying to use a frequency that isn't
> supported
> on the chip.
> 
> Thierry

ACK. I will add a check for this.

> 
> >         if (i2c_dev->dma_mode && !i2c_dev->msg_read)
> >                 *dma_buf++ = packet_header;
> >         else
> > @@ -1618,10 +1637,13 @@ static const struct tegra_i2c_hw_feature
> > tegra194_i2c_hw = {
> >         .thigh_std_mode = 0x7,
> >         .tlow_fast_fastplus_mode = 0x2,
> >         .thigh_fast_fastplus_mode = 0x2,
> > +       .tlow_hs_mode = 0x8,
> > +       .thigh_hs_mode = 0x3,
> >         .setup_hold_time_std_mode = 0x08080808,
> >         .setup_hold_time_fast_fast_plus_mode = 0x02020202,
> >         .setup_hold_time_hs_mode = 0x090909,
> >         .has_interface_timing_reg = true,
> > +       .has_hs_mode_support = true,
> >  };
> >  
> >  static const struct of_device_id tegra_i2c_of_match[] = {
> > -- 
> > 2.43.0
> >