| Message ID | 1475927979-23484-3-git-send-email-tomas.winkler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote: > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT device, > where most of the registers in the control area except START, REQUEST, > CANCEL, and LOC_CTRL lost retention when the device is in the idle state. > Hence we need to bring the device to ready state before accessing the other > registers. The fix brings device to ready state before trying to read > command and response buffer addresses in order to remap the for access. > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > --- > V2: cmd read need to be called also before crb_init as this will run > self test. > V3: resend. > V4: add Kabylake to the list of effected platforms > > drivers/char/tpm/tpm_crb.c | 47 ++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 39 insertions(+), 8 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 0f3b3f3d12d3..4eb96b85c653 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > struct list_head resources; > struct resource io_res; > struct device *dev = &device->dev; > + u32 pa_high, pa_low; > u64 cmd_pa; > u32 cmd_size; > u64 rsp_pa; > @@ -346,12 +347,27 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > if (IS_ERR(priv->cca)) > return PTR_ERR(priv->cca); > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > - (u64) ioread32(&priv->cca->cmd_pa_low); > + /* > + * PTT HW bug w/a: wake up the device to access > + * possibly not retained registers. > + */ > + ret = crb_cmd_ready(dev, priv); > + if (ret) > + return ret; > + > + pa_high = ioread32(&priv->cca->cmd_pa_high); > + pa_low = ioread32(&priv->cca->cmd_pa_low); > + cmd_pa = ((u64)pa_high << 32) | pa_low; > cmd_size = ioread32(&priv->cca->cmd_size); > + > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > + pa_high, pa_low, cmd_size); > + > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > - if (IS_ERR(priv->cmd)) > - return PTR_ERR(priv->cmd); > + if (IS_ERR(priv->cmd)) { > + ret = PTR_ERR(priv->cmd); > + goto out; > + } > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > rsp_pa = le64_to_cpu(rsp_pa); > @@ -359,7 +375,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > if (cmd_pa != rsp_pa) { > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > - return PTR_ERR_OR_ZERO(priv->rsp); > + ret = PTR_ERR_OR_ZERO(priv->rsp); > + goto out; > } > > /* According to the PTP specification, overlapping command and response > @@ -367,12 +384,18 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, > */ > if (cmd_size != rsp_size) { > dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical"); > - return -EINVAL; > + ret = -EINVAL; > + goto out; > } > + > priv->cmd_size = cmd_size; > > priv->rsp = priv->cmd; > - return 0; > + > +out: > + crb_go_idle(dev, priv); > + > + return ret; > } > > static int crb_acpi_add(struct acpi_device *device) > @@ -416,7 +439,15 @@ static int crb_acpi_add(struct acpi_device *device) > if (rc) > return rc; > > - return crb_init(device, priv); > + rc = crb_cmd_ready(dev, priv); > + if (rc) > + return rc; I cannot find any valid reason why crb_map_io calls crb_go_idle in the except in the case of a failure. This is something I complained earlier. A minor thing but the extra crb_cmd_ready is basically clutter to the initialization. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote: > > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > > > There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT > > device, where most of the registers in the control area except START, > > REQUEST, CANCEL, and LOC_CTRL lost retention when the device is in the > idle state. > > Hence we need to bring the device to ready state before accessing the > > other registers. The fix brings device to ready state before trying to > > read command and response buffer addresses in order to remap the for > access. > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > --- > > V2: cmd read need to be called also before crb_init as this will run > > self test. > > V3: resend. > > V4: add Kabylake to the list of effected platforms > > > > drivers/char/tpm/tpm_crb.c | 47 > > ++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > index 0f3b3f3d12d3..4eb96b85c653 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device *device, > struct crb_priv *priv, > > struct list_head resources; > > struct resource io_res; > > struct device *dev = &device->dev; > > + u32 pa_high, pa_low; > > u64 cmd_pa; > > u32 cmd_size; > > u64 rsp_pa; > > @@ -346,12 +347,27 @@ static int crb_map_io(struct acpi_device *device, > struct crb_priv *priv, > > if (IS_ERR(priv->cca)) > > return PTR_ERR(priv->cca); > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > + /* > > + * PTT HW bug w/a: wake up the device to access > > + * possibly not retained registers. > > + */ > > + ret = crb_cmd_ready(dev, priv); > > + if (ret) > > + return ret; > > + > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > cmd_size = ioread32(&priv->cca->cmd_size); > > + > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > + pa_high, pa_low, cmd_size); > > + > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > > - if (IS_ERR(priv->cmd)) > > - return PTR_ERR(priv->cmd); > > + if (IS_ERR(priv->cmd)) { > > + ret = PTR_ERR(priv->cmd); > > + goto out; > > + } > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > rsp_pa = le64_to_cpu(rsp_pa); > > @@ -359,7 +375,8 @@ static int crb_map_io(struct acpi_device *device, > > struct crb_priv *priv, > > > > if (cmd_pa != rsp_pa) { > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > > - return PTR_ERR_OR_ZERO(priv->rsp); > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > + goto out; > > } > > > > /* According to the PTP specification, overlapping command and > > response @@ -367,12 +384,18 @@ static int crb_map_io(struct acpi_device > *device, struct crb_priv *priv, > > */ > > if (cmd_size != rsp_size) { > > dev_err(dev, FW_BUG "overlapping command and response > buffer sizes are not identical"); > > - return -EINVAL; > > + ret = -EINVAL; > > + goto out; > > } > > + > > priv->cmd_size = cmd_size; > > > > priv->rsp = priv->cmd; > > - return 0; > > + > > +out: > > + crb_go_idle(dev, priv); > > + > > + return ret; > > } > > > > static int crb_acpi_add(struct acpi_device *device) @@ -416,7 +439,15 > > @@ static int crb_acpi_add(struct acpi_device *device) > > if (rc) > > return rc; > > > > - return crb_init(device, priv); > > + rc = crb_cmd_ready(dev, priv); > > + if (rc) > > + return rc; > > I cannot find any valid reason why crb_map_io calls crb_go_idle in the except > in the case of a failure. This is something I complained earlier. Haven't I've already explained that? Each flow has to be enclosed by cmdReady and goIdle. There is nothing different here from the other flows in that matter. The assumption here is that we are starting in the idle state bug because of the HW bug we cannot access the registers. So the whole w/o is enclosed in the crb_map_io. After that we are starting from new, from the idle state. > A minor thing but the extra crb_cmd_ready is basically clutter to the > initialization. The extra cmdReady is here in case runtime pm is not compiled into the kernel and assumption here is that we are starting in the idle state. Please remember that unfortunately we cannot detect whether we are in ready on idle state as the status register is not retained, so everything has to be ordered properly. Thanks Tomas ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote: > > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote: > > > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > > > > > There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT > > > device, where most of the registers in the control area except START, > > > REQUEST, CANCEL, and LOC_CTRL lost retention when the device is in the > > idle state. > > > Hence we need to bring the device to ready state before accessing the > > > other registers. The fix brings device to ready state before trying to > > > read command and response buffer addresses in order to remap the for > > access. > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > --- > > > V2: cmd read need to be called also before crb_init as this will run > > > self test. > > > V3: resend. > > > V4: add Kabylake to the list of effected platforms > > > > > > drivers/char/tpm/tpm_crb.c | 47 > > > ++++++++++++++++++++++++++++++++++++++-------- > > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > > index 0f3b3f3d12d3..4eb96b85c653 100644 > > > --- a/drivers/char/tpm/tpm_crb.c > > > +++ b/drivers/char/tpm/tpm_crb.c > > > @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device *device, > > struct crb_priv *priv, > > > struct list_head resources; > > > struct resource io_res; > > > struct device *dev = &device->dev; > > > + u32 pa_high, pa_low; > > > u64 cmd_pa; > > > u32 cmd_size; > > > u64 rsp_pa; > > > @@ -346,12 +347,27 @@ static int crb_map_io(struct acpi_device *device, > > struct crb_priv *priv, > > > if (IS_ERR(priv->cca)) > > > return PTR_ERR(priv->cca); > > > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > > + /* > > > + * PTT HW bug w/a: wake up the device to access > > > + * possibly not retained registers. > > > + */ > > > + ret = crb_cmd_ready(dev, priv); > > > + if (ret) > > > + return ret; > > > + > > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > > cmd_size = ioread32(&priv->cca->cmd_size); > > > + > > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > > + pa_high, pa_low, cmd_size); > > > + > > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > > > - if (IS_ERR(priv->cmd)) > > > - return PTR_ERR(priv->cmd); > > > + if (IS_ERR(priv->cmd)) { > > > + ret = PTR_ERR(priv->cmd); > > > + goto out; > > > + } > > > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > > rsp_pa = le64_to_cpu(rsp_pa); > > > @@ -359,7 +375,8 @@ static int crb_map_io(struct acpi_device *device, > > > struct crb_priv *priv, > > > > > > if (cmd_pa != rsp_pa) { > > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > > > - return PTR_ERR_OR_ZERO(priv->rsp); > > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > > + goto out; > > > } > > > > > > /* According to the PTP specification, overlapping command and > > > response @@ -367,12 +384,18 @@ static int crb_map_io(struct acpi_device > > *device, struct crb_priv *priv, > > > */ > > > if (cmd_size != rsp_size) { > > > dev_err(dev, FW_BUG "overlapping command and response > > buffer sizes are not identical"); > > > - return -EINVAL; > > > + ret = -EINVAL; > > > + goto out; > > > } > > > + > > > priv->cmd_size = cmd_size; > > > > > > priv->rsp = priv->cmd; > > > - return 0; > > > + > > > +out: > > > + crb_go_idle(dev, priv); > > > + > > > + return ret; > > > } > > > > > > static int crb_acpi_add(struct acpi_device *device) @@ -416,7 +439,15 > > > @@ static int crb_acpi_add(struct acpi_device *device) > > > if (rc) > > > return rc; > > > > > > - return crb_init(device, priv); > > > + rc = crb_cmd_ready(dev, priv); > > > + if (rc) > > > + return rc; > > > > I cannot find any valid reason why crb_map_io calls crb_go_idle in the except > > in the case of a failure. This is something I complained earlier. > > Haven't I've already explained that? Each flow has to be enclosed by > cmdReady and goIdle. There is nothing different here from the other > flows in that matter. The assumption here is that we are starting in > the idle state bug because of the HW bug we cannot access the > registers. So the whole w/o is enclosed in the crb_map_io. After that > we are starting from new, from the idle state. But why you have to do crb_go_idle upon returning from crb_map_io()? > > A minor thing but the extra crb_cmd_ready is basically clutter to the > > initialization. > > The extra cmdReady is here in case runtime pm is not compiled into the > kernel and assumption here is that we are starting in the idle state. > Please remember that unfortunately we cannot detect whether we are in > ready on idle state as the status register is not retained, so > everything has to be ordered properly. Rather you it would be better not to use cmdReady/goIdle at all if kernel is not compiled with CONFIG_PM. > Thanks > Tomas /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Sat, Oct 08, 2016 at 07:00:06PM +0300, Jarkko Sakkinen wrote: > Rather you it would be better not to use cmdReady/goIdle at all if > kernel is not compiled with CONFIG_PM. It would be best practice to ensure the tpm is powered up no matter how the kernel is compiled.. The firmware may have left it in idle or something FWIW, I broadly agree there is no need to put the device into idle on the error paths. It probably wasn't idle when the driver attached to it. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote: > > > > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote: > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > > > > > > > There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT > > > > device, where most of the registers in the control area except > > > > START, REQUEST, CANCEL, and LOC_CTRL lost retention when the > > > > device is in the > > > idle state. > > > > Hence we need to bring the device to ready state before accessing > > > > the other registers. The fix brings device to ready state before > > > > trying to read command and response buffer addresses in order to > > > > remap the for > > > access. > > > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > > --- > > > > V2: cmd read need to be called also before crb_init as this will > > > > run self test. > > > > V3: resend. > > > > V4: add Kabylake to the list of effected platforms > > > > > > > > drivers/char/tpm/tpm_crb.c | 47 > > > > ++++++++++++++++++++++++++++++++++++++-------- > > > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > b/drivers/char/tpm/tpm_crb.c index 0f3b3f3d12d3..4eb96b85c653 > > > > 100644 > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device > > > > *device, > > > struct crb_priv *priv, > > > > struct list_head resources; > > > > struct resource io_res; > > > > struct device *dev = &device->dev; > > > > + u32 pa_high, pa_low; > > > > u64 cmd_pa; > > > > u32 cmd_size; > > > > u64 rsp_pa; > > > > @@ -346,12 +347,27 @@ static int crb_map_io(struct acpi_device > > > > *device, > > > struct crb_priv *priv, > > > > if (IS_ERR(priv->cca)) > > > > return PTR_ERR(priv->cca); > > > > > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > > > + /* > > > > + * PTT HW bug w/a: wake up the device to access > > > > + * possibly not retained registers. > > > > + */ > > > > + ret = crb_cmd_ready(dev, priv); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > > > cmd_size = ioread32(&priv->cca->cmd_size); > > > > + > > > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > > > + pa_high, pa_low, cmd_size); > > > > + > > > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > > > > - if (IS_ERR(priv->cmd)) > > > > - return PTR_ERR(priv->cmd); > > > > + if (IS_ERR(priv->cmd)) { > > > > + ret = PTR_ERR(priv->cmd); > > > > + goto out; > > > > + } > > > > > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > > > rsp_pa = le64_to_cpu(rsp_pa); > > > > @@ -359,7 +375,8 @@ static int crb_map_io(struct acpi_device > > > > *device, struct crb_priv *priv, > > > > > > > > if (cmd_pa != rsp_pa) { > > > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > > > > - return PTR_ERR_OR_ZERO(priv->rsp); > > > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > > > + goto out; > > > > } > > > > > > > > /* According to the PTP specification, overlapping command and > > > > response @@ -367,12 +384,18 @@ static int crb_map_io(struct > > > > acpi_device > > > *device, struct crb_priv *priv, > > > > */ > > > > if (cmd_size != rsp_size) { > > > > dev_err(dev, FW_BUG "overlapping command and response > > > buffer sizes are not identical"); > > > > - return -EINVAL; > > > > + ret = -EINVAL; > > > > + goto out; > > > > } > > > > + > > > > priv->cmd_size = cmd_size; > > > > > > > > priv->rsp = priv->cmd; > > > > - return 0; > > > > + > > > > +out: > > > > + crb_go_idle(dev, priv); > > > > + > > > > + return ret; > > > > } > > > > > > > > static int crb_acpi_add(struct acpi_device *device) @@ -416,7 > > > > +439,15 @@ static int crb_acpi_add(struct acpi_device *device) > > > > if (rc) > > > > return rc; > > > > > > > > - return crb_init(device, priv); > > > > + rc = crb_cmd_ready(dev, priv); > > > > + if (rc) > > > > + return rc; > > > > > > I cannot find any valid reason why crb_map_io calls crb_go_idle in > > > the except in the case of a failure. This is something I complained earlier. > > > > Haven't I've already explained that? Each flow has to be enclosed by > > cmdReady and goIdle. There is nothing different here from the other > > flows in that matter. The assumption here is that we are starting in > > the idle state bug because of the HW bug we cannot access the > > registers. So the whole w/o is enclosed in the crb_map_io. After that > > we are starting from new, from the idle state. > > But why you have to do crb_go_idle upon returning from crb_map_io()? As written above it's is closed code... cmdReady <perform the task> goIdle. > > > > A minor thing but the extra crb_cmd_ready is basically clutter to > > > the initialization. > > > > The extra cmdReady is here in case runtime pm is not compiled into the > > kernel and assumption here is that we are starting in the idle state. > > Please remember that unfortunately we cannot detect whether we are in > > ready on idle state as the status register is not retained, so > > everything has to be ordered properly. > > Rather you it would be better not to use cmdReady/goIdle at all if kernel is not > compiled with CONFIG_PM. ????. You have to do the initial cmdReady, and that's what the code does, otherwise your driver won't work at all. CONFIG_PM is the software thing, it doesn't magically match the HW power state. So you need to do cmdReady. Thanks Tomas ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> On Sat, Oct 08, 2016 at 07:00:06PM +0300, Jarkko Sakkinen wrote: > > > Rather you it would be better not to use cmdReady/goIdle at all if > > kernel is not compiled with CONFIG_PM. > > It would be best practice to ensure the tpm is powered up no matter how the > kernel is compiled.. The firmware may have left it in idle or something > > FWIW, I broadly agree there is no need to put the device into idle on the error > paths. It probably wasn't idle when the driver attached to it. Actually, currently it is more common to have the device in the idle state, so in case a driver is not loaded for any reason by the kernel, the platform won't leak power. Thanks Tomas ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Sat, Oct 08, 2016 at 04:56:39PM +0000, Winkler, Tomas wrote: > > > > > On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote: > > > > > > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote: > > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > > > > > > > > > There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel PTT > > > > > device, where most of the registers in the control area except > > > > > START, REQUEST, CANCEL, and LOC_CTRL lost retention when the > > > > > device is in the > > > > idle state. > > > > > Hence we need to bring the device to ready state before accessing > > > > > the other registers. The fix brings device to ready state before > > > > > trying to read command and response buffer addresses in order to > > > > > remap the for > > > > access. > > > > > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > > > --- > > > > > V2: cmd read need to be called also before crb_init as this will > > > > > run self test. > > > > > V3: resend. > > > > > V4: add Kabylake to the list of effected platforms > > > > > > > > > > drivers/char/tpm/tpm_crb.c | 47 > > > > > ++++++++++++++++++++++++++++++++++++++-------- > > > > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > > b/drivers/char/tpm/tpm_crb.c index 0f3b3f3d12d3..4eb96b85c653 > > > > > 100644 > > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > > @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device > > > > > *device, > > > > struct crb_priv *priv, > > > > > struct list_head resources; > > > > > struct resource io_res; > > > > > struct device *dev = &device->dev; > > > > > + u32 pa_high, pa_low; > > > > > u64 cmd_pa; > > > > > u32 cmd_size; > > > > > u64 rsp_pa; > > > > > @@ -346,12 +347,27 @@ static int crb_map_io(struct acpi_device > > > > > *device, > > > > struct crb_priv *priv, > > > > > if (IS_ERR(priv->cca)) > > > > > return PTR_ERR(priv->cca); > > > > > > > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > > > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > > > > + /* > > > > > + * PTT HW bug w/a: wake up the device to access > > > > > + * possibly not retained registers. > > > > > + */ > > > > > + ret = crb_cmd_ready(dev, priv); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > > > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > > > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > > > > cmd_size = ioread32(&priv->cca->cmd_size); > > > > > + > > > > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > > > > + pa_high, pa_low, cmd_size); > > > > > + > > > > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); > > > > > - if (IS_ERR(priv->cmd)) > > > > > - return PTR_ERR(priv->cmd); > > > > > + if (IS_ERR(priv->cmd)) { > > > > > + ret = PTR_ERR(priv->cmd); > > > > > + goto out; > > > > > + } > > > > > > > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > > > > rsp_pa = le64_to_cpu(rsp_pa); > > > > > @@ -359,7 +375,8 @@ static int crb_map_io(struct acpi_device > > > > > *device, struct crb_priv *priv, > > > > > > > > > > if (cmd_pa != rsp_pa) { > > > > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > > > > > - return PTR_ERR_OR_ZERO(priv->rsp); > > > > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > > > > + goto out; > > > > > } > > > > > > > > > > /* According to the PTP specification, overlapping command and > > > > > response @@ -367,12 +384,18 @@ static int crb_map_io(struct > > > > > acpi_device > > > > *device, struct crb_priv *priv, > > > > > */ > > > > > if (cmd_size != rsp_size) { > > > > > dev_err(dev, FW_BUG "overlapping command and response > > > > buffer sizes are not identical"); > > > > > - return -EINVAL; > > > > > + ret = -EINVAL; > > > > > + goto out; > > > > > } > > > > > + > > > > > priv->cmd_size = cmd_size; > > > > > > > > > > priv->rsp = priv->cmd; > > > > > - return 0; > > > > > + > > > > > +out: > > > > > + crb_go_idle(dev, priv); > > > > > + > > > > > + return ret; > > > > > } > > > > > > > > > > static int crb_acpi_add(struct acpi_device *device) @@ -416,7 > > > > > +439,15 @@ static int crb_acpi_add(struct acpi_device *device) > > > > > if (rc) > > > > > return rc; > > > > > > > > > > - return crb_init(device, priv); > > > > > + rc = crb_cmd_ready(dev, priv); > > > > > + if (rc) > > > > > + return rc; > > > > > > > > I cannot find any valid reason why crb_map_io calls crb_go_idle in > > > > the except in the case of a failure. This is something I complained earlier. > > > > > > Haven't I've already explained that? Each flow has to be enclosed by > > > cmdReady and goIdle. There is nothing different here from the other > > > flows in that matter. The assumption here is that we are starting in > > > the idle state bug because of the HW bug we cannot access the > > > registers. So the whole w/o is enclosed in the crb_map_io. After that > > > we are starting from new, from the idle state. > > > > But why you have to do crb_go_idle upon returning from crb_map_io()? > > As written above it's is closed code... cmdReady <perform the task> goIdle. It does extra cmdReady/goIdle. I don't really get your explanation here. crb_map_io() is not a utility function used after the driver has initialized. crb_map_io() could just leave it ready so you don't have to do an explicit call to crb_cmd_ready() in crb_acpi_add. I think this is quite obvious. > > > > > > A minor thing but the extra crb_cmd_ready is basically clutter to > > > > the initialization. > > > > > > The extra cmdReady is here in case runtime pm is not compiled into the > > > kernel and assumption here is that we are starting in the idle state. > > > Please remember that unfortunately we cannot detect whether we are in > > > ready on idle state as the status register is not retained, so > > > everything has to be ordered properly. > > > > Rather you it would be better not to use cmdReady/goIdle at all if kernel is not > > compiled with CONFIG_PM. > > ????. You have to do the initial cmdReady, and that's what the code > does, otherwise your driver won't work at all. CONFIG_PM is the > software thing, it doesn't magically match the HW power state. So you > need to do cmdReady. In every machine that I've tried so far the PTT has worked if cmdReady/goIdle haven't been used at all by the OS during a power cycle. > Thanks > Tomas I'll keep the current commit (v4 version) because it is tested quite well, no worries about that, but this is something where a cleanup would make sense later on. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Sat, Oct 08, 2016 at 10:42:03AM -0600, Jason Gunthorpe wrote: > On Sat, Oct 08, 2016 at 07:00:06PM +0300, Jarkko Sakkinen wrote: > > > Rather you it would be better not to use cmdReady/goIdle at all if > > kernel is not compiled with CONFIG_PM. > > It would be best practice to ensure the tpm is powered up no matter > how the kernel is compiled.. The firmware may have left it in idle or > something > > FWIW, I broadly agree there is no need to put the device into idle on > the error paths. It probably wasn't idle when the driver attached to > it. I'll keep the current commit because it's anyway tested code. If I want to change this I'll send a separate patch for review. > Jason /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> Subject: Re: [PATCH 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle state > > On Sat, Oct 08, 2016 at 04:56:39PM +0000, Winkler, Tomas wrote: > > > > > > > > On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote: > > > > > > > > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote: > > > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > > > > > > > > > > > There is a HW bug in Skylake, Kabylake, and Broxton PCH Intel > > > > > > PTT device, where most of the registers in the control area > > > > > > except START, REQUEST, CANCEL, and LOC_CTRL lost retention > > > > > > when the device is in the > > > > > idle state. > > > > > > Hence we need to bring the device to ready state before > > > > > > accessing the other registers. The fix brings device to ready > > > > > > state before trying to read command and response buffer > > > > > > addresses in order to remap the for > > > > > access. > > > > > > > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > > > > --- > > > > > > V2: cmd read need to be called also before crb_init as this > > > > > > will run self test. > > > > > > V3: resend. > > > > > > V4: add Kabylake to the list of effected platforms > > > > > > > > > > > > drivers/char/tpm/tpm_crb.c | 47 > > > > > > ++++++++++++++++++++++++++++++++++++++-------- > > > > > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > > > b/drivers/char/tpm/tpm_crb.c index 0f3b3f3d12d3..4eb96b85c653 > > > > > > 100644 > > > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > > > @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device > > > > > > *device, > > > > > struct crb_priv *priv, > > > > > > struct list_head resources; > > > > > > struct resource io_res; > > > > > > struct device *dev = &device->dev; > > > > > > + u32 pa_high, pa_low; > > > > > > u64 cmd_pa; > > > > > > u32 cmd_size; > > > > > > u64 rsp_pa; > > > > > > @@ -346,12 +347,27 @@ static int crb_map_io(struct acpi_device > > > > > > *device, > > > > > struct crb_priv *priv, > > > > > > if (IS_ERR(priv->cca)) > > > > > > return PTR_ERR(priv->cca); > > > > > > > > > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > > > > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > > > > > + /* > > > > > > + * PTT HW bug w/a: wake up the device to access > > > > > > + * possibly not retained registers. > > > > > > + */ > > > > > > + ret = crb_cmd_ready(dev, priv); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > > > > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > > > > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > > > > > cmd_size = ioread32(&priv->cca->cmd_size); > > > > > > + > > > > > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > > > > > + pa_high, pa_low, cmd_size); > > > > > > + > > > > > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, > cmd_size); > > > > > > - if (IS_ERR(priv->cmd)) > > > > > > - return PTR_ERR(priv->cmd); > > > > > > + if (IS_ERR(priv->cmd)) { > > > > > > + ret = PTR_ERR(priv->cmd); > > > > > > + goto out; > > > > > > + } > > > > > > > > > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > > > > > rsp_pa = le64_to_cpu(rsp_pa); @@ -359,7 +375,8 @@ static > int > > > > > > crb_map_io(struct acpi_device *device, struct crb_priv *priv, > > > > > > > > > > > > if (cmd_pa != rsp_pa) { > > > > > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, > rsp_size); > > > > > > - return PTR_ERR_OR_ZERO(priv->rsp); > > > > > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > > > > > + goto out; > > > > > > } > > > > > > > > > > > > /* According to the PTP specification, overlapping command > > > > > > and response @@ -367,12 +384,18 @@ static int > > > > > > crb_map_io(struct acpi_device > > > > > *device, struct crb_priv *priv, > > > > > > */ > > > > > > if (cmd_size != rsp_size) { > > > > > > dev_err(dev, FW_BUG "overlapping command and > response > > > > > buffer sizes are not identical"); > > > > > > - return -EINVAL; > > > > > > + ret = -EINVAL; > > > > > > + goto out; > > > > > > } > > > > > > + > > > > > > priv->cmd_size = cmd_size; > > > > > > > > > > > > priv->rsp = priv->cmd; > > > > > > - return 0; > > > > > > + > > > > > > +out: > > > > > > + crb_go_idle(dev, priv); > > > > > > + > > > > > > + return ret; > > > > > > } > > > > > > > > > > > > static int crb_acpi_add(struct acpi_device *device) @@ -416,7 > > > > > > +439,15 @@ static int crb_acpi_add(struct acpi_device *device) > > > > > > if (rc) > > > > > > return rc; > > > > > > > > > > > > - return crb_init(device, priv); > > > > > > + rc = crb_cmd_ready(dev, priv); > > > > > > + if (rc) > > > > > > + return rc; > > > > > > > > > > I cannot find any valid reason why crb_map_io calls crb_go_idle > > > > > in the except in the case of a failure. This is something I complained > earlier. > > > > > > > > Haven't I've already explained that? Each flow has to be enclosed > > > > by cmdReady and goIdle. There is nothing different here from the > > > > other flows in that matter. The assumption here is that we are > > > > starting in the idle state bug because of the HW bug we cannot > > > > access the registers. So the whole w/o is enclosed in the > > > > crb_map_io. After that we are starting from new, from the idle state. > > > > > > But why you have to do crb_go_idle upon returning from crb_map_io()? > > > > As written above it's is closed code... cmdReady <perform the task> goIdle. > > It does extra cmdReady/goIdle. I don't really get your explanation here. > crb_map_io() is not a utility function used after the driver has initialized. > > crb_map_io() could just leave it ready so you don't have to do an explicit call > to crb_cmd_ready() in crb_acpi_add. I think this is quite obvious. > > > > > > > > > A minor thing but the extra crb_cmd_ready is basically clutter > > > > > to the initialization. > > > > > > > > The extra cmdReady is here in case runtime pm is not compiled into > > > > the kernel and assumption here is that we are starting in the idle state. > > > > Please remember that unfortunately we cannot detect whether we are > > > > in ready on idle state as the status register is not retained, so > > > > everything has to be ordered properly. > > > > > > Rather you it would be better not to use cmdReady/goIdle at all if > > > kernel is not compiled with CONFIG_PM. > > > > ????. You have to do the initial cmdReady, and that's what the code > > does, otherwise your driver won't work at all. CONFIG_PM is the > > software thing, it doesn't magically match the HW power state. So you > > need to do cmdReady. > > In every machine that I've tried so far the PTT has worked if cmdReady/goIdle > haven't been used at all by the OS during a power cycle. You mean BXT and SKL, right, but that's because you've been blessed with a specific BIOS that keeps it up. > > I'll keep the current commit (v4 version) because it is tested quite well, no > worries about that, but this is something where a cleanup would make sense > later on. No problem, though so far I believe the flow is correct. Tomas ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > > Subject: Re: [PATCH 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle > > state > > > > On Sat, Oct 08, 2016 at 04:56:39PM +0000, Winkler, Tomas wrote: > > > > > > > > > > > On Sat, Oct 08, 2016 at 02:27:06PM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > On Sat, Oct 08, 2016 at 02:59:37PM +0300, Tomas Winkler wrote: > > > > > > > From: "Winkler, Tomas" <tomas.winkler@intel.com> > > > > > > > > > > > > > > There is a HW bug in Skylake, Kabylake, and Broxton PCH > > > > > > > Intel PTT device, where most of the registers in the control > > > > > > > area except START, REQUEST, CANCEL, and LOC_CTRL lost > > > > > > > retention when the device is in the > > > > > > idle state. > > > > > > > Hence we need to bring the device to ready state before > > > > > > > accessing the other registers. The fix brings device to > > > > > > > ready state before trying to read command and response > > > > > > > buffer addresses in order to remap the for > > > > > > access. > > > > > > > > > > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > > > > > > --- > > > > > > > V2: cmd read need to be called also before crb_init as this > > > > > > > will run self test. > > > > > > > V3: resend. > > > > > > > V4: add Kabylake to the list of effected platforms > > > > > > > > > > > > > > drivers/char/tpm/tpm_crb.c | 47 > > > > > > > ++++++++++++++++++++++++++++++++++++++-------- > > > > > > > 1 file changed, 39 insertions(+), 8 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > > > > b/drivers/char/tpm/tpm_crb.c index > > > > > > > 0f3b3f3d12d3..4eb96b85c653 > > > > > > > 100644 > > > > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > > > > @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device > > > > > > > *device, > > > > > > struct crb_priv *priv, > > > > > > > struct list_head resources; > > > > > > > struct resource io_res; > > > > > > > struct device *dev = &device->dev; > > > > > > > + u32 pa_high, pa_low; > > > > > > > u64 cmd_pa; > > > > > > > u32 cmd_size; > > > > > > > u64 rsp_pa; > > > > > > > @@ -346,12 +347,27 @@ static int crb_map_io(struct > > > > > > > acpi_device *device, > > > > > > struct crb_priv *priv, > > > > > > > if (IS_ERR(priv->cca)) > > > > > > > return PTR_ERR(priv->cca); > > > > > > > > > > > > > > - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | > > > > > > > - (u64) ioread32(&priv->cca->cmd_pa_low); > > > > > > > + /* > > > > > > > + * PTT HW bug w/a: wake up the device to access > > > > > > > + * possibly not retained registers. > > > > > > > + */ > > > > > > > + ret = crb_cmd_ready(dev, priv); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + > > > > > > > + pa_high = ioread32(&priv->cca->cmd_pa_high); > > > > > > > + pa_low = ioread32(&priv->cca->cmd_pa_low); > > > > > > > + cmd_pa = ((u64)pa_high << 32) | pa_low; > > > > > > > cmd_size = ioread32(&priv->cca->cmd_size); > > > > > > > + > > > > > > > + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", > > > > > > > + pa_high, pa_low, cmd_size); > > > > > > > + > > > > > > > priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, > > cmd_size); > > > > > > > - if (IS_ERR(priv->cmd)) > > > > > > > - return PTR_ERR(priv->cmd); > > > > > > > + if (IS_ERR(priv->cmd)) { > > > > > > > + ret = PTR_ERR(priv->cmd); > > > > > > > + goto out; > > > > > > > + } > > > > > > > > > > > > > > memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); > > > > > > > rsp_pa = le64_to_cpu(rsp_pa); @@ -359,7 +375,8 @@ static > > int > > > > > > > crb_map_io(struct acpi_device *device, struct crb_priv > > > > > > > *priv, > > > > > > > > > > > > > > if (cmd_pa != rsp_pa) { > > > > > > > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, > > rsp_size); > > > > > > > - return PTR_ERR_OR_ZERO(priv->rsp); > > > > > > > + ret = PTR_ERR_OR_ZERO(priv->rsp); > > > > > > > + goto out; > > > > > > > } > > > > > > > > > > > > > > /* According to the PTP specification, overlapping command > > > > > > > and response @@ -367,12 +384,18 @@ static int > > > > > > > crb_map_io(struct acpi_device > > > > > > *device, struct crb_priv *priv, > > > > > > > */ > > > > > > > if (cmd_size != rsp_size) { > > > > > > > dev_err(dev, FW_BUG "overlapping command and > > response > > > > > > buffer sizes are not identical"); > > > > > > > - return -EINVAL; > > > > > > > + ret = -EINVAL; > > > > > > > + goto out; > > > > > > > } > > > > > > > + > > > > > > > priv->cmd_size = cmd_size; > > > > > > > > > > > > > > priv->rsp = priv->cmd; > > > > > > > - return 0; > > > > > > > + > > > > > > > +out: > > > > > > > + crb_go_idle(dev, priv); > > > > > > > + > > > > > > > + return ret; > > > > > > > } > > > > > > > > > > > > > > static int crb_acpi_add(struct acpi_device *device) @@ > > > > > > > -416,7 > > > > > > > +439,15 @@ static int crb_acpi_add(struct acpi_device > > > > > > > +*device) > > > > > > > if (rc) > > > > > > > return rc; > > > > > > > > > > > > > > - return crb_init(device, priv); > > > > > > > + rc = crb_cmd_ready(dev, priv); > > > > > > > + if (rc) > > > > > > > + return rc; > > > > > > > > > > > > I cannot find any valid reason why crb_map_io calls > > > > > > crb_go_idle in the except in the case of a failure. This is > > > > > > something I complained > > earlier. > > > > > > > > > > Haven't I've already explained that? Each flow has to be > > > > > enclosed by cmdReady and goIdle. There is nothing different > > > > > here from the other flows in that matter. The assumption here > > > > > is that we are starting in the idle state bug because of the HW > > > > > bug we cannot access the registers. So the whole w/o is enclosed > > > > > in the crb_map_io. After that we are starting from new, from the idle > state. > > > > > > > > But why you have to do crb_go_idle upon returning from crb_map_io()? > > > > > > As written above it's is closed code... cmdReady <perform the task> > goIdle. > > > > It does extra cmdReady/goIdle. I don't really get your explanation here. > > crb_map_io() is not a utility function used after the driver has initialized. > > > > crb_map_io() could just leave it ready so you don't have to do an > > explicit call to crb_cmd_ready() in crb_acpi_add. I think this is quite obvious. > > > > > > > > > > > > A minor thing but the extra crb_cmd_ready is basically clutter > > > > > > to the initialization. > > > > > > > > > > The extra cmdReady is here in case runtime pm is not compiled > > > > > into the kernel and assumption here is that we are starting in the idle > state. > > > > > Please remember that unfortunately we cannot detect whether we > > > > > are in ready on idle state as the status register is not > > > > > retained, so everything has to be ordered properly. > > > > > > > > Rather you it would be better not to use cmdReady/goIdle at all if > > > > kernel is not compiled with CONFIG_PM. > > > > > > ????. You have to do the initial cmdReady, and that's what the code > > > does, otherwise your driver won't work at all. CONFIG_PM is the > > > software thing, it doesn't magically match the HW power state. So > > > you need to do cmdReady. > > > > In every machine that I've tried so far the PTT has worked if > > cmdReady/goIdle haven't been used at all by the OS during a power cycle. > > You mean BXT and SKL, right, but that's because you've been blessed with a > specific BIOS that keeps it up. Hmm, now I understand why you have a bit different angle on the problem. Tomas ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 0f3b3f3d12d3..4eb96b85c653 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -319,6 +319,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, struct list_head resources; struct resource io_res; struct device *dev = &device->dev; + u32 pa_high, pa_low; u64 cmd_pa; u32 cmd_size; u64 rsp_pa; @@ -346,12 +347,27 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, if (IS_ERR(priv->cca)) return PTR_ERR(priv->cca); - cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) | - (u64) ioread32(&priv->cca->cmd_pa_low); + /* + * PTT HW bug w/a: wake up the device to access + * possibly not retained registers. + */ + ret = crb_cmd_ready(dev, priv); + if (ret) + return ret; + + pa_high = ioread32(&priv->cca->cmd_pa_high); + pa_low = ioread32(&priv->cca->cmd_pa_low); + cmd_pa = ((u64)pa_high << 32) | pa_low; cmd_size = ioread32(&priv->cca->cmd_size); + + dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n", + pa_high, pa_low, cmd_size); + priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size); - if (IS_ERR(priv->cmd)) - return PTR_ERR(priv->cmd); + if (IS_ERR(priv->cmd)) { + ret = PTR_ERR(priv->cmd); + goto out; + } memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8); rsp_pa = le64_to_cpu(rsp_pa); @@ -359,7 +375,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, if (cmd_pa != rsp_pa) { priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); - return PTR_ERR_OR_ZERO(priv->rsp); + ret = PTR_ERR_OR_ZERO(priv->rsp); + goto out; } /* According to the PTP specification, overlapping command and response @@ -367,12 +384,18 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv, */ if (cmd_size != rsp_size) { dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical"); - return -EINVAL; + ret = -EINVAL; + goto out; } + priv->cmd_size = cmd_size; priv->rsp = priv->cmd; - return 0; + +out: + crb_go_idle(dev, priv); + + return ret; } static int crb_acpi_add(struct acpi_device *device) @@ -416,7 +439,15 @@ static int crb_acpi_add(struct acpi_device *device) if (rc) return rc; - return crb_init(device, priv); + rc = crb_cmd_ready(dev, priv); + if (rc) + return rc; + + rc = crb_init(device, priv); + if (rc) + crb_go_idle(dev, priv); + + return rc; } static int crb_acpi_remove(struct acpi_device *device)