| Message ID | 1403558626-13422-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 23, 2014 at 03:23:44PM -0600, Stephen Warren wrote: > This branch reworks the set of regulators that the Tegra PCIe driver > uses, so that the driver and DT bindings more correctly model what's > really going on in HW. > > I've made this a separate branch in case it needs to be pulled into the > PCIe tree to resolve any conflicts. Any branch that adds Tegra124 > support to the PCIe driver will need to be based on this branch, and > such patches might show up for 3.17, and be taken through the ARM tree > so we can manage our own dependencies. Isn't PCI broken if you boot with an older device tree now? I would like to see this as two branches: One to the PCI driver, and one modifying DT contents. The PCI driver should remain working for old DTs, so the last couple of commits on this branch can't be there. -Olof
On Sun, Jul 06, 2014 at 05:38:54PM -0700, Olof Johansson wrote: > On Mon, Jun 23, 2014 at 03:23:44PM -0600, Stephen Warren wrote: > > This branch reworks the set of regulators that the Tegra PCIe driver > > uses, so that the driver and DT bindings more correctly model what's > > really going on in HW. > > > > I've made this a separate branch in case it needs to be pulled into the > > PCIe tree to resolve any conflicts. Any branch that adds Tegra124 > > support to the PCIe driver will need to be based on this branch, and > > such patches might show up for 3.17, and be taken through the ARM tree > > so we can manage our own dependencies. > > Isn't PCI broken if you boot with an older device tree now? Yes. > I would like to see this as two branches: One to the PCI driver, and one > modifying DT contents. The PCI driver should remain working for old DTs, > so the last couple of commits on this branch can't be there. This is one of my main gripes with device tree these days. We have many situations where device tree bindings got rushed with the result that many of them describe hardware in a *completely* wrong way. The Tegra PCIe binding was designed with an incomplete understanding of the hardware (I wonder how many other cases there are like this in mainline) and it just happens to work by accident on existing platforms. So this really boils down to one question: how do we fix bugs in device tree bindings? I suppose we could keep some sort of backwards-compatible shim inside the driver to cope with the existing, wrong device tree binding. However that means an additional maintenance burden and I'm not convinced that there's a need in this particular case. Thierry
On Sun, Jul 6, 2014 at 10:52 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Sun, Jul 06, 2014 at 05:38:54PM -0700, Olof Johansson wrote: >> On Mon, Jun 23, 2014 at 03:23:44PM -0600, Stephen Warren wrote: >> > This branch reworks the set of regulators that the Tegra PCIe driver >> > uses, so that the driver and DT bindings more correctly model what's >> > really going on in HW. >> > >> > I've made this a separate branch in case it needs to be pulled into the >> > PCIe tree to resolve any conflicts. Any branch that adds Tegra124 >> > support to the PCIe driver will need to be based on this branch, and >> > such patches might show up for 3.17, and be taken through the ARM tree >> > so we can manage our own dependencies. >> >> Isn't PCI broken if you boot with an older device tree now? > > Yes. > >> I would like to see this as two branches: One to the PCI driver, and one >> modifying DT contents. The PCI driver should remain working for old DTs, >> so the last couple of commits on this branch can't be there. > > This is one of my main gripes with device tree these days. We have many > situations where device tree bindings got rushed with the result that > many of them describe hardware in a *completely* wrong way. > > The Tegra PCIe binding was designed with an incomplete understanding of > the hardware (I wonder how many other cases there are like this in > mainline) and it just happens to work by accident on existing platforms. > > So this really boils down to one question: how do we fix bugs in device > tree bindings? > > I suppose we could keep some sort of backwards-compatible shim inside > the driver to cope with the existing, wrong device tree binding. However > that means an additional maintenance burden and I'm not convinced that > there's a need in this particular case. It's really unfortunate, and this is very likely not the last time we're going to see something like this. Backwards compatibility is only needed where there actually are users of it. Are you absolutely 100% sure that there will not be such a case? Trimslice users? Out-of-tree DTS:es that now need to be fixed up or they will break? This isn't the same thing as in-kernel interface changes where we say "out of tree, out of mind". If you have to stay compatible, then I suggest you try to fill in local driver variables with derivatives of the old properties (and directly from the newer properties where you can). I haven't looked at the specifics here so I don't know how hard it might be. If you are 100% sure that you don't have to stay compatible, then you can remove the code handling the old bindings. Still, even then I am a little worried about dependencies (and more importantly conflicts) between these dtsi changes and others done by tegra platform code for this release. I suppose that can be resolved by having this as a base of any DT changes for tegra if needed. -Olof
On Mon, Jul 07, 2014 at 09:45:46PM -0700, Olof Johansson wrote: > On Sun, Jul 6, 2014 at 10:52 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Sun, Jul 06, 2014 at 05:38:54PM -0700, Olof Johansson wrote: > >> On Mon, Jun 23, 2014 at 03:23:44PM -0600, Stephen Warren wrote: > >> > This branch reworks the set of regulators that the Tegra PCIe driver > >> > uses, so that the driver and DT bindings more correctly model what's > >> > really going on in HW. > >> > > >> > I've made this a separate branch in case it needs to be pulled into the > >> > PCIe tree to resolve any conflicts. Any branch that adds Tegra124 > >> > support to the PCIe driver will need to be based on this branch, and > >> > such patches might show up for 3.17, and be taken through the ARM tree > >> > so we can manage our own dependencies. > >> > >> Isn't PCI broken if you boot with an older device tree now? > > > > Yes. > > > >> I would like to see this as two branches: One to the PCI driver, and one > >> modifying DT contents. The PCI driver should remain working for old DTs, > >> so the last couple of commits on this branch can't be there. > > > > This is one of my main gripes with device tree these days. We have many > > situations where device tree bindings got rushed with the result that > > many of them describe hardware in a *completely* wrong way. > > > > The Tegra PCIe binding was designed with an incomplete understanding of > > the hardware (I wonder how many other cases there are like this in > > mainline) and it just happens to work by accident on existing platforms. > > > > So this really boils down to one question: how do we fix bugs in device > > tree bindings? > > > > I suppose we could keep some sort of backwards-compatible shim inside > > the driver to cope with the existing, wrong device tree binding. However > > that means an additional maintenance burden and I'm not convinced that > > there's a need in this particular case. > > It's really unfortunate, and this is very likely not the last time > we're going to see something like this. > > Backwards compatibility is only needed where there actually are users > of it. Are you absolutely 100% sure that there will not be such a > case? Of course not. Nobody can be absolutely sure about that. > Trimslice users? I think there are two categories here: people that use non-upstream kernels provided by Compulab (because you get all the hardware acceleration goodness) and those that use upstream kernels. Those that use the downstream kernels likely use bindings (if DT at all) that are not supported upstream. And everybody that uses upstream almost certainly uses an upstream U-Boot where it's trivial to update kernel and DTB in one go. And this brings me back to another complaint about DTB: we keep saying things are stable ABI based on the rationale that somebody may have shipped some DT version in a read-only ROM and therefore can't be replaced and thus can't be broken. Today more people are likely to have some version of a downstream DTB in their device that was never supported upstream. And by the above argument we're giving downstream users the finger because it's impossible to update to an upstream kernel. So there's a lot of hypocrisy in that whole argument. > Out-of-tree DTS:es that now need to be fixed up or they will break? Yes, any out-of-tree DTS'es would need to be updated or things will indeed break. There at least one case where an out-of-tree DTS was already updated updated during the process of getting the support upstream (Toradex). > This isn't the same thing as in-kernel interface changes where we say > "out of tree, out of mind". That's exactly the problem. When we started out with the DT conversion this was never mentioned. Maybe some people always took this for granted but certainly not all. Most of the device tree bindings back at the time were pulled out of a hat. Many were subsequently changed because they didn't work out as expected. Until the big discussions around ABI stability started. > If you have to stay compatible, then I suggest you try to fill in > local driver variables with derivatives of the old properties (and > directly from the newer properties where you can). I haven't looked at > the specifics here so I don't know how hard it might be. > > If you are 100% sure that you don't have to stay compatible, then you > can remove the code handling the old bindings. Still, even then I am a > little worried about dependencies (and more importantly conflicts) > between these dtsi changes and others done by tegra platform code for > this release. I suppose that can be resolved by having this as a base > of any DT changes for tegra if needed. To be honest, I'm very much tempted to just drop this series. Even if that means keeping a totally broken DT binding. But frankly I don't have any energy left to debate DT stability. Thierry
On Thu, Jul 17, 2014 at 7:20 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Jul 10, 2014 at 12:15:28PM +0200, Thierry Reding wrote: >> On Mon, Jul 07, 2014 at 09:45:46PM -0700, Olof Johansson wrote: > [...] >> > If you have to stay compatible, then I suggest you try to fill in >> > local driver variables with derivatives of the old properties (and >> > directly from the newer properties where you can). I haven't looked at >> > the specifics here so I don't know how hard it might be. >> > >> > If you are 100% sure that you don't have to stay compatible, then you >> > can remove the code handling the old bindings. Still, even then I am a >> > little worried about dependencies (and more importantly conflicts) >> > between these dtsi changes and others done by tegra platform code for >> > this release. I suppose that can be resolved by having this as a base >> > of any DT changes for tegra if needed. >> >> To be honest, I'm very much tempted to just drop this series. Even if >> that means keeping a totally broken DT binding. But frankly I don't have >> any energy left to debate DT stability. > > So this kept bugging me and I couldn't leave it alone after all. How > about if I squash in the attached patch. I've verified that that keeps > compatibility with old device trees on TrimSlice and Beaver. I think the > remainder of the series could still remain as-is (the top few commits > that you said shouldn't be there) if I squash this into > > PCI: tegra: Implement accurate power supply scheme > > That way the binding will be the new one so that people don't get any > wrong ideas about taking shortcuts while still preserving compatibility > with existing DTBs. Taking a quick look at the patch, it looks sane to me and pretty much exactly what I would expect the code to look like w.r.t. handling old bindings. > Interestingly, despite my initial disgust for having to keep around old > code (it's in fact new code in this case) for compatibility reasons, it > ended up making the code look more mature. I'm glad it went that way. It shouldn't be _too_ bad to keep the compatibility with old bindings in the code like this in most cases, it's mostly a matter of filling in the newer structures like you did in this patch. -Olof
On 07/06/2014 06:38 PM, Olof Johansson wrote: > On Mon, Jun 23, 2014 at 03:23:44PM -0600, Stephen Warren wrote: >> This branch reworks the set of regulators that the Tegra PCIe driver >> uses, so that the driver and DT bindings more correctly model what's >> really going on in HW. >> >> I've made this a separate branch in case it needs to be pulled into the >> PCIe tree to resolve any conflicts. Any branch that adds Tegra124 >> support to the PCIe driver will need to be based on this branch, and >> such patches might show up for 3.17, and be taken through the ARM tree >> so we can manage our own dependencies. > > Isn't PCI broken if you boot with an older device tree now? > > I would like to see this as two branches: One to the PCI driver, and one > modifying DT contents. The PCI driver should remain working for old DTs, > so the last couple of commits on this branch can't be there. I thought it was still completely legal to change DT bindings that were not yet considered stable. None of the Tegra bindings have yet been deemed stable.
This branch reworks the set of regulators that the Tegra PCIe driver uses, so that the driver and DT bindings more correctly model what's really going on in HW. I've made this a separate branch in case it needs to be pulled into the PCIe tree to resolve any conflicts. Any branch that adds Tegra124 support to the PCIe driver will need to be based on this branch, and such patches might show up for 3.17, and be taken through the ARM tree so we can manage our own dependencies. ---------------------------------------------------------------- The following changes since commit 7171511eaec5bf23fb06078f59784a3a0626b38f: Linux 3.16-rc1 are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git tegra-for-3.17-pcie-regulators for you to fetch changes up to f4dfa465e03f0faf3f52d0e13a2a912c0998ddce: ARM: tegra: Remove legacy PCIe power supply properties ---------------------------------------------------------------- Thierry Reding (5): PCI: tegra: Overhaul regulator usage ARM: tegra: Add new PCIe regulator properties PCI: tegra: Implement accurate power supply scheme PCI: tegra: Remove deprecated power supply properties ARM: tegra: Remove legacy PCIe power supply properties .../bindings/pci/nvidia,tegra20-pcie.txt | 30 +++- arch/arm/boot/dts/tegra20-harmony.dts | 8 +- arch/arm/boot/dts/tegra20-tamonten.dtsi | 7 +- arch/arm/boot/dts/tegra20-trimslice.dts | 8 +- arch/arm/boot/dts/tegra30-beaver.dts | 12 +- arch/arm/boot/dts/tegra30-cardhu.dtsi | 11 +- drivers/pci/host/pci-tegra.c | 148 +++++++++++-------- 7 files changed, 148 insertions(+), 76 deletions(-)