diff mbox series

[RFC,v1,1/2] ufs: core: Advanced RPMB detection

Message ID 20221107131038.201724-2-beanhuo@iokpp.de
State New
Headers
Series UFS Advanced RPMB |

Commit Message

Bean Huo Nov. 7, 2022, 1:10 p.m. UTC 
  From: Bean Huo <beanhuo@micron.com>

Check UFS Advanced RPMB LU enablement during ufshcd_lu_init().

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c | 4 ++++
 include/ufs/ufs.h         | 3 +++
 2 files changed, 7 insertions(+)

Comments

Avri Altman Nov. 8, 2022, 1:40 p.m. UTC | #1 
> From: Bean Huo <beanhuo@micron.com>
> 
> Check UFS Advanced RPMB LU enablement during ufshcd_lu_init().
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/ufs/core/ufshcd.c | 4 ++++
>  include/ufs/ufs.h         | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> ee73d7036133..d49e7a0b82ca 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4940,6 +4940,10 @@ static void ufshcd_lu_init(struct ufs_hba *hba,
> struct scsi_device *sdev)
>             desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] ==
> UFS_LU_POWER_ON_WP)
>                 hba->dev_info.is_lu_power_on_wp = true;
> 
> +       if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_RPMB_UNIT &&
Please remind me why do we need both UFS_RPMB_UNIT and UFS_UPIU_RPMB_WLUN ?

> +           desc_buf[UNIT_DESC_PARAM_RPMB_REGION_EN] & 1 << 4)
(1 << 4) or BIT(4) ?

> +                       hba->dev_info.b_advanced_rpmb_en = true;
> +
>         kfree(desc_buf);
>  set_qdepth:
>         /*
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index
> 1bba3fead2ce..2e617ab87750 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -199,6 +199,7 @@ enum unit_desc_param {
>         UNIT_DESC_PARAM_PSA_SENSITIVE           = 0x7,
>         UNIT_DESC_PARAM_MEM_TYPE                = 0x8,
>         UNIT_DESC_PARAM_DATA_RELIABILITY        = 0x9,
> +       UNIT_DESC_PARAM_RPMB_REGION_EN          = 0x9,
This is awkward.  Better to define it, or - 
Maybe it's time for rpmb to have its own unit descriptor - it surely deserve it.
 
Thanks,
Avri

>         UNIT_DESC_PARAM_LOGICAL_BLK_SIZE        = 0xA,
>         UNIT_DESC_PARAM_LOGICAL_BLK_COUNT       = 0xB,
>         UNIT_DESC_PARAM_ERASE_BLK_SIZE          = 0x13,
> @@ -601,6 +602,8 @@ struct ufs_dev_info {
> 
>         bool    b_rpm_dev_flush_capable;
>         u8      b_presrv_uspc_en;
> +
> +       bool    b_advanced_rpmb_en;
>  };
> 
>  /*
> --
> 2.25.1
Bean Huo Nov. 8, 2022, 4:50 p.m. UTC | #2 
Avri, 

thanks for your review.

On Tue, 2022-11-08 at 13:40 +0000, Avri Altman wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > Check UFS Advanced RPMB LU enablement during ufshcd_lu_init().
> > 
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >  drivers/ufs/core/ufshcd.c | 4 ++++
> >  include/ufs/ufs.h         | 3 +++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index
> > ee73d7036133..d49e7a0b82ca 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -4940,6 +4940,10 @@ static void ufshcd_lu_init(struct ufs_hba
> > *hba,
> > struct scsi_device *sdev)
> >             desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] ==
> > UFS_LU_POWER_ON_WP)
> >                 hba->dev_info.is_lu_power_on_wp = true;
> > 
> > +       if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_RPMB_UNIT
> > &&
> Please remind me why do we need both UFS_RPMB_UNIT and
> UFS_UPIU_RPMB_WLUN ?

I see. they are the same value, we should remove one, will change it in
next version.
> 
> > +           desc_buf[UNIT_DESC_PARAM_RPMB_REGION_EN] & 1 << 4)
> (1 << 4) or BIT(4) ?
> 
> > +                       hba->dev_info.b_advanced_rpmb_en = true;
> > +
> >         kfree(desc_buf);
> >  set_qdepth:
> >         /*
> > diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index
> > 1bba3fead2ce..2e617ab87750 100644
> > --- a/include/ufs/ufs.h
> > +++ b/include/ufs/ufs.h
> > @@ -199,6 +199,7 @@ enum unit_desc_param {
> >         UNIT_DESC_PARAM_PSA_SENSITIVE           = 0x7,
> >         UNIT_DESC_PARAM_MEM_TYPE                = 0x8,
> >         UNIT_DESC_PARAM_DATA_RELIABILITY        = 0x9,
> > +       UNIT_DESC_PARAM_RPMB_REGION_EN          = 0x9,
> This is awkward.  Better to define it, or - 
> Maybe it's time for rpmb to have its own unit descriptor - it surely
> deserve it.
>  

no problem, let me think about it, will add in the next version.




Kind regards,
Bean
Avri Altman Nov. 8, 2022, 9:41 p.m. UTC | #3 
> Avri,
> 
> thanks for your review.
> 
> On Tue, 2022-11-08 at 13:40 +0000, Avri Altman wrote:
> > > From: Bean Huo <beanhuo@micron.com>
> > >
> > > Check UFS Advanced RPMB LU enablement during ufshcd_lu_init().
> > >
> > > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > > ---
> > >  drivers/ufs/core/ufshcd.c | 4 ++++
> > >  include/ufs/ufs.h         | 3 +++
> > >  2 files changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > > index ee73d7036133..d49e7a0b82ca 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -4940,6 +4940,10 @@ static void ufshcd_lu_init(struct ufs_hba
> > > *hba, struct scsi_device *sdev)
> > >             desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] ==
> > > UFS_LU_POWER_ON_WP)
> > >                 hba->dev_info.is_lu_power_on_wp = true;
> > >
> > > +       if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_RPMB_UNIT
> > > &&
> > Please remind me why do we need both UFS_RPMB_UNIT and
> > UFS_UPIU_RPMB_WLUN ?
> 
> I see. they are the same value, we should remove one, will change it in next
> version.
> >
> > > +           desc_buf[UNIT_DESC_PARAM_RPMB_REGION_EN] & 1 << 4)
> > (1 << 4) or BIT(4) ?
Not saying that testing bit 4 of bRPMBRegionEnable is wrong,
Have you considered using bit 10 of dExtendedUFSFeaturesSupport and decided otherwise?

Thanks,
Avri
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ee73d7036133..d49e7a0b82ca 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4940,6 +4940,10 @@  static void ufshcd_lu_init(struct ufs_hba *hba, struct scsi_device *sdev)
 	    desc_buf[UNIT_DESC_PARAM_LU_WR_PROTECT] == UFS_LU_POWER_ON_WP)
 		hba->dev_info.is_lu_power_on_wp = true;
 
+	if (desc_buf[UNIT_DESC_PARAM_UNIT_INDEX] == UFS_RPMB_UNIT &&
+	    desc_buf[UNIT_DESC_PARAM_RPMB_REGION_EN] & 1 << 4)
+			hba->dev_info.b_advanced_rpmb_en = true;
+
 	kfree(desc_buf);
 set_qdepth:
 	/*
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 1bba3fead2ce..2e617ab87750 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -199,6 +199,7 @@  enum unit_desc_param {
 	UNIT_DESC_PARAM_PSA_SENSITIVE		= 0x7,
 	UNIT_DESC_PARAM_MEM_TYPE		= 0x8,
 	UNIT_DESC_PARAM_DATA_RELIABILITY	= 0x9,
+	UNIT_DESC_PARAM_RPMB_REGION_EN		= 0x9,
 	UNIT_DESC_PARAM_LOGICAL_BLK_SIZE	= 0xA,
 	UNIT_DESC_PARAM_LOGICAL_BLK_COUNT	= 0xB,
 	UNIT_DESC_PARAM_ERASE_BLK_SIZE		= 0x13,
@@ -601,6 +602,8 @@  struct ufs_dev_info {
 
 	bool	b_rpm_dev_flush_capable;
 	u8	b_presrv_uspc_en;
+
+	bool    b_advanced_rpmb_en;
 };
 
 /*