| Message ID | 1412712189-1234-8-git-send-email-christophe-h.ricard@st.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Oct 07, 2014 at 10:03:00PM +0200, Christophe Ricard wrote: > +_irq_probe: > + /* IRQ */ > + r = irq_of_parse_and_map(pp, 0); > + if (r < 0) { > + pr_err("Unable to get irq, error: %d\n", r); > + interrupts = 0; > + goto _end; > + } > + interrupts = 1; > + client->irq = r; client->irq is already set correctly by of_i2c_register_devices - so I don't think this sequence is necessary? > + if (interrupts) { > + r = devm_gpio_request_one(&client->dev, pdata->io_serirq, > + GPIOF_IN, "TPM IO_SERIRQ"); Similarly, I wonder if pdata->io_serirq is just duplication of client->irq and that should be set by the creator instead? > +#ifdef CONFIG_OF > +static const struct of_device_id of_st33zp24_i2c_match[] = { > + { .compatible = "st,st33zp24_i2c", }, > + {} > +}; missing: MODULE_DEVICE_TABLE(of, of_st33zp24_i2c_match); > + #ifdef CONFIG_OF > + .of_match_table = of_match_ptr(of_st33zp24_i2c_match), > + #endif The #ifdef is unnecessary, the of_match_ptr macro takes care of that already. Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Hi Jason, On 08/10/2014 00:30, Jason Gunthorpe wrote: > On Tue, Oct 07, 2014 at 10:03:00PM +0200, Christophe Ricard wrote: > >> +_irq_probe: >> + /* IRQ */ >> + r = irq_of_parse_and_map(pp, 0); >> + if (r < 0) { >> + pr_err("Unable to get irq, error: %d\n", r); >> + interrupts = 0; >> + goto _end; >> + } >> + interrupts = 1; >> + client->irq = r; > client->irq is already set correctly by of_i2c_register_devices - so I > don't think this sequence is necessary? May be you are right. I will crosscheck. >> + if (interrupts) { >> + r = devm_gpio_request_one(&client->dev, pdata->io_serirq, >> + GPIOF_IN, "TPM IO_SERIRQ"); > Similarly, I wonder if pdata->io_serirq is just duplication of > client->irq and that should be set by the creator instead? pdata->io_serirq stores the gpio number which will be converted into irq number. pdata->io_serirq is only use by static platform configuration not devicetree configuration >> +#ifdef CONFIG_OF >> +static const struct of_device_id of_st33zp24_i2c_match[] = { >> + { .compatible = "st,st33zp24_i2c", }, >> + {} >> +}; > missing: > > MODULE_DEVICE_TABLE(of, of_st33zp24_i2c_match); Oops. Good catch :). >> + #ifdef CONFIG_OF >> + .of_match_table = of_match_ptr(of_st33zp24_i2c_match), >> + #endif > The #ifdef is unnecessary, the of_match_ptr macro takes care of that > already. Ok will clean that. > > Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
On Wed, Oct 08, 2014 at 07:47:58AM +0200, Christophe RICARD wrote: > >>+ if (interrupts) { > >>+ r = devm_gpio_request_one(&client->dev, pdata->io_serirq, > >>+ GPIOF_IN, "TPM IO_SERIRQ"); > >Similarly, I wonder if pdata->io_serirq is just duplication of > >client->irq and that should be set by the creator instead? > pdata->io_serirq stores the gpio number which will be converted into > irq number. pdata->io_serirq is only use by static platform > configuration not devicetree configuration Right, but the driver never uses it as a GPIO, so accepting a GPIO is actually less flexible - a platform may connect the TPM to a dedicated IRQ pin, for instance. The creator should just specify the irq in client->irq, however that is typically done.. Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Hi Jason, I have currently seen both. I agree on the principles as it is simplifying the code a little bit. I including this clean up in this patch Add devicetree structure for a future v2 submission Best Regards Christophe -----Original Message----- From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] Sent: mercredi 8 octobre 2014 18:29 To: Christophe RICARD Cc: peterhuewe@gmx.de; ashley@ashleylai.com; tpmdd@selhorst.net; devicetree@vger.kernel.org; tpmdd-devel@lists.sourceforge.net; Christophe Henri RICARD; Jean-Luc BLANC Subject: Re: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure On Wed, Oct 08, 2014 at 07:47:58AM +0200, Christophe RICARD wrote: > >>+ if (interrupts) { > >>+ r = devm_gpio_request_one(&client->dev, pdata->io_serirq, > >>+ GPIOF_IN, "TPM IO_SERIRQ"); > >Similarly, I wonder if pdata->io_serirq is just duplication of > >client->irq and that should be set by the creator instead? > pdata->io_serirq stores the gpio number which will be converted into > irq number. pdata->io_serirq is only use by static platform > configuration not devicetree configuration Right, but the driver never uses it as a GPIO, so accepting a GPIO is actually less flexible - a platform may connect the TPM to a dedicated IRQ pin, for instance. The creator should just specify the irq in client->irq, however that is typically done.. Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
On Wed, Oct 08, 2014 at 10:41:36PM +0200, Christophe Henri RICARD wrote: > Hi Jason, > > I have currently seen both. I agree on the principles as it is > simplifying the code a little bit. I including this clean up in > this patch Add devicetree structure for a future v2 submission I was just looking at what the remaining lpcpd GPIO does, and.. 1) I think you can probably just make this optional, if it is not specified in the DT or through platform data, just don't fiddle with it. This makes the driver much friendlier - ie it can be manually attached via sysfs for instance. 2) This looks fishy: static int tpm_st33_i2c_pm_suspend(struct device *dev) if (power_mgt) { gpio_set_value(pin_infos->io_lpcpd, 0); } else { ret = tpm_pm_suspend(dev); } I can't think of a reason why the driver wouldn't need to call tpm_pm_suspend to save the internal state? Ditto for restore? I'm also suspicious of the power_mgt module option, that should probably go away entirely? Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jason, For your information, the lpcpd pin is named like this as per PCClientTPMSpecification is also known as TPMSTB on this product. The TPMSTB pin can be used in order to support D1 and D2 power management state. I am actually sorry to refer to Microsoft website here ;) : http://msdn.microsoft.com/en-us/library/windows/hardware/ff543162%28v=vs.85%29.aspx. As mention by our product datasheet: "In order to reach D1/D2 state, the TPMSTB pin must be connected to a GPIO signal. In state D1/D2, the contents of the volatile memory is *preserved*." Also "... the implementation of the LPCPD# pin on a TPM is platform and chipset implementation specific." Cf PCClientTPMSpecification. I am currently giving the opportunity to an integrator to use this feature. I don't mind removing the power_mgt module option but don't you think there might be interest for an integrator to update their TPM power management policy on their platform ? Best Regards Christophe -----Original Message----- From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] Sent: mercredi 8 octobre 2014 22:51 To: Christophe Henri RICARD Cc: Christophe RICARD; peterhuewe@gmx.de; ashley@ashleylai.com; tpmdd@selhorst.net; devicetree@vger.kernel.org; tpmdd-devel@lists.sourceforge.net; Jean-Luc BLANC Subject: Re: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure On Wed, Oct 08, 2014 at 10:41:36PM +0200, Christophe Henri RICARD wrote: > Hi Jason, > > I have currently seen both. I agree on the principles as it is > simplifying the code a little bit. I including this clean up in this > patch Add devicetree structure for a future v2 submission I was just looking at what the remaining lpcpd GPIO does, and.. 1) I think you can probably just make this optional, if it is not specified in the DT or through platform data, just don't fiddle with it. This makes the driver much friendlier - ie it can be manually attached via sysfs for instance. 2) This looks fishy: static int tpm_st33_i2c_pm_suspend(struct device *dev) if (power_mgt) { gpio_set_value(pin_infos->io_lpcpd, 0); } else { ret = tpm_pm_suspend(dev); } I can't think of a reason why the driver wouldn't need to call tpm_pm_suspend to save the internal state? Ditto for restore? I'm also suspicious of the power_mgt module option, that should probably go away entirely? Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
On Wed, Oct 08, 2014 at 11:14:40PM +0200, Christophe Henri RICARD wrote: > For your information, the lpcpd pin is named like this as per > PCClientTPMSpecification is also known as TPMSTB on this product. > The TPMSTB pin can be used in order to support D1 and D2 power > management state. I am actually sorry to refer to Microsoft website > here ;) : So, having the option to enter D1/D2 through that pin in the driver seems fine, and it should just be enabled if the driver is told what that pin is. No need for a module parameter, and no need for the driver to fail probe if it isn't told how to access that GPIO. However.. suspend/resume are called from the broader PM framework and a platform might arrange things so the TPM is put into a deeper sleep than D1/D2 (ie entirely powered off because the CPU was put into suspend-to-ram and the peripheral power converter was switched off) Thus the driver really must save the TPM state at suspend, and if the volatile state was wiped after resume then it must be restored from the saved state. Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c index 9466422..ac23f0f 100644 --- a/drivers/char/tpm/tpm_i2c_stm_st33.c +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c @@ -48,6 +48,10 @@ #include <linux/uaccess.h> #include <linux/io.h> #include <linux/slab.h> +#ifdef CONFIG_OF +#include <linux/of_irq.h> +#include <linux/of_gpio.h> +#endif #include <linux/platform_data/tpm_i2c_stm_st33.h> #include "tpm.h" @@ -618,6 +622,108 @@ static int power_mgt = 1; module_param(power_mgt, int, 0444); MODULE_PARM_DESC(power_mgt, "Power Management"); +#ifdef CONFIG_OF +static int tpm_stm_i2c_of_request_resources(struct tpm_chip *chip) +{ + struct device_node *pp; + struct tpm_stm_dev *tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip); + struct i2c_client *client = tpm_dev->client; + + int gpio; + int r; + + pp = client->dev.of_node; + if (!pp) + return -ENODEV; + + /* Get GPIO from device tree */ + gpio = of_get_named_gpio(pp, "lpcpd-gpios", 0); + if (gpio < 0) { + pr_err("Failed to retrieve lpcpd-gpios from dts.\n"); + power_mgt = 0; + goto _irq_probe; + } + power_mgt = 1; + /* GPIO request and configuration */ + r = devm_gpio_request_one(&client->dev, gpio, + GPIOF_OUT_INIT_HIGH, "TPM IO LPCPD"); + if (r) { + pr_err("Failed to request serirq pin\n"); + return -ENODEV; + } + tpm_dev->io_lpcpd = gpio; + +_irq_probe: + /* IRQ */ + r = irq_of_parse_and_map(pp, 0); + if (r < 0) { + pr_err("Unable to get irq, error: %d\n", r); + interrupts = 0; + goto _end; + } + interrupts = 1; + client->irq = r; + + return 0; +_end: + return r; +} +#else +static int tpm_stm_i2c_of_request_resources(struct i2c_client *client) +{ + return -ENODEV; +} +#endif + +static int tpm_stm_i2c_request_resources(struct i2c_client *client, + struct tpm_chip *chip) +{ + struct st33zp24_platform_data *pdata; + struct tpm_stm_dev *tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip); + int r; + int irq; + + pdata = client->dev.platform_data; + if (pdata == NULL) { + pr_err("No platform data\n"); + return -EINVAL; + } + + /* store for late use */ + tpm_dev->io_serirq = pdata->io_serirq; + tpm_dev->io_lpcpd = pdata->io_lpcpd; + + if (interrupts) { + r = devm_gpio_request_one(&client->dev, pdata->io_serirq, + GPIOF_IN, "TPM IO_SERIRQ"); + if (r) { + pr_err("%s : gpio_request failed\n", __FILE__); + return -ENODEV; + } + + /* IRQ */ + irq = gpio_to_irq(pdata->io_serirq); + if (irq < 0) { + pr_err("Unable to get irq number for GPIO %d %d\n", + pdata->io_serirq, r); + return -ENODEV; + } + client->irq = irq; + } + + if (power_mgt) { + r = devm_gpio_request_one(&client->dev, + pdata->io_lpcpd, GPIOF_OUT_INIT_HIGH, + "TPM IO_LPCPD"); + if (r) { + pr_err("%s : reset gpio_request failed\n", __FILE__); + return -ENODEV; + } + } + + return 0; +} + /* * tpm_stm_i2c_probe initialize the TPM device * @param: client, the i2c_client drescription (TPM I2C description). @@ -637,30 +743,19 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) if (client == NULL) { pr_info("%s: i2c client is NULL. Device not accessible.\n", __func__); - r = -ENODEV; - goto end; + return -ENODEV; } if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { dev_info(&client->dev, "client not i2c capable\n"); - r = -ENODEV; - goto end; + return -ENODEV; } tpm_dev = devm_kzalloc(&client->dev, sizeof(struct tpm_stm_dev), GFP_KERNEL); if (!tpm_dev) { dev_info(&client->dev, "cannot allocate memory for tpm data\n"); - r = -ENOMEM; - goto _tpm_clean_answer; - } - - platform_data = client->dev.platform_data; - - if (!platform_data) { - dev_info(&client->dev, "chip not available\n"); - r = -ENODEV; - goto _tpm_clean_answer; + return -ENOMEM; } chip = tpm_register_hardware(&client->dev, &st_i2c_tpm); @@ -672,6 +767,25 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) TPM_VPRIV(chip) = tpm_dev; tpm_dev->client = client; + platform_data = client->dev.platform_data; + if (!platform_data && client->dev.of_node) { + r = tpm_stm_i2c_of_request_resources(chip); + if (r) { + pr_err("No platform data\n"); + goto _tpm_clean_answer; + } + } else if (platform_data) { + r = tpm_stm_i2c_request_resources(client, chip); + if (r) { + pr_err("Cannot get platform resources\n"); + goto _tpm_clean_answer; + } + } else { + pr_err("tpm_stm_st33 platform resources not available\n"); + goto _tpm_clean_answer; + } + + chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT); chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT); chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT); @@ -679,37 +793,27 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) chip->vendor.locality = LOCALITY0; - if (power_mgt) { - r = gpio_request(platform_data->io_lpcpd, "TPM IO_LPCPD"); - if (r) - goto _gpio_init1; - gpio_set_value(platform_data->io_lpcpd, 1); - } - if (interrupts) { init_completion(&tpm_dev->irq_detection); if (request_locality(chip) != LOCALITY0) { r = -ENODEV; goto _tpm_clean_answer; } - r = gpio_request(platform_data->io_serirq, "TPM IO_SERIRQ"); - if (r) - goto _gpio_init2; clear_interruption(tpm_dev); - r = request_irq(gpio_to_irq(platform_data->io_serirq), - &tpm_ioserirq_handler, - IRQF_TRIGGER_HIGH, + r = devm_request_threaded_irq(&client->dev, client->irq, NULL, + tpm_ioserirq_handler, + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "TPM SERIRQ management", chip); if (r < 0) { dev_err(chip->dev , "TPM SERIRQ signals %d not available\n", - gpio_to_irq(platform_data->io_serirq)); - goto _irq_set; + client->irq); + goto _tpm_clean_answer; } r = I2C_READ_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1); if (r < 0) - goto _irq_set; + goto _tpm_clean_answer; intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_FIFO_AVALAIBLE_INT @@ -720,18 +824,18 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) r = I2C_WRITE_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1); if (r < 0) - goto _irq_set; + goto _tpm_clean_answer; intmask = TPM_GLOBAL_INT_ENABLE; r = I2C_WRITE_DATA(tpm_dev, (TPM_INT_ENABLE + 3), &intmask, 1); if (r < 0) - goto _irq_set; + goto _tpm_clean_answer; r = I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &intmask, 1); if (r < 0) - goto _irq_set; + goto _tpm_clean_answer; - chip->vendor.irq = interrupts; + chip->vendor.irq = client->irq; tpm_gen_interrupt(chip); } @@ -741,17 +845,8 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) dev_info(chip->dev, "TPM I2C Initialized\n"); return 0; -_irq_set: - free_irq(gpio_to_irq(platform_data->io_serirq), (void *)chip); -_gpio_init2: - if (interrupts) - gpio_free(platform_data->io_serirq); -_gpio_init1: - if (power_mgt) - gpio_free(platform_data->io_lpcpd); _tpm_clean_answer: tpm_remove_hardware(chip->dev); -end: pr_info("TPM I2C initialisation fail\n"); return r; } @@ -823,14 +918,25 @@ static const struct i2c_device_id tpm_stm_i2c_id[] = { {TPM_ST33_I2C, 0}, {} }; + +#ifdef CONFIG_OF +static const struct of_device_id of_st33zp24_i2c_match[] = { + { .compatible = "st,st33zp24_i2c", }, + {} +}; +#endif + MODULE_DEVICE_TABLE(i2c, tpm_stm_i2c_id); static SIMPLE_DEV_PM_OPS(tpm_stm_i2c_ops, tpm_stm_i2c_pm_suspend, tpm_stm_i2c_pm_resume); static struct i2c_driver tpm_stm_i2c_driver = { .driver = { - .owner = THIS_MODULE, - .name = TPM_ST33_I2C, - .pm = &tpm_stm_i2c_ops, + .owner = THIS_MODULE, + .name = TPM_ST33_I2C, + .pm = &tpm_stm_i2c_ops, + #ifdef CONFIG_OF + .of_match_table = of_match_ptr(of_st33zp24_i2c_match), + #endif }, .probe = tpm_stm_i2c_probe, .remove = tpm_stm_i2c_remove,
Add tpm_stm_st33_i2c dts structure keeping backward compatibility with static platform_data support as well. In the mean time the code is made much simpler by: - Moving all gpio_request to devm_gpio_request_one primitive - Moving request_irq to devm_request_threaded_irq Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> --- drivers/char/tpm/tpm_i2c_stm_st33.c | 198 +++++++++++++++++++++++++++--------- 1 file changed, 152 insertions(+), 46 deletions(-)