Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Tuesday 04 November 2014 10:33:18 Al Stone wrote: > On 11/03/2014 10:14 AM, Arnd Bergmann wrote: > > On Monday 03 November 2014 09:15:51 Mark Salter wrote: > >> On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote: > >>> Note that the discussions about merging ACPI support on ARM64 > >>> are based on the assumption that we'd only ever support SBSA-like > >>> platforms, not something like X-Gene that looks more like an > >>> embedded SoC. Your XHCI patches still obviously make sense for > >>> other platforms, so that's not a show-stopper. > >> > >> But for some misconfiguration, the arm64 kernels in fedora arm > >> koji would boot using ACPI on Mustang, the Foundation model, > >> and AMD Seattle platforms. All very much a work in progress, > >> but the tree from which the fedora patches are taken is the > >> devel branch of: > >> > >> git.fedorahosted.org/git/kernel-arm64.git > >> > >> The configuration will be fixed this week and then you can > >> just grab an arm64 fedora kernel and boot with acpi=force. > > Please note that I work directly with Mark Salter and that I have > personally handed out this particular URL many times at either Linaro > Connect and/or to individuals directly. It is not now nor has it ever > been secret, at any time. What I meant was that the patches haven't been circulated on the usual mailing lists when this is exactly the work that needs to be discussed before the base patches that are being sent out can be merged. I was also under the impression (based on the URL) that this was the official Fedora kernel for ARM64. Apparently that is not true, and I probably overreacted based on that. I certainly don't want to see this kind of patches being put into Fedora before there is basic consensus about whether or not they can eventaully get merged upstream. > > It's not as bad as I had feared, but it still does a lot of > > the things that in previous discussions were said that ACPI > > would avoid doing, and that were used as arguments against > > just using DT on all arm64 servers: > > > > - The use of the device properties API that was introduced for > > Intel's embedded devices for on-chip components directly > > contradicts the "standardized firmware interfaces" argument > > that certain people keep making. This certainly explains how > > you plan to use the networking side, but I suspect it's not > > what the ACPI proponents were looking for, or what the base > > patch set is being advertised as. > > I do not understand this statement at all. One of the things > that was added to the ACPI 5.1 specification was the _DSD method > -- Device Specific Properties -- specifically so that device > properties could be standardized. The API mentioned relies on the > existence of _DSD for ACPI. Further, there are people from Linaro > (who happen to work in the kernel and ACPI), the Linux community, > as well as from Intel, ARM and even Microsoft working together to > figure out ways to standardize the device properties being used in > the ACPI Spec Working Group (ASWG) of the UEFI Forum; several of us > are of course paying attention to, participating in, and incorporating > the discussions on this kernel list and others. > > So what is not being standardized? From where I sit, as part of ASWG, > Linaro, Red Hat, and some-time contributor to the Linux kernel, this > whole device properties API was driven by the desire to have a > standardized firmware interface. It even seems to be going a step > further and trying to standardize how the kernel interacts with any > firmware. The point of _DSD is to allow a more generalized format for describing devices that are not standardized. We need this for (x86) embedded systems, because the standardization process is not scalable for coming up with an official description for every single device, so _DSD with the DT properties extension is a great workaround. In particular, this allows us to have a common stable binding for devices between ACPI and DT, which is also great because we already have DT bindings for hundreds or thousands of peripherals that are used on embedded systems. Grant has also started conversations with a number of parties about creating an official standard for the subsystem specific DT bindings that are not part of IEEE1275 or ePAPR, but the individual device bindings are not standardized in the sense of having a standardization body oversee the addition of every single device property in the form that the UEFI forum oversees the things that go into the ACPI specification. We still need to discuss about what this means for ARM servers, and when we raised this topic during the kernel summit, it was far from clear whether we would apply the _DSD device properties to ARM64 servers or not. I think there are good reasons on both sides of the argument. > > - Using custom drivers for hardware that is required by SBSA > > (ahci, pci, ...) means you are contradicting the > > Documentation/arm64/ar
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On 11/03/2014 10:14 AM, Arnd Bergmann wrote: > On Monday 03 November 2014 09:15:51 Mark Salter wrote: >> On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote: >>> You should definitely make sure that this also works with DT, as >>> I don't think it's possible to support X-Gene with ACPI. I know >>> that Al Stone has experimented with it in the past, but he never >>> came back with any results, so I assume the experiment failed. My apologies; I am still remiss in making more patches public. Life interferes sometimes with my kernel development. I make no excuses for being farther behind in this than I have wanted. The experiment has not failed. The assumption is completely wrong. The git tree listed works on the X-Gene platform and it works on AMD Seattle; I have both running on my desk. Both DT and ACPI work, with the proper versions of the DT or the ACPI tables. I will post RFCs as soon as I can, but my work in progress is basically the same as the git tree mentioned below, based on slightly different starting points. >>> Note that the discussions about merging ACPI support on ARM64 >>> are based on the assumption that we'd only ever support SBSA-like >>> platforms, not something like X-Gene that looks more like an >>> embedded SoC. Your XHCI patches still obviously make sense for >>> other platforms, so that's not a show-stopper. >> >> But for some misconfiguration, the arm64 kernels in fedora arm >> koji would boot using ACPI on Mustang, the Foundation model, >> and AMD Seattle platforms. All very much a work in progress, >> but the tree from which the fedora patches are taken is the >> devel branch of: >> >> git.fedorahosted.org/git/kernel-arm64.git >> >> The configuration will be fixed this week and then you can >> just grab an arm64 fedora kernel and boot with acpi=force. Please note that I work directly with Mark Salter and that I have personally handed out this particular URL many times at either Linaro Connect and/or to individuals directly. It is not now nor has it ever been secret, at any time. > It's not as bad as I had feared, but it still does a lot of > the things that in previous discussions were said that ACPI > would avoid doing, and that were used as arguments against > just using DT on all arm64 servers: > > - The use of the device properties API that was introduced for > Intel's embedded devices for on-chip components directly > contradicts the "standardized firmware interfaces" argument > that certain people keep making. This certainly explains how > you plan to use the networking side, but I suspect it's not > what the ACPI proponents were looking for, or what the base > patch set is being advertised as. I do not understand this statement at all. One of the things that was added to the ACPI 5.1 specification was the _DSD method -- Device Specific Properties -- specifically so that device properties could be standardized. The API mentioned relies on the existence of _DSD for ACPI. Further, there are people from Linaro (who happen to work in the kernel and ACPI), the Linux community, as well as from Intel, ARM and even Microsoft working together to figure out ways to standardize the device properties being used in the ACPI Spec Working Group (ASWG) of the UEFI Forum; several of us are of course paying attention to, participating in, and incorporating the discussions on this kernel list and others. So what is not being standardized? From where I sit, as part of ASWG, Linaro, Red Hat, and some-time contributor to the Linux kernel, this whole device properties API was driven by the desire to have a standardized firmware interface. It even seems to be going a step further and trying to standardize how the kernel interacts with any firmware. > - Using custom drivers for hardware that is required by SBSA > (ahci, pci, ...) means you are contradicting the > Documentation/arm64/arm-acpi.txt document that is merged as > one of your early patches and that keeps getting posted as > the base of discussion for the inclusion of the base ACPI > support. I don't think you can have it both ways, saying that > SBSA is required and supporting hardware that doesn't do any > of it won't work. This is where I start the serious whinging On the one hand, I'm told "show the patches." Fine. Someone other than me happens to show patches for work in progress. Or, perhaps I show patches that work but are still proof of concept. It doesn't seem to matter which. The response then looks to me like I'm being told, "don't show patches that are not the absolute, final, irrefutable and irrevocable version." So which is it? The git tree mentioned is a work in progress. No one has ever claimed it was otherwise. This is exactly the same case as with the Linaro ACPI and kernel development trees. What I find incredibly frustrating is that I feel I am being told to submit the final form of patches for ACPI drivers, but those can ONLY be a work a progress until
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Mon, 2014-11-03 at 18:14 +0100, Arnd Bergmann wrote: > On Monday 03 November 2014 09:15:51 Mark Salter wrote: > > On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote: > > > You should definitely make sure that this also works with DT, as > > > I don't think it's possible to support X-Gene with ACPI. I know > > > that Al Stone has experimented with it in the past, but he never > > > came back with any results, so I assume the experiment failed. > > > > > > Note that the discussions about merging ACPI support on ARM64 > > > are based on the assumption that we'd only ever support SBSA-like > > > platforms, not something like X-Gene that looks more like an > > > embedded SoC. Your XHCI patches still obviously make sense for > > > other platforms, so that's not a show-stopper. > > > > But for some misconfiguration, the arm64 kernels in fedora arm > > koji would boot using ACPI on Mustang, the Foundation model, > > and AMD Seattle platforms. All very much a work in progress, > > but the tree from which the fedora patches are taken is the > > devel branch of: > > > > git.fedorahosted.org/git/kernel-arm64.git > > > > The configuration will be fixed this week and then you can > > just grab an arm64 fedora kernel and boot with acpi=force. > > It's not as bad as I had feared, but it still does a lot of > the things that in previous discussions were said that ACPI > would avoid doing, and that were used as arguments against > just using DT on all arm64 servers: As I said, this is very much a work in progress. The point of the fedorahosted tree is to have *something* that will boot with ACPI and have working disk and network as a minimum for the few existing early platforms. It is certainly not an example of what an ACPI/SBSA server should look like. At least not yet. And it isn't a big secret. Linaro, Arm, and other folk are aware of that fedorahosted tree. I wouldn't read too much into any given patch in that tree as it stands right now. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Monday 03 November 2014 09:15:51 Mark Salter wrote: > On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote: > > You should definitely make sure that this also works with DT, as > > I don't think it's possible to support X-Gene with ACPI. I know > > that Al Stone has experimented with it in the past, but he never > > came back with any results, so I assume the experiment failed. > > > > Note that the discussions about merging ACPI support on ARM64 > > are based on the assumption that we'd only ever support SBSA-like > > platforms, not something like X-Gene that looks more like an > > embedded SoC. Your XHCI patches still obviously make sense for > > other platforms, so that's not a show-stopper. > > But for some misconfiguration, the arm64 kernels in fedora arm > koji would boot using ACPI on Mustang, the Foundation model, > and AMD Seattle platforms. All very much a work in progress, > but the tree from which the fedora patches are taken is the > devel branch of: > > git.fedorahosted.org/git/kernel-arm64.git > > The configuration will be fixed this week and then you can > just grab an arm64 fedora kernel and boot with acpi=force. It's not as bad as I had feared, but it still does a lot of the things that in previous discussions were said that ACPI would avoid doing, and that were used as arguments against just using DT on all arm64 servers: - The use of the device properties API that was introduced for Intel's embedded devices for on-chip components directly contradicts the "standardized firmware interfaces" argument that certain people keep making. This certainly explains how you plan to use the networking side, but I suspect it's not what the ACPI proponents were looking for, or what the base patch set is being advertised as. - Using custom drivers for hardware that is required by SBSA (ahci, pci, ...) means you are contradicting the Documentation/arm64/arm-acpi.txt document that is merged as one of your early patches and that keeps getting posted as the base of discussion for the inclusion of the base ACPI support. I don't think you can have it both ways, saying that SBSA is required and supporting hardware that doesn't do any of it won't work. - Adding a generalized method to support arbitrary PCI host controllers indicates that you expect this practice to not just be a special exception for APM but that others would require the same hack to work around firmware or hardware that is not compliant with ACPI. At the same time you introduce a significant of driver code in arch/arm64/kernel/pci.c. Some of that code is probably just an oversight and can be moved into the PCI core later, but it doesn't even seem you tried that hard. All of the above are probably things we can discuss, but by working on this in secret until now, you have really made it hard for the Linaro team whose work you are basing on. They have tried hard for a long time to get basic ACPI support into a mergeable state, and have been working on incorrect base assumptions the whole time because they didn't have a platform implementation to show and to verify their assumptions. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote: > You should definitely make sure that this also works with DT, as > I don't think it's possible to support X-Gene with ACPI. I know > that Al Stone has experimented with it in the past, but he never > came back with any results, so I assume the experiment failed. > > Note that the discussions about merging ACPI support on ARM64 > are based on the assumption that we'd only ever support SBSA-like > platforms, not something like X-Gene that looks more like an > embedded SoC. Your XHCI patches still obviously make sense for > other platforms, so that's not a show-stopper. But for some misconfiguration, the arm64 kernels in fedora arm koji would boot using ACPI on Mustang, the Foundation model, and AMD Seattle platforms. All very much a work in progress, but the tree from which the fedora patches are taken is the devel branch of: git.fedorahosted.org/git/kernel-arm64.git The configuration will be fixed this week and then you can just grab an arm64 fedora kernel and boot with acpi=force. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Friday 31 October 2014 12:32:47 Mark Langsdorf wrote:
> On 10/31/2014 10:49 AM, Arnd Bergmann wrote:
> > On Friday 31 October 2014 09:22:26 Mark Langsdorf wrote:
> >> On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
> >>> On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
> >>>
> >>> You should not access pdev->dev.dma_mask here, that gets set
> >>> by the platform code. You should be able to just use
> >>> dma_set_mask_and_coherent to set both.
> >>
> >> So:
> >>
> >> if (sizeof(dma_addr_t) < 8 ||
> >> dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
> >> ret = dma_set_mask_and_coherent(&pdev->dev,
> >> DMA_BIT_MASK(32));
> >> if (ret)
> >> return ret;
> >> }
> >>
> >> This doesn't actually work for me. I experimented a bit on the
> >> hardware and I always fail if I don't set the coherent mask
> >> first.
> >
> > Very strange, the code looks right to me. What is the initial value
> > of dev->dma_mask?
>
> Did you mean &pdev->dev.dma_mask? It's 0xdc759df8.
No, that would be the pointer to the pointer to the dma mask ;-)
I meant the DMA mask that was set by the platform code in
*pdev->dev.dma_mask. It would also be interesting to know where
pdev->dev.dma_mask points to. Does ACPI allocate memory for that,
or does it point to the coherent mask?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On 10/31/2014 10:49 AM, Arnd Bergmann wrote:
On Friday 31 October 2014 09:22:26 Mark Langsdorf wrote:
On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
You should not access pdev->dev.dma_mask here, that gets set
by the platform code. You should be able to just use
dma_set_mask_and_coherent to set both.
So:
if (sizeof(dma_addr_t) < 8 ||
dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
if (ret)
return ret;
}
This doesn't actually work for me. I experimented a bit on the
hardware and I always fail if I don't set the coherent mask
first.
Very strange, the code looks right to me. What is the initial value
of dev->dma_mask?
Did you mean &pdev->dev.dma_mask? It's 0xdc759df8.
--Mark Langsdorf
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Friday 31 October 2014 09:22:26 Mark Langsdorf wrote:
> On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
> > On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
> >
> > You should not access pdev->dev.dma_mask here, that gets set
> > by the platform code. You should be able to just use
> > dma_set_mask_and_coherent to set both.
>
> So:
>
> if (sizeof(dma_addr_t) < 8 ||
> dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
> ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> if (ret)
> return ret;
> }
>
> This doesn't actually work for me. I experimented a bit on the
> hardware and I always fail if I don't set the coherent mask
> first.
Very strange, the code looks right to me. What is the initial value
of dev->dma_mask?
> >>> Also, we should no longer need to worry about the case where
> >>> pdev->dev.dma_mask is NULL, as this now gets initialized from
> >>> the DT setup.
> >>
> >> I'm running this on a system with ACPI enabled and no DT. Does
> >> that make a difference?
> >
> > I don't know how the DMA mask gets initialized on ACPI, I assume it
> > doesn't at the moment, but that is something that should be fixed
> > in the ACPI code, not worked around in the driver.
> >
> > You should definitely make sure that this also works with DT, as
> > I don't think it's possible to support X-Gene with ACPI. I know
> > that Al Stone has experimented with it in the past, but he never
> > came back with any results, so I assume the experiment failed.
>
> I'm running my test code on an X-Gene with ACPI. Al Stone, Mark
> Salter, and I got it working.
The question is whether that is in a form that we could merge upstream.
I haven't seen any patches being posted, so I can't know for sure, but
from all I know about the hardware it doesn't seem likely, unless
you leave out all the interesting bits such as power mangement, PCI
and networking.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
On 10/30/2014 02:05 PM, Arnd Bergmann wrote:
On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
- /* Initialize dma_mask and coherent_dma_mask to 32-bits */
- ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
- if (ret)
- return ret;
+ /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
+ if (sizeof(dma_addr_t) < 8 ||
+ dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
+ ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+ if (ret)
+ return ret;
+ }
if (!pdev->dev.dma_mask)
pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
else
- dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+ dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
if (!hcd)
The logic here seems wrong: if dma_set_mask is successful, you
can rely on on dma_set_coherent_mask suceeding as well, but
not the other way round.
That's the order in the existing driver. Would you prefer I
switch it to:
if (sizeof(dma_addr_t) < 8 ||
dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
if (ret)
return ret;
}
dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
or based on the comment below:
ret = dma_set_mask(&pdev->dev, pdev->dev.dma_mask);
if (ret)
return ret;
dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
I prefer this version but I don't know if it would work.
You should not access pdev->dev.dma_mask here, that gets set
by the platform code. You should be able to just use
dma_set_mask_and_coherent to set both.
So:
if (sizeof(dma_addr_t) < 8 ||
dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
if (ret)
return ret;
}
This doesn't actually work for me. I experimented a bit on the
hardware and I always fail if I don't set the coherent mask
first.
Also, we should no longer need to worry about the case where
pdev->dev.dma_mask is NULL, as this now gets initialized from
the DT setup.
I'm running this on a system with ACPI enabled and no DT. Does
that make a difference?
I don't know how the DMA mask gets initialized on ACPI, I assume it
doesn't at the moment, but that is something that should be fixed
in the ACPI code, not worked around in the driver.
You should definitely make sure that this also works with DT, as
I don't think it's possible to support X-Gene with ACPI. I know
that Al Stone has experimented with it in the past, but he never
came back with any results, so I assume the experiment failed.
I'm running my test code on an X-Gene with ACPI. Al Stone, Mark
Salter, and I got it working.
--Mark Langsdorf
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
> On 10/30/2014 02:05 PM, Arnd Bergmann wrote:
> > On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
> >> - /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> >> - ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> >> - if (ret)
> >> - return ret;
> >> + /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits
> >> */
> >> + if (sizeof(dma_addr_t) < 8 ||
> >> + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> >> + ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> >> + if (ret)
> >> + return ret;
> >> + }
> >> if (!pdev->dev.dma_mask)
> >> pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> >> else
> >> - dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> >> + dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
> >>
> >> hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> >> if (!hcd)
> >>
> >
> > The logic here seems wrong: if dma_set_mask is successful, you
> > can rely on on dma_set_coherent_mask suceeding as well, but
> > not the other way round.
>
> That's the order in the existing driver. Would you prefer I
> switch it to:
> if (sizeof(dma_addr_t) < 8 ||
> dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> if (ret)
> return ret;
> }
> dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
>
> or based on the comment below:
> ret = dma_set_mask(&pdev->dev, pdev->dev.dma_mask);
> if (ret)
> return ret;
> dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
>
> I prefer this version but I don't know if it would work.
You should not access pdev->dev.dma_mask here, that gets set
by the platform code. You should be able to just use
dma_set_mask_and_coherent to set both.
> > Also, we should no longer need to worry about the case where
> > pdev->dev.dma_mask is NULL, as this now gets initialized from
> > the DT setup.
>
> I'm running this on a system with ACPI enabled and no DT. Does
> that make a difference?
I don't know how the DMA mask gets initialized on ACPI, I assume it
doesn't at the moment, but that is something that should be fixed
in the ACPI code, not worked around in the driver.
You should definitely make sure that this also works with DT, as
I don't think it's possible to support X-Gene with ACPI. I know
that Al Stone has experimented with it in the past, but he never
came back with any results, so I assume the experiment failed.
Note that the discussions about merging ACPI support on ARM64
are based on the assumption that we'd only ever support SBSA-like
platforms, not something like X-Gene that looks more like an
embedded SoC. Your XHCI patches still obviously make sense for
other platforms, so that's not a show-stopper.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On 10/30/2014 02:05 PM, Arnd Bergmann wrote:
On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
- /* Initialize dma_mask and coherent_dma_mask to 32-bits */
- ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
- if (ret)
- return ret;
+ /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
+ if (sizeof(dma_addr_t) < 8 ||
+ dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
+ ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+ if (ret)
+ return ret;
+ }
if (!pdev->dev.dma_mask)
pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
else
- dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+ dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
if (!hcd)
The logic here seems wrong: if dma_set_mask is successful, you
can rely on on dma_set_coherent_mask suceeding as well, but
not the other way round.
That's the order in the existing driver. Would you prefer I
switch it to:
if (sizeof(dma_addr_t) < 8 ||
dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
if (ret)
return ret;
}
dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
or based on the comment below:
ret = dma_set_mask(&pdev->dev, pdev->dev.dma_mask);
if (ret)
return ret;
dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
I prefer this version but I don't know if it would work.
Also, we should no longer need to worry about the case where
pdev->dev.dma_mask is NULL, as this now gets initialized from
the DT setup.
I'm running this on a system with ACPI enabled and no DT. Does
that make a difference?
--Mark Langsdorf
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
> - /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> - ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> - if (ret)
> - return ret;
> + /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
> + if (sizeof(dma_addr_t) < 8 ||
> + dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> + ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> + if (ret)
> + return ret;
> + }
> if (!pdev->dev.dma_mask)
> pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> else
> - dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> + dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
>
> hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> if (!hcd)
>
The logic here seems wrong: if dma_set_mask is successful, you
can rely on on dma_set_coherent_mask suceeding as well, but
not the other way round.
Also, we should no longer need to worry about the case where
pdev->dev.dma_mask is NULL, as this now gets initialized from
the DT setup.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] [usb] make xhci platform driver use 64 bit or 32 bit DMA
The xhci platform driver needs to work on systems that either only
support 64-bit DMA or only support 32-bit DMA. Attempt to set a
coherent dma mask for 64-bit DMA, and attempt again with 32-bit
DMA if that fails.
Signed-off-by: Mark Langsdorf
---
drivers/usb/host/xhci-plat.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 1a0cf9f..3045e77 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -147,14 +147,17 @@ static int xhci_plat_probe(struct platform_device *pdev)
return ret;
}
- /* Initialize dma_mask and coherent_dma_mask to 32-bits */
- ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
- if (ret)
- return ret;
+ /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
+ if (sizeof(dma_addr_t) < 8 ||
+ dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
+ ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+ if (ret)
+ return ret;
+ }
if (!pdev->dev.dma_mask)
pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
else
- dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+ dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
if (!hcd)
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
