Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
Hi Alain, > Ok, understood. Fine for me that way as well. I am just a little worrying that > the "host-notify" can now be present in both controller AND slave nodes > and might be a bit hard to understand. At the same time I don't have a better > proposal for naming the binding for the controller. It is a valid concern, maybe we could name the binding for the host "enable-host-notify"? > Please do not consider serie v2 I just posted few days ago and I will > post a serie v3 updating the binding information and using the host-notify > binding in the i2c-stm32f7 driver. I also have an idea for the SMBusAlert topic, hopefully I can come up with a summary later today. All the best, Wolfram signature.asc Description: PGP signature
Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
Hi Wolfram, > I meant a generic binding for the host-controller. It could be seen as a > HW description if we need HostNotify on that bus or not. > > Maybe it becomes more clear with the R-Car I2C controller as an example. > It only supports one slave address. If I want HostNotify there, I can't > use another slave backend. Now, it could be that I need the slave EEPROM > backend, although there is a HostNotify capable device on the bus. So, I > am leaning to have a generic "host-notify" binding for the host. > > I consider platform_data legacy. If we use device_property, we should be > safe regarding all current and future HW descriptions, or? Ok, understood. Fine for me that way as well. I am just a little worrying that the "host-notify" can now be present in both controller AND slave nodes and might be a bit hard to understand. At the same time I don't have a better proposal for naming the binding for the controller. Please do not consider serie v2 I just posted few days ago and I will post a serie v3 updating the binding information and using the host-notify binding in the i2c-stm32f7 driver. Alain
Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
Hi Alain, > > So, as mentioned in the other review, I'd like to evaluate other > > possibilities for the above: > > > > - One option is to enable it globally in probe(). Then you lose the > > possibility to have a device at address 0x08. > > I'd prefer avoid this solution to not lose the address 0x08. Understandably. > > - Enable it in probe() only if there is a generic binding "host-notify". > > Do you mean having the adapter walk through childs node and see if at least > one of them have the host-notify property ? This mean that such solution > wouldn't work for device relying on platform data rather than DT nodes. I meant a generic binding for the host-controller. It could be seen as a HW description if we need HostNotify on that bus or not. Maybe it becomes more clear with the R-Car I2C controller as an example. It only supports one slave address. If I want HostNotify there, I can't use another slave backend. Now, it could be that I need the slave EEPROM backend, although there is a HostNotify capable device on the bus. So, I am leaning to have a generic "host-notify" binding for the host. I consider platform_data legacy. If we use device_property, we should be safe regarding all current and future HW descriptions, or? > > - Let the core scan for a device with HOST_NOTIFY when registering an > > adapter and then call back into the driver somehow? > > You mean at adapter registration time only ? Not device probing time ? > At probing time, we could have the core (i2c_device_probe) check for the flag > HOST_NOTIFY and if setted call a dedicated host-notify reg callback ? As said above, I am leaning to the generic property. In addition, it doesn't feel right to me to add/remove the HostNotify feature at runtime depending on the client devices. Imagine someone changes another slave backend to address 0x08 and the HostNotify device comes later. Then, it won't work all of a sudden. It feels much safer to me to declare HostNotify as a feature of the IP core which it either has or it has not, configurable at boot-time. Makes sense? Kind regards, Wolfram signature.asc Description: PGP signature
Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
On Sat, May 23, 2020 at 01:01:40PM +0200, Wolfram Sang wrote:
>
> > +static int stm32f7_i2c_reg_client(struct i2c_client *client)
> > +{
> > + struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
> > + int ret;
> > +
> > + if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
> > + /* Only enable on the first device registration */
> > + if (atomic_inc_return(_dev->host_notify_cnt) == 1) {
> > + ret = stm32f7_i2c_enable_smbus_host(i2c_dev);
> > + if (ret) {
> > + dev_err(i2c_dev->dev,
> > + "failed to enable SMBus host notify
> > (%d)\n",
> > + ret);
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> So, as mentioned in the other review, I'd like to evaluate other
> possibilities for the above:
>
> - One option is to enable it globally in probe(). Then you lose the
> possibility to have a device at address 0x08.
I'd prefer avoid this solution to not lose the address 0x08.
> - Enable it in probe() only if there is a generic binding "host-notify".
Do you mean having the adapter walk through childs node and see if at least
one of them have the host-notify property ? This mean that such solution
wouldn't work for device relying on platform data rather than DT nodes.
> - Let the core scan for a device with HOST_NOTIFY when registering an
> adapter and then call back into the driver somehow?
You mean at adapter registration time only ? Not device probing time ?
At probing time, we could have the core (i2c_device_probe) check for the flag
HOST_NOTIFY and if setted call a dedicated host-notify reg callback ?
>
> Other ideas?
>
Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
> +static int stm32f7_i2c_reg_client(struct i2c_client *client)
> +{
> + struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(client->adapter);
> + int ret;
> +
> + if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
> + /* Only enable on the first device registration */
> + if (atomic_inc_return(_dev->host_notify_cnt) == 1) {
> + ret = stm32f7_i2c_enable_smbus_host(i2c_dev);
> + if (ret) {
> + dev_err(i2c_dev->dev,
> + "failed to enable SMBus host notify
> (%d)\n",
> + ret);
> + return ret;
> + }
> + }
> + }
> +
> + return 0;
> +}
So, as mentioned in the other review, I'd like to evaluate other
possibilities for the above:
- One option is to enable it globally in probe(). Then you lose the
possibility to have a device at address 0x08.
- Enable it in probe() only if there is a generic binding "host-notify".
- Let the core scan for a device with HOST_NOTIFY when registering an
adapter and then call back into the driver somehow?
Other ideas?
signature.asc
Description: PGP signature
Re: [PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
Hi all,
Reviewed-by: Pierre-Yves MORDRET
Thanks
On 5/5/20 7:51 AM, Alain Volmat wrote:
> This patch adds the support for SMBus Host notify and SMBus Alert
> extensions protocols
>
> Signed-off-by: Alain Volmat
> ---
> drivers/i2c/busses/Kconfig | 1 +
> drivers/i2c/busses/i2c-stm32f7.c | 198 +--
> 2 files changed, 189 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2f6e39b41e6c..b82c2f7d7d50 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1024,6 +1024,7 @@ config I2C_STM32F7
> tristate "STMicroelectronics STM32F7 I2C support"
> depends on ARCH_STM32 || COMPILE_TEST
> select I2C_SLAVE
> + select I2C_SMBUS
> help
> Enable this option to add support for STM32 I2C controller embedded
> in STM32F7 SoCs.
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c
> b/drivers/i2c/busses/i2c-stm32f7.c
> index 9c9e10ea9199..6d02ddbc1ab4 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -14,10 +14,12 @@
> * This driver is based on i2c-stm32f4.c
> *
> */
> +#include
> #include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -50,6 +52,8 @@
>
> /* STM32F7 I2C control 1 */
> #define STM32F7_I2C_CR1_PECENBIT(23)
> +#define STM32F7_I2C_CR1_ALERTEN BIT(22)
> +#define STM32F7_I2C_CR1_SMBHEN BIT(20)
> #define STM32F7_I2C_CR1_WUPENBIT(18)
> #define STM32F7_I2C_CR1_SBC BIT(16)
> #define STM32F7_I2C_CR1_RXDMAEN BIT(15)
> @@ -121,6 +125,7 @@
> (((n) & STM32F7_I2C_ISR_ADDCODE_MASK) >> 17)
> #define STM32F7_I2C_ISR_DIR BIT(16)
> #define STM32F7_I2C_ISR_BUSY BIT(15)
> +#define STM32F7_I2C_ISR_ALERTBIT(13)
> #define STM32F7_I2C_ISR_PECERR BIT(11)
> #define STM32F7_I2C_ISR_ARLO BIT(9)
> #define STM32F7_I2C_ISR_BERR BIT(8)
> @@ -134,6 +139,7 @@
> #define STM32F7_I2C_ISR_TXE BIT(0)
>
> /* STM32F7 I2C Interrupt Clear */
> +#define STM32F7_I2C_ICR_ALERTCF BIT(13)
> #define STM32F7_I2C_ICR_PECCFBIT(11)
> #define STM32F7_I2C_ICR_ARLOCF BIT(9)
> #define STM32F7_I2C_ICR_BERRCF BIT(8)
> @@ -150,7 +156,7 @@
>
> #define STM32F7_I2C_MAX_LEN 0xff
> #define STM32F7_I2C_DMA_LEN_MIN 0x16
> -#define STM32F7_I2C_MAX_SLAVE0x2
> +#define STM32F7_I2C_MAX_SLAVE0x3
>
> #define STM32F7_I2C_DNF_DEFAULT 0
> #define STM32F7_I2C_DNF_MAX 16
> @@ -274,6 +280,29 @@ struct stm32f7_i2c_msg {
> u8 smbus_buf[I2C_SMBUS_BLOCK_MAX + 3] __aligned(4);
> };
>
> +/**
> + * struct stm32f7_i2c_host - SMBus host specific data
> + * @client: I2C slave device that represents SMBus host
> + * @notify_start: indicate that this is the start of the notify transaction
> + * @addr: device address of SMBus device that initiate SMBus host protocol
> + */
> +struct stm32f7_i2c_host {
> + struct i2c_client *client;
> + bool notify_start;
> + u8 addr;
> +};
> +
> +/**
> + * struct stm32f7_i2c_alert - SMBus alert specific data
> + * @setup: platform data for the smbus_alert i2c client
> + * @ara: I2C slave device used to respond to the SMBus Alert with Alert
> + * Response Address
> + */
> +struct stm32f7_i2c_alert {
> + struct i2c_smbus_alert_setup setup;
> + struct i2c_client *ara;
> +};
> +
> /**
> * struct stm32f7_i2c_dev - private data of the controller
> * @adap: I2C adapter for this controller
> @@ -301,6 +330,9 @@ struct stm32f7_i2c_msg {
> * @fmp_creg: register address for clearing Fast Mode Plus bits
> * @fmp_mask: mask for Fast Mode Plus bits in set register
> * @wakeup_src: boolean to know if the device is a wakeup source
> + * @host_notify_cnt: atomic to know number of host_notify enabled clients
> + * @host_notify_client: SMBus host-notify client
> + * @alert: SMBus alert specific data
> */
> struct stm32f7_i2c_dev {
> struct i2c_adapter adap;
> @@ -327,6 +359,9 @@ struct stm32f7_i2c_dev {
> u32 fmp_creg;
> u32 fmp_mask;
> bool wakeup_src;
> + atomic_t host_notify_cnt;
> + struct i2c_client *host_notify_client;
> + struct stm32f7_i2c_alert *alert;
> };
>
> /*
> @@ -1321,10 +1356,20 @@ static int stm32f7_i2c_get_free_slave_id(struct
> stm32f7_i2c_dev *i2c_dev,
> int i;
>
> /*
> - * slave[0] supports 7-bit and 10-bit slave address
> - * slave[1] supports 7-bit slave address only
> + * slave[0] support only SMBus Host address (0x8)
> + * slave[1] supports 7-bit and 10-bit
[PATCH 4/4] i2c: stm32f7: Add SMBus-specific protocols support
This patch adds the support for SMBus Host notify and SMBus Alert
extensions protocols
Signed-off-by: Alain Volmat
---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-stm32f7.c | 198 +--
2 files changed, 189 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2f6e39b41e6c..b82c2f7d7d50 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1024,6 +1024,7 @@ config I2C_STM32F7
tristate "STMicroelectronics STM32F7 I2C support"
depends on ARCH_STM32 || COMPILE_TEST
select I2C_SLAVE
+ select I2C_SMBUS
help
Enable this option to add support for STM32 I2C controller embedded
in STM32F7 SoCs.
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 9c9e10ea9199..6d02ddbc1ab4 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -14,10 +14,12 @@
* This driver is based on i2c-stm32f4.c
*
*/
+#include
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -50,6 +52,8 @@
/* STM32F7 I2C control 1 */
#define STM32F7_I2C_CR1_PECEN BIT(23)
+#define STM32F7_I2C_CR1_ALERTENBIT(22)
+#define STM32F7_I2C_CR1_SMBHEN BIT(20)
#define STM32F7_I2C_CR1_WUPEN BIT(18)
#define STM32F7_I2C_CR1_SBCBIT(16)
#define STM32F7_I2C_CR1_RXDMAENBIT(15)
@@ -121,6 +125,7 @@
(((n) & STM32F7_I2C_ISR_ADDCODE_MASK) >> 17)
#define STM32F7_I2C_ISR_DIRBIT(16)
#define STM32F7_I2C_ISR_BUSY BIT(15)
+#define STM32F7_I2C_ISR_ALERT BIT(13)
#define STM32F7_I2C_ISR_PECERR BIT(11)
#define STM32F7_I2C_ISR_ARLO BIT(9)
#define STM32F7_I2C_ISR_BERR BIT(8)
@@ -134,6 +139,7 @@
#define STM32F7_I2C_ISR_TXEBIT(0)
/* STM32F7 I2C Interrupt Clear */
+#define STM32F7_I2C_ICR_ALERTCFBIT(13)
#define STM32F7_I2C_ICR_PECCF BIT(11)
#define STM32F7_I2C_ICR_ARLOCF BIT(9)
#define STM32F7_I2C_ICR_BERRCF BIT(8)
@@ -150,7 +156,7 @@
#define STM32F7_I2C_MAX_LEN0xff
#define STM32F7_I2C_DMA_LEN_MIN0x16
-#define STM32F7_I2C_MAX_SLAVE 0x2
+#define STM32F7_I2C_MAX_SLAVE 0x3
#define STM32F7_I2C_DNF_DEFAULT0
#define STM32F7_I2C_DNF_MAX16
@@ -274,6 +280,29 @@ struct stm32f7_i2c_msg {
u8 smbus_buf[I2C_SMBUS_BLOCK_MAX + 3] __aligned(4);
};
+/**
+ * struct stm32f7_i2c_host - SMBus host specific data
+ * @client: I2C slave device that represents SMBus host
+ * @notify_start: indicate that this is the start of the notify transaction
+ * @addr: device address of SMBus device that initiate SMBus host protocol
+ */
+struct stm32f7_i2c_host {
+ struct i2c_client *client;
+ bool notify_start;
+ u8 addr;
+};
+
+/**
+ * struct stm32f7_i2c_alert - SMBus alert specific data
+ * @setup: platform data for the smbus_alert i2c client
+ * @ara: I2C slave device used to respond to the SMBus Alert with Alert
+ * Response Address
+ */
+struct stm32f7_i2c_alert {
+ struct i2c_smbus_alert_setup setup;
+ struct i2c_client *ara;
+};
+
/**
* struct stm32f7_i2c_dev - private data of the controller
* @adap: I2C adapter for this controller
@@ -301,6 +330,9 @@ struct stm32f7_i2c_msg {
* @fmp_creg: register address for clearing Fast Mode Plus bits
* @fmp_mask: mask for Fast Mode Plus bits in set register
* @wakeup_src: boolean to know if the device is a wakeup source
+ * @host_notify_cnt: atomic to know number of host_notify enabled clients
+ * @host_notify_client: SMBus host-notify client
+ * @alert: SMBus alert specific data
*/
struct stm32f7_i2c_dev {
struct i2c_adapter adap;
@@ -327,6 +359,9 @@ struct stm32f7_i2c_dev {
u32 fmp_creg;
u32 fmp_mask;
bool wakeup_src;
+ atomic_t host_notify_cnt;
+ struct i2c_client *host_notify_client;
+ struct stm32f7_i2c_alert *alert;
};
/*
@@ -1321,10 +1356,20 @@ static int stm32f7_i2c_get_free_slave_id(struct
stm32f7_i2c_dev *i2c_dev,
int i;
/*
-* slave[0] supports 7-bit and 10-bit slave address
-* slave[1] supports 7-bit slave address only
+* slave[0] support only SMBus Host address (0x8)
+* slave[1] supports 7-bit and 10-bit slave address
+* slave[2] supports 7-bit slave address only
*/
- for (i = STM32F7_I2C_MAX_SLAVE - 1; i >= 0; i--) {
+ if (atomic_read(_dev->host_notify_cnt)) {
+ if (slave->addr == 0x08) {
+ if (i2c_dev->slave[0])
+ goto fail;
+
