Message ID | 20240430115111.3453-4-kabel@kernel.org (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Arnd Bergmann |
Headers | show |
Series | Turris Omnia MCU driver | expand |
On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote: > > Add support for GPIOs connected to the MCU on the Turris Omnia board. > > This includes: > - front button pin > - enable pins for USB regulators > - MiniPCIe / mSATA card presence pins in MiniPCIe port 0 > - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports > - on board revisions 32+ also various peripheral resets and another > voltage regulator enable pin ... > -int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply, > - unsigned int len) > +int omnia_cmd_write_read(const struct i2c_client *client, > + void *cmd, unsigned int cmd_len, > + void *reply, unsigned int reply_len) > { > struct i2c_msg msgs[2]; > - int ret; > + int ret, num; > > msgs[0].addr = client->addr; > msgs[0].flags = 0; > - msgs[0].len = 1; > - msgs[0].buf = &cmd; > - msgs[1].addr = client->addr; > - msgs[1].flags = I2C_M_RD; > - msgs[1].len = len; > - msgs[1].buf = reply; > - > - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > + msgs[0].len = cmd_len; > + msgs[0].buf = cmd; > + num = 1; > + > + if (reply_len) { > + msgs[1].addr = client->addr; > + msgs[1].flags = I2C_M_RD; > + msgs[1].len = reply_len; > + msgs[1].buf = reply; > + num++; > + } > + > + ret = i2c_transfer(client->adapter, msgs, num); > if (ret < 0) > return ret; > - if (ret != ARRAY_SIZE(msgs)) > + if (ret != num) > return -EIO; > > return 0; Hold on, you are "patching" the code you just brought by the previous patch?! No, just do from the beginning how it should be at the end. ... > +#include <linux/devm-helpers.h> Do you still need this? ... > +#define FRONT_BUTTON_RELEASE_DELAY 50 /* ms */ Use proper unit suffix instead of a comment like many others do. _MS here. ... > +static const char * const omnia_mcu_gpio_templates[64] = { > + /* GPIOs with value read from the 16-bit wide status */ > + [4] = "gpio%u.MiniPCIe0 Card Detect", > + [5] = "gpio%u.MiniPCIe0 mSATA Indicator", > + [6] = "gpio%u.Front USB3 port over-current", > + [7] = "gpio%u.Rear USB3 port over-current", > + [8] = "gpio%u.Front USB3 port power", > + [9] = "gpio%u.Rear USB3 port power", > + [12] = "gpio%u.Front Button", > + > + /* GPIOs with value read from the 32-bit wide extended status */ > + [16] = "gpio%u.SFP nDET", > + [28] = "gpio%u.MiniPCIe0 LED", > + [29] = "gpio%u.MiniPCIe1 LED", > + [30] = "gpio%u.MiniPCIe2 LED", > + [31] = "gpio%u.MiniPCIe0 PAN LED", > + [32] = "gpio%u.MiniPCIe1 PAN LED", > + [33] = "gpio%u.MiniPCIe2 PAN LED", > + [34] = "gpio%u.WAN PHY LED0", > + [35] = "gpio%u.WAN PHY LED1", > + [36] = "gpio%u.LAN switch p0 LED0", > + [37] = "gpio%u.LAN switch p0 LED1", > + [38] = "gpio%u.LAN switch p1 LED0", > + [39] = "gpio%u.LAN switch p1 LED1", > + [40] = "gpio%u.LAN switch p2 LED0", > + [41] = "gpio%u.LAN switch p2 LED1", > + [42] = "gpio%u.LAN switch p3 LED0", > + [43] = "gpio%u.LAN switch p3 LED1", > + [44] = "gpio%u.LAN switch p4 LED0", > + [45] = "gpio%u.LAN switch p4 LED1", > + [46] = "gpio%u.LAN switch p5 LED0", > + [47] = "gpio%u.LAN switch p5 LED1", > + > + /* GPIOs with value read from the 16-bit wide extended control status */ > + [48] = "gpio%u.eMMC nRESET", > + [49] = "gpio%u.LAN switch nRESET", > + [50] = "gpio%u.WAN PHY nRESET", > + [51] = "gpio%u.MiniPCIe0 nPERST", > + [52] = "gpio%u.MiniPCIe1 nPERST", > + [53] = "gpio%u.MiniPCIe2 nPERST", > + [54] = "gpio%u.WAN PHY SFP mux", > +}; You may reduce the memory footprint of these just by doing "gpio%u." part(s) outside. Here compiler won't compress this (as in the case of repetitive string literals), ... > +static const struct omnia_gpio { > + u8 cmd, ctl_cmd; > + u32 bit, ctl_bit; > + u32 int_bit; > + u16 feat, feat_mask; > +} omnia_gpios[64] = { It's much better to decouple definition and assignment and put definition _before_ the macros that fill it. > +}; ... > + scoped_guard(mutex, &mcu->lock) > + val = omnia_cmd_read_bit(mcu->client, > + CMD_GET_EXT_CONTROL_STATUS, > + EXT_CTL_PHY_SFP_AUTO); > + > + if (val < 0) > + return val; I would move that under guard for the sake of better readability (usual pattern in place). It's anyway a slow path and one branch under the mutex won't affect anything. > + if (val) > + return GPIO_LINE_DIRECTION_IN; > + > + return GPIO_LINE_DIRECTION_OUT; ... > + struct omnia_mcu *mcu = gpiochip_get_data(gc); > + u32 sts_bits, ext_sts_bits, ext_ctl_bits; > + int err, i; > + sts_bits = 0; > + ext_sts_bits = 0; > + ext_ctl_bits = 0; Why not assign these inside the definition line? ... > + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { > + if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD) > + __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit); > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD) > + __assign_bit(i, bits, ext_sts_bits & > + omnia_gpios[i].bit); One line? > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS) > + __assign_bit(i, bits, ext_ctl_bits & > + omnia_gpios[i].bit); Ditto? > + } ... > + struct omnia_mcu *mcu = gpiochip_get_data(gc); > + u16 ext_ctl, ext_ctl_mask; > + u8 ctl, ctl_mask; > + int i; > + > + ctl = 0; > + ctl_mask = 0; > + ext_ctl = 0; > + ext_ctl_mask = 0; Assignments in the definition line? ... > + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { > + if (omnia_gpios[i].ctl_cmd == CMD_GENERAL_CONTROL) { > + ctl_mask |= omnia_gpios[i].ctl_bit; > + if (test_bit(i, bits)) > + ctl |= omnia_gpios[i].ctl_bit; > + } else if (omnia_gpios[i].ctl_cmd == CMD_EXT_CONTROL) { > + ext_ctl_mask |= omnia_gpios[i].ctl_bit; > + if (test_bit(i, bits)) > + ext_ctl |= omnia_gpios[i].ctl_bit; > + } > + } ... > +/** > + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling > + * @dst: the destination u8 array of interleaved bytes > + * @rising: rising mask > + * @falling: falling mask > + * > + * Interleaves the little-endian bytes from @rising and @falling words. > + * > + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is > + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3). > + * > + * The MCU receives interrupt mask and reports pending interrupt bitmap int this an interrupt a pending int --> in ? > + * interleaved format. The rationale behind it is that the low-indexed bits are > + * more important - in many cases, the user will be interested only in > + * interrupts with indexes 0 to 7, and so the system can stop reading after > + * first 2 bytes (r0, f0), to save time on the slow I2C bus. > + * > + * Feel free to remove this function and its inverse, omnia_mask_deinterleave, > + * and use an appropriate bitmap_* function once such a function exists. > + */ > +static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling) > +{ > + for (int i = 0; i < sizeof(u32); ++i) { > + dst[2 * i] = rising >> (8 * i); > + dst[2 * i + 1] = falling >> (8 * i); > + } I will look at this later on, > +} > +static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling) > +{ > + *rising = *falling = 0; > + > + for (int i = 0; i < sizeof(u32); ++i) { > + *rising |= src[2 * i] << (8 * i); > + *falling |= src[2 * i + 1] << (8 * i); > + } > +} and into this. ... > +static size_t omnia_irq_compute_pending_length(u32 rising, u32 falling) > +{ > + size_t rlen = 0, flen = 0; > + > + if (rising) > + rlen = ((__fls(rising) >> 3) << 1) + 1; > + > + if (falling) > + flen = ((__fls(falling) >> 3) << 1) + 2; > + > + return max(rlen, flen); > +} I am not sure why you need this, but it can be done easily x = ((__fls(falling | rising) >> 3) << 1) + 1; if (falling) return x + 1; return x; and most likely you can drop minmax.h. ... > +static const char * const front_button_modes[2] = { "mcu", "cpu" }; 2 is not needed. ... > -#include <linux/i2c.h> ??? That is exactly the point to have things done from the beginning. > +#include <linux/bitops.h> > +#include <linux/gpio/driver.h> > #include <linux/if_ether.h> > +#include <linux/mutex.h> > #include <linux/types.h> > +#include <linux/workqueue.h> > #include <asm/byteorder.h> ... > + err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used in a few places. > + if (err) > + return err; -- With Best Regards, Andy Shevchenko
On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote: > > > > Add support for GPIOs connected to the MCU on the Turris Omnia board. > > > > This includes: > > - front button pin > > - enable pins for USB regulators > > - MiniPCIe / mSATA card presence pins in MiniPCIe port 0 > > - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports > > - on board revisions 32+ also various peripheral resets and another > > voltage regulator enable pin > > ... > > > -int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply, > > - unsigned int len) > > +int omnia_cmd_write_read(const struct i2c_client *client, > > + void *cmd, unsigned int cmd_len, > > + void *reply, unsigned int reply_len) > > { > > struct i2c_msg msgs[2]; > > - int ret; > > + int ret, num; > > > > msgs[0].addr = client->addr; > > msgs[0].flags = 0; > > - msgs[0].len = 1; > > - msgs[0].buf = &cmd; > > - msgs[1].addr = client->addr; > > - msgs[1].flags = I2C_M_RD; > > - msgs[1].len = len; > > - msgs[1].buf = reply; > > - > > - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > + msgs[0].len = cmd_len; > > + msgs[0].buf = cmd; > > + num = 1; > > + > > + if (reply_len) { > > + msgs[1].addr = client->addr; > > + msgs[1].flags = I2C_M_RD; > > + msgs[1].len = reply_len; > > + msgs[1].buf = reply; > > + num++; > > + } > > + > > + ret = i2c_transfer(client->adapter, msgs, num); > > if (ret < 0) > > return ret; > > - if (ret != ARRAY_SIZE(msgs)) > > + if (ret != num) > > return -EIO; > > > > return 0; > > Hold on, you are "patching" the code you just brought by the previous > patch?! No, just do from the beginning how it should be at the end. Fixed. > > +#include <linux/devm-helpers.h> > > Do you still need this? Yes, for devm_delayed_work_drop(). > ... > > > +#define FRONT_BUTTON_RELEASE_DELAY 50 /* ms */ > > Use proper unit suffix instead of a comment like many others do. > > _MS here. Fixed. > > +static const char * const omnia_mcu_gpio_templates[64] = { > > + /* GPIOs with value read from the 16-bit wide status */ > > + [4] = "gpio%u.MiniPCIe0 Card Detect", > > + [5] = "gpio%u.MiniPCIe0 mSATA Indicator", > > + [6] = "gpio%u.Front USB3 port over-current", > > + [7] = "gpio%u.Rear USB3 port over-current", > > + [8] = "gpio%u.Front USB3 port power", > > + [9] = "gpio%u.Rear USB3 port power", > > + [12] = "gpio%u.Front Button", > > + > > + /* GPIOs with value read from the 32-bit wide extended status */ > > + [16] = "gpio%u.SFP nDET", > > + [28] = "gpio%u.MiniPCIe0 LED", > > + [29] = "gpio%u.MiniPCIe1 LED", > > + [30] = "gpio%u.MiniPCIe2 LED", > > + [31] = "gpio%u.MiniPCIe0 PAN LED", > > + [32] = "gpio%u.MiniPCIe1 PAN LED", > > + [33] = "gpio%u.MiniPCIe2 PAN LED", > > + [34] = "gpio%u.WAN PHY LED0", > > + [35] = "gpio%u.WAN PHY LED1", > > + [36] = "gpio%u.LAN switch p0 LED0", > > + [37] = "gpio%u.LAN switch p0 LED1", > > + [38] = "gpio%u.LAN switch p1 LED0", > > + [39] = "gpio%u.LAN switch p1 LED1", > > + [40] = "gpio%u.LAN switch p2 LED0", > > + [41] = "gpio%u.LAN switch p2 LED1", > > + [42] = "gpio%u.LAN switch p3 LED0", > > + [43] = "gpio%u.LAN switch p3 LED1", > > + [44] = "gpio%u.LAN switch p4 LED0", > > + [45] = "gpio%u.LAN switch p4 LED1", > > + [46] = "gpio%u.LAN switch p5 LED0", > > + [47] = "gpio%u.LAN switch p5 LED1", > > + > > + /* GPIOs with value read from the 16-bit wide extended control status */ > > + [48] = "gpio%u.eMMC nRESET", > > + [49] = "gpio%u.LAN switch nRESET", > > + [50] = "gpio%u.WAN PHY nRESET", > > + [51] = "gpio%u.MiniPCIe0 nPERST", > > + [52] = "gpio%u.MiniPCIe1 nPERST", > > + [53] = "gpio%u.MiniPCIe2 nPERST", > > + [54] = "gpio%u.WAN PHY SFP mux", > > +}; > > You may reduce the memory footprint of these just by doing "gpio%u." > part(s) outside. Here compiler won't compress this (as in the case of > repetitive string literals), Are you saying that I should dynamically create another array just to pass it to the gpiochip's names pointer? > > +static const struct omnia_gpio { > > + u8 cmd, ctl_cmd; > > + u32 bit, ctl_bit; > > + u32 int_bit; > > + u16 feat, feat_mask; > > +} omnia_gpios[64] = { > > It's much better to decouple definition and assignment and put > definition _before_ the macros that fill it. Fixed. > > + scoped_guard(mutex, &mcu->lock) > > + val = omnia_cmd_read_bit(mcu->client, > > + CMD_GET_EXT_CONTROL_STATUS, > > + EXT_CTL_PHY_SFP_AUTO); > > + > > + if (val < 0) > > + return val; > > I would move that under guard for the sake of better readability > (usual pattern in place). It's anyway a slow path and one branch under > the mutex won't affect anything. OK. > > + if (val) > > + return GPIO_LINE_DIRECTION_IN; > > + > > + return GPIO_LINE_DIRECTION_OUT; > > ... > > > + struct omnia_mcu *mcu = gpiochip_get_data(gc); > > + u32 sts_bits, ext_sts_bits, ext_ctl_bits; > > + int err, i; > > > + sts_bits = 0; > > + ext_sts_bits = 0; > > + ext_ctl_bits = 0; > > Why not assign these inside the definition line? OK. > > + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { > > + if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD) > > + __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit); > > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD) > > + __assign_bit(i, bits, ext_sts_bits & > > + omnia_gpios[i].bit); > > One line? That would be 81 columns, which I would like to avoid. I can remove the _bits suffix from these variables, though. What do you think? > > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS) > > + __assign_bit(i, bits, ext_ctl_bits & > > + omnia_gpios[i].bit); > > Ditto? > > > + } > > ... > > > + struct omnia_mcu *mcu = gpiochip_get_data(gc); > > + u16 ext_ctl, ext_ctl_mask; > > + u8 ctl, ctl_mask; > > + int i; > > + > > + ctl = 0; > > + ctl_mask = 0; > > + ext_ctl = 0; > > + ext_ctl_mask = 0; > > Assignments in the definition line? OK > > + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { > > + if (omnia_gpios[i].ctl_cmd == CMD_GENERAL_CONTROL) { > > + ctl_mask |= omnia_gpios[i].ctl_bit; > > + if (test_bit(i, bits)) > > + ctl |= omnia_gpios[i].ctl_bit; > > + } else if (omnia_gpios[i].ctl_cmd == CMD_EXT_CONTROL) { > > + ext_ctl_mask |= omnia_gpios[i].ctl_bit; > > + if (test_bit(i, bits)) > > + ext_ctl |= omnia_gpios[i].ctl_bit; > > + } > > + } > > ... > > > +/** > > + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling > > + * @dst: the destination u8 array of interleaved bytes > > + * @rising: rising mask > > + * @falling: falling mask > > + * > > + * Interleaves the little-endian bytes from @rising and @falling words. > > + * > > + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is > > + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3). > > + * > > + * The MCU receives interrupt mask and reports pending interrupt bitmap int this > > an interrupt > a pending > int --> in ? Thx. > > + * interleaved format. The rationale behind it is that the low-indexed bits are > > + * more important - in many cases, the user will be interested only in > > + * interrupts with indexes 0 to 7, and so the system can stop reading after > > + * first 2 bytes (r0, f0), to save time on the slow I2C bus. > > + * > > + * Feel free to remove this function and its inverse, omnia_mask_deinterleave, > > + * and use an appropriate bitmap_* function once such a function exists. > > + */ > > +static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling) > > +{ > > + for (int i = 0; i < sizeof(u32); ++i) { > > + dst[2 * i] = rising >> (8 * i); > > + dst[2 * i + 1] = falling >> (8 * i); > > + } > > I will look at this later on, > > > +} > > > +static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling) > > +{ > > + *rising = *falling = 0; > > + > > + for (int i = 0; i < sizeof(u32); ++i) { > > + *rising |= src[2 * i] << (8 * i); > > + *falling |= src[2 * i + 1] << (8 * i); > > + } > > +} > > and into this. > > ... > > > +static size_t omnia_irq_compute_pending_length(u32 rising, u32 falling) > > +{ > > + size_t rlen = 0, flen = 0; > > + > > + if (rising) > > + rlen = ((__fls(rising) >> 3) << 1) + 1; > > + > > + if (falling) > > + flen = ((__fls(falling) >> 3) << 1) + 2; > > + > > + return max(rlen, flen); > > +} > > I am not sure why you need this, but it can be done easily > > x = ((__fls(falling | rising) >> 3) << 1) + 1; > if (falling) > return x + 1; > return x; > > and most likely you can drop minmax.h. This will return different results for for example when rising = 0x100 falling = 0x10 where we need to read only 3 bytes, but your version will say that we need 4 bytes. > > +static const char * const front_button_modes[2] = { "mcu", "cpu" }; > > 2 is not needed. OK > > -#include <linux/i2c.h> > > ??? That is exactly the point to have things done from the beginning. I must have somehow missed this before sending, I see that I've already fixed it in my working branch. > > +#include <linux/bitops.h> > > +#include <linux/gpio/driver.h> > > #include <linux/if_ether.h> > > +#include <linux/mutex.h> > > #include <linux/types.h> > > +#include <linux/workqueue.h> > > #include <asm/byteorder.h> > > ... > > > + err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); > > Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used > in a few places. > > > > + if (err) > > + return err; > > -- > With Best Regards, > Andy Shevchenko
On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: ... > > + err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); > > Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used > in a few places. It is used 3 times: rlen = ((__fls(rising) >> 3) << 1) + 1; flen = ((__fls(falling) >> 3) << 1) + 2; err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); The last one is not compatible with the first two (because of the "<< 1"). The first two instances are contained within a function that is dedicated to "computing needed reply length". I could probably do something like static inline unsigned int omnia_compute_reply_len(uin32_t mask, bool interleaved, unsigned int offset) { return ((__fls(mask) >> 3) << interleaved) + 1 + offset; } Then the 3 instances would become rlen = omnia_compute_reply_len(rising, true, 0); flen = omnia_compute_reply_len(falling, true, 1); err = omnia_cmd_read(client, cmd, &reply, omnia_compute_reply_len(bits, false, 0)); What do you think?
On Fri, May 03, 2024 at 10:43:06AM +0200, Marek Behún wrote: > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: ... > > > + err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); > > > > Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used > > in a few places. > > It is used 3 times: > rlen = ((__fls(rising) >> 3) << 1) + 1; > flen = ((__fls(falling) >> 3) << 1) + 2; > err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); > > The last one is not compatible with the first two (because of the "<< 1"). > > The first two instances are contained within a function that is dedicated > to "computing needed reply length". > > I could probably do something like > > static inline unsigned int > omnia_compute_reply_len(uin32_t mask, bool interleaved, unsigned int offset) > { > return ((__fls(mask) >> 3) << interleaved) + 1 + offset; > } > > Then the 3 instances would become > > rlen = omnia_compute_reply_len(rising, true, 0); > flen = omnia_compute_reply_len(falling, true, 1); > err = omnia_cmd_read(client, cmd, &reply, > omnia_compute_reply_len(bits, false, 0)); > > What do you think? Fine, but think about these bit operations, I believe it can be optimised.
On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote: > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote: ... > > > +static const char * const omnia_mcu_gpio_templates[64] = { > > > + /* GPIOs with value read from the 16-bit wide status */ > > > + [4] = "gpio%u.MiniPCIe0 Card Detect", > > > + [5] = "gpio%u.MiniPCIe0 mSATA Indicator", > > > + [6] = "gpio%u.Front USB3 port over-current", > > > + [7] = "gpio%u.Rear USB3 port over-current", > > > + [8] = "gpio%u.Front USB3 port power", > > > + [9] = "gpio%u.Rear USB3 port power", > > > + [12] = "gpio%u.Front Button", > > > + > > > + /* GPIOs with value read from the 32-bit wide extended status */ > > > + [16] = "gpio%u.SFP nDET", > > > + [28] = "gpio%u.MiniPCIe0 LED", > > > + [29] = "gpio%u.MiniPCIe1 LED", > > > + [30] = "gpio%u.MiniPCIe2 LED", > > > + [31] = "gpio%u.MiniPCIe0 PAN LED", > > > + [32] = "gpio%u.MiniPCIe1 PAN LED", > > > + [33] = "gpio%u.MiniPCIe2 PAN LED", > > > + [34] = "gpio%u.WAN PHY LED0", > > > + [35] = "gpio%u.WAN PHY LED1", > > > + [36] = "gpio%u.LAN switch p0 LED0", > > > + [37] = "gpio%u.LAN switch p0 LED1", > > > + [38] = "gpio%u.LAN switch p1 LED0", > > > + [39] = "gpio%u.LAN switch p1 LED1", > > > + [40] = "gpio%u.LAN switch p2 LED0", > > > + [41] = "gpio%u.LAN switch p2 LED1", > > > + [42] = "gpio%u.LAN switch p3 LED0", > > > + [43] = "gpio%u.LAN switch p3 LED1", > > > + [44] = "gpio%u.LAN switch p4 LED0", > > > + [45] = "gpio%u.LAN switch p4 LED1", > > > + [46] = "gpio%u.LAN switch p5 LED0", > > > + [47] = "gpio%u.LAN switch p5 LED1", > > > + > > > + /* GPIOs with value read from the 16-bit wide extended control status */ > > > + [48] = "gpio%u.eMMC nRESET", > > > + [49] = "gpio%u.LAN switch nRESET", > > > + [50] = "gpio%u.WAN PHY nRESET", > > > + [51] = "gpio%u.MiniPCIe0 nPERST", > > > + [52] = "gpio%u.MiniPCIe1 nPERST", > > > + [53] = "gpio%u.MiniPCIe2 nPERST", > > > + [54] = "gpio%u.WAN PHY SFP mux", > > > +}; > > > > You may reduce the memory footprint of these just by doing "gpio%u." > > part(s) outside. Here compiler won't compress this (as in the case of > > repetitive string literals), > > Are you saying that I should dynamically create another array just to > pass it to the gpiochip's names pointer? I have looked into this again and now I'm puzzled how you tested this. To me it seems that those gpio%u will go as a fixed string to the user space, there is no %u --> number substitution happens. Moreover, this data anyway is redundant. Userspace and debugfs all have line numbers being printed. ... > > > + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { > > > + if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD) > > > + __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit); > > > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD) > > > + __assign_bit(i, bits, ext_sts_bits & > > > + omnia_gpios[i].bit); > > > > One line? > > That would be 81 columns, which I would like to avoid. 81?! It's fine! Even documentation allows that for the readability. > I can remove the _bits suffix from these variables, though. What do you > think? Make sense as well. > > > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS) > > > + __assign_bit(i, bits, ext_ctl_bits & > > > + omnia_gpios[i].bit); > > > > Ditto? > > > > > + }
On Fri, May 03, 2024 at 08:33:25PM +0300, Andy Shevchenko wrote: > On Fri, May 03, 2024 at 10:43:06AM +0200, Marek Behún wrote: > > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: > > ... > > > > > + err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); > > > > > > Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used > > > in a few places. > > > > It is used 3 times: > > rlen = ((__fls(rising) >> 3) << 1) + 1; > > flen = ((__fls(falling) >> 3) << 1) + 2; > > err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); > > > > The last one is not compatible with the first two (because of the "<< 1"). > > > > The first two instances are contained within a function that is dedicated > > to "computing needed reply length". > > > > I could probably do something like > > > > static inline unsigned int > > omnia_compute_reply_len(uin32_t mask, bool interleaved, unsigned int offset) > > { > > return ((__fls(mask) >> 3) << interleaved) + 1 + offset; > > } > > > > Then the 3 instances would become > > > > rlen = omnia_compute_reply_len(rising, true, 0); > > flen = omnia_compute_reply_len(falling, true, 1); > > err = omnia_cmd_read(client, cmd, &reply, > > omnia_compute_reply_len(bits, false, 0)); > > > > What do you think? > > Fine, but think about these bit operations, I believe it can be optimised. I will try something, altough I fear it might need 64-bit operations, which may be even more suboptimal since the driver is supposed to run on armv7. Also, isn't this premature optimization? Since the computed length is then passed as a parameter to I2C reading, and the I2C bus is orders of magnitudes slower and the handling of I2C requests in Marvell I2C driver will kill any optimization I do here...
On Fri, May 03, 2024 at 09:51:17PM +0300, Andy Shevchenko wrote: > On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote: > > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote: > > ... > > > > > +static const char * const omnia_mcu_gpio_templates[64] = { > > > > + /* GPIOs with value read from the 16-bit wide status */ > > > > + [4] = "gpio%u.MiniPCIe0 Card Detect", > > > > + [5] = "gpio%u.MiniPCIe0 mSATA Indicator", > > > > + [6] = "gpio%u.Front USB3 port over-current", > > > > + [7] = "gpio%u.Rear USB3 port over-current", > > > > + [8] = "gpio%u.Front USB3 port power", > > > > + [9] = "gpio%u.Rear USB3 port power", > > > > + [12] = "gpio%u.Front Button", > > > > + > > > > + /* GPIOs with value read from the 32-bit wide extended status */ > > > > + [16] = "gpio%u.SFP nDET", > > > > + [28] = "gpio%u.MiniPCIe0 LED", > > > > + [29] = "gpio%u.MiniPCIe1 LED", > > > > + [30] = "gpio%u.MiniPCIe2 LED", > > > > + [31] = "gpio%u.MiniPCIe0 PAN LED", > > > > + [32] = "gpio%u.MiniPCIe1 PAN LED", > > > > + [33] = "gpio%u.MiniPCIe2 PAN LED", > > > > + [34] = "gpio%u.WAN PHY LED0", > > > > + [35] = "gpio%u.WAN PHY LED1", > > > > + [36] = "gpio%u.LAN switch p0 LED0", > > > > + [37] = "gpio%u.LAN switch p0 LED1", > > > > + [38] = "gpio%u.LAN switch p1 LED0", > > > > + [39] = "gpio%u.LAN switch p1 LED1", > > > > + [40] = "gpio%u.LAN switch p2 LED0", > > > > + [41] = "gpio%u.LAN switch p2 LED1", > > > > + [42] = "gpio%u.LAN switch p3 LED0", > > > > + [43] = "gpio%u.LAN switch p3 LED1", > > > > + [44] = "gpio%u.LAN switch p4 LED0", > > > > + [45] = "gpio%u.LAN switch p4 LED1", > > > > + [46] = "gpio%u.LAN switch p5 LED0", > > > > + [47] = "gpio%u.LAN switch p5 LED1", > > > > + > > > > + /* GPIOs with value read from the 16-bit wide extended control status */ > > > > + [48] = "gpio%u.eMMC nRESET", > > > > + [49] = "gpio%u.LAN switch nRESET", > > > > + [50] = "gpio%u.WAN PHY nRESET", > > > > + [51] = "gpio%u.MiniPCIe0 nPERST", > > > > + [52] = "gpio%u.MiniPCIe1 nPERST", > > > > + [53] = "gpio%u.MiniPCIe2 nPERST", > > > > + [54] = "gpio%u.WAN PHY SFP mux", > > > > +}; > > > > > > You may reduce the memory footprint of these just by doing "gpio%u." > > > part(s) outside. Here compiler won't compress this (as in the case of > > > repetitive string literals), > > > > Are you saying that I should dynamically create another array just to > > pass it to the gpiochip's names pointer? > > I have looked into this again and now I'm puzzled how you tested this. > To me it seems that those gpio%u will go as a fixed string to the user space, > there is no %u --> number substitution happens. Moreover, this data anyway > is redundant. Userspace and debugfs all have line numbers being printed. > It gets substituted in drivers/gpio/gpiolib-sysfs.c, function gpiod_export(): dev = device_create_with_groups(&gpio_class, &gdev->dev, MKDEV(0, 0), data, gpio_groups, ioname ? ioname : "gpio%u", desc_to_gpio(desc)); The ioname variable contains the string. This is then visible in sysfs: $ cd /sys/class/gpio $ echo 560 >export $ ls ... gpio560.eMMC nRESET ... > ... > > > > > + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { > > > > + if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD) > > > > + __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit); > > > > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD) > > > > + __assign_bit(i, bits, ext_sts_bits & > > > > + omnia_gpios[i].bit); > > > > > > One line? > > > > That would be 81 columns, which I would like to avoid. > > 81?! It's fine! Even documentation allows that for the readability. OK > > I can remove the _bits suffix from these variables, though. What do you > > think? > > Make sense as well. > > > > > + else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS) > > > > + __assign_bit(i, bits, ext_ctl_bits & > > > > + omnia_gpios[i].bit); > > > > > > Ditto? > > > > > > > + } > > -- > With Best Regards, > Andy Shevchenko > >
On Sun, May 5, 2024 at 11:18 AM Marek Behún <kabel@kernel.org> wrote: > On Fri, May 03, 2024 at 09:51:17PM +0300, Andy Shevchenko wrote: > > On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote: > > > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote: ... > > > > > +static const char * const omnia_mcu_gpio_templates[64] = { > > > > > + /* GPIOs with value read from the 16-bit wide status */ > > > > > + [4] = "gpio%u.MiniPCIe0 Card Detect", > > > > > + [5] = "gpio%u.MiniPCIe0 mSATA Indicator", > > > > > + [6] = "gpio%u.Front USB3 port over-current", > > > > > + [7] = "gpio%u.Rear USB3 port over-current", > > > > > + [8] = "gpio%u.Front USB3 port power", > > > > > + [9] = "gpio%u.Rear USB3 port power", > > > > > + [12] = "gpio%u.Front Button", > > > > > + > > > > > + /* GPIOs with value read from the 32-bit wide extended status */ > > > > > + [16] = "gpio%u.SFP nDET", > > > > > + [28] = "gpio%u.MiniPCIe0 LED", > > > > > + [29] = "gpio%u.MiniPCIe1 LED", > > > > > + [30] = "gpio%u.MiniPCIe2 LED", > > > > > + [31] = "gpio%u.MiniPCIe0 PAN LED", > > > > > + [32] = "gpio%u.MiniPCIe1 PAN LED", > > > > > + [33] = "gpio%u.MiniPCIe2 PAN LED", > > > > > + [34] = "gpio%u.WAN PHY LED0", > > > > > + [35] = "gpio%u.WAN PHY LED1", > > > > > + [36] = "gpio%u.LAN switch p0 LED0", > > > > > + [37] = "gpio%u.LAN switch p0 LED1", > > > > > + [38] = "gpio%u.LAN switch p1 LED0", > > > > > + [39] = "gpio%u.LAN switch p1 LED1", > > > > > + [40] = "gpio%u.LAN switch p2 LED0", > > > > > + [41] = "gpio%u.LAN switch p2 LED1", > > > > > + [42] = "gpio%u.LAN switch p3 LED0", > > > > > + [43] = "gpio%u.LAN switch p3 LED1", > > > > > + [44] = "gpio%u.LAN switch p4 LED0", > > > > > + [45] = "gpio%u.LAN switch p4 LED1", > > > > > + [46] = "gpio%u.LAN switch p5 LED0", > > > > > + [47] = "gpio%u.LAN switch p5 LED1", > > > > > + > > > > > + /* GPIOs with value read from the 16-bit wide extended control status */ > > > > > + [48] = "gpio%u.eMMC nRESET", > > > > > + [49] = "gpio%u.LAN switch nRESET", > > > > > + [50] = "gpio%u.WAN PHY nRESET", > > > > > + [51] = "gpio%u.MiniPCIe0 nPERST", > > > > > + [52] = "gpio%u.MiniPCIe1 nPERST", > > > > > + [53] = "gpio%u.MiniPCIe2 nPERST", > > > > > + [54] = "gpio%u.WAN PHY SFP mux", > > > > > +}; > > > > > > > > You may reduce the memory footprint of these just by doing "gpio%u." > > > > part(s) outside. Here compiler won't compress this (as in the case of > > > > repetitive string literals), > > > > > > Are you saying that I should dynamically create another array just to > > > pass it to the gpiochip's names pointer? > > > > I have looked into this again and now I'm puzzled how you tested this. > > To me it seems that those gpio%u will go as a fixed string to the user space, > > there is no %u --> number substitution happens. Moreover, this data anyway > > is redundant. Userspace and debugfs all have line numbers being printed. > > > > It gets substituted in drivers/gpio/gpiolib-sysfs.c, function > gpiod_export(): > > dev = device_create_with_groups(&gpio_class, &gdev->dev, > MKDEV(0, 0), data, gpio_groups, > ioname ? ioname : "gpio%u", > desc_to_gpio(desc)); > > The ioname variable contains the string. > > This is then visible in sysfs: > > $ cd /sys/class/gpio > $ echo 560 >export > $ ls > ... > gpio560.eMMC nRESET > ... Interesting. But before giving my conclusion on this, what is the output of /sys/kernel/debug/gpio and `gpioinfo` (the latter from libgpiod)?
On Sun, May 05, 2024 at 04:34:28PM +0300, Andy Shevchenko wrote: > On Sun, May 5, 2024 at 11:18 AM Marek Behún <kabel@kernel.org> wrote: > > On Fri, May 03, 2024 at 09:51:17PM +0300, Andy Shevchenko wrote: > > > On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote: > > > > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote: > > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote: > > ... > > > > > > > +static const char * const omnia_mcu_gpio_templates[64] = { > > > > > > + /* GPIOs with value read from the 16-bit wide status */ > > > > > > + [4] = "gpio%u.MiniPCIe0 Card Detect", > > > > > > + [5] = "gpio%u.MiniPCIe0 mSATA Indicator", > > > > > > + [6] = "gpio%u.Front USB3 port over-current", > > > > > > + [7] = "gpio%u.Rear USB3 port over-current", > > > > > > + [8] = "gpio%u.Front USB3 port power", > > > > > > + [9] = "gpio%u.Rear USB3 port power", > > > > > > + [12] = "gpio%u.Front Button", > > > > > > + > > > > > > + /* GPIOs with value read from the 32-bit wide extended status */ > > > > > > + [16] = "gpio%u.SFP nDET", > > > > > > + [28] = "gpio%u.MiniPCIe0 LED", > > > > > > + [29] = "gpio%u.MiniPCIe1 LED", > > > > > > + [30] = "gpio%u.MiniPCIe2 LED", > > > > > > + [31] = "gpio%u.MiniPCIe0 PAN LED", > > > > > > + [32] = "gpio%u.MiniPCIe1 PAN LED", > > > > > > + [33] = "gpio%u.MiniPCIe2 PAN LED", > > > > > > + [34] = "gpio%u.WAN PHY LED0", > > > > > > + [35] = "gpio%u.WAN PHY LED1", > > > > > > + [36] = "gpio%u.LAN switch p0 LED0", > > > > > > + [37] = "gpio%u.LAN switch p0 LED1", > > > > > > + [38] = "gpio%u.LAN switch p1 LED0", > > > > > > + [39] = "gpio%u.LAN switch p1 LED1", > > > > > > + [40] = "gpio%u.LAN switch p2 LED0", > > > > > > + [41] = "gpio%u.LAN switch p2 LED1", > > > > > > + [42] = "gpio%u.LAN switch p3 LED0", > > > > > > + [43] = "gpio%u.LAN switch p3 LED1", > > > > > > + [44] = "gpio%u.LAN switch p4 LED0", > > > > > > + [45] = "gpio%u.LAN switch p4 LED1", > > > > > > + [46] = "gpio%u.LAN switch p5 LED0", > > > > > > + [47] = "gpio%u.LAN switch p5 LED1", > > > > > > + > > > > > > + /* GPIOs with value read from the 16-bit wide extended control status */ > > > > > > + [48] = "gpio%u.eMMC nRESET", > > > > > > + [49] = "gpio%u.LAN switch nRESET", > > > > > > + [50] = "gpio%u.WAN PHY nRESET", > > > > > > + [51] = "gpio%u.MiniPCIe0 nPERST", > > > > > > + [52] = "gpio%u.MiniPCIe1 nPERST", > > > > > > + [53] = "gpio%u.MiniPCIe2 nPERST", > > > > > > + [54] = "gpio%u.WAN PHY SFP mux", > > > > > > +}; > > > > > > > > > > You may reduce the memory footprint of these just by doing "gpio%u." > > > > > part(s) outside. Here compiler won't compress this (as in the case of > > > > > repetitive string literals), > > > > > > > > Are you saying that I should dynamically create another array just to > > > > pass it to the gpiochip's names pointer? > > > > > > I have looked into this again and now I'm puzzled how you tested this. > > > To me it seems that those gpio%u will go as a fixed string to the user space, > > > there is no %u --> number substitution happens. Moreover, this data anyway > > > is redundant. Userspace and debugfs all have line numbers being printed. > > > > > > > It gets substituted in drivers/gpio/gpiolib-sysfs.c, function > > gpiod_export(): > > > > dev = device_create_with_groups(&gpio_class, &gdev->dev, > > MKDEV(0, 0), data, gpio_groups, > > ioname ? ioname : "gpio%u", > > desc_to_gpio(desc)); > > > > The ioname variable contains the string. > > > > This is then visible in sysfs: > > > > $ cd /sys/class/gpio > > $ echo 560 >export > > $ ls > > ... > > gpio560.eMMC nRESET > > ... > > Interesting. But before giving my conclusion on this, what is the > output of /sys/kernel/debug/gpio and `gpioinfo` (the latter from > libgpiod)? It prints %u, as you suspected and I see you already posted patch to avoid this. I'm dropping the "gpio%u." prefixes from GPIO line names. Marek
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu index ff5e7cb00206..b27b60d00c87 100644 --- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu @@ -22,6 +22,22 @@ Description: (RO) Contains device first MAC address. Each Turris Omnia is Format: %pM. +What: /sys/bus/i2c/devices/<mcu_device>/front_button_mode +Date: July 2024 +KernelVersion: 6.10 +Contact: Marek Behún <kabel@kernel.org> +Description: (RW) The front button on the Turris Omnia router can be + configured either to change the intensity of all the LEDs on the + front panel, or to send the press event to the CPU as an + interrupt. + + This file switches between these two modes: + - "mcu" makes the button press event be handled by the MCU to + change the LEDs panel intensity. + - "cpu" makes the button press event be handled by the CPU. + + Format: %s. + What: /sys/bus/i2c/devices/<mcu_device>/fw_features Date: July 2024 KernelVersion: 6.10 diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig index f8560ff9c1af..3a8c3edcd7e6 100644 --- a/drivers/platform/cznic/Kconfig +++ b/drivers/platform/cznic/Kconfig @@ -17,9 +17,24 @@ config TURRIS_OMNIA_MCU tristate "Turris Omnia MCU driver" depends on MACH_ARMADA_38X || COMPILE_TEST depends on I2C + select GPIOLIB + select GPIOLIB_IRQCHIP help Say Y here to add support for the features implemented by the microcontroller on the CZ.NIC's Turris Omnia SOHO router. + The features include: + - GPIO pins + - to get front button press events (the front button can be + configured either to generate press events to the CPU or to change + front LEDs panel brightness) + - to enable / disable USB port voltage regulators and to detect + USB overcurrent + - to detect MiniPCIe / mSATA card presence in MiniPCIe port 0 + - to configure resets of various peripherals on board revisions 32+ + - to enable / disable the VHV voltage regulator to the SOC in order + to be able to program SOC's OTP on board revisions 32+ + - to get input from the LED output pins of the WAN ethernet PHY, LAN + switch and MiniPCIe ports To compile this driver as a module, choose M here; the module will be called turris-omnia-mcu. diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile index 31adca73bb94..53fd8f1777a3 100644 --- a/drivers/platform/cznic/Makefile +++ b/drivers/platform/cznic/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o turris-omnia-mcu-y := turris-omnia-mcu-base.o +turris-omnia-mcu-y += turris-omnia-mcu-gpio.o diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c index 8893c990853a..708b084cb5da 100644 --- a/drivers/platform/cznic/turris-omnia-mcu-base.c +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c @@ -22,25 +22,31 @@ #define OMNIA_FW_VERSION_HEX_LEN (2 * OMNIA_FW_VERSION_LEN + 1) #define OMNIA_BOARD_INFO_LEN 16 -int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply, - unsigned int len) +int omnia_cmd_write_read(const struct i2c_client *client, + void *cmd, unsigned int cmd_len, + void *reply, unsigned int reply_len) { struct i2c_msg msgs[2]; - int ret; + int ret, num; msgs[0].addr = client->addr; msgs[0].flags = 0; - msgs[0].len = 1; - msgs[0].buf = &cmd; - msgs[1].addr = client->addr; - msgs[1].flags = I2C_M_RD; - msgs[1].len = len; - msgs[1].buf = reply; - - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); + msgs[0].len = cmd_len; + msgs[0].buf = cmd; + num = 1; + + if (reply_len) { + msgs[1].addr = client->addr; + msgs[1].flags = I2C_M_RD; + msgs[1].len = reply_len; + msgs[1].buf = reply; + num++; + } + + ret = i2c_transfer(client->adapter, msgs, num); if (ret < 0) return ret; - if (ret != ARRAY_SIZE(msgs)) + if (ret != num) return -EIO; return 0; @@ -188,6 +194,7 @@ static const struct attribute_group omnia_mcu_base_group = { static const struct attribute_group *omnia_mcu_groups[] = { &omnia_mcu_base_group, + &omnia_mcu_gpio_group, NULL }; @@ -353,7 +360,7 @@ static int omnia_mcu_probe(struct i2c_client *client) "Cannot read board info\n"); } - return 0; + return omnia_mcu_register_gpiochip(mcu); } static const struct of_device_id of_omnia_mcu_match[] = { diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c new file mode 100644 index 000000000000..bcc12bd1453c --- /dev/null +++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c @@ -0,0 +1,1048 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * CZ.NIC's Turris Omnia MCU GPIO and IRQ driver + * + * 2024 by Marek Behún <kabel@kernel.org> + */ + +#include <linux/array_size.h> +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/bug.h> +#include <linux/cleanup.h> +#include <linux/device.h> +#include <linux/devm-helpers.h> +#include <linux/gpio/driver.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/mutex.h> +#include <linux/sysfs.h> +#include <linux/turris-omnia-mcu-interface.h> +#include <linux/types.h> +#include <linux/workqueue.h> +#include <asm/unaligned.h> + +#include "turris-omnia-mcu.h" + +#define CMD_INT_ARG_LEN 8 +#define FRONT_BUTTON_RELEASE_DELAY 50 /* ms */ + +static const char * const omnia_mcu_gpio_templates[64] = { + /* GPIOs with value read from the 16-bit wide status */ + [4] = "gpio%u.MiniPCIe0 Card Detect", + [5] = "gpio%u.MiniPCIe0 mSATA Indicator", + [6] = "gpio%u.Front USB3 port over-current", + [7] = "gpio%u.Rear USB3 port over-current", + [8] = "gpio%u.Front USB3 port power", + [9] = "gpio%u.Rear USB3 port power", + [12] = "gpio%u.Front Button", + + /* GPIOs with value read from the 32-bit wide extended status */ + [16] = "gpio%u.SFP nDET", + [28] = "gpio%u.MiniPCIe0 LED", + [29] = "gpio%u.MiniPCIe1 LED", + [30] = "gpio%u.MiniPCIe2 LED", + [31] = "gpio%u.MiniPCIe0 PAN LED", + [32] = "gpio%u.MiniPCIe1 PAN LED", + [33] = "gpio%u.MiniPCIe2 PAN LED", + [34] = "gpio%u.WAN PHY LED0", + [35] = "gpio%u.WAN PHY LED1", + [36] = "gpio%u.LAN switch p0 LED0", + [37] = "gpio%u.LAN switch p0 LED1", + [38] = "gpio%u.LAN switch p1 LED0", + [39] = "gpio%u.LAN switch p1 LED1", + [40] = "gpio%u.LAN switch p2 LED0", + [41] = "gpio%u.LAN switch p2 LED1", + [42] = "gpio%u.LAN switch p3 LED0", + [43] = "gpio%u.LAN switch p3 LED1", + [44] = "gpio%u.LAN switch p4 LED0", + [45] = "gpio%u.LAN switch p4 LED1", + [46] = "gpio%u.LAN switch p5 LED0", + [47] = "gpio%u.LAN switch p5 LED1", + + /* GPIOs with value read from the 16-bit wide extended control status */ + [48] = "gpio%u.eMMC nRESET", + [49] = "gpio%u.LAN switch nRESET", + [50] = "gpio%u.WAN PHY nRESET", + [51] = "gpio%u.MiniPCIe0 nPERST", + [52] = "gpio%u.MiniPCIe1 nPERST", + [53] = "gpio%u.MiniPCIe2 nPERST", + [54] = "gpio%u.WAN PHY SFP mux", +}; + +#define _DEF_GPIO(_cmd, _ctl_cmd, _bit, _ctl_bit, _int_bit, _feat, _feat_mask) \ + { \ + .cmd = _cmd, \ + .ctl_cmd = _ctl_cmd, \ + .bit = _bit, \ + .ctl_bit = _ctl_bit, \ + .int_bit = _int_bit, \ + .feat = _feat, \ + .feat_mask = _feat_mask, \ + } +#define _DEF_GPIO_STS(_name) \ + _DEF_GPIO(CMD_GET_STATUS_WORD, 0, STS_ ## _name, 0, INT_ ## _name, 0, 0) +#define _DEF_GPIO_CTL(_name) \ + _DEF_GPIO(CMD_GET_STATUS_WORD, CMD_GENERAL_CONTROL, STS_ ## _name, \ + CTL_ ## _name, 0, 0, 0) +#define _DEF_GPIO_EXT_STS(_name, _feat) \ + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \ + INT_ ## _name, FEAT_ ## _feat | FEAT_EXT_CMDS, \ + FEAT_ ## _feat | FEAT_EXT_CMDS) +#define _DEF_GPIO_EXT_STS_LED(_name, _ledext) \ + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \ + INT_ ## _name, FEAT_LED_STATE_ ## _ledext, \ + FEAT_LED_STATE_EXT_MASK) +#define _DEF_GPIO_EXT_STS_LEDALL(_name) \ + _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \ + INT_ ## _name, FEAT_LED_STATE_EXT_MASK, 0) +#define _DEF_GPIO_EXT_CTL(_name, _feat) \ + _DEF_GPIO(CMD_GET_EXT_CONTROL_STATUS, CMD_EXT_CONTROL, \ + EXT_CTL_ ## _name, EXT_CTL_ ## _name, 0, \ + FEAT_ ## _feat | FEAT_EXT_CMDS, \ + FEAT_ ## _feat | FEAT_EXT_CMDS) +#define _DEF_INT(_name) \ + _DEF_GPIO(0, 0, 0, 0, INT_ ## _name, 0, 0) + +static const struct omnia_gpio { + u8 cmd, ctl_cmd; + u32 bit, ctl_bit; + u32 int_bit; + u16 feat, feat_mask; +} omnia_gpios[64] = { + /* GPIOs with value read from the 16-bit wide status */ + [4] = _DEF_GPIO_STS(CARD_DET), + [5] = _DEF_GPIO_STS(MSATA_IND), + [6] = _DEF_GPIO_STS(USB30_OVC), + [7] = _DEF_GPIO_STS(USB31_OVC), + [8] = _DEF_GPIO_CTL(USB30_PWRON), + [9] = _DEF_GPIO_CTL(USB31_PWRON), + + /* brightness changed interrupt, no GPIO */ + [11] = _DEF_INT(BRIGHTNESS_CHANGED), + + [12] = _DEF_GPIO_STS(BUTTON_PRESSED), + + /* TRNG interrupt, no GPIO */ + [13] = _DEF_INT(TRNG), + + /* MESSAGE_SIGNED interrupt, no GPIO */ + [14] = _DEF_INT(MESSAGE_SIGNED), + + /* GPIOs with value read from the 32-bit wide extended status */ + [16] = _DEF_GPIO_EXT_STS(SFP_nDET, PERIPH_MCU), + [28] = _DEF_GPIO_EXT_STS_LEDALL(WLAN0_MSATA_LED), + [29] = _DEF_GPIO_EXT_STS_LEDALL(WLAN1_LED), + [30] = _DEF_GPIO_EXT_STS_LEDALL(WLAN2_LED), + [31] = _DEF_GPIO_EXT_STS_LED(WPAN0_LED, EXT), + [32] = _DEF_GPIO_EXT_STS_LED(WPAN1_LED, EXT), + [33] = _DEF_GPIO_EXT_STS_LED(WPAN2_LED, EXT), + [34] = _DEF_GPIO_EXT_STS_LEDALL(WAN_LED0), + [35] = _DEF_GPIO_EXT_STS_LED(WAN_LED1, EXT_V32), + [36] = _DEF_GPIO_EXT_STS_LEDALL(LAN0_LED0), + [37] = _DEF_GPIO_EXT_STS_LEDALL(LAN0_LED1), + [38] = _DEF_GPIO_EXT_STS_LEDALL(LAN1_LED0), + [39] = _DEF_GPIO_EXT_STS_LEDALL(LAN1_LED1), + [40] = _DEF_GPIO_EXT_STS_LEDALL(LAN2_LED0), + [41] = _DEF_GPIO_EXT_STS_LEDALL(LAN2_LED1), + [42] = _DEF_GPIO_EXT_STS_LEDALL(LAN3_LED0), + [43] = _DEF_GPIO_EXT_STS_LEDALL(LAN3_LED1), + [44] = _DEF_GPIO_EXT_STS_LEDALL(LAN4_LED0), + [45] = _DEF_GPIO_EXT_STS_LEDALL(LAN4_LED1), + [46] = _DEF_GPIO_EXT_STS_LEDALL(LAN5_LED0), + [47] = _DEF_GPIO_EXT_STS_LEDALL(LAN5_LED1), + + /* GPIOs with value read from the 16-bit wide extended control status */ + [48] = _DEF_GPIO_EXT_CTL(nRES_MMC, PERIPH_MCU), + [49] = _DEF_GPIO_EXT_CTL(nRES_LAN, PERIPH_MCU), + [50] = _DEF_GPIO_EXT_CTL(nRES_PHY, PERIPH_MCU), + [51] = _DEF_GPIO_EXT_CTL(nPERST0, PERIPH_MCU), + [52] = _DEF_GPIO_EXT_CTL(nPERST1, PERIPH_MCU), + [53] = _DEF_GPIO_EXT_CTL(nPERST2, PERIPH_MCU), + [54] = _DEF_GPIO_EXT_CTL(PHY_SFP, PERIPH_MCU), +}; + +/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */ +static const u8 omnia_int_to_gpio_idx[32] = { + [__bf_shf(INT_CARD_DET)] = 4, + [__bf_shf(INT_MSATA_IND)] = 5, + [__bf_shf(INT_USB30_OVC)] = 6, + [__bf_shf(INT_USB31_OVC)] = 7, + [__bf_shf(INT_BUTTON_PRESSED)] = 12, + [__bf_shf(INT_TRNG)] = 13, + [__bf_shf(INT_MESSAGE_SIGNED)] = 14, + [__bf_shf(INT_SFP_nDET)] = 16, + [__bf_shf(INT_BRIGHTNESS_CHANGED)] = 11, + [__bf_shf(INT_WLAN0_MSATA_LED)] = 28, + [__bf_shf(INT_WLAN1_LED)] = 29, + [__bf_shf(INT_WLAN2_LED)] = 30, + [__bf_shf(INT_WPAN0_LED)] = 31, + [__bf_shf(INT_WPAN1_LED)] = 32, + [__bf_shf(INT_WPAN2_LED)] = 33, + [__bf_shf(INT_WAN_LED0)] = 34, + [__bf_shf(INT_WAN_LED1)] = 35, + [__bf_shf(INT_LAN0_LED0)] = 36, + [__bf_shf(INT_LAN0_LED1)] = 37, + [__bf_shf(INT_LAN1_LED0)] = 38, + [__bf_shf(INT_LAN1_LED1)] = 39, + [__bf_shf(INT_LAN2_LED0)] = 40, + [__bf_shf(INT_LAN2_LED1)] = 41, + [__bf_shf(INT_LAN3_LED0)] = 42, + [__bf_shf(INT_LAN3_LED1)] = 43, + [__bf_shf(INT_LAN4_LED0)] = 44, + [__bf_shf(INT_LAN4_LED1)] = 45, + [__bf_shf(INT_LAN5_LED0)] = 46, + [__bf_shf(INT_LAN5_LED1)] = 47, +}; + +/* index of PHY_SFP GPIO in the omnia_gpios array */ +#define OMNIA_GPIO_PHY_SFP_OFFSET 54 + +static int omnia_ctl_cmd_locked(struct omnia_mcu *mcu, u8 cmd, u16 val, + u16 mask) +{ + size_t len; + u8 buf[5]; + + buf[0] = cmd; + + switch (cmd) { + case CMD_GENERAL_CONTROL: + buf[1] = val; + buf[2] = mask; + len = 3; + break; + + case CMD_EXT_CONTROL: + put_unaligned_le16(val, &buf[1]); + put_unaligned_le16(mask, &buf[3]); + len = 5; + break; + + default: + BUG(); + } + + return omnia_cmd_write(mcu->client, buf, len); +} + +static int omnia_ctl_cmd(struct omnia_mcu *mcu, u8 cmd, u16 val, u16 mask) +{ + guard(mutex)(&mcu->lock); + + return omnia_ctl_cmd_locked(mcu, cmd, val, mask); +} + +static int omnia_gpio_request(struct gpio_chip *gc, unsigned int offset) +{ + if (!omnia_gpios[offset].cmd) + return -EINVAL; + + return 0; +} + +static int omnia_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + struct omnia_mcu *mcu = gpiochip_get_data(gc); + + if (offset == OMNIA_GPIO_PHY_SFP_OFFSET) { + int val; + + scoped_guard(mutex, &mcu->lock) + val = omnia_cmd_read_bit(mcu->client, + CMD_GET_EXT_CONTROL_STATUS, + EXT_CTL_PHY_SFP_AUTO); + + if (val < 0) + return val; + + if (val) + return GPIO_LINE_DIRECTION_IN; + + return GPIO_LINE_DIRECTION_OUT; + } + + if (omnia_gpios[offset].ctl_cmd) + return GPIO_LINE_DIRECTION_OUT; + + return GPIO_LINE_DIRECTION_IN; +} + +static int omnia_gpio_direction_input(struct gpio_chip *gc, unsigned int offset) +{ + const struct omnia_gpio *gpio = &omnia_gpios[offset]; + struct omnia_mcu *mcu = gpiochip_get_data(gc); + + if (offset == OMNIA_GPIO_PHY_SFP_OFFSET) + return omnia_ctl_cmd(mcu, CMD_EXT_CONTROL, EXT_CTL_PHY_SFP_AUTO, + EXT_CTL_PHY_SFP_AUTO); + + if (gpio->ctl_cmd) + return -ENOTSUPP; + + return 0; +} + +static int omnia_gpio_direction_output(struct gpio_chip *gc, + unsigned int offset, int value) +{ + const struct omnia_gpio *gpio = &omnia_gpios[offset]; + struct omnia_mcu *mcu = gpiochip_get_data(gc); + u16 val, mask; + + if (!gpio->ctl_cmd) + return -ENOTSUPP; + + mask = gpio->ctl_bit; + val = value ? mask : 0; + + if (offset == OMNIA_GPIO_PHY_SFP_OFFSET) + mask |= EXT_CTL_PHY_SFP_AUTO; + + return omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask); +} + +static int omnia_gpio_get(struct gpio_chip *gc, unsigned int offset) +{ + const struct omnia_gpio *gpio = &omnia_gpios[offset]; + struct omnia_mcu *mcu = gpiochip_get_data(gc); + + /* + * If firmware does not support the new interrupt API, we are informed + * of every change of the status word by an interrupt from MCU and save + * its value in the interrupt service routine. Simply return the saved + * value. + */ + if (gpio->cmd == CMD_GET_STATUS_WORD && + !(mcu->features & FEAT_NEW_INT_API)) + return !!(mcu->last_status & gpio->bit); + + guard(mutex)(&mcu->lock); + + /* + * If firmware does support the new interrupt API, we may have cached + * the value of a GPIO in the interrupt service routine. If not, read + * the relevant bit now. + */ + if (gpio->int_bit && (mcu->is_cached & gpio->int_bit)) + return !!(mcu->cached & gpio->int_bit); + + return omnia_cmd_read_bit(mcu->client, gpio->cmd, gpio->bit); +} + +static int omnia_gpio_get_multiple(struct gpio_chip *gc, unsigned long *mask, + unsigned long *bits) +{ + struct omnia_mcu *mcu = gpiochip_get_data(gc); + u32 sts_bits, ext_sts_bits, ext_ctl_bits; + int err, i; + + sts_bits = 0; + ext_sts_bits = 0; + ext_ctl_bits = 0; + + /* determine which bits to read from the 3 possible commands */ + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { + if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD) + sts_bits |= omnia_gpios[i].bit; + else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD) + ext_sts_bits |= omnia_gpios[i].bit; + else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS) + ext_ctl_bits |= omnia_gpios[i].bit; + } + + guard(mutex)(&mcu->lock); + + if (mcu->features & FEAT_NEW_INT_API) { + /* read relevant bits from status */ + err = omnia_cmd_read_bits(mcu->client, CMD_GET_STATUS_WORD, + sts_bits, &sts_bits); + if (err) + return err; + } else { + /* + * Use status word value cached in the interrupt service routine + * if firmware does not support the new interrupt API. + */ + sts_bits = mcu->last_status; + } + + /* read relevant bits from extended status */ + err = omnia_cmd_read_bits(mcu->client, CMD_GET_EXT_STATUS_DWORD, + ext_sts_bits, &ext_sts_bits); + if (err) + return err; + + /* read relevant bits from extended control */ + err = omnia_cmd_read_bits(mcu->client, CMD_GET_EXT_CONTROL_STATUS, + ext_ctl_bits, &ext_ctl_bits); + if (err) + return err; + + /* assign relevant bits in result */ + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { + if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD) + __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit); + else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD) + __assign_bit(i, bits, ext_sts_bits & + omnia_gpios[i].bit); + else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS) + __assign_bit(i, bits, ext_ctl_bits & + omnia_gpios[i].bit); + } + + return 0; +} + +static void omnia_gpio_set(struct gpio_chip *gc, unsigned int offset, int value) +{ + const struct omnia_gpio *gpio = &omnia_gpios[offset]; + struct omnia_mcu *mcu = gpiochip_get_data(gc); + u16 val, mask; + + if (!gpio->ctl_cmd) + return; + + mask = gpio->ctl_bit; + val = value ? mask : 0; + + omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask); +} + +static void omnia_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, + unsigned long *bits) +{ + struct omnia_mcu *mcu = gpiochip_get_data(gc); + u16 ext_ctl, ext_ctl_mask; + u8 ctl, ctl_mask; + int i; + + ctl = 0; + ctl_mask = 0; + ext_ctl = 0; + ext_ctl_mask = 0; + + for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) { + if (omnia_gpios[i].ctl_cmd == CMD_GENERAL_CONTROL) { + ctl_mask |= omnia_gpios[i].ctl_bit; + if (test_bit(i, bits)) + ctl |= omnia_gpios[i].ctl_bit; + } else if (omnia_gpios[i].ctl_cmd == CMD_EXT_CONTROL) { + ext_ctl_mask |= omnia_gpios[i].ctl_bit; + if (test_bit(i, bits)) + ext_ctl |= omnia_gpios[i].ctl_bit; + } + } + + guard(mutex)(&mcu->lock); + + if (ctl_mask) + omnia_ctl_cmd_locked(mcu, CMD_GENERAL_CONTROL, ctl, ctl_mask); + + if (ext_ctl_mask) + omnia_ctl_cmd_locked(mcu, CMD_EXT_CONTROL, ext_ctl, + ext_ctl_mask); +} + +static bool omnia_gpio_available(struct omnia_mcu *mcu, + const struct omnia_gpio *gpio) +{ + if (gpio->feat_mask) + return (mcu->features & gpio->feat_mask) == gpio->feat; + + if (gpio->feat) + return mcu->features & gpio->feat; + + return true; +} + +static int omnia_gpio_init_valid_mask(struct gpio_chip *gc, + unsigned long *valid_mask, + unsigned int ngpios) +{ + struct omnia_mcu *mcu = gpiochip_get_data(gc); + + for (unsigned int i = 0; i < ngpios; i++) { + const struct omnia_gpio *gpio = &omnia_gpios[i]; + + if (gpio->cmd || gpio->int_bit) + __assign_bit(i, valid_mask, + omnia_gpio_available(mcu, gpio)); + else + __clear_bit(i, valid_mask); + } + + return 0; +} + +static int omnia_gpio_of_xlate(struct gpio_chip *gc, + const struct of_phandle_args *gpiospec, + u32 *flags) +{ + u32 bank, gpio; + + if (WARN_ON(gpiospec->args_count != 3)) + return -EINVAL; + + if (flags) + *flags = gpiospec->args[2]; + + bank = gpiospec->args[0]; + gpio = gpiospec->args[1]; + + switch (bank) { + case 0: + return gpio < 16 ? gpio : -EINVAL; + case 1: + return gpio < 32 ? 16 + gpio : -EINVAL; + case 2: + return gpio < 16 ? 48 + gpio : -EINVAL; + default: + return -EINVAL; + } +} + +static void omnia_irq_shutdown(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct omnia_mcu *mcu = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + u32 bit = omnia_gpios[hwirq].int_bit; + + mcu->rising &= ~bit; + mcu->falling &= ~bit; +} + +static void omnia_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct omnia_mcu *mcu = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + u32 bit = omnia_gpios[hwirq].int_bit; + + if (!omnia_gpios[hwirq].cmd) + mcu->rising &= ~bit; + mcu->mask &= ~bit; + gpiochip_disable_irq(gc, hwirq); +} + +static void omnia_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct omnia_mcu *mcu = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + u32 bit = omnia_gpios[hwirq].int_bit; + + gpiochip_enable_irq(gc, hwirq); + mcu->mask |= bit; + if (!omnia_gpios[hwirq].cmd) + mcu->rising |= bit; +} + +static int omnia_irq_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct omnia_mcu *mcu = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct device *dev = &mcu->client->dev; + u32 bit = omnia_gpios[hwirq].int_bit; + + if (!(type & IRQ_TYPE_EDGE_BOTH)) { + dev_err(dev, "irq %u: unsupported type %u\n", d->irq, type); + return -EINVAL; + } + + if (type & IRQ_TYPE_EDGE_RISING) + mcu->rising |= bit; + else + mcu->rising &= ~bit; + + if (type & IRQ_TYPE_EDGE_FALLING) + mcu->falling |= bit; + else + mcu->falling &= ~bit; + + return 0; +} + +static void omnia_irq_bus_lock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct omnia_mcu *mcu = gpiochip_get_data(gc); + + /* nothing to do if MCU firmware does not support new interrupt API */ + if (!(mcu->features & FEAT_NEW_INT_API)) + return; + + mutex_lock(&mcu->lock); +} + +/** + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling + * @dst: the destination u8 array of interleaved bytes + * @rising: rising mask + * @falling: falling mask + * + * Interleaves the little-endian bytes from @rising and @falling words. + * + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3). + * + * The MCU receives interrupt mask and reports pending interrupt bitmap int this + * interleaved format. The rationale behind it is that the low-indexed bits are + * more important - in many cases, the user will be interested only in + * interrupts with indexes 0 to 7, and so the system can stop reading after + * first 2 bytes (r0, f0), to save time on the slow I2C bus. + * + * Feel free to remove this function and its inverse, omnia_mask_deinterleave, + * and use an appropriate bitmap_* function once such a function exists. + */ +static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling) +{ + for (int i = 0; i < sizeof(u32); ++i) { + dst[2 * i] = rising >> (8 * i); + dst[2 * i + 1] = falling >> (8 * i); + } +} + +/** + * omnia_mask_deinterleave - Deinterleaves the bytes into @rising and @falling + * @src: the source u8 array containing the interleaved bytes + * @rising: pointer where to store the rising mask gathered from @src + * @falling: pointer where to store the falling mask gathered from @src + * + * This is the inverse function to omnia_mask_interleave. + */ +static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling) +{ + *rising = *falling = 0; + + for (int i = 0; i < sizeof(u32); ++i) { + *rising |= src[2 * i] << (8 * i); + *falling |= src[2 * i + 1] << (8 * i); + } +} + +static void omnia_irq_bus_sync_unlock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct omnia_mcu *mcu = gpiochip_get_data(gc); + struct device *dev = &mcu->client->dev; + u8 cmd[1 + CMD_INT_ARG_LEN]; + u32 rising, falling; + int err; + + /* nothing to do if MCU firmware does not support new interrupt API */ + if (!(mcu->features & FEAT_NEW_INT_API)) + return; + + cmd[0] = CMD_SET_INT_MASK; + + rising = mcu->rising & mcu->mask; + falling = mcu->falling & mcu->mask; + + /* interleave the rising and falling bytes into the command arguments */ + omnia_mask_interleave(&cmd[1], rising, falling); + + dev_dbg(dev, "set int mask %8ph\n", &cmd[1]); + + err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd)); + if (err) { + dev_err(dev, "Cannot set mask: %d\n", err); + goto unlock; + } + + /* + * Remember which GPIOs have both rising and falling interrupts enabled. + * For those we will cache their value so that .get() method is faster. + * We also need to forget cached values of GPIOs that aren't cached + * anymore. + */ + mcu->both = rising & falling; + mcu->is_cached &= mcu->both; + +unlock: + mutex_unlock(&mcu->lock); +} + +static const struct irq_chip omnia_mcu_irq_chip = { + .name = "Turris Omnia MCU interrupts", + .irq_shutdown = omnia_irq_shutdown, + .irq_mask = omnia_irq_mask, + .irq_unmask = omnia_irq_unmask, + .irq_set_type = omnia_irq_set_type, + .irq_bus_lock = omnia_irq_bus_lock, + .irq_bus_sync_unlock = omnia_irq_bus_sync_unlock, + .flags = IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + +static void omnia_irq_init_valid_mask(struct gpio_chip *gc, + unsigned long *valid_mask, + unsigned int ngpios) +{ + struct omnia_mcu *mcu = gpiochip_get_data(gc); + + for (unsigned int i = 0; i < ngpios; i++) { + const struct omnia_gpio *gpio = &omnia_gpios[i]; + + if (gpio->int_bit) + __assign_bit(i, valid_mask, + omnia_gpio_available(mcu, gpio)); + else + __clear_bit(i, valid_mask); + } +} + +static int omnia_irq_init_hw(struct gpio_chip *gc) +{ + struct omnia_mcu *mcu = gpiochip_get_data(gc); + u8 cmd[1 + CMD_INT_ARG_LEN] = {}; + + cmd[0] = CMD_SET_INT_MASK; + + return omnia_cmd_write(mcu->client, cmd, sizeof(cmd)); +} + +/* + * Determine how many bytes we need to read from the reply to the + * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked interrupts. + */ +static size_t omnia_irq_compute_pending_length(u32 rising, u32 falling) +{ + size_t rlen = 0, flen = 0; + + if (rising) + rlen = ((__fls(rising) >> 3) << 1) + 1; + + if (falling) + flen = ((__fls(falling) >> 3) << 1) + 2; + + return max(rlen, flen); +} + +static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu, + unsigned long *pending) +{ + struct device *dev = &mcu->client->dev; + u8 reply[CMD_INT_ARG_LEN] = {}; + u32 rising, falling; + size_t len; + int err; + + len = omnia_irq_compute_pending_length(mcu->rising & mcu->mask, + mcu->falling & mcu->mask); + if (!len) + return false; + + guard(mutex)(&mcu->lock); + + err = omnia_cmd_read(mcu->client, CMD_GET_INT_AND_CLEAR, reply, + len); + if (err) { + dev_err(dev, "Cannot read pending IRQs: %d\n", err); + return false; + } + + /* deinterleave the reply bytes into rising and falling */ + omnia_mask_deinterleave(reply, &rising, &falling); + + rising &= mcu->mask; + falling &= mcu->mask; + *pending = rising | falling; + + /* cache values for GPIOs that have both edges enabled */ + mcu->is_cached &= ~(rising & falling); + mcu->is_cached |= mcu->both & (rising ^ falling); + mcu->cached = (mcu->cached | rising) & ~falling; + + return true; +} + +static int omnia_read_status_word_old_fw(struct omnia_mcu *mcu, u16 *status) +{ + int ret; + + ret = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD); + if (ret < 0) + return ret; + + /* + * Old firmware has a bug wherein it never resets the USB port + * overcurrent bits back to zero. Ignore them. + */ + *status = ret & ~(STS_USB30_OVC | STS_USB31_OVC); + + return 0; +} + +static void button_release_emul_fn(struct work_struct *work) +{ + struct omnia_mcu *mcu = container_of(to_delayed_work(work), + struct omnia_mcu, + button_release_emul_work); + + mcu->button_pressed_emul = false; + generic_handle_irq_safe(mcu->client->irq); +} + +static void fill_int_from_sts(u32 *rising, u32 *falling, u16 rising_sts, + u16 falling_sts, u16 sts_bit, u32 int_bit) +{ + if (rising_sts & sts_bit) + *rising |= int_bit; + if (falling_sts & sts_bit) + *falling |= int_bit; +} + +static bool omnia_irq_read_pending_old(struct omnia_mcu *mcu, + unsigned long *pending) +{ + struct device *dev = &mcu->client->dev; + u16 status, rising_sts, falling_sts; + u32 rising, falling; + int ret; + + guard(mutex)(&mcu->lock); + + ret = omnia_read_status_word_old_fw(mcu, &status); + if (ret < 0) { + dev_err(dev, "Cannot read pending IRQs: %d\n", ret); + return false; + } + + /* + * The old firmware triggers an interrupt whenever status word changes, + * but does not inform about which bits rose or fell. We need to compute + * this here by comparing with the last status word value. + * + * The STS_BUTTON_PRESSED bit needs special handling, because the old + * firmware clears the STS_BUTTON_PRESSED bit on successful completion + * of the CMD_GET_STATUS_WORD command, resulting in another interrupt: + * - first we get an interrupt, we read the status word where + * STS_BUTTON_PRESSED is present, + * - MCU clears the STS_BUTTON_PRESSED bit because we read the status + * word, + * - we get another interrupt because the status word changed again + * (STS_BUTTON_PRESSED was cleared). + * + * The gpiolib-cdev, gpiolib-sysfs and gpio-keys input driver all call + * the gpiochip's .get() method after an edge event on a requested GPIO + * occurs. + * + * We ensure that the .get() method reads 1 for the button GPIO for some + * time. + */ + + if (status & STS_BUTTON_PRESSED) { + mcu->button_pressed_emul = true; + mod_delayed_work(system_wq, &mcu->button_release_emul_work, + msecs_to_jiffies(FRONT_BUTTON_RELEASE_DELAY)); + } else if (mcu->button_pressed_emul) { + status |= STS_BUTTON_PRESSED; + } + + rising_sts = ~mcu->last_status & status; + falling_sts = mcu->last_status & ~status; + + mcu->last_status = status; + + /* + * Fill in the relevant interrupt bits from status bits for CARD_DET, + * MSATA_IND and BUTTON_PRESSED. + */ + rising = 0; + falling = 0; + fill_int_from_sts(&rising, &falling, rising_sts, falling_sts, + STS_CARD_DET, INT_CARD_DET); + fill_int_from_sts(&rising, &falling, rising_sts, falling_sts, + STS_MSATA_IND, INT_MSATA_IND); + fill_int_from_sts(&rising, &falling, rising_sts, falling_sts, + STS_BUTTON_PRESSED, INT_BUTTON_PRESSED); + + /* Use only bits that are enabled */ + rising &= mcu->rising & mcu->mask; + falling &= mcu->falling & mcu->mask; + *pending = rising | falling; + + return true; +} + +static bool omnia_irq_read_pending(struct omnia_mcu *mcu, + unsigned long *pending) +{ + if (mcu->features & FEAT_NEW_INT_API) + return omnia_irq_read_pending_new(mcu, pending); + else + return omnia_irq_read_pending_old(mcu, pending); +} + +static irqreturn_t omnia_irq_thread_handler(int irq, void *dev_id) +{ + struct omnia_mcu *mcu = dev_id; + struct irq_domain *domain; + unsigned long pending; + int i; + + if (!omnia_irq_read_pending(mcu, &pending)) + return IRQ_NONE; + + domain = mcu->gc.irq.domain; + + for_each_set_bit(i, &pending, 32) { + unsigned int nested_irq; + + nested_irq = irq_find_mapping(domain, omnia_int_to_gpio_idx[i]); + + handle_nested_irq(nested_irq); + } + + return IRQ_RETVAL(pending); +} + +static const char * const front_button_modes[2] = { "mcu", "cpu" }; + +static ssize_t front_button_mode_show(struct device *dev, + struct device_attribute *a, char *buf) +{ + struct omnia_mcu *mcu = dev_get_drvdata(dev); + int val; + + if (mcu->features & FEAT_NEW_INT_API) { + val = omnia_cmd_read_bit(mcu->client, CMD_GET_STATUS_WORD, + STS_BUTTON_MODE); + if (val < 0) + return val; + } else { + val = !!(mcu->last_status & STS_BUTTON_MODE); + } + + return sysfs_emit(buf, "%s\n", front_button_modes[val]); +} + +static ssize_t front_button_mode_store(struct device *dev, + struct device_attribute *a, + const char *buf, size_t count) +{ + struct omnia_mcu *mcu = dev_get_drvdata(dev); + int err, i; + + i = sysfs_match_string(front_button_modes, buf); + if (i < 0) + return i; + + err = omnia_ctl_cmd_locked(mcu, CMD_GENERAL_CONTROL, + i ? CTL_BUTTON_MODE : 0, + CTL_BUTTON_MODE); + if (err) + return err; + + return count; +} +static DEVICE_ATTR_RW(front_button_mode); + +static struct attribute *omnia_mcu_gpio_attrs[] = { + &dev_attr_front_button_mode.attr, + NULL +}; + +const struct attribute_group omnia_mcu_gpio_group = { + .attrs = omnia_mcu_gpio_attrs, +}; + +int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu) +{ + bool new_api = mcu->features & FEAT_NEW_INT_API; + struct device *dev = &mcu->client->dev; + unsigned long irqflags; + int err; + + err = devm_mutex_init(dev, &mcu->lock); + if (err) + return err; + + mcu->gc.request = omnia_gpio_request; + mcu->gc.get_direction = omnia_gpio_get_direction; + mcu->gc.direction_input = omnia_gpio_direction_input; + mcu->gc.direction_output = omnia_gpio_direction_output; + mcu->gc.get = omnia_gpio_get; + mcu->gc.get_multiple = omnia_gpio_get_multiple; + mcu->gc.set = omnia_gpio_set; + mcu->gc.set_multiple = omnia_gpio_set_multiple; + mcu->gc.init_valid_mask = omnia_gpio_init_valid_mask; + mcu->gc.can_sleep = true; + mcu->gc.names = omnia_mcu_gpio_templates; + mcu->gc.base = -1; + mcu->gc.ngpio = ARRAY_SIZE(omnia_gpios); + mcu->gc.label = "Turris Omnia MCU GPIOs"; + mcu->gc.parent = dev; + mcu->gc.owner = THIS_MODULE; + mcu->gc.of_gpio_n_cells = 3; + mcu->gc.of_xlate = omnia_gpio_of_xlate; + + gpio_irq_chip_set_chip(&mcu->gc.irq, &omnia_mcu_irq_chip); + /* This will let us handle the parent IRQ in the driver */ + mcu->gc.irq.parent_handler = NULL; + mcu->gc.irq.num_parents = 0; + mcu->gc.irq.parents = NULL; + mcu->gc.irq.default_type = IRQ_TYPE_NONE; + mcu->gc.irq.handler = handle_bad_irq; + mcu->gc.irq.threaded = true; + if (new_api) + mcu->gc.irq.init_hw = omnia_irq_init_hw; + mcu->gc.irq.init_valid_mask = omnia_irq_init_valid_mask; + + err = devm_gpiochip_add_data(dev, &mcu->gc, mcu); + if (err) + return dev_err_probe(dev, err, "Cannot add GPIO chip\n"); + + /* + * Before requesting the interrupt, if firmware does not support the new + * interrupt API, we need to cache the value of the status word, so that + * when it changes, we may compare the new value with the cached one in + * the interrupt handler. + */ + if (!new_api) { + err = omnia_read_status_word_old_fw(mcu, &mcu->last_status); + if (err < 0) + return dev_err_probe(dev, err, + "Cannot read status word\n"); + + INIT_DELAYED_WORK(&mcu->button_release_emul_work, + button_release_emul_fn); + } + + irqflags = IRQF_ONESHOT; + if (new_api) + irqflags |= IRQF_TRIGGER_LOW; + else + irqflags |= IRQF_TRIGGER_FALLING; + + err = devm_request_threaded_irq(dev, mcu->client->irq, NULL, + omnia_irq_thread_handler, irqflags, + "turris-omnia-mcu", mcu); + if (err) + return dev_err_probe(dev, err, "Cannot request IRQ\n"); + + if (!new_api) { + /* + * The button_release_emul_work has to be initialized before the + * thread is requested, and on driver remove it needs to be + * canceled before the thread is freed. Therefore we can't use + * devm_delayed_work_autocancel() directly, because the order + * devm_delayed_work_autocancel(); + * devm_request_threaded_irq(); + * would cause improper release order: + * free_irq(); + * cancel_delayed_work_sync(); + * Instead we first initialize the work above, and only now + * after IRQ is requested we add the work devm action. + */ + err = devm_add_action(dev, devm_delayed_work_drop, + &mcu->button_release_emul_work); + if (err) + return err; + } + + return 0; +} diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h index 5b21fbca8fe1..e8d9bf1c8fdc 100644 --- a/drivers/platform/cznic/turris-omnia-mcu.h +++ b/drivers/platform/cznic/turris-omnia-mcu.h @@ -8,9 +8,12 @@ #ifndef __TURRIS_OMNIA_MCU_H #define __TURRIS_OMNIA_MCU_H -#include <linux/i2c.h> +#include <linux/bitops.h> +#include <linux/gpio/driver.h> #include <linux/if_ether.h> +#include <linux/mutex.h> #include <linux/types.h> +#include <linux/workqueue.h> #include <asm/byteorder.h> struct omnia_mcu { @@ -22,10 +25,66 @@ struct omnia_mcu { u64 board_serial_number; u8 board_first_mac[ETH_ALEN]; u8 board_revision; + + /* GPIO chip */ + struct gpio_chip gc; + struct mutex lock; + u32 mask, rising, falling, both, cached, is_cached; + /* Old MCU firmware handling needs the following */ + struct delayed_work button_release_emul_work; + u16 last_status; + bool button_pressed_emul; }; -int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply, - unsigned int len); +int omnia_cmd_write_read(const struct i2c_client *client, + void *cmd, unsigned int cmd_len, + void *reply, unsigned int reply_len); + +static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd, + size_t len) +{ + return omnia_cmd_write_read(client, cmd, len, NULL, 0); +} + +static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, + void *reply, unsigned int len) +{ + return omnia_cmd_write_read(client, &cmd, 1, reply, len); +} + +/* Returns 0 on success */ +static inline int omnia_cmd_read_bits(const struct i2c_client *client, u8 cmd, + u32 bits, u32 *dst) +{ + __le32 reply; + int err; + + if (!bits) { + *dst = 0; + return 0; + } + + err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1); + if (err) + return err; + + *dst = le32_to_cpu(reply) & bits; + + return 0; +} + +static inline int omnia_cmd_read_bit(const struct i2c_client *client, u8 cmd, + u32 bit) +{ + u32 reply; + int err; + + err = omnia_cmd_read_bits(client, cmd, bit, &reply); + if (err) + return err; + + return !!reply; +} static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd) { @@ -47,4 +106,8 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd) return err ?: reply; } +extern const struct attribute_group omnia_mcu_gpio_group; + +int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu); + #endif /* __TURRIS_OMNIA_MCU_H */
Add support for GPIOs connected to the MCU on the Turris Omnia board. This includes: - front button pin - enable pins for USB regulators - MiniPCIe / mSATA card presence pins in MiniPCIe port 0 - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports - on board revisions 32+ also various peripheral resets and another voltage regulator enable pin Signed-off-by: Marek Behún <kabel@kernel.org> --- .../sysfs-bus-i2c-devices-turris-omnia-mcu | 16 + drivers/platform/cznic/Kconfig | 15 + drivers/platform/cznic/Makefile | 1 + .../platform/cznic/turris-omnia-mcu-base.c | 33 +- .../platform/cznic/turris-omnia-mcu-gpio.c | 1048 +++++++++++++++++ drivers/platform/cznic/turris-omnia-mcu.h | 69 +- 6 files changed, 1166 insertions(+), 16 deletions(-) create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c