* [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci @ 2020-02-06 11:17 Roger Quadros 2020-02-06 11:50 ` Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Roger Quadros @ 2020-02-06 11:17 UTC (permalink / raw) To: hdegoede, axboe Cc: vigneshr, nsekhar, linux-ide, linux-kernel, Roger Quadros, stable 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; + port = acpi_device_get_match_data(dev); if (!port) port = &ahci_port_info; -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 2020-02-06 11:17 [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci Roger Quadros @ 2020-02-06 11:50 ` Hans de Goede 2020-02-06 11:54 ` Roger Quadros 2020-02-12 11:01 ` Roger Quadros 0 siblings, 2 replies; 9+ messages in thread From: Hans de Goede @ 2020-02-06 11:50 UTC (permalink / raw) To: Roger Quadros, axboe; +Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable 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). Regards, Hans > port = acpi_device_get_match_data(dev); > if (!port) > port = &ahci_port_info; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 2020-02-06 11:50 ` Hans de Goede @ 2020-02-06 11:54 ` Roger Quadros 2020-02-12 11:01 ` Roger Quadros 1 sibling, 0 replies; 9+ messages in thread From: Roger Quadros @ 2020-02-06 11:54 UTC (permalink / raw) To: Hans de Goede, axboe; +Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable Hans, 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. You are right, Vignesh also pointed this out to me offline. snps,dwc-ahci does indeed support 64-bit addressing, so this patch is wrong. > > 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). The TI platform's TRM does say it has only 36-bits of the controller wired in the device. If that was the case and DDR address never goes beyond 36-bits, we don't understand why it fails in the first place. 80000000-afcfffff : System RAM b0000000-feffffff : System RAM 200000000-27fffffff : System RAM cheers, -roger > > Regards, > > Hans > > > >> port = acpi_device_get_match_data(dev); >> if (!port) >> port = &ahci_port_info; >> > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 2020-02-06 11:50 ` Hans de Goede 2020-02-06 11:54 ` Roger Quadros @ 2020-02-12 11:01 ` Roger Quadros 2020-02-12 11:32 ` Hans de Goede 1 sibling, 1 reply; 9+ messages in thread From: Roger Quadros @ 2020-02-12 11:01 UTC (permalink / raw) To: Hans de Goede, axboe Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable, robin.murphy, Christoph Hellwig, Rob Herring, devicetree, Kishon Vijay Abraham I, Péter Ujfalusi 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 2020-02-12 11:01 ` Roger Quadros @ 2020-02-12 11:32 ` Hans de Goede 2020-02-12 11:43 ` Robin Murphy 0 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2020-02-12 11:32 UTC (permalink / raw) To: Roger Quadros, axboe Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable, robin.murphy, Christoph Hellwig, Rob Herring, devicetree, Kishon Vijay Abraham I, Péter Ujfalusi Hi, On 2/12/20 12:01 PM, Roger Quadros wrote: > 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? I agree with you in principal, but I'm afraid this might cause regressions for existing hardware. We only do this if the host has set the CAP_64 flag, this code is quite old, it comes from the following commit: ### From cc7a9e27562cd78a1dc885504086fab24addce40 Mon Sep 17 00:00:00 2001 From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> Date: Thu, 12 Jun 2014 12:40:23 -0500 Subject: [PATCH v3] ahci: Check and set 64-bit DMA mask for platform AHCI driver The current platform AHCI driver 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> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Reviewed-by: Hans de Goede <hdegoede@redhat.com> Tested-by: Hans de Goede <hdegoede@redhat.com> Tested-by: Suman Tripathi <stripathi@apm.com> Signed-off-by: Tejun Heo <tj@kernel.org> ### Presumably this was added for a reason, I'm guessing this might come from AMD's ARM server chips adventures, but I'm afraid that AHCI support on other (ARM) SoC's has become to rely on this behavior too. Maybe we can add a check to see if the mask was not already set and skip setting the mask in that case ? Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 2020-02-12 11:32 ` Hans de Goede @ 2020-02-12 11:43 ` Robin Murphy 2020-02-12 11:56 ` David Laight 2020-02-13 7:24 ` Roger Quadros 0 siblings, 2 replies; 9+ messages in thread From: Robin Murphy @ 2020-02-12 11:43 UTC (permalink / raw) To: Hans de Goede, Roger Quadros, axboe Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable, Christoph Hellwig, Rob Herring, devicetree, Kishon Vijay Abraham I, Péter Ujfalusi On 2020-02-12 11:32 am, Hans de Goede wrote: > Hi, > > On 2/12/20 12:01 PM, Roger Quadros wrote: >> 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? > > I agree with you in principal, but I'm afraid this might cause > regressions for > existing hardware. We only do this if the host has set the CAP_64 flag, > this code is quite old, it comes from the following commit: > > ### > From cc7a9e27562cd78a1dc885504086fab24addce40 Mon Sep 17 00:00:00 2001 > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Date: Thu, 12 Jun 2014 12:40:23 -0500 > Subject: [PATCH v3] ahci: Check and set 64-bit DMA mask for platform > AHCI driver > > The current platform AHCI driver 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> > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Tested-by: Hans de Goede <hdegoede@redhat.com> > Tested-by: Suman Tripathi <stripathi@apm.com> > Signed-off-by: Tejun Heo <tj@kernel.org> > ### > > Presumably this was added for a reason, I'm guessing this might come > from AMD's ARM server chips adventures, but I'm afraid that AHCI support > on other (ARM) SoC's has become to rely on this behavior too. > > Maybe we can add a check to see if the mask was not already set and skip > setting the mask in that case ? If the device *is* inherently 64-bit capable, then setting 64-bit masks in the driver is correct - if a 64-bit IP block happens to have been integrated with only 32 address bits wired up, but the system has memory above the 32-bit boundary, then that should be described via "dma-ranges", which should then end up being used to further constrain the device masks internally to the DMA API. Robin. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 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 1 sibling, 1 reply; 9+ messages in thread From: David Laight @ 2020-02-12 11:56 UTC (permalink / raw) To: 'Robin Murphy', Hans de Goede, Roger Quadros, axboe Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable, Christoph Hellwig, Rob Herring, devicetree, Kishon Vijay Abraham I, Péter Ujfalusi From: Robin Murphy > Sent: 12 February 2020 11:43 ... > If the device *is* inherently 64-bit capable, then setting 64-bit masks > in the driver is correct - if a 64-bit IP block happens to have been > integrated with only 32 address bits wired up, but the system has memory > above the 32-bit boundary, then that should be described via > "dma-ranges", which should then end up being used to further constrain > the device masks internally to the DMA API. Given how often this happens (please can I shoot some more hardware engineers - he says while compiling some VHDL) is it possible to allocate some memory pages that are aliases if the address bits over 31 are ignored? Then (at least some) drivers could to a run-time probe reading to the high address and checking the data didn't appear in the low address. Only one such set of pages is needed - access can be locked. But they'd need to be reserved early on. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 2020-02-12 11:56 ` David Laight @ 2020-02-12 12:09 ` Robin Murphy 0 siblings, 0 replies; 9+ messages in thread From: Robin Murphy @ 2020-02-12 12:09 UTC (permalink / raw) To: David Laight, Hans de Goede, Roger Quadros, axboe Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable, Christoph Hellwig, Rob Herring, devicetree, Kishon Vijay Abraham I, Péter Ujfalusi On 2020-02-12 11:56 am, David Laight wrote: > From: Robin Murphy >> Sent: 12 February 2020 11:43 > ... >> If the device *is* inherently 64-bit capable, then setting 64-bit masks >> in the driver is correct - if a 64-bit IP block happens to have been >> integrated with only 32 address bits wired up, but the system has memory >> above the 32-bit boundary, then that should be described via >> "dma-ranges", which should then end up being used to further constrain >> the device masks internally to the DMA API. > > Given how often this happens (please can I shoot some more > hardware engineers - he says while compiling some VHDL) > is it possible to allocate some memory pages that are > aliases if the address bits over 31 are ignored? > > Then (at least some) drivers could to a run-time probe > reading to the high address and checking the data didn't > appear in the low address. > > Only one such set of pages is needed - access can be locked. > But they'd need to be reserved early on. (Oh, for that much control over page allocation!) It's a fun idea, but there's no guarantee that the platform memory map is actually suitable (AMD Seattle is the go-to counterexample), and having an opt-in test that every driver has to implement individually sounds like a maintenance nightmare. I think it's far better all round to just expect the firmware-provided machine description - be it devicetree, ACPI, or whatever - to correctly describe the integration. Robin. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci 2020-02-12 11:43 ` Robin Murphy 2020-02-12 11:56 ` David Laight @ 2020-02-13 7:24 ` Roger Quadros 1 sibling, 0 replies; 9+ messages in thread From: Roger Quadros @ 2020-02-13 7:24 UTC (permalink / raw) To: Robin Murphy, Hans de Goede, axboe Cc: vigneshr, nsekhar, linux-ide, linux-kernel, stable, Christoph Hellwig, Rob Herring, devicetree, Kishon Vijay Abraham I, Péter Ujfalusi On 12/02/2020 13:43, Robin Murphy wrote: > On 2020-02-12 11:32 am, Hans de Goede wrote: >> Hi, >> >> On 2/12/20 12:01 PM, Roger Quadros wrote: >>> 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? >> >> I agree with you in principal, but I'm afraid this might cause regressions for >> existing hardware. We only do this if the host has set the CAP_64 flag, >> this code is quite old, it comes from the following commit: >> >> ### >> From cc7a9e27562cd78a1dc885504086fab24addce40 Mon Sep 17 00:00:00 2001 >> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> Date: Thu, 12 Jun 2014 12:40:23 -0500 >> Subject: [PATCH v3] ahci: Check and set 64-bit DMA mask for platform AHCI driver >> >> The current platform AHCI driver 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> >> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> Tested-by: Hans de Goede <hdegoede@redhat.com> >> Tested-by: Suman Tripathi <stripathi@apm.com> >> Signed-off-by: Tejun Heo <tj@kernel.org> >> ### >> >> Presumably this was added for a reason, I'm guessing this might come >> from AMD's ARM server chips adventures, but I'm afraid that AHCI support >> on other (ARM) SoC's has become to rely on this behavior too. >> >> Maybe we can add a check to see if the mask was not already set and skip >> setting the mask in that case ? > > If the device *is* inherently 64-bit capable, then setting 64-bit masks in the driver is correct - if a 64-bit IP block happens to have been integrated with only 32 address bits wired up, but the system has memory above the 32-bit boundary, then that should be described via "dma-ranges", which should then end up being used to further constrain the device masks internally to the DMA API. I agree. In this case, it looks like DMA API is allocating memory > 32-bits even if "dma-ranges" size and dma_bus_limit is < 32-bits so issue is with platform DMA code. I will continue the discussion in the other thread https://lkml.org/lkml/2020/2/12/907 -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-13 7:25 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-06 11:17 [PATCH] ata: ahci_platform: add 32-bit quirk for dwc-ahci Roger Quadros 2020-02-06 11:50 ` Hans de Goede 2020-02-06 11:54 ` Roger Quadros 2020-02-12 11:01 ` Roger Quadros 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).