Hans, Bartlomiej, can you guys please review this patch?
Thanks.
On Fri, May 23, 2014 at 12:35:10PM -0500, [email protected] wrote:
> From: Suravee Suthikulpanit <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
tejun
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, [email protected] wrote:
>> From: Suravee Suthikulpanit <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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, [email protected] wrote:
> >> From: Suravee Suthikulpanit <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> 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
On 06/11/2014 04:41 AM, Bartlomiej Zolnierkiewicz wrote:
>>> On Fri, May 23, 2014 at 12:35:10PM -0500,[email protected] wrote:
>>>> > >>From: Suravee Suthikulpanit<[email protected]>
>>>> > >>
>>>> > >>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<[email protected]>
>>>> > >>---
>>>> > >> 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
>