| Message ID | 20171012134743.10625-1-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: > The meson secure monitor seems to be compatible with more SoCs than > initially thought. Let's use the most generic compatible he have in > DT instead of the gxbb specific one > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 ++-- > drivers/firmware/meson/meson_sm.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) Seems like a pointless, not backwards compatible change to me. In the end, it's just a string to match on. Who cares what the string is. Rob
On Tue, 2017-10-17 at 15:50 -0500, Rob Herring wrote: > On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: > > The meson secure monitor seems to be compatible with more SoCs than > > initially thought. Let's use the most generic compatible he have in > > DT instead of the gxbb specific one > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > --- > > Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 ++-- > > drivers/firmware/meson/meson_sm.c | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > Seems like a pointless, not backwards compatible The related DTS have both the SoC family compatible and the SoC specific compatible. So it is backward compatible (even if this contraints is relaxed on meson because we are figuring out what is shared between SoCs) We are asked to put SoC family and SoC specific compatibles in DTS. What's the point of that if we are going to match on whatever was there first ? > change to me. In the > end, it's just a string to match on. Who cares what the string is. > The matched string has to keep some sort of logic to be maintainable. It should be clear what the data (or the absence of data) attached to a string relates to. If we really don't care what the string is, we could pick words at random in the dictionary ... it would be chaos, but at least it would not be confusing > Rob
Rob Herring <robh@kernel.org> writes: > On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: >> The meson secure monitor seems to be compatible with more SoCs than >> initially thought. Let's use the most generic compatible he have in >> DT instead of the gxbb specific one >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 ++-- >> drivers/firmware/meson/meson_sm.c | 4 ++-- >> 2 files changed, 4 insertions(+), 4 deletions(-) > > Seems like a pointless, not backwards compatible change to me. I've verified that it's backwards compatible with existing upstream DTs. > end, it's just a string to match on. Who cares what the string is. As platform maintiner, I very much care what the strings are and I want it to be coherent with the platform generic names, and I want the SoC-specific strings to correspond to the actual SoC names. Kevin
On Thu, Oct 19, 2017 at 5:25 AM, Kevin Hilman <khilman@baylibre.com> wrote: > Rob Herring <robh@kernel.org> writes: > >> On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: >>> The meson secure monitor seems to be compatible with more SoCs than >>> initially thought. Let's use the most generic compatible he have in >>> DT instead of the gxbb specific one >>> >>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >>> --- >>> Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 ++-- >>> drivers/firmware/meson/meson_sm.c | 4 ++-- >>> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> Seems like a pointless, not backwards compatible change to me. > > I've verified that it's backwards compatible with existing upstream DTs. Perhaps if you all are documenting only what the driver uses, not what the dts can have as Jerome said. >> end, it's just a string to match on. Who cares what the string is. > > As platform maintiner, I very much care what the strings are and I want > it to be coherent with the platform generic names, and I want the > SoC-specific strings to correspond to the actual SoC names. The most specific compatible should be, absolutely. The fallbacks can be anything really. Ideally, they are the compatible string for the 1st SoC with "the same" compatible IP. Could be another vendor entirely even because mergers happen. Rob
On Thu, 2017-10-19 at 16:18 -0500, Rob Herring wrote: > On Thu, Oct 19, 2017 at 5:25 AM, Kevin Hilman <khilman@baylibre.com> wrote: > > Rob Herring <robh@kernel.org> writes: > > > > > On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: > > > > The meson secure monitor seems to be compatible with more SoCs than > > > > initially thought. Let's use the most generic compatible he have in > > > > DT instead of the gxbb specific one > > > > > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > > --- > > > > Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 ++-- > > > > drivers/firmware/meson/meson_sm.c | 4 ++-- > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > Seems like a pointless, not backwards compatible change to me. > > > > I've verified that it's backwards compatible with existing upstream DTs. > > Perhaps if you all are documenting only what the driver uses, not what > the dts can have as Jerome said. > > > > end, it's just a string to match on. Who cares what the string is. > > > > As platform maintiner, I very much care what the strings are and I want > > it to be coherent with the platform generic names, and I want the > > SoC-specific strings to correspond to the actual SoC names. > > The most specific compatible should be, absolutely. The fallbacks can > be anything really. Ideally, they are the compatible string for the > 1st SoC with "the same" compatible IP. Could be another vendor > entirely even because mergers happen. Then what's your problem with these patches again ? I am just asking the driver to match the generic binding instead of the SoC specific, because we are also using it on other SoC, as explain in the patch comment. Does not seems that "pointless" to me. Right now the driver match only on: vendor,soc-one in dts, we have compatible = "vendor,family", "vendor,soc-one" but it is compatible with soc-two as well. to match we would have to put "vendor,soc-one" as well which is a mess By expressing correctly what the driver is compatible with, "vendor,family" we can dts that makes sense for soc-two as well compatible = "vendor,family", "vendor,soc-two" Same goes for the other patches > > Rob
On Fri, Oct 20, 2017 at 3:30 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Thu, 2017-10-19 at 16:18 -0500, Rob Herring wrote: >> On Thu, Oct 19, 2017 at 5:25 AM, Kevin Hilman <khilman@baylibre.com> wrote: >> > Rob Herring <robh@kernel.org> writes: >> > >> > > On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: >> > > > The meson secure monitor seems to be compatible with more SoCs than >> > > > initially thought. Let's use the most generic compatible he have in >> > > > DT instead of the gxbb specific one >> > > > >> > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> > > > --- >> > > > Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 ++-- >> > > > drivers/firmware/meson/meson_sm.c | 4 ++-- >> > > > 2 files changed, 4 insertions(+), 4 deletions(-) >> > > >> > > Seems like a pointless, not backwards compatible change to me. >> > >> > I've verified that it's backwards compatible with existing upstream DTs. >> >> Perhaps if you all are documenting only what the driver uses, not what >> the dts can have as Jerome said. >> >> > > end, it's just a string to match on. Who cares what the string is. >> > >> > As platform maintiner, I very much care what the strings are and I want >> > it to be coherent with the platform generic names, and I want the >> > SoC-specific strings to correspond to the actual SoC names. >> >> The most specific compatible should be, absolutely. The fallbacks can >> be anything really. Ideally, they are the compatible string for the >> 1st SoC with "the same" compatible IP. Could be another vendor >> entirely even because mergers happen. > > Then what's your problem with these patches again ? Removal of the SoC specific compatible and breaking compatibility. Kevin says compatibility is not broken, but it obviously it based on the example and the driver change. So that can only mean your dts file doesn't match the example. > I am just asking the driver to match the generic binding instead of the SoC > specific, because we are also using it on other SoC, as explain in the patch > comment. Does not seems that "pointless" to me. > > Right now the driver match only on: vendor,soc-one > in dts, we have compatible = "vendor,family", "vendor,soc-one" That's backwards if I understand this right. It should be most specific first, but that's a separate issue. > but it is compatible with soc-two as well. > to match we would have to put "vendor,soc-one" as well which is a mess Why? That's how DT works and every other platform follows. Either you have: "vendor,soc-one", "vendor,family" "vendor,soc-two", "vendor,family" or "vendor,soc-one" "vendor,soc-two", "vendor,soc-one" The latter is how DT has existed and worked for 20+ years. The former is what we allow because for some reason people have such an aversion to saying soc2 is compatible with soc1. Either one is fine, but the documentation must be clear what the constraints are for the dts file. For example, "vendor,soc-two" or "vendor,family" alone are not valid. There's plenty of examples to follow. Renesas is one using "vendor,family" extensively. > By expressing correctly what the driver is compatible with, "vendor,family" > we can dts that makes sense for soc-two as well > compatible = "vendor,family", "vendor,soc-two" Binding docs may live in the kernel, but they are separate from drivers. They describe the constraints of the dts files. There's no reason to document what the driver wants because I can read the driver source. I can't say the same thing about the dts file. The dts may not come from the kernel and one dts file is not all possible options for a given binding. Rob
On Fri, 2017-10-20 at 14:34 -0500, Rob Herring wrote: > On Fri, Oct 20, 2017 at 3:30 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Thu, 2017-10-19 at 16:18 -0500, Rob Herring wrote: > > > On Thu, Oct 19, 2017 at 5:25 AM, Kevin Hilman <khilman@baylibre.com> > > > wrote: > > > > Rob Herring <robh@kernel.org> writes: > > > > > > > > > On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: > > > > > > The meson secure monitor seems to be compatible with more SoCs than > > > > > > initially thought. Let's use the most generic compatible he have in > > > > > > DT instead of the gxbb specific one > > > > > > > > > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > > > > --- > > > > > > Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 > > > > > > ++-- > > > > > > drivers/firmware/meson/meson_sm.c | 4 > > > > > > ++-- > > > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > Seems like a pointless, not backwards compatible change to me. > > > > > > > > I've verified that it's backwards compatible with existing upstream DTs. > > > > > > Perhaps if you all are documenting only what the driver uses, not what > > > the dts can have as Jerome said. > > > > > > > > end, it's just a string to match on. Who cares what the string is. > > > > > > > > As platform maintiner, I very much care what the strings are and I want > > > > it to be coherent with the platform generic names, and I want the > > > > SoC-specific strings to correspond to the actual SoC names. > > > > > > The most specific compatible should be, absolutely. The fallbacks can > > > be anything really. Ideally, they are the compatible string for the > > > 1st SoC with "the same" compatible IP. Could be another vendor > > > entirely even because mergers happen. > > > > Then what's your problem with these patches again ? > > Removal of the SoC specific compatible and breaking compatibility. > Kevin says compatibility is not broken, but it obviously it based on > the example and the driver change. So that can only mean your dts file > doesn't match the example. I believe it does, but I suppose I have missed something. Currently the driver only matches on: .compatible = "amlogic,meson-gxbb-sm" (drivers/firmware/meson_sm.c) The only device-tree file using it is meson-gx.dtsi: compatible = "amlogic,meson-gx-sm", "amlogic,meson-gxbb-sm"; So: 1) yes the order is backward (thx for pointing this out) 2) by changing the compatible matched by the driver from "amlogic,meson-gxbb-sm" to "amlogic,meson-gx-sm", I don't think I am breaking anything. Feel free to point out why this is wrong because I don't get it. > > > I am just asking the driver to match the generic binding instead of the SoC > > specific, because we are also using it on other SoC, as explain in the patch > > comment. Does not seems that "pointless" to me. > > > > Right now the driver match only on: vendor,soc-one > > in dts, we have compatible = "vendor,family", "vendor,soc-one" > > That's backwards if I understand this right. It should be most > specific first, but that's a separate issue. > > > but it is compatible with soc-two as well. > > to match we would have to put "vendor,soc-one" as well which is a mess > > Why? That's how DT works and every other platform follows. Either you have: > > "vendor,soc-one", "vendor,family" > "vendor,soc-two", "vendor,family" > > or > > "vendor,soc-one" > "vendor,soc-two", "vendor,soc-one" > > The latter is how DT has existed and worked for 20+ years. The former > is what we allow because for some reason people have such an aversion > to saying soc2 is compatible with soc1. > > Either one is fine, but the documentation must be clear what the > constraints are for the dts file. For example, "vendor,soc-two" or > "vendor,family" alone are not valid. There's plenty of examples to except that the DT file where it is used is a soc family dtsi. meson-gx.dtsi is the common DT for gxbb and gxl family. Wouldn't it make sense to have the SoC generic compatible alone here ? and override it in the SoC DT if necessary ? > follow. Renesas is one using "vendor,family" extensively. > > > By expressing correctly what the driver is compatible with, "vendor,family" > > we can dts that makes sense for soc-two as well > > compatible = "vendor,family", "vendor,soc-two" Honestly, I don't care which of the 2 solutions we take. The important thing for me is to eliminate the confusion introduced by the generic-compatible being here and useless. The driver is indeed generic, but the related compatible is not matched. This makes no sense. So either: * we use "vendor,soc-one", "vendor,family" model: I believe this what I'm trying to do here. For this, we need to match the soc generic compatible in the driver. * we use the "vendor,soc-two", "vendor,soc-one" model : The driver keeps on matching the SoC specific compatible. We won't ever match the generic one with this model, what is the point of keeping it around in DT ? Should we remove it ? Maybe the later is better, it would end up just being a removal of an undocumented property from DT. > > Binding docs may live in the kernel, but they are separate from > drivers. They describe the constraints of the dts files. There's no > reason to document what the driver wants because I can read the driver > source. I can't say the same thing about the dts file. The dts may not > come from the kernel and one dts file is not all possible options for > a given binding. > > Rob
On Mon, Oct 23, 2017 at 3:13 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Fri, 2017-10-20 at 14:34 -0500, Rob Herring wrote: >> On Fri, Oct 20, 2017 at 3:30 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> > On Thu, 2017-10-19 at 16:18 -0500, Rob Herring wrote: >> > > On Thu, Oct 19, 2017 at 5:25 AM, Kevin Hilman <khilman@baylibre.com> >> > > wrote: >> > > > Rob Herring <robh@kernel.org> writes: >> > > > >> > > > > On Thu, Oct 12, 2017 at 03:47:43PM +0200, Jerome Brunet wrote: >> > > > > > The meson secure monitor seems to be compatible with more SoCs than >> > > > > > initially thought. Let's use the most generic compatible he have in >> > > > > > DT instead of the gxbb specific one >> > > > > > >> > > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> > > > > > --- >> > > > > > Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 >> > > > > > ++-- >> > > > > > drivers/firmware/meson/meson_sm.c | 4 >> > > > > > ++-- >> > > > > > 2 files changed, 4 insertions(+), 4 deletions(-) >> > > > > >> > > > > Seems like a pointless, not backwards compatible change to me. >> > > > >> > > > I've verified that it's backwards compatible with existing upstream DTs. >> > > >> > > Perhaps if you all are documenting only what the driver uses, not what >> > > the dts can have as Jerome said. >> > > >> > > > > end, it's just a string to match on. Who cares what the string is. >> > > > >> > > > As platform maintiner, I very much care what the strings are and I want >> > > > it to be coherent with the platform generic names, and I want the >> > > > SoC-specific strings to correspond to the actual SoC names. >> > > >> > > The most specific compatible should be, absolutely. The fallbacks can >> > > be anything really. Ideally, they are the compatible string for the >> > > 1st SoC with "the same" compatible IP. Could be another vendor >> > > entirely even because mergers happen. >> > >> > Then what's your problem with these patches again ? >> >> Removal of the SoC specific compatible and breaking compatibility. >> Kevin says compatibility is not broken, but it obviously it based on >> the example and the driver change. So that can only mean your dts file >> doesn't match the example. > > I believe it does, but I suppose I have missed something. > > Currently the driver only matches on: > .compatible = "amlogic,meson-gxbb-sm" (drivers/firmware/meson_sm.c) > > The only device-tree file using it is > meson-gx.dtsi: compatible = "amlogic,meson-gx-sm", "amlogic,meson-gxbb-sm"; THIS is not what the binding example shows which is precisely the problem. Reading the binding doc and driver alone makes it look like you are breaking compatibility. > So: > 1) yes the order is backward (thx for pointing this out) > 2) by changing the compatible matched by the driver from "amlogic,meson-gxbb-sm" > to "amlogic,meson-gx-sm", I don't think I am breaking anything. Feel free to > point out why this is wrong because I don't get it. You are not breaking compatibility. That's not what I'm saying. If I had followed the binding doc and wrote my own dts, then compatibility would have been broken. >> > I am just asking the driver to match the generic binding instead of the SoC >> > specific, because we are also using it on other SoC, as explain in the patch >> > comment. Does not seems that "pointless" to me. >> > >> > Right now the driver match only on: vendor,soc-one >> > in dts, we have compatible = "vendor,family", "vendor,soc-one" >> >> That's backwards if I understand this right. It should be most >> specific first, but that's a separate issue. >> >> > but it is compatible with soc-two as well. >> > to match we would have to put "vendor,soc-one" as well which is a mess >> >> Why? That's how DT works and every other platform follows. Either you have: >> >> "vendor,soc-one", "vendor,family" >> "vendor,soc-two", "vendor,family" >> >> or >> >> "vendor,soc-one" >> "vendor,soc-two", "vendor,soc-one" >> >> The latter is how DT has existed and worked for 20+ years. The former >> is what we allow because for some reason people have such an aversion >> to saying soc2 is compatible with soc1. >> >> Either one is fine, but the documentation must be clear what the >> constraints are for the dts file. For example, "vendor,soc-two" or >> "vendor,family" alone are not valid. There's plenty of examples to > > except that the DT file where it is used is a soc family dtsi. > meson-gx.dtsi is the common DT for gxbb and gxl family. Wouldn't it make sense > to have the SoC generic compatible alone here ? and override it in the SoC DT if > necessary ? That's a detail I don't really care so much about because that's just dts partitioning. I mainly care how the dtb ends up looking and that must have an SoC specific compatible. I will say though, I think that overriding properties like compatible would not be the best style. Generally, enough things change like memory address, irq numbers, etc. from SoC to SoC that it's not worth trying to share dtsi files that way. >> follow. Renesas is one using "vendor,family" extensively. >> >> > By expressing correctly what the driver is compatible with, "vendor,family" >> > we can dts that makes sense for soc-two as well >> > compatible = "vendor,family", "vendor,soc-two" > > Honestly, I don't care which of the 2 solutions we take. The important thing for > me is to eliminate the confusion introduced by the generic-compatible being here > and useless. The driver is indeed generic, but the related compatible is not > matched. This makes no sense. The driver change is fine. The dts is fine too it seems, other than the string ordering. It's the binding doc that is not fine (read the last paragraph again if you still don't understand why). It needs to document which of the below you are doing. > So either: > * we use "vendor,soc-one", "vendor,family" model: I believe this what I'm trying > to do here. For this, we need to match the soc generic compatible in the > driver. > > * we use the "vendor,soc-two", "vendor,soc-one" model : The driver keeps on > matching the SoC specific compatible. We won't ever match the generic one with > this model, what is the point of keeping it around in DT ? Should we remove it ? > > Maybe the later is better, it would end up just being a removal of an > undocumented property from DT. > >> >> Binding docs may live in the kernel, but they are separate from >> drivers. They describe the constraints of the dts files. There's no >> reason to document what the driver wants because I can read the driver >> source. I can't say the same thing about the dts file. The dts may not >> come from the kernel and one dts file is not all possible options for >> a given binding.
diff --git a/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt b/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt index c248cd44f727..13a7815ac2b9 100644 --- a/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt +++ b/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt @@ -4,12 +4,12 @@ In the Amlogic SoCs the Secure Monitor code is used to provide access to the NVMEM, enable JTAG, set USB boot, etc... Required properties for the secure monitor node: -- compatible: Should be "amlogic,meson-gxbb-sm" +- compatible: Should be "amlogic,meson-gx-sm" Example: firmware { sm: secure-monitor { - compatible = "amlogic,meson-gxbb-sm"; + compatible = "amlogic,meson-gx-sm"; }; }; diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c index ff204421117b..c83f7be2b1f4 100644 --- a/drivers/firmware/meson/meson_sm.c +++ b/drivers/firmware/meson/meson_sm.c @@ -38,7 +38,7 @@ struct meson_sm_chip { struct meson_sm_cmd cmd[]; }; -struct meson_sm_chip gxbb_chip = { +struct meson_sm_chip gx_chip = { .shmem_size = SZ_4K, .cmd_shmem_in_base = 0x82000020, .cmd_shmem_out_base = 0x82000021, @@ -213,7 +213,7 @@ int meson_sm_call_write(void *buffer, unsigned int size, unsigned int cmd_index, EXPORT_SYMBOL(meson_sm_call_write); static const struct of_device_id meson_sm_ids[] = { - { .compatible = "amlogic,meson-gxbb-sm", .data = &gxbb_chip }, + { .compatible = "amlogic,meson-gx-sm", .data = &gx_chip }, { /* sentinel */ }, };
The meson secure monitor seems to be compatible with more SoCs than initially thought. Let's use the most generic compatible he have in DT instead of the gxbb specific one Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- Documentation/devicetree/bindings/firmware/meson/meson_sm.txt | 4 ++-- drivers/firmware/meson/meson_sm.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)