| Message ID | 20240913121110.1611340-7-Shyam-sundar.S-k@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce initial AMD ASF Controller driver support | expand |
On Fri, Sep 13, 2024 at 05:41:08PM +0530, Shyam Sundar S K wrote: > Add support for handling ASF slave process events as described in the AMD > ASF databook. This involves implementing the correct programming sequence > to manage each ASF packet appropriately. ... > /* ASF address offsets */ > +#define ASFINDEX (7 + piix4_smba) 0x07 ... > +#define ASF_ERROR_STATUS 0xE So, according to the usage this seems to be a mask, then perhaps GENMASK(3, 1) ? ... > +static void amd_asf_process_target(struct work_struct *work) > +{ > + struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work); > + unsigned short piix4_smba = dev->port_addr->start; > + u8 data[ASF_BLOCK_MAX_BYTES]; > + u8 len, idx, val = 0; Hmm... Does val = 0 assignment is due to false positive (or missing error check)? > + u8 bank, reg, cmd; > + > + /* Read target status register */ > + reg = inb_p(ASFSLVSTA); > + > + /* Check if no error bits are set in target status register */ > + if (reg & ASF_ERROR_STATUS) { > + /* Set bank as full */ > + cmd = 0; > + reg = reg | GENMASK(3, 2); > + outb_p(reg, ASFDATABNKSEL); > + } else { > + /* Read data bank */ > + reg = inb_p(ASFDATABNKSEL); > + bank = (reg & BIT(3)) ? 1 : 0; > + > + /* Set read data bank */ > + if (bank) { > + reg = reg | BIT(4); > + reg = reg & ~BIT(3); > + } else { > + reg = reg & ~BIT(4); > + reg = reg & ~BIT(2); > + } > + > + /* Read command register */ > + outb_p(reg, ASFDATABNKSEL); > + cmd = inb_p(ASFINDEX); > + len = inb_p(ASFDATARWPTR); > + for (idx = 0; idx < len; idx++) > + data[idx] = inb_p(ASFINDEX); > + > + /* Clear data bank status */ > + if (bank) { > + reg = reg | BIT(3); > + outb_p(reg, ASFDATABNKSEL); > + } else { > + reg = reg | BIT(2); > + outb_p(reg, ASFDATABNKSEL); > + } > + } > + > + outb_p(0, ASFSETDATARDPTR); > + if (cmd & BIT(0)) > + return; > + > + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val); Can this fail / return an error code? (I haven't checked) > + for (idx = 0; idx < len; idx++) { > + val = data[idx]; > + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val); > + } > + i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val); > +} ... > + irq = platform_get_irq(pdev, 0); > + if (!irq) Incorrect check. > + return dev_err_probe(&pdev->dev, -EINVAL, "missing IRQ resources\n"); Shadower real error code. ... > +static void amd_asf_remove(struct platform_device *pdev) > +{ > + struct amd_asf_dev *dev = platform_get_drvdata(pdev); > + > + cancel_delayed_work_sync(&dev->work_buf); > +} Wouldn't devm-helpers.h APIs help with avoiding ->remove() creation?
On 9/14/2024 00:47, Andy Shevchenko wrote: > On Fri, Sep 13, 2024 at 05:41:08PM +0530, Shyam Sundar S K wrote: >> Add support for handling ASF slave process events as described in the AMD >> ASF databook. This involves implementing the correct programming sequence >> to manage each ASF packet appropriately. > > ... > >> /* ASF address offsets */ >> +#define ASFINDEX (7 + piix4_smba) > > 0x07 > > ... > >> +#define ASF_ERROR_STATUS 0xE > > So, according to the usage this seems to be a mask, then perhaps GENMASK(3, 1) ? > GENMASK() works here. > ... > >> +static void amd_asf_process_target(struct work_struct *work) >> +{ >> + struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work); >> + unsigned short piix4_smba = dev->port_addr->start; >> + u8 data[ASF_BLOCK_MAX_BYTES]; > >> + u8 len, idx, val = 0; > > Hmm... Does val = 0 assignment is due to false positive (or missing error check)? > I can remove the explicit assignment to zero. >> + u8 bank, reg, cmd; >> + >> + /* Read target status register */ >> + reg = inb_p(ASFSLVSTA); >> + >> + /* Check if no error bits are set in target status register */ >> + if (reg & ASF_ERROR_STATUS) { >> + /* Set bank as full */ >> + cmd = 0; >> + reg = reg | GENMASK(3, 2); >> + outb_p(reg, ASFDATABNKSEL); >> + } else { >> + /* Read data bank */ >> + reg = inb_p(ASFDATABNKSEL); >> + bank = (reg & BIT(3)) ? 1 : 0; >> + >> + /* Set read data bank */ >> + if (bank) { >> + reg = reg | BIT(4); >> + reg = reg & ~BIT(3); >> + } else { >> + reg = reg & ~BIT(4); >> + reg = reg & ~BIT(2); >> + } >> + >> + /* Read command register */ >> + outb_p(reg, ASFDATABNKSEL); >> + cmd = inb_p(ASFINDEX); >> + len = inb_p(ASFDATARWPTR); >> + for (idx = 0; idx < len; idx++) >> + data[idx] = inb_p(ASFINDEX); >> + >> + /* Clear data bank status */ >> + if (bank) { >> + reg = reg | BIT(3); >> + outb_p(reg, ASFDATABNKSEL); >> + } else { >> + reg = reg | BIT(2); >> + outb_p(reg, ASFDATABNKSEL); >> + } >> + } >> + >> + outb_p(0, ASFSETDATARDPTR); >> + if (cmd & BIT(0)) >> + return; >> + >> + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val); > > Can this fail / return an error code? (I haven't checked) i2c_slave_event() returns an error code, but here it is done with the workqueue callback context. Hence I skipped the error checking part. > >> + for (idx = 0; idx < len; idx++) { >> + val = data[idx]; >> + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val); >> + } >> + i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val); >> +} > > ... > >> + irq = platform_get_irq(pdev, 0); >> + if (!irq) > > Incorrect check. > >> + return dev_err_probe(&pdev->dev, -EINVAL, "missing IRQ resources\n"); > > Shadower real error code. > > ... > >> +static void amd_asf_remove(struct platform_device *pdev) >> +{ >> + struct amd_asf_dev *dev = platform_get_drvdata(pdev); >> + >> + cancel_delayed_work_sync(&dev->work_buf); >> +} > > Wouldn't devm-helpers.h APIs help with avoiding ->remove() creation? >
On Tue, Sep 17, 2024 at 11:51:27PM +0530, Shyam Sundar S K wrote: > On 9/14/2024 00:47, Andy Shevchenko wrote: > > On Fri, Sep 13, 2024 at 05:41:08PM +0530, Shyam Sundar S K wrote: ... > >> + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val); > > > > Can this fail / return an error code? (I haven't checked) > > i2c_slave_event() returns an error code, but here it is done with the > workqueue callback context. Hence I skipped the error checking part. This requires a comment why it's okay. ... > >> + for (idx = 0; idx < len; idx++) { > >> + val = data[idx]; > >> + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val); > >> + } > >> + i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val); Same here. > >> +}
diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c index aa85e10a3927..1beef717ef40 100644 --- a/drivers/i2c/busses/i2c-amd-asf-plat.c +++ b/drivers/i2c/busses/i2c-amd-asf-plat.c @@ -29,13 +29,17 @@ #define ASF_CLK_EN 17 /* ASF address offsets */ +#define ASFINDEX (7 + piix4_smba) #define ASFLISADDR (9 + piix4_smba) #define ASFSTA (0xA + piix4_smba) #define ASFSLVSTA (0xD + piix4_smba) +#define ASFDATARWPTR (0x11 + piix4_smba) +#define ASFSETDATARDPTR (0x12 + piix4_smba) #define ASFDATABNKSEL (0x13 + piix4_smba) #define ASFSLVEN (0x15 + piix4_smba) #define ASF_BLOCK_MAX_BYTES 72 +#define ASF_ERROR_STATUS 0xE static const char *amd_asf_port_name = " port 1"; @@ -43,10 +47,71 @@ struct amd_asf_dev { struct i2c_adapter adap; struct device *dev; struct i2c_client *target; + struct delayed_work work_buf; struct sb800_mmio_cfg mmio_cfg; struct resource *port_addr; }; +static void amd_asf_process_target(struct work_struct *work) +{ + struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work); + unsigned short piix4_smba = dev->port_addr->start; + u8 data[ASF_BLOCK_MAX_BYTES]; + u8 len, idx, val = 0; + u8 bank, reg, cmd; + + /* Read target status register */ + reg = inb_p(ASFSLVSTA); + + /* Check if no error bits are set in target status register */ + if (reg & ASF_ERROR_STATUS) { + /* Set bank as full */ + cmd = 0; + reg = reg | GENMASK(3, 2); + outb_p(reg, ASFDATABNKSEL); + } else { + /* Read data bank */ + reg = inb_p(ASFDATABNKSEL); + bank = (reg & BIT(3)) ? 1 : 0; + + /* Set read data bank */ + if (bank) { + reg = reg | BIT(4); + reg = reg & ~BIT(3); + } else { + reg = reg & ~BIT(4); + reg = reg & ~BIT(2); + } + + /* Read command register */ + outb_p(reg, ASFDATABNKSEL); + cmd = inb_p(ASFINDEX); + len = inb_p(ASFDATARWPTR); + for (idx = 0; idx < len; idx++) + data[idx] = inb_p(ASFINDEX); + + /* Clear data bank status */ + if (bank) { + reg = reg | BIT(3); + outb_p(reg, ASFDATABNKSEL); + } else { + reg = reg | BIT(2); + outb_p(reg, ASFDATABNKSEL); + } + } + + outb_p(0, ASFSETDATARDPTR); + if (cmd & BIT(0)) + return; + + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val); + for (idx = 0; idx < len; idx++) { + val = data[idx]; + i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val); + } + i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val); +} + static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit, unsigned long offset, bool set) { @@ -212,9 +277,28 @@ static const struct i2c_algorithm amd_asf_smbus_algorithm = { .functionality = amd_asf_func, }; +static irqreturn_t amd_asf_irq_handler(int irq, void *ptr) +{ + struct amd_asf_dev *dev = ptr; + unsigned short piix4_smba = dev->port_addr->start; + u8 target_int = inb_p(ASFSTA); + + if (target_int & BIT(6)) { + /* Target Interrupt */ + outb_p(target_int | BIT(6), ASFSTA); + schedule_delayed_work(&dev->work_buf, HZ); + } else { + /* Controller Interrupt */ + amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, SMBHSTSTS, true); + } + + return IRQ_HANDLED; +} + static int amd_asf_probe(struct platform_device *pdev) { struct amd_asf_dev *asf_dev; + int ret, irq; asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL); if (!asf_dev) @@ -228,6 +312,17 @@ static int amd_asf_probe(struct platform_device *pdev) if (!asf_dev->port_addr) return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n"); + INIT_DELAYED_WORK(&asf_dev->work_buf, amd_asf_process_target); + + irq = platform_get_irq(pdev, 0); + if (!irq) + return dev_err_probe(&pdev->dev, -EINVAL, "missing IRQ resources\n"); + + ret = devm_request_irq(&pdev->dev, irq, amd_asf_irq_handler, IRQF_SHARED, + "amd_smbus_asf", asf_dev); + if (ret) + return dev_err_probe(&pdev->dev, ret, "Unable to request irq: %d for use\n", irq); + asf_dev->adap.owner = THIS_MODULE; asf_dev->adap.algo = &amd_asf_smbus_algorithm; asf_dev->adap.dev.parent = &pdev->dev; @@ -239,6 +334,13 @@ static int amd_asf_probe(struct platform_device *pdev) return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap); } +static void amd_asf_remove(struct platform_device *pdev) +{ + struct amd_asf_dev *dev = platform_get_drvdata(pdev); + + cancel_delayed_work_sync(&dev->work_buf); +} + static const struct acpi_device_id amd_asf_acpi_ids[] = { { "AMDI001A" }, { } @@ -251,6 +353,7 @@ static struct platform_driver amd_asf_driver = { .acpi_match_table = amd_asf_acpi_ids, }, .probe = amd_asf_probe, + .remove_new = amd_asf_remove, }; module_platform_driver(amd_asf_driver);