| Message ID | 1381134884-5816-28-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/07/2013 02:34 AM, Thierry Reding wrote: > The gr2d hardware in Tegra114 is compatible with that of Tegra20 and > Tegra30. No functionaly changes are required. Similarly here, if the HW is 100% backwards-compatible, there's no need to add compatible values to the driver.
On 12.10.2013 01:43, Stephen Warren wrote: > On 10/07/2013 02:34 AM, Thierry Reding wrote: >> The gr2d hardware in Tegra114 is compatible with that of Tegra20 and >> Tegra30. No functionaly changes are required. > Similarly here, if the HW is 100% backwards-compatible, there's no need > to add compatible values to the driver. We've used this mechanism for attaching a per-hw-version data structure in match table to accomodate differences in how the hardware is power gated, reset, booted, some per-soc performance related changes etc. It's also used in staging features for new chips, such as disabling power features when they're not working/verified yet. Upstream driver is not yet in a state where that is relevant. With this, would we still be able to do that with match table? It sounds like we could, because we can still (even with multiple compatible properties) add separate entries in match table and I guess the compatible properties matched in order. Terje
On Mon, Oct 14, 2013 at 08:58:34AM +0300, Terje Bergström wrote: > On 12.10.2013 01:43, Stephen Warren wrote: > > On 10/07/2013 02:34 AM, Thierry Reding wrote: > >> The gr2d hardware in Tegra114 is compatible with that of Tegra20 and > >> Tegra30. No functionaly changes are required. > > Similarly here, if the HW is 100% backwards-compatible, there's no need > > to add compatible values to the driver. > > We've used this mechanism for attaching a per-hw-version data structure > in match table to accomodate differences in how the hardware is power > gated, reset, booted, some per-soc performance related changes etc. It's > also used in staging features for new chips, such as disabling power > features when they're not working/verified yet. > > Upstream driver is not yet in a state where that is relevant. > > With this, would we still be able to do that with match table? It sounds > like we could, because we can still (even with multiple compatible > properties) add separate entries in match table and I guess the > compatible properties matched in order. Yes, as long as the device tree files includes the most specific value in the compatible this should still be possible. So we'd have this: gr2d@54140000 { compatible = "nvida,tegra114-gr2d", "nvidia,tegra20-gr2d"; ... }; and the driver will match on "nvidia,tegra20-gr2d" if the more specific "nvidia,tegra114-gr2d" is not there. When the driver is updated to support Tegra114 specific functionality, then a more specific entry can be added to the compatible table to handle it. Thierry
On 10/13/2013 11:58 PM, Terje Bergström wrote: > On 12.10.2013 01:43, Stephen Warren wrote: >> On 10/07/2013 02:34 AM, Thierry Reding wrote: >>> The gr2d hardware in Tegra114 is compatible with that of Tegra20 and >>> Tegra30. No functionaly changes are required. >> >> Similarly here, if the HW is 100% backwards-compatible, there's no need >> to add compatible values to the driver. > > We've used this mechanism for attaching a per-hw-version data structure > in match table to accomodate differences in how the hardware is power > gated, reset, booted, some per-soc performance related changes etc. If there are differences in those aspects of the HW, such that a driver written only to the full specification of e.g. Tegra30 would not work on Tegra114, then the HW is not actually compatible, and hence we do need multiple compatible values in DT, and entries in the of_match table. It sounds like the statement in the commit description: >>> The gr2d hardware in Tegra114 is compatible with that of Tegra20 and >>> Tegra30. No functionaly changes are required. Might not be absolutely accurate in terms of HW, but only in terms of the features that the driver uses so far. It'd be good to explicitly qualify this in the commit description. ... > Upstream driver is not yet in a state where that is relevant. The compatible values should be picked based on the full feature-set of the HW, not based on the subset of features supported by a particular driver.
On 10/14/2013 08:00 AM, Thierry Reding wrote: > On Mon, Oct 14, 2013 at 08:58:34AM +0300, Terje Bergström wrote: >> On 12.10.2013 01:43, Stephen Warren wrote: >>> On 10/07/2013 02:34 AM, Thierry Reding wrote: >>>> The gr2d hardware in Tegra114 is compatible with that of >>>> Tegra20 and Tegra30. No functionaly changes are required. >>> Similarly here, if the HW is 100% backwards-compatible, there's >>> no need to add compatible values to the driver. >> >> We've used this mechanism for attaching a per-hw-version data >> structure in match table to accomodate differences in how the >> hardware is power gated, reset, booted, some per-soc performance >> related changes etc. It's also used in staging features for new >> chips, such as disabling power features when they're not >> working/verified yet. >> >> Upstream driver is not yet in a state where that is relevant. >> >> With this, would we still be able to do that with match table? It >> sounds like we could, because we can still (even with multiple >> compatible properties) add separate entries in match table and I >> guess the compatible properties matched in order. > > Yes, as long as the device tree files includes the most specific > value in the compatible this should still be possible. So we'd have > this: > > gr2d@54140000 { compatible = "nvida,tegra114-gr2d", > "nvidia,tegra20-gr2d"; ... }; > > and the driver will match on "nvidia,tegra20-gr2d" if the more > specific "nvidia,tegra114-gr2d" is not there. When the driver is > updated to support Tegra114 specific functionality, then a more > specific entry can be added to the compatible table to handle it. True, but the DT fragment above is also only accurate /if/ a driver that only knows about "nvidia,tegra20-gr2d" can operate 100% of the features in Tegra20 HW on Tegra114 HW forever.
On 14.10.2013 21:14, Stephen Warren wrote: > On 10/14/2013 08:00 AM, Thierry Reding wrote: >> Yes, as long as the device tree files includes the most specific >> value in the compatible this should still be possible. So we'd have >> this: >> >> gr2d@54140000 { compatible = "nvida,tegra114-gr2d", >> "nvidia,tegra20-gr2d"; ... }; >> >> and the driver will match on "nvidia,tegra20-gr2d" if the more >> specific "nvidia,tegra114-gr2d" is not there. When the driver is >> updated to support Tegra114 specific functionality, then a more >> specific entry can be added to the compatible table to handle it. > > True, but the DT fragment above is also only accurate /if/ a driver > that only knows about "nvidia,tegra20-gr2d" can operate 100% of the > features in Tegra20 HW on Tegra114 HW forever. I don't know of any hardware incompatibility. The only difference is related to something not directly to 2D. We moved host1x away from the power domain, so in Tegra114 we're able to power gate 2D and EPP. The DVFS tables are also different. I'd say say adding the compatible property "nvidia,tegra20-gr2d" for Tegra114's 2D is accurate and we're able to use the match table to drive any SW policy differences. Terje
On Mon, Oct 14, 2013 at 12:14:47PM -0600, Stephen Warren wrote: > On 10/14/2013 08:00 AM, Thierry Reding wrote: > > On Mon, Oct 14, 2013 at 08:58:34AM +0300, Terje Bergström wrote: > >> On 12.10.2013 01:43, Stephen Warren wrote: > >>> On 10/07/2013 02:34 AM, Thierry Reding wrote: > >>>> The gr2d hardware in Tegra114 is compatible with that of > >>>> Tegra20 and Tegra30. No functionaly changes are required. > >>> Similarly here, if the HW is 100% backwards-compatible, there's > >>> no need to add compatible values to the driver. > >> > >> We've used this mechanism for attaching a per-hw-version data > >> structure in match table to accomodate differences in how the > >> hardware is power gated, reset, booted, some per-soc performance > >> related changes etc. It's also used in staging features for new > >> chips, such as disabling power features when they're not > >> working/verified yet. > >> > >> Upstream driver is not yet in a state where that is relevant. > >> > >> With this, would we still be able to do that with match table? It > >> sounds like we could, because we can still (even with multiple > >> compatible properties) add separate entries in match table and I > >> guess the compatible properties matched in order. > > > > Yes, as long as the device tree files includes the most specific > > value in the compatible this should still be possible. So we'd have > > this: > > > > gr2d@54140000 { compatible = "nvida,tegra114-gr2d", > > "nvidia,tegra20-gr2d"; ... }; > > > > and the driver will match on "nvidia,tegra20-gr2d" if the more > > specific "nvidia,tegra114-gr2d" is not there. When the driver is > > updated to support Tegra114 specific functionality, then a more > > specific entry can be added to the compatible table to handle it. > > True, but the DT fragment above is also only accurate /if/ a driver > that only knows about "nvidia,tegra20-gr2d" can operate 100% of the > features in Tegra20 HW on Tegra114 HW forever. Yes, but given that we also list "nvidia,tegra114-gr2d" in the property it will be possible to add that to the driver when new functionality is implemented and immediately take advantage of it on Tegra114 hardware with an old device tree file which has both compatible values. Thierry
On 10/14/2013 11:51 PM, Terje Bergström wrote: > On 14.10.2013 21:14, Stephen Warren wrote: >> On 10/14/2013 08:00 AM, Thierry Reding wrote: >>> Yes, as long as the device tree files includes the most specific >>> value in the compatible this should still be possible. So we'd have >>> this: >>> >>> gr2d@54140000 { compatible = "nvida,tegra114-gr2d", >>> "nvidia,tegra20-gr2d"; ... }; >>> >>> and the driver will match on "nvidia,tegra20-gr2d" if the more >>> specific "nvidia,tegra114-gr2d" is not there. When the driver is >>> updated to support Tegra114 specific functionality, then a more >>> specific entry can be added to the compatible table to handle it. >> >> True, but the DT fragment above is also only accurate /if/ a driver >> that only knows about "nvidia,tegra20-gr2d" can operate 100% of the >> features in Tegra20 HW on Tegra114 HW forever. > > I don't know of any hardware incompatibility. The only difference is > related to something not directly to 2D. We moved host1x away from the > power domain, so in Tegra114 we're able to power gate 2D and EPP. The > DVFS tables are also different. > > I'd say say adding the compatible property "nvidia,tegra20-gr2d" for > Tegra114's 2D is accurate and we're able to use the match table to drive > any SW policy differences. The compatible value shouldn't be used for SW policy differences. DT is about HW, not SW policy. You have to decide: either the HW is 100% backwards-compatible, or it's not. You can't decide that the HW is compatible, but then require different compatible values.
On 10/15/2013 02:37 AM, Thierry Reding wrote: > On Mon, Oct 14, 2013 at 12:14:47PM -0600, Stephen Warren wrote: >> On 10/14/2013 08:00 AM, Thierry Reding wrote: >>> On Mon, Oct 14, 2013 at 08:58:34AM +0300, Terje Bergström >>> wrote: >>>> On 12.10.2013 01:43, Stephen Warren wrote: >>>>> On 10/07/2013 02:34 AM, Thierry Reding wrote: >>>>>> The gr2d hardware in Tegra114 is compatible with that of >>>>>> Tegra20 and Tegra30. No functionaly changes are >>>>>> required. >>>>> Similarly here, if the HW is 100% backwards-compatible, >>>>> there's no need to add compatible values to the driver. >>>> >>>> We've used this mechanism for attaching a per-hw-version >>>> data structure in match table to accomodate differences in >>>> how the hardware is power gated, reset, booted, some per-soc >>>> performance related changes etc. It's also used in staging >>>> features for new chips, such as disabling power features when >>>> they're not working/verified yet. >>>> >>>> Upstream driver is not yet in a state where that is >>>> relevant. >>>> >>>> With this, would we still be able to do that with match >>>> table? It sounds like we could, because we can still (even >>>> with multiple compatible properties) add separate entries in >>>> match table and I guess the compatible properties matched in >>>> order. >>> >>> Yes, as long as the device tree files includes the most >>> specific value in the compatible this should still be possible. >>> So we'd have this: >>> >>> gr2d@54140000 { compatible = "nvida,tegra114-gr2d", >>> "nvidia,tegra20-gr2d"; ... }; >>> >>> and the driver will match on "nvidia,tegra20-gr2d" if the more >>> specific "nvidia,tegra114-gr2d" is not there. When the driver >>> is updated to support Tegra114 specific functionality, then a >>> more specific entry can be added to the compatible table to >>> handle it. >> >> True, but the DT fragment above is also only accurate /if/ a >> driver that only knows about "nvidia,tegra20-gr2d" can operate >> 100% of the features in Tegra20 HW on Tegra114 HW forever. > > Yes, but given that we also list "nvidia,tegra114-gr2d" in the > property it will be possible to add that to the driver when new > functionality is implemented and immediately take advantage of it > on Tegra114 hardware with an old device tree file which has both > compatible values. Taking advantage of new functionality isn't the issue. The issue is whether a driver that was written purely to the spec of Tegra20 can successfully operate on the HW. If it can't, then the HW is not compatible with Tegra20. Terje's previous comments sounded like the HW is not backwards-compatible, although his more recent comments make it sound like only SW policy differences, which shouldn't be part of DT anyway.
On Tue, Oct 15, 2013 at 09:19:18AM -0600, Stephen Warren wrote: > On 10/15/2013 02:37 AM, Thierry Reding wrote: > > On Mon, Oct 14, 2013 at 12:14:47PM -0600, Stephen Warren wrote: > >> On 10/14/2013 08:00 AM, Thierry Reding wrote: > >>> On Mon, Oct 14, 2013 at 08:58:34AM +0300, Terje Bergström > >>> wrote: > >>>> On 12.10.2013 01:43, Stephen Warren wrote: > >>>>> On 10/07/2013 02:34 AM, Thierry Reding wrote: > >>>>>> The gr2d hardware in Tegra114 is compatible with that of > >>>>>> Tegra20 and Tegra30. No functionaly changes are > >>>>>> required. > >>>>> Similarly here, if the HW is 100% backwards-compatible, > >>>>> there's no need to add compatible values to the driver. > >>>> > >>>> We've used this mechanism for attaching a per-hw-version > >>>> data structure in match table to accomodate differences in > >>>> how the hardware is power gated, reset, booted, some per-soc > >>>> performance related changes etc. It's also used in staging > >>>> features for new chips, such as disabling power features when > >>>> they're not working/verified yet. > >>>> > >>>> Upstream driver is not yet in a state where that is > >>>> relevant. > >>>> > >>>> With this, would we still be able to do that with match > >>>> table? It sounds like we could, because we can still (even > >>>> with multiple compatible properties) add separate entries in > >>>> match table and I guess the compatible properties matched in > >>>> order. > >>> > >>> Yes, as long as the device tree files includes the most > >>> specific value in the compatible this should still be possible. > >>> So we'd have this: > >>> > >>> gr2d@54140000 { compatible = "nvida,tegra114-gr2d", > >>> "nvidia,tegra20-gr2d"; ... }; > >>> > >>> and the driver will match on "nvidia,tegra20-gr2d" if the more > >>> specific "nvidia,tegra114-gr2d" is not there. When the driver > >>> is updated to support Tegra114 specific functionality, then a > >>> more specific entry can be added to the compatible table to > >>> handle it. > >> > >> True, but the DT fragment above is also only accurate /if/ a > >> driver that only knows about "nvidia,tegra20-gr2d" can operate > >> 100% of the features in Tegra20 HW on Tegra114 HW forever. > > > > Yes, but given that we also list "nvidia,tegra114-gr2d" in the > > property it will be possible to add that to the driver when new > > functionality is implemented and immediately take advantage of it > > on Tegra114 hardware with an old device tree file which has both > > compatible values. > > Taking advantage of new functionality isn't the issue. The issue is > whether a driver that was written purely to the spec of Tegra20 can > successfully operate on the HW. If it can't, then the HW is not > compatible with Tegra20. Terje's previous comments sounded like the HW > is not backwards-compatible, although his more recent comments make it > sound like only SW policy differences, which shouldn't be part of DT > anyway. Well, as good as I can tell it is backwards-compatible. I've been testing the code included in this series with the same simple test program that clears a rectangle on Tegra20, Tegra30 and Tegra114. Furthermore our internal register specification files are identical, with the exception of some whitespace changes for all three generations. I think that qualifies as backwards-compatible? Thierry
On 16.10.2013 11:48, Thierry Reding wrote: > On Tue, Oct 15, 2013 at 09:19:18AM -0600, Stephen Warren wrote: >> Taking advantage of new functionality isn't the issue. The issue is >> whether a driver that was written purely to the spec of Tegra20 can >> successfully operate on the HW. If it can't, then the HW is not >> compatible with Tegra20. Terje's previous comments sounded like the HW >> is not backwards-compatible, although his more recent comments make it >> sound like only SW policy differences, which shouldn't be part of DT >> anyway. > > Well, as good as I can tell it is backwards-compatible. I've been > testing the code included in this series with the same simple test > program that clears a rectangle on Tegra20, Tegra30 and Tegra114. > > Furthermore our internal register specification files are identical, > with the exception of some whitespace changes for all three generations. > I think that qualifies as backwards-compatible? A driver written for Tegra20 2D works with Tegra114 2D. Not necessarily vice versa, because Tegra114's driver can ask for higher clocks, or it might want to power gate 2D. I mentioned earlier bringup activities. Perhaps Stephen took that in a way I didn't intend. Terje
On 10/16/2013 02:48 AM, Thierry Reding wrote: > On Tue, Oct 15, 2013 at 09:19:18AM -0600, Stephen Warren wrote: >> On 10/15/2013 02:37 AM, Thierry Reding wrote: >>> On Mon, Oct 14, 2013 at 12:14:47PM -0600, Stephen Warren >>> wrote: >>>> On 10/14/2013 08:00 AM, Thierry Reding wrote: >>>>> On Mon, Oct 14, 2013 at 08:58:34AM +0300, Terje Bergström >>>>> wrote: >>>>>> On 12.10.2013 01:43, Stephen Warren wrote: >>>>>>> On 10/07/2013 02:34 AM, Thierry Reding wrote: >>>>>>>> The gr2d hardware in Tegra114 is compatible with that >>>>>>>> of Tegra20 and Tegra30. No functionaly changes are >>>>>>>> required. >>>>>>> Similarly here, if the HW is 100% >>>>>>> backwards-compatible, there's no need to add compatible >>>>>>> values to the driver. >>>>>> >>>>>> We've used this mechanism for attaching a per-hw-version >>>>>> data structure in match table to accomodate differences >>>>>> in how the hardware is power gated, reset, booted, some >>>>>> per-soc performance related changes etc. It's also used >>>>>> in staging features for new chips, such as disabling >>>>>> power features when they're not working/verified yet. >>>>>> >>>>>> Upstream driver is not yet in a state where that is >>>>>> relevant. >>>>>> >>>>>> With this, would we still be able to do that with match >>>>>> table? It sounds like we could, because we can still >>>>>> (even with multiple compatible properties) add separate >>>>>> entries in match table and I guess the compatible >>>>>> properties matched in order. >>>>> >>>>> Yes, as long as the device tree files includes the most >>>>> specific value in the compatible this should still be >>>>> possible. So we'd have this: >>>>> >>>>> gr2d@54140000 { compatible = "nvida,tegra114-gr2d", >>>>> "nvidia,tegra20-gr2d"; ... }; >>>>> >>>>> and the driver will match on "nvidia,tegra20-gr2d" if the >>>>> more specific "nvidia,tegra114-gr2d" is not there. When the >>>>> driver is updated to support Tegra114 specific >>>>> functionality, then a more specific entry can be added to >>>>> the compatible table to handle it. >>>> >>>> True, but the DT fragment above is also only accurate /if/ a >>>> driver that only knows about "nvidia,tegra20-gr2d" can >>>> operate 100% of the features in Tegra20 HW on Tegra114 HW >>>> forever. >>> >>> Yes, but given that we also list "nvidia,tegra114-gr2d" in the >>> property it will be possible to add that to the driver when >>> new functionality is implemented and immediately take advantage >>> of it on Tegra114 hardware with an old device tree file which >>> has both compatible values. >> >> Taking advantage of new functionality isn't the issue. The issue >> is whether a driver that was written purely to the spec of >> Tegra20 can successfully operate on the HW. If it can't, then the >> HW is not compatible with Tegra20. Terje's previous comments >> sounded like the HW is not backwards-compatible, although his >> more recent comments make it sound like only SW policy >> differences, which shouldn't be part of DT anyway. > > Well, as good as I can tell it is backwards-compatible. I've been > testing the code included in this series with the same simple test > program that clears a rectangle on Tegra20, Tegra30 and Tegra114. All that means is that the subset of features we use so far is compatible. > Furthermore our internal register specification files are > identical, with the exception of some whitespace changes for all > three generations. I think that qualifies as backwards-compatible? On the other hand, that sounds like an almost perfect definition of backwards-compatible. Can you also validate that any module clock/power/reset inputs are identical? If so, the case is closed:-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index d95be81..b9e0977 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -492,6 +492,7 @@ static const struct of_device_id host1x_drm_subdevs[] = { { .compatible = "nvidia,tegra114-dc", }, { .compatible = "nvidia,tegra114-dsi", }, { .compatible = "nvidia,tegra114-hdmi", }, + { .compatible = "nvidia,tegra114-gr2d", }, { /* sentinel */ } }; diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c index 9588072..f4e478b 100644 --- a/drivers/gpu/drm/tegra/gr2d.c +++ b/drivers/gpu/drm/tegra/gr2d.c @@ -232,6 +232,7 @@ static const struct tegra_drm_client_ops gr2d_ops = { }; static const struct of_device_id gr2d_match[] = { + { .compatible = "nvidia,tegra114-gr2d" }, { .compatible = "nvidia,tegra30-gr2d" }, { .compatible = "nvidia,tegra20-gr2d" }, { },
The gr2d hardware in Tegra114 is compatible with that of Tegra20 and Tegra30. No functionaly changes are required. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/gpu/drm/tegra/drm.c | 1 + drivers/gpu/drm/tegra/gr2d.c | 1 + 2 files changed, 2 insertions(+)