Linux-ide Archive on lore.kernel.org
 help / color / Atom feed
From: Roger Quadros <rogerq@ti.com>
To: Hans de Goede <hdegoede@redhat.com>, <axboe@kernel.dk>
Cc: vigneshr@ti.com, nsekhar@ti.com, linux-ide@vger.kernel.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	robin.murphy@arm.com, "Christoph Hellwig" <hch@lst.de>,
	"Rob Herring" <robh+dt@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Kishon Vijay Abraham I" <kishon@ti.com>,
	"Péter Ujfalusi" <peter.ujfalusi@ti.com>
Subject: Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci
Date: Wed, 12 Feb 2020 13:01:32 +0200
Message-ID: <1c3ec10c-8505-a067-d51d-667f47d8d55b@ti.com> (raw)
In-Reply-To: <d3a80407-a40a-c9e4-830f-138cfe9b163c@redhat.com>

Hi,

On 06/02/2020 13:50, Hans de Goede wrote:
> Hi,
> 
> On 2/6/20 12:17 PM, Roger Quadros wrote:
>> On TI Platforms using LPAE, SATA breaks with 64-bit DMA.
>> Restrict it to 32-bit.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>   drivers/ata/ahci_platform.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index 3aab2e3d57f3..b925dc54cfa5 100644
>> --- a/drivers/ata/ahci_platform.c
>> +++ b/drivers/ata/ahci_platform.c
>> @@ -62,6 +62,9 @@ static int ahci_probe(struct platform_device *pdev)
>>       if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci"))
>>           hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ;
>> +    if (of_device_is_compatible(dev->of_node, "snps,dwc-ahci"))
>> +        hpriv->flags |= AHCI_HFLAG_32BIT_ONLY;
>> +
> 
> The "snps,dwc-ahci" is a generic (non TI specific) compatible which
> is e.g. also used on some exynos devices. So using that to key the
> setting of the 32 bit flag seems wrong to me.
> 
> IMHO it would be better to introduce a TI specific compatible
> and use that to match on instead (and also adjust the dts files
> accordingly).

Thinking further on this I think it is a bad idea to add a special
binding because the IP is not different. It is just that it is
wired differently on the TI SoC so DMA range is limited.

IMO the proper solution is to have the right dma-ranges property in the
device tree. However, SATA platform driver is doing the wrong thing
by overriding the dma masks.
i.e. in ahci_platform_init_host() in libahci_platform.c

         if (hpriv->cap & HOST_CAP_64) {
                 rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
                 if (rc) {
                         rc = dma_coerce_mask_and_coherent(dev,
                                                           DMA_BIT_MASK(32));
                         if (rc) {
                                 dev_err(dev, "Failed to enable 64-bit DMA.\n");
                                 return rc;
                         }
                         dev_warn(dev, "Enable 32-bit DMA instead of 64-bit.\n");
                 }
         }

This should be removed. Do you agree?

You can also see a similar in ahci_configure_dma_masks() with the XXX disclaimer
explaining that it is wrong.

static int ahci_configure_dma_masks(struct pci_dev *pdev, int using_dac)
{
         const int dma_bits = using_dac ? 64 : 32;
         int rc;

         /*
          * If the device fixup already set the dma_mask to some non-standard
          * value, don't extend it here. This happens on STA2X11, for example.
          *
          * XXX: manipulating the DMA mask from platform code is completely
          * bogus, platform code should use dev->bus_dma_limit instead..
          */
         if (pdev->dma_mask && pdev->dma_mask < DMA_BIT_MASK(32))
                 return 0;

         rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(dma_bits));
         if (rc)
                 dev_err(&pdev->dev, "DMA enable failed\n");
         return rc;
}


> 
> Regards,
> 
> Hans
> 
> 
> 
>>       port = acpi_device_get_match_data(dev);
>>       if (!port)
>>           port = &ahci_port_info;
>>
> 

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

  parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-06 11:17 Roger Quadros
2020-02-06 11:50 ` Hans de Goede
2020-02-06 11:54   ` Roger Quadros
2020-02-12 11:01   ` Roger Quadros [this message]
2020-02-12 11:32     ` Hans de Goede
2020-02-12 11:43       ` Robin Murphy
2020-02-12 11:56         ` David Laight
2020-02-12 12:09           ` Robin Murphy
2020-02-13  7:24         ` Roger Quadros

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1c3ec10c-8505-a067-d51d-667f47d8d55b@ti.com \
    --to=rogerq@ti.com \
    --cc=axboe@kernel.dk \
    --cc=devicetree@vger.kernel.org \
    --cc=hch@lst.de \
    --cc=hdegoede@redhat.com \
    --cc=kishon@ti.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nsekhar@ti.com \
    --cc=peter.ujfalusi@ti.com \
    --cc=robh+dt@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=stable@vger.kernel.org \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ide Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ide/0 linux-ide/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ide linux-ide/ https://lore.kernel.org/linux-ide \
		linux-ide@vger.kernel.org
	public-inbox-index linux-ide

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ide


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git