Linux ATA/IDE development
 help / color / mirror / Atom feed
* (unknown), 
From: beautyink @ 2017-06-08 18:00 UTC (permalink / raw)
  To: linux-ide

[-- Attachment #1: 31571386.zip --]
[-- Type: application/zip, Size: 3183 bytes --]

^ permalink raw reply

* Re: ZBC_IN command translation
From: Damien Le Moal @ 2017-06-08  8:27 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-ide, Hannes Reinecke, Tejun Heo, Jens Axboe,
	Martin K. Petersen
In-Reply-To: <20170608065553.GA14568@infradead.org>



On 6/8/17 15:55, Christoph Hellwig wrote:
> On Thu, Jun 08, 2017 at 09:18:39AM +0900, Damien Le Moal wrote:
>> So I am limiting support in target to the passthrough SCSI (pscsi)
>> backstore type (and will also add a user emulation backstore). Fixes
>> needed in pscsi are rather trivial, but the block layer is not involved
>> at the highest level as scsi commands are simply passed along through
>> requests. Hence the ZBC/ZAC translation problem showing up.
> 
> Bah, don't do that.  Passthrough backends are a nightmare in so many
> ways and I really prefer to not see them spread.

I found out this morning how hard it is... And that was just with report
zones. Sending a large report zone for instance triggers warnings all
over the place, including block and scsi layer. Handling of incoming
requests sgl seem to be very deficient.

>> I could work on completing support for ZBC at the block layer API:
>> 1) Add REQ_OP_ZONE_OPEN/CLOSE/FINISH, with corresponding
>> blkdev_issue_xxx functions
> 
> Or just emulate them.

That would mean maintaining the condition of all zones in memory to
override the condition reported by report zone. And doing so without
knowing the actual disk limit on the maximum number of open zones.

Doable I guess. Need to think more about it. Also wondering if the
emulated zone condition can be volatile...

>> 2) Add more queue attributes and corresponding files in sysfs for the
>> zoned block device characteristics
>> 3) Fix sd.c to support the new REQ_OP_ZONE_xxx
>> 4) Add corresponding user ioctls for OPEN/CLOSE/FINISH
> 
> What is the practical use cae for that?

The only one that I can see would be to simplify the above mentioned
emulation. With this support in place, no zone condition emulation is
necessary as zones can be fully managed. The emulation is reduced to
conversion from SCSI command to block layer calls.

Apart from that, I do not see any compelling use case for
open/close/finish. f2fs does not use it, on-going btrfs work does not
need it either, same for the device mapper support.

So that would be a lot of work for just the target block backstore.

-- 
Damien Le Moal,
Western Digital

^ permalink raw reply

* Re: ZBC_IN command translation
From: Christoph Hellwig @ 2017-06-08  6:55 UTC (permalink / raw)
  To: Damien Le Moal
  Cc: Christoph Hellwig, linux-ide, Hannes Reinecke, Tejun Heo,
	Jens Axboe, Martin K. Petersen
In-Reply-To: <8b223c09-81ff-19dd-fe32-63711c79b9cb@wdc.com>

On Thu, Jun 08, 2017 at 09:18:39AM +0900, Damien Le Moal wrote:
> So I am limiting support in target to the passthrough SCSI (pscsi)
> backstore type (and will also add a user emulation backstore). Fixes
> needed in pscsi are rather trivial, but the block layer is not involved
> at the highest level as scsi commands are simply passed along through
> requests. Hence the ZBC/ZAC translation problem showing up.

Bah, don't do that.  Passthrough backends are a nightmare in so many
ways and I really prefer to not see them spread.

> 
> I could work on completing support for ZBC at the block layer API:
> 1) Add REQ_OP_ZONE_OPEN/CLOSE/FINISH, with corresponding
> blkdev_issue_xxx functions

Or just emulate them.

> 2) Add more queue attributes and corresponding files in sysfs for the
> zoned block device characteristics
> 3) Fix sd.c to support the new REQ_OP_ZONE_xxx
> 4) Add corresponding user ioctls for OPEN/CLOSE/FINISH

What is the practical use cae for that?

^ permalink raw reply

* Re: [PATCH V4 1/3] ata: ahci_tegra: Add AHCI support for tegra210
From: Mikko Perttunen @ 2017-06-08  6:20 UTC (permalink / raw)
  To: Preetham Chandru Ramchandra,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, tj-DgEjT+Ai2ygdnm+yROfE0A
  Cc: preetham260-Re5JQEeQqe8AvxtiuMwx3w,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-ide-u79uwXL29TY76Z2rM5mHXA,
	vbyravarasu-DDmLM1+adcrQT0dZR+AlfA,
	pkunapuli-DDmLM1+adcrQT0dZR+AlfA
In-Reply-To: <1496408000-24856-2-git-send-email-pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Some small things, but generally this is starting to look good. Can 
probably wait for other people's reviews before v5.

On 06/02/2017 03:53 PM, Preetham Chandru Ramchandra wrote:
> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 1. Move tegra124 specifics to tegra124_ahci_init.
> 2. Separate the regulators needed for tegra124 and tegra210.
> 3. Disable DIPM for t210 and t124 as there are known issues.
> 4. Disable Devslp for t210 since devslp pin is shared with
>     pcie clkreq.
> 
> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v4:
> * Instead of disabling DIPM through quirk function,
>    hardcode by adding ATA_FLAG_NO_DIPM to
>    ahci_tegra_port_info's static definition
> * Changed access to aux register to optional because for T124,
>    we need to continue to support older device trees without
>    the aux_regs property
> v3:
> * Remove inline functions for read/write and modify to
>    SATA, SATA Config and SATA Aux registers.
> * Add code to disable DIPM and DevSlp for t210 and t124
> v2:
> * Fix indentation issues
> * Move the change to disable DIPM, HIPM, DevSlp, partial,
>    slumber and NCQ into a separate patch
> ---
>   drivers/ata/ahci_tegra.c | 386 ++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 302 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> index 3a62eb2..75520e5 100644
> --- a/drivers/ata/ahci_tegra.c
> +++ b/drivers/ata/ahci_tegra.c
> @@ -34,7 +34,8 @@
>   #define DRV_NAME "tegra-ahci"
>   
>   #define SATA_CONFIGURATION_0				0x180
> -#define SATA_CONFIGURATION_EN_FPCI			BIT(0)
> +#define SATA_CONFIGURATION_0_EN_FPCI			BIT(0)
> +#define SATA_CONFIGURATION_0_CLK_OVERRIDE			BIT(31)
>   
>   #define SCFG_OFFSET					0x1000
>   
> @@ -45,17 +46,55 @@
>   #define T_SATA0_CFG_1_SERR				BIT(8)
>   
>   #define T_SATA0_CFG_9					0x24
> -#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT		13
> +#define T_SATA0_CFG_9_BASE_ADDRESS			0x40020000
>   
>   #define SATA_FPCI_BAR5					0x94
> -#define SATA_FPCI_BAR5_START_SHIFT			4
> +#define SATA_FPCI_BAR5_START_MASK			(0xfffffff << 4)
> +#define SATA_FPCI_BAR5_START				(0x0040020 << 4)
> +#define SATA_FPCI_BAR5_ACCESS_TYPE			(0x1)
>   
>   #define SATA_INTR_MASK					0x188
>   #define SATA_INTR_MASK_IP_INT_MASK			BIT(16)
>   
> +#define T_SATA0_CFG_35					0x94
> +#define T_SATA0_CFG_35_IDP_INDEX_MASK			(0x7ff << 2)
> +#define T_SATA0_CFG_35_IDP_INDEX			(0x2a << 2)
> +
> +#define T_SATA0_AHCI_IDP1				0x98
> +#define T_SATA0_AHCI_IDP1_DATA				(0x400040)
> +
> +#define T_SATA0_CFG_PHY_1				0x12c
> +#define T_SATA0_CFG_PHY_1_PADS_IDDQ_EN			BIT(23)
> +#define T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN		BIT(22)
> +
> +#define T_SATA0_NVOOB                                   0x114
> +#define T_SATA0_NVOOB_COMMA_CNT_MASK                    (0xff << 16)
> +#define T_SATA0_NVOOB_COMMA_CNT                         (0x07 << 16)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK          (0x3 << 24)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE               (0x1 << 24)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK        (0x3 << 26)
> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH             (0x3 << 26)
> +
> +#define T_SATA_CFG_PHY_0                                0x120
> +#define T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD     BIT(11)
> +#define T_SATA_CFG_PHY_0_MASK_SQUELCH                   BIT(24)
> +
> +#define T_SATA0_CFG2NVOOB_2				0x134
> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK	(0x1ff << 18)
> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW	(0xc << 18)
> +
>   #define T_SATA0_AHCI_HBA_CAP_BKDR			0x300
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP	BIT(13)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP	BIT(14)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SALP			BIT(26)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM		BIT(17)
> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ			BIT(30)
>   
>   #define T_SATA0_BKDOOR_CC				0x4a4
> +#define T_SATA0_BKDOOR_CC_CLASS_CODE_MASK		(0xffff << 16)
> +#define T_SATA0_BKDOOR_CC_CLASS_CODE			(0x0106 << 16)
> +#define T_SATA0_BKDOOR_CC_PROG_IF_MASK			(0xff << 8)
> +#define T_SATA0_BKDOOR_CC_PROG_IF			(0x01 << 8)
>   
>   #define T_SATA0_CFG_SATA				0x54c
>   #define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN		BIT(12)
> @@ -82,9 +121,35 @@
>   #define T_SATA0_CHX_PHY_CTRL11				0x6d0
>   #define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ		(0x2800 << 16)
>   
> +#define T_SATA0_CHX_PHY_CTRL17_0			0x6e8
> +#define T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1	0x55010000
> +#define T_SATA0_CHX_PHY_CTRL18_0			0x6ec
> +#define T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2	0x55010000
> +#define T_SATA0_CHX_PHY_CTRL20_0			0x6f4
> +#define T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1	0x1
> +#define T_SATA0_CHX_PHY_CTRL21_0			0x6f8
> +#define T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2	0x1
> +
> +/* AUX Registers */
> +#define SATA_AUX_MISC_CNTL_1_0				0x8
> +#define SATA_AUX_MISC_CNTL_1_0_DEVSLP_OVERRIDE		BIT(17)
> +#define SATA_AUX_MISC_CNTL_1_0_SDS_SUPPORT		BIT(13)
> +#define SATA_AUX_MISC_CNTL_1_0_DESO_SUPPORT		BIT(15)
> +
> +#define SATA_AUX_RX_STAT_INT_0				0xc
> +#define SATA_AUX_RX_STAT_INT_0_SATA_DEVSLP		BIT(7)
> +
> +#define SATA_AUX_SPARE_CFG0_0				0x18
> +#define SATA_AUX_SPARE_CFG0_0_MDAT_TIMER_AFTER_PG_VALID	BIT(14)
> +
>   #define FUSE_SATA_CALIB					0x124
>   #define FUSE_SATA_CALIB_MASK				0x3
>   
> +enum {
> +	NO_DEVSLP	= (1 << 0),
> +	NO_DIPM		= (1 << 1),

NO_DIPM can be removed now.

> +};
> +
>   struct sata_pad_calibration {
>   	u8 gen1_tx_amp;
>   	u8 gen1_tx_peak;
> @@ -99,15 +164,91 @@ static const struct sata_pad_calibration tegra124_pad_calibration[] = {
>   	{0x14, 0x0e, 0x1a, 0x0e},
>   };
>   
> +struct tegra_ahci_ops {
> +	int (*init)(struct ahci_host_priv *);
> +};
> +
> +struct tegra_ahci_soc {
> +	const char *const	*supply_names;
> +	u32			num_supplies;
> +	u32			quirks;
> +	struct tegra_ahci_ops	ops;
> +};
> +
>   struct tegra_ahci_priv {
>   	struct platform_device	   *pdev;
>   	void __iomem		   *sata_regs;
> +	void __iomem		   *sata_aux_regs;
>   	struct reset_control	   *sata_rst;
>   	struct reset_control	   *sata_oob_rst;
>   	struct reset_control	   *sata_cold_rst;
>   	/* Needs special handling, cannot use ahci_platform */
>   	struct clk		   *sata_clk;
> -	struct regulator_bulk_data supplies[5];
> +	struct regulator_bulk_data *supplies;
> +	struct tegra_ahci_soc	   *soc_data;
> +};
> +
> +static const char *const tegra124_supply_names[] = {
> +	"avdd", "hvdd", "vddio", "target-5v", "target-12v"
> +};
> +
> +static void tegra_ahci_handle_quirks(struct ahci_host_priv *hpriv);

We should be able to move the tegra_ahci_handle_quirks function 
definition here now, instead of having a declaration, I think.

> +
> +static int tegra124_ahci_init(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	struct sata_pad_calibration calib;
> +	int ret;
> +	u32 val;
> +
> +	/* Pad calibration */
> +	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> +	if (ret)
> +		return ret;
> +
> +	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
> +
> +	writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	val = readl(tegra->sata_regs +
> +		    SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> +	val |= calib.gen1_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> +	val |= calib.gen1_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET +
> +	       T_SATA0_CHX_PHY_CTRL1_GEN1);
> +
> +	val = readl(tegra->sata_regs +
> +		    SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
> +	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> +	val |= calib.gen2_tx_amp << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> +	val |= calib.gen2_tx_peak << T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET +
> +	       T_SATA0_CHX_PHY_CTRL1_GEN2);
> +
> +	writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> +	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> +	writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> +	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> +
> +	writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> +
> +	return 0;
> +}
> +
> +static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
> +	.supply_names = tegra124_supply_names,
> +	.num_supplies = ARRAY_SIZE(tegra124_supply_names),
> +	.quirks = NO_DIPM | NO_DEVSLP,

also from here

> +	.ops = {
> +		.init = tegra124_ahci_init,
> +	},
> +};
> +
> +static const struct tegra_ahci_soc tegra210_ahci_soc_data = {
> +	.quirks = NO_DIPM | NO_DEVSLP,

and here

>   };
>   
>   static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> @@ -115,7 +256,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
>   	struct tegra_ahci_priv *tegra = hpriv->plat_data;
>   	int ret;
>   
> -	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> +	ret = regulator_bulk_enable(tegra->soc_data->num_supplies,
>   				    tegra->supplies);
>   	if (ret)
>   		return ret;
> @@ -144,8 +285,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
>   	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>   
>   disable_regulators:
> -	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> -
> +	regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
>   	return ret;
>   }
>   
> @@ -162,97 +302,139 @@ static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
>   	clk_disable_unprepare(tegra->sata_clk);
>   	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>   
> -	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> +	regulator_bulk_disable(tegra->soc_data->num_supplies, tegra->supplies);
>   }
>   
>   static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>   {
>   	struct tegra_ahci_priv *tegra = hpriv->plat_data;
>   	int ret;
> -	unsigned int val;
> -	struct sata_pad_calibration calib;
> +	u32 val;
>   
>   	ret = tegra_ahci_power_on(hpriv);
> -	if (ret) {
> -		dev_err(&tegra->pdev->dev,
> -			"failed to power on AHCI controller: %d\n", ret);
> +	if (ret)
>   		return ret;
> -	}
>   
> +	/*
> +	 * Program the following SATA IPFS registers
> +	 * to allow SW accesses to SATA's MMIO register range.
> +	 */
> +	val = readl(tegra->sata_regs + SATA_FPCI_BAR5);
> +	val &= ~(SATA_FPCI_BAR5_START_MASK | SATA_FPCI_BAR5_ACCESS_TYPE);
> +	val |= SATA_FPCI_BAR5_START | SATA_FPCI_BAR5_ACCESS_TYPE;
> +	writel(val, tegra->sata_regs + SATA_FPCI_BAR5);
> +
> +	/* Program the following SATA IPFS register to enable the SATA */
>   	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
> -	val |= SATA_CONFIGURATION_EN_FPCI;
> +	val |= SATA_CONFIGURATION_0_EN_FPCI;
>   	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
>   
> -	/* Pad calibration */
> -
> -	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> -	if (ret) {
> -		dev_err(&tegra->pdev->dev,
> -			"failed to read calibration fuse: %d\n", ret);
> -		return ret;
> -	}
> -
> -	calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
> -
> -	writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> -
> -	val = readl(tegra->sata_regs +
> -		SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
> -	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
> -	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
> -	val |= calib.gen1_tx_amp <<
> -			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> -	val |= calib.gen1_tx_peak <<
> -			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> -	writel(val, tegra->sata_regs + SCFG_OFFSET +
> -		T_SATA0_CHX_PHY_CTRL1_GEN1);
> -
> -	val = readl(tegra->sata_regs +
> -			SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
> -	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
> -	val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
> -	val |= calib.gen2_tx_amp <<
> -			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
> -	val |= calib.gen2_tx_peak <<
> -			T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
> -	writel(val, tegra->sata_regs + SCFG_OFFSET +
> -		T_SATA0_CHX_PHY_CTRL1_GEN2);
> -
> -	writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
> -		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL11);
> -	writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
> -		tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL2);
> -
> -	writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
> -
> -	/* Program controller device ID */
> +	/* Electrical settings for better link stability */
> +	val = T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL17_0);
> +	val = T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL18_0);
> +	val = T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL20_0);
> +	val = T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL21_0);
> +
> +	/* For SQUELCH Filter & Gen3 drive getting detected as Gen1 drive */
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA_CFG_PHY_0);
> +	val |= T_SATA_CFG_PHY_0_MASK_SQUELCH;
> +	val &= ~T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA_CFG_PHY_0);
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_NVOOB);
> +	val &= ~(T_SATA0_NVOOB_COMMA_CNT_MASK |
> +		 T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK |
> +		 T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK);
> +	val |= (T_SATA0_NVOOB_COMMA_CNT |
> +		T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH |
> +		T_SATA0_NVOOB_SQUELCH_FILTER_MODE);
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_NVOOB);
> +
> +	/*
> +	 * Change CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW from 83.3 ns to 58.8ns
> +	 */
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG2NVOOB_2);
> +	val &= ~T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK;
> +	val |= T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG2NVOOB_2);
> +
> +	if (tegra->soc_data->ops.init)
> +		tegra->soc_data->ops.init(hpriv);
> +
> +	/*
> +	 * Program the following SATA configuration registers
> +	 * to initialize SATA
> +	 */
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> +	val |= (T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
> +		T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR);
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> +	val = T_SATA0_CFG_9_BASE_ADDRESS;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
>   
> +	/* Program Class Code and Programming interface for SATA */
>   	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>   	val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
>   	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>   
> -	writel(0x01060100, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
> +	val &=
> +	    ~(T_SATA0_BKDOOR_CC_CLASS_CODE_MASK |
> +	      T_SATA0_BKDOOR_CC_PROG_IF_MASK);
> +	val |= T_SATA0_BKDOOR_CC_CLASS_CODE | T_SATA0_BKDOOR_CC_PROG_IF;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_BKDOOR_CC);
>   
>   	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>   	val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
>   	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>   
> -	/* Enable IO & memory access, bus master mode */
> -
> -	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> -	val |= T_SATA0_CFG_1_IO_SPACE | T_SATA0_CFG_1_MEMORY_SPACE |
> -		T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
> -	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
> -
> -	/* Program SATA MMIO */
> -
> -	writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
> -	       tegra->sata_regs + SATA_FPCI_BAR5);
> +	/* Enabling LPM capabilities through Backdoor Programming */
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_AHCI_HBA_CAP_BKDR);
> +	val |= (T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP |
> +		T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP |
> +		T_SATA0_AHCI_HBA_CAP_BKDR_SALP |
> +		T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM);
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_AHCI_HBA_CAP_BKDR);
> +
> +	/* SATA Second Level Clock Gating configuration
> +	 * Enabling Gating of Tx/Rx clocks and driving Pad IDDQ and Lane
> +	 * IDDQ Signals
> +	 */
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_35);
> +	val &= ~T_SATA0_CFG_35_IDP_INDEX_MASK;
> +	val |= T_SATA0_CFG_35_IDP_INDEX;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_35);
> +
> +	val = T_SATA0_AHCI_IDP1_DATA;
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_AHCI_IDP1);
> +
> +	val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_PHY_1);
> +	val |= (T_SATA0_CFG_PHY_1_PADS_IDDQ_EN |
> +		T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN);
> +	writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_PHY_1);
> +
> +	/*
> +	 * Indicate Sata only has the capability to enter DevSleep
> +	 * from slumber link.
> +	 */
> +
> +	if (tegra->sata_aux_regs) {
> +		val = readl(tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> +		val |= SATA_AUX_MISC_CNTL_1_0_DESO_SUPPORT;
> +		writel(val, tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> +	}
>   
> -	writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
> -	       tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
> +	/* Enabling IPFS Clock Gating */
> +	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
> +	val &= ~SATA_CONFIGURATION_0_CLK_OVERRIDE;
> +	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
>   
> -	/* Unmask SATA interrupts */
> +	tegra_ahci_handle_quirks(hpriv);
>   
>   	val = readl(tegra->sata_regs + SATA_INTR_MASK);
>   	val |= SATA_INTR_MASK_IP_INT_MASK;
> @@ -278,15 +460,22 @@ static struct ata_port_operations ahci_tegra_port_ops = {
>   	.host_stop	= tegra_ahci_host_stop,
>   };
>   
> -static const struct ata_port_info ahci_tegra_port_info = {
> -	.flags		= AHCI_FLAG_COMMON,
> +static struct ata_port_info ahci_tegra_port_info = {

This can be const now

> +	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
>   	.pio_mask	= ATA_PIO4,
>   	.udma_mask	= ATA_UDMA6,
>   	.port_ops	= &ahci_tegra_port_ops,
>   };
>   
>   static const struct of_device_id tegra_ahci_of_match[] = {
> -	{ .compatible = "nvidia,tegra124-ahci" },
> +	{
> +		.compatible = "nvidia,tegra124-ahci",
> +		.data = &tegra124_ahci_soc_data
> +	},
> +	{
> +		.compatible = "nvidia,tegra210-ahci",
> +		.data = &tegra210_ahci_soc_data
> +	},
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> @@ -295,12 +484,25 @@ static struct scsi_host_template ahci_platform_sht = {
>   	AHCI_SHT(DRV_NAME),
>   };
>   
> +static void tegra_ahci_handle_quirks(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	u32 val;
> +
> +	if (tegra->sata_aux_regs && (tegra->soc_data->quirks & NO_DEVSLP)) {
> +		val = readl(tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> +		val &= ~SATA_AUX_MISC_CNTL_1_0_SDS_SUPPORT;
> +		writel(val, tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0);
> +	}
> +}
> +
>   static int tegra_ahci_probe(struct platform_device *pdev)
>   {
>   	struct ahci_host_priv *hpriv;
>   	struct tegra_ahci_priv *tegra;
>   	struct resource *res;
>   	int ret;
> +	unsigned int i;
>   
>   	hpriv = ahci_platform_get_resources(pdev);
>   	if (IS_ERR(hpriv))
> @@ -311,13 +513,25 @@ static int tegra_ahci_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	hpriv->plat_data = tegra;
> -
>   	tegra->pdev = pdev;
> +	tegra->soc_data =
> +	    (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev);
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>   	tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
>   	if (IS_ERR(tegra->sata_regs))
>   		return PTR_ERR(tegra->sata_regs);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	/*
> +	 * Aux register is optional.
> +	 */
> +	if (res) {
> +		tegra->sata_aux_regs = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(tegra->sata_aux_regs))
> +			return PTR_ERR(tegra->sata_aux_regs);
> +	} else {
> +		tegra->sata_aux_regs = NULL;
> +	}

'tegra' is allocated using kzalloc, so the else branch is not required

>   
>   	tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
>   	if (IS_ERR(tegra->sata_rst)) {
> @@ -343,13 +557,17 @@ static int tegra_ahci_probe(struct platform_device *pdev)
>   		return PTR_ERR(tegra->sata_clk);
>   	}
>   
> -	tegra->supplies[0].supply = "avdd";
> -	tegra->supplies[1].supply = "hvdd";
> -	tegra->supplies[2].supply = "vddio";
> -	tegra->supplies[3].supply = "target-5v";
> -	tegra->supplies[4].supply = "target-12v";
> +	tegra->supplies = devm_kcalloc(&pdev->dev,
> +				       tegra->soc_data->num_supplies,
> +				       sizeof(*tegra->supplies), GFP_KERNEL);
> +	if (!tegra->supplies)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < tegra->soc_data->num_supplies; i++)
> +		tegra->supplies[i].supply = tegra->soc_data->supply_names[i];
>   
> -	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> +	ret = devm_regulator_bulk_get(&pdev->dev,
> +				      tegra->soc_data->num_supplies,
>   				      tegra->supplies);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to get regulators\n");
> @@ -385,5 +603,5 @@ static struct platform_driver tegra_ahci_driver = {
>   module_platform_driver(tegra_ahci_driver);
>   
>   MODULE_AUTHOR("Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> -MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
> +MODULE_DESCRIPTION("Tegra AHCI SATA driver");
>   MODULE_LICENSE("GPL v2");
> 

Thanks,
Mikko

^ permalink raw reply

* Re: ZBC_IN command translation
From: Damien Le Moal @ 2017-06-08  0:18 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-ide, Hannes Reinecke, Tejun Heo, Jens Axboe,
	Martin K. Petersen
In-Reply-To: <20170607132443.GA3501@infradead.org>

Christoph,

(+CC Jens and Martin)

On 6/7/17 22:24, Christoph Hellwig wrote:
> On Wed, Jun 07, 2017 at 06:11:33PM +0900, Damien Le Moal wrote:
>> Sure, that is one solution. However, not a ideal one as that would
>> prevent ZBC/ZAC drives working with target/iscsi.
>> There are a few problems there that I am fixing right now, but that
>> translation error is stopping all effort.
> 
> The normal way to use the target code is through the block layer.

A block backstore in target indeed is great as any block device, not
just physical ones, can be exported. The problem is that for a zoned
block device the block layer API lacks functions and information to
fully emulate the corresponding ZBC SCSI device in target: no
open/close/finish zone functions (no REQ_OP_xx for that), and the
information from the zoned block device characteristics VPD page 0xB6 is
not available.

So I am limiting support in target to the passthrough SCSI (pscsi)
backstore type (and will also add a user emulation backstore). Fixes
needed in pscsi are rather trivial, but the block layer is not involved
at the highest level as scsi commands are simply passed along through
requests. Hence the ZBC/ZAC translation problem showing up.

I could work on completing support for ZBC at the block layer API:
1) Add REQ_OP_ZONE_OPEN/CLOSE/FINISH, with corresponding
blkdev_issue_xxx functions
2) Add more queue attributes and corresponding files in sysfs for the
zoned block device characteristics
3) Fix sd.c to support the new REQ_OP_ZONE_xxx
4) Add corresponding user ioctls for OPEN/CLOSE/FINISH

That's a lot of work for mostly just being used in the target code as
none of the other areas in the kernel supporting zoned block devices
don't really need these features (dm, f2fs, and on-going btrfs). But if
you think that is OK to add all this, I will. And fix target ZBC support
using that.

Best regards.

-- 
Damien Le Moal,
Western Digital

^ permalink raw reply

* Re: ZBC_IN command translation
From: Christoph Hellwig @ 2017-06-07 13:24 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: Christoph Hellwig, linux-ide, Hannes Reinecke, Tejun Heo
In-Reply-To: <f9d14e79-fffd-1398-4767-ffe527bf9ee6@wdc.com>

On Wed, Jun 07, 2017 at 06:11:33PM +0900, Damien Le Moal wrote:
> Sure, that is one solution. However, not a ideal one as that would
> prevent ZBC/ZAC drives working with target/iscsi.
> There are a few problems there that I am fixing right now, but that
> translation error is stopping all effort.

The normal way to use the target code is through the block layer.

^ permalink raw reply

* Re: ZBC_IN command translation
From: Damien Le Moal @ 2017-06-07  9:11 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-ide, Hannes Reinecke, Tejun Heo
In-Reply-To: <20170607090751.GA29487@infradead.org>

Christoph,

On 6/7/17 18:07, Christoph Hellwig wrote:
> On Wed, Jun 07, 2017 at 06:03:52PM +0900, Damien Le Moal wrote:
>> Tejun, Hannes,
>>
>> Currently, in libata-scsi.c, the function ata_scsi_zbc_in_xlat
>> translating ZBC REPORT ZONES into the ZAC version returns an error if
>> the scsi command buffer length is not aligned on 512. This is possible
>> since the ZBC version allows report zones buffer as a multiple of 64B,
>> while the ZAC version of the same command requires 512B size alignment
>> of the command buffer.
>>
>> However, SAT-4, in section 9.13.3 says:
>>
>> "The SATL shall send the ATA REPORT ZONES EXT command with the ATA
>> RETURN PAGE COUNT field set to INT((ALLOCATION LENGTH + 511)/512)."
>>
>> So clearly, instead of returning an error, we need to bounce the scsi
>> command buffer to a bigger 512B size aligned temporary buffer for the
>> ZAC report zones. I do not see any code ready to use to do so easily,
>> but I may be missing it. Is there something that can be used to do so or
>> do I need to cook something ?
> 
> Just like for TRIM we should reject any ZBC_IN/OUT command that doesn't
> come from the block layer, and then we can tightly control what input
> we get.

Sure, that is one solution. However, not a ideal one as that would
prevent ZBC/ZAC drives working with target/iscsi.
There are a few problems there that I am fixing right now, but that
translation error is stopping all effort.

-- 
Damien Le Moal,
Western Digital

^ permalink raw reply

* Re: ZBC_IN command translation
From: Christoph Hellwig @ 2017-06-07  9:07 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: linux-ide, Hannes Reinecke, Tejun Heo
In-Reply-To: <4b12630e-1add-6a4e-51ab-52417e60d4b2@wdc.com>

On Wed, Jun 07, 2017 at 06:03:52PM +0900, Damien Le Moal wrote:
> Tejun, Hannes,
> 
> Currently, in libata-scsi.c, the function ata_scsi_zbc_in_xlat
> translating ZBC REPORT ZONES into the ZAC version returns an error if
> the scsi command buffer length is not aligned on 512. This is possible
> since the ZBC version allows report zones buffer as a multiple of 64B,
> while the ZAC version of the same command requires 512B size alignment
> of the command buffer.
> 
> However, SAT-4, in section 9.13.3 says:
> 
> "The SATL shall send the ATA REPORT ZONES EXT command with the ATA
> RETURN PAGE COUNT field set to INT((ALLOCATION LENGTH + 511)/512)."
> 
> So clearly, instead of returning an error, we need to bounce the scsi
> command buffer to a bigger 512B size aligned temporary buffer for the
> ZAC report zones. I do not see any code ready to use to do so easily,
> but I may be missing it. Is there something that can be used to do so or
> do I need to cook something ?

Just like for TRIM we should reject any ZBC_IN/OUT command that doesn't
come from the block layer, and then we can tightly control what input
we get.

^ permalink raw reply

* ZBC_IN command translation
From: Damien Le Moal @ 2017-06-07  9:03 UTC (permalink / raw)
  To: linux-ide, Hannes Reinecke, Tejun Heo

Tejun, Hannes,

Currently, in libata-scsi.c, the function ata_scsi_zbc_in_xlat
translating ZBC REPORT ZONES into the ZAC version returns an error if
the scsi command buffer length is not aligned on 512. This is possible
since the ZBC version allows report zones buffer as a multiple of 64B,
while the ZAC version of the same command requires 512B size alignment
of the command buffer.

However, SAT-4, in section 9.13.3 says:

"The SATL shall send the ATA REPORT ZONES EXT command with the ATA
RETURN PAGE COUNT field set to INT((ALLOCATION LENGTH + 511)/512)."

So clearly, instead of returning an error, we need to bounce the scsi
command buffer to a bigger 512B size aligned temporary buffer for the
ZAC report zones. I do not see any code ready to use to do so easily,
but I may be missing it. Is there something that can be used to do so or
do I need to cook something ?

Best regards.

-- 
Damien Le Moal,
Western Digital

^ permalink raw reply

* [PATCH 1/2] dt-bindings: ahci-fsl-qoriq: add ls1088a chip name to the list
From: Yuantian Tang @ 2017-06-07  6:32 UTC (permalink / raw)
  To: shawnguo
  Cc: tj, robh+dt, mark.rutland, catalin.marinas, will.deacon,
	linux-ide, devicetree, linux-kernel, linux-arm-kernel,
	Yuantian Tang

Signed-off-by: Tang Yuantian <andy.tang@nxp.com>
---
 Documentation/devicetree/bindings/ata/ahci-fsl-qoriq.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-fsl-qoriq.txt b/Documentation/devicetree/bindings/ata/ahci-fsl-qoriq.txt
index fc33ca0..7c3ca0e 100644
--- a/Documentation/devicetree/bindings/ata/ahci-fsl-qoriq.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-fsl-qoriq.txt
@@ -3,7 +3,7 @@ Binding for Freescale QorIQ AHCI SATA Controller
 Required properties:
   - reg: Physical base address and size of the controller's register area.
   - compatible: Compatibility string. Must be 'fsl,<chip>-ahci', where
-    chip could be ls1021a, ls1043a, ls1046a, ls2080a etc.
+    chip could be ls1021a, ls1043a, ls1046a, ls1088a, ls2080a etc.
   - clocks: Input clock specifier. Refer to common clock bindings.
   - interrupts: Interrupt specifier. Refer to interrupt binding.
 
-- 
2.1.0.27.g96db324


^ permalink raw reply related

* [PATCH 2/2 v2] arm64: dts: ls1088a: update sata node
From: Yuantian Tang @ 2017-06-07  6:32 UTC (permalink / raw)
  To: shawnguo
  Cc: tj, robh+dt, mark.rutland, catalin.marinas, will.deacon,
	linux-ide, devicetree, linux-kernel, linux-arm-kernel,
	Yuantian Tang
In-Reply-To: <1496817168-26567-1-git-send-email-andy.tang@nxp.com>

1. Remove ls1043a compatible string from node
2. Fix the sata ecc register address error

Signed-off-by: Tang Yuantian <andy.tang@nxp.com>
---
v2:
	- no change, resend it with binding update

 arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
index df16284..c144d06 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
@@ -360,9 +360,9 @@
 		};
 
 		sata: sata@3200000 {
-			compatible = "fsl,ls1088a-ahci", "fsl,ls1043a-ahci";
+			compatible = "fsl,ls1088a-ahci";
 			reg = <0x0 0x3200000 0x0 0x10000>,
-				<0x0 0x20140520 0x0 0x4>;
+				<0x7 0x100520 0x0 0x4>;
 			reg-names = "ahci", "sata-ecc";
 			interrupts = <0 133 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clockgen 4 3>;
-- 
2.1.0.27.g96db324

^ permalink raw reply related

* (unknown), 
From: lucia.germino @ 2017-06-07  3:19 UTC (permalink / raw)
  To: linux-ide

[-- Attachment #1: 6949818.zip --]
[-- Type: application/zip, Size: 4774 bytes --]

^ permalink raw reply

* Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks
From: Scott Bauer @ 2017-06-06 21:40 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Tejun Heo, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170606095955.GC20590@lst.de>

On Tue, Jun 06, 2017 at 11:59:55AM +0200, Christoph Hellwig wrote:
> On Mon, Jun 05, 2017 at 03:15:31PM -0600, Scott Bauer wrote:
> > I'm not familiar at all with ATA, but I noticed there was no unlock from suspend support
> > in the series. Does ATA not have a way to determine if we're coming out of a suspend?
> 
> I don't know, and not having a test system with a OPAL capable driver
> and suspend support I could not even test the code.
> 
> > I see there are some power-ops in scsi/sd.c, if you do want to add it you can mabe toss a
> > 
> > if (sdkp->security)
> >    opal_unlock_from_suspend(sdpk->opal_dev)
> > 
> > somewhere in the resume path? We handle null opal_devs and no unlock from suspend list
> > so calling it when nothing is set up is just a no-op.
> 
> Yeah, maybe.  We'll just need someone who could test it first.

I was given a sata drive that apparently has opal enabled on it. If it actually has opal
I can run some tests.

^ permalink raw reply

* (unknown), 
From: dengx @ 2017-06-06 20:36 UTC (permalink / raw)
  To: linux-ide

[-- Attachment #1: 8658933842225.zip --]
[-- Type: application/zip, Size: 3176 bytes --]

^ permalink raw reply

* Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks
From: Christoph Hellwig @ 2017-06-06  9:59 UTC (permalink / raw)
  To: Scott Bauer
  Cc: Christoph Hellwig, Tejun Heo, Jonathan Derrick, Rafael Antognolli,
	Robert Elliott, linux-ide, linux-block, linux-scsi
In-Reply-To: <20170605211530.aqxwigcrxgsdvmhs@sbauer-Z170X-UD5>

On Mon, Jun 05, 2017 at 03:15:31PM -0600, Scott Bauer wrote:
> I'm not familiar at all with ATA, but I noticed there was no unlock from suspend support
> in the series. Does ATA not have a way to determine if we're coming out of a suspend?

I don't know, and not having a test system with a OPAL capable driver
and suspend support I could not even test the code.

> I see there are some power-ops in scsi/sd.c, if you do want to add it you can mabe toss a
> 
> if (sdkp->security)
>    opal_unlock_from_suspend(sdpk->opal_dev)
> 
> somewhere in the resume path? We handle null opal_devs and no unlock from suspend list
> so calling it when nothing is set up is just a no-op.

Yeah, maybe.  We'll just need someone who could test it first.

^ permalink raw reply

* Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks
From: Christoph Hellwig @ 2017-06-06  9:58 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: Christoph Hellwig, Tejun Heo, Scott Bauer, Jonathan Derrick,
	Rafael Antognolli, Robert Elliott, linux-ide, linux-block,
	linux-scsi
In-Reply-To: <yq1lgp6djr3.fsf@oracle.com>

On Mon, Jun 05, 2017 at 08:48:00PM -0400, Martin K. Petersen wrote:
> For WRITE SAME, scsi_report_opcode() is gated not only by
> sdev->no_report_opcodes but by sdev->no_write_same.
> 
> I'm concerned about firing off REPORT OPCODES to random devices without
> a sufficiently good heuristic. Doesn't look like SAT has anything to
> offer in this department, though. Maybe it's time to consider a
> vendor-specific Linux VPD page...

Eww.  Given that as far as I can tell only ATA devices implement
OPAL we could key it off that for now.  But that's only going to
defer the problem until support for other security protocols comes
along for real SCSI devices.

But as we already set no_report_opcodes for all usb-storage and
quirked uas devices I think the worst offenders are already covered
anyway.

^ permalink raw reply

* Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks
From: Hannes Reinecke @ 2017-06-06  6:31 UTC (permalink / raw)
  To: Christoph Hellwig, Tejun Heo
  Cc: Scott Bauer, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-7-hch@lst.de>

On 06/04/2017 02:42 PM, Christoph Hellwig wrote:
> Just wire up the generic TCG OPAL infrastructure to the SCSI disk driver
> and the Security In/Out commands.
> 
> Note that I don't know of any actual SCSI disks that do support TCG OPAL,
> but this is required to support ATA disks through libata.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/sd.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/sd.h |  2 ++
>  2 files changed, 46 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH 5/6] libata: implement SECURITY PROTOCOL IN/OUT
From: Hannes Reinecke @ 2017-06-06  6:30 UTC (permalink / raw)
  To: Christoph Hellwig, Tejun Heo
  Cc: Scott Bauer, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-6-hch@lst.de>

On 06/04/2017 02:42 PM, Christoph Hellwig wrote:
> This allows us to use the generic OPAL code with ATA devices.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libata-core.c | 32 ++++++++++++++++++++
>  drivers/ata/libata-scsi.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/ata.h       |  1 +
>  include/linux/libata.h    |  1 +
>  4 files changed, 110 insertions(+)
> Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH 4/6] libata: factor out a ata_identify_page_supported helper
From: Hannes Reinecke @ 2017-06-06  6:29 UTC (permalink / raw)
  To: Christoph Hellwig, Tejun Heo
  Cc: Scott Bauer, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-5-hch@lst.de>

On 06/04/2017 02:42 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libata-core.c | 59 +++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 27 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH 3/6] libata: clarify log page naming / grouping
From: Hannes Reinecke @ 2017-06-06  6:29 UTC (permalink / raw)
  To: Christoph Hellwig, Tejun Heo
  Cc: Scott Bauer, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-4-hch@lst.de>

On 06/04/2017 02:42 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libata-core.c | 10 +++++-----
>  include/linux/ata.h       | 10 +++++++---
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH 2/6] libata: factor out a ata_log_supported helper
From: Hannes Reinecke @ 2017-06-06  6:28 UTC (permalink / raw)
  To: Christoph Hellwig, Tejun Heo
  Cc: Scott Bauer, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-3-hch@lst.de>

On 06/04/2017 02:42 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libata-core.c | 59 +++++++++++++----------------------------------
>  1 file changed, 16 insertions(+), 43 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH 1/6] libata: move ata_read_log_page to libata-core.c
From: Hannes Reinecke @ 2017-06-06  6:27 UTC (permalink / raw)
  To: Christoph Hellwig, Tejun Heo
  Cc: Scott Bauer, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-2-hch@lst.de>

On 06/04/2017 02:42 PM, Christoph Hellwig wrote:
> It is core functionality, and only one of the users is in the EH code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/ata/libata-core.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-eh.c   | 64 -----------------------------------------------
>  drivers/ata/libata.h      |  4 +--
>  3 files changed, 66 insertions(+), 66 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks
From: Martin K. Petersen @ 2017-06-06  0:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Tejun Heo, Scott Bauer, Jonathan Derrick, Rafael Antognolli,
	Robert Elliott, linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-7-hch@lst.de>


Christoph,

> +static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer)
> +{
> +	struct scsi_device *sdev = sdkp->device;
> +
> +	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> +			SECURITY_PROTOCOL_IN) == 1 &&
> +	    scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> +			SECURITY_PROTOCOL_OUT) == 1)
> +		sdkp->security = 1;
> +}
> +
>  /**
>   *	sd_revalidate_disk - called the first time a new disk is seen,
>   *	performs disk spin up, read_capacity, etc.
> @@ -3067,6 +3102,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  		sd_read_cache_type(sdkp, buffer);
>  		sd_read_app_tag_own(sdkp, buffer);
>  		sd_read_write_same(sdkp, buffer);
> +		sd_read_security(sdkp, buffer);
>  	}

For WRITE SAME, scsi_report_opcode() is gated not only by
sdev->no_report_opcodes but by sdev->no_write_same.

I'm concerned about firing off REPORT OPCODES to random devices without
a sufficiently good heuristic. Doesn't look like SAT has anything to
offer in this department, though. Maybe it's time to consider a
vendor-specific Linux VPD page...

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply

* Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks
From: Scott Bauer @ 2017-06-05 21:15 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Tejun Heo, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
	linux-ide, linux-block, linux-scsi
In-Reply-To: <20170604124225.27032-7-hch@lst.de>

On Sun, Jun 04, 2017 at 02:42:25PM +0200, Christoph Hellwig wrote:
> Just wire up the generic TCG OPAL infrastructure to the SCSI disk driver
> and the Security In/Out commands.
> 
> Note that I don't know of any actual SCSI disks that do support TCG OPAL,
> but this is required to support ATA disks through libata.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/sd.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/sd.h |  2 ++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b6bb4e0ce0e3..782f909a223c 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -50,6 +50,7 @@
>  #include <linux/string_helpers.h>
>  #include <linux/async.h>
>  #include <linux/slab.h>
> +#include <linux/sed-opal.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pr.h>
>  #include <linux/t10-pi.h>
> @@ -643,6 +644,26 @@ static void scsi_disk_put(struct scsi_disk *sdkp)
>  	mutex_unlock(&sd_ref_mutex);
>  }
>  
> +#ifdef CONFIG_BLK_SED_OPAL
> +static int sd_sec_submit(void *data, u16 spsp, u8 secp, void *buffer,
> +		size_t len, bool send)
> +{
> +	struct scsi_device *sdev = data;
> +	u8 cdb[12] = { 0, };
> +	int ret;
> +
> +	cdb[0] = send ? SECURITY_PROTOCOL_OUT : SECURITY_PROTOCOL_IN;
> +	cdb[1] = secp;
> +	put_unaligned_be16(spsp, &cdb[2]);
> +	put_unaligned_be32(len, &cdb[6]);
> +
> +	ret = scsi_execute_req(sdev, cdb,
> +			send ? DMA_TO_DEVICE : DMA_FROM_DEVICE,
> +			buffer, len, NULL, SD_TIMEOUT, SD_MAX_RETRIES, NULL);
> +	return ret <= 0 ? ret : -EIO;
> +}
> +#endif /* CONFIG_BLK_SED_OPAL */
> +
>  static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd,
>  					   unsigned int dix, unsigned int dif)
>  {
> @@ -1454,6 +1475,9 @@ static int sd_ioctl(struct block_device *bdev, fmode_t mode,
>  	if (error)
>  		goto out;
>  
> +	if (is_sed_ioctl(cmd))
> +		return sed_ioctl(sdkp->opal_dev, cmd, p);
> +
>  	/*
>  	 * Send SCSI addressing ioctls directly to mid level, send other
>  	 * ioctls to block level and then onto mid level if they can't be
> @@ -3014,6 +3038,17 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
>  		sdkp->ws10 = 1;
>  }
>  
> +static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer)
> +{
> +	struct scsi_device *sdev = sdkp->device;
> +
> +	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> +			SECURITY_PROTOCOL_IN) == 1 &&
> +	    scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> +			SECURITY_PROTOCOL_OUT) == 1)
> +		sdkp->security = 1;
> +}
> +
>  /**
>   *	sd_revalidate_disk - called the first time a new disk is seen,
>   *	performs disk spin up, read_capacity, etc.
> @@ -3067,6 +3102,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  		sd_read_cache_type(sdkp, buffer);
>  		sd_read_app_tag_own(sdkp, buffer);
>  		sd_read_write_same(sdkp, buffer);
> +		sd_read_security(sdkp, buffer);
>  	}
>  
>  	sdkp->first_scan = 0;
> @@ -3227,6 +3263,12 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>  
>  	sd_revalidate_disk(gd);
>  
> +	if (sdkp->security) {
> +		sdkp->opal_dev = init_opal_dev(sdp, &sd_sec_submit);
> +		if (sdkp->opal_dev)
> +			sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
> +	}
> +
> 
I'm not familiar at all with ATA, but I noticed there was no unlock from suspend support
in the series. Does ATA not have a way to determine if we're coming out of a suspend?

I see there are some power-ops in scsi/sd.c, if you do want to add it you can mabe toss a

if (sdkp->security)
   opal_unlock_from_suspend(sdpk->opal_dev)

somewhere in the resume path? We handle null opal_devs and no unlock from suspend list
so calling it when nothing is set up is just a no-op.

^ permalink raw reply

* [GIT PULL] libata fixes for v4.12-rc4
From: Tejun Heo @ 2017-06-05 20:21 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, linux-ide

Hello, Linus.

libata fixes for v4.12-rc4.

* Revert of sata_mv devm_ioremap_resource() conversion.  It made init
  fail if there are overlapping resources which led to detection
  failures on some setups.

* A workaround for an Acer laptop which sometimes reports corrupt port
  map.

* Other non-critical fixes.

Thanks.

The following changes since commit a95cfad947d5f40cfbf9ad3019575aac1d8ac7a6:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2017-05-15 15:50:49 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata.git for-4.12-fixes

for you to fetch changes up to f7cf69ae171592d133c69b9adaa5de7cfb6038ea:

  libata: fix error checking in in ata_parse_force_one() (2017-05-31 14:26:26 -0400)

----------------------------------------------------------------
Andrew Lunn (1):
      Revert "ata: sata_mv: Convert to devm_ioremap_resource()"

Arvind Yadav (1):
      ata: sata_rcar: Handle return value of clk_prepare_enable

Sui Chen (1):
      ahci: Acer SA5-271 SSD Not Detected Fix

Tejun Heo (1):
      libata: fix error checking in in ata_parse_force_one()

Thomas Petazzoni (1):
      ata: libahci: properly propagate return value of platform_get_irq()

 drivers/ata/ahci.c             | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/ata/libahci_platform.c |  5 +++--
 drivers/ata/libata-core.c      |  2 +-
 drivers/ata/sata_mv.c          | 13 ++++++++-----
 drivers/ata/sata_rcar.c        | 15 ++++++++++++---
 5 files changed, 62 insertions(+), 11 deletions(-)

-- 
tejun

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox