| Message ID | 1415116839-4323-2-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ezequiel, On Tue, Nov 04, 2014 at 01:00:38PM -0300, Ezequiel Garcia wrote: > The Armada 375 Z1 SoC revision is no longer supported. This commit > removes the quirk needed for the thermal sensor. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > .../devicetree/bindings/thermal/armada-thermal.txt | 8 -------- > drivers/thermal/armada_thermal.c | 20 -------------------- > 2 files changed, 28 deletions(-) > > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > index 4cf0249..4698e0e 100644 > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > @@ -5,17 +5,9 @@ Required properties: > - compatible: Should be set to one of the following: > marvell,armada370-thermal > marvell,armada375-thermal > - marvell,armada375-z1-thermal > marvell,armada380-thermal > marvell,armadaxp-thermal > > - Note: As the name suggests, "marvell,armada375-z1-thermal" > - applies for the SoC Z1 stepping only. On such stepping > - some quirks need to be done and the register offset differs > - from the one in the A0 stepping. > - The operating system may auto-detect the SoC stepping and > - update the compatible and register offsets at runtime. > - > - reg: Device's register space. > Two entries are expected, see the examples below. > The first one is required for the sensor register; I've no problem with removing support for the z1 stepping from the kernel. However, I don't think we should erase it from binding docs. I'm not sure what the DT maintainers think is the appropriate action here, but I'm thinking we could add a 'Deprecated' section at the end of the doc and move these hunks there. With a little rewording of course. I'm primarily concerned about users with older dtbs looking to upgrade, "Hey, wtf is marvell,armada375-z1-thermal? I looked in the binding docs and there's nothing there, do I replace it with marvell,armada375-thermal?" thx, Jason.
On 11/07/2014 12:26 AM, Jason Cooper wrote: > Ezequiel, > > On Tue, Nov 04, 2014 at 01:00:38PM -0300, Ezequiel Garcia wrote: >> The Armada 375 Z1 SoC revision is no longer supported. This commit >> removes the quirk needed for the thermal sensor. >> >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> >> --- >> .../devicetree/bindings/thermal/armada-thermal.txt | 8 -------- >> drivers/thermal/armada_thermal.c | 20 -------------------- >> 2 files changed, 28 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt >> index 4cf0249..4698e0e 100644 >> --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt >> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt >> @@ -5,17 +5,9 @@ Required properties: >> - compatible: Should be set to one of the following: >> marvell,armada370-thermal >> marvell,armada375-thermal >> - marvell,armada375-z1-thermal >> marvell,armada380-thermal >> marvell,armadaxp-thermal >> >> - Note: As the name suggests, "marvell,armada375-z1-thermal" >> - applies for the SoC Z1 stepping only. On such stepping >> - some quirks need to be done and the register offset differs >> - from the one in the A0 stepping. >> - The operating system may auto-detect the SoC stepping and >> - update the compatible and register offsets at runtime. >> - >> - reg: Device's register space. >> Two entries are expected, see the examples below. >> The first one is required for the sensor register; > > I've no problem with removing support for the z1 stepping from the > kernel. However, I don't think we should erase it from binding docs. > I'm not sure what the DT maintainers think is the appropriate action > here, but I'm thinking we could add a 'Deprecated' section at the end of > the doc and move these hunks there. With a little rewording of course. > > I'm primarily concerned about users with older dtbs looking to upgrade, > "Hey, wtf is marvell,armada375-z1-thermal? I looked in the binding docs > and there's nothing there, do I replace it with marvell,armada375-thermal?" > We can do that if you think it's really useful. However, I think we've designed this so *nobody* would actually have to put the z1 compatible string. The mvebu quirk (tries) to auto-detect it from the revision register and hot fix the compatible string. Moreover, I'm not at all sure *anyone* would have a Z1 board except early developers like us. Am I being too naive here?
Ezequiel, + Mark, Grant (sorry, thought you were Cc'd on the original patch) On Fri, Nov 07, 2014 at 09:41:19AM -0300, Ezequiel Garcia wrote: > On 11/07/2014 12:26 AM, Jason Cooper wrote: > > Ezequiel, > > > > On Tue, Nov 04, 2014 at 01:00:38PM -0300, Ezequiel Garcia wrote: > >> The Armada 375 Z1 SoC revision is no longer supported. This commit > >> removes the quirk needed for the thermal sensor. > >> > >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > >> --- > >> .../devicetree/bindings/thermal/armada-thermal.txt | 8 -------- > >> drivers/thermal/armada_thermal.c | 20 -------------------- > >> 2 files changed, 28 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > >> index 4cf0249..4698e0e 100644 > >> --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt > >> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > >> @@ -5,17 +5,9 @@ Required properties: > >> - compatible: Should be set to one of the following: > >> marvell,armada370-thermal > >> marvell,armada375-thermal > >> - marvell,armada375-z1-thermal > >> marvell,armada380-thermal > >> marvell,armadaxp-thermal > >> > >> - Note: As the name suggests, "marvell,armada375-z1-thermal" > >> - applies for the SoC Z1 stepping only. On such stepping > >> - some quirks need to be done and the register offset differs > >> - from the one in the A0 stepping. > >> - The operating system may auto-detect the SoC stepping and > >> - update the compatible and register offsets at runtime. > >> - > >> - reg: Device's register space. > >> Two entries are expected, see the examples below. > >> The first one is required for the sensor register; > > > > I've no problem with removing support for the z1 stepping from the > > kernel. However, I don't think we should erase it from binding docs. > > I'm not sure what the DT maintainers think is the appropriate action > > here, but I'm thinking we could add a 'Deprecated' section at the end of > > the doc and move these hunks there. With a little rewording of course. > > > > I'm primarily concerned about users with older dtbs looking to upgrade, > > "Hey, wtf is marvell,armada375-z1-thermal? I looked in the binding docs > > and there's nothing there, do I replace it with marvell,armada375-thermal?" > > > > We can do that if you think it's really useful. However, I think we've > designed this so *nobody* would actually have to put the z1 compatible > string. The mvebu quirk (tries) to auto-detect it from the revision > register and hot fix the compatible string. > > Moreover, I'm not at all sure *anyone* would have a Z1 board except > early developers like us. Am I being too naive here? I'm primarily concerned about dtbs in the wild which have that compatible string. But as you mention, and a quick grep proves, this compatible string was never in a dts{i} file. At least not in the kernel tree. It's not a rush, so I'd like to hear from Mark/Grant what they prefer for this sort of situation. It'll be in the git history either way, but not everyone goes there when the current docs don't mention something. :) thx, Jason.
Dear Ezequiel Garcia, On Fri, 07 Nov 2014 09:41:19 -0300, Ezequiel Garcia wrote: > We can do that if you think it's really useful. However, I think we've > designed this so *nobody* would actually have to put the z1 compatible > string. The mvebu quirk (tries) to auto-detect it from the revision > register and hot fix the compatible string. Correct. > Moreover, I'm not at all sure *anyone* would have a Z1 board except > early developers like us. Am I being too naive here? I confirm: those early revisions have only been provided to a very small selection of persons, mainly within Marvell, and only a few external partners which are all well aware of the early status of such SoC revisions. Thomas
On Tue, Nov 04, 2014 at 01:00:38PM -0300, Ezequiel Garcia wrote: > The Armada 375 Z1 SoC revision is no longer supported. This commit > removes the quirk needed for the thermal sensor. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > .../devicetree/bindings/thermal/armada-thermal.txt | 8 -------- > drivers/thermal/armada_thermal.c | 20 -------------------- > 2 files changed, 28 deletions(-) As Thomas and Ezequiel rightfully mentioned, the compatible string below has never been in a dts file. Our arch code detected the Z1, and changed the compatible on the fly. As a result, there shouldn't be any dtbs in the wild with this compatible string. If there are, it will be held by the few developers (Marvell, partners) that know what they are doing. Acked-by: Jason Cooper <jason@lakedaemon.net> thx, Jason. > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > index 4cf0249..4698e0e 100644 > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > @@ -5,17 +5,9 @@ Required properties: > - compatible: Should be set to one of the following: > marvell,armada370-thermal > marvell,armada375-thermal > - marvell,armada375-z1-thermal > marvell,armada380-thermal > marvell,armadaxp-thermal > > - Note: As the name suggests, "marvell,armada375-z1-thermal" > - applies for the SoC Z1 stepping only. On such stepping > - some quirks need to be done and the register offset differs > - from the one in the A0 stepping. > - The operating system may auto-detect the SoC stepping and > - update the compatible and register offsets at runtime. > - > - reg: Device's register space. > Two entries are expected, see the examples below. > The first one is required for the sensor register; > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c > index 9d1420a..9c8e783 100644 > --- a/drivers/thermal/armada_thermal.c > +++ b/drivers/thermal/armada_thermal.c > @@ -35,10 +35,6 @@ > #define PMU_TDC0_OTF_CAL_MASK (0x1 << 30) > #define PMU_TDC0_START_CAL_MASK (0x1 << 25) > > -#define A375_Z1_CAL_RESET_LSB 0x8011e214 > -#define A375_Z1_CAL_RESET_MSB 0x30a88019 > -#define A375_Z1_WORKAROUND_BIT BIT(9) > - > #define A375_UNIT_CONTROL_SHIFT 27 > #define A375_UNIT_CONTROL_MASK 0x7 > #define A375_READOUT_INVERT BIT(15) > @@ -124,24 +120,12 @@ static void armada375_init_sensor(struct platform_device *pdev, > struct armada_thermal_priv *priv) > { > unsigned long reg; > - bool quirk_needed = > - !!of_device_is_compatible(pdev->dev.of_node, > - "marvell,armada375-z1-thermal"); > - > - if (quirk_needed) { > - /* Ensure these registers have the default (reset) values */ > - writel(A375_Z1_CAL_RESET_LSB, priv->control); > - writel(A375_Z1_CAL_RESET_MSB, priv->control + 0x4); > - } > > reg = readl(priv->control + 4); > reg &= ~(A375_UNIT_CONTROL_MASK << A375_UNIT_CONTROL_SHIFT); > reg &= ~A375_READOUT_INVERT; > reg &= ~A375_HW_RESETn; > > - if (quirk_needed) > - reg |= A375_Z1_WORKAROUND_BIT; > - > writel(reg, priv->control + 4); > mdelay(20); > > @@ -260,10 +244,6 @@ static const struct of_device_id armada_thermal_id_table[] = { > .data = &armada375_data, > }, > { > - .compatible = "marvell,armada375-z1-thermal", > - .data = &armada375_data, > - }, > - { > .compatible = "marvell,armada380-thermal", > .data = &armada380_data, > }, > -- > 2.1.0 >
Ezequiel and Jason, On Sat, Nov 08, 2014 at 10:16:46PM -0500, Jason Cooper wrote: > On Tue, Nov 04, 2014 at 01:00:38PM -0300, Ezequiel Garcia wrote: > > The Armada 375 Z1 SoC revision is no longer supported. This commit > > removes the quirk needed for the thermal sensor. > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > --- > > .../devicetree/bindings/thermal/armada-thermal.txt | 8 -------- > > drivers/thermal/armada_thermal.c | 20 -------------------- > > 2 files changed, 28 deletions(-) > > As Thomas and Ezequiel rightfully mentioned, the compatible string below > has never been in a dts file. Our arch code detected the Z1, and > changed the compatible on the fly. As a result, there shouldn't be any > dtbs in the wild with this compatible string. If there are, it will be > held by the few developers (Marvell, partners) that know what they are > doing. First of, I would like to mention that best thing to avoid such situation is to be careful when documenting dt entries that represent hw that no one has access to (except internal people). > > Acked-by: Jason Cooper <jason@lakedaemon.net> > OK Jason. I saw you picked patch 2. Does it mean I can queue this one? BR, Eduardo Valentin > thx, > > Jason. > > > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > > index 4cf0249..4698e0e 100644 > > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt > > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt > > @@ -5,17 +5,9 @@ Required properties: > > - compatible: Should be set to one of the following: > > marvell,armada370-thermal > > marvell,armada375-thermal > > - marvell,armada375-z1-thermal > > marvell,armada380-thermal > > marvell,armadaxp-thermal > > > > - Note: As the name suggests, "marvell,armada375-z1-thermal" > > - applies for the SoC Z1 stepping only. On such stepping > > - some quirks need to be done and the register offset differs > > - from the one in the A0 stepping. > > - The operating system may auto-detect the SoC stepping and > > - update the compatible and register offsets at runtime. > > - > > - reg: Device's register space. > > Two entries are expected, see the examples below. > > The first one is required for the sensor register; > > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c > > index 9d1420a..9c8e783 100644 > > --- a/drivers/thermal/armada_thermal.c > > +++ b/drivers/thermal/armada_thermal.c > > @@ -35,10 +35,6 @@ > > #define PMU_TDC0_OTF_CAL_MASK (0x1 << 30) > > #define PMU_TDC0_START_CAL_MASK (0x1 << 25) > > > > -#define A375_Z1_CAL_RESET_LSB 0x8011e214 > > -#define A375_Z1_CAL_RESET_MSB 0x30a88019 > > -#define A375_Z1_WORKAROUND_BIT BIT(9) > > - > > #define A375_UNIT_CONTROL_SHIFT 27 > > #define A375_UNIT_CONTROL_MASK 0x7 > > #define A375_READOUT_INVERT BIT(15) > > @@ -124,24 +120,12 @@ static void armada375_init_sensor(struct platform_device *pdev, > > struct armada_thermal_priv *priv) > > { > > unsigned long reg; > > - bool quirk_needed = > > - !!of_device_is_compatible(pdev->dev.of_node, > > - "marvell,armada375-z1-thermal"); > > - > > - if (quirk_needed) { > > - /* Ensure these registers have the default (reset) values */ > > - writel(A375_Z1_CAL_RESET_LSB, priv->control); > > - writel(A375_Z1_CAL_RESET_MSB, priv->control + 0x4); > > - } > > > > reg = readl(priv->control + 4); > > reg &= ~(A375_UNIT_CONTROL_MASK << A375_UNIT_CONTROL_SHIFT); > > reg &= ~A375_READOUT_INVERT; > > reg &= ~A375_HW_RESETn; > > > > - if (quirk_needed) > > - reg |= A375_Z1_WORKAROUND_BIT; > > - > > writel(reg, priv->control + 4); > > mdelay(20); > > > > @@ -260,10 +244,6 @@ static const struct of_device_id armada_thermal_id_table[] = { > > .data = &armada375_data, > > }, > > { > > - .compatible = "marvell,armada375-z1-thermal", > > - .data = &armada375_data, > > - }, > > - { > > .compatible = "marvell,armada380-thermal", > > .data = &armada380_data, > > }, > > -- > > 2.1.0 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 20, 2014 at 03:38:06PM -0400, Eduardo Valentin wrote: > Ezequiel and Jason, > > On Sat, Nov 08, 2014 at 10:16:46PM -0500, Jason Cooper wrote: > > On Tue, Nov 04, 2014 at 01:00:38PM -0300, Ezequiel Garcia wrote: > > > The Armada 375 Z1 SoC revision is no longer supported. This commit > > > removes the quirk needed for the thermal sensor. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > > --- > > > .../devicetree/bindings/thermal/armada-thermal.txt | 8 -------- > > > drivers/thermal/armada_thermal.c | 20 -------------------- > > > 2 files changed, 28 deletions(-) > > > > As Thomas and Ezequiel rightfully mentioned, the compatible string below > > has never been in a dts file. Our arch code detected the Z1, and > > changed the compatible on the fly. As a result, there shouldn't be any > > dtbs in the wild with this compatible string. If there are, it will be > > held by the few developers (Marvell, partners) that know what they are > > doing. > > First of, I would like to mention that best thing to avoid such > situation is to be careful when documenting dt entries that represent hw > that no one has access to (except internal people). Agreed, mainline support for an SoC so early in it's lifetime was new for all of us. Lesson learned. > > > > Acked-by: Jason Cooper <jason@lakedaemon.net> > > > > OK Jason. I saw you picked patch 2. Does it mean I can queue this one? Yep. Take it away. thx, Jason.
Dear Jason Cooper, On Fri, 21 Nov 2014 15:18:58 -0500, Jason Cooper wrote: > > First of, I would like to mention that best thing to avoid such > > situation is to be careful when documenting dt entries that represent hw > > that no one has access to (except internal people). > > Agreed, mainline support for an SoC so early in it's lifetime was new > for all of us. Lesson learned. So the suggestion would be to not document the DT bindings at all, until we reach a "stable" hardware that is distributed externally? Note that the Z1 stepping is not completely internal: a small selection of early customers have access to it. But it normally never ends up in final products, it's aimed at allowing those early customers to start their development soon. I don't mind adjusting how DT bindings are documented for such early SoCs stepping. But I really believe it's important to have a way to handle this situation nicely: we've been asking for years SoC vendors to start upstreaming their code early. Now that they start to do it, we shouldn't complain and instead adapt to this situation :-) Best regards, Thomas
On Fri, Nov 21, 2014 at 10:51:22PM +0100, Thomas Petazzoni wrote: > Dear Jason Cooper, > > On Fri, 21 Nov 2014 15:18:58 -0500, Jason Cooper wrote: > > > > First of, I would like to mention that best thing to avoid such > > > situation is to be careful when documenting dt entries that represent hw > > > that no one has access to (except internal people). > > > > Agreed, mainline support for an SoC so early in it's lifetime was new > > for all of us. Lesson learned. > > So the suggestion would be to not document the DT bindings at all, > until we reach a "stable" hardware that is distributed externally? No, I think it's better to document. However, we should also document the fact that the binding is for an early version of the SoC and subject to incompatible change once the production version of the SoC is released. ie, an un-stable binding. thx, Jason. > I don't mind adjusting how DT bindings are documented for such early > SoCs stepping. But I really believe it's important to have a way to > handle this situation nicely: we've been asking for years SoC vendors > to start upstreaming their code early. Now that they start to do it, we > shouldn't complain and instead adapt to this situation :-) Does the above proposal meet your expectations? thx, Jason.
Hello, On Fri, 21 Nov 2014 17:05:03 -0500, Jason Cooper wrote: > > So the suggestion would be to not document the DT bindings at all, > > until we reach a "stable" hardware that is distributed externally? > > No, I think it's better to document. However, we should also document > the fact that the binding is for an early version of the SoC and subject > to incompatible change once the production version of the SoC is > released. ie, an un-stable binding. Sounds good. > > I don't mind adjusting how DT bindings are documented for such early > > SoCs stepping. But I really believe it's important to have a way to > > handle this situation nicely: we've been asking for years SoC vendors > > to start upstreaming their code early. Now that they start to do it, we > > shouldn't complain and instead adapt to this situation :-) > > Does the above proposal meet your expectations? Absolutely! Thanks, Thomas
diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt index 4cf0249..4698e0e 100644 --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt @@ -5,17 +5,9 @@ Required properties: - compatible: Should be set to one of the following: marvell,armada370-thermal marvell,armada375-thermal - marvell,armada375-z1-thermal marvell,armada380-thermal marvell,armadaxp-thermal - Note: As the name suggests, "marvell,armada375-z1-thermal" - applies for the SoC Z1 stepping only. On such stepping - some quirks need to be done and the register offset differs - from the one in the A0 stepping. - The operating system may auto-detect the SoC stepping and - update the compatible and register offsets at runtime. - - reg: Device's register space. Two entries are expected, see the examples below. The first one is required for the sensor register; diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c index 9d1420a..9c8e783 100644 --- a/drivers/thermal/armada_thermal.c +++ b/drivers/thermal/armada_thermal.c @@ -35,10 +35,6 @@ #define PMU_TDC0_OTF_CAL_MASK (0x1 << 30) #define PMU_TDC0_START_CAL_MASK (0x1 << 25) -#define A375_Z1_CAL_RESET_LSB 0x8011e214 -#define A375_Z1_CAL_RESET_MSB 0x30a88019 -#define A375_Z1_WORKAROUND_BIT BIT(9) - #define A375_UNIT_CONTROL_SHIFT 27 #define A375_UNIT_CONTROL_MASK 0x7 #define A375_READOUT_INVERT BIT(15) @@ -124,24 +120,12 @@ static void armada375_init_sensor(struct platform_device *pdev, struct armada_thermal_priv *priv) { unsigned long reg; - bool quirk_needed = - !!of_device_is_compatible(pdev->dev.of_node, - "marvell,armada375-z1-thermal"); - - if (quirk_needed) { - /* Ensure these registers have the default (reset) values */ - writel(A375_Z1_CAL_RESET_LSB, priv->control); - writel(A375_Z1_CAL_RESET_MSB, priv->control + 0x4); - } reg = readl(priv->control + 4); reg &= ~(A375_UNIT_CONTROL_MASK << A375_UNIT_CONTROL_SHIFT); reg &= ~A375_READOUT_INVERT; reg &= ~A375_HW_RESETn; - if (quirk_needed) - reg |= A375_Z1_WORKAROUND_BIT; - writel(reg, priv->control + 4); mdelay(20); @@ -260,10 +244,6 @@ static const struct of_device_id armada_thermal_id_table[] = { .data = &armada375_data, }, { - .compatible = "marvell,armada375-z1-thermal", - .data = &armada375_data, - }, - { .compatible = "marvell,armada380-thermal", .data = &armada380_data, },
The Armada 375 Z1 SoC revision is no longer supported. This commit removes the quirk needed for the thermal sensor. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- .../devicetree/bindings/thermal/armada-thermal.txt | 8 -------- drivers/thermal/armada_thermal.c | 20 -------------------- 2 files changed, 28 deletions(-)