Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 07:53:06PM +0200, Jarkko Sakkinen wrote: > On Mon, Dec 12, 2016 at 03:57:54PM +, Winkler, Tomas wrote: > > > > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > > > > > In order to provide access to locality registers, this commits > > > > > > adds mapping of the head of the CRB registers, which are located > > > > > > right > > > before the control area. > > > > > > > > > > > > Signed-off-by: Jarkko Sakkinen> > > > > > --- > > > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > > > + > > > > > > - > > > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > > > b/drivers/char/tpm/tpm_crb.c index > > > > > > 717b6b4..80b9759 100644 > > > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > > > }; > > > > > > > > > > > > -struct crb_control_area { > > > > > > - u32 req; > > > > > > - u32 sts; > > > > > > - u32 cancel; > > > > > > - u32 start; > > > > > > - u32 int_enable; > > > > > > - u32 int_sts; > > > > > > - u32 cmd_size; > > > > > > - u32 cmd_pa_low; > > > > > > - u32 cmd_pa_high; > > > > > > - u32 rsp_size; > > > > > > - u64 rsp_pa; > > > > > > +struct crb_regs_head { > > > > > > + u32 loc_state; > > > > > > + u32 reserved1; > > > > > > + u32 loc_ctrl; > > > > > > + u32 loc_sts; > > > > > > + u8 reserved2[32]; > > > > > > + u64 intf_id; > > > > > > + u64 ctrl_ext; > > > > > > +} __packed; > > > > > > + > > > > > > > > > > > +struct crb_regs_tail { > > > > > Why to change the name this is still control_area > > > > > > And how would you name struct crb_regs_h then? > > > > Just crb_regs > > > > > In my opinion PC it makes a lot of sense to speak about registers here > > > rather > > > than control area now that it is extended to the full range. The PC Client > > > Specification also speaks about registers. > > > > Right so crb_regs is to be and the nonstandard implementation of the > > legacy platforms should be even factored out. > > I do not see that we would stop supporting pre-Skylake platforms in the > near future so probably won't be factored out. s/near/forseeable/ /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 07:53:06PM +0200, Jarkko Sakkinen wrote: > On Mon, Dec 12, 2016 at 03:57:54PM +, Winkler, Tomas wrote: > > > > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > > > > > In order to provide access to locality registers, this commits > > > > > > adds mapping of the head of the CRB registers, which are located > > > > > > right > > > before the control area. > > > > > > > > > > > > Signed-off-by: Jarkko Sakkinen > > > > > > --- > > > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > > > + > > > > > > - > > > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > > > b/drivers/char/tpm/tpm_crb.c index > > > > > > 717b6b4..80b9759 100644 > > > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > > > }; > > > > > > > > > > > > -struct crb_control_area { > > > > > > - u32 req; > > > > > > - u32 sts; > > > > > > - u32 cancel; > > > > > > - u32 start; > > > > > > - u32 int_enable; > > > > > > - u32 int_sts; > > > > > > - u32 cmd_size; > > > > > > - u32 cmd_pa_low; > > > > > > - u32 cmd_pa_high; > > > > > > - u32 rsp_size; > > > > > > - u64 rsp_pa; > > > > > > +struct crb_regs_head { > > > > > > + u32 loc_state; > > > > > > + u32 reserved1; > > > > > > + u32 loc_ctrl; > > > > > > + u32 loc_sts; > > > > > > + u8 reserved2[32]; > > > > > > + u64 intf_id; > > > > > > + u64 ctrl_ext; > > > > > > +} __packed; > > > > > > + > > > > > > > > > > > +struct crb_regs_tail { > > > > > Why to change the name this is still control_area > > > > > > And how would you name struct crb_regs_h then? > > > > Just crb_regs > > > > > In my opinion PC it makes a lot of sense to speak about registers here > > > rather > > > than control area now that it is extended to the full range. The PC Client > > > Specification also speaks about registers. > > > > Right so crb_regs is to be and the nonstandard implementation of the > > legacy platforms should be even factored out. > > I do not see that we would stop supporting pre-Skylake platforms in the > near future so probably won't be factored out. s/near/forseeable/ /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 03:57:54PM +, Winkler, Tomas wrote: > > > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > > > In order to provide access to locality registers, this commits > > > > > adds mapping of the head of the CRB registers, which are located right > > before the control area. > > > > > > > > > > Signed-off-by: Jarkko Sakkinen> > > > > --- > > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > > + > > > > > - > > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > > b/drivers/char/tpm/tpm_crb.c index > > > > > 717b6b4..80b9759 100644 > > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > > }; > > > > > > > > > > -struct crb_control_area { > > > > > - u32 req; > > > > > - u32 sts; > > > > > - u32 cancel; > > > > > - u32 start; > > > > > - u32 int_enable; > > > > > - u32 int_sts; > > > > > - u32 cmd_size; > > > > > - u32 cmd_pa_low; > > > > > - u32 cmd_pa_high; > > > > > - u32 rsp_size; > > > > > - u64 rsp_pa; > > > > > +struct crb_regs_head { > > > > > + u32 loc_state; > > > > > + u32 reserved1; > > > > > + u32 loc_ctrl; > > > > > + u32 loc_sts; > > > > > + u8 reserved2[32]; > > > > > + u64 intf_id; > > > > > + u64 ctrl_ext; > > > > > +} __packed; > > > > > + > > > > > > > > > +struct crb_regs_tail { > > > > Why to change the name this is still control_area > > > > And how would you name struct crb_regs_h then? > > Just crb_regs > > > In my opinion PC it makes a lot of sense to speak about registers here > > rather > > than control area now that it is extended to the full range. The PC Client > > Specification also speaks about registers. > > Right so crb_regs is to be and the nonstandard implementation of the > legacy platforms should be even factored out. I do not see that we would stop supporting pre-Skylake platforms in the near future so probably won't be factored out. /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 03:57:54PM +, Winkler, Tomas wrote: > > > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > > > In order to provide access to locality registers, this commits > > > > > adds mapping of the head of the CRB registers, which are located right > > before the control area. > > > > > > > > > > Signed-off-by: Jarkko Sakkinen > > > > > --- > > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > > + > > > > > - > > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > > b/drivers/char/tpm/tpm_crb.c index > > > > > 717b6b4..80b9759 100644 > > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > > }; > > > > > > > > > > -struct crb_control_area { > > > > > - u32 req; > > > > > - u32 sts; > > > > > - u32 cancel; > > > > > - u32 start; > > > > > - u32 int_enable; > > > > > - u32 int_sts; > > > > > - u32 cmd_size; > > > > > - u32 cmd_pa_low; > > > > > - u32 cmd_pa_high; > > > > > - u32 rsp_size; > > > > > - u64 rsp_pa; > > > > > +struct crb_regs_head { > > > > > + u32 loc_state; > > > > > + u32 reserved1; > > > > > + u32 loc_ctrl; > > > > > + u32 loc_sts; > > > > > + u8 reserved2[32]; > > > > > + u64 intf_id; > > > > > + u64 ctrl_ext; > > > > > +} __packed; > > > > > + > > > > > > > > > +struct crb_regs_tail { > > > > Why to change the name this is still control_area > > > > And how would you name struct crb_regs_h then? > > Just crb_regs > > > In my opinion PC it makes a lot of sense to speak about registers here > > rather > > than control area now that it is extended to the full range. The PC Client > > Specification also speaks about registers. > > Right so crb_regs is to be and the nonstandard implementation of the > legacy platforms should be even factored out. I do not see that we would stop supporting pre-Skylake platforms in the near future so probably won't be factored out. /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 03:28:01PM +, Winkler, Tomas wrote: > > > > I've tested is completley disjoint from the region pointed by the ACPI > > device > > tree (SSDT in PTT's case). > > > > Here's example device from my x250: > > > > Device (TPM) > > { > > Name (_HID, "MSFT0101" /* TPM 2.0 Security Device */) // _HID: Hardware ID > > Name (_STR, Unicode ("TPM 2.0 Device")) // _STR: Description String Name > > (CRS, ResourceTemplate () { Memory32Fixed (ReadOnly, > > 0x, // Address Base > > 0x1000, // Address Length > > _Y00) > > > I'm not sure what is that? > > > Memory32Fixed (ReadOnly, > > 0xFED7, // Address Base > > 0x1000, // Address Length > > ) > > But 0xFED7 should point to CCA When I implemented originally the driver I followed the MS specification and used the value from TPM2 ACPI table. The driver ioremaps CCDFF000, not the address from SSDT. > Tomas > Thanks /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 03:28:01PM +, Winkler, Tomas wrote: > > > > I've tested is completley disjoint from the region pointed by the ACPI > > device > > tree (SSDT in PTT's case). > > > > Here's example device from my x250: > > > > Device (TPM) > > { > > Name (_HID, "MSFT0101" /* TPM 2.0 Security Device */) // _HID: Hardware ID > > Name (_STR, Unicode ("TPM 2.0 Device")) // _STR: Description String Name > > (CRS, ResourceTemplate () { Memory32Fixed (ReadOnly, > > 0x, // Address Base > > 0x1000, // Address Length > > _Y00) > > > I'm not sure what is that? > > > Memory32Fixed (ReadOnly, > > 0xFED7, // Address Base > > 0x1000, // Address Length > > ) > > But 0xFED7 should point to CCA When I implemented originally the driver I followed the MS specification and used the value from TPM2 ACPI table. The driver ioremaps CCDFF000, not the address from SSDT. > Tomas > Thanks /Jarkko
RE: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
> > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > In order to provide access to locality registers, this commits > > > > adds mapping of the head of the CRB registers, which are located right > before the control area. > > > > > > > > Signed-off-by: Jarkko Sakkinen> > > > --- > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > + > > > > - > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > b/drivers/char/tpm/tpm_crb.c index > > > > 717b6b4..80b9759 100644 > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > }; > > > > > > > > -struct crb_control_area { > > > > - u32 req; > > > > - u32 sts; > > > > - u32 cancel; > > > > - u32 start; > > > > - u32 int_enable; > > > > - u32 int_sts; > > > > - u32 cmd_size; > > > > - u32 cmd_pa_low; > > > > - u32 cmd_pa_high; > > > > - u32 rsp_size; > > > > - u64 rsp_pa; > > > > +struct crb_regs_head { > > > > + u32 loc_state; > > > > + u32 reserved1; > > > > + u32 loc_ctrl; > > > > + u32 loc_sts; > > > > + u8 reserved2[32]; > > > > + u64 intf_id; > > > > + u64 ctrl_ext; > > > > +} __packed; > > > > + > > > > > > > +struct crb_regs_tail { > > > Why to change the name this is still control_area > > And how would you name struct crb_regs_h then? Just crb_regs > In my opinion PC it makes a lot of sense to speak about registers here rather > than control area now that it is extended to the full range. The PC Client > Specification also speaks about registers. Right so crb_regs is to be and the nonstandard implementation of the legacy platforms should be even factored out. > > > > > + u32 ctrl_req; > > > > + u32 ctrl_sts; > > > > + u32 ctrl_cancel; > > > > + u32 ctrl_start; > > > > + u32 ctrl_int_enable; > > > > + u32 ctrl_int_sts; > > > > + u32 ctrl_cmd_size; > > > > + u32 ctrl_cmd_pa_low; > > > > + u32 ctrl_cmd_pa_high; > > > > + u32 ctrl_rsp_size; > > > > + u64 ctrl_rsp_pa; > > > > } __packed; > > > > > > > > enum crb_status { > > > > @@ -78,7 +88,8 @@ enum crb_flags { struct crb_priv { > > > > unsigned int flags; > > > > void __iomem *iobase; > > > > - struct crb_control_area __iomem *cca; > > > > + struct crb_regs_head __iomem *regs_h; > > > > + struct crb_regs_tail __iomem *regs_t; > > > > > > Same here, why to change the name, let's keep it cca it will reduce > > > the size of patch and make it more back portable. > > Backportability is a always a secondary priority for new features albeit > something that can be considered if it doesn't get in the way. I think of it as a staged steps. First do the minimal fix then do the overhaul. Anyway you are kind of counterproductive to yourself as you already know this need a back port. Thanks Tomas
RE: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
> > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > In order to provide access to locality registers, this commits > > > > adds mapping of the head of the CRB registers, which are located right > before the control area. > > > > > > > > Signed-off-by: Jarkko Sakkinen > > > > --- > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > + > > > > - > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c > > > > b/drivers/char/tpm/tpm_crb.c index > > > > 717b6b4..80b9759 100644 > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > }; > > > > > > > > -struct crb_control_area { > > > > - u32 req; > > > > - u32 sts; > > > > - u32 cancel; > > > > - u32 start; > > > > - u32 int_enable; > > > > - u32 int_sts; > > > > - u32 cmd_size; > > > > - u32 cmd_pa_low; > > > > - u32 cmd_pa_high; > > > > - u32 rsp_size; > > > > - u64 rsp_pa; > > > > +struct crb_regs_head { > > > > + u32 loc_state; > > > > + u32 reserved1; > > > > + u32 loc_ctrl; > > > > + u32 loc_sts; > > > > + u8 reserved2[32]; > > > > + u64 intf_id; > > > > + u64 ctrl_ext; > > > > +} __packed; > > > > + > > > > > > > +struct crb_regs_tail { > > > Why to change the name this is still control_area > > And how would you name struct crb_regs_h then? Just crb_regs > In my opinion PC it makes a lot of sense to speak about registers here rather > than control area now that it is extended to the full range. The PC Client > Specification also speaks about registers. Right so crb_regs is to be and the nonstandard implementation of the legacy platforms should be even factored out. > > > > > + u32 ctrl_req; > > > > + u32 ctrl_sts; > > > > + u32 ctrl_cancel; > > > > + u32 ctrl_start; > > > > + u32 ctrl_int_enable; > > > > + u32 ctrl_int_sts; > > > > + u32 ctrl_cmd_size; > > > > + u32 ctrl_cmd_pa_low; > > > > + u32 ctrl_cmd_pa_high; > > > > + u32 ctrl_rsp_size; > > > > + u64 ctrl_rsp_pa; > > > > } __packed; > > > > > > > > enum crb_status { > > > > @@ -78,7 +88,8 @@ enum crb_flags { struct crb_priv { > > > > unsigned int flags; > > > > void __iomem *iobase; > > > > - struct crb_control_area __iomem *cca; > > > > + struct crb_regs_head __iomem *regs_h; > > > > + struct crb_regs_tail __iomem *regs_t; > > > > > > Same here, why to change the name, let's keep it cca it will reduce > > > the size of patch and make it more back portable. > > Backportability is a always a secondary priority for new features albeit > something that can be considered if it doesn't get in the way. I think of it as a staged steps. First do the minimal fix then do the overhaul. Anyway you are kind of counterproductive to yourself as you already know this need a back port. Thanks Tomas
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 05:31:08PM +0200, Jarkko Sakkinen wrote: > On Mon, Dec 12, 2016 at 05:20:01PM +0200, Jarkko Sakkinen wrote: > > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > In order to provide access to locality registers, this commits adds > > > > mapping of > > > > the head of the CRB registers, which are located right before the > > > > control area. > > > > > > > > Signed-off-by: Jarkko Sakkinen> > > > --- > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > + > > > > - > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > > > index > > > > 717b6b4..80b9759 100644 > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > }; > > > > > > > > -struct crb_control_area { > > > > - u32 req; > > > > - u32 sts; > > > > - u32 cancel; > > > > - u32 start; > > > > - u32 int_enable; > > > > - u32 int_sts; > > > > - u32 cmd_size; > > > > - u32 cmd_pa_low; > > > > - u32 cmd_pa_high; > > > > - u32 rsp_size; > > > > - u64 rsp_pa; > > > > +struct crb_regs_head { > > > > + u32 loc_state; > > > > + u32 reserved1; > > > > + u32 loc_ctrl; > > > > + u32 loc_sts; > > > > + u8 reserved2[32]; > > > > + u64 intf_id; > > > > + u64 ctrl_ext; > > > > +} __packed; > > > > + > > > > > > > +struct crb_regs_tail { > > > Why to change the name this is still control_area > > And how would you name struct crb_regs_h then? > > In my opinion PC it makes a lot of sense to speak about registers here ~~ Typo. /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 05:31:08PM +0200, Jarkko Sakkinen wrote: > On Mon, Dec 12, 2016 at 05:20:01PM +0200, Jarkko Sakkinen wrote: > > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > > > In order to provide access to locality registers, this commits adds > > > > mapping of > > > > the head of the CRB registers, which are located right before the > > > > control area. > > > > > > > > Signed-off-by: Jarkko Sakkinen > > > > --- > > > > drivers/char/tpm/tpm_crb.c | 89 > > > > + > > > > - > > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > > > index > > > > 717b6b4..80b9759 100644 > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > > CRB_CANCEL_INVOKE = BIT(0), > > > > }; > > > > > > > > -struct crb_control_area { > > > > - u32 req; > > > > - u32 sts; > > > > - u32 cancel; > > > > - u32 start; > > > > - u32 int_enable; > > > > - u32 int_sts; > > > > - u32 cmd_size; > > > > - u32 cmd_pa_low; > > > > - u32 cmd_pa_high; > > > > - u32 rsp_size; > > > > - u64 rsp_pa; > > > > +struct crb_regs_head { > > > > + u32 loc_state; > > > > + u32 reserved1; > > > > + u32 loc_ctrl; > > > > + u32 loc_sts; > > > > + u8 reserved2[32]; > > > > + u64 intf_id; > > > > + u64 ctrl_ext; > > > > +} __packed; > > > > + > > > > > > > +struct crb_regs_tail { > > > Why to change the name this is still control_area > > And how would you name struct crb_regs_h then? > > In my opinion PC it makes a lot of sense to speak about registers here ~~ Typo. /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 05:20:01PM +0200, Jarkko Sakkinen wrote: > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > In order to provide access to locality registers, this commits adds > > > mapping of > > > the head of the CRB registers, which are located right before the control > > > area. > > > > > > Signed-off-by: Jarkko Sakkinen> > > --- > > > drivers/char/tpm/tpm_crb.c | 89 + > > > - > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > > > 717b6b4..80b9759 100644 > > > --- a/drivers/char/tpm/tpm_crb.c > > > +++ b/drivers/char/tpm/tpm_crb.c > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > CRB_CANCEL_INVOKE = BIT(0), > > > }; > > > > > > -struct crb_control_area { > > > - u32 req; > > > - u32 sts; > > > - u32 cancel; > > > - u32 start; > > > - u32 int_enable; > > > - u32 int_sts; > > > - u32 cmd_size; > > > - u32 cmd_pa_low; > > > - u32 cmd_pa_high; > > > - u32 rsp_size; > > > - u64 rsp_pa; > > > +struct crb_regs_head { > > > + u32 loc_state; > > > + u32 reserved1; > > > + u32 loc_ctrl; > > > + u32 loc_sts; > > > + u8 reserved2[32]; > > > + u64 intf_id; > > > + u64 ctrl_ext; > > > +} __packed; > > > + > > > > > +struct crb_regs_tail { > > Why to change the name this is still control_area And how would you name struct crb_regs_h then? In my opinion PC it makes a lot of sense to speak about registers here rather than control area now that it is extended to the full range. The PC Client Specification also speaks about registers. > > > + u32 ctrl_req; > > > + u32 ctrl_sts; > > > + u32 ctrl_cancel; > > > + u32 ctrl_start; > > > + u32 ctrl_int_enable; > > > + u32 ctrl_int_sts; > > > + u32 ctrl_cmd_size; > > > + u32 ctrl_cmd_pa_low; > > > + u32 ctrl_cmd_pa_high; > > > + u32 ctrl_rsp_size; > > > + u64 ctrl_rsp_pa; > > > } __packed; > > > > > > enum crb_status { > > > @@ -78,7 +88,8 @@ enum crb_flags { > > > struct crb_priv { > > > unsigned int flags; > > > void __iomem *iobase; > > > - struct crb_control_area __iomem *cca; > > > + struct crb_regs_head __iomem *regs_h; > > > + struct crb_regs_tail __iomem *regs_t; > > > > Same here, why to change the name, let's keep it cca it will reduce > > the size of patch and make it more back portable. Backportability is a always a secondary priority for new features albeit something that can be considered if it doesn't get in the way. /Jarkko
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 05:20:01PM +0200, Jarkko Sakkinen wrote: > On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > > > In order to provide access to locality registers, this commits adds > > > mapping of > > > the head of the CRB registers, which are located right before the control > > > area. > > > > > > Signed-off-by: Jarkko Sakkinen > > > --- > > > drivers/char/tpm/tpm_crb.c | 89 + > > > - > > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > > > 717b6b4..80b9759 100644 > > > --- a/drivers/char/tpm/tpm_crb.c > > > +++ b/drivers/char/tpm/tpm_crb.c > > > @@ -52,18 +52,28 @@ enum crb_cancel { > > > CRB_CANCEL_INVOKE = BIT(0), > > > }; > > > > > > -struct crb_control_area { > > > - u32 req; > > > - u32 sts; > > > - u32 cancel; > > > - u32 start; > > > - u32 int_enable; > > > - u32 int_sts; > > > - u32 cmd_size; > > > - u32 cmd_pa_low; > > > - u32 cmd_pa_high; > > > - u32 rsp_size; > > > - u64 rsp_pa; > > > +struct crb_regs_head { > > > + u32 loc_state; > > > + u32 reserved1; > > > + u32 loc_ctrl; > > > + u32 loc_sts; > > > + u8 reserved2[32]; > > > + u64 intf_id; > > > + u64 ctrl_ext; > > > +} __packed; > > > + > > > > > +struct crb_regs_tail { > > Why to change the name this is still control_area And how would you name struct crb_regs_h then? In my opinion PC it makes a lot of sense to speak about registers here rather than control area now that it is extended to the full range. The PC Client Specification also speaks about registers. > > > + u32 ctrl_req; > > > + u32 ctrl_sts; > > > + u32 ctrl_cancel; > > > + u32 ctrl_start; > > > + u32 ctrl_int_enable; > > > + u32 ctrl_int_sts; > > > + u32 ctrl_cmd_size; > > > + u32 ctrl_cmd_pa_low; > > > + u32 ctrl_cmd_pa_high; > > > + u32 ctrl_rsp_size; > > > + u64 ctrl_rsp_pa; > > > } __packed; > > > > > > enum crb_status { > > > @@ -78,7 +88,8 @@ enum crb_flags { > > > struct crb_priv { > > > unsigned int flags; > > > void __iomem *iobase; > > > - struct crb_control_area __iomem *cca; > > > + struct crb_regs_head __iomem *regs_h; > > > + struct crb_regs_tail __iomem *regs_t; > > > > Same here, why to change the name, let's keep it cca it will reduce > > the size of patch and make it more back portable. Backportability is a always a secondary priority for new features albeit something that can be considered if it doesn't get in the way. /Jarkko
RE: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
> > I've tested is completley disjoint from the region pointed by the ACPI device > tree (SSDT in PTT's case). > > Here's example device from my x250: > > Device (TPM) > { > Name (_HID, "MSFT0101" /* TPM 2.0 Security Device */) // _HID: Hardware ID > Name (_STR, Unicode ("TPM 2.0 Device")) // _STR: Description String Name > (CRS, ResourceTemplate () { Memory32Fixed (ReadOnly, > 0x, // Address Base > 0x1000, // Address Length > _Y00) I'm not sure what is that? > Memory32Fixed (ReadOnly, > 0xFED7, // Address Base > 0x1000, // Address Length > ) But 0xFED7 should point to CCA Tomas Thanks
RE: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
> > I've tested is completley disjoint from the region pointed by the ACPI device > tree (SSDT in PTT's case). > > Here's example device from my x250: > > Device (TPM) > { > Name (_HID, "MSFT0101" /* TPM 2.0 Security Device */) // _HID: Hardware ID > Name (_STR, Unicode ("TPM 2.0 Device")) // _STR: Description String Name > (CRS, ResourceTemplate () { Memory32Fixed (ReadOnly, > 0x, // Address Base > 0x1000, // Address Length > _Y00) I'm not sure what is that? > Memory32Fixed (ReadOnly, > 0xFED7, // Address Base > 0x1000, // Address Length > ) But 0xFED7 should point to CCA Tomas Thanks
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > In order to provide access to locality registers, this commits adds mapping > > of > > the head of the CRB registers, which are located right before the control > > area. > > > > Signed-off-by: Jarkko Sakkinen> > --- > > drivers/char/tpm/tpm_crb.c | 89 + > > - > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > > 717b6b4..80b9759 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -52,18 +52,28 @@ enum crb_cancel { > > CRB_CANCEL_INVOKE = BIT(0), > > }; > > > > -struct crb_control_area { > > - u32 req; > > - u32 sts; > > - u32 cancel; > > - u32 start; > > - u32 int_enable; > > - u32 int_sts; > > - u32 cmd_size; > > - u32 cmd_pa_low; > > - u32 cmd_pa_high; > > - u32 rsp_size; > > - u64 rsp_pa; > > +struct crb_regs_head { > > + u32 loc_state; > > + u32 reserved1; > > + u32 loc_ctrl; > > + u32 loc_sts; > > + u8 reserved2[32]; > > + u64 intf_id; > > + u64 ctrl_ext; > > +} __packed; > > + > > > +struct crb_regs_tail { > Why to change the name this is still control_area > > + u32 ctrl_req; > > + u32 ctrl_sts; > > + u32 ctrl_cancel; > > + u32 ctrl_start; > > + u32 ctrl_int_enable; > > + u32 ctrl_int_sts; > > + u32 ctrl_cmd_size; > > + u32 ctrl_cmd_pa_low; > > + u32 ctrl_cmd_pa_high; > > + u32 ctrl_rsp_size; > > + u64 ctrl_rsp_pa; > > } __packed; > > > > enum crb_status { > > @@ -78,7 +88,8 @@ enum crb_flags { > > struct crb_priv { > > unsigned int flags; > > void __iomem *iobase; > > - struct crb_control_area __iomem *cca; > > + struct crb_regs_head __iomem *regs_h; > > + struct crb_regs_tail __iomem *regs_t; > > Same here, why to change the name, let's keep it cca it will reduce the size > of patch and make it more back portable. > > > u8 __iomem *cmd; > > u8 __iomem *rsp; > > u32 cmd_size; > > @@ -104,7 +115,7 @@ static int __maybe_unused crb_go_idle(struct device > > *dev, struct crb_priv *priv) > > if (priv->flags & CRB_FL_ACPI_START) > > return 0; > > > > - iowrite32(CRB_CTRL_REQ_GO_IDLE, >cca->req); > > + iowrite32(CRB_CTRL_REQ_GO_IDLE, >regs_t->ctrl_req); > > /* we don't really care when this settles */ > > > > return 0; > > @@ -128,21 +139,23 @@ static int __maybe_unused crb_cmd_ready(struct > > device *dev, > > struct crb_priv *priv) > > { > > ktime_t stop, start; > > + u32 req; > > > > if (priv->flags & CRB_FL_ACPI_START) > > return 0; > > > > - iowrite32(CRB_CTRL_REQ_CMD_READY, >cca->req); > > + iowrite32(CRB_CTRL_REQ_CMD_READY, >regs_t->ctrl_req); > > > > start = ktime_get(); > > stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > > do { > > - if (!(ioread32(>cca->req) & > > CRB_CTRL_REQ_CMD_READY)) > > + req = ioread32(>regs_t->ctrl_req); > > + if (!(req & CRB_CTRL_REQ_CMD_READY)) > > return 0; > > usleep_range(50, 100); > > } while (ktime_before(ktime_get(), stop)); > > > > - if (ioread32(>cca->req) & CRB_CTRL_REQ_CMD_READY) { > > + if (ioread32(>regs_t->ctrl_req) & CRB_CTRL_REQ_CMD_READY) > > { > > dev_warn(dev, "cmdReady timed out\n"); > > return -ETIME; > > } > > @@ -155,7 +168,7 @@ static u8 crb_status(struct tpm_chip *chip) > > struct crb_priv *priv = dev_get_drvdata(>dev); > > u8 sts = 0; > > > > - if ((ioread32(>cca->start) & CRB_START_INVOKE) != > > + if ((ioread32(>regs_t->ctrl_start) & CRB_START_INVOKE) != > > CRB_START_INVOKE) > > sts |= CRB_DRV_STS_COMPLETE; > > > > @@ -171,7 +184,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, > > size_t count) > > if (count < 6) > > return -EIO; > > > > - if (ioread32(>cca->sts) & CRB_CTRL_STS_ERROR) > > + if (ioread32(>regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR) > > return -EIO; > > > > memcpy_fromio(buf, priv->rsp, 6); > > @@ -210,7 +223,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > > size_t len) > > /* Zero the cancel register so that the next command will not get > > * canceled. > > */ > > - iowrite32(0, >cca->cancel); > > + iowrite32(0, >regs_t->ctrl_cancel); > > > > if (len > priv->cmd_size) { > > dev_err(>dev, "invalid command count value %zd %d\n", > > @@ -224,7 +237,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > > size_t len) > > wmb(); > > > > if (priv->flags & CRB_FL_CRB_START) > > - iowrite32(CRB_START_INVOKE, >cca->start); > > + iowrite32(CRB_START_INVOKE, >regs_t->ctrl_start); > > > > if (priv->flags & CRB_FL_ACPI_START) > > rc = crb_do_acpi_start(chip); > > @@ -236,7
Re: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
On Mon, Dec 12, 2016 at 02:25:32PM +, Winkler, Tomas wrote: > > > > In order to provide access to locality registers, this commits adds mapping > > of > > the head of the CRB registers, which are located right before the control > > area. > > > > Signed-off-by: Jarkko Sakkinen > > --- > > drivers/char/tpm/tpm_crb.c | 89 + > > - > > 1 file changed, 57 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > > 717b6b4..80b9759 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -52,18 +52,28 @@ enum crb_cancel { > > CRB_CANCEL_INVOKE = BIT(0), > > }; > > > > -struct crb_control_area { > > - u32 req; > > - u32 sts; > > - u32 cancel; > > - u32 start; > > - u32 int_enable; > > - u32 int_sts; > > - u32 cmd_size; > > - u32 cmd_pa_low; > > - u32 cmd_pa_high; > > - u32 rsp_size; > > - u64 rsp_pa; > > +struct crb_regs_head { > > + u32 loc_state; > > + u32 reserved1; > > + u32 loc_ctrl; > > + u32 loc_sts; > > + u8 reserved2[32]; > > + u64 intf_id; > > + u64 ctrl_ext; > > +} __packed; > > + > > > +struct crb_regs_tail { > Why to change the name this is still control_area > > + u32 ctrl_req; > > + u32 ctrl_sts; > > + u32 ctrl_cancel; > > + u32 ctrl_start; > > + u32 ctrl_int_enable; > > + u32 ctrl_int_sts; > > + u32 ctrl_cmd_size; > > + u32 ctrl_cmd_pa_low; > > + u32 ctrl_cmd_pa_high; > > + u32 ctrl_rsp_size; > > + u64 ctrl_rsp_pa; > > } __packed; > > > > enum crb_status { > > @@ -78,7 +88,8 @@ enum crb_flags { > > struct crb_priv { > > unsigned int flags; > > void __iomem *iobase; > > - struct crb_control_area __iomem *cca; > > + struct crb_regs_head __iomem *regs_h; > > + struct crb_regs_tail __iomem *regs_t; > > Same here, why to change the name, let's keep it cca it will reduce the size > of patch and make it more back portable. > > > u8 __iomem *cmd; > > u8 __iomem *rsp; > > u32 cmd_size; > > @@ -104,7 +115,7 @@ static int __maybe_unused crb_go_idle(struct device > > *dev, struct crb_priv *priv) > > if (priv->flags & CRB_FL_ACPI_START) > > return 0; > > > > - iowrite32(CRB_CTRL_REQ_GO_IDLE, >cca->req); > > + iowrite32(CRB_CTRL_REQ_GO_IDLE, >regs_t->ctrl_req); > > /* we don't really care when this settles */ > > > > return 0; > > @@ -128,21 +139,23 @@ static int __maybe_unused crb_cmd_ready(struct > > device *dev, > > struct crb_priv *priv) > > { > > ktime_t stop, start; > > + u32 req; > > > > if (priv->flags & CRB_FL_ACPI_START) > > return 0; > > > > - iowrite32(CRB_CTRL_REQ_CMD_READY, >cca->req); > > + iowrite32(CRB_CTRL_REQ_CMD_READY, >regs_t->ctrl_req); > > > > start = ktime_get(); > > stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > > do { > > - if (!(ioread32(>cca->req) & > > CRB_CTRL_REQ_CMD_READY)) > > + req = ioread32(>regs_t->ctrl_req); > > + if (!(req & CRB_CTRL_REQ_CMD_READY)) > > return 0; > > usleep_range(50, 100); > > } while (ktime_before(ktime_get(), stop)); > > > > - if (ioread32(>cca->req) & CRB_CTRL_REQ_CMD_READY) { > > + if (ioread32(>regs_t->ctrl_req) & CRB_CTRL_REQ_CMD_READY) > > { > > dev_warn(dev, "cmdReady timed out\n"); > > return -ETIME; > > } > > @@ -155,7 +168,7 @@ static u8 crb_status(struct tpm_chip *chip) > > struct crb_priv *priv = dev_get_drvdata(>dev); > > u8 sts = 0; > > > > - if ((ioread32(>cca->start) & CRB_START_INVOKE) != > > + if ((ioread32(>regs_t->ctrl_start) & CRB_START_INVOKE) != > > CRB_START_INVOKE) > > sts |= CRB_DRV_STS_COMPLETE; > > > > @@ -171,7 +184,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, > > size_t count) > > if (count < 6) > > return -EIO; > > > > - if (ioread32(>cca->sts) & CRB_CTRL_STS_ERROR) > > + if (ioread32(>regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR) > > return -EIO; > > > > memcpy_fromio(buf, priv->rsp, 6); > > @@ -210,7 +223,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > > size_t len) > > /* Zero the cancel register so that the next command will not get > > * canceled. > > */ > > - iowrite32(0, >cca->cancel); > > + iowrite32(0, >regs_t->ctrl_cancel); > > > > if (len > priv->cmd_size) { > > dev_err(>dev, "invalid command count value %zd %d\n", > > @@ -224,7 +237,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > > size_t len) > > wmb(); > > > > if (priv->flags & CRB_FL_CRB_START) > > - iowrite32(CRB_START_INVOKE, >cca->start); > > + iowrite32(CRB_START_INVOKE, >regs_t->ctrl_start); > > > > if (priv->flags & CRB_FL_ACPI_START) > > rc = crb_do_acpi_start(chip); > > @@ -236,7 +249,7 @@ static void
RE: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
> > In order to provide access to locality registers, this commits adds mapping of > the head of the CRB registers, which are located right before the control > area. > > Signed-off-by: Jarkko Sakkinen> --- > drivers/char/tpm/tpm_crb.c | 89 + > - > 1 file changed, 57 insertions(+), 32 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > 717b6b4..80b9759 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -52,18 +52,28 @@ enum crb_cancel { > CRB_CANCEL_INVOKE = BIT(0), > }; > > -struct crb_control_area { > - u32 req; > - u32 sts; > - u32 cancel; > - u32 start; > - u32 int_enable; > - u32 int_sts; > - u32 cmd_size; > - u32 cmd_pa_low; > - u32 cmd_pa_high; > - u32 rsp_size; > - u64 rsp_pa; > +struct crb_regs_head { > + u32 loc_state; > + u32 reserved1; > + u32 loc_ctrl; > + u32 loc_sts; > + u8 reserved2[32]; > + u64 intf_id; > + u64 ctrl_ext; > +} __packed; > + > +struct crb_regs_tail { Why to change the name this is still control_area > + u32 ctrl_req; > + u32 ctrl_sts; > + u32 ctrl_cancel; > + u32 ctrl_start; > + u32 ctrl_int_enable; > + u32 ctrl_int_sts; > + u32 ctrl_cmd_size; > + u32 ctrl_cmd_pa_low; > + u32 ctrl_cmd_pa_high; > + u32 ctrl_rsp_size; > + u64 ctrl_rsp_pa; > } __packed; > > enum crb_status { > @@ -78,7 +88,8 @@ enum crb_flags { > struct crb_priv { > unsigned int flags; > void __iomem *iobase; > - struct crb_control_area __iomem *cca; > + struct crb_regs_head __iomem *regs_h; > + struct crb_regs_tail __iomem *regs_t; Same here, why to change the name, let's keep it cca it will reduce the size of patch and make it more back portable. > u8 __iomem *cmd; > u8 __iomem *rsp; > u32 cmd_size; > @@ -104,7 +115,7 @@ static int __maybe_unused crb_go_idle(struct device > *dev, struct crb_priv *priv) > if (priv->flags & CRB_FL_ACPI_START) > return 0; > > - iowrite32(CRB_CTRL_REQ_GO_IDLE, >cca->req); > + iowrite32(CRB_CTRL_REQ_GO_IDLE, >regs_t->ctrl_req); > /* we don't really care when this settles */ > > return 0; > @@ -128,21 +139,23 @@ static int __maybe_unused crb_cmd_ready(struct > device *dev, > struct crb_priv *priv) > { > ktime_t stop, start; > + u32 req; > > if (priv->flags & CRB_FL_ACPI_START) > return 0; > > - iowrite32(CRB_CTRL_REQ_CMD_READY, >cca->req); > + iowrite32(CRB_CTRL_REQ_CMD_READY, >regs_t->ctrl_req); > > start = ktime_get(); > stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > do { > - if (!(ioread32(>cca->req) & > CRB_CTRL_REQ_CMD_READY)) > + req = ioread32(>regs_t->ctrl_req); > + if (!(req & CRB_CTRL_REQ_CMD_READY)) > return 0; > usleep_range(50, 100); > } while (ktime_before(ktime_get(), stop)); > > - if (ioread32(>cca->req) & CRB_CTRL_REQ_CMD_READY) { > + if (ioread32(>regs_t->ctrl_req) & CRB_CTRL_REQ_CMD_READY) > { > dev_warn(dev, "cmdReady timed out\n"); > return -ETIME; > } > @@ -155,7 +168,7 @@ static u8 crb_status(struct tpm_chip *chip) > struct crb_priv *priv = dev_get_drvdata(>dev); > u8 sts = 0; > > - if ((ioread32(>cca->start) & CRB_START_INVOKE) != > + if ((ioread32(>regs_t->ctrl_start) & CRB_START_INVOKE) != > CRB_START_INVOKE) > sts |= CRB_DRV_STS_COMPLETE; > > @@ -171,7 +184,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, > size_t count) > if (count < 6) > return -EIO; > > - if (ioread32(>cca->sts) & CRB_CTRL_STS_ERROR) > + if (ioread32(>regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR) > return -EIO; > > memcpy_fromio(buf, priv->rsp, 6); > @@ -210,7 +223,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > size_t len) > /* Zero the cancel register so that the next command will not get >* canceled. >*/ > - iowrite32(0, >cca->cancel); > + iowrite32(0, >regs_t->ctrl_cancel); > > if (len > priv->cmd_size) { > dev_err(>dev, "invalid command count value %zd %d\n", > @@ -224,7 +237,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > size_t len) > wmb(); > > if (priv->flags & CRB_FL_CRB_START) > - iowrite32(CRB_START_INVOKE, >cca->start); > + iowrite32(CRB_START_INVOKE, >regs_t->ctrl_start); > > if (priv->flags & CRB_FL_ACPI_START) > rc = crb_do_acpi_start(chip); > @@ -236,7 +249,7 @@ static void crb_cancel(struct tpm_chip *chip) { > struct crb_priv *priv = dev_get_drvdata(>dev); > > - iowrite32(CRB_CANCEL_INVOKE, >cca->cancel); > +
RE: [tpmdd-devel] [PATCH v3 1/3] tpm_crb: map locality registers
> > In order to provide access to locality registers, this commits adds mapping of > the head of the CRB registers, which are located right before the control > area. > > Signed-off-by: Jarkko Sakkinen > --- > drivers/char/tpm/tpm_crb.c | 89 + > - > 1 file changed, 57 insertions(+), 32 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > 717b6b4..80b9759 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -52,18 +52,28 @@ enum crb_cancel { > CRB_CANCEL_INVOKE = BIT(0), > }; > > -struct crb_control_area { > - u32 req; > - u32 sts; > - u32 cancel; > - u32 start; > - u32 int_enable; > - u32 int_sts; > - u32 cmd_size; > - u32 cmd_pa_low; > - u32 cmd_pa_high; > - u32 rsp_size; > - u64 rsp_pa; > +struct crb_regs_head { > + u32 loc_state; > + u32 reserved1; > + u32 loc_ctrl; > + u32 loc_sts; > + u8 reserved2[32]; > + u64 intf_id; > + u64 ctrl_ext; > +} __packed; > + > +struct crb_regs_tail { Why to change the name this is still control_area > + u32 ctrl_req; > + u32 ctrl_sts; > + u32 ctrl_cancel; > + u32 ctrl_start; > + u32 ctrl_int_enable; > + u32 ctrl_int_sts; > + u32 ctrl_cmd_size; > + u32 ctrl_cmd_pa_low; > + u32 ctrl_cmd_pa_high; > + u32 ctrl_rsp_size; > + u64 ctrl_rsp_pa; > } __packed; > > enum crb_status { > @@ -78,7 +88,8 @@ enum crb_flags { > struct crb_priv { > unsigned int flags; > void __iomem *iobase; > - struct crb_control_area __iomem *cca; > + struct crb_regs_head __iomem *regs_h; > + struct crb_regs_tail __iomem *regs_t; Same here, why to change the name, let's keep it cca it will reduce the size of patch and make it more back portable. > u8 __iomem *cmd; > u8 __iomem *rsp; > u32 cmd_size; > @@ -104,7 +115,7 @@ static int __maybe_unused crb_go_idle(struct device > *dev, struct crb_priv *priv) > if (priv->flags & CRB_FL_ACPI_START) > return 0; > > - iowrite32(CRB_CTRL_REQ_GO_IDLE, >cca->req); > + iowrite32(CRB_CTRL_REQ_GO_IDLE, >regs_t->ctrl_req); > /* we don't really care when this settles */ > > return 0; > @@ -128,21 +139,23 @@ static int __maybe_unused crb_cmd_ready(struct > device *dev, > struct crb_priv *priv) > { > ktime_t stop, start; > + u32 req; > > if (priv->flags & CRB_FL_ACPI_START) > return 0; > > - iowrite32(CRB_CTRL_REQ_CMD_READY, >cca->req); > + iowrite32(CRB_CTRL_REQ_CMD_READY, >regs_t->ctrl_req); > > start = ktime_get(); > stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > do { > - if (!(ioread32(>cca->req) & > CRB_CTRL_REQ_CMD_READY)) > + req = ioread32(>regs_t->ctrl_req); > + if (!(req & CRB_CTRL_REQ_CMD_READY)) > return 0; > usleep_range(50, 100); > } while (ktime_before(ktime_get(), stop)); > > - if (ioread32(>cca->req) & CRB_CTRL_REQ_CMD_READY) { > + if (ioread32(>regs_t->ctrl_req) & CRB_CTRL_REQ_CMD_READY) > { > dev_warn(dev, "cmdReady timed out\n"); > return -ETIME; > } > @@ -155,7 +168,7 @@ static u8 crb_status(struct tpm_chip *chip) > struct crb_priv *priv = dev_get_drvdata(>dev); > u8 sts = 0; > > - if ((ioread32(>cca->start) & CRB_START_INVOKE) != > + if ((ioread32(>regs_t->ctrl_start) & CRB_START_INVOKE) != > CRB_START_INVOKE) > sts |= CRB_DRV_STS_COMPLETE; > > @@ -171,7 +184,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, > size_t count) > if (count < 6) > return -EIO; > > - if (ioread32(>cca->sts) & CRB_CTRL_STS_ERROR) > + if (ioread32(>regs_t->ctrl_sts) & CRB_CTRL_STS_ERROR) > return -EIO; > > memcpy_fromio(buf, priv->rsp, 6); > @@ -210,7 +223,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > size_t len) > /* Zero the cancel register so that the next command will not get >* canceled. >*/ > - iowrite32(0, >cca->cancel); > + iowrite32(0, >regs_t->ctrl_cancel); > > if (len > priv->cmd_size) { > dev_err(>dev, "invalid command count value %zd %d\n", > @@ -224,7 +237,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, > size_t len) > wmb(); > > if (priv->flags & CRB_FL_CRB_START) > - iowrite32(CRB_START_INVOKE, >cca->start); > + iowrite32(CRB_START_INVOKE, >regs_t->ctrl_start); > > if (priv->flags & CRB_FL_ACPI_START) > rc = crb_do_acpi_start(chip); > @@ -236,7 +249,7 @@ static void crb_cancel(struct tpm_chip *chip) { > struct crb_priv *priv = dev_get_drvdata(>dev); > > - iowrite32(CRB_CANCEL_INVOKE, >cca->cancel); > + iowrite32(CRB_CANCEL_INVOKE, >regs_t->ctrl_cancel);