[PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

The current platform AHCI drier does not set the dma_mask correctly
for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
capability bit and set the dma_mask and coherent_dma_mask accordingly.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 drivers/ata/libahci_platform.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 7cb3a85..85049ef 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
 	ahci_init_controller(host);
 	ahci_print_info(host, "platform");
 
+	if (hpriv->cap & HOST_CAP_64) {
+		if (!dev->dma_mask)
+			dev->dma_mask = &dev->coherent_dma_mask;
+
+		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+		if (rc)
+			return rc;
+	}
+
 	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
 				 &ahci_platform_sht);
 }
-- 
1.9.0

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[Suravee Suthikulpanit]

Hi Tejun,

I am following up on this patch.

Thank you,

Suravee

On 05/23/2014 12:35 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>
> The current platform AHCI drier does not set the dma_mask correctly
> for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
> capability bit and set the dma_mask and coherent_dma_mask accordingly.
>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>   drivers/ata/libahci_platform.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 7cb3a85..85049ef 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
>   	ahci_init_controller(host);
>   	ahci_print_info(host, "platform");
>
> +	if (hpriv->cap & HOST_CAP_64) {
> +		if (!dev->dma_mask)
> +			dev->dma_mask = &dev->coherent_dma_mask;
> +
> +		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +		if (rc)
> +			return rc;
> +	}
> +
>   	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
>   				 &ahci_platform_sht);
>   }
>

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[Tejun Heo]

Hans, Bartlomiej, can you guys please review this patch?

Thanks.

On Fri, May 23, 2014 at 12:35:10PM -0500, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> The current platform AHCI drier does not set the dma_mask correctly
> for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
> capability bit and set the dma_mask and coherent_dma_mask accordingly.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  drivers/ata/libahci_platform.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 7cb3a85..85049ef 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
>  	ahci_init_controller(host);
>  	ahci_print_info(host, "platform");
>  
> +	if (hpriv->cap & HOST_CAP_64) {
> +		if (!dev->dma_mask)
> +			dev->dma_mask = &dev->coherent_dma_mask;
> +
> +		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +		if (rc)
> +			return rc;
> +	}
> +
>  	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
>  				 &ahci_platform_sht);
>  }
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
tejun

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[Suravee Suthikulanit]

Hans/Bartlomiej,

Do you guys have any questions about this patch?

Thank you,

Suravee

On 6/3/2014 12:58 PM, Tejun Heo wrote:
> Hans, Bartlomiej, can you guys please review this patch?
>
> Thanks.
>
> On Fri, May 23, 2014 at 12:35:10PM -0500, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>
>> The current platform AHCI drier does not set the dma_mask correctly
>> for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
>> capability bit and set the dma_mask and coherent_dma_mask accordingly.
>>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>>   drivers/ata/libahci_platform.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>> index 7cb3a85..85049ef 100644
>> --- a/drivers/ata/libahci_platform.c
>> +++ b/drivers/ata/libahci_platform.c
>> @@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
>>   	ahci_init_controller(host);
>>   	ahci_print_info(host, "platform");
>>
>> +	if (hpriv->cap & HOST_CAP_64) {
>> +		if (!dev->dma_mask)
>> +			dev->dma_mask = &dev->coherent_dma_mask;
>> +
>> +		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>>   	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
>>   				 &ahci_platform_sht);
>>   }
>> --
>> 1.9.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, June 06, 2014 11:10:58 AM Suravee Suthikulanit wrote:
> Hans/Bartlomiej,
> 
> Do you guys have any questions about this patch?

Sorry for the delay, some questions regarding the patch below.

> Thank you,
> 
> Suravee
> 
> On 6/3/2014 12:58 PM, Tejun Heo wrote:
> > Hans, Bartlomiej, can you guys please review this patch?
> >
> > Thanks.
> >
> > On Fri, May 23, 2014 at 12:35:10PM -0500, suravee.suthikulpanit@amd.com wrote:
> >> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >>
> >> The current platform AHCI drier does not set the dma_mask correctly
> >> for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
> >> capability bit and set the dma_mask and coherent_dma_mask accordingly.
> >>
> >> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >> ---
> >>   drivers/ata/libahci_platform.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> >> index 7cb3a85..85049ef 100644
> >> --- a/drivers/ata/libahci_platform.c
> >> +++ b/drivers/ata/libahci_platform.c
> >> @@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
> >>   	ahci_init_controller(host);
> >>   	ahci_print_info(host, "platform");
> >>
> >> +	if (hpriv->cap & HOST_CAP_64) {
> >> +		if (!dev->dma_mask)

What configuration is the above dev->dma_mask checking supposed to handle?
Is it really needed?  If not the current dma_set_mask_and_coherent() call
can be replaced by dma_coerce_mask_and_coherent() one.

> >> +			dev->dma_mask = &dev->coherent_dma_mask;
> >> +
> >> +		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> >> +		if (rc)
> >> +			return rc;

Shouldn't we try to set DMA masks to 32-bit ones on error (like it is done
in ahci_configure_dma_masks()) instead of failing the initialization?

> >> +	}
> >> +
> >>   	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
> >>   				 &ahci_platform_sht);
> >>   }
> >> --
> >> 1.9.0

BTW It seems that after DMA masks handling is fixed in the generic AHCI
platform code the driver specific code in ahci_xgene.c can be removed.

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

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[Suravee Suthikulpanit]

On 06/11/2014 04:41 AM, Bartlomiej Zolnierkiewicz wrote:
>>> On Fri, May 23, 2014 at 12:35:10PM -0500,suravee.suthikulpanit@amd.com  wrote:
>>>> > >>From: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
>>>> > >>
>>>> > >>The current platform AHCI drier does not set the dma_mask correctly
>>>> > >>for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
>>>> > >>capability bit and set the dma_mask and coherent_dma_mask accordingly.
>>>> > >>
>>>> > >>Signed-off-by: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com>
>>>> > >>---
>>>> > >>   drivers/ata/libahci_platform.c | 9 +++++++++
>>>> > >>   1 file changed, 9 insertions(+)
>>>> > >>
>>>> > >>diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>>> > >>index 7cb3a85..85049ef 100644
>>>> > >>--- a/drivers/ata/libahci_platform.c
>>>> > >>+++ b/drivers/ata/libahci_platform.c
>>>> > >>@@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
>>>> > >>   	ahci_init_controller(host);
>>>> > >>   	ahci_print_info(host, "platform");
>>>> > >>
>>>> > >>+	if (hpriv->cap & HOST_CAP_64) {
>>>> > >>+		if (!dev->dma_mask)
> What configuration is the above dev->dma_mask checking supposed to handle?
> Is it really needed?  If not the current dma_set_mask_and_coherent() call
> can be replaced by dma_coerce_mask_and_coherent() one.

I was just trying to be safe and not always assuming that dev->dma_mask 
is pointing to the dev->coherent_dma_mask. If you think this is not 
necessary, I can remove this.

>
>>>> > >>+			dev->dma_mask = &dev->coherent_dma_mask;
>>>> > >>+
>>>> > >>+		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>>>> > >>+		if (rc)
>>>> > >>+			return rc;
> Shouldn't we try to set DMA masks to 32-bit ones on error (like it is done
> in ahci_configure_dma_masks()) instead of failing the initialization?

I can also add the additional logic to try for 32-bit.

>
>>>> > >>+	}
>>>> > >>+
>>>> > >>   	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
>>>> > >>   				 &ahci_platform_sht);
>>>> > >>   }
>>>> > >>--
>>>> > >>1.9.0
> BTW It seems that after DMA masks handling is fixed in the generic AHCI
> platform code the driver specific code in ahci_xgene.c can be removed.

I am including Loc Ho, Tuan Phan, and Suman Tripathi to help verifying 
if code could be removed. If so, I'll send out the change in V2.

Suravee

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

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[David Milburn]

On 05/23/2014 12:35 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>
> The current platform AHCI drier does not set the dma_mask correctly
> for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
> capability bit and set the dma_mask and coherent_dma_mask accordingly.
>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>   drivers/ata/libahci_platform.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 7cb3a85..85049ef 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,

Hi Suravee,

Would it be better to do the following before ahci_reset_controller()?

         /* initialize adapter */
         rc = ahci_configure_dma_masks(pdev, hpriv->cap & HOST_CAP_64);
         if (rc)
                	return rc;


Thanks,
David



>   	ahci_init_controller(host);
>   	ahci_print_info(host, "platform");
>
> +	if (hpriv->cap & HOST_CAP_64) {
> +		if (!dev->dma_mask)
> +			dev->dma_mask = &dev->coherent_dma_mask;
> +
> +		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +		if (rc)
> +			return rc;
> +	}
> +
>   	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
>   				 &ahci_platform_sht);
>   }
>

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, June 10, 2014 04:54:01 PM David Milburn wrote:
> On 05/23/2014 12:35 PM, suravee.suthikulpanit@amd.com wrote:
> > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >
> > The current platform AHCI drier does not set the dma_mask correctly
> > for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
> > capability bit and set the dma_mask and coherent_dma_mask accordingly.
> >
> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> > ---
> >   drivers/ata/libahci_platform.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index 7cb3a85..85049ef 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
> 
> Hi Suravee,
> 
> Would it be better to do the following before ahci_reset_controller()?
> 
>          /* initialize adapter */
>          rc = ahci_configure_dma_masks(pdev, hpriv->cap & HOST_CAP_64);
>          if (rc)
>                 	return rc;

ahci_configure_dma_masks() is currently for PCI controllers only
so it can't be used for libahci_platform.c.

When it comes to setting DMA masks before ahci_reset_controller()
I don't think that it matters but it will make platform code more
similar to PCI one which is a good thing.

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


> Thanks,
> David
> 
> 
> 
> >   	ahci_init_controller(host);
> >   	ahci_print_info(host, "platform");
> >
> > +	if (hpriv->cap & HOST_CAP_64) {
> > +		if (!dev->dma_mask)
> > +			dev->dma_mask = &dev->coherent_dma_mask;
> > +
> > +		rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> >   	return ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
> >   				 &ahci_platform_sht);
> >   }

Re: [PATCH 1/1] ata: Check and set 64-bit DMA mask for platform AHCI driver

[Suravee Suthikulpanit]

On 06/11/2014 04:30 AM, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Tuesday, June 10, 2014 04:54:01 PM David Milburn wrote:
>> On 05/23/2014 12:35 PM, suravee.suthikulpanit@amd.com wrote:
>>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>>
>>> The current platform AHCI drier does not set the dma_mask correctly
>>> for 64-bit DMA capable AHCI controller.  This patch checks the AHCI
>>> capability bit and set the dma_mask and coherent_dma_mask accordingly.
>>>
>>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>> ---
>>>    drivers/ata/libahci_platform.c | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>> index 7cb3a85..85049ef 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -368,6 +368,15 @@ int ahci_platform_init_host(struct platform_device *pdev,
>>
>> Hi Suravee,
>>
>> Would it be better to do the following before ahci_reset_controller()?
>>
>>           /* initialize adapter */
>>           rc = ahci_configure_dma_masks(pdev, hpriv->cap & HOST_CAP_64);
>>           if (rc)
>>                  	return rc;
>
> ahci_configure_dma_masks() is currently for PCI controllers only
> so it can't be used for libahci_platform.c.
>
> When it comes to setting DMA masks before ahci_reset_controller()
> I don't think that it matters but it will make platform code more
> similar to PCI one which is a good thing.

I can move this logic before ahci_reset_controller() in V2.

Suravee