Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 21, 2016 at 11:10:47AM -0700, Andrey Pronin wrote: > On Wed, Jul 20, 2016 at 11:03:36AM -0600, Jason Gunthorpe wrote: > > On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote: > > > > > The only two things that bother me with such approach are > > > (1) whatever names I pick for the new set of functions, they > > > will be similar to and thus might be confused with the > > > original tpm_tis_read/writeXX; > > > > tpm_tis_helper_read16 ? > > > > > (2) these functions are phy-specific, so possibly it's better > > > to create tpm_tis_spi.h and put them there with proper > > > name prefixes. And then use in tpm_tis_spi and cr50_spi. > > > > No, they are generic to any tis phy that implements read only through > > read_bytes. > > > > (Honestly, I'm not sure we made the best choice here having phy > > functions for all the versions, we are not that performance > > sensitive, just getting rid of everything but read_bytes from the > > phy_ops would probably also be a reasonable thing to do.) > > > > One thing we can do is re-implement functions tpm_tis_read/writeXX > to use phy-specific implementations of read16, read32, write32 if they > are provided. But if those function pointers are left NULL in phy_ops, > fallback to using read/write_bytes and byte-swapping. I was thinking of just getting rid of phy_ops->read16 entirely and only use read_bytes at the ops layer. Jason
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 21, 2016 at 11:10:47AM -0700, Andrey Pronin wrote: > On Wed, Jul 20, 2016 at 11:03:36AM -0600, Jason Gunthorpe wrote: > > On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote: > > > > > The only two things that bother me with such approach are > > > (1) whatever names I pick for the new set of functions, they > > > will be similar to and thus might be confused with the > > > original tpm_tis_read/writeXX; > > > > tpm_tis_helper_read16 ? > > > > > (2) these functions are phy-specific, so possibly it's better > > > to create tpm_tis_spi.h and put them there with proper > > > name prefixes. And then use in tpm_tis_spi and cr50_spi. > > > > No, they are generic to any tis phy that implements read only through > > read_bytes. > > > > (Honestly, I'm not sure we made the best choice here having phy > > functions for all the versions, we are not that performance > > sensitive, just getting rid of everything but read_bytes from the > > phy_ops would probably also be a reasonable thing to do.) > > > > One thing we can do is re-implement functions tpm_tis_read/writeXX > to use phy-specific implementations of read16, read32, write32 if they > are provided. But if those function pointers are left NULL in phy_ops, > fallback to using read/write_bytes and byte-swapping. I was thinking of just getting rid of phy_ops->read16 entirely and only use read_bytes at the ops layer. Jason
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Wed, Jul 20, 2016 at 11:03:36AM -0600, Jason Gunthorpe wrote:
> On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote:
>
> > The only two things that bother me with such approach are
> > (1) whatever names I pick for the new set of functions, they
> > will be similar to and thus might be confused with the
> > original tpm_tis_read/writeXX;
>
> tpm_tis_helper_read16 ?
>
> > (2) these functions are phy-specific, so possibly it's better
> > to create tpm_tis_spi.h and put them there with proper
> > name prefixes. And then use in tpm_tis_spi and cr50_spi.
>
> No, they are generic to any tis phy that implements read only through
> read_bytes.
>
> (Honestly, I'm not sure we made the best choice here having phy
> functions for all the versions, we are not that performance
> sensitive, just getting rid of everything but read_bytes from the
> phy_ops would probably also be a reasonable thing to do.)
>
One thing we can do is re-implement functions tpm_tis_read/writeXX
to use phy-specific implementations of read16, read32, write32 if they
are provided. But if those function pointers are left NULL in phy_ops,
fallback to using read/write_bytes and byte-swapping.
I.e., instead of:
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
return data->phy_ops->read16(data, addr, result);
}
do the following:
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
int rc;
if (data->phy_ops->read16)
return data->phy_ops->read16(data, addr, result);
rc = data->phy_ops->read_bytes(data, addr,
sizeof(u16), (u8 *)result);
if (!rc)
*result = le16_to_cpu(*result);
return rc;
}
If you like the idea, I'll submit it as a separate patch.
Andrey
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Wed, Jul 20, 2016 at 11:03:36AM -0600, Jason Gunthorpe wrote:
> On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote:
>
> > The only two things that bother me with such approach are
> > (1) whatever names I pick for the new set of functions, they
> > will be similar to and thus might be confused with the
> > original tpm_tis_read/writeXX;
>
> tpm_tis_helper_read16 ?
>
> > (2) these functions are phy-specific, so possibly it's better
> > to create tpm_tis_spi.h and put them there with proper
> > name prefixes. And then use in tpm_tis_spi and cr50_spi.
>
> No, they are generic to any tis phy that implements read only through
> read_bytes.
>
> (Honestly, I'm not sure we made the best choice here having phy
> functions for all the versions, we are not that performance
> sensitive, just getting rid of everything but read_bytes from the
> phy_ops would probably also be a reasonable thing to do.)
>
One thing we can do is re-implement functions tpm_tis_read/writeXX
to use phy-specific implementations of read16, read32, write32 if they
are provided. But if those function pointers are left NULL in phy_ops,
fallback to using read/write_bytes and byte-swapping.
I.e., instead of:
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
return data->phy_ops->read16(data, addr, result);
}
do the following:
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
int rc;
if (data->phy_ops->read16)
return data->phy_ops->read16(data, addr, result);
rc = data->phy_ops->read_bytes(data, addr,
sizeof(u16), (u8 *)result);
if (!rc)
*result = le16_to_cpu(*result);
return rc;
}
If you like the idea, I'll submit it as a separate patch.
Andrey
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote: > The only two things that bother me with such approach are > (1) whatever names I pick for the new set of functions, they > will be similar to and thus might be confused with the > original tpm_tis_read/writeXX; tpm_tis_helper_read16 ? > (2) these functions are phy-specific, so possibly it's better > to create tpm_tis_spi.h and put them there with proper > name prefixes. And then use in tpm_tis_spi and cr50_spi. No, they are generic to any tis phy that implements read only through read_bytes. (Honestly, I'm not sure we made the best choice here having phy functions for all the versions, we are not that performance sensitive, just getting rid of everything but read_bytes from the phy_ops would probably also be a reasonable thing to do.) Jason
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Tue, Jul 19, 2016 at 05:24:11PM -0700, Andrey Pronin wrote: > The only two things that bother me with such approach are > (1) whatever names I pick for the new set of functions, they > will be similar to and thus might be confused with the > original tpm_tis_read/writeXX; tpm_tis_helper_read16 ? > (2) these functions are phy-specific, so possibly it's better > to create tpm_tis_spi.h and put them there with proper > name prefixes. And then use in tpm_tis_spi and cr50_spi. No, they are generic to any tis phy that implements read only through read_bytes. (Honestly, I'm not sure we made the best choice here having phy functions for all the versions, we are not that performance sensitive, just getting rid of everything but read_bytes from the phy_ops would probably also be a reasonable thing to do.) Jason
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Tue, Jul 19, 2016 at 03:55:27PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 14, 2016 at 08:44:44PM -0700, Andrey Pronin wrote:
> > On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> > > On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> > >
> > > > +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16
> > > > *result)
> > > > +{
> > > > + int rc;
> > > > +
> > > > + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8
> > > > *)result);
> > > > + if (!rc)
> > > > + *result = le16_to_cpu(*result);
> > > > + return rc;
> > > > +}
> > >
> > > I thought we had core support for this pattern?
> > >
> > > Christophe ?
> > >
> > > Please change this so this code isn't duplicated.
> > >
> > > Jason
> > >
> > Hmm, didn't see the support. Would be great if there is.
> > The pattern itself is copied from tpm_tis_spi as is.
> > read_bytes/write_bytes were de-dup'ed as they used a lot of common code
> > (even more for this driver than for tpm_tis_spi).
> > But as for _readNN/_writeNN, there're only three of these functions,
> > so it din't seem too bad.
>
> If there isn't, please add a separate commit before this that adds an
> inline function to tpm_tis_core.h.
>
tpm_tis_core.h currently has access functions defined like:
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
return data->phy_ops->read16(data, addr, result);
}
And it's up to the drivers implementing phy_ops to do (or not)
byte-swapping as necessary before/after IO ops. For example,
tpm_tis.c in its phy_ops->read16 simply does ioread16(), while
tpm_tis_spi.c (and cr50_spi.c) does phy_ops->read_bytes()
followed by le16_to_cpu().
Still, I can create add'l inline functions:
static inline int tpm_tis_read_le16(struct tpm_tis_data *data,
u32 addr, u16 *result)
{
int rc;
rc = data->phy_ops->read_bytes(data, addr,
sizeof(u16), (u8 *)result);
if (!rc)
*result = le16_to_cpu(*result);
return rc;
}
and re-use them both in cr50_spi.c and tpm_tis_spi.c
The only two things that bother me with such approach are
(1) whatever names I pick for the new set of functions, they
will be similar to and thus might be confused with the
original tpm_tis_read/writeXX;
(2) these functions are phy-specific, so possibly it's better
to create tpm_tis_spi.h and put them there with proper
name prefixes. And then use in tpm_tis_spi and cr50_spi.
Any preferences on what I should better do?
Andrey
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Tue, Jul 19, 2016 at 03:55:27PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 14, 2016 at 08:44:44PM -0700, Andrey Pronin wrote:
> > On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> > > On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> > >
> > > > +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16
> > > > *result)
> > > > +{
> > > > + int rc;
> > > > +
> > > > + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8
> > > > *)result);
> > > > + if (!rc)
> > > > + *result = le16_to_cpu(*result);
> > > > + return rc;
> > > > +}
> > >
> > > I thought we had core support for this pattern?
> > >
> > > Christophe ?
> > >
> > > Please change this so this code isn't duplicated.
> > >
> > > Jason
> > >
> > Hmm, didn't see the support. Would be great if there is.
> > The pattern itself is copied from tpm_tis_spi as is.
> > read_bytes/write_bytes were de-dup'ed as they used a lot of common code
> > (even more for this driver than for tpm_tis_spi).
> > But as for _readNN/_writeNN, there're only three of these functions,
> > so it din't seem too bad.
>
> If there isn't, please add a separate commit before this that adds an
> inline function to tpm_tis_core.h.
>
tpm_tis_core.h currently has access functions defined like:
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
return data->phy_ops->read16(data, addr, result);
}
And it's up to the drivers implementing phy_ops to do (or not)
byte-swapping as necessary before/after IO ops. For example,
tpm_tis.c in its phy_ops->read16 simply does ioread16(), while
tpm_tis_spi.c (and cr50_spi.c) does phy_ops->read_bytes()
followed by le16_to_cpu().
Still, I can create add'l inline functions:
static inline int tpm_tis_read_le16(struct tpm_tis_data *data,
u32 addr, u16 *result)
{
int rc;
rc = data->phy_ops->read_bytes(data, addr,
sizeof(u16), (u8 *)result);
if (!rc)
*result = le16_to_cpu(*result);
return rc;
}
and re-use them both in cr50_spi.c and tpm_tis_spi.c
The only two things that bother me with such approach are
(1) whatever names I pick for the new set of functions, they
will be similar to and thus might be confused with the
original tpm_tis_read/writeXX;
(2) these functions are phy-specific, so possibly it's better
to create tpm_tis_spi.h and put them there with proper
name prefixes. And then use in tpm_tis_spi and cr50_spi.
Any preferences on what I should better do?
Andrey
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 14, 2016 at 08:44:44PM -0700, Andrey Pronin wrote:
> On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> > On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> >
> > > +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16
> > > *result)
> > > +{
> > > + int rc;
> > > +
> > > + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
> > > + if (!rc)
> > > + *result = le16_to_cpu(*result);
> > > + return rc;
> > > +}
> >
> > I thought we had core support for this pattern?
> >
> > Christophe ?
> >
> > Please change this so this code isn't duplicated.
> >
> > Jason
> >
> Hmm, didn't see the support. Would be great if there is.
> The pattern itself is copied from tpm_tis_spi as is.
> read_bytes/write_bytes were de-dup'ed as they used a lot of common code
> (even more for this driver than for tpm_tis_spi).
> But as for _readNN/_writeNN, there're only three of these functions,
> so it din't seem too bad.
If there isn't, please add a separate commit before this that adds an
inline function to tpm_tis_core.h.
/Jarkko
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 14, 2016 at 08:44:44PM -0700, Andrey Pronin wrote:
> On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> > On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> >
> > > +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16
> > > *result)
> > > +{
> > > + int rc;
> > > +
> > > + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
> > > + if (!rc)
> > > + *result = le16_to_cpu(*result);
> > > + return rc;
> > > +}
> >
> > I thought we had core support for this pattern?
> >
> > Christophe ?
> >
> > Please change this so this code isn't duplicated.
> >
> > Jason
> >
> Hmm, didn't see the support. Would be great if there is.
> The pattern itself is copied from tpm_tis_spi as is.
> read_bytes/write_bytes were de-dup'ed as they used a lot of common code
> (even more for this driver than for tpm_tis_spi).
> But as for _readNN/_writeNN, there're only three of these functions,
> so it din't seem too bad.
If there isn't, please add a separate commit before this that adds an
inline function to tpm_tis_core.h.
/Jarkko
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
>
> > +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16
> > *result)
> > +{
> > + int rc;
> > +
> > + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
> > + if (!rc)
> > + *result = le16_to_cpu(*result);
> > + return rc;
> > +}
>
> I thought we had core support for this pattern?
>
> Christophe ?
>
> Please change this so this code isn't duplicated.
>
> Jason
>
Hmm, didn't see the support. Would be great if there is.
The pattern itself is copied from tpm_tis_spi as is.
read_bytes/write_bytes were de-dup'ed as they used a lot of common code
(even more for this driver than for tpm_tis_spi).
But as for _readNN/_writeNN, there're only three of these functions,
so it din't seem too bad.
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 14, 2016 at 09:32:36PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
>
> > +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16
> > *result)
> > +{
> > + int rc;
> > +
> > + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
> > + if (!rc)
> > + *result = le16_to_cpu(*result);
> > + return rc;
> > +}
>
> I thought we had core support for this pattern?
>
> Christophe ?
>
> Please change this so this code isn't duplicated.
>
> Jason
>
Hmm, didn't see the support. Would be great if there is.
The pattern itself is copied from tpm_tis_spi as is.
read_bytes/write_bytes were de-dup'ed as they used a lot of common code
(even more for this driver than for tpm_tis_spi).
But as for _readNN/_writeNN, there're only three of these functions,
so it din't seem too bad.
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
> +{
> + int rc;
> +
> + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
> + if (!rc)
> + *result = le16_to_cpu(*result);
> + return rc;
> +}
I thought we had core support for this pattern?
Christophe ?
Please change this so this code isn't duplicated.
Jason
Re: [PATCH 2/2] tpm: add driver for cr50 on SPI
On Thu, Jul 14, 2016 at 07:20:18PM -0700, Andrey Pronin wrote:
> +static int cr50_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
> +{
> + int rc;
> +
> + rc = data->phy_ops->read_bytes(data, addr, sizeof(u16), (u8 *)result);
> + if (!rc)
> + *result = le16_to_cpu(*result);
> + return rc;
> +}
I thought we had core support for this pattern?
Christophe ?
Please change this so this code isn't duplicated.
Jason
