[PATCH 1/2] ufs-pltfrm: initialize DMA mask for device-tree probed device

[PATCH 1/2] ufs-pltfrm: initialize DMA mask for device-tree probed device

[Akinobu Mita]

The device-tree probed device for ARM doesn't have dev->dma_mask.
So dma_set_mask() for the device doesn't succeed.  The popular trick
for this is - dev->dma_mask = &dev->coherent_dma_mask;

Currently there is no dma_set_mask() call in ufs-pltfrm, but the
forthcoming fix needs proper DMA mask setting in ufs core driver.  So
initializing dev->dma_mask as described above is required.

Signed-off-by: Akinobu Mita <mita@fixstars.com>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: Santosh Y <santoshsy@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/ufs/ufshcd-pltfrm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 94ba40c..c780840 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -122,6 +122,9 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
 		goto out;
 	}
 
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+
 	err = ufshcd_init(dev, &hba, mmio_base, irq);
 	if (err) {
 		dev_err(dev, "Intialization failed\n");
-- 
1.8.3.1

[PATCH 2/2] ufs: fix DMA mask setting

[Akinobu Mita]

If the controller doesn't support 64-bit addressing mode, it must not
set the DMA mask to 64-bit.  But it's unconditionally trying to set to
64-bit without checking 64-bit addressing support in the controller
capabilities.

It was correctly checked before commit 3b1d05807a9a68c6d0580e9248247a774a4d3be6
("[SCSI] ufs: Segregate PCI Specific Code"), this aims to restores
the correct behaviour.

To achieve this in a generic way, firstly we should push down the DMA
mask setting routine ufshcd_set_dma_mask() from PCI glue driver to core
driver in order to do it for both PCI glue driver and Platform glue
driver.  Secondly, we should change pci_ DMA mapping API to dma_ DMA
mapping API because core driver is independent of glue drivers.

Signed-off-by: Akinobu Mita <mita@fixstars.com>
Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
Cc: Vinayak Holikatti <vinholikatti@gmail.com>
Cc: Santosh Y <santoshsy@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
This patch was rejected because of the incorrect usage of dma_set_mask().
Now it is fixed, but it also requires ufshcd-pltfrm change in the separate
patch 1/2.

In order to apply this patch cleanly, this patch depends on 
'ufshcd-pci: release ioremapped region during removing driver' which
has already acked by Santosh.

 drivers/scsi/ufs/ufshcd-pci.c | 26 --------------------------
 drivers/scsi/ufs/ufshcd.c     | 30 ++++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 2349c0e..91a2e79 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -96,26 +96,6 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
 }
 
 /**
- * ufshcd_set_dma_mask - Set dma mask based on the controller
- *			 addressing capability
- * @pdev: PCI device structure
- *
- * Returns 0 for success, non-zero for failure
- */
-static int ufshcd_set_dma_mask(struct pci_dev *pdev)
-{
-	int err;
-
-	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(64))
-		&& !pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)))
-		return 0;
-	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
-	if (!err)
-		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
-	return err;
-}
-
-/**
  * ufshcd_pci_probe - probe routine of the driver
  * @pdev: pointer to PCI device handle
  * @id: PCI device id
@@ -145,12 +125,6 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	mmio_base = pcim_iomap_table(pdev)[0];
 
-	err = ufshcd_set_dma_mask(pdev);
-	if (err) {
-		dev_err(&pdev->dev, "set dma mask failed\n");
-		return err;
-	}
-
 	err = ufshcd_init(&pdev->dev, &hba, mmio_base, pdev->irq);
 	if (err) {
 		dev_err(&pdev->dev, "Initialization failed\n");
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index bd4d418..1f37c36 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1667,6 +1667,30 @@ void ufshcd_remove(struct ufs_hba *hba)
 EXPORT_SYMBOL_GPL(ufshcd_remove);
 
 /**
+ * ufshcd_set_dma_mask - Set dma mask based on the controller
+ *			 addressing capability
+ * @hba: per adapter instance
+ *
+ * Returns 0 for success, non-zero for failure
+ */
+static int ufshcd_set_dma_mask(struct ufs_hba *hba)
+{
+	int err;
+
+	if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
+		if (!dma_set_mask(hba->dev, DMA_BIT_MASK(64))) {
+			dma_set_coherent_mask(hba->dev, DMA_BIT_MASK(64));
+			return 0;
+		}
+	}
+	err = dma_set_mask(hba->dev, DMA_BIT_MASK(32));
+	if (!err)
+		dma_set_coherent_mask(hba->dev, DMA_BIT_MASK(32));
+
+	return err;
+}
+
+/**
  * ufshcd_init - Driver initialization routine
  * @dev: pointer to device handle
  * @hba_handle: driver private handle
@@ -1717,6 +1741,12 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
 	/* Get Interrupt bit mask per version */
 	hba->intr_mask = ufshcd_get_intr_mask(hba);
 
+	err = ufshcd_set_dma_mask(hba);
+	if (err) {
+		dev_err(hba->dev, "set dma mask failed\n");
+		goto out_disable;
+	}
+
 	/* Allocate memory for host memory space */
 	err = ufshcd_memory_alloc(hba);
 	if (err) {
-- 
1.8.3.1

Re: [PATCH 1/2] ufs-pltfrm: initialize DMA mask for device-tree probed device

[James Bottomley]

On Mon, 2013-08-19 at 22:56 +0900, Akinobu Mita wrote:
> The device-tree probed device for ARM doesn't have dev->dma_mask.
> So dma_set_mask() for the device doesn't succeed.  The popular trick
> for this is - dev->dma_mask = &dev->coherent_dma_mask;
> 
> Currently there is no dma_set_mask() call in ufs-pltfrm, but the
> forthcoming fix needs proper DMA mask setting in ufs core driver.  So
> initializing dev->dma_mask as described above is required.
> 
> Signed-off-by: Akinobu Mita <mita@fixstars.com>
> Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Cc: Vinayak Holikatti <vinholikatti@gmail.com>
> Cc: Santosh Y <santoshsy@gmail.com>
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/ufs/ufshcd-pltfrm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 94ba40c..c780840 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -122,6 +122,9 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
>  		goto out;
>  	}
>  
> +	if (!dev->dma_mask)
> +		dev->dma_mask = &dev->coherent_dma_mask;
> +

If the DMA mask is NULL, it means there's buggy platform code somewhere;
I'm not sure we should be hacking a fix in a SCSI driver.

James

Re: [PATCH 1/2] ufs-pltfrm: initialize DMA mask for device-tree probed device

[Sujit Reddy Thumma]

On 8/19/2013 8:02 PM, James Bottomley wrote:
> On Mon, 2013-08-19 at 22:56 +0900, Akinobu Mita wrote:
>> The device-tree probed device for ARM doesn't have dev->dma_mask.
>> So dma_set_mask() for the device doesn't succeed.  The popular trick
>> for this is - dev->dma_mask = &dev->coherent_dma_mask;
>>
>> Currently there is no dma_set_mask() call in ufs-pltfrm, but the
>> forthcoming fix needs proper DMA mask setting in ufs core driver.  So
>> initializing dev->dma_mask as described above is required.
>>
>> Signed-off-by: Akinobu Mita <mita@fixstars.com>
>> Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
>> Cc: Vinayak Holikatti <vinholikatti@gmail.com>
>> Cc: Santosh Y <santoshsy@gmail.com>
>> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
>> Cc: linux-scsi@vger.kernel.org
>> ---
>>   drivers/scsi/ufs/ufshcd-pltfrm.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index 94ba40c..c780840 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -122,6 +122,9 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
>>   		goto out;
>>   	}
>>
>> +	if (!dev->dma_mask)
>> +		dev->dma_mask = &dev->coherent_dma_mask;
>> +
>
> If the DMA mask is NULL, it means there's buggy platform code somewhere;
> I'm not sure we should be hacking a fix in a SCSI driver.

Yes, ideally DT core should do this, there are patches lying around but
are not converged. Adding devicetree@vger.kernel.org to see if someone
has better suggestions.

Recent additions to kernel with similar hacks -
https://patchwork.kernel.org/patch/2537021/


-- 
Regards,
Sujit

Re: [PATCH 1/2] ufs-pltfrm: initialize DMA mask for device-tree probed device

[Akinobu Mita]

> On 8/19/2013 8:02 PM, James Bottomley wrote:
>> On Mon, 2013-08-19 at 22:56 +0900, Akinobu Mita wrote:
>>> The device-tree probed device for ARM doesn't have dev->dma_mask.
>>> So dma_set_mask() for the device doesn't succeed.  The popular trick
>>> for this is - dev->dma_mask = &dev->coherent_dma_mask;
>>>
>>> Currently there is no dma_set_mask() call in ufs-pltfrm, but the
>>> forthcoming fix needs proper DMA mask setting in ufs core driver.  So
>>> initializing dev->dma_mask as described above is required.
>>>
>>> Signed-off-by: Akinobu Mita <mita@fixstars.com>
>>> Cc: Sujit Reddy Thumma <sthumma@codeaurora.org>
>>> Cc: Vinayak Holikatti <vinholikatti@gmail.com>
>>> Cc: Santosh Y <santoshsy@gmail.com>
>>> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
>>> Cc: linux-scsi@vger.kernel.org
>>> ---
>>>   drivers/scsi/ufs/ufshcd-pltfrm.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c
>>> b/drivers/scsi/ufs/ufshcd-pltfrm.c
>>> index 94ba40c..c780840 100644
>>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>>> @@ -122,6 +122,9 @@ static int ufshcd_pltfrm_probe(struct
>>> platform_device *pdev)
>>>   goto out;
>>>   }
>>>
>>> +if (!dev->dma_mask)
>>> +dev->dma_mask = &dev->coherent_dma_mask;
>>> +
>>
>> If the DMA mask is NULL, it means there's buggy platform code somewhere;
>> I'm not sure we should be hacking a fix in a SCSI driver.
>
> Yes, ideally DT core should do this, there are patches lying around but
> are not converged. Adding devicetree@vger.kernel.org to see if someone
> has better suggestions.
>
> Recent additions to kernel with similar hacks -
> https://patchwork.kernel.org/patch/2537021/

The discussion in that thread is useful.  Also, I found that Russell King
proposed replacing the boilerplate by using dma_coerce_mask_and_coherent()
in his patch set "Preview of DMA mask changes".
https://patchwork.kernel.org/patch/2837359/

Re: [PATCH 1/2] ufs-pltfrm: initialize DMA mask for device-tree probed device

[Russell King - ARM Linux]

On Wed, Aug 21, 2013 at 12:03:50AM +0900, Akinobu Mita wrote:
> The discussion in that thread is useful.  Also, I found that Russell King
> proposed replacing the boilerplate by using dma_coerce_mask_and_coherent()
> in his patch set "Preview of DMA mask changes".
> https://patchwork.kernel.org/patch/2837359/

That is an attempt to pull all that stuff into one central place so we
don't have loads of drivers dealing with it in their own way.

As far as I can gather from those who deal with DT, such as Arnd, is that
they believe it to be wrong that the DT code sets up the DMA masks, and
they think that stuff will break if the DT code does set these pointers
up.

The big problem which we have is we have a whole bunch of drivers which
don't bother at all with the DMA set mask functions (because they've been
fiddling with the mask directly) and sorting that mess out is going to be
pretty damn difficult.

So, the above series is all about bringing stuff to a central place where
we can then start thinking about changing the behaviour and not have to
patch lots of drivers throughout the tree for every change that's made.

The way I envision the change happening is this:

1. Introduce the notion of mask coercion (drivers forcing the mask to a
   particular value.)

2. Add dma_set_mask() and similar functions to these drivers incrementally.

3. Move the initialization of the mask up to the device creation level
   (iow, the DT code) and out of the drivers (this can be done by
   adding it to the DT code, and removing it from the mask coercion code.)

4. Remove dma_coerce_mask_and_coherent() from the kernel once complete.

So, why bother with dma_coerce_mask_and_coherent()?  Not only does it
centralise the "hack" but it also provides a means to identify drivers
which need work and/or have been missed (you just have to grep for the
direct assignments to the DMA masks.)  Not only that but once the hack
is centralised, it removes some of the variability in the drivers, and
provides a step where we can allow things to be tested hopefully without
causing any regressions.

At least that's the theory.