Linux ATA/IDE development
 help / color / mirror / Atom feed
* Re: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
From: Chaitanya Kulkarni @ 2017-01-10 22:40 UTC (permalink / raw)
  To: lsf-pc@lists.linux-foundation.org
  Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
	linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org,
	linux-ide@vger.kernel.org
In-Reply-To: <CO2PR04MB218427BF42159B20FB26F74186670@CO2PR04MB2184.namprd04.prod.outlook.com>

Resending it at as a plain text.

From: Chaitanya Kulkarni
Sent: Tuesday, January 10, 2017 2:37 PM
To: lsf-pc@lists.linux-foundation.org
Cc: linux-fsdevel@vger.kernel.org; linux-block@vger.kernel.org; linux-nvme@lists.infradead.org; linux-scsi@vger.kernel.org; linux-ide@vger.kernel.org
Subject: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
  

Hi Folks,

I would like to propose a general discussion on Storage stack and device driver testing.

Purpose:-
-------------
The main objective of this discussion is to address the need for 
a Unified Test Automation Framework which can be used by different subsystems
in the kernel in order to improve the overall development and stability
of the storage stack.

For Example:- 
>From my previous experience, I've worked on the NVMe driver testing last year and we
have developed simple unit test framework
 (https://github.com/linux-nvme/nvme-cli/tree/master/tests). 
In current implementation Upstream NVMe Driver supports following subsystems:-
1. PCI Host.
2. RDMA Target.
3. Fiber Channel Target (in progress).
Today due to lack of centralized automated test framework NVMe Driver testing is 
scattered and performed using the combination of various utilities like nvme-cli/tests, 
nvmet-cli, shell scripts (git://git.infradead.org/nvme-fabrics.git nvmf-selftests) etc.

In order to improve overall driver stability with various subsystems, it will be beneficial
to have a Unified Test Automation Framework (UTAF) which will centralize overall
testing. 

This topic will allow developers from various subsystem engage in the discussion about 
how to collaborate efficiently instead of having discussions on lengthy email threads.

Participants:-
------------------
I'd like to invite developers from different subsystems to discuss an approach towards 
a unified testing methodology for storage stack and device drivers belongs to 
different subsystems.

Topics for Discussion:-
------------------------------
As a part of discussion following are some of the key points which we can focus on:-
1. What are the common components of the kernel used by the various subsystems?
2. What are the potential target drivers which can benefit from this approach? 
  (e.g. NVMe, NVMe Over Fabric, Open Channel Solid State Drives etc.)
3. What are the desired features that can be implemented in this Framework?
  (code coverage, unit tests, stress testings, regression, generating Coccinelle reports etc.) 
4. Desirable Report generation mechanism?
5. Basic performance validation?
6. Whether QEMU can be used to emulate some of the H/W functionality to create a test 
  platform? (Optional subsystem specific)

Some background about myself I'm Chaitanya Kulkarni, I worked as a team lead 
which was responsible for delivering scalable multiplatform Automated Test 
Framework for device drivers testing at HGST. It's been used for more than 1 year on 
Linux/Windows for unit testing/regression/performance validation of the NVMe Linux and
Windows driver successfully. I've also recently started contributing to the 

NVMe Host and NVMe over Fabrics Target driver.

Regards,
-Chaitanya

     

^ permalink raw reply

* Re: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
From: Hannes Reinecke @ 2017-01-11  7:42 UTC (permalink / raw)
  To: Chaitanya Kulkarni, lsf-pc@lists.linux-foundation.org
  Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
	linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org,
	linux-ide@vger.kernel.org
In-Reply-To: <CO2PR04MB2184DB653C04FB620B41435486670@CO2PR04MB2184.namprd04.prod.outlook.com>

On 01/10/2017 11:40 PM, Chaitanya Kulkarni wrote:
> Resending it at as a plain text.
> 
> From: Chaitanya Kulkarni
> Sent: Tuesday, January 10, 2017 2:37 PM
> To: lsf-pc@lists.linux-foundation.org
> Cc: linux-fsdevel@vger.kernel.org; linux-block@vger.kernel.org; linux-nvme@lists.infradead.org; linux-scsi@vger.kernel.org; linux-ide@vger.kernel.org
> Subject: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
>   
> 
> Hi Folks,
> 
> I would like to propose a general discussion on Storage stack and device driver testing.
> 
> Purpose:-
> -------------
> The main objective of this discussion is to address the need for 
> a Unified Test Automation Framework which can be used by different subsystems
> in the kernel in order to improve the overall development and stability
> of the storage stack.
> 
> For Example:- 
> From my previous experience, I've worked on the NVMe driver testing last year and we
> have developed simple unit test framework
>  (https://github.com/linux-nvme/nvme-cli/tree/master/tests). 
> In current implementation Upstream NVMe Driver supports following subsystems:-
> 1. PCI Host.
> 2. RDMA Target.
> 3. Fiber Channel Target (in progress).
> Today due to lack of centralized automated test framework NVMe Driver testing is 
> scattered and performed using the combination of various utilities like nvme-cli/tests, 
> nvmet-cli, shell scripts (git://git.infradead.org/nvme-fabrics.git nvmf-selftests) etc.
> 
> In order to improve overall driver stability with various subsystems, it will be beneficial
> to have a Unified Test Automation Framework (UTAF) which will centralize overall
> testing. 
> 
> This topic will allow developers from various subsystem engage in the discussion about 
> how to collaborate efficiently instead of having discussions on lengthy email threads.
> 
> Participants:-
> ------------------
> I'd like to invite developers from different subsystems to discuss an approach towards 
> a unified testing methodology for storage stack and device drivers belongs to 
> different subsystems.
> 
> Topics for Discussion:-
> ------------------------------
> As a part of discussion following are some of the key points which we can focus on:-
> 1. What are the common components of the kernel used by the various subsystems?
> 2. What are the potential target drivers which can benefit from this approach? 
>   (e.g. NVMe, NVMe Over Fabric, Open Channel Solid State Drives etc.)
> 3. What are the desired features that can be implemented in this Framework?
>   (code coverage, unit tests, stress testings, regression, generating Coccinelle reports etc.) 
> 4. Desirable Report generation mechanism?
> 5. Basic performance validation?
> 6. Whether QEMU can be used to emulate some of the H/W functionality to create a test 
>   platform? (Optional subsystem specific)
> 
> Some background about myself I'm Chaitanya Kulkarni, I worked as a team lead 
> which was responsible for delivering scalable multiplatform Automated Test 
> Framework for device drivers testing at HGST. It's been used for more than 1 year on 
> Linux/Windows for unit testing/regression/performance validation of the NVMe Linux and
> Windows driver successfully. I've also recently started contributing to the 
> 
> NVMe Host and NVMe over Fabrics Target driver.
> 
Oh, yes, please.
That's a discussion I'd like to have, too.

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: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
From: Johannes Thumshirn @ 2017-01-11  9:19 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: lsf-pc@lists.linux-foundation.org, linux-fsdevel@vger.kernel.org,
	linux-block@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org
In-Reply-To: <CO2PR04MB2184DB653C04FB620B41435486670@CO2PR04MB2184.namprd04.prod.outlook.com>

On Tue, Jan 10, 2017 at 10:40:53PM +0000, Chaitanya Kulkarni wrote:
> Resending it at as a plain text.
> 
> From: Chaitanya Kulkarni
> Sent: Tuesday, January 10, 2017 2:37 PM
> To: lsf-pc@lists.linux-foundation.org
> Cc: linux-fsdevel@vger.kernel.org; linux-block@vger.kernel.org; linux-nvme@lists.infradead.org; linux-scsi@vger.kernel.org; linux-ide@vger.kernel.org
> Subject: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
>   
> 
> Hi Folks,
> 
> I would like to propose a general discussion on Storage stack and device driver testing.
> 
> Purpose:-
> -------------
> The main objective of this discussion is to address the need for 
> a Unified Test Automation Framework which can be used by different subsystems
> in the kernel in order to improve the overall development and stability
> of the storage stack.
> 
> For Example:- 
> From my previous experience, I've worked on the NVMe driver testing last year and we
> have developed simple unit test framework
>  (https://github.com/linux-nvme/nvme-cli/tree/master/tests). 
> In current implementation Upstream NVMe Driver supports following subsystems:-
> 1. PCI Host.
> 2. RDMA Target.
> 3. Fiber Channel Target (in progress).
> Today due to lack of centralized automated test framework NVMe Driver testing is 
> scattered and performed using the combination of various utilities like nvme-cli/tests, 
> nvmet-cli, shell scripts (git://git.infradead.org/nvme-fabrics.git nvmf-selftests) etc.
> 
> In order to improve overall driver stability with various subsystems, it will be beneficial
> to have a Unified Test Automation Framework (UTAF) which will centralize overall
> testing. 
> 
> This topic will allow developers from various subsystem engage in the discussion about 
> how to collaborate efficiently instead of having discussions on lengthy email threads.
> 
> Participants:-
> ------------------
> I'd like to invite developers from different subsystems to discuss an approach towards 
> a unified testing methodology for storage stack and device drivers belongs to 
> different subsystems.
> 
> Topics for Discussion:-
> ------------------------------
> As a part of discussion following are some of the key points which we can focus on:-
> 1. What are the common components of the kernel used by the various subsystems?
> 2. What are the potential target drivers which can benefit from this approach? 
>   (e.g. NVMe, NVMe Over Fabric, Open Channel Solid State Drives etc.)
> 3. What are the desired features that can be implemented in this Framework?
>   (code coverage, unit tests, stress testings, regression, generating Coccinelle reports etc.) 
> 4. Desirable Report generation mechanism?
> 5. Basic performance validation?
> 6. Whether QEMU can be used to emulate some of the H/W functionality to create a test 
>   platform? (Optional subsystem specific)

Well, something I was thinking about but didn't find enough time to actually
implement is making a xfstestes like test suite written using sg3_utils for
SCSI. This idea could very well be extented to NVMe, AHCI, blk, etc...

Byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

^ permalink raw reply

* Re: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
From: Christoph Hellwig @ 2017-01-11  9:24 UTC (permalink / raw)
  To: Johannes Thumshirn
  Cc: Chaitanya Kulkarni, linux-scsi@vger.kernel.org,
	linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	linux-ide@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	lsf-pc@lists.linux-foundation.org
In-Reply-To: <20170111091945.GD6286@linux-x5ow.site>

On Wed, Jan 11, 2017 at 10:19:45AM +0100, Johannes Thumshirn wrote:
> Well, something I was thinking about but didn't find enough time to actually
> implement is making a xfstestes like test suite written using sg3_utils for
> SCSI.

Ronnie's libiscsi testsuite can use SG_IO for a new years now:

https://github.com/sahlberg/libiscsi/tree/master/test-tool

and has been very useful to find bus in various protocol
implementations.

> This idea could very well be extented to NVMe

Chaitanya suite is doing something similar for NVMe, although the
coverage is still much more limited so far.

^ permalink raw reply

* Re: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
From: Hannes Reinecke @ 2017-01-11  9:40 UTC (permalink / raw)
  To: Christoph Hellwig, Johannes Thumshirn
  Cc: Chaitanya Kulkarni, linux-scsi@vger.kernel.org,
	linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	linux-ide@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	lsf-pc@lists.linux-foundation.org
In-Reply-To: <20170111092447.GA13600@infradead.org>

On 01/11/2017 10:24 AM, Christoph Hellwig wrote:
> On Wed, Jan 11, 2017 at 10:19:45AM +0100, Johannes Thumshirn wrote:
>> Well, something I was thinking about but didn't find enough time to actually
>> implement is making a xfstestes like test suite written using sg3_utils for
>> SCSI.
> 
> Ronnie's libiscsi testsuite can use SG_IO for a new years now:
> 
> https://github.com/sahlberg/libiscsi/tree/master/test-tool
> 
> and has been very useful to find bus in various protocol
> implementations.
> 
>> This idea could very well be extented to NVMe
> 
> Chaitanya suite is doing something similar for NVMe, although the
> coverage is still much more limited so far.
> 
One of the discussion points here indeed would be if we want to go in
the direction of a protocol-specific testsuites (of which we already
have several) or if it makes sense to move to functional testing.

And if we can have a common interface / documentation on how these
things should run.

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

* [PATCH] ahci: imx: fix building without hwmon or thermal
From: Arnd Bergmann @ 2017-01-11 13:36 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Arnd Bergmann, Bartlomiej Zolnierkiewicz, Fabien Lahoudere,
	linux-ide, linux-kernel

When CONFIG_HWMON is disabled, we now get a link failure:

ERROR: "devm_hwmon_device_register_with_groups" [drivers/ata/ahci_imx.ko] undefined!
drivers/ata/ahci_imx.o: In function `imx_ahci_probe':
ahci_imx.c:(.text.imx_ahci_probe+0x304): undefined reference to `devm_thermal_zone_of_sensor_register'

This makes the code calling into the hwmon subsystem compile-time
conditional, and adds a Kconfig dependency to avoid the corner
case of having HWMON=m and AHCI_IMX=y, by forcing AHCI_IMX=m in this
case. The thermal subsystem already has a check in its header, but
that also doesn't cover the THERMAL=m case, so we need a somewhat
complex Kconfig expression to handle all cases.

Fixes: 54643a83b41a ("ahci: imx: Add imx53 SATA temperature sensor support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/ata/Kconfig    | 1 +
 drivers/ata/ahci_imx.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 78c002021029..70b57d2229d6 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -129,6 +129,7 @@ config AHCI_ST
 config AHCI_IMX
 	tristate "Freescale i.MX AHCI SATA support"
 	depends on MFD_SYSCON && (ARCH_MXC || COMPILE_TEST)
+	depends on (HWMON && (THERMAL || !THERMAL_OF)) || !HWMON
 	help
 	  This option enables support for the Freescale i.MX SoC's
 	  onboard AHCI SATA.
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 420f065978dc..787567e840bd 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -774,7 +774,8 @@ static int imx_ahci_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (imxpriv->type == AHCI_IMX53) {
+	if (imxpriv->type == AHCI_IMX53 &&
+	    IS_ENABLED(CONFIG_HWMON)) {
 		/* Add the temperature monitor */
 		struct device *hwmon_dev;
 
-- 
2.9.0


^ permalink raw reply related

* Re: [PATCH] ahci: imx: fix building without hwmon or thermal
From: Bartlomiej Zolnierkiewicz @ 2017-01-11 14:16 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Tejun Heo, Fabien Lahoudere, linux-ide, linux-kernel
In-Reply-To: <20170111133652.3715437-1-arnd@arndb.de>


Hi,

On Wednesday, January 11, 2017 02:36:16 PM Arnd Bergmann wrote:
> When CONFIG_HWMON is disabled, we now get a link failure:
> 
> ERROR: "devm_hwmon_device_register_with_groups" [drivers/ata/ahci_imx.ko] undefined!
> drivers/ata/ahci_imx.o: In function `imx_ahci_probe':
> ahci_imx.c:(.text.imx_ahci_probe+0x304): undefined reference to `devm_thermal_zone_of_sensor_register'
> 
> This makes the code calling into the hwmon subsystem compile-time
> conditional, and adds a Kconfig dependency to avoid the corner
> case of having HWMON=m and AHCI_IMX=y, by forcing AHCI_IMX=m in this
> case. The thermal subsystem already has a check in its header, but
> that also doesn't cover the THERMAL=m case, so we need a somewhat
> complex Kconfig expression to handle all cases.
> 
> Fixes: 54643a83b41a ("ahci: imx: Add imx53 SATA temperature sensor support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Looks fine to me (I see that this is the same solution as for
TOUCHSCREEN_SUN4I from commit 4a6155a46565 but without hard
dependency on HWMON).

Thanks for fixing this.

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  drivers/ata/Kconfig    | 1 +
>  drivers/ata/ahci_imx.c | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 78c002021029..70b57d2229d6 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -129,6 +129,7 @@ config AHCI_ST
>  config AHCI_IMX
>  	tristate "Freescale i.MX AHCI SATA support"
>  	depends on MFD_SYSCON && (ARCH_MXC || COMPILE_TEST)
> +	depends on (HWMON && (THERMAL || !THERMAL_OF)) || !HWMON
>  	help
>  	  This option enables support for the Freescale i.MX SoC's
>  	  onboard AHCI SATA.
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index 420f065978dc..787567e840bd 100644
> --- a/drivers/ata/ahci_imx.c
> +++ b/drivers/ata/ahci_imx.c
> @@ -774,7 +774,8 @@ static int imx_ahci_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	if (imxpriv->type == AHCI_IMX53) {
> +	if (imxpriv->type == AHCI_IMX53 &&
> +	    IS_ENABLED(CONFIG_HWMON)) {
>  		/* Add the temperature monitor */
>  		struct device *hwmon_dev;


^ permalink raw reply

* Re: [PATCH] PCI:MSI Return -ENOSPC when requested vectors is not enough
From: Bjorn Helgaas @ 2017-01-11 18:18 UTC (permalink / raw)
  To: Dennis Chen
  Cc: linux-pci, linux-ide, Lorenzo Pieralisi, Steve Capper,
	Marc Zyngier, Tom Long Nguyen, Bjorn Helgaas, Greg Kroah-Hartman,
	Tejun Heo, nd, Christoph Hellwig, linux-arm-kernel
In-Reply-To: <1480558504-18691-1-git-send-email-dennis.chen@arm.com>

On Thu, Dec 01, 2016 at 10:15:04AM +0800, Dennis Chen wrote:
> The __pci_enable_msi_range() should return -ENOSPC instead of -EINVAL
> when the device doesn't have enough vectors as required, just as the 
> MSI-X vector allocator does in __pci_enable_msix_range(). Otherwise, 
> some drivers depending on that return value will probably fallback to
> the legacy interrupt directly, for example, in commit 17a51f12cfbd2814
> ("ahci: only try to use multi-MSI mode if there is more than 1 port"), the
> ahci driver will fallback to single MSI mode only when the return value
> is -ENOSPC in case of required vectors is not enough, else the driver will
> use legacy interrupt which has been observed on a x86 box with 6-port SATA
> controller.

Unless Christoph objects, I'll apply this, but I don't understand the
situation with 17a51f12cfbd.  That commit doesn't check for EINVAL or
ENOSPC so I don't know what the connection with this patch is.

I know Christoph said he changed something in ahci to treat all errors
the same, but I don't know where that is, either.

If there's a revision of Linus' tree that is broken, please give the
details so I can at least describe which versions are broken, when it
got fixed by Christoph, and figure out whether we need stable
backports or anything.

Bjorn

> With this patch, when a MSI-capable device doesn't have enough MSI
> vectors as requested, it will fallback to single MSI mode while not
> legacy interrupt.
> 
> Signed-off-by: Dennis Chen <dennis.chen@arm.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Tom Long Nguyen <tom.l.nguyen@intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Steve Capper <steve.capper@arm.com>
> Cc: linux-ide@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/pci/msi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index ad70507..da37113 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1084,7 +1084,7 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
>  	if (nvec < 0)
>  		return nvec;
>  	if (nvec < minvec)
> -		return -EINVAL;
> +		return -ENOSPC;
>  
>  	if (nvec > maxvec)
>  		nvec = maxvec;
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [Lsf-pc] [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
From: Sagi Grimberg @ 2017-01-12 11:01 UTC (permalink / raw)
  To: Chaitanya Kulkarni, lsf-pc@lists.linux-foundation.org
  Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
	linux-scsi@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-ide@vger.kernel.org
In-Reply-To: <CO2PR04MB218427BF42159B20FB26F74186670@CO2PR04MB2184.namprd04.prod.outlook.com>


> Hi Folks,
>
> I would like to propose a general discussion on Storage stack and device driver testing.

I think its very useful and needed.

> Purpose:-
> -------------
> The main objective of this discussion is to address the need for
> a Unified Test Automation Framework which can be used by different subsystems
> in the kernel in order to improve the overall development and stability
> of the storage stack.
>
> For Example:-
> From my previous experience, I've worked on the NVMe driver testing last year and we
> have developed simple unit test framework
>  (https://github.com/linux-nvme/nvme-cli/tree/master/tests).
> In current implementation Upstream NVMe Driver supports following subsystems:-
> 1. PCI Host.
> 2. RDMA Target.
> 3. Fiber Channel Target (in progress).
> Today due to lack of centralized automated test framework NVMe Driver testing is
> scattered and performed using the combination of various utilities like nvme-cli/tests,
> nvmet-cli, shell scripts (git://git.infradead.org/nvme-fabrics.git nvmf-selftests) etc.
>
> In order to improve overall driver stability with various subsystems, it will be beneficial
> to have a Unified Test Automation Framework (UTAF) which will centralize overall
> testing.
>
> This topic will allow developers from various subsystem engage in the discussion about
> how to collaborate efficiently instead of having discussions on lengthy email threads.

While a unified test framework for all sounds great, I suspect that the
difference might be too large. So I think that for this framework to be
maintainable, it needs to be carefully designed such that we don't have
too much code churn.

For example we should start by classifying tests and then see where
sharing is feasible:

1. basic management - I think not a lot can be shared
2. spec compliance - again, not much sharing here
3. data-verification - probably everything can be shared
4. basic performance - probably a lot can be shared
5. vectored-io - probably everything can be shared
6. error handling - I can think of some sharing that can be used.

This repository can also store some useful tracing scripts (ebpf and
friends) that are useful for performance analysis.

So I think that for this to happen, we can start with the shared
test under block/, then migrate proto specific tests into
scsi/, nvme/, and then add transport specific tests so
we can have something like:

├── block
├── lib
├── nvme
│   ├── fabrics
│   │   ├── loop
│   │   └── rdma
│   └── pci
└── scsi
     ├── fc
     └── iscsi

Thoughts?

^ permalink raw reply

* Re: [PATCH] PCI:MSI Return -ENOSPC when requested vectors is not enough
From: Christoph Hellwig @ 2017-01-12 13:42 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Dennis Chen, linux-pci, linux-ide, Lorenzo Pieralisi,
	Steve Capper, Marc Zyngier, Tom Long Nguyen, Bjorn Helgaas,
	Greg Kroah-Hartman, Tejun Heo, nd, Christoph Hellwig,
	linux-arm-kernel
In-Reply-To: <20170111181853.GA14532@bhelgaas-glaptop.roam.corp.google.com>

On Wed, Jan 11, 2017 at 12:18:53PM -0600, Bjorn Helgaas wrote:
> Unless Christoph objects, I'll apply this, but I don't understand the
> situation with 17a51f12cfbd.  That commit doesn't check for EINVAL or
> ENOSPC so I don't know what the connection with this patch is.

I don't think that commit is the culprit.

> I know Christoph said he changed something in ahci to treat all errors
> the same, but I don't know where that is, either.

"ahci: always fall back to single-MSI mode"

^ permalink raw reply

* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Finn Thain @ 2017-01-13  2:33 UTC (permalink / raw)
  To: Michael Schmitz
  Cc: Bartlomiej Zolnierkiewicz, Tejun Heo, Geert Uytterhoeven,
	linux-ide, Linux/m68k, Linux Kernel Development, Andreas Schwab
In-Reply-To: <CAOmrzkJfQu6PuCpyyRJL8PNvSYxYO42F-k8jEYf2d1-5C28A_A@mail.gmail.com>


On Wed, 11 Jan 2017, Michael Schmitz wrote:

> What is still correct is that the IDE driver does use the interrupt 
> only, not the ST-DMA chip. And a single IDE interrupt can be correctly 
> assigned to IDE by looking at the status register.
> 
> With the SCSI (and IIRC also floppy) interrupts, we don't have direct 
> access to the status registers without disturbing the state of the DMA 
> though. Unless we know for definite that either chips have raised the 
> interrupt (and DMA ops are in flight), we must not touch the DMA chip at 
> all.
> 
> The case I'm worried about is both IDE and SCSI raising an interrupt. We 
> don't currently mask the IDE/ST-DMA interrupt so a stacked interrupt 
> must be processed in the same pass as the initial interrupt or it will 
> get dropped. We'd have to peek at the DMA registers to check the SCSI or 
> floppy interrupt status, and we just can't safely do that. So races of 
> this kind are currently prevented by including IDE in the IRQ locking 
> process.
> 
> Whether it's possible to mask the interrupt, do one pass, unmask and 
> process the second interrupt I don't know.

Would that require handling the SCSI DMA interrupt in the first pass? Or 
handling IDE first, and ensuring that the IDE handler does not access 
ST-DMA registers? What about FDC?

The atari_scsi handler accesses the ST-DMA registers; it can do so because 
it knows that any DMA must have completed -- it can infer this because a 
simultaneous pending interrupt from FDC or IDE is impossible due to 
stdma_lock().

Your suggestion would seem to allow other pending interrupts, hence the 
atari_scsi interrupt handler logic has to be tossed out. What logic would 
replace it?

If all else fails, perhaps we could inhibit DMA entirely when the new ATA 
driver is loaded. Then we can just dispatch the ST-DMA irq like a shared 
irq. I'm sure that atari_scsi can work without DMA. No idea about the FDC 
driver though (ataflop.c).

Another solution would be to dedicate the DMA function to atari_scsi, and 
then mask the FDC and IDE interrupts during each DMA transfer. But once 
again, this would mean changing the FDC driver to eliminate DMA, if that 
is possible. From the schematic it looks the the FDC chip, "AJAX", is 
another custom ...
http://dev-docs.atariforge.org/files/Falcon030_Schematic.pdf

Unfortunately my grasp of the ST hardware reflects my inability to read 
German; those who can may want to take a look at "ATARI Profibuch 
ST-STE-TT.pdf".

-- 

> Maybe Andreas does?
> 
> Cheers,
> 
>   Michael
> 
> 
> > it should be okay to use IDE at the same time as SCSI/Floppy which is 
> > what the new driver does (the old one is serialized operations by 
> > ST-DMA related IRQ handling magic).
> >
> > Also the comment itself may need some fixups as on Falcon it is SCSI 
> > not ACSI (according to the earlier comment in same file) and the old 
> > IDE host driver name is not falhd.c but falconide.c.
> >
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* [PATCH 00/10] ARM: da850-lcdk: add SATA support
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree

This series contains all the changes necessary to make SATA work on
the da850-lcdk board.

The first patch adds a clock lookup entry required for the ahci core
to retrieve a functional clock.

The second enables relevant config options for all davinci boards.

The third adds device tree bindings for the ahci_da850 driver.

The fourth adds a workaround for a SATA controller instability we
detected after increasing the PLL0 frequency for proper LCD
controller support.

Patches 5 through 7 extend the ahci_da850 driver - add DT support,
un-hardcode the clock multiplier value and add a workaround for
a quirk present on the da850 SATA controller.

Patches 8-10 add the device tree changes required to probe the driver.

I'm posting the series as a whole to give all reviewers the full
picture and visibility of the changes required, if needed I can resend
the patches separately.

Bartosz Golaszewski (10):
  ARM: davinci: add a clock lookup entry for the SATA clock
  ARM: davinci_all_defconfig: enable SATA modules
  devicetree: bindings: add bindings for ahci-da850
  sata: hardreset: retry if phys link is down
  sata: ahci_da850: add device tree match table
  sata: ahci_da850: implement a softreset quirk
  sata: ahci_da850: add support for the da850,clk_multiplier DT property
  ARM: dts: da850: add pinmux settings for the SATA controller
  ARM: dts: da850: add the SATA node
  ARM: dts: da850-lcdk: enable the SATA node

 .../devicetree/bindings/ata/ahci-da850.txt         |  21 ++++
 arch/arm/boot/dts/da850-lcdk.dts                   |   5 +
 arch/arm/boot/dts/da850.dtsi                       |  30 ++++++
 arch/arm/configs/davinci_all_defconfig             |   2 +
 arch/arm/mach-davinci/da8xx-dt.c                   |   1 +
 drivers/ata/ahci_da850.c                           | 112 +++++++++++++++++++--
 drivers/ata/libata-core.c                          |  16 ++-
 include/linux/libata.h                             |   4 +-
 8 files changed, 177 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt

-- 
2.9.3

^ permalink raw reply

* [PATCH 01/10] ARM: davinci: add a clock lookup entry for the SATA clock
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

This entry is needed for the ahci driver to get a functional clock.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da8xx-dt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index 9ee44da..b83e5d1 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -42,6 +42,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("ti,da830-ohci", 0x01e25000, "ohci-da8xx", NULL),
 	OF_DEV_AUXDATA("ti,da830-musb", 0x01e00000, "musb-da8xx", NULL),
 	OF_DEV_AUXDATA("ti,da830-usb-phy", 0x01c1417c, "da8xx-usb-phy", NULL),
+	OF_DEV_AUXDATA("ti,da850-ahci", 0x01e18000, "ahci_da850", NULL),
 	{}
 };
 
-- 
2.9.3

^ permalink raw reply related

* [PATCH 02/10] ARM: davinci_all_defconfig: enable SATA modules
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

Add the da850-ahci driver to davinci defconfig.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/configs/davinci_all_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index 8806754..a1b9c58 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -78,6 +78,8 @@ CONFIG_IDE=m
 CONFIG_BLK_DEV_PALMCHIP_BK3710=m
 CONFIG_SCSI=m
 CONFIG_BLK_DEV_SD=m
+CONFIG_ATA=m
+CONFIG_AHCI_DA850=m
 CONFIG_NETDEVICES=y
 CONFIG_NETCONSOLE=y
 CONFIG_TUN=m
-- 
2.9.3

^ permalink raw reply related

* [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

Add DT bindings for the TI DA850 AHCI SATA controller.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../devicetree/bindings/ata/ahci-da850.txt          | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt

diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt b/Documentation/devicetree/bindings/ata/ahci-da850.txt
new file mode 100644
index 0000000..d07c241
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
@@ -0,0 +1,21 @@
+Device tree binding for the TI DA850 AHCI SATA Controller
+---------------------------------------------------------
+
+Required properties:
+  - compatible: must be "ti,da850-ahci"
+  - reg: physical base addresses and sizes of the controller's register areas
+  - interrupts: interrupt specifier (refer to the interrupt binding)
+
+Optional properties:
+  - clocks: clock specifier (refer to the common clock binding)
+  - da850,clk_multiplier: the multiplier for the reference clock needed
+                          for 1.5GHz PLL output
+
+Example:
+
+	sata: ahci@0x218000 {
+		compatible = "ti,da850-ahci";
+		reg = <0x218000 0x2000>, <0x22c018 0x4>;
+		interrupts = <67>;
+		da850,clk_multiplier = <7>;
+	};
-- 
2.9.3

^ permalink raw reply related

* [PATCH 04/10] sata: hardreset: retry if phys link is down
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

The sata core driver already retries to resume the link because some
controllers ignore writes to the SControl register.

We have a use case with the da850 SATA controller where at PLL0
frequency of 456MHz (needed to properly service the LCD controller)
the chip becomes unstable and the hardreset operation is ignored the
first time 50% of times.

Retrying just the resume operation doesn't work - we need to issue
the phy/wake reset again to make it work.

If ata_phys_link_offline() returns true in sata_link_hardreset(),
retry a couple times before really giving up.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/libata-core.c | 16 ++++++++++++----
 include/linux/libata.h    |  4 +++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9cd0a2d..3b848a3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3985,8 +3985,8 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 			unsigned long deadline,
 			bool *online, int (*check_ready)(struct ata_link *))
 {
+	int rc, retry = ATA_LINK_RESET_TRIES;
 	u32 scontrol;
-	int rc;
 
 	DPRINTK("ENTER\n");
 
@@ -4009,7 +4009,7 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 
 		sata_set_spd(link);
 	}
-
+retry:
 	/* issue phy wake/reset */
 	if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
 		goto out;
@@ -4028,9 +4028,17 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 	rc = sata_link_resume(link, timing, deadline);
 	if (rc)
 		goto out;
-	/* if link is offline nothing more to do */
-	if (ata_phys_link_offline(link))
+
+	if (ata_phys_link_offline(link)) {
+		if (retry--) {
+			ata_link_warn(link,
+				      "link still offline after hardreset - retrying\n");
+			goto retry;
+		}
+
+		/* if link is still offline nothing more to do */
 		goto out;
+	}
 
 	/* Link is online.  From this point, -ENODEV too is an error. */
 	if (online)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c170be5..2c840c0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -392,8 +392,10 @@ enum {
 	/* max tries if error condition is still set after ->error_handler */
 	ATA_EH_MAX_TRIES	= 5,
 
-	/* sometimes resuming a link requires several retries */
+	/* sometimes resuming a link requires several retries... */
 	ATA_LINK_RESUME_TRIES	= 5,
+	/* ... and sometimes we need to retry the whole reset procedure */
+	ATA_LINK_RESET_TRIES	= 5,
 
 	/* how hard are we gonna try to probe/recover devices */
 	ATA_PROBE_MAX_TRIES	= 3,
-- 
2.9.3

^ permalink raw reply related

* [PATCH 05/10] sata: ahci_da850: add device tree match table
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

We're using device tree for da850-lcdk. Add the match table to allow
to probe the driver.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/ahci_da850.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index 267a3d3..5930af81 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -105,11 +105,18 @@ static int ahci_da850_probe(struct platform_device *pdev)
 static SIMPLE_DEV_PM_OPS(ahci_da850_pm_ops, ahci_platform_suspend,
 			 ahci_platform_resume);
 
+static const struct of_device_id ahci_da850_of_match[] = {
+	{ .compatible = "ti,da850-ahci", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ahci_da850_of_match);
+
 static struct platform_driver ahci_da850_driver = {
 	.probe = ahci_da850_probe,
 	.remove = ata_platform_remove_one,
 	.driver = {
 		.name = DRV_NAME,
+		.of_match_table = ahci_da850_of_match,
 		.pm = &ahci_da850_pm_ops,
 	},
 };
-- 
2.9.3

^ permalink raw reply related

* [PATCH 06/10] sata: ahci_da850: implement a softreset quirk
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

There's an issue with the da850 SATA controller: if port multiplier
support is compiled in, but we're connecting the drive directly to
the SATA port on the board, the drive can't be detected.

To make SATA work on the da850-lcdk board: first try to softreset
with pmp - if the operation fails with -EBUSY, retry without pmp.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/ahci_da850.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index 5930af81..bb9eb4c 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -54,11 +54,31 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
 	writel(val, ahci_base + SATA_P0PHYCR_REG);
 }
 
+static int ahci_da850_softreset(struct ata_link *link,
+				unsigned int *class, unsigned long deadline)
+{
+	int pmp, ret;
+
+	pmp = sata_srst_pmp(link);
+
+	ret = ahci_do_softreset(link, class, pmp, deadline, ahci_check_ready);
+	if (pmp && ret == -EBUSY)
+		return ahci_do_softreset(link, class, 0,
+					 deadline, ahci_check_ready);
+
+	return ret;
+}
+
+static struct ata_port_operations ahci_da850_port_ops = {
+	.inherits = &ahci_platform_ops,
+	.softreset = ahci_da850_softreset,
+};
+
 static const struct ata_port_info ahci_da850_port_info = {
 	.flags		= AHCI_FLAG_COMMON,
 	.pio_mask	= ATA_PIO4,
 	.udma_mask	= ATA_UDMA6,
-	.port_ops	= &ahci_platform_ops,
+	.port_ops	= &ahci_da850_port_ops,
 };
 
 static struct scsi_host_template ahci_platform_sht = {
-- 
2.9.3

^ permalink raw reply related

* [PATCH 07/10] sata: ahci_da850: add support for the da850, clk_multiplier DT property
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
	devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

Currently the clock multiplier is hardcoded in the driver for
the da850-evm board. Make it configurable over DT, but keep the
previous value as default in case the property is missing.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/ahci_da850.c | 83 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index bb9eb4c..cd04caf 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -28,17 +28,70 @@
 #define SATA_PHY_TXSWING(x)	((x) << 19)
 #define SATA_PHY_ENPLL(x)	((x) << 31)
 
+struct da850_sata_mpy_mapping {
+	unsigned int multiplier;
+	unsigned int regval;
+};
+
+static const struct da850_sata_mpy_mapping da850_sata_mpy_table[] = {
+	{
+		.multiplier	= 5,
+		.regval		= 0x01,
+	},
+	{
+		.multiplier	= 6,
+		.regval		= 0x02,
+	},
+	{
+		.multiplier	= 8,
+		.regval		= 0x04,
+	},
+	{
+		.multiplier	= 10,
+		.regval		= 0x05,
+	},
+	{
+		.multiplier	= 12,
+		.regval		= 0x06,
+	},
+	/* TODO Add 12.5 multiplier. */
+	{
+		.multiplier	= 15,
+		.regval		= 0x08,
+	},
+	{
+		.multiplier	= 20,
+		.regval		= 0x09,
+	},
+	{
+		.multiplier	= 25,
+		.regval		= 0x0a,
+	}
+};
+
+static const struct da850_sata_mpy_mapping *
+da850_sata_get_mpy(unsigned int multiplier)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da850_sata_mpy_table); i++)
+		if (da850_sata_mpy_table[i].multiplier == multiplier)
+			return &da850_sata_mpy_table[i];
+
+	return NULL;
+}
+
 /*
  * The multiplier needed for 1.5GHz PLL output.
  *
- * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
- * frequency (which is used by DA850 EVM board) and may need to be changed
- * if you would like to use this driver on some other board.
+ * This is the default value suitable for the 100MHz crystal frequency
+ * used by DA850 EVM board, which doesn't use DT.
  */
-#define DA850_SATA_CLK_MULTIPLIER	7
+#define DA850_SATA_CLK_MULTIPLIER_DEFAULT	15
 
 static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
-			    void __iomem *ahci_base)
+			    void __iomem *ahci_base,
+			    const struct da850_sata_mpy_mapping *mpy)
 {
 	unsigned int val;
 
@@ -47,7 +100,7 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
 	val &= ~BIT(0);
 	writel(val, pwrdn_reg);
 
-	val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
+	val = SATA_PHY_MPY(mpy->regval) | SATA_PHY_LOS(1) |
 	      SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
 	      SATA_PHY_ENPLL(1);
 
@@ -87,10 +140,12 @@ static struct scsi_host_template ahci_platform_sht = {
 
 static int ahci_da850_probe(struct platform_device *pdev)
 {
+	const struct da850_sata_mpy_mapping *mpy;
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
-	struct resource *res;
+	unsigned int multiplier;
 	void __iomem *pwrdn_reg;
+	struct resource *res;
 	int rc;
 
 	hpriv = ahci_platform_get_resources(pdev);
@@ -109,7 +164,19 @@ static int ahci_da850_probe(struct platform_device *pdev)
 	if (!pwrdn_reg)
 		goto disable_resources;
 
-	da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
+	rc = of_property_read_u32(dev->of_node,
+				  "da850,clk_multiplier", &multiplier);
+	if (rc)
+		multiplier = DA850_SATA_CLK_MULTIPLIER_DEFAULT;
+
+	mpy = da850_sata_get_mpy(multiplier);
+	if (!mpy) {
+		dev_err(dev, "invalid multiplier value: %u\n", multiplier);
+		rc = -EINVAL;
+		goto disable_resources;
+	}
+
+	da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
 
 	rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
 				     &ahci_platform_sht);
-- 
2.9.3

^ permalink raw reply related

* [PATCH 09/10] ARM: dts: da850: add the SATA node
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel,
	Bartosz Golaszewski
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

Add the SATA node to the da850 device tree.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1f6a47d..f5086b1 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -427,6 +427,12 @@
 			phy-names = "usb-phy";
 			status = "disabled";
 		};
+		sata: ahci@0x218000 {
+			compatible = "ti,da850-ahci";
+			reg = <0x218000 0x2000>, <0x22c018 0x4>;
+			interrupts = <67>;
+			status = "disabled";
+		};
 		mdio: mdio@224000 {
 			compatible = "ti,davinci_mdio";
 			#address-cells = <1>;
-- 
2.9.3

^ permalink raw reply related

* [PATCH 10/10] ARM: dts: da850-lcdk: enable the SATA node
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Bartosz Golaszewski
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

Enable the SATA node for da850-lcdk. We omit the pinctrl property on
purpose - the muxed SATA pins are not hooked up to anything
SATA-related on the lcdk.

The REFCLKN/P rate on the board is 100MHz, so we need a multiplier of
15 for 1.5GHz PLL rate.

Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 arch/arm/boot/dts/da850-lcdk.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index afcb482..1e638da 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -105,6 +105,11 @@
 	status = "okay";
 };
 
+&sata {
+	status = "okay";
+	da850,clk_multiplier = <15>;
+};
+
 &mdio {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mdio_pins>;
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH 08/10] ARM: dts: da850: add pinmux settings for the SATA controller
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
  To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
	Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
  Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel,
	Bartosz Golaszewski
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

Add pinmux sub-nodes for all muxed SATA pins.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 104155d..1f6a47d 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -78,6 +78,30 @@
 					0x10 0x00220000 0x00ff0000
 				>;
 			};
+			sata_cp_det_pin: pinmux_sata_cp_det_pin {
+				pinctrl-single,bits = <
+					/* SATA_CP_DET */
+					0x0c 0x00000000 0xf0000000
+				>;
+			};
+			sata_mp_switch_pin: pinmux_sata_mp_switch_pin {
+				pinctrl-single,bits = <
+					/* SATA_MP_SWITCH */
+					0x0c 0x00000000 0x0f000000
+				>;
+			};
+			sata_cp_pod_pin: pinmux_sata_cp_pod_pin {
+				pinctrl-single,bits = <
+					/* SATA_CP_POD */
+					0x10 0x40000000 0xf0000000
+				>;
+			};
+			sata_led_pin: pinmux_sata_led_pin {
+				pinctrl-single,bits = <
+					/* SATA_LED */
+					0x10 0x04000000 0x0f000000
+				>;
+			};
 			i2c0_pins: pinmux_i2c0_pins {
 				pinctrl-single,bits = <
 					/* I2C0_SDA,I2C0_SCL */
-- 
2.9.3


^ permalink raw reply related

* Re: [PATCH 00/10] ARM: da850-lcdk: add SATA support
From: Sekhar Nori @ 2017-01-13 14:32 UTC (permalink / raw)
  To: Bartosz Golaszewski, Kevin Hilman, Patrick Titiano,
	Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
	Russell King, David Lechner
  Cc: linux-ide, linux-kernel, linux-arm-kernel, devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>

On Friday 13 January 2017 06:07 PM, Bartosz Golaszewski wrote:
> This series contains all the changes necessary to make SATA work on
> the da850-lcdk board.
> 
> The first patch adds a clock lookup entry required for the ahci core
> to retrieve a functional clock.
> 
> The second enables relevant config options for all davinci boards.
> 
> The third adds device tree bindings for the ahci_da850 driver.
> 
> The fourth adds a workaround for a SATA controller instability we
> detected after increasing the PLL0 frequency for proper LCD
> controller support.
> 
> Patches 5 through 7 extend the ahci_da850 driver - add DT support,
> un-hardcode the clock multiplier value and add a workaround for
> a quirk present on the da850 SATA controller.
> 
> Patches 8-10 add the device tree changes required to probe the driver.
> 
> I'm posting the series as a whole to give all reviewers the full
> picture and visibility of the changes required, if needed I can resend
> the patches separately.

I just tested this series on my LCDK board using a Western Digital SATA
HDD and it works great with some basic read / write tests. Thanks!

For the non-platform patches which I wont be queuing:

Tested-by: Sekhar Nori <nsekhar@ti.com>

I will take a look at the series closely next week.

Thanks,
Sekhar

^ permalink raw reply

* Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850
From: David Lechner @ 2017-01-13 19:25 UTC (permalink / raw)
  To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
	Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-ide-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1484311084-31547-4-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

On 01/13/2017 06:37 AM, Bartosz Golaszewski wrote:
> Add DT bindings for the TI DA850 AHCI SATA controller.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  .../devicetree/bindings/ata/ahci-da850.txt          | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> new file mode 100644
> index 0000000..d07c241
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> @@ -0,0 +1,21 @@
> +Device tree binding for the TI DA850 AHCI SATA Controller
> +---------------------------------------------------------
> +
> +Required properties:
> +  - compatible: must be "ti,da850-ahci"
> +  - reg: physical base addresses and sizes of the controller's register areas
> +  - interrupts: interrupt specifier (refer to the interrupt binding)
> +
> +Optional properties:
> +  - clocks: clock specifier (refer to the common clock binding)
> +  - da850,clk_multiplier: the multiplier for the reference clock needed
> +                          for 1.5GHz PLL output

A clock multiplier property seems redundant if you are specifying a 
clock. It should be possible to get the rate from the clock to determine 
which multiplier is needed.

> +
> +Example:
> +
> +	sata: ahci@0x218000 {
> +		compatible = "ti,da850-ahci";
> +		reg = <0x218000 0x2000>, <0x22c018 0x4>;
> +		interrupts = <67>;
> +		da850,clk_multiplier = <7>;
> +	};
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 07/10] sata: ahci_da850: add support for the da850,clk_multiplier DT property
From: David Lechner @ 2017-01-13 19:29 UTC (permalink / raw)
  To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
	Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-ide, linux-kernel, linux-arm-kernel, devicetree
In-Reply-To: <1484311084-31547-8-git-send-email-bgolaszewski@baylibre.com>

On 01/13/2017 06:38 AM, Bartosz Golaszewski wrote:
> Currently the clock multiplier is hardcoded in the driver for
> the da850-evm board. Make it configurable over DT, but keep the
> previous value as default in case the property is missing.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/ata/ahci_da850.c | 83 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
> index bb9eb4c..cd04caf 100644
> --- a/drivers/ata/ahci_da850.c
> +++ b/drivers/ata/ahci_da850.c
> @@ -28,17 +28,70 @@
>  #define SATA_PHY_TXSWING(x)	((x) << 19)
>  #define SATA_PHY_ENPLL(x)	((x) << 31)
>
> +struct da850_sata_mpy_mapping {
> +	unsigned int multiplier;
> +	unsigned int regval;
> +};
> +
> +static const struct da850_sata_mpy_mapping da850_sata_mpy_table[] = {
> +	{
> +		.multiplier	= 5,
> +		.regval		= 0x01,
> +	},
> +	{
> +		.multiplier	= 6,
> +		.regval		= 0x02,
> +	},
> +	{
> +		.multiplier	= 8,
> +		.regval		= 0x04,
> +	},
> +	{
> +		.multiplier	= 10,
> +		.regval		= 0x05,
> +	},
> +	{
> +		.multiplier	= 12,
> +		.regval		= 0x06,
> +	},
> +	/* TODO Add 12.5 multiplier. */

Looks like you should be using an enum here for the multiplier field.

> +	{
> +		.multiplier	= 15,
> +		.regval		= 0x08,
> +	},
> +	{
> +		.multiplier	= 20,
> +		.regval		= 0x09,
> +	},
> +	{
> +		.multiplier	= 25,
> +		.regval		= 0x0a,
> +	}
> +};
> +
> +static const struct da850_sata_mpy_mapping *
> +da850_sata_get_mpy(unsigned int multiplier)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(da850_sata_mpy_table); i++)
> +		if (da850_sata_mpy_table[i].multiplier == multiplier)
> +			return &da850_sata_mpy_table[i];
> +
> +	return NULL;
> +}
> +
>  /*
>   * The multiplier needed for 1.5GHz PLL output.
>   *
> - * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
> - * frequency (which is used by DA850 EVM board) and may need to be changed
> - * if you would like to use this driver on some other board.
> + * This is the default value suitable for the 100MHz crystal frequency
> + * used by DA850 EVM board, which doesn't use DT.

As I said in a reply on another patch, it seems like it would be better 
to use a clock that represents the crystal and use clk_get_rate() and 
calculate the multiplier from that.

For example, we have done something like this in 
usb20_phy_clk_set_parent() in arch/arm/mach-davinci/usb-da8xx.c.

>   */
> -#define DA850_SATA_CLK_MULTIPLIER	7
> +#define DA850_SATA_CLK_MULTIPLIER_DEFAULT	15
>
>  static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
> -			    void __iomem *ahci_base)
> +			    void __iomem *ahci_base,
> +			    const struct da850_sata_mpy_mapping *mpy)
>  {
>  	unsigned int val;
>
> @@ -47,7 +100,7 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
>  	val &= ~BIT(0);
>  	writel(val, pwrdn_reg);
>
> -	val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
> +	val = SATA_PHY_MPY(mpy->regval) | SATA_PHY_LOS(1) |
>  	      SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
>  	      SATA_PHY_ENPLL(1);
>
> @@ -87,10 +140,12 @@ static struct scsi_host_template ahci_platform_sht = {
>
>  static int ahci_da850_probe(struct platform_device *pdev)
>  {
> +	const struct da850_sata_mpy_mapping *mpy;
>  	struct device *dev = &pdev->dev;
>  	struct ahci_host_priv *hpriv;
> -	struct resource *res;
> +	unsigned int multiplier;
>  	void __iomem *pwrdn_reg;
> +	struct resource *res;
>  	int rc;
>
>  	hpriv = ahci_platform_get_resources(pdev);
> @@ -109,7 +164,19 @@ static int ahci_da850_probe(struct platform_device *pdev)
>  	if (!pwrdn_reg)
>  		goto disable_resources;
>
> -	da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
> +	rc = of_property_read_u32(dev->of_node,
> +				  "da850,clk_multiplier", &multiplier);
> +	if (rc)
> +		multiplier = DA850_SATA_CLK_MULTIPLIER_DEFAULT;
> +
> +	mpy = da850_sata_get_mpy(multiplier);
> +	if (!mpy) {
> +		dev_err(dev, "invalid multiplier value: %u\n", multiplier);
> +		rc = -EINVAL;
> +		goto disable_resources;
> +	}
> +
> +	da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
>
>  	rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
>  				     &ahci_platform_sht);
>

^ 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