i2c: designware_i2c: Add ic_enable_status to ic_regs struct
PATCH 1/6 i2c: designware_i2c: Add ic_enable_status to ic_regs struct
Add the ic_enable_status register to the ic_regs struct. Additionally the register offsets are added, to better check, if the offset matches the register description in the datasheet.
--- drivers/i2c/designware_i2c.h | 68 +++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 33 deletions(-)
drivers/i2c/designware_i2c.h +35 -33
@@ -9,39 +9,41 @@#define __DW_I2C_H_struct i2c_regs { -u32 ic_con; -u32 ic_tar; -u32 ic_sar; -u32 ic_hs_maddr; -u32 ic_cmd_data; -u32 ic_ss_scl_hcnt; -u32 ic_ss_scl_lcnt; -u32 ic_fs_scl_hcnt; -u32 ic_fs_scl_lcnt; -u32 ic_hs_scl_hcnt; -u32 ic_hs_scl_lcnt; -u32 ic_intr_stat; -u32 ic_intr_mask; -u32 ic_raw_intr_stat; -u32 ic_rx_tl; -u32 ic_tx_tl; -u32 ic_clr_intr; -u32 ic_clr_rx_under; -u32 ic_clr_rx_over; -u32 ic_clr_tx_over; -u32 ic_clr_rd_req; -u32 ic_clr_tx_abrt; -u32 ic_clr_rx_done; -u32 ic_clr_activity; -u32 ic_clr_stop_det; -u32 ic_clr_start_det; -u32 ic_clr_gen_call; -u32 ic_enable; -u32 ic_status; -u32 ic_txflr; -u32 ix_rxflr; -u32 reserved_1; -u32 ic_tx_abrt_source; +u32 ic_con;/* 0x00 */ +u32 ic_tar;/* 0x04 */ +u32 ic_sar;/* 0x08 */ +u32 ic_hs_maddr;/* 0x0c */ +u32 ic_cmd_data;/* 0x10 */ +u32 ic_ss_scl_hcnt;/* 0x14 */ +u32 ic_ss_scl_lcnt;/* 0x18 */ +u32 ic_fs_scl_hcnt;/* 0x1c */ +u32 ic_fs_scl_lcnt;/* 0x20 */ +u32 ic_hs_scl_hcnt;/* 0x24 */ +u32 ic_hs_scl_lcnt;/* 0x28 */ +u32 ic_intr_stat;/* 0x2c */ +u32 ic_intr_mask;/* 0x30 */ +u32 ic_raw_intr_stat;/* 0x34 */ +u32 ic_rx_tl;/* 0x38 */ +u32 ic_tx_tl;/* 0x3c */ +u32 ic_clr_intr;/* 0x40 */ +u32 ic_clr_rx_under;/* 0x44 */ +u32 ic_clr_rx_over;/* 0x48 */ +u32 ic_clr_tx_over;/* 0x4c */ +u32 ic_clr_rd_req;/* 0x50 */ +u32 ic_clr_tx_abrt;/* 0x54 */ +u32 ic_clr_rx_done;/* 0x58 */ +u32 ic_clr_activity;/* 0x5c */ +u32 ic_clr_stop_det;/* 0x60 */ +u32 ic_clr_start_det;/* 0x64 */ +u32 ic_clr_gen_call;/* 0x68 */ +u32 ic_enable;/* 0x6c */ +u32 ic_status;/* 0x70 */ +u32 ic_txflr;/* 0x74 */ +u32 ic_rxflr;/* 0x78 */ +u32 ic_sda_hold;/* 0x7c */ +u32 ic_tx_abrt_source;/* 0x80 */ +u8 res1[0x18 ];/* 0x84 */ +u32 ic_enable_status;/* 0x9c */ };#if !defined(IC_CLK )
Signature · 1 line
2.7.3
PATCH 2/6 i2c: designware_i2c: Add dw_i2c_enable() helper function
dw_i2c_enable() is used to dis-/en-able the I2C controller. It makes sense to add such a function, as the controller is dis-/en-abled multiple times in the code. Additionally, this function now checks, if the controller is really dis-/en-abled. This code is copied from the Linux I2C driver version.
--- drivers/i2c/designware_i2c.c | 46 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 20 deletions(-)
drivers/i2c/designware_i2c.c +26 -20
@@ -34,6 +34,26 @@ static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap)return NULL ; } +static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) +{ +int timeout =100 ; + +do { +writel (enable, &i2c_base->ic_enable); +if ((readl (&i2c_base->ic_enable_status) &1 ) == enable) +return ; + +/* +* Wait 10 times the signaling period of the highest I2C +* transfer supported by the driver (for 400KHz this is +* 25us) as described in the DesignWare I2C databook. +*/ +udelay (25 ); + }while (timeout--); + +printf ("timeout in %sabling I2C adapter\n" , enable ?"en" :"dis" ); +} +/* * set_speed - Set the i2c speed mode (standard, high, fast) * @i2c_spd: required i2c speed mode
@@ -45,12 +65,9 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt; -unsigned int enbl;/* to set speed cltr must be disabled */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 ); cntl = (readl (&i2c_base->ic_con) & (~IC_CON_SPD_MSK ));
@@ -84,8 +101,7 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)writel (cntl, &i2c_base->ic_con);/* Enable back i2c now speed set */ - enbl |=IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,1 ); }/*
@@ -123,12 +139,9 @@ static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) {struct i2c_regs *i2c_base =i2c_get_base (adap); -unsigned int enbl;/* Disable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con);writel (IC_RX_TL , &i2c_base->ic_rx_tl);
@@ -138,9 +151,7 @@ static void dw_i2c_init(struct i2c_adapter *adap, int speed,writel (slaveaddr, &i2c_base->ic_sar);/* Enable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl |=IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,1 ); }/*
@@ -152,19 +163,14 @@ static void dw_i2c_init(struct i2c_adapter *adap, int speed,static void i2c_setaddress (struct i2c_adapter *adap,unsigned int i2c_addr) {struct i2c_regs *i2c_base =i2c_get_base (adap); -unsigned int enbl;/* Disable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );writel (i2c_addr, &i2c_base->ic_tar);/* Enable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl |=IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,1 ); }/*
Signature · 1 line
2.7.3
PATCH 2/6 i2c: designware_i2c: Add dw_i2c_enable() helper function
Quoted 20 lines · Stefan Roese
dw_i2c_enable()is used to dis-/en-able the I2C controller. It makes sense to add such a function, as the controller is dis-/en-abled multiple times in the code. Additionally, this function now checks, if the controller is really dis-/en-abled. This code is copied from the Linux I2C driver version.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 46 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 20 deletions(-)drivers/i2c/designware_i2c.c +0
-0
drivers/i2c/designware_i2c.c static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap) · line 42
Quoted 9 lines
drivers/i2c/designware_i2c.c +6
-0@@ -34,6 +34,26 @@ static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap)return NULL ; } +static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) +{ +int timeout =100 ; + + do { + writel(enable, &i2c_base->ic_enable);
This should at least use IC_ENABLE_0B and not the boot enable.
Quoted 18 lines
drivers/i2c/designware_i2c.c +14
-0+if ((readl (&i2c_base->ic_enable_status) &1 ) == enable) +return ; + +/* +* Wait 10 times the signaling period of the highest I2C +* transfer supported by the driver (for 400KHz this is +* 25us) as described in the DesignWare I2C databook. +*/ +udelay (25 ); + }while (timeout--); + +printf ("timeout in %sabling I2C adapter\n" , enable ?"en" :"dis" ); +} +/* * set_speed - Set the i2c speed mode (standard, high, fast) * @i2c_spd: required i2c speed mode
drivers/i2c/designware_i2c.c static void set_speed(struct i2c_adapter *adap, int i2c_spd) · line 70
Quoted 10 lines
drivers/i2c/designware_i2c.c +1
-4@@ -45,12 +65,9 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt; -unsigned int enbl;/* to set speed cltr must be disabled */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );
This and all the other places which you changed actually change the logic of the code, right ? Is that a problem ?
Best regards,
Marek Vasut
PATCH 2/6 i2c: designware_i2c: Add dw_i2c_enable() helper function
Quoted 56 lines · Marek Vasut
Quoted 20 lines · Stefan Roese
dw_i2c_enable()is used to dis-/en-able the I2C controller. It makes sense to add such a function, as the controller is dis-/en-abled multiple times in the code. Additionally, this function now checks, if the controller is really dis-/en-abled. This code is copied from the Linux I2C driver version.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 46 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 20 deletions(-)drivers/i2c/designware_i2c.c +0
-0drivers/i2c/designware_i2c.c static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap) · line 42
Quoted 9 lines
drivers/i2c/designware_i2c.c +6
-0@@ -34,6 +34,26 @@ static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap)return NULL ; } +static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) +{ +int timeout =100 ; + + do { + writel(enable, &i2c_base->ic_enable);This should at least use
IC_ENABLE_0Band not the boot enable.Quoted 23 lines
drivers/i2c/designware_i2c.c +14
-1+if ((readl (&i2c_base->ic_enable_status) &1 ) == enable) +return ; + +/* +* Wait 10 times the signaling period of the highest I2C +* transfer supported by the driver (for 400KHz this is +* 25us) as described in the DesignWare I2C databook. +*/ +udelay (25 ); + }while (timeout--); + +printf ("timeout in %sabling I2C adapter\n" , enable ?"en" :"dis" ); +} +/* * set_speed - Set the i2c speed mode (standard, high, fast) * @i2c_spd: required i2c speed mode @@ -45,12 +65,9 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt; -unsigned int enbl;
Quoted 8 lines
drivers/i2c/designware_i2c.c static void set_speed(struct i2c_adapter *adap, int i2c_spd) · line 70
Quoted 5 lines
drivers/i2c/designware_i2c.c +1
-3/* to set speed cltr must be disabled */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );This and all the other places which you changed actually change the logic of the code, right ? Is that a problem ?
It is a functional change, yes. With a now added check, if the controller is actually getting enabled or disabled. The code is taken from the Linux kernel, as noted in the commit text. And I've tested this code on SoCFPGA without any issues so far.
Additional testing would be very welcome though. ;)
Thanks,
Stefan
PATCH 2/6 i2c: designware_i2c: Add dw_i2c_enable() helper function
Quoted 69 lines · Stefan Roese
Quoted 59 lines · Marek Vasut
Quoted 21 lines · Stefan Roese
dw_i2c_enable()is used to dis-/en-able the I2C controller. It makes sense to add such a function, as the controller is dis-/en-abled multiple times in the code. Additionally, this function now checks, if the controller is really dis-/en-abled. This code is copied from the Linux I2C driver version.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 46 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 20 deletions(-)drivers/i2c/designware_i2c.c +0
-0drivers/i2c/designware_i2c.c static struct i2c_regs *i2c_get_base(struct · line 43
Quoted 10 lines
drivers/i2c/designware_i2c.c +6
-0@@ -34,6 +34,26 @@ static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap)return NULL ; } +static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) +{ +int timeout =100 ; + + do { + writel(enable, &i2c_base->ic_enable);This should at least use
IC_ENABLE_0Band not the boot enable.Quoted 24 lines
drivers/i2c/designware_i2c.c +14
-0+if ((readl (&i2c_base->ic_enable_status) &1 ) == enable) +return ; + +/* +* Wait 10 times the signaling period of the highest I2C +* transfer supported by the driver (for 400KHz this is +* 25us) as described in the DesignWare I2C databook. +*/ +udelay (25 ); + }while (timeout--); + +printf ("timeout in %sabling I2C adapter\n" , enable ?"en" :"dis" ); +} +/* * set_speed - Set the i2c speed mode (standard, high, fast) * @i2c_spd: required i2c speed mode
drivers/i2c/designware_i2c.c +0
-1@@ -45,12 +65,9 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt; -unsigned int enbl;Quoted 8 lines
drivers/i2c/designware_i2c.c static void set_speed(struct i2c_adapter *adap, int i2c_spd) · line 70
Quoted 5 lines
drivers/i2c/designware_i2c.c +1
-3/* to set speed cltr must be disabled */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );This and all the other places which you changed actually change the logic of the code, right ? Is that a problem ?
Quoted 6 lines
It is a functional change, yes. With a now added check, if the controller is actually getting enabled or disabled. The code is taken from the Linux kernel, as noted in the commit text. And I've tested this code on SoCFPGA without any issues so far.
Additional testing would be very welcome though. ;)
I will have a board ready for mainlining that uses i2c, so if something breaks, I will cry ;-)
Signature · 2 lines
Best regards, Marek Vasut
PATCH 2/6 i2c: designware_i2c: Add dw_i2c_enable() helper function
In reply to Stefan RoeseHi Stefan,
Quoted 47 lines · Stefan Roese
dw_i2c_enable()is used to dis-/en-able the I2C controller. It makes sense to add such a function, as the controller is dis-/en-abled multiple times in the code. Additionally, this function now checks, if the controller is really dis-/en-abled. This code is copied from the Linux I2C driver version.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 46 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 20 deletions(-)drivers/i2c/designware_i2c.c +20
-0@@ -34,6 +34,26 @@ static struct i2c_regs *i2c_get_base(struct i2c_adapter *adap)return NULL ; } +static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) +{ +int timeout =100 ; + +do { +writel (enable, &i2c_base->ic_enable); +if ((readl (&i2c_base->ic_enable_status) &1 ) == enable) +return ; + +/* +* Wait 10 times the signaling period of the highest I2C +* transfer supported by the driver (for 400KHz this is +* 25us) as described in the DesignWare I2C databook. +*/ +udelay (25 ); + }while (timeout--); + +printf ("timeout in %sabling I2C adapter\n" , enable ?"en" :"dis" ); +} +/* * set_speed - Set the i2c speed mode (standard, high, fast) * @i2c_spd: required i2c speed mode
drivers/i2c/designware_i2c.c static void set_speed(struct i2c_adapter *adap, int i2c_spd) · line 70
Quoted 10 lines
drivers/i2c/designware_i2c.c +1
-4@@ -45,12 +65,9 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt; -unsigned int enbl;/* to set speed cltr must be disabled */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );
nits: change 0 to false since the parameter type is bool?
drivers/i2c/designware_i2c.c static void set_speed(struct i2c_adapter *adap, int i2c_spd) · line 104
Quoted 10 lines
drivers/i2c/designware_i2c.c +1
-2cntl = (readl (&i2c_base->ic_con) & (~IC_CON_SPD_MSK ));@@ -84,8 +101,7 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)writel (cntl, &i2c_base->ic_con);/* Enable back i2c now speed set */ - enbl |=IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,1 );
ditto, 1-> true?
Quoted 4 lines
drivers/i2c/designware_i2c.c +0
-0}/*
drivers/i2c/designware_i2c.c static void dw_i2c_init(struct i2c_adapter *adap, int speed, · line 144
Quoted 10 lines
drivers/i2c/designware_i2c.c +1
-4@@ -123,12 +139,9 @@ static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) {struct i2c_regs *i2c_base =i2c_get_base (adap); -unsigned int enbl;/* Disable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );
ditto
drivers/i2c/designware_i2c.c static void dw_i2c_init(struct i2c_adapter *adap, int speed, · line 154
Quoted 11 lines
drivers/i2c/designware_i2c.c +1
-3writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con);writel (IC_RX_TL , &i2c_base->ic_rx_tl);@@ -138,9 +151,7 @@ static void dw_i2c_init(struct i2c_adapter *adap, int speed,writel (slaveaddr, &i2c_base->ic_sar);/* Enable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl |=IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,1 );
ditto
Quoted 14 lines
drivers/i2c/designware_i2c.c +1
-4}/* @@ -152,19 +163,14 @@ static void dw_i2c_init(struct i2c_adapter *adap, int speed,static void i2c_setaddress (struct i2c_adapter *adap,unsigned int i2c_addr) {struct i2c_regs *i2c_base =i2c_get_base (adap); -unsigned int enbl;/* Disable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl &= ~IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,0 );
drivers/i2c/designware_i2c.c static void dw_i2c_init(struct i2c_adapter *adap, int speed, · line 175
Quoted 10 lines
drivers/i2c/designware_i2c.c +1
-3writel (i2c_addr, &i2c_base->ic_tar);/* Enable i2c */ - enbl =readl (&i2c_base->ic_enable); - enbl |=IC_ENABLE_0B ; -writel (enbl, &i2c_base->ic_enable); +dw_i2c_enable (i2c_base,1 ); }
Regards,
Bin
PATCH 3/6 i2c: designware_i2c: Integrate set_speed() into dw_i2c_set_bus_speed()
In reply to Stefan RoeseIntegrating set_speed() into dw_i2c_set_bus_speed() will make the conversion to DM easier for this driver.
--- drivers/i2c/designware_i2c.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)
drivers/i2c/designware_i2c.c +13 -24
@@ -55,16 +55,25 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) }/* -* set_speed - Set the i2c speed mode (standard, high, fast) -* @i2c_spd: required i2c speed mode +* i2c_set_bus_speed - Set the i2c speed +* @speed: required i2c speed * -* Set the i2c speed mode (standard, high, fast) +* Set the i2c speed. */ -static void set_speed (struct i2c_adapter *adap,int i2c_spd) +static unsigned int dw_i2c_set_bus_speed (struct i2c_adapter *adap, +unsigned int speed) {struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt; +int i2c_spd; + +if (speed >=I2C_MAX_SPEED ) + i2c_spd =IC_SPEED_MODE_MAX ; +else if (speed >=I2C_FAST_SPEED ) + i2c_spd =IC_SPEED_MODE_FAST ; +else + i2c_spd =IC_SPEED_MODE_STANDARD ;/* to set speed cltr must be disabled */ dw_i2c_enable (i2c_base,0 );
@@ -102,27 +111,7 @@ static void set_speed(struct i2c_adapter *adap, int i2c_spd)/* Enable back i2c now speed set */ dw_i2c_enable (i2c_base,1 ); -} - -/* -* i2c_set_bus_speed - Set the i2c speed -* @speed: required i2c speed -* -* Set the i2c speed. -*/ -static unsigned int dw_i2c_set_bus_speed (struct i2c_adapter *adap, -unsigned int speed) -{ -int i2c_spd; - -if (speed >=I2C_MAX_SPEED ) - i2c_spd =IC_SPEED_MODE_MAX ; -else if (speed >=I2C_FAST_SPEED ) - i2c_spd =IC_SPEED_MODE_FAST ; -else - i2c_spd =IC_SPEED_MODE_STANDARD ; -set_speed (adap, i2c_spd); adap->speed = speed;return 0 ;
Signature · 1 line
2.7.3
PATCH 3/6 i2c: designware_i2c: Integrate set_speed() into dw_i2c_set_bus_speed()
Quoted 8 lines · Stefan Roese
Integrating
set_speed()intodw_i2c_set_bus_speed()will make the conversion to DM easier for this driver.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>
Signature · 2 lines
Best regards, Marek Vasut
PATCH 3/6 i2c: designware_i2c: Integrate set_speed() into dw_i2c_set_bus_speed()
In reply to Stefan RoeseQuoted 12 lines · Stefan Roese
Integrating
set_speed()intodw_i2c_set_bus_speed()will make the conversion to DM easier for this driver.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)
PATCH 4/6 i2c: designware_i2c: Prepare for DM driver conversion
In reply to Stefan RoeseThis patch prepares the designware I2C driver for the DM conversion. This is mainly done by removing struct i2c_adapter from the functions that shall be used by the DM driver version as well.
--- drivers/i2c/designware_i2c.c | 173 ++++++++++++++++++++++--------------------- 1 file changed, 90 insertions(+), 83 deletions(-)
drivers/i2c/designware_i2c.c +90 -83
@@ -10,30 +10,6 @@#include <asm/io.h> #include "designware_i2c.h" -static struct i2c_regs *i2c_get_base (struct i2c_adapter *adap) -{ -switch (adap->hwadapnr) { -#if CONFIG_SYS_I2C_BUS_MAX >=4 -case 3 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE3 ; -#endif -#if CONFIG_SYS_I2C_BUS_MAX >=3 -case 2 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE2 ; -#endif -#if CONFIG_SYS_I2C_BUS_MAX >=2 -case 1 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE1 ; -#endif -case 0 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE ; -default : -printf ("Wrong I2C-adapter number %d\n" , adap->hwadapnr); - } - -return NULL ; -} -static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) {int timeout =100 ;
@@ -60,10 +36,9 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)* * Set the i2c speed. */ -static unsigned int dw_i2c_set_bus_speed (struct i2c_adapter *adap, -unsigned int speed) +static unsigned int __dw_i2c_set_bus_speed (struct i2c_regs *i2c_base, +unsigned int speed) { -struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt;int i2c_spd;
@@ -112,47 +87,17 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap,/* Enable back i2c now speed set */ dw_i2c_enable (i2c_base,1 ); - adap->speed = speed; -return 0 ; }/* -* i2c_init - Init function -* @speed: required i2c speed -* @slaveaddr: slave address for the device -* -* Initialization function. -*/ -static void dw_i2c_init (struct i2c_adapter *adap,int speed, -int slaveaddr) -{ -struct i2c_regs *i2c_base =i2c_get_base (adap); - -/* Disable i2c */ -dw_i2c_enable (i2c_base,0 ); - -writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con); -writel (IC_RX_TL , &i2c_base->ic_rx_tl); -writel (IC_TX_TL , &i2c_base->ic_tx_tl); -dw_i2c_set_bus_speed (adap, speed); -writel (IC_STOP_DET , &i2c_base->ic_intr_mask); -writel (slaveaddr, &i2c_base->ic_sar); - -/* Enable i2c */ -dw_i2c_enable (i2c_base,1 ); -} - -/* * i2c_setaddress - Sets the target slave address * @i2c_addr: target i2c address * * Sets the target slave address. */ -static void i2c_setaddress (struct i2c_adapter *adap,unsigned int i2c_addr) +static void i2c_setaddress (struct i2c_regs *i2c_base,unsigned int i2c_addr) { -struct i2c_regs *i2c_base =i2c_get_base (adap); -/* Disable i2c */ dw_i2c_enable (i2c_base,0 );
@@ -167,10 +112,8 @@ static void i2c_setaddress(struct i2c_adapter *adap, unsigned int i2c_addr)* * Flushes the i2c RX FIFO */ -static void i2c_flush_rxfifo (struct i2c_adapter *adap) +static void i2c_flush_rxfifo (struct i2c_regs *i2c_base) { -struct i2c_regs *i2c_base =i2c_get_base (adap); -while (readl (&i2c_base->ic_status) &IC_STATUS_RFNE )readl (&i2c_base->ic_cmd_data); }
@@ -180,9 +123,8 @@ static void i2c_flush_rxfifo(struct i2c_adapter *adap)* * Waits for bus busy */ -static int i2c_wait_for_bb (struct i2c_adapter *adap) +static int i2c_wait_for_bb (struct i2c_regs *i2c_base) { -struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned long start_time_bb =get_timer (0 ); while ((readl(&i2c_base->ic_status) & IC_STATUS_MA) ||
@@ -196,15 +138,13 @@ static int i2c_wait_for_bb(struct i2c_adapter *adap)return 0 ; } -static int i2c_xfer_init (struct i2c_adapter *adap,uchar chip,uint addr, +static int i2c_xfer_init (struct i2c_regs *i2c_base,uchar chip,uint addr,int alen) { -struct i2c_regs *i2c_base =i2c_get_base (adap); - -if (i2c_wait_for_bb (adap)) +if (i2c_wait_for_bb (i2c_base))return 1 ; -i2c_setaddress (adap, chip); +i2c_setaddress (i2c_base, chip);while (alen) { alen--;/* high byte address going out first */
@@ -214,9 +154,8 @@ static int i2c_xfer_init(struct i2c_adapter *adap, uchar chip, uint addr,return 0 ; } -static int i2c_xfer_finish (struct i2c_adapter *adap) +static int i2c_xfer_finish (struct i2c_regs *i2c_base) { -struct i2c_regs *i2c_base =i2c_get_base (adap);ulong start_stop_det =get_timer (0 );while (1 ) {
@@ -228,12 +167,12 @@ static int i2c_xfer_finish(struct i2c_adapter *adap) } } -if (i2c_wait_for_bb (adap)) { +if (i2c_wait_for_bb (i2c_base)) {printf ("Timed out waiting for bus\n" );return 1 ; } -i2c_flush_rxfifo (adap); +i2c_flush_rxfifo (i2c_base);return 0 ; }
@@ -248,10 +187,9 @@ static int i2c_xfer_finish(struct i2c_adapter *adap)* * Read from i2c memory. */ -static int dw_i2c_read (struct i2c_adapter *adap,u8 dev,uint addr, -int alen,u8 *buffer,int len) +static int __dw_i2c_read (struct i2c_regs *i2c_base,u8 dev,uint addr, +int alen,u8 *buffer,int len) { -struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned long start_time_rx;#ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW
@@ -273,7 +211,7 @@ static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr, addr);#endif -if (i2c_xfer_init (adap, dev, addr, alen)) +if (i2c_xfer_init (i2c_base, dev, addr, alen))return 1 ; start_time_rx =get_timer (0 );
@@ -293,7 +231,7 @@ static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr, } } -return i2c_xfer_finish (adap); +return i2c_xfer_finish (i2c_base); }/*
@@ -306,10 +244,9 @@ static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr,* * Write to i2c memory. */ -static int dw_i2c_write (struct i2c_adapter *adap,u8 dev,uint addr, -int alen,u8 *buffer,int len) +static int __dw_i2c_write (struct i2c_regs *i2c_base,u8 dev,uint addr, +int alen,u8 *buffer,int len) { -struct i2c_regs *i2c_base =i2c_get_base (adap);int nb = len;unsigned long start_time_tx;
@@ -332,7 +269,7 @@ static int dw_i2c_write(struct i2c_adapter *adap, u8 dev, uint addr, addr);#endif -if (i2c_xfer_init (adap, dev, addr, alen)) +if (i2c_xfer_init (i2c_base, dev, addr, alen))return 1 ; start_time_tx =get_timer (0 );
@@ -353,7 +290,76 @@ static int dw_i2c_write(struct i2c_adapter *adap, u8 dev, uint addr, } } -return i2c_xfer_finish (adap); +return i2c_xfer_finish (i2c_base); +} + +static struct i2c_regs *i2c_get_base (struct i2c_adapter *adap) +{ +switch (adap->hwadapnr) { +#if CONFIG_SYS_I2C_BUS_MAX >=4 +case 3 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE3 ; +#endif +#if CONFIG_SYS_I2C_BUS_MAX >=3 +case 2 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE2 ; +#endif +#if CONFIG_SYS_I2C_BUS_MAX >=2 +case 1 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE1 ; +#endif +case 0 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE ; +default : +printf ("Wrong I2C-adapter number %d\n" , adap->hwadapnr); + } + +return NULL ; +} + +static unsigned int dw_i2c_set_bus_speed (struct i2c_adapter *adap, +unsigned int speed) +{ + adap->speed = speed; +return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); +} + +/* +* i2c_init - Init function +* @speed: required i2c speed +* @slaveaddr: slave address for the device +* +* Initialization function. +*/ +static void dw_i2c_init (struct i2c_adapter *adap,int speed, +int slaveaddr) +{ +struct i2c_regs *i2c_base =i2c_get_base (adap); + +/* Disable i2c */ +dw_i2c_enable (i2c_base,0 ); + +writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con); +writel (IC_RX_TL , &i2c_base->ic_rx_tl); +writel (IC_TX_TL , &i2c_base->ic_tx_tl); +dw_i2c_set_bus_speed (adap, speed); +writel (IC_STOP_DET , &i2c_base->ic_intr_mask); +writel (slaveaddr, &i2c_base->ic_sar); + +/* Enable i2c */ +dw_i2c_enable (i2c_base,1 ); +} + +static int dw_i2c_read (struct i2c_adapter *adap,u8 dev,uint addr, +int alen,u8 *buffer,int len) +{ +return __dw_i2c_read (i2c_get_base (adap), dev, addr, alen, buffer, len); +} + +static int dw_i2c_write (struct i2c_adapter *adap,u8 dev,uint addr, +int alen,u8 *buffer,int len) +{ +return __dw_i2c_write (i2c_get_base (adap), dev, addr, alen, buffer, len); }/*
@@ -361,13 +367,14 @@ static int dw_i2c_write(struct i2c_adapter *adap, u8 dev, uint addr,*/ static int dw_i2c_probe (struct i2c_adapter *adap,u8 dev) { +struct i2c_regs *i2c_base =i2c_get_base (adap);u32 tmp;int ret;/* * Try to read the first location of the chip. */ - ret =dw_i2c_read (adap, dev,0 ,1 , (uchar *)&tmp,1 ); + ret =__dw_i2c_read (i2c_base, dev,0 ,1 , (uchar *)&tmp,1 );if (ret)dw_i2c_init (adap, adap->speed, adap->slaveaddr);
Signature · 1 line
2.7.3
PATCH 4/6 i2c: designware_i2c: Prepare for DM driver conversion
Quoted 262 lines · Stefan Roese
This patch prepares the designware I2C driver for the DM conversion. This is mainly done by removing struct
i2c_adapterfrom the functions that shall be used by the DM driver version as well.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 173 ++++++++++++++++++++++--------------------- 1 file changed, 90 insertions(+), 83 deletions(-)drivers/i2c/designware_i2c.c +44
-82@@ -10,30 +10,6 @@#include <asm/io.h> #include "designware_i2c.h" -static struct i2c_regs *i2c_get_base (struct i2c_adapter *adap) -{ -switch (adap->hwadapnr) { -#if CONFIG_SYS_I2C_BUS_MAX >=4 -case 3 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE3 ; -#endif -#if CONFIG_SYS_I2C_BUS_MAX >=3 -case 2 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE2 ; -#endif -#if CONFIG_SYS_I2C_BUS_MAX >=2 -case 1 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE1 ; -#endif -case 0 : -return (struct i2c_regs *)CONFIG_SYS_I2C_BASE ; -default : -printf ("Wrong I2C-adapter number %d\n" , adap->hwadapnr); - } - -return NULL ; -} -static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) {int timeout =100 ;@@ -60,10 +36,9 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)* * Set the i2c speed. */ -static unsigned int dw_i2c_set_bus_speed (struct i2c_adapter *adap, -unsigned int speed) +static unsigned int __dw_i2c_set_bus_speed (struct i2c_regs *i2c_base, +unsigned int speed) { -struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned int cntl;unsigned int hcnt, lcnt;int i2c_spd;@@ -112,47 +87,17 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap,/* Enable back i2c now speed set */ dw_i2c_enable (i2c_base,1 ); - adap->speed = speed; -return 0 ; }/* -* i2c_init - Init function -* @speed: required i2c speed -* @slaveaddr: slave address for the device -* -* Initialization function. -*/ -static void dw_i2c_init (struct i2c_adapter *adap,int speed, -int slaveaddr) -{ -struct i2c_regs *i2c_base =i2c_get_base (adap); - -/* Disable i2c */ -dw_i2c_enable (i2c_base,0 ); - -writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con); -writel (IC_RX_TL , &i2c_base->ic_rx_tl); -writel (IC_TX_TL , &i2c_base->ic_tx_tl); -dw_i2c_set_bus_speed (adap, speed); -writel (IC_STOP_DET , &i2c_base->ic_intr_mask); -writel (slaveaddr, &i2c_base->ic_sar); - -/* Enable i2c */ -dw_i2c_enable (i2c_base,1 ); -} - -/* * i2c_setaddress - Sets the target slave address * @i2c_addr: target i2c address * * Sets the target slave address. */ -static void i2c_setaddress (struct i2c_adapter *adap,unsigned int i2c_addr) +static void i2c_setaddress (struct i2c_regs *i2c_base,unsigned int i2c_addr) { -struct i2c_regs *i2c_base =i2c_get_base (adap); -/* Disable i2c */ dw_i2c_enable (i2c_base,0 );@@ -167,10 +112,8 @@ static void i2c_setaddress(struct i2c_adapter *adap, unsigned int i2c_addr)* * Flushes the i2c RX FIFO */ -static void i2c_flush_rxfifo (struct i2c_adapter *adap) +static void i2c_flush_rxfifo (struct i2c_regs *i2c_base) { -struct i2c_regs *i2c_base =i2c_get_base (adap); -while (readl (&i2c_base->ic_status) &IC_STATUS_RFNE )readl (&i2c_base->ic_cmd_data); }@@ -180,9 +123,8 @@ static void i2c_flush_rxfifo(struct i2c_adapter *adap)* * Waits for bus busy */ -static int i2c_wait_for_bb (struct i2c_adapter *adap) +static int i2c_wait_for_bb (struct i2c_regs *i2c_base) { -struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned long start_time_bb =get_timer (0 ); while ((readl(&i2c_base->ic_status) & IC_STATUS_MA) ||@@ -196,15 +138,13 @@ static int i2c_wait_for_bb(struct i2c_adapter *adap)return 0 ; } -static int i2c_xfer_init (struct i2c_adapter *adap,uchar chip,uint addr, +static int i2c_xfer_init (struct i2c_regs *i2c_base,uchar chip,uint addr,int alen) { -struct i2c_regs *i2c_base =i2c_get_base (adap); - -if (i2c_wait_for_bb (adap)) +if (i2c_wait_for_bb (i2c_base))return 1 ; -i2c_setaddress (adap, chip); +i2c_setaddress (i2c_base, chip);while (alen) { alen--;/* high byte address going out first */ @@ -214,9 +154,8 @@ static int i2c_xfer_init(struct i2c_adapter *adap, uchar chip, uint addr,return 0 ; } -static int i2c_xfer_finish (struct i2c_adapter *adap) +static int i2c_xfer_finish (struct i2c_regs *i2c_base) { -struct i2c_regs *i2c_base =i2c_get_base (adap);ulong start_stop_det =get_timer (0 );while (1 ) {@@ -228,12 +167,12 @@ static int i2c_xfer_finish(struct i2c_adapter *adap) } } -if (i2c_wait_for_bb (adap)) { +if (i2c_wait_for_bb (i2c_base)) {printf ("Timed out waiting for bus\n" );return 1 ; } -i2c_flush_rxfifo (adap); +i2c_flush_rxfifo (i2c_base);return 0 ; }@@ -248,10 +187,9 @@ static int i2c_xfer_finish(struct i2c_adapter *adap)* * Read from i2c memory. */ -static int dw_i2c_read (struct i2c_adapter *adap,u8 dev,uint addr, -int alen,u8 *buffer,int len) +static int __dw_i2c_read (struct i2c_regs *i2c_base,u8 dev,uint addr, +int alen,u8 *buffer,int len) { -struct i2c_regs *i2c_base =i2c_get_base (adap);unsigned long start_time_rx;#ifdef CONFIG_SYS_I2C_EEPROM_ADDR_OVERFLOW @@ -273,7 +211,7 @@ static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr, addr);#endif -if (i2c_xfer_init (adap, dev, addr, alen)) +if (i2c_xfer_init (i2c_base, dev, addr, alen))return 1 ; start_time_rx =get_timer (0 );@@ -293,7 +231,7 @@ static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr, } } -return i2c_xfer_finish (adap); +return i2c_xfer_finish (i2c_base); }/* @@ -306,10 +244,9 @@ static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr,* * Write to i2c memory. */ -static int dw_i2c_write (struct i2c_adapter *adap,u8 dev,uint addr, -int alen,u8 *buffer,int len) +static int __dw_i2c_write (struct i2c_regs *i2c_base,u8 dev,uint addr, +int alen,u8 *buffer,int len) { -struct i2c_regs *i2c_base =i2c_get_base (adap);int nb = len;unsigned long start_time_tx;@@ -332,7 +269,7 @@ static int dw_i2c_write(struct i2c_adapter *adap, u8 dev, uint addr, addr);#endif -if (i2c_xfer_init (adap, dev, addr, alen)) +if (i2c_xfer_init (i2c_base, dev, addr, alen))return 1 ; start_time_tx =get_timer (0 );@@ -353,7 +290,76 @@ static int dw_i2c_write(struct i2c_adapter *adap, u8 dev, uint addr, } } -return i2c_xfer_finish (adap); +return i2c_xfer_finish (i2c_base); +} + +static struct i2c_regs *i2c_get_base (struct i2c_adapter *adap) +{ +switch (adap->hwadapnr) { +#if CONFIG_SYS_I2C_BUS_MAX >=4 +case 3 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE3 ; +#endif +#if CONFIG_SYS_I2C_BUS_MAX >=3 +case 2 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE2 ; +#endif +#if CONFIG_SYS_I2C_BUS_MAX >=2 +case 1 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE1 ; +#endif +case 0 : +return (struct i2c_regs *)CONFIG_SYS_I2C_BASE ; +default : +printf ("Wrong I2C-adapter number %d\n" , adap->hwadapnr); + } + +return NULL ; +}
drivers/i2c/designware_i2c.c static int dw_i2c_write(struct i2c_adapter *adap, u8 dev, uint addr, · line 328
Quoted 10 lines
drivers/i2c/designware_i2c.c +10
-0+ +static unsigned int dw_i2c_set_bus_speed (struct i2c_adapter *adap, +unsigned int speed) +{ + adap->speed = speed; +return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); +} + +/* +* i2c_init - Init function
dw_i2c_init
Quoted 14 lines
drivers/i2c/designware_i2c.c +14
-0+* @speed: required i2c speed +* @slaveaddr: slave address for the device +* +* Initialization function. +*/ +static void dw_i2c_init (struct i2c_adapter *adap,int speed, +int slaveaddr) +{ +struct i2c_regs *i2c_base =i2c_get_base (adap); + +/* Disable i2c */ +dw_i2c_enable (i2c_base,0 ); + +writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con);
drivers/i2c/designware_i2c.c static int dw_i2c_write(struct i2c_adapter *adap, u8 dev, uint addr, · line 352
Quoted 10 lines
drivers/i2c/designware_i2c.c +10
-0+writel (IC_RX_TL , &i2c_base->ic_rx_tl); +writel (IC_TX_TL , &i2c_base->ic_tx_tl); +dw_i2c_set_bus_speed (adap, speed); +writel (IC_STOP_DET , &i2c_base->ic_intr_mask); +writel (slaveaddr, &i2c_base->ic_sar); + +/* Enable i2c */ +dw_i2c_enable (i2c_base,1 ); +} +
PATCH 5/6 i2c: designware_i2c: Add DM support
In reply to Stefan RoeseThis patch adds DM support to the designware I2C driver. It currently supports DM and the legacy I2C support. The legacy support should be removed, once all platforms using it have DM enabled.
--- drivers/i2c/designware_i2c.c | 149 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 126 insertions(+), 23 deletions(-)
drivers/i2c/designware_i2c.c +126 -23
@@ -6,10 +6,15 @@*/ #include <common.h> +#include <dm.h> #include <i2c.h> #include <asm/io.h> #include "designware_i2c.h" +struct dw_i2c { +struct i2c_regs *regs; +}; +static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) {int timeout =100 ;
@@ -293,6 +298,36 @@ static int __dw_i2c_write(struct i2c_regs *i2c_base, u8 dev, uint addr,return i2c_xfer_finish (i2c_base); } +/* +* i2c_init - Init function +* @speed: required i2c speed +* @slaveaddr: slave address for the device +* +* Initialization function. +*/ +static void __dw_i2c_init (struct i2c_regs *i2c_base,int speed,int slaveaddr) +{ +/* Disable i2c */ +dw_i2c_enable (i2c_base,0 ); + +writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con); +writel (IC_RX_TL , &i2c_base->ic_rx_tl); +writel (IC_TX_TL , &i2c_base->ic_tx_tl); +writel (IC_STOP_DET , &i2c_base->ic_intr_mask); +#ifndef CONFIG_DM_I2C +__dw_i2c_set_bus_speed (i2c_base, speed); +writel (slaveaddr, &i2c_base->ic_sar); +#endif + +/* Enable i2c */ +dw_i2c_enable (i2c_base,1 ); +} + +#ifndef CONFIG_DM_I2C +/* +* The legacy I2C functions. These need to get removed once +* all users of this driver are converted to DM. +*/ static struct i2c_regs *i2c_get_base (struct i2c_adapter *adap) {switch (adap->hwadapnr) {
@@ -324,30 +359,9 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap,return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); } -/* -* i2c_init - Init function -* @speed: required i2c speed -* @slaveaddr: slave address for the device -* -* Initialization function. -*/ -static void dw_i2c_init (struct i2c_adapter *adap,int speed, -int slaveaddr) +static void dw_i2c_init (struct i2c_adapter *adap,int speed,int slaveaddr) { -struct i2c_regs *i2c_base =i2c_get_base (adap); - -/* Disable i2c */ -dw_i2c_enable (i2c_base,0 ); - -writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con); -writel (IC_RX_TL , &i2c_base->ic_rx_tl); -writel (IC_TX_TL , &i2c_base->ic_tx_tl); -dw_i2c_set_bus_speed (adap, speed); -writel (IC_STOP_DET , &i2c_base->ic_intr_mask); -writel (slaveaddr, &i2c_base->ic_sar); - -/* Enable i2c */ -dw_i2c_enable (i2c_base,1 ); +__dw_i2c_init (i2c_get_base (adap), speed, slaveaddr); } static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr,
@@ -402,3 +416,92 @@ U_BOOT_I2C_ADAP_COMPLETE(dw_3, dw_i2c_init, dw_i2c_probe, dw_i2c_read, dw_i2c_write, dw_i2c_set_bus_speed, CONFIG_SYS_I2C_SPEED3, CONFIG_SYS_I2C_SLAVE3, 3)#endif + +#else /* CONFIG_DM_I2C */ +/* +* The DM I2C functions +*/ + +static int designware_i2c_xfer (struct udevice *bus,struct i2c_msg *msg, +int nmsgs) +{ +struct dw_i2c *i2c =dev_get_priv (bus); +int ret; + +debug ("i2c_xfer: %d messages\n" , nmsgs); +for (; nmsgs >0 ; nmsgs--, msg++) { +debug ("i2c_xfer: chip=0x%x, len=0x%x\n" , msg->addr, msg->len); +if (msg->flags &I2C_M_RD ) { + ret =__dw_i2c_read (i2c->regs, msg->addr,0 ,0 , + msg->buf, msg->len); + }else { + ret =__dw_i2c_write (i2c->regs, msg->addr,0 ,0 , + msg->buf, msg->len); + } +if (ret) { +debug ("i2c_write: error sending\n" ); +return -EREMOTEIO ; + } + } + +return 0 ; +} + +static int designware_i2c_set_bus_speed (struct udevice *bus,unsigned int speed) +{ +struct dw_i2c *i2c =dev_get_priv (bus); + +return __dw_i2c_set_bus_speed (i2c->regs, speed); +} + +static int designware_i2c_probe_chip (struct udevice *bus,uint chip_addr, +uint chip_flags) +{ +struct dw_i2c *i2c =dev_get_priv (bus); +struct i2c_regs *i2c_base = i2c->regs; +u32 tmp; +int ret; + +/* +* Try to read the first location of the chip. +*/ + ret =__dw_i2c_read (i2c_base, chip_addr,0 ,1 , (uchar *)&tmp,1 ); +if (ret) +__dw_i2c_init (i2c_base,0 ,0 ); + +return ret; +} + +static int designware_i2c_probe (struct udevice *bus) +{ +struct dw_i2c *priv =dev_get_priv (bus); + +/* Save base address from device-tree */ + priv->regs = (struct i2c_regs *)dev_get_addr (bus); + +__dw_i2c_init (priv->regs,0 ,0 ); + +return 0 ; +} + +static const struct dm_i2c_ops designware_i2c_ops = { + .xfer = designware_i2c_xfer, + .probe_chip = designware_i2c_probe_chip, + .set_bus_speed = designware_i2c_set_bus_speed, +}; + +static const struct udevice_id designware_i2c_ids[] = { + { .compatible ="snps,designware-i2c" }, + { } +}; + +U_BOOT_DRIVER (i2c_designware) = { + .name ="i2c_designware" , + .id =UCLASS_I2C , + .of_match = designware_i2c_ids, + .probe = designware_i2c_probe, + .priv_auto_alloc_size =sizeof (struct dw_i2c ), + .ops = &designware_i2c_ops, +}; + +#endif /* CONFIG_DM_I2C */
Signature · 1 line
2.7.3
PATCH 5/6 i2c: designware_i2c: Add DM support
Hi Stefan,
Quoted 34 lines · Stefan Roese
This patch adds DM support to the designware I2C driver. It currently supports DM and the legacy I2C support. The legacy support should be removed, once all platforms using it have DM enabled.
Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 149 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 126 insertions(+), 23 deletions(-)drivers/i2c/designware_i2c.c +5
-0@@ -6,10 +6,15 @@*/ #include <common.h> +#include <dm.h> #include <i2c.h> #include <asm/io.h> #include "designware_i2c.h" +struct dw_i2c { +struct i2c_regs *regs; +}; +static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable) {int timeout =100 ;
drivers/i2c/designware_i2c.c static int __dw_i2c_write(struct i2c_regs *i2c_base, u8 dev, uint addr, · line 302
Quoted 5 lines
drivers/i2c/designware_i2c.c +2
-0@@ -293,6 +298,36 @@ static int __dw_i2c_write(struct i2c_regs *i2c_base, u8 dev, uint addr,return i2c_xfer_finish (i2c_base); } +/* +* i2c_init - Init function
__dw_i2c_init
Quoted 65 lines
drivers/i2c/designware_i2c.c +30
-23+* @speed: required i2c speed +* @slaveaddr: slave address for the device +* +* Initialization function. +*/ +static void __dw_i2c_init (struct i2c_regs *i2c_base,int speed,int slaveaddr) +{ +/* Disable i2c */ +dw_i2c_enable (i2c_base,0 ); + +writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con); +writel (IC_RX_TL , &i2c_base->ic_rx_tl); +writel (IC_TX_TL , &i2c_base->ic_tx_tl); +writel (IC_STOP_DET , &i2c_base->ic_intr_mask); +#ifndef CONFIG_DM_I2C +__dw_i2c_set_bus_speed (i2c_base, speed); +writel (slaveaddr, &i2c_base->ic_sar); +#endif + +/* Enable i2c */ +dw_i2c_enable (i2c_base,1 ); +} + +#ifndef CONFIG_DM_I2C +/* +* The legacy I2C functions. These need to get removed once +* all users of this driver are converted to DM. +*/ static struct i2c_regs *i2c_get_base (struct i2c_adapter *adap) {switch (adap->hwadapnr) {@@ -324,30 +359,9 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap,return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); } -/* -* i2c_init - Init function -* @speed: required i2c speed -* @slaveaddr: slave address for the device -* -* Initialization function. -*/ -static void dw_i2c_init (struct i2c_adapter *adap,int speed, -int slaveaddr) +static void dw_i2c_init (struct i2c_adapter *adap,int speed,int slaveaddr) { -struct i2c_regs *i2c_base =i2c_get_base (adap); - -/* Disable i2c */ -dw_i2c_enable (i2c_base,0 ); - -writel ((IC_CON_SD |IC_CON_SPD_FS |IC_CON_MM ), &i2c_base->ic_con); -writel (IC_RX_TL , &i2c_base->ic_rx_tl); -writel (IC_TX_TL , &i2c_base->ic_tx_tl); -dw_i2c_set_bus_speed (adap, speed); -writel (IC_STOP_DET , &i2c_base->ic_intr_mask); -writel (slaveaddr, &i2c_base->ic_sar); - -/* Enable i2c */ -dw_i2c_enable (i2c_base,1 ); +__dw_i2c_init (i2c_get_base (adap), speed, slaveaddr); } static int dw_i2c_read(struct i2c_adapter *adap, u8 dev, uint addr,
drivers/i2c/designware_i2c.c U_BOOT_I2C_ADAP_COMPLETE(dw_3, dw_i2c_init, dw_i2c_probe, dw_i2c_read, · line 423
Quoted 8 lines
drivers/i2c/designware_i2c.c +5
-0@@ -402,3 +416,92 @@ U_BOOT_I2C_ADAP_COMPLETE(dw_3, dw_i2c_init, dw_i2c_probe, dw_i2c_read, dw_i2c_write, dw_i2c_set_bus_speed, CONFIG_SYS_I2C_SPEED3, CONFIG_SYS_I2C_SLAVE3, 3)#endif + +#else /* CONFIG_DM_I2C */ +/* +* The DM I2C functions +*/
nits: single line comment format
Quoted 34 lines
drivers/i2c/designware_i2c.c +34
-0+ +static int designware_i2c_xfer (struct udevice *bus,struct i2c_msg *msg, +int nmsgs) +{ +struct dw_i2c *i2c =dev_get_priv (bus); +int ret; + +debug ("i2c_xfer: %d messages\n" , nmsgs); +for (; nmsgs >0 ; nmsgs--, msg++) { +debug ("i2c_xfer: chip=0x%x, len=0x%x\n" , msg->addr, msg->len); +if (msg->flags &I2C_M_RD ) { + ret =__dw_i2c_read (i2c->regs, msg->addr,0 ,0 , + msg->buf, msg->len); + }else { + ret =__dw_i2c_write (i2c->regs, msg->addr,0 ,0 , + msg->buf, msg->len); + } +if (ret) { +debug ("i2c_write: error sending\n" ); +return -EREMOTEIO ; + } + } + +return 0 ; +} + +static int designware_i2c_set_bus_speed (struct udevice *bus,unsigned int speed) +{ +struct dw_i2c *i2c =dev_get_priv (bus); + +return __dw_i2c_set_bus_speed (i2c->regs, speed); +} + +static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr,
drivers/i2c/designware_i2c.c U_BOOT_I2C_ADAP_COMPLETE(dw_3, dw_i2c_init, dw_i2c_probe, dw_i2c_read, · line 467
Quoted 10 lines
drivers/i2c/designware_i2c.c +10
-0+ uint chip_flags) +{ +struct dw_i2c *i2c =dev_get_priv (bus); +struct i2c_regs *i2c_base = i2c->regs; +u32 tmp; +int ret; + +/* +* Try to read the first location of the chip. +*/
nits: single line comment format
Quoted 31 lines
drivers/i2c/designware_i2c.c +31
-0+ ret =__dw_i2c_read (i2c_base, chip_addr,0 ,1 , (uchar *)&tmp,1 ); +if (ret) +__dw_i2c_init (i2c_base,0 ,0 ); + +return ret; +} + +static int designware_i2c_probe (struct udevice *bus) +{ +struct dw_i2c *priv =dev_get_priv (bus); + +/* Save base address from device-tree */ + priv->regs = (struct i2c_regs *)dev_get_addr (bus); + +__dw_i2c_init (priv->regs,0 ,0 ); + +return 0 ; +} + +static const struct dm_i2c_ops designware_i2c_ops = { + .xfer = designware_i2c_xfer, + .probe_chip = designware_i2c_probe_chip, + .set_bus_speed = designware_i2c_set_bus_speed, +}; + +static const struct udevice_id designware_i2c_ids[] = { + { .compatible ="snps,designware-i2c" }, + { } +}; + +U_BOOT_DRIVER (i2c_designware) = {
drivers/i2c/designware_i2c.c U_BOOT_I2C_ADAP_COMPLETE(dw_3, dw_i2c_init, dw_i2c_probe, dw_i2c_read,
Quoted 10 lines
drivers/i2c/designware_i2c.c +9
-1+ .name = "i2c_designware", + .id = UCLASS_I2C, + .of_match = designware_i2c_ids, + .probe = designware_i2c_probe, + .priv_auto_alloc_size = sizeof(struct dw_i2c), + .ops = &designware_i2c_ops, +}; + +#endif /* CONFIG_DM_I2C */ --
Other than that,
Regards,
Bin
PATCH 5/6 i2c: designware_i2c: Add DM support
In reply to Stefan RoeseQuoted 4 lines · Stefan Roese
This patch adds DM support to the designware I2C driver. It currently supports DM and the legacy I2C support. The legacy support should be removed, once all platforms using it have DM enabled.
Quoted 8 lines
Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 149 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 126 insertions(+), 23 deletions(-)
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
In reply to Stefan RoeseThis patch adds support for the PCI(e) based I2C cores. Which can be found for example on the Intel Bay Trail SoC. It has 7 I2C controllers implemented as PCI devices.
This patch also adds the fixed values for the timing registers for BayTrail which are taken from the Linux designware I2C driver.
--- drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 10 deletions(-)
drivers/i2c/designware_i2c.c +101 -10
@@ -8,11 +8,32 @@#include <common.h> #include <dm.h> #include <i2c.h> +#include <pci.h> #include <asm/io.h> #include "designware_i2c.h" +struct dw_scl_sda_cfg { +u32 ss_hcnt; +u32 fs_hcnt; +u32 ss_lcnt; +u32 fs_lcnt; +u32 sda_hold; +}; + +#ifdef CONFIG_X86 +/* BayTrail HCNT/LCNT/SDA hold time */ +static struct dw_scl_sda_cfg byt_config = { + .ss_hcnt =0x200 , + .fs_hcnt =0x55 , + .ss_lcnt =0x200 , + .fs_lcnt =0x99 , + .sda_hold =0x6 , +}; +#endif +struct dw_i2c {struct i2c_regs *regs; +struct dw_scl_sda_cfg *scl_sda_cfg; };static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable)
@@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)* Set the i2c speed. */ static unsigned int __dw_i2c_set_bus_speed (struct i2c_regs *i2c_base, +struct dw_scl_sda_cfg *scl_sda_cfg,unsigned int speed) {unsigned int cntl;
@@ -61,34 +83,55 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, cntl = (readl (&i2c_base->ic_con) & (~IC_CON_SPD_MSK ));switch (i2c_spd) { +#ifndef CONFIG_X86 /* No High-speed for BayTrail yet */ case IC_SPEED_MODE_MAX : - cntl |=IC_CON_SPD_HS ; - hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + cntl |=IC_CON_SPD_SS ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_hs_scl_hcnt); - lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_hs_scl_lcnt);break ; +#endif case IC_SPEED_MODE_STANDARD : cntl |=IC_CON_SPD_SS ; - hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->ss_hcnt; + lcnt = scl_sda_cfg->ss_lcnt; + }else { + hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_ss_scl_hcnt); - lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_ss_scl_lcnt);break ;case IC_SPEED_MODE_FAST :default : cntl |=IC_CON_SPD_FS ; - hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_fs_scl_hcnt); - lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_fs_scl_lcnt);break ; }writel (cntl, &i2c_base->ic_con); +/* Configure SDA Hold Time if required */ +if (scl_sda_cfg) +writel (scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); +/* Enable back i2c now speed set */ dw_i2c_enable (i2c_base,1 );
@@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr)writel (IC_TX_TL , &i2c_base->ic_tx_tl);writel (IC_STOP_DET , &i2c_base->ic_intr_mask);#ifndef CONFIG_DM_I2C -__dw_i2c_set_bus_speed (i2c_base, speed); +__dw_i2c_set_bus_speed (i2c_base,NULL , speed);writel (slaveaddr, &i2c_base->ic_sar);#endif
@@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, unsigned int speed) { adap->speed = speed; -return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); +return __dw_i2c_set_bus_speed (i2c_get_base (adap),NULL , speed); }static void dw_i2c_init (struct i2c_adapter *adap,int speed,int slaveaddr)
@@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) {struct dw_i2c *i2c =dev_get_priv (bus); -return __dw_i2c_set_bus_speed (i2c->regs, speed); +return __dw_i2c_set_bus_speed (i2c->regs, i2c->scl_sda_cfg, speed); } static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr,
@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif __dw_i2c_init (priv->regs,0 ,0 );return 0 ; } +static int designware_i2c_bind (struct udevice *dev) +{ +static int num_cards; +char name[20 ]; + +/* Create a unique device name for PCI type devices */ +if (device_is_on_pci_bus (dev)) { +/* +* ToDo: +* Setting req_seq in the driver is probably not recommended. +* But without a DT alias the number is not configured. And +* using this driver is impossible for PCIe I2C devices. +* This can be removed, once a better (correct) way for this +* is found and implemented. +*/ + dev->req_seq = num_cards; +sprintf (name,"i2c_designware#%u" , num_cards++); +device_set_name (dev, name); + } + +return 0 ; +} +static const struct dm_i2c_ops designware_i2c_ops = { .xfer = designware_i2c_xfer, .probe_chip = designware_i2c_probe_chip,
@@ -499,9 +573,26 @@ U_BOOT_DRIVER(i2c_designware) = { .name = "i2c_designware", .id = UCLASS_I2C, .of_match = designware_i2c_ids, + .bind = designware_i2c_bind, .probe = designware_i2c_probe, .priv_auto_alloc_size = sizeof(struct dw_i2c), .ops = &designware_i2c_ops, }; +#ifdef CONFIG_X86 +static struct pci_device_id designware_pci_supported[] = { +/* Intel BayTrail has 7 I2C controller located on the PCI bus */ + {PCI_VDEVICE (INTEL ,0x0f41 ) }, + {PCI_VDEVICE (INTEL ,0x0f42 ) }, + {PCI_VDEVICE (INTEL ,0x0f43 ) }, + {PCI_VDEVICE (INTEL ,0x0f44 ) }, + {PCI_VDEVICE (INTEL ,0x0f45 ) }, + {PCI_VDEVICE (INTEL ,0x0f46 ) }, + {PCI_VDEVICE (INTEL ,0x0f47 ) }, + {}, +}; + +U_BOOT_PCI_DEVICE (i2c_designware, designware_pci_supported); +#endif +#endif /* CONFIG_DM_I2C */
Signature · 1 line
2.7.3
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Stefan,
Quoted 152 lines · Stefan Roese
This patch adds support for the PCI(e) based I2C cores. Which can be found for example on the Intel Bay Trail SoC. It has 7 I2C controllers implemented as PCI devices.
This patch also adds the fixed values for the timing registers for BayTrail which are taken from the Linux designware I2C driver.
Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 10 deletions(-)drivers/i2c/designware_i2c.c +53
-10@@ -8,11 +8,32 @@#include <common.h> #include <dm.h> #include <i2c.h> +#include <pci.h> #include <asm/io.h> #include "designware_i2c.h" +struct dw_scl_sda_cfg { +u32 ss_hcnt; +u32 fs_hcnt; +u32 ss_lcnt; +u32 fs_lcnt; +u32 sda_hold; +}; + +#ifdef CONFIG_X86 +/* BayTrail HCNT/LCNT/SDA hold time */ +static struct dw_scl_sda_cfg byt_config = { + .ss_hcnt =0x200 , + .fs_hcnt =0x55 , + .ss_lcnt =0x200 , + .fs_lcnt =0x99 , + .sda_hold =0x6 , +}; +#endif +struct dw_i2c {struct i2c_regs *regs; +struct dw_scl_sda_cfg *scl_sda_cfg; };static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable)@@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)* Set the i2c speed. */ static unsigned int __dw_i2c_set_bus_speed (struct i2c_regs *i2c_base, +struct dw_scl_sda_cfg *scl_sda_cfg,unsigned int speed) {unsigned int cntl;@@ -61,34 +83,55 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, cntl = (readl (&i2c_base->ic_con) & (~IC_CON_SPD_MSK ));switch (i2c_spd) { +#ifndef CONFIG_X86 /* No High-speed for BayTrail yet */ case IC_SPEED_MODE_MAX : - cntl |=IC_CON_SPD_HS ; - hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + cntl |=IC_CON_SPD_SS ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_hs_scl_hcnt); - lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_hs_scl_lcnt);break ; +#endif case IC_SPEED_MODE_STANDARD : cntl |=IC_CON_SPD_SS ; - hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->ss_hcnt; + lcnt = scl_sda_cfg->ss_lcnt; + }else { + hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_ss_scl_hcnt); - lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_ss_scl_lcnt);break ;case IC_SPEED_MODE_FAST :default : cntl |=IC_CON_SPD_FS ; - hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_fs_scl_hcnt); - lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_fs_scl_lcnt);break ; }writel (cntl, &i2c_base->ic_con); +/* Configure SDA Hold Time if required */ +if (scl_sda_cfg) +writel (scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); +/* Enable back i2c now speed set */ dw_i2c_enable (i2c_base,1 );@@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr)writel (IC_TX_TL , &i2c_base->ic_tx_tl);writel (IC_STOP_DET , &i2c_base->ic_intr_mask);#ifndef CONFIG_DM_I2C -__dw_i2c_set_bus_speed (i2c_base, speed); +__dw_i2c_set_bus_speed (i2c_base,NULL , speed);writel (slaveaddr, &i2c_base->ic_sar);#endif @@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, unsigned int speed) { adap->speed = speed; -return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); +return __dw_i2c_set_bus_speed (i2c_get_base (adap),NULL , speed); }static void dw_i2c_init (struct i2c_adapter *adap,int speed,int slaveaddr)@@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) {struct dw_i2c *i2c =dev_get_priv (bus); -return __dw_i2c_set_bus_speed (i2c->regs, speed); +return __dw_i2c_set_bus_speed (i2c->regs, i2c->scl_sda_cfg, speed); } static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr,
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 10 lines
drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else
How about:
if (device_is_on_pci_bus(dev)) {
do the PCI I2C stuff here;
}See driver/net/designware.c for example.
Quoted 18 lines
drivers/i2c/designware_i2c.c +10
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif __dw_i2c_init (priv->regs,0 ,0 );return 0 ; } +static int designware_i2c_bind (struct udevice *dev) +{ +static int num_cards; +char name[20 ]; + +/* Create a unique device name for PCI type devices */ +if (device_is_on_pci_bus (dev)) { +/* +* ToDo:
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 556
Quoted 10 lines
drivers/i2c/designware_i2c.c +10
-0+* Setting req_seq in the driver is probably not recommended. +* But without a DT alias the number is not configured. And +* using this driver is impossible for PCIe I2C devices. +* This can be removed, once a better (correct) way for this +* is found and implemented. +*/ + dev->req_seq = num_cards; +sprintf (name,"i2c_designware#%u" , num_cards++); +device_set_name (dev, name); + }
I believe the above should be wrapped by #ifdef CONFIG_DM_PCI #endif, otherwise it won't build when DM_PCI is not enabled.
Quoted 25 lines
drivers/i2c/designware_i2c.c +13
-0+ +return 0 ; +} +static const struct dm_i2c_ops designware_i2c_ops = { .xfer = designware_i2c_xfer, .probe_chip = designware_i2c_probe_chip,@@ -499,9 +573,26 @@ U_BOOT_DRIVER(i2c_designware) = { .name = "i2c_designware", .id = UCLASS_I2C, .of_match = designware_i2c_ids, + .bind = designware_i2c_bind, .probe = designware_i2c_probe, .priv_auto_alloc_size = sizeof(struct dw_i2c), .ops = &designware_i2c_ops, }; +#ifdef CONFIG_X86 +static struct pci_device_id designware_pci_supported[] = { +/* Intel BayTrail has 7 I2C controller located on the PCI bus */ + {PCI_VDEVICE (INTEL ,0x0f41 ) }, + {PCI_VDEVICE (INTEL ,0x0f42 ) }, + {PCI_VDEVICE (INTEL ,0x0f43 ) }, + {PCI_VDEVICE (INTEL ,0x0f44 ) }, + { PCI_VDEVICE(INTEL, 0x0f45) },
drivers/i2c/designware_i2c.c U_BOOT_DRIVER(i2c_designware) = {
Quoted 10 lines
drivers/i2c/designware_i2c.c +8
-1+ { PCI_VDEVICE(INTEL, 0x0f46) }, + { PCI_VDEVICE(INTEL, 0x0f47) }, + {}, +}; + +U_BOOT_PCI_DEVICE(i2c_designware, designware_pci_supported); +#endif + #endif /* CONFIG_DM_I2C */ --
Regards,
Bin
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Bin,
Quoted 166 lines · Bin Meng
Quoted 152 lines · Stefan Roese
This patch adds support for the PCI(e) based I2C cores. Which can be found for example on the Intel Bay Trail SoC. It has 7 I2C controllers implemented as PCI devices.
This patch also adds the fixed values for the timing registers for BayTrail which are taken from the Linux designware I2C driver.
Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 10 deletions(-)drivers/i2c/designware_i2c.c +53
-10@@ -8,11 +8,32 @@#include <common.h> #include <dm.h> #include <i2c.h> +#include <pci.h> #include <asm/io.h> #include "designware_i2c.h" +struct dw_scl_sda_cfg { +u32 ss_hcnt; +u32 fs_hcnt; +u32 ss_lcnt; +u32 fs_lcnt; +u32 sda_hold; +}; + +#ifdef CONFIG_X86 +/* BayTrail HCNT/LCNT/SDA hold time */ +static struct dw_scl_sda_cfg byt_config = { + .ss_hcnt =0x200 , + .fs_hcnt =0x55 , + .ss_lcnt =0x200 , + .fs_lcnt =0x99 , + .sda_hold =0x6 , +}; +#endif +struct dw_i2c {struct i2c_regs *regs; +struct dw_scl_sda_cfg *scl_sda_cfg; };static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable)@@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)* Set the i2c speed. */ static unsigned int __dw_i2c_set_bus_speed (struct i2c_regs *i2c_base, +struct dw_scl_sda_cfg *scl_sda_cfg,unsigned int speed) {unsigned int cntl;@@ -61,34 +83,55 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, cntl = (readl (&i2c_base->ic_con) & (~IC_CON_SPD_MSK ));switch (i2c_spd) { +#ifndef CONFIG_X86 /* No High-speed for BayTrail yet */ case IC_SPEED_MODE_MAX : - cntl |=IC_CON_SPD_HS ; - hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + cntl |=IC_CON_SPD_SS ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_hs_scl_hcnt); - lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_hs_scl_lcnt);break ; +#endif case IC_SPEED_MODE_STANDARD : cntl |=IC_CON_SPD_SS ; - hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->ss_hcnt; + lcnt = scl_sda_cfg->ss_lcnt; + }else { + hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_ss_scl_hcnt); - lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_ss_scl_lcnt);break ;case IC_SPEED_MODE_FAST :default : cntl |=IC_CON_SPD_FS ; - hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_fs_scl_hcnt); - lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_fs_scl_lcnt);break ; }writel (cntl, &i2c_base->ic_con); +/* Configure SDA Hold Time if required */ +if (scl_sda_cfg) +writel (scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); +/* Enable back i2c now speed set */ dw_i2c_enable (i2c_base,1 );@@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr)writel (IC_TX_TL , &i2c_base->ic_tx_tl);writel (IC_STOP_DET , &i2c_base->ic_intr_mask);#ifndef CONFIG_DM_I2C -__dw_i2c_set_bus_speed (i2c_base, speed); +__dw_i2c_set_bus_speed (i2c_base,NULL , speed);writel (slaveaddr, &i2c_base->ic_sar);#endif @@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, unsigned int speed) { adap->speed = speed; -return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); +return __dw_i2c_set_bus_speed (i2c_get_base (adap),NULL , speed); }static void dw_i2c_init (struct i2c_adapter *adap,int speed,int slaveaddr)@@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) {struct dw_i2c *i2c =dev_get_priv (bus); -return __dw_i2c_set_bus_speed (i2c->regs, speed); +return __dw_i2c_set_bus_speed (i2c->regs, i2c->scl_sda_cfg, speed); } static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr,drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 10 lines
drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else How about:
Quoted 3 lines
if (device_is_on_pci_bus(dev)) {
do the PCI I2C stuff here;
}I've tried this but it generated compilation errors on socfpga, as the dm_pci_xxx functions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #if CONFIG_DM_PCI as well.
Quoted 25 lines
See
driver/net/designware.cfor example.Quoted 23 lines
drivers/i2c/designware_i2c.c +15
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif __dw_i2c_init (priv->regs,0 ,0 );return 0 ; } +static int designware_i2c_bind (struct udevice *dev) +{ +static int num_cards; +char name[20 ]; + +/* Create a unique device name for PCI type devices */ +if (device_is_on_pci_bus (dev)) { +/* +* ToDo: +* Setting req_seq in the driver is probably not recommended. +* But without a DT alias the number is not configured. And +* using this driver is impossible for PCIe I2C devices. +* This can be removed, once a better (correct) way for this +* is found and implemented.
Quoted 8 lines
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 556
Quoted 5 lines
drivers/i2c/designware_i2c.c +5
-0+*/ + dev->req_seq = num_cards; +sprintf (name,"i2c_designware#%u" , num_cards++); +device_set_name (dev, name); + }I believe the above should be wrapped by #ifdef
CONFIG_DM_PCI#endif, otherwise it won't build whenDM_PCIis not enabled.
It does build and work on socfpga without CONFIG_DM_PCI enabled. I've double-checked this. The only problem is the dm_pci_xxx() in designware_i2c_probe() as I mentioned above.
Thanks,
Stefan
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Stefan,
Quoted 212 lines · Stefan Roese
Hi Bin,
Quoted 166 lines · Bin Meng
Quoted 152 lines · Stefan Roese
This patch adds support for the PCI(e) based I2C cores. Which can be found for example on the Intel Bay Trail SoC. It has 7 I2C controllers implemented as PCI devices.
This patch also adds the fixed values for the timing registers for BayTrail which are taken from the Linux designware I2C driver.
Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 10 deletions(-)drivers/i2c/designware_i2c.c +53
-10@@ -8,11 +8,32 @@#include <common.h> #include <dm.h> #include <i2c.h> +#include <pci.h> #include <asm/io.h> #include "designware_i2c.h" +struct dw_scl_sda_cfg { +u32 ss_hcnt; +u32 fs_hcnt; +u32 ss_lcnt; +u32 fs_lcnt; +u32 sda_hold; +}; + +#ifdef CONFIG_X86 +/* BayTrail HCNT/LCNT/SDA hold time */ +static struct dw_scl_sda_cfg byt_config = { + .ss_hcnt =0x200 , + .fs_hcnt =0x55 , + .ss_lcnt =0x200 , + .fs_lcnt =0x99 , + .sda_hold =0x6 , +}; +#endif +struct dw_i2c {struct i2c_regs *regs; +struct dw_scl_sda_cfg *scl_sda_cfg; };static void dw_i2c_enable (struct i2c_regs *i2c_base,bool enable)@@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)* Set the i2c speed. */ static unsigned int __dw_i2c_set_bus_speed (struct i2c_regs *i2c_base, +struct dw_scl_sda_cfg *scl_sda_cfg,unsigned int speed) {unsigned int cntl;@@ -61,34 +83,55 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, cntl = (readl (&i2c_base->ic_con) & (~IC_CON_SPD_MSK ));switch (i2c_spd) { +#ifndef CONFIG_X86 /* No High-speed for BayTrail yet */ case IC_SPEED_MODE_MAX : - cntl |=IC_CON_SPD_HS ; - hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + cntl |=IC_CON_SPD_SS ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_HS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_hs_scl_hcnt); - lcnt = (IC_CLK *MIN_HS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_hs_scl_lcnt);break ; +#endif case IC_SPEED_MODE_STANDARD : cntl |=IC_CON_SPD_SS ; - hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->ss_hcnt; + lcnt = scl_sda_cfg->ss_lcnt; + }else { + hcnt = (IC_CLK *MIN_SS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_ss_scl_hcnt); - lcnt = (IC_CLK *MIN_SS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_ss_scl_lcnt);break ;case IC_SPEED_MODE_FAST :default : cntl |=IC_CON_SPD_FS ; - hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; +if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + }else { + hcnt = (IC_CLK *MIN_FS_SCL_HIGHTIME ) /NANO_TO_MICRO ; + lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ; + }writel (hcnt, &i2c_base->ic_fs_scl_hcnt); - lcnt = (IC_CLK *MIN_FS_SCL_LOWTIME ) /NANO_TO_MICRO ;writel (lcnt, &i2c_base->ic_fs_scl_lcnt);break ; }writel (cntl, &i2c_base->ic_con); +/* Configure SDA Hold Time if required */ +if (scl_sda_cfg) +writel (scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); +/* Enable back i2c now speed set */ dw_i2c_enable (i2c_base,1 );@@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr)writel (IC_TX_TL , &i2c_base->ic_tx_tl);writel (IC_STOP_DET , &i2c_base->ic_intr_mask);#ifndef CONFIG_DM_I2C -__dw_i2c_set_bus_speed (i2c_base, speed); +__dw_i2c_set_bus_speed (i2c_base,NULL , speed);writel (slaveaddr, &i2c_base->ic_sar);#endif @@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, unsigned int speed) { adap->speed = speed; -return __dw_i2c_set_bus_speed (i2c_get_base (adap), speed); +return __dw_i2c_set_bus_speed (i2c_get_base (adap),NULL , speed); }static void dw_i2c_init (struct i2c_adapter *adap,int speed,int slaveaddr)@@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) {struct dw_i2c *i2c =dev_get_priv (bus); -return __dw_i2c_set_bus_speed (i2c->regs, speed); +return __dw_i2c_set_bus_speed (i2c->regs, i2c->scl_sda_cfg, speed); } static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr,drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 10 lines
drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else How about:
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 25 lines
See
driver/net/designware.cfor example.Quoted 23 lines
drivers/i2c/designware_i2c.c +15
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif __dw_i2c_init (priv->regs,0 ,0 );return 0 ; } +static int designware_i2c_bind (struct udevice *dev) +{ +static int num_cards; +char name[20 ]; + +/* Create a unique device name for PCI type devices */ +if (device_is_on_pci_bus (dev)) { +/* +* ToDo: +* Setting req_seq in the driver is probably not recommended. +* But without a DT alias the number is not configured. And +* using this driver is impossible for PCIe I2C devices. +* This can be removed, once a better (correct) way for this +* is found and implemented. Quoted 8 lines
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 556
Quoted 5 lines
drivers/i2c/designware_i2c.c +5
-0+*/ + dev->req_seq = num_cards; +sprintf (name,"i2c_designware#%u" , num_cards++); +device_set_name (dev, name); + }I believe the above should be wrapped by #ifdef
CONFIG_DM_PCI#endif, otherwise it won't build whenDM_PCIis not enabled.
Quoted 4 lines
It does build and work on socfpga without
CONFIG_DM_PCIenabled. I've double-checked this. The only problem is thedm_pci_xxx()indesignware_i2c_probe()as I mentioned above.
Great. So looks we may consolidate one usage for such both PCI and SoC devices, like the one used in drivers/net/designware.c?
Regards,
Bin
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
In reply to Stefan RoeseHi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 15 lines
Quoted 15 lines
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 12 lines
static int designware_i2c_probe_chip(struct udevice *bus, uint
chip_addr,drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else How about:
Quoted 14 lines
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
drivers/i2c/designware_i2c.c +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif
Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
priv->regs = (struct i2c_regs *)dev_get_addr(bus);
^This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So I'm wondering, how dev_get_addr() should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bit phy_addr representation again? So that dev_get_addr() matches the (void *) size again?
The other option would to just leave the code as in v1 so that dev_get_addr() is not referenced on x86 here.
Thanks,
Stefan
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Stefan,
Quoted 41 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 15 lines
Quoted 15 lines
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 12 lines
static int designware_i2c_probe_chip(struct udevice *bus, uint
chip_addr,drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else How about:
Quoted 14 lines
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
drivers/i2c/designware_i2c.c +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif Enabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?
dev_get_addr() is being used on x86 drivers. See ns16550_serial_ofdata_to_platdata() for example. There is no build warning for the ns16550 driver.
Quoted 3 lines
The other option would to just leave the code as in v1 so that
dev_get_addr()is not referenced on x86 here.
Regards,
Bin
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Bin,
Quoted 52 lines · Bin Meng
Quoted 41 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 15 lines
Quoted 15 lines
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 12 lines
static int designware_i2c_probe_chip(struct udevice *bus, uint
chip_addr,drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else How about:
Quoted 14 lines
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
drivers/i2c/designware_i2c.c +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif Enabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?
Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.
I see, thanks. Its used differently there though. Let me see, if I cook something up...
Thanks,
Stefan
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
In reply to Bin MengHi Bin,
Quoted 52 lines · Bin Meng
Quoted 41 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 15 lines
Quoted 15 lines
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 12 lines
static int designware_i2c_probe_chip(struct udevice *bus, uint
chip_addr,drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else How about:
Quoted 14 lines
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
drivers/i2c/designware_i2c.c +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif Enabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?
Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.
Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "plat->base = addr" in ns16550.c does not cause any warnings.
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the
* register base directly in dev_get_addr() results in this compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macro POINTER_SIZE_CAST, allows us to cast the result of
* dev_get_addr() into a 32bit value before casting it to the pointer
* (struct i2c_regs *).
*/
#ifdef CONFIG_X86
#define POINTER_SIZE_CAST u32
#endif
...
static int designware_i2c_probe(struct udevice *bus)
{
struct dw_i2c *priv = dev_get_priv(bus);
if (device_is_on_pci_bus(bus)) {
#ifdef CONFIG_DM_PCI
/* Save base address from PCI BAR */
priv->regs = (struct i2c_regs *)
dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
#ifdef CONFIG_X86
/* Use BayTrail specific timing values */
priv->scl_sda_cfg = &byt_config;
#endif
#endif
} else {
/* Save base address from device-tree */
priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus);
}But I'm not 100% happy with this approach.
So what are the alternatives:
a) Don't compile the dev_get_addr() part for x86 similar to what I've done in v1
b) This approach with POINTER_SIZE_CAST
Any preferences of other ideas?
Side note: My general feeling is, that dev_get_addr() should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...
Thanks,
Stefan
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Stefan,
Quoted 64 lines · Stefan Roese
Hi Bin,
Quoted 52 lines · Bin Meng
Quoted 41 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 15 lines
Quoted 15 lines
drivers/i2c/designware_i2c.c static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 12 lines
static int designware_i2c_probe_chip(struct udevice *bus, uint
chip_addr,drivers/i2c/designware_i2c.c +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {struct dw_i2c *priv =dev_get_priv (bus); +#ifdef CONFIG_X86 +/* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) +dm_pci_map_bar (bus,PCI_BASE_ADDRESS_0 ,PCI_REGION_MEM ); +/* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else How about:
Quoted 14 lines
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
drivers/i2c/designware_i2c.c +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr (bus); +#endif Enabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "
plat->base= addr" inns16550.cdoes not cause any warnings.
Quoted 5 lines
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
Thanks for digging out these.
Quoted 35 lines
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the
* register base directly indev_get_addr()results in this compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macroPOINTER_SIZE_CAST, allows us to cast the result of
*dev_get_addr()into a 32bit value before casting it to the pointer
* (structi2c_regs*).
*/
#ifdefCONFIG_X86
#definePOINTER_SIZE_CASTu32
#endif...
static int
designware_i2c_probe(struct udevice *bus){ struct dw_i2c *priv = dev_get_priv(bus); if (device_is_on_pci_bus(bus)) { #ifdef CONFIG_DM_PCI /* Save base address from PCI BAR */ priv->regs = (struct i2c_regs *) dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); #ifdef CONFIG_X86 /* Use BayTrail specific timing values */ priv->scl_sda_cfg = &byt_config; #endif #endif } else { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); }
Quoted 2 lines
But I'm not 100% happy with this approach.
Yes, it's annoying.
Quoted 5 lines
So what are the alternatives:
a) Don't compile the
dev_get_addr()part for x86 similar to what I've done in v1
Quoted 9 lines
b) This approach with
POINTER_SIZE_CASTAny preferences of other ideas?
Side note: My general feeling is, that
dev_get_addr()should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...
I would like to hear Simon's input. Simon?
Regards,
Bin
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Simon,
as you seem to be back from vacation (?), we (Bin and myself) would like to hear your expert comment on a x86 issue I've discovered while porting the Designware I2C driver to x86. Please see below:
Quoted 124 lines · Bin Meng
Hi Stefan,
Quoted 64 lines · Stefan Roese
Hi Bin,
Quoted 52 lines · Bin Meng
Quoted 41 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 15 lines
Quoted 15 lines
Review static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 12 lines
static int designware_i2c_probe_chip(struct udevice *bus, uint
chip_addr,(quoted diff) +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) { struct dw_i2c *priv = dev_get_priv(bus); +#ifdef CONFIG_X86 + /* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); + /* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#elseHow about:
Quoted 14 lines
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
(quoted diff) +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr(bus); +#endifEnabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "
plat->base= addr" inns16550.cdoes not cause any warnings.Quoted 5 lines
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
Thanks for digging out these.
Quoted 35 lines
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the
* register base directly indev_get_addr()results in this compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macroPOINTER_SIZE_CAST, allows us to cast the result of
*dev_get_addr()into a 32bit value before casting it to the pointer
* (structi2c_regs*).
*/
#ifdefCONFIG_X86
#definePOINTER_SIZE_CASTu32
#endif...
static int
designware_i2c_probe(struct udevice *bus){ struct dw_i2c *priv = dev_get_priv(bus); if (device_is_on_pci_bus(bus)) { #ifdef CONFIG_DM_PCI /* Save base address from PCI BAR */ priv->regs = (struct i2c_regs *) dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); #ifdef CONFIG_X86 /* Use BayTrail specific timing values */ priv->scl_sda_cfg = &byt_config; #endif #endif } else { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); }Quoted 2 lines
But I'm not 100% happy with this approach.
Yes, it's annoying.
Quoted 9 lines
So what are the alternatives:
a) Don't compile the
dev_get_addr()part for x86 similar to what I've done in v1b) This approach with
POINTER_SIZE_CASTAny preferences of other ideas?
Quoted 7 lines
Quoted 5 lines
Side note: My general feeling is, that
dev_get_addr()should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...I would like to hear Simon's input. Simon?
Yes, Simon, what do you think?
Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) for the cast:
https://patchwork.ozlabs.org/patch/601113/
Thanks,
Stefan
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Simon,
Quoted 137 lines · Stefan Roese
Hi Simon,
as you seem to be back from vacation (?), we (Bin and myself) would like to hear your expert comment on a x86 issue I've discovered while porting the Designware I2C driver to x86. Please see below:
Quoted 130 lines · Bin Meng
Hi Stefan,
Quoted 64 lines · Stefan Roese
Hi Bin,
Quoted 52 lines · Bin Meng
Quoted 41 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 15 lines
Quoted 15 lines
Review static int designware_i2c_probe(struct udevice *bus) · line 528
Quoted 12 lines
static int designware_i2c_probe_chip(struct udevice *bus, uint
chip_addr,(quoted diff) +7
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) { struct dw_i2c *priv = dev_get_priv(bus); +#ifdef CONFIG_X86 + /* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); + /* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#elseHow about:
Quoted 14 lines
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
(quoted diff) +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr(bus); +#endifEnabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "
plat->base= addr" inns16550.cdoes not cause any warnings.Quoted 5 lines
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
Thanks for digging out these.
Quoted 35 lines
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the
* register base directly indev_get_addr()results in this compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macroPOINTER_SIZE_CAST, allows us to cast the result of
*dev_get_addr()into a 32bit value before casting it to the pointer
* (structi2c_regs*).
*/
#ifdefCONFIG_X86
#definePOINTER_SIZE_CASTu32
#endif...
static int
designware_i2c_probe(struct udevice *bus){ struct dw_i2c *priv = dev_get_priv(bus); if (device_is_on_pci_bus(bus)) { #ifdef CONFIG_DM_PCI /* Save base address from PCI BAR */ priv->regs = (struct i2c_regs *) dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); #ifdef CONFIG_X86 /* Use BayTrail specific timing values */ priv->scl_sda_cfg = &byt_config; #endif #endif } else { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); }Quoted 2 lines
But I'm not 100% happy with this approach.
Yes, it's annoying.
Quoted 14 lines
So what are the alternatives:
a) Don't compile the
dev_get_addr()part for x86 similar to what I've done in v1b) This approach with
POINTER_SIZE_CASTAny preferences of other ideas?
Side note: My general feeling is, that
dev_get_addr()should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...
Quoted 8 lines
Quoted 1 line
I would like to hear Simon's input. Simon?
Yes, Simon, what do you think?
Please also see my v2 of this patch which uses (
__UINTPTR_TYPE__) for the cast:
Simon, could you please take a quick look at this patch? With the general problem of dev_get_addr() on x86 (as described above). Do you have some other suggestions to solve this? Or is the solution in v2 which uses (__UINTPTR_TYPE__) acceptable?
https://patchwork.ozlabs.org/patch/601113/
Thanks,
Stefan
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Stefan,
Quoted 171 lines · Stefan Roese
Hi Simon,
Quoted 159 lines · Stefan Roese
Hi Simon,
as you seem to be back from vacation (?), we (Bin and myself) would like to hear your expert comment on a x86 issue I've discovered while porting the Designware I2C driver to x86. Please see below:
Quoted 142 lines · Bin Meng
Hi Stefan,
Quoted 76 lines · Stefan Roese
Hi Bin,
Quoted 62 lines · Bin Meng
Quoted 50 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 24 lines
Quoted 19 lines
Quoted 5 lines
static int designware_i2c_probe_chip(struct udevice *bus,
uintchip_addr,(quoted diff) +0
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {Review static int designware_i2c_probe(struct · line 530
Quoted 10 lines
(quoted diff) +7
-0struct dw_i2c *priv = dev_get_priv(bus); +#ifdef CONFIG_X86 + /* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); + /* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#elseHow about:
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }Quoted 11 lines
I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
(quoted diff) +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr(bus); +#endifEnabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "
plat->base= addr" inns16550.cdoes not cause any warnings.Quoted 5 lines
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
Thanks for digging out these.
Quoted 39 lines
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning
the
* register base directly indev_get_addr()results in this
compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macroPOINTER_SIZE_CAST, allows us to cast the result of
*dev_get_addr()into a 32bit value before casting it to the pointer
* (structi2c_regs*).
*/
#ifdefCONFIG_X86
#definePOINTER_SIZE_CASTu32
#endif...
static int
designware_i2c_probe(struct udevice *bus){ struct dw_i2c *priv = dev_get_priv(bus); if (device_is_on_pci_bus(bus)) { #ifdef CONFIG_DM_PCI /* Save base address from PCI BAR */ priv->regs = (struct i2c_regs *) dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); #ifdef CONFIG_X86 /* Use BayTrail specific timing values */ priv->scl_sda_cfg = &byt_config; #endif #endif } else { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); }Quoted 2 lines
But I'm not 100% happy with this approach.
Yes, it's annoying.
Quoted 9 lines
So what are the alternatives:
a) Don't compile the
dev_get_addr()part for x86 similar to what I've done in v1b) This approach with
POINTER_SIZE_CASTAny preferences of other ideas?
Quoted 7 lines
Quoted 5 lines
Side note: My general feeling is, that
dev_get_addr()should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...I would like to hear Simon's input. Simon?
Quoted 6 lines
Yes, Simon, what do you think?
Please also see my v2 of this patch which uses (
__UINTPTR_TYPE__) for the cast:
Quoted 6 lines
Simon, could you please take a quick look at this patch? With the general problem of
dev_get_addr()on x86 (as described above). Do you have some other suggestions to solve this? Or is the solution in v2 which uses (__UINTPTR_TYPE__) acceptable?
I feel that you should store the return value from dev_get_addr() in an fdt_addr_t or a ulong. Then it can be cast to a pointer as you wish. Platform data should hold the ulong, and private data (dev_get_priv()) should hold the pointer.
I'm not keen on the POINTER_SIZE_CAST idea.
Does that fix the problem?
Regards,
Simon
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Simon.
Quoted 179 lines · Simon Glass
Quoted 171 lines · Stefan Roese
Hi Simon,
Quoted 159 lines · Stefan Roese
Hi Simon,
as you seem to be back from vacation (?), we (Bin and myself) would like to hear your expert comment on a x86 issue I've discovered while porting the Designware I2C driver to x86. Please see below:
Quoted 142 lines · Bin Meng
Hi Stefan,
Quoted 76 lines · Stefan Roese
Hi Bin,
Quoted 62 lines · Bin Meng
Quoted 50 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 24 lines
Quoted 19 lines
Quoted 5 lines
static int designware_i2c_probe_chip(struct udevice *bus,
uintchip_addr,(quoted diff) +0
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {Review static int designware_i2c_probe(struct · line 530
Quoted 10 lines
(quoted diff) +7
-0struct dw_i2c *priv = dev_get_priv(bus); +#ifdef CONFIG_X86 + /* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); + /* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#elseHow about:
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }Quoted 11 lines
I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
(quoted diff) +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr(bus); +#endifEnabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "
plat->base= addr" inns16550.cdoes not cause any warnings.Quoted 5 lines
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
Thanks for digging out these.
Quoted 39 lines
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning
the
* register base directly indev_get_addr()results in this
compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macroPOINTER_SIZE_CAST, allows us to cast the result of
*dev_get_addr()into a 32bit value before casting it to the pointer
* (structi2c_regs*).
*/
#ifdefCONFIG_X86
#definePOINTER_SIZE_CASTu32
#endif...
static int
designware_i2c_probe(struct udevice *bus){ struct dw_i2c *priv = dev_get_priv(bus); if (device_is_on_pci_bus(bus)) { #ifdef CONFIG_DM_PCI /* Save base address from PCI BAR */ priv->regs = (struct i2c_regs *) dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); #ifdef CONFIG_X86 /* Use BayTrail specific timing values */ priv->scl_sda_cfg = &byt_config; #endif #endif } else { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); }Quoted 2 lines
But I'm not 100% happy with this approach.
Yes, it's annoying.
Quoted 9 lines
So what are the alternatives:
a) Don't compile the
dev_get_addr()part for x86 similar to what I've done in v1b) This approach with
POINTER_SIZE_CASTAny preferences of other ideas?
Quoted 7 lines
Quoted 5 lines
Side note: My general feeling is, that
dev_get_addr()should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...I would like to hear Simon's input. Simon?
Quoted 6 lines
Yes, Simon, what do you think?
Please also see my v2 of this patch which uses (
__UINTPTR_TYPE__) for the cast:Quoted 6 lines
Simon, could you please take a quick look at this patch? With the general problem of
dev_get_addr()on x86 (as described above). Do you have some other suggestions to solve this? Or is the solution in v2 which uses (__UINTPTR_TYPE__) acceptable?
Quoted 8 lines
I feel that you should store the return value from
dev_get_addr()in anfdt_addr_tor a ulong. Then it can be cast to a pointer as you wish. Platform data should hold the ulong, and private data (dev_get_priv()) should hold the pointer.I'm not keen on the
POINTER_SIZE_CASTidea.Does that fix the problem?
Yes, it does. In a somewhat less ugly way. This is my current result:
} else {
ulong base;
/* Save base address from device-tree */
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit.
* So assigning the register base directly in dev_get_addr()
* results in this compilation warning:
* warning: cast to pointer from integer of different size
*
* Using an intermediate "ulong" variable before assigning
* this pointer to the "regs" variable solves this issue.
*/
base = dev_get_addr(bus);
priv->regs = (struct i2c_regs *)base;
}If you think this is acceptable, I'll send a new patch version to the list.
Thanks,
Stefan
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Stefan,
Quoted 213 lines · Stefan Roese
Hi Simon.
Quoted 179 lines · Simon Glass
Quoted 171 lines · Stefan Roese
Hi Simon,
Quoted 159 lines · Stefan Roese
Hi Simon,
as you seem to be back from vacation (?), we (Bin and myself) would like to hear your expert comment on a x86 issue I've discovered while porting the Designware I2C driver to x86. Please see below:
Quoted 142 lines · Bin Meng
Hi Stefan,
Quoted 76 lines · Stefan Roese
Hi Bin,
Quoted 62 lines · Bin Meng
Quoted 50 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 24 lines
Quoted 19 lines
Quoted 5 lines
static int designware_i2c_probe_chip(struct udevice *bus,
uintchip_addr,(quoted diff) +0
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {Review static int designware_i2c_probe(struct · line 530
Quoted 10 lines
(quoted diff) +7
-0struct dw_i2c *priv = dev_get_priv(bus); +#ifdef CONFIG_X86 + /* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); + /* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#elseHow about:
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }Quoted 11 lines
I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
(quoted diff) +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr(bus); +#endifEnabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "
plat->base= addr" inns16550.cdoes not cause any warnings.Quoted 5 lines
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
Thanks for digging out these.
Quoted 39 lines
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning
the
* register base directly indev_get_addr()results in this
compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macroPOINTER_SIZE_CAST, allows us to cast the result of
*dev_get_addr()into a 32bit value before casting it to the pointer
* (structi2c_regs*).
*/
#ifdefCONFIG_X86
#definePOINTER_SIZE_CASTu32
#endif...
static int
designware_i2c_probe(struct udevice *bus){ struct dw_i2c *priv = dev_get_priv(bus); if (device_is_on_pci_bus(bus)) { #ifdef CONFIG_DM_PCI /* Save base address from PCI BAR */ priv->regs = (struct i2c_regs *) dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); #ifdef CONFIG_X86 /* Use BayTrail specific timing values */ priv->scl_sda_cfg = &byt_config; #endif #endif } else { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); }Quoted 2 lines
But I'm not 100% happy with this approach.
Yes, it's annoying.
Quoted 9 lines
So what are the alternatives:
a) Don't compile the
dev_get_addr()part for x86 similar to what I've done in v1b) This approach with
POINTER_SIZE_CASTAny preferences of other ideas?
Quoted 7 lines
Quoted 5 lines
Side note: My general feeling is, that
dev_get_addr()should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...I would like to hear Simon's input. Simon?
Quoted 6 lines
Yes, Simon, what do you think?
Please also see my v2 of this patch which uses (
__UINTPTR_TYPE__) for the cast:Quoted 6 lines
Simon, could you please take a quick look at this patch? With the general problem of
dev_get_addr()on x86 (as described above). Do you have some other suggestions to solve this? Or is the solution in v2 which uses (__UINTPTR_TYPE__) acceptable?Quoted 8 lines
I feel that you should store the return value from
dev_get_addr()in anfdt_addr_tor a ulong. Then it can be cast to a pointer as you wish. Platform data should hold the ulong, and private data (dev_get_priv()) should hold the pointer.I'm not keen on the
POINTER_SIZE_CASTidea.Does that fix the problem?
Yes, it does. In a somewhat less ugly way. This is my current result:
} else { ulong base; /* Save base address from device-tree */ /* * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. * So assigning the register base directly in dev_get_addr() * results in this compilation warning: * warning: cast to pointer from integer of different size * * Using an intermediate "ulong" variable before assigning * this pointer to the "regs" variable solves this issue. */ base = dev_get_addr(bus); priv->regs = (struct i2c_regs *)base; }
Quoted 2 lines
If you think this is acceptable, I'll send a new patch version to the list.
Seems fine to me. Perhaps we should have dev_get_addr_ptr() to do this for us?
Regards,
Simon
PATCH 6/6 i2c: designware_i2c: Add support for PCI(e) based I2C cores (x86)
Hi Simon,
Quoted 216 lines · Simon Glass
Hi Stefan,
Quoted 213 lines · Stefan Roese
Hi Simon.
Quoted 179 lines · Simon Glass
Quoted 171 lines · Stefan Roese
Hi Simon,
Quoted 159 lines · Stefan Roese
Hi Simon,
as you seem to be back from vacation (?), we (Bin and myself) would like to hear your expert comment on a x86 issue I've discovered while porting the Designware I2C driver to x86. Please see below:
Quoted 142 lines · Bin Meng
Hi Stefan,
Quoted 76 lines · Stefan Roese
Hi Bin,
Quoted 62 lines · Bin Meng
Quoted 50 lines · Stefan Roese
Hi Bin,
On 21.03.2016 10:03, Stefan Roese wrote:
<snip>
Quoted 24 lines
Quoted 19 lines
Quoted 5 lines
static int designware_i2c_probe_chip(struct udevice *bus,
uintchip_addr,(quoted diff) +0
-0@@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) {Review static int designware_i2c_probe(struct · line 530
Quoted 10 lines
(quoted diff) +7
-0struct dw_i2c *priv = dev_get_priv(bus); +#ifdef CONFIG_X86 + /* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); + /* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#elseHow about:
Quoted 3 lines
if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; }Quoted 11 lines
I've tried this but it generated compilation errors on socfpga, as the
dm_pci_xxxfunctions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #ifCONFIG_DM_PCIas well.Quoted 5 lines
See
driver/net/designware.cfor example.Quoted 3 lines
(quoted diff) +1
-0/* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr(bus); +#endifEnabling this code for x86 via if (
device_is_on_pci_bus(dev)) results in this ugly compilation warning:
drivers/i2c/designware_i2c.c: In function ?designware_i2c_probe?:drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]Quoted 9 lines
priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^This is because x86 defines
fdt_addr_t/phys_addr_tas 64bit. So I'm wondering, howdev_get_addr()should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bitphy_addrrepresentation again? So thatdev_get_addr()matches the (void *) size again?Quoted 3 lines
dev_get_addr()is being used on x86 drivers. Seens16550_serial_ofdata_to_platdata()for example. There is no build warning for the ns16550 driver.Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "
plat->base= addr" inns16550.cdoes not cause any warnings.Quoted 5 lines
Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach:
Thanks for digging out these.
Quoted 39 lines
/*
* On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning
the
* register base directly indev_get_addr()results in this
compilation warning:
* warning: cast to pointer from integer of different size
*
* Using this macroPOINTER_SIZE_CAST, allows us to cast the result of
*dev_get_addr()into a 32bit value before casting it to the pointer
* (structi2c_regs*).
*/
#ifdefCONFIG_X86
#definePOINTER_SIZE_CASTu32
#endif...
static int
designware_i2c_probe(struct udevice *bus){ struct dw_i2c *priv = dev_get_priv(bus); if (device_is_on_pci_bus(bus)) { #ifdef CONFIG_DM_PCI /* Save base address from PCI BAR */ priv->regs = (struct i2c_regs *) dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); #ifdef CONFIG_X86 /* Use BayTrail specific timing values */ priv->scl_sda_cfg = &byt_config; #endif #endif } else { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); }Quoted 2 lines
But I'm not 100% happy with this approach.
Yes, it's annoying.
Quoted 9 lines
So what are the alternatives:
a) Don't compile the
dev_get_addr()part for x86 similar to what I've done in v1b) This approach with
POINTER_SIZE_CASTAny preferences of other ideas?
Quoted 7 lines
Quoted 5 lines
Side note: My general feeling is, that
dev_get_addr()should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well...I would like to hear Simon's input. Simon?
Quoted 6 lines
Yes, Simon, what do you think?
Please also see my v2 of this patch which uses (
__UINTPTR_TYPE__) for the cast:Quoted 6 lines
Simon, could you please take a quick look at this patch? With the general problem of
dev_get_addr()on x86 (as described above). Do you have some other suggestions to solve this? Or is the solution in v2 which uses (__UINTPTR_TYPE__) acceptable?Quoted 8 lines
I feel that you should store the return value from
dev_get_addr()in anfdt_addr_tor a ulong. Then it can be cast to a pointer as you wish. Platform data should hold the ulong, and private data (dev_get_priv()) should hold the pointer.I'm not keen on the
POINTER_SIZE_CASTidea.Does that fix the problem?
Yes, it does. In a somewhat less ugly way. This is my current result:
} else { ulong base; /* Save base address from device-tree */ /* * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. * So assigning the register base directly in dev_get_addr() * results in this compilation warning: * warning: cast to pointer from integer of different size * * Using an intermediate "ulong" variable before assigning * this pointer to the "regs" variable solves this issue. */ base = dev_get_addr(bus); priv->regs = (struct i2c_regs *)base; }
Quoted 5 lines
Quoted 2 lines
If you think this is acceptable, I'll send a new patch version to the list.
Seems fine to me. Perhaps we should have
dev_get_addr_ptr()to do this for us?
Might make sense. I can generate a small patch for this.
Perhaps we should better use "uintptr_t" as type for the intermediate variable instead. But then we can effectively drop the intermediate variable and do the casting directly.
What do you think?
Thanks,
Stefan
PATCH 1/6 i2c: designware_i2c: Add ic_enable_status to ic_regs struct
In reply to Stefan RoeseQuoted 9 lines · Stefan Roese
Add the
ic_enable_statusregister to theic_regsstruct. Additionally the register offsets are added, to better check, if the offset matches the register description in the datasheet.Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>
Best regards,
Marek Vasut
PATCH 1/6 i2c: designware_i2c: Add ic_enable_status to ic_regs struct
In reply to Stefan RoeseHi Stefan,
Quoted 1 line · Stefan Roese
Add the
ic_enable_statusregister to theic_regsstruct. Additionally
typo: i2c_regs
Quoted 80 lines
the register offsets are added, to better check, if the offset matches the register description in the datasheet.
Signed-off-by Stefan Roese <[email protected]>Cc Simon Glass <[email protected]>Cc Bin Meng <[email protected]>Cc Marek Vasut <[email protected]>Cc Heiko Schocher <[email protected]>--- drivers/i2c/designware_i2c.h | 68 +++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 33 deletions(-)drivers/i2c/designware_i2c.h +27
-33@@ -9,39 +9,41 @@#define __DW_I2C_H_struct i2c_regs { -u32 ic_con; -u32 ic_tar; -u32 ic_sar; -u32 ic_hs_maddr; -u32 ic_cmd_data; -u32 ic_ss_scl_hcnt; -u32 ic_ss_scl_lcnt; -u32 ic_fs_scl_hcnt; -u32 ic_fs_scl_lcnt; -u32 ic_hs_scl_hcnt; -u32 ic_hs_scl_lcnt; -u32 ic_intr_stat; -u32 ic_intr_mask; -u32 ic_raw_intr_stat; -u32 ic_rx_tl; -u32 ic_tx_tl; -u32 ic_clr_intr; -u32 ic_clr_rx_under; -u32 ic_clr_rx_over; -u32 ic_clr_tx_over; -u32 ic_clr_rd_req; -u32 ic_clr_tx_abrt; -u32 ic_clr_rx_done; -u32 ic_clr_activity; -u32 ic_clr_stop_det; -u32 ic_clr_start_det; -u32 ic_clr_gen_call; -u32 ic_enable; -u32 ic_status; -u32 ic_txflr; -u32 ix_rxflr; -u32 reserved_1; -u32 ic_tx_abrt_source; +u32 ic_con;/* 0x00 */ +u32 ic_tar;/* 0x04 */ +u32 ic_sar;/* 0x08 */ +u32 ic_hs_maddr;/* 0x0c */ +u32 ic_cmd_data;/* 0x10 */ +u32 ic_ss_scl_hcnt;/* 0x14 */ +u32 ic_ss_scl_lcnt;/* 0x18 */ +u32 ic_fs_scl_hcnt;/* 0x1c */ +u32 ic_fs_scl_lcnt;/* 0x20 */ +u32 ic_hs_scl_hcnt;/* 0x24 */ +u32 ic_hs_scl_lcnt;/* 0x28 */ +u32 ic_intr_stat;/* 0x2c */ +u32 ic_intr_mask;/* 0x30 */ +u32 ic_raw_intr_stat;/* 0x34 */ +u32 ic_rx_tl;/* 0x38 */ +u32 ic_tx_tl;/* 0x3c */ +u32 ic_clr_intr;/* 0x40 */ +u32 ic_clr_rx_under;/* 0x44 */ +u32 ic_clr_rx_over;/* 0x48 */ +u32 ic_clr_tx_over;/* 0x4c */ +u32 ic_clr_rd_req;/* 0x50 */ +u32 ic_clr_tx_abrt;/* 0x54 */ +u32 ic_clr_rx_done;/* 0x58 */ +u32 ic_clr_activity;/* 0x5c */ +u32 ic_clr_stop_det;/* 0x60 */ +u32 ic_clr_start_det;/* 0x64 */ +u32 ic_clr_gen_call;/* 0x68 */
drivers/i2c/designware_i2c.h line 48
Quoted 10 lines
drivers/i2c/designware_i2c.h +8
-0+u32 ic_enable;/* 0x6c */ +u32 ic_status;/* 0x70 */ +u32 ic_txflr;/* 0x74 */ +u32 ic_rxflr;/* 0x78 */ +u32 ic_sda_hold;/* 0x7c */ +u32 ic_tx_abrt_source;/* 0x80 */ +u8 res1[0x18 ];/* 0x84 */ +u32 ic_enable_status;/* 0x9c */ };
Other than that,
Regards,
Bin