v4:
fix typos on v3 commit message.
v3:
1. revert v2 changes.
2. add 'reset-duration-us' property to declare a minimum delay,
which needs to be waited between assert and deassert.
3. add 'mediatek,infra-reset' to compatible.
v2 changes:
https://patchwork.kernel.org/patch/11697371/
1. add 'assert-deassert-together' property to introduce a new reset handler,
which allows device to do serialized assert and deassert operations in a single
step by 'reset' method.
2. add 'update-force' property to introduce force-update method, which forces
the write operation in case the read already happens to return the correct value.
3. add 'generic-reset' to compatible
v1 changes:
https://patchwork.kernel.org/patch/11690523/
https://patchwork.kernel.org/patch/11690527/
Crystal Guo (4):
dt-binding: reset-controller: ti: add reset-duration-us property
dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to
compatible
reset-controller: ti: introduce a new reset handler
arm64: dts: mt8192: add infracfg_rst node
.../bindings/reset/ti-syscon-reset.txt | 6 +++++
arch/arm64/boot/dts/mediatek/mt8192.dtsi | 11 +++++++-
drivers/reset/reset-ti-syscon.c | 26 +++++++++++++++++--
3 files changed, 40 insertions(+), 3 deletions(-)
introduce 'reset' method to allow device do serialized assert and
deassert operations in a single step, which needs a minimum delay
to be waited between assert and deassert.
Signed-off-by: Crystal Guo <[email protected]>
---
Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
index 86945502ccb5..ab041032339b 100644
--- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
+++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
@@ -59,6 +59,11 @@ Required properties:
Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
common reset controller usage by consumers.
+Optional properties:
+- reset-duration-us: When do serialized assert and deassert operations, minimum delay in microseconds
+is needed to be waited between an assert and a deassert to reset the device. This value can be 0, 0 means
+that such a delay is not needed.
+
Example:
--------
The following example demonstrates a syscon node, the reset controller node
--
2.18.0
The TI syscon reset controller provides a common reset management,
and is suitable for MTK SoCs. Add compatible 'mediatek,infra-reset',
which denotes to use ti reset-controller driver directly.
Signed-off-by: Crystal Guo <[email protected]>
---
Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
index ab041032339b..5a0e9365b51b 100644
--- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
+++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
@@ -25,6 +25,7 @@ Required properties:
"ti,k2l-pscrst"
"ti,k2hk-pscrst"
"ti,syscon-reset"
+ "mediatek,infra-reset", "ti,syscon-reset"
- #reset-cells : Should be 1. Please see the reset consumer node below
for usage details
- ti,reset-bits : Contains the reset control register information
--
2.18.0
add infracfg_rst node which is for MT8192 platform
Signed-off-by: Crystal Guo <[email protected]>
---
arch/arm64/boot/dts/mediatek/mt8192.dtsi | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
index 931e1ca17220..a0cb9904706b 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
@@ -10,6 +10,7 @@
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/pinctrl/mt8192-pinfunc.h>
#include <dt-bindings/power/mt8192-power.h>
+#include <dt-bindings/reset/ti-syscon.h>
/ {
compatible = "mediatek,mt8192";
@@ -219,9 +220,17 @@
};
infracfg: infracfg@10001000 {
- compatible = "mediatek,mt8192-infracfg", "syscon";
+ compatible = "mediatek,mt8192-infracfg", "syscon", "simple-mfd";
reg = <0 0x10001000 0 0x1000>;
#clock-cells = <1>;
+
+ infracfg_rst: reset-controller {
+ compatible = "mediatek,infra-reset", "ti,syscon-reset";
+ #reset-cells = <1>;
+ ti,reset-bits = <
+ 0x140 15 0x144 15 0 0 (ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 0: pcie */
+ >;
+ };
};
pericfg: pericfg@10003000 {
--
2.18.0
Introduce ti_syscon_reset() to integrate assert and deassert together.
If some modules need do serialized assert and deassert operations
to reset itself, reset_control_reset can be called for convenience.
Such as reset-qcom-aoss.c, it integrates assert and deassert together
by 'reset' method. MTK Socs also need this method to perform reset.
Signed-off-by: Crystal Guo <[email protected]>
---
drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
index a2635c21db7f..08289342f9af 100644
--- a/drivers/reset/reset-ti-syscon.c
+++ b/drivers/reset/reset-ti-syscon.c
@@ -15,6 +15,7 @@
* GNU General Public License for more details.
*/
+#include <linux/delay.h>
#include <linux/mfd/syscon.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
struct regmap *regmap;
struct ti_syscon_reset_control *controls;
unsigned int nr_controls;
+ unsigned int reset_duration_us;
};
#define to_ti_syscon_reset_data(rcdev) \
@@ -89,7 +91,7 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
mask = BIT(control->assert_bit);
value = (control->flags & ASSERT_SET) ? mask : 0x0;
- return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
+ return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
}
/**
@@ -120,7 +122,7 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
mask = BIT(control->deassert_bit);
value = (control->flags & DEASSERT_SET) ? mask : 0x0;
- return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
+ return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
}
/**
@@ -158,9 +160,26 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
!(control->flags & STATUS_SET);
}
+static int ti_syscon_reset(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
+ int ret;
+
+ ret = ti_syscon_reset_assert(rcdev, id);
+ if (ret)
+ return ret;
+
+ if (data->reset_duration_us)
+ usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
+
+ return ti_syscon_reset_deassert(rcdev, id);
+}
+
static const struct reset_control_ops ti_syscon_reset_ops = {
.assert = ti_syscon_reset_assert,
.deassert = ti_syscon_reset_deassert,
+ .reset = ti_syscon_reset,
.status = ti_syscon_reset_status,
};
@@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
controls[i].flags = be32_to_cpup(list++);
}
+ of_property_read_u32(pdev->dev.of_node, "reset-duration-us",
+ &data->reset_duration_us);
+
data->rcdev.ops = &ti_syscon_reset_ops;
data->rcdev.owner = THIS_MODULE;
data->rcdev.of_node = np;
--
2.18.0
On Mon, Aug 17, 2020 at 11:03:21AM +0800, Crystal Guo wrote:
> introduce 'reset' method to allow device do serialized assert and
> deassert operations in a single step, which needs a minimum delay
> to be waited between assert and deassert.
Why is Mediatek adding to a TI binding?
>
> Signed-off-by: Crystal Guo <[email protected]>
> ---
> Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> index 86945502ccb5..ab041032339b 100644
> --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> @@ -59,6 +59,11 @@ Required properties:
> Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
> common reset controller usage by consumers.
>
> +Optional properties:
> +- reset-duration-us: When do serialized assert and deassert operations, minimum delay in microseconds
> +is needed to be waited between an assert and a deassert to reset the device. This value can be 0, 0 means
> +that such a delay is not needed.
This goes in the reset controller node or each consumer? For the latter,
it should be a cell in 'resets' if you need this. But really, I think
the reset controller should enforce some minimum time that works for all
consumers. Surely having a minimum time per reset isn't really needed.
Rob
On Mon, Aug 17, 2020 at 11:03:22AM +0800, Crystal Guo wrote:
> The TI syscon reset controller provides a common reset management,
> and is suitable for MTK SoCs. Add compatible 'mediatek,infra-reset',
> which denotes to use ti reset-controller driver directly.
>
> Signed-off-by: Crystal Guo <[email protected]>
> ---
> Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> index ab041032339b..5a0e9365b51b 100644
> --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> @@ -25,6 +25,7 @@ Required properties:
> "ti,k2l-pscrst"
> "ti,k2hk-pscrst"
> "ti,syscon-reset"
> + "mediatek,infra-reset", "ti,syscon-reset"
You need your own binding doc. If you can use the same driver then fine,
but that's a separate issue. There's also reset-simple driver if you
have just array of 32-bit registers with a bit per reset.
Don't repeat 'ti,reset-bits' either.
> - #reset-cells : Should be 1. Please see the reset consumer node below
> for usage details
> - ti,reset-bits : Contains the reset control register information
> --
> 2.18.0
On Wed, 2020-08-26 at 01:42 +0800, Rob Herring wrote:
> On Mon, Aug 17, 2020 at 11:03:21AM +0800, Crystal Guo wrote:
> > introduce 'reset' method to allow device do serialized assert and
> > deassert operations in a single step, which needs a minimum delay
> > to be waited between assert and deassert.
>
> Why is Mediatek adding to a TI binding?
TI reset-controller provides a common reset management,
and is suitable for Mediatek SoCs, thus Mediatek wants to reuse this
driver for reset.
>
> >
> > Signed-off-by: Crystal Guo <[email protected]>
> > ---
> > Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > index 86945502ccb5..ab041032339b 100644
> > --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > @@ -59,6 +59,11 @@ Required properties:
> > Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
> > common reset controller usage by consumers.
> >
> > +Optional properties:
> > +- reset-duration-us: When do serialized assert and deassert operations, minimum delay in microseconds
> > +is needed to be waited between an assert and a deassert to reset the device. This value can be 0, 0 means
> > +that such a delay is not needed.
>
> This goes in the reset controller node or each consumer? For the latter,
> it should be a cell in 'resets' if you need this. But really, I think
> the reset controller should enforce some minimum time that works for all
> consumers. Surely having a minimum time per reset isn't really needed.
>
> Rob
'reset-duration-us' will be in the reset controller node, and it's
optional. If minimum delay is needed to be waited between an assert and
a deassert to reset the device, this property will be set.Otherwise no
need to set this property.
Best Regards
Crystal
On Wed, 2020-08-26 at 03:02 +0800, Rob Herring wrote:
> On Mon, Aug 17, 2020 at 11:03:22AM +0800, Crystal Guo wrote:
> > The TI syscon reset controller provides a common reset management,
> > and is suitable for MTK SoCs. Add compatible 'mediatek,infra-reset',
> > which denotes to use ti reset-controller driver directly.
> >
> > Signed-off-by: Crystal Guo <[email protected]>
> > ---
> > Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > index ab041032339b..5a0e9365b51b 100644
> > --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > @@ -25,6 +25,7 @@ Required properties:
> > "ti,k2l-pscrst"
> > "ti,k2hk-pscrst"
> > "ti,syscon-reset"
> > + "mediatek,infra-reset", "ti,syscon-reset"
>
> You need your own binding doc. If you can use the same driver then fine,
> but that's a separate issue. There's also reset-simple driver if you
> have just array of 32-bit registers with a bit per reset.
>
> Don't repeat 'ti,reset-bits' either.
Do you mean I should add a Mediatek reset binding doc, although Mediatek
reuse the TI reset controller directly?
Best Regards
Crystal
>
> > - #reset-cells : Should be 1. Please see the reset consumer node below
> > for usage details
> > - ti,reset-bits : Contains the reset control register information
> > --
> > 2.18.0
Hi Rob, Philipp, Matthias and all
Gentle ping for this patch set.
Thanks
Crystal
>
> -----Original Message-----
> From: Crystal Guo [mailto:[email protected]]
> Sent: Monday, August 17, 2020 11:03 AM
> To: [email protected]; [email protected]; [email protected]
> Cc: srv_heupstream; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Seiya Wang (王迺君); Stanley Chu (朱原陞); Yingjoe Chen (陳英洲); Fan Chen (陳凡); Yong Liang (梁勇)
> Subject: [v4,0/4] introduce TI reset controller for MT8192 SoC
>
> v4:
> fix typos on v3 commit message.
>
> v3:
> 1. revert v2 changes.
> 2. add 'reset-duration-us' property to declare a minimum delay, which needs to be waited between assert and deassert.
> 3. add 'mediatek,infra-reset' to compatible.
>
>
> v2 changes:
> https://patchwork.kernel.org/patch/11697371/
> 1. add 'assert-deassert-together' property to introduce a new reset handler, which allows device to do serialized assert and deassert operations in a single step by 'reset' method.
> 2. add 'update-force' property to introduce force-update method, which forces the write operation in case the read already happens to return the correct value.
> 3. add 'generic-reset' to compatible
>
> v1 changes:
> https://patchwork.kernel.org/patch/11690523/
> https://patchwork.kernel.org/patch/11690527/
>
> Crystal Guo (4):
> dt-binding: reset-controller: ti: add reset-duration-us property
> dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to
> compatible
> reset-controller: ti: introduce a new reset handler
> arm64: dts: mt8192: add infracfg_rst node
>
> .../bindings/reset/ti-syscon-reset.txt | 6 +++++
> arch/arm64/boot/dts/mediatek/mt8192.dtsi | 11 +++++++-
> drivers/reset/reset-ti-syscon.c | 26 +++++++++++++++++--
> 3 files changed, 40 insertions(+), 3 deletions(-)
>
>
> *********************MEDIATEK Confidential/Internal Use*********************
Hi Rob,
On 8/26/20 6:09 AM, Crystal Guo wrote:
> On Wed, 2020-08-26 at 03:02 +0800, Rob Herring wrote:
>> On Mon, Aug 17, 2020 at 11:03:22AM +0800, Crystal Guo wrote:
>>> The TI syscon reset controller provides a common reset management,
>>> and is suitable for MTK SoCs. Add compatible 'mediatek,infra-reset',
>>> which denotes to use ti reset-controller driver directly.
>>>
>>> Signed-off-by: Crystal Guo <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
>>> index ab041032339b..5a0e9365b51b 100644
>>> --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
>>> +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
>>> @@ -25,6 +25,7 @@ Required properties:
>>> "ti,k2l-pscrst"
>>> "ti,k2hk-pscrst"
>>> "ti,syscon-reset"
>>> + "mediatek,infra-reset", "ti,syscon-reset"
>>
>> You need your own binding doc. If you can use the same driver then fine,
>> but that's a separate issue. There's also reset-simple driver if you
>> have just array of 32-bit registers with a bit per reset.
>>
>> Don't repeat 'ti,reset-bits' either.
>
> Do you mean I should add a Mediatek reset binding doc, although Mediatek
> reuse the TI reset controller directly?
Hmm, how do you envision not repeating the same bits in a separate binding?
Does it help if I convert this to YAML first without a ti, prefix in the file name?
The usage philosophy definitely was to use a <soc-compatible> followed by the
<generic-compatible>. This is how all of our reset nodes were added as well.
Looks like Andrew may have misinterpreted your comment [1] during the original
binding and changed "syscon-reset" to "ti,syscon-reset" in the final version [2].
regards
Suman
[1] https://lore.kernel.org/patchwork/comment/876688/
[2] https://lore.kernel.org/patchwork/patch/693172/
>
> Best Regards
> Crystal
>>
>>> - #reset-cells : Should be 1. Please see the reset consumer node below
>>> for usage details
>>> - ti,reset-bits : Contains the reset control register information
>>> --
>>> 2.18.0
>
Hi Crystal,
On 8/16/20 10:03 PM, Crystal Guo wrote:
> add infracfg_rst node which is for MT8192 platform
>
> Signed-off-by: Crystal Guo <[email protected]>
I understand you are posting these together for complete reference, but driver
subsystem maintainers typically don't pick dts patches. In anycase, can you
clarify if your registers are self-clearing registers?
regards
Suman
> ---
> arch/arm64/boot/dts/mediatek/mt8192.dtsi | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> index 931e1ca17220..a0cb9904706b 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> @@ -10,6 +10,7 @@
> #include <dt-bindings/interrupt-controller/irq.h>
> #include <dt-bindings/pinctrl/mt8192-pinfunc.h>
> #include <dt-bindings/power/mt8192-power.h>
> +#include <dt-bindings/reset/ti-syscon.h>
>
> / {
> compatible = "mediatek,mt8192";
> @@ -219,9 +220,17 @@
> };
>
> infracfg: infracfg@10001000 {
> - compatible = "mediatek,mt8192-infracfg", "syscon";
> + compatible = "mediatek,mt8192-infracfg", "syscon", "simple-mfd";
> reg = <0 0x10001000 0 0x1000>;
> #clock-cells = <1>;
> +
> + infracfg_rst: reset-controller {
> + compatible = "mediatek,infra-reset", "ti,syscon-reset";
> + #reset-cells = <1>;
> + ti,reset-bits = <
> + 0x140 15 0x144 15 0 0 (ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 0: pcie */
> + >;
> + };
> };
>
> pericfg: pericfg@10003000 {
>
Hi Crystal,
On 8/16/20 10:03 PM, Crystal Guo wrote:
> Introduce ti_syscon_reset() to integrate assert and deassert together.
> If some modules need do serialized assert and deassert operations
> to reset itself, reset_control_reset can be called for convenience.
There are multiple changes in this same patch. I think you should split this
functionality away from the change for the regmap_update_bits() to
regmap_write_bits(), similar to what you have done in your v2 Patch 4.
>
> Such as reset-qcom-aoss.c, it integrates assert and deassert together
> by 'reset' method. MTK Socs also need this method to perform reset.
>
> Signed-off-by: Crystal Guo <[email protected]>
> ---
> drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index a2635c21db7f..08289342f9af 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -15,6 +15,7 @@
> * GNU General Public License for more details.
> */
>
> +#include <linux/delay.h>
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
> #include <linux/of.h>
> @@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
> struct regmap *regmap;
> struct ti_syscon_reset_control *controls;
> unsigned int nr_controls;
> + unsigned int reset_duration_us;
> };
>
> #define to_ti_syscon_reset_data(rcdev) \
> @@ -89,7 +91,7 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
> mask = BIT(control->assert_bit);
> value = (control->flags & ASSERT_SET) ? mask : 0x0;
>
> - return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> + return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> }
>
> /**
> @@ -120,7 +122,7 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
> mask = BIT(control->deassert_bit);
> value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>
> - return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> + return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> }
>
> /**
> @@ -158,9 +160,26 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
> !(control->flags & STATUS_SET);
> }
>
> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> + int ret;
> +
> + ret = ti_syscon_reset_assert(rcdev, id);
> + if (ret)
> + return ret;
> +
> + if (data->reset_duration_us)
> + usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
> +
> + return ti_syscon_reset_deassert(rcdev, id);
I echo Philipp's comments [1] from your original v1 series about this. We don't
need a property to distinguish this, but you could add a flag using match data
and Mediatek compatible, and use that within this function, or optionally set
this ops based on compatible (whatever is preferred by Philipp).
regards
Suman
[1] https://patchwork.kernel.org/comment/23519193/
> +}
> +
> static const struct reset_control_ops ti_syscon_reset_ops = {
> .assert = ti_syscon_reset_assert,
> .deassert = ti_syscon_reset_deassert,
> + .reset = ti_syscon_reset,
> .status = ti_syscon_reset_status,
> };
>
> @@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
> controls[i].flags = be32_to_cpup(list++);
> }
>
> + of_property_read_u32(pdev->dev.of_node, "reset-duration-us",
> + &data->reset_duration_us);
> +
> data->rcdev.ops = &ti_syscon_reset_ops;
> data->rcdev.owner = THIS_MODULE;
> data->rcdev.of_node = np;
>
On Thu, 2020-09-03 at 07:29 +0800, Suman Anna wrote:
> Hi Crystal,
>
> On 8/16/20 10:03 PM, Crystal Guo wrote:
> > add infracfg_rst node which is for MT8192 platform
> >
> > Signed-off-by: Crystal Guo <[email protected]>
>
> I understand you are posting these together for complete reference, but driver
> subsystem maintainers typically don't pick dts patches. In anycase, can you
> clarify if your registers are self-clearing registers?
>
> regards
> Suman
>
Hi Suman,
Thanks for your reply.
Our reset registers are not self-clearing, it needs to set the clear bit
to 1 to clear the related bit.
And should I separate this dts patch from the patch sets?
regards
Crystal
> > ---
> > arch/arm64/boot/dts/mediatek/mt8192.dtsi | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > index 931e1ca17220..a0cb9904706b 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > @@ -10,6 +10,7 @@
> > #include <dt-bindings/interrupt-controller/irq.h>
> > #include <dt-bindings/pinctrl/mt8192-pinfunc.h>
> > #include <dt-bindings/power/mt8192-power.h>
> > +#include <dt-bindings/reset/ti-syscon.h>
> >
> > / {
> > compatible = "mediatek,mt8192";
> > @@ -219,9 +220,17 @@
> > };
> >
> > infracfg: infracfg@10001000 {
> > - compatible = "mediatek,mt8192-infracfg", "syscon";
> > + compatible = "mediatek,mt8192-infracfg", "syscon", "simple-mfd";
> > reg = <0 0x10001000 0 0x1000>;
> > #clock-cells = <1>;
> > +
> > + infracfg_rst: reset-controller {
> > + compatible = "mediatek,infra-reset", "ti,syscon-reset";
> > + #reset-cells = <1>;
> > + ti,reset-bits = <
> > + 0x140 15 0x144 15 0 0 (ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 0: pcie */
> > + >;
> > + };
> > };
> >
> > pericfg: pericfg@10003000 {
> >
>
On Wed, Sep 2, 2020 at 5:26 PM Suman Anna <[email protected]> wrote:
>
> Hi Rob,
>
> On 8/26/20 6:09 AM, Crystal Guo wrote:
> > On Wed, 2020-08-26 at 03:02 +0800, Rob Herring wrote:
> >> On Mon, Aug 17, 2020 at 11:03:22AM +0800, Crystal Guo wrote:
> >>> The TI syscon reset controller provides a common reset management,
> >>> and is suitable for MTK SoCs. Add compatible 'mediatek,infra-reset',
> >>> which denotes to use ti reset-controller driver directly.
> >>>
> >>> Signed-off-by: Crystal Guo <[email protected]>
> >>> ---
> >>> Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> >>> index ab041032339b..5a0e9365b51b 100644
> >>> --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> >>> +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> >>> @@ -25,6 +25,7 @@ Required properties:
> >>> "ti,k2l-pscrst"
> >>> "ti,k2hk-pscrst"
> >>> "ti,syscon-reset"
> >>> + "mediatek,infra-reset", "ti,syscon-reset"
> >>
> >> You need your own binding doc. If you can use the same driver then fine,
> >> but that's a separate issue. There's also reset-simple driver if you
> >> have just array of 32-bit registers with a bit per reset.
> >>
> >> Don't repeat 'ti,reset-bits' either.
> >
> > Do you mean I should add a Mediatek reset binding doc, although Mediatek
> > reuse the TI reset controller directly?
>
> Hmm, how do you envision not repeating the same bits in a separate binding?
I mean 'ti,reset-bits' isn't really something that should have been in
DT in the first place, but rather implied by the compatible string.
> Does it help if I convert this to YAML first without a ti, prefix in the file name?
No, I don't think this should be a shared binding. The driver may be
able to be shared, but that's independent from the binding.
Rob
On 9/8/20 8:26 AM, Crystal Guo wrote:
> On Thu, 2020-09-03 at 07:29 +0800, Suman Anna wrote:
>> Hi Crystal,
>>
>> On 8/16/20 10:03 PM, Crystal Guo wrote:
>>> add infracfg_rst node which is for MT8192 platform
>>>
>>> Signed-off-by: Crystal Guo <[email protected]>
>>
>> I understand you are posting these together for complete reference, but driver
>> subsystem maintainers typically don't pick dts patches. In anycase, can you
>> clarify if your registers are self-clearing registers?
>>
>> regards
>> Suman
>>
> Hi Suman,
>
> Thanks for your reply.
> Our reset registers are not self-clearing, it needs to set the clear bit
> to 1 to clear the related bit.
> And should I separate this dts patch from the patch sets?
Typically yes, but will leave it upto Philipp and Mediatek DT maintainers.
regards
Suman
>
> regards
> Crystal
>>> ---
>>> arch/arm64/boot/dts/mediatek/mt8192.dtsi | 11 ++++++++++-
>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> index 931e1ca17220..a0cb9904706b 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> @@ -10,6 +10,7 @@
>>> #include <dt-bindings/interrupt-controller/irq.h>
>>> #include <dt-bindings/pinctrl/mt8192-pinfunc.h>
>>> #include <dt-bindings/power/mt8192-power.h>
>>> +#include <dt-bindings/reset/ti-syscon.h>
>>>
>>> / {
>>> compatible = "mediatek,mt8192";
>>> @@ -219,9 +220,17 @@
>>> };
>>>
>>> infracfg: infracfg@10001000 {
>>> - compatible = "mediatek,mt8192-infracfg", "syscon";
>>> + compatible = "mediatek,mt8192-infracfg", "syscon", "simple-mfd";
>>> reg = <0 0x10001000 0 0x1000>;
>>> #clock-cells = <1>;
>>> +
>>> + infracfg_rst: reset-controller {
>>> + compatible = "mediatek,infra-reset", "ti,syscon-reset";
>>> + #reset-cells = <1>;
>>> + ti,reset-bits = <
>>> + 0x140 15 0x144 15 0 0 (ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 0: pcie */
>>> + >;
>>> + };
>>> };
>>>
>>> pericfg: pericfg@10003000 {
>>>
>>
>
On Thu, 2020-09-03 at 07:40 +0800, Suman Anna wrote:
> Hi Crystal,
>
> On 8/16/20 10:03 PM, Crystal Guo wrote:
> > Introduce ti_syscon_reset() to integrate assert and deassert together.
> > If some modules need do serialized assert and deassert operations
> > to reset itself, reset_control_reset can be called for convenience.
>
> There are multiple changes in this same patch. I think you should split this
> functionality away from the change for the regmap_update_bits() to
> regmap_write_bits(), similar to what you have done in your v2 Patch 4.
>
Thanks for your suggestion.
I will split this patch in the next version.
> >
> > Such as reset-qcom-aoss.c, it integrates assert and deassert together
> > by 'reset' method. MTK Socs also need this method to perform reset.
> >
> > Signed-off-by: Crystal Guo <[email protected]>
> > ---
> > drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> > index a2635c21db7f..08289342f9af 100644
> > --- a/drivers/reset/reset-ti-syscon.c
> > +++ b/drivers/reset/reset-ti-syscon.c
> > @@ -15,6 +15,7 @@
> > * GNU General Public License for more details.
> > */
> >
> > +#include <linux/delay.h>
> > #include <linux/mfd/syscon.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
> > struct regmap *regmap;
> > struct ti_syscon_reset_control *controls;
> > unsigned int nr_controls;
> > + unsigned int reset_duration_us;
> > };
> >
> > #define to_ti_syscon_reset_data(rcdev) \
> > @@ -89,7 +91,7 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
> > mask = BIT(control->assert_bit);
> > value = (control->flags & ASSERT_SET) ? mask : 0x0;
> >
> > - return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> > + return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> > }
> >
> > /**
> > @@ -120,7 +122,7 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
> > mask = BIT(control->deassert_bit);
> > value = (control->flags & DEASSERT_SET) ? mask : 0x0;
> >
> > - return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> > + return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> > }
> >
> > /**
> > @@ -158,9 +160,26 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
> > !(control->flags & STATUS_SET);
> > }
> >
> > +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> > + int ret;
> > +
> > + ret = ti_syscon_reset_assert(rcdev, id);
> > + if (ret)
> > + return ret;
> > +
> > + if (data->reset_duration_us)
> > + usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
> > +
> > + return ti_syscon_reset_deassert(rcdev, id);
>
> I echo Philipp's comments [1] from your original v1 series about this. We don't
> need a property to distinguish this, but you could add a flag using match data
> and Mediatek compatible, and use that within this function, or optionally set
> this ops based on compatible (whatever is preferred by Philipp).
>
> regards
> Suman
>
> [1] https://patchwork.kernel.org/comment/23519193/
>
Hi Suman, Philipp
Which method would you recommend more?
1. like v2 patch, but assign the flag "data->assert_deassert_together"
directly (maybe rename "assert_deassert_together" to
"reset_op_available")
2. use Mediatek compatible to decide the reset handler available or not.
Thanks
Crystal
> > +}
> > +
> > static const struct reset_control_ops ti_syscon_reset_ops = {
> > .assert = ti_syscon_reset_assert,
> > .deassert = ti_syscon_reset_deassert,
> > + .reset = ti_syscon_reset,
> > .status = ti_syscon_reset_status,
> > };
> >
> > @@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
> > controls[i].flags = be32_to_cpup(list++);
> > }
> >
> > + of_property_read_u32(pdev->dev.of_node, "reset-duration-us",
> > + &data->reset_duration_us);
> > +
> > data->rcdev.ops = &ti_syscon_reset_ops;
> > data->rcdev.owner = THIS_MODULE;
> > data->rcdev.of_node = np;
> >
>
On 9/8/20 1:49 PM, Rob Herring wrote:
> On Wed, Sep 2, 2020 at 5:26 PM Suman Anna <[email protected]> wrote:
>>
>> Hi Rob,
>>
>> On 8/26/20 6:09 AM, Crystal Guo wrote:
>>> On Wed, 2020-08-26 at 03:02 +0800, Rob Herring wrote:
>>>> On Mon, Aug 17, 2020 at 11:03:22AM +0800, Crystal Guo wrote:
>>>>> The TI syscon reset controller provides a common reset management,
>>>>> and is suitable for MTK SoCs. Add compatible 'mediatek,infra-reset',
>>>>> which denotes to use ti reset-controller driver directly.
>>>>>
>>>>> Signed-off-by: Crystal Guo <[email protected]>
>>>>> ---
>>>>> Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
>>>>> index ab041032339b..5a0e9365b51b 100644
>>>>> --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
>>>>> +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
>>>>> @@ -25,6 +25,7 @@ Required properties:
>>>>> "ti,k2l-pscrst"
>>>>> "ti,k2hk-pscrst"
>>>>> "ti,syscon-reset"
>>>>> + "mediatek,infra-reset", "ti,syscon-reset"
>>>>
>>>> You need your own binding doc. If you can use the same driver then fine,
>>>> but that's a separate issue. There's also reset-simple driver if you
>>>> have just array of 32-bit registers with a bit per reset.
>>>>
>>>> Don't repeat 'ti,reset-bits' either.
>>>
>>> Do you mean I should add a Mediatek reset binding doc, although Mediatek
>>> reuse the TI reset controller directly?
>>
>> Hmm, how do you envision not repeating the same bits in a separate binding?
>
> I mean 'ti,reset-bits' isn't really something that should have been in
> DT in the first place, but rather implied by the compatible string.
Ok, should I be deprecating this and move this data to driver then?
I am assuming that is how you are envisioning the new Mediatek binding to be
atleast.
regards
Suman
>
>> Does it help if I convert this to YAML first without a ti, prefix in the file name?
>
> No, I don't think this should be a shared binding. The driver may be
> able to be shared, but that's independent from the binding.
>
> Rob
>
On 9/8/20 9:57 PM, Crystal Guo wrote:
> On Thu, 2020-09-03 at 07:40 +0800, Suman Anna wrote:
>> Hi Crystal,
>>
>> On 8/16/20 10:03 PM, Crystal Guo wrote:
>>> Introduce ti_syscon_reset() to integrate assert and deassert together.
>>> If some modules need do serialized assert and deassert operations
>>> to reset itself, reset_control_reset can be called for convenience.
>>
>> There are multiple changes in this same patch. I think you should split this
>> functionality away from the change for the regmap_update_bits() to
>> regmap_write_bits(), similar to what you have done in your v2 Patch 4.
>>
>
> Thanks for your suggestion.
> I will split this patch in the next version.
>
>>>
>>> Such as reset-qcom-aoss.c, it integrates assert and deassert together
>>> by 'reset' method. MTK Socs also need this method to perform reset.
>>>
>>> Signed-off-by: Crystal Guo <[email protected]>
>>> ---
>>> drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
>>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
>>> index a2635c21db7f..08289342f9af 100644
>>> --- a/drivers/reset/reset-ti-syscon.c
>>> +++ b/drivers/reset/reset-ti-syscon.c
>>> @@ -15,6 +15,7 @@
>>> * GNU General Public License for more details.
>>> */
>>>
>>> +#include <linux/delay.h>
>>> #include <linux/mfd/syscon.h>
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> @@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
>>> struct regmap *regmap;
>>> struct ti_syscon_reset_control *controls;
>>> unsigned int nr_controls;
>>> + unsigned int reset_duration_us;
>>> };
>>>
>>> #define to_ti_syscon_reset_data(rcdev) \
>>> @@ -89,7 +91,7 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
>>> mask = BIT(control->assert_bit);
>>> value = (control->flags & ASSERT_SET) ? mask : 0x0;
>>>
>>> - return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
>>> + return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
>>> }
>>>
>>> /**
>>> @@ -120,7 +122,7 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
>>> mask = BIT(control->deassert_bit);
>>> value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>>>
>>> - return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
>>> + return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
>>> }
>>>
>>> /**
>>> @@ -158,9 +160,26 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
>>> !(control->flags & STATUS_SET);
>>> }
>>>
>>> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
>>> + unsigned long id)
>>> +{
>>> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
>>> + int ret;
>>> +
>>> + ret = ti_syscon_reset_assert(rcdev, id);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (data->reset_duration_us)
>>> + usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
>>> +
>>> + return ti_syscon_reset_deassert(rcdev, id);
>>
>> I echo Philipp's comments [1] from your original v1 series about this. We don't
>> need a property to distinguish this, but you could add a flag using match data
>> and Mediatek compatible, and use that within this function, or optionally set
>> this ops based on compatible (whatever is preferred by Philipp).
>>
>> regards
>> Suman
>>
>> [1] https://patchwork.kernel.org/comment/23519193/
>>
> Hi Suman, Philipp
>
> Which method would you recommend more?
> 1. like v2 patch, but assign the flag "data->assert_deassert_together"
> directly (maybe rename "assert_deassert_together" to
> "reset_op_available")
>
> 2. use Mediatek compatible to decide the reset handler available or not.
I would go with this option. Anyway, I think you might have to add the reset SoC
data as well, based on Rob's comment on the binding.
regards
Suman
>
> Thanks
> Crystal
>
>>> +}
>>> +
>>> static const struct reset_control_ops ti_syscon_reset_ops = {
>>> .assert = ti_syscon_reset_assert,
>>> .deassert = ti_syscon_reset_deassert,
>>> + .reset = ti_syscon_reset,
>>> .status = ti_syscon_reset_status,
>>> };
>>>
>>> @@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>>> controls[i].flags = be32_to_cpup(list++);
>>> }
>>>
>>> + of_property_read_u32(pdev->dev.of_node, "reset-duration-us",
>>> + &data->reset_duration_us);
>>> +
>>> data->rcdev.ops = &ti_syscon_reset_ops;
>>> data->rcdev.owner = THIS_MODULE;
>>> data->rcdev.of_node = np;
>>>
>>
>
On Wed, Sep 9, 2020 at 9:10 AM Suman Anna <[email protected]> wrote:
>
> On 9/8/20 1:49 PM, Rob Herring wrote:
> > On Wed, Sep 2, 2020 at 5:26 PM Suman Anna <[email protected]> wrote:
> >>
> >> Hi Rob,
> >>
> >> On 8/26/20 6:09 AM, Crystal Guo wrote:
> >>> On Wed, 2020-08-26 at 03:02 +0800, Rob Herring wrote:
> >>>> On Mon, Aug 17, 2020 at 11:03:22AM +0800, Crystal Guo wrote:
> >>>>> The TI syscon reset controller provides a common reset management,
> >>>>> and is suitable for MTK SoCs. Add compatible 'mediatek,infra-reset',
> >>>>> which denotes to use ti reset-controller driver directly.
> >>>>>
> >>>>> Signed-off-by: Crystal Guo <[email protected]>
> >>>>> ---
> >>>>> Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
> >>>>> 1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> >>>>> index ab041032339b..5a0e9365b51b 100644
> >>>>> --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> >>>>> +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> >>>>> @@ -25,6 +25,7 @@ Required properties:
> >>>>> "ti,k2l-pscrst"
> >>>>> "ti,k2hk-pscrst"
> >>>>> "ti,syscon-reset"
> >>>>> + "mediatek,infra-reset", "ti,syscon-reset"
> >>>>
> >>>> You need your own binding doc. If you can use the same driver then fine,
> >>>> but that's a separate issue. There's also reset-simple driver if you
> >>>> have just array of 32-bit registers with a bit per reset.
> >>>>
> >>>> Don't repeat 'ti,reset-bits' either.
> >>>
> >>> Do you mean I should add a Mediatek reset binding doc, although Mediatek
> >>> reuse the TI reset controller directly?
> >>
> >> Hmm, how do you envision not repeating the same bits in a separate binding?
> >
> > I mean 'ti,reset-bits' isn't really something that should have been in
> > DT in the first place, but rather implied by the compatible string.
>
> Ok, should I be deprecating this and move this data to driver then?
No, I'm just asking to not have new users.
> I am assuming that is how you are envisioning the new Mediatek binding to be
> atleast.
Right.
Rob
On Wed, 2020-09-09 at 23:39 +0800, Suman Anna wrote:
> On 9/8/20 9:57 PM, Crystal Guo wrote:
> > On Thu, 2020-09-03 at 07:40 +0800, Suman Anna wrote:
> >> Hi Crystal,
> >>
> >> On 8/16/20 10:03 PM, Crystal Guo wrote:
> >>> Introduce ti_syscon_reset() to integrate assert and deassert together.
> >>> If some modules need do serialized assert and deassert operations
> >>> to reset itself, reset_control_reset can be called for convenience.
> >>
> >> There are multiple changes in this same patch. I think you should split this
> >> functionality away from the change for the regmap_update_bits() to
> >> regmap_write_bits(), similar to what you have done in your v2 Patch 4.
> >>
> >
> > Thanks for your suggestion.
> > I will split this patch in the next version.
> >
> >>>
> >>> Such as reset-qcom-aoss.c, it integrates assert and deassert together
> >>> by 'reset' method. MTK Socs also need this method to perform reset.
> >>>
> >>> Signed-off-by: Crystal Guo <[email protected]>
> >>> ---
> >>> drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
> >>> 1 file changed, 24 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> >>> index a2635c21db7f..08289342f9af 100644
> >>> --- a/drivers/reset/reset-ti-syscon.c
> >>> +++ b/drivers/reset/reset-ti-syscon.c
> >>> @@ -15,6 +15,7 @@
> >>> * GNU General Public License for more details.
> >>> */
> >>>
> >>> +#include <linux/delay.h>
> >>> #include <linux/mfd/syscon.h>
> >>> #include <linux/module.h>
> >>> #include <linux/of.h>
> >>> @@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
> >>> struct regmap *regmap;
> >>> struct ti_syscon_reset_control *controls;
> >>> unsigned int nr_controls;
> >>> + unsigned int reset_duration_us;
> >>> };
> >>>
> >>> #define to_ti_syscon_reset_data(rcdev) \
> >>> @@ -89,7 +91,7 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
> >>> mask = BIT(control->assert_bit);
> >>> value = (control->flags & ASSERT_SET) ? mask : 0x0;
> >>>
> >>> - return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> >>> + return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> >>> }
> >>>
> >>> /**
> >>> @@ -120,7 +122,7 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
> >>> mask = BIT(control->deassert_bit);
> >>> value = (control->flags & DEASSERT_SET) ? mask : 0x0;
> >>>
> >>> - return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> >>> + return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> >>> }
> >>>
> >>> /**
> >>> @@ -158,9 +160,26 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
> >>> !(control->flags & STATUS_SET);
> >>> }
> >>>
> >>> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> >>> + unsigned long id)
> >>> +{
> >>> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> >>> + int ret;
> >>> +
> >>> + ret = ti_syscon_reset_assert(rcdev, id);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + if (data->reset_duration_us)
> >>> + usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
> >>> +
> >>> + return ti_syscon_reset_deassert(rcdev, id);
> >>
> >> I echo Philipp's comments [1] from your original v1 series about this. We don't
> >> need a property to distinguish this, but you could add a flag using match data
> >> and Mediatek compatible, and use that within this function, or optionally set
> >> this ops based on compatible (whatever is preferred by Philipp).
> >>
> >> regards
> >> Suman
> >>
> >> [1] https://patchwork.kernel.org/comment/23519193/
> >>
> > Hi Suman, Philipp
> >
> > Which method would you recommend more?
> > 1. like v2 patch, but assign the flag "data->assert_deassert_together"
> > directly (maybe rename "assert_deassert_together" to
> > "reset_op_available")
> >
> > 2. use Mediatek compatible to decide the reset handler available or not.
>
> I would go with this option. Anyway, I think you might have to add the reset SoC
> data as well, based on Rob's comment on the binding.
>
> regards
> Suman
Thanks for your suggestions
I will add the following changes in the next version,
please correct me if there is any misunderstanding.
1). revert ti-syscon-reset.txt add a new mediatek reset binding doc.
2). split the patch [v4,3/4] with the change for the
regmap_update_bits() to regmap_write_bits() and the change to integrate
assert and deassert together.
3). add the reset SoC data, which contains the flag "reset_op_available"
to decide the reset handler available or not.
4). separate the dts patch from this patch sets
> >
> > Thanks
> > Crystal
> >
> >>> +}
> >>> +
> >>> static const struct reset_control_ops ti_syscon_reset_ops = {
> >>> .assert = ti_syscon_reset_assert,
> >>> .deassert = ti_syscon_reset_deassert,
> >>> + .reset = ti_syscon_reset,
> >>> .status = ti_syscon_reset_status,
> >>> };
> >>>
> >>> @@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
> >>> controls[i].flags = be32_to_cpup(list++);
> >>> }
> >>>
> >>> + of_property_read_u32(pdev->dev.of_node, "reset-duration-us",
> >>> + &data->reset_duration_us);
> >>> +
> >>> data->rcdev.ops = &ti_syscon_reset_ops;
> >>> data->rcdev.owner = THIS_MODULE;
> >>> data->rcdev.of_node = np;
> >>>
> >>
> >
>
On 9/10/20 9:42 PM, Crystal Guo wrote:
> On Wed, 2020-09-09 at 23:39 +0800, Suman Anna wrote:
>> On 9/8/20 9:57 PM, Crystal Guo wrote:
>>> On Thu, 2020-09-03 at 07:40 +0800, Suman Anna wrote:
>>>> Hi Crystal,
>>>>
>>>> On 8/16/20 10:03 PM, Crystal Guo wrote:
>>>>> Introduce ti_syscon_reset() to integrate assert and deassert together.
>>>>> If some modules need do serialized assert and deassert operations
>>>>> to reset itself, reset_control_reset can be called for convenience.
>>>>
>>>> There are multiple changes in this same patch. I think you should split this
>>>> functionality away from the change for the regmap_update_bits() to
>>>> regmap_write_bits(), similar to what you have done in your v2 Patch 4.
>>>>
>>>
>>> Thanks for your suggestion.
>>> I will split this patch in the next version.
>>>
>>>>>
>>>>> Such as reset-qcom-aoss.c, it integrates assert and deassert together
>>>>> by 'reset' method. MTK Socs also need this method to perform reset.
>>>>>
>>>>> Signed-off-by: Crystal Guo <[email protected]>
>>>>> ---
>>>>> drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
>>>>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
>>>>> index a2635c21db7f..08289342f9af 100644
>>>>> --- a/drivers/reset/reset-ti-syscon.c
>>>>> +++ b/drivers/reset/reset-ti-syscon.c
>>>>> @@ -15,6 +15,7 @@
>>>>> * GNU General Public License for more details.
>>>>> */
>>>>>
>>>>> +#include <linux/delay.h>
>>>>> #include <linux/mfd/syscon.h>
>>>>> #include <linux/module.h>
>>>>> #include <linux/of.h>
>>>>> @@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
>>>>> struct regmap *regmap;
>>>>> struct ti_syscon_reset_control *controls;
>>>>> unsigned int nr_controls;
>>>>> + unsigned int reset_duration_us;
>>>>> };
>>>>>
>>>>> #define to_ti_syscon_reset_data(rcdev) \
>>>>> @@ -89,7 +91,7 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
>>>>> mask = BIT(control->assert_bit);
>>>>> value = (control->flags & ASSERT_SET) ? mask : 0x0;
>>>>>
>>>>> - return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
>>>>> + return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
>>>>> }
>>>>>
>>>>> /**
>>>>> @@ -120,7 +122,7 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
>>>>> mask = BIT(control->deassert_bit);
>>>>> value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>>>>>
>>>>> - return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
>>>>> + return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
>>>>> }
>>>>>
>>>>> /**
>>>>> @@ -158,9 +160,26 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
>>>>> !(control->flags & STATUS_SET);
>>>>> }
>>>>>
>>>>> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
>>>>> + unsigned long id)
>>>>> +{
>>>>> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
>>>>> + int ret;
>>>>> +
>>>>> + ret = ti_syscon_reset_assert(rcdev, id);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + if (data->reset_duration_us)
>>>>> + usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
>>>>> +
>>>>> + return ti_syscon_reset_deassert(rcdev, id);
>>>>
>>>> I echo Philipp's comments [1] from your original v1 series about this. We don't
>>>> need a property to distinguish this, but you could add a flag using match data
>>>> and Mediatek compatible, and use that within this function, or optionally set
>>>> this ops based on compatible (whatever is preferred by Philipp).
>>>>
>>>> regards
>>>> Suman
>>>>
>>>> [1] https://patchwork.kernel.org/comment/23519193/
>>>>
>>> Hi Suman, Philipp
>>>
>>> Which method would you recommend more?
>>> 1. like v2 patch, but assign the flag "data->assert_deassert_together"
>>> directly (maybe rename "assert_deassert_together" to
>>> "reset_op_available")
>>>
>>> 2. use Mediatek compatible to decide the reset handler available or not.
>>
>> I would go with this option. Anyway, I think you might have to add the reset SoC
>> data as well, based on Rob's comment on the binding.
>>
>> regards
>> Suman
>
>
> Thanks for your suggestions
> I will add the following changes in the next version,
> please correct me if there is any misunderstanding.
> 1). revert ti-syscon-reset.txt add a new mediatek reset binding doc.
> 2). split the patch [v4,3/4] with the change for the
> regmap_update_bits() to regmap_write_bits() and the change to integrate
> assert and deassert together.
> 3). add the reset SoC data, which contains the flag "reset_op_available"
> to decide the reset handler available or not.
You would also need to add your SoC-specific data equivalent of the current
ti,reset-bits in your infra node. Please see Rob's comments on patch 2. Also,
your new Mediatek binding should be in YAML format.
regards
Suman
> 4). separate the dts patch from this patch sets
>
>>>
>>> Thanks
>>> Crystal
>>>
>>>>> +}
>>>>> +
>>>>> static const struct reset_control_ops ti_syscon_reset_ops = {
>>>>> .assert = ti_syscon_reset_assert,
>>>>> .deassert = ti_syscon_reset_deassert,
>>>>> + .reset = ti_syscon_reset,
>>>>> .status = ti_syscon_reset_status,
>>>>> };
>>>>>
>>>>> @@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>>>>> controls[i].flags = be32_to_cpup(list++);
>>>>> }
>>>>>
>>>>> + of_property_read_u32(pdev->dev.of_node, "reset-duration-us",
>>>>> + &data->reset_duration_us);
>>>>> +
>>>>> data->rcdev.ops = &ti_syscon_reset_ops;
>>>>> data->rcdev.owner = THIS_MODULE;
>>>>> data->rcdev.of_node = np;
>>>>>
>>>>
>>>
>>
>
On Fri, 2020-09-11 at 10:52 +0800, Suman Anna wrote:
> On 9/10/20 9:42 PM, Crystal Guo wrote:
> > On Wed, 2020-09-09 at 23:39 +0800, Suman Anna wrote:
> >> On 9/8/20 9:57 PM, Crystal Guo wrote:
> >>> On Thu, 2020-09-03 at 07:40 +0800, Suman Anna wrote:
> >>>> Hi Crystal,
> >>>>
> >>>> On 8/16/20 10:03 PM, Crystal Guo wrote:
> >>>>> Introduce ti_syscon_reset() to integrate assert and deassert together.
> >>>>> If some modules need do serialized assert and deassert operations
> >>>>> to reset itself, reset_control_reset can be called for convenience.
> >>>>
> >>>> There are multiple changes in this same patch. I think you should split this
> >>>> functionality away from the change for the regmap_update_bits() to
> >>>> regmap_write_bits(), similar to what you have done in your v2 Patch 4.
> >>>>
> >>>
> >>> Thanks for your suggestion.
> >>> I will split this patch in the next version.
> >>>
> >>>>>
> >>>>> Such as reset-qcom-aoss.c, it integrates assert and deassert together
> >>>>> by 'reset' method. MTK Socs also need this method to perform reset.
> >>>>>
> >>>>> Signed-off-by: Crystal Guo <[email protected]>
> >>>>> ---
> >>>>> drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
> >>>>> 1 file changed, 24 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> >>>>> index a2635c21db7f..08289342f9af 100644
> >>>>> --- a/drivers/reset/reset-ti-syscon.c
> >>>>> +++ b/drivers/reset/reset-ti-syscon.c
> >>>>> @@ -15,6 +15,7 @@
> >>>>> * GNU General Public License for more details.
> >>>>> */
> >>>>>
> >>>>> +#include <linux/delay.h>
> >>>>> #include <linux/mfd/syscon.h>
> >>>>> #include <linux/module.h>
> >>>>> #include <linux/of.h>
> >>>>> @@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
> >>>>> struct regmap *regmap;
> >>>>> struct ti_syscon_reset_control *controls;
> >>>>> unsigned int nr_controls;
> >>>>> + unsigned int reset_duration_us;
> >>>>> };
> >>>>>
> >>>>> #define to_ti_syscon_reset_data(rcdev) \
> >>>>> @@ -89,7 +91,7 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
> >>>>> mask = BIT(control->assert_bit);
> >>>>> value = (control->flags & ASSERT_SET) ? mask : 0x0;
> >>>>>
> >>>>> - return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> >>>>> + return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> >>>>> }
> >>>>>
> >>>>> /**
> >>>>> @@ -120,7 +122,7 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
> >>>>> mask = BIT(control->deassert_bit);
> >>>>> value = (control->flags & DEASSERT_SET) ? mask : 0x0;
> >>>>>
> >>>>> - return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> >>>>> + return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> >>>>> }
> >>>>>
> >>>>> /**
> >>>>> @@ -158,9 +160,26 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
> >>>>> !(control->flags & STATUS_SET);
> >>>>> }
> >>>>>
> >>>>> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> >>>>> + unsigned long id)
> >>>>> +{
> >>>>> + struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> >>>>> + int ret;
> >>>>> +
> >>>>> + ret = ti_syscon_reset_assert(rcdev, id);
> >>>>> + if (ret)
> >>>>> + return ret;
> >>>>> +
> >>>>> + if (data->reset_duration_us)
> >>>>> + usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
> >>>>> +
> >>>>> + return ti_syscon_reset_deassert(rcdev, id);
> >>>>
> >>>> I echo Philipp's comments [1] from your original v1 series about this. We don't
> >>>> need a property to distinguish this, but you could add a flag using match data
> >>>> and Mediatek compatible, and use that within this function, or optionally set
> >>>> this ops based on compatible (whatever is preferred by Philipp).
> >>>>
> >>>> regards
> >>>> Suman
> >>>>
> >>>> [1] https://patchwork.kernel.org/comment/23519193/
> >>>>
> >>> Hi Suman, Philipp
> >>>
> >>> Which method would you recommend more?
> >>> 1. like v2 patch, but assign the flag "data->assert_deassert_together"
> >>> directly (maybe rename "assert_deassert_together" to
> >>> "reset_op_available")
> >>>
> >>> 2. use Mediatek compatible to decide the reset handler available or not.
> >>
> >> I would go with this option. Anyway, I think you might have to add the reset SoC
> >> data as well, based on Rob's comment on the binding.
> >>
> >> regards
> >> Suman
> >
> >
> > Thanks for your suggestions
> > I will add the following changes in the next version,
> > please correct me if there is any misunderstanding.
> > 1). revert ti-syscon-reset.txt add a new mediatek reset binding doc.
> > 2). split the patch [v4,3/4] with the change for the
> > regmap_update_bits() to regmap_write_bits() and the change to integrate
> > assert and deassert together.
> > 3). add the reset SoC data, which contains the flag "reset_op_available"
> > to decide the reset handler available or not.
>
> You would also need to add your SoC-specific data equivalent of the current
> ti,reset-bits in your infra node. Please see Rob's comments on patch 2. Also,
> your new Mediatek binding should be in YAML format.
>
> regards
> Suman
>
Should I add the SoC-specific data as follows?
This may also modify the ti original code, is it OK?
+ data->reset_data = of_device_get_match_data(&pdev->dev);
+
+ list = of_get_property(np, data->reset_data->reset_bits, &size);
+static const struct common_reset_data ti_reset_data = {
+ .reset_op_available = false,
+ .reset_bits = "ti, reset-bits",
+};
+
+static const struct common_reset_data mediatek_reset_data = {
+ .reset_op_available = true,
+ .reset_bits = "mediatek, reset-bits",
+};
+
static const struct of_device_id ti_syscon_reset_of_match[] = {
- { .compatible = "ti,syscon-reset", },
+ { .compatible = "ti,syscon-reset", .data = &ti_reset_data},
+ { .compatible = "mediatek,syscon-reset", .data =
&mediatek_reset_data},
{ /* sentinel */ },
};
Thanks
Crystal
> > 4). separate the dts patch from this patch sets
> >
> >>>
> >>> Thanks
> >>> Crystal
> >>>
> >>>>> +}
> >>>>> +
> >>>>> static const struct reset_control_ops ti_syscon_reset_ops = {
> >>>>> .assert = ti_syscon_reset_assert,
> >>>>> .deassert = ti_syscon_reset_deassert,
> >>>>> + .reset = ti_syscon_reset,
> >>>>> .status = ti_syscon_reset_status,
> >>>>> };
> >>>>>
> >>>>> @@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
> >>>>> controls[i].flags = be32_to_cpup(list++);
> >>>>> }
> >>>>>
> >>>>> + of_property_read_u32(pdev->dev.of_node, "reset-duration-us",
> >>>>> + &data->reset_duration_us);
> >>>>> +
> >>>>> data->rcdev.ops = &ti_syscon_reset_ops;
> >>>>> data->rcdev.owner = THIS_MODULE;
> >>>>> data->rcdev.of_node = np;
> >>>>>
> >>>>
> >>>
> >>
> >
>
On 9/11/20 9:26 AM, Philipp Zabel wrote:
> Hi Crystal,
>
> On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote:
> [...]
>> Should I add the SoC-specific data as follows?
>> This may also modify the ti original code, is it OK?
>>
>> + data->reset_data = of_device_get_match_data(&pdev->dev);
>> +
>> + list = of_get_property(np, data->reset_data->reset_bits, &size);
>>
>> +static const struct common_reset_data ti_reset_data = {
>> + .reset_op_available = false,
>> + .reset_bits = "ti, reset-bits",
> ^
> That space doesn't belong there.
>
>> +};
>> +
>> +static const struct common_reset_data mediatek_reset_data = {
>> + .reset_op_available = true,
>> + .reset_bits = "mediatek, reset-bits",
>> +};
>
> I understand Robs comments as meaning "ti,reset-bits" should have been
> called "reset-bits" in the first place, and you shouldn't repeat adding
> the vendor prefix, as that is implied by the compatible. So this should
> probably be just "reset-bits".
Hmm, not sure about that. I think Rob wants the reset data itself to be added in
the driver as is being done on some other SoCs (eg: like in reset-qcom-pdc.c).
regards
Suman
>
> Otherwise this looks like it should work.
>
> regards
> Philipp
>
Hi Crystal,
On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote:
[...]
> Should I add the SoC-specific data as follows?
> This may also modify the ti original code, is it OK?
>
> + data->reset_data = of_device_get_match_data(&pdev->dev);
> +
> + list = of_get_property(np, data->reset_data->reset_bits, &size);
>
> +static const struct common_reset_data ti_reset_data = {
> + .reset_op_available = false,
> + .reset_bits = "ti, reset-bits",
^
That space doesn't belong there.
> +};
> +
> +static const struct common_reset_data mediatek_reset_data = {
> + .reset_op_available = true,
> + .reset_bits = "mediatek, reset-bits",
> +};
I understand Robs comments as meaning "ti,reset-bits" should have been
called "reset-bits" in the first place, and you shouldn't repeat adding
the vendor prefix, as that is implied by the compatible. So this should
probably be just "reset-bits".
Otherwise this looks like it should work.
regards
Philipp
On Fri, 2020-09-11 at 22:44 +0800, Suman Anna wrote:
> On 9/11/20 9:26 AM, Philipp Zabel wrote:
> > Hi Crystal,
> >
> > On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote:
> > [...]
> >> Should I add the SoC-specific data as follows?
> >> This may also modify the ti original code, is it OK?
> >>
> >> + data->reset_data = of_device_get_match_data(&pdev->dev);
> >> +
> >> + list = of_get_property(np, data->reset_data->reset_bits, &size);
> >>
> >> +static const struct common_reset_data ti_reset_data = {
> >> + .reset_op_available = false,
> >> + .reset_bits = "ti, reset-bits",
> > ^
> > That space doesn't belong there.
> >
> >> +};
> >> +
> >> +static const struct common_reset_data mediatek_reset_data = {
> >> + .reset_op_available = true,
> >> + .reset_bits = "mediatek, reset-bits",
> >> +};
> >
> > I understand Robs comments as meaning "ti,reset-bits" should have been
> > called "reset-bits" in the first place, and you shouldn't repeat adding
> > the vendor prefix, as that is implied by the compatible. So this should
> > probably be just "reset-bits".
>
> Hmm, not sure about that. I think Rob wants the reset data itself to be added in
> the driver as is being done on some other SoCs (eg: like in reset-qcom-pdc.c).
>
> regards
> Suman
>
Hi Rob,
Can you help to comment about this point?
Modify "ti,reset-bits" to "reset-bits" or add "mediatek,reset-bits" ?
Many thanks~
Crystal
> >
> > Otherwise this looks like it should work.
> >
> > regards
> > Philipp
> >
>
On Mon, 2020-09-14 at 22:00 +0800, Crystal Guo wrote:
> On Fri, 2020-09-11 at 22:44 +0800, Suman Anna wrote:
> > On 9/11/20 9:26 AM, Philipp Zabel wrote:
> > > Hi Crystal,
> > >
> > > On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote:
> > > [...]
> > >> Should I add the SoC-specific data as follows?
> > >> This may also modify the ti original code, is it OK?
> > >>
> > >> + data->reset_data = of_device_get_match_data(&pdev->dev);
> > >> +
> > >> + list = of_get_property(np, data->reset_data->reset_bits, &size);
> > >>
> > >> +static const struct common_reset_data ti_reset_data = {
> > >> + .reset_op_available = false,
> > >> + .reset_bits = "ti, reset-bits",
> > > ^
> > > That space doesn't belong there.
> > >
> > >> +};
> > >> +
> > >> +static const struct common_reset_data mediatek_reset_data = {
> > >> + .reset_op_available = true,
> > >> + .reset_bits = "mediatek, reset-bits",
> > >> +};
> > >
> > > I understand Robs comments as meaning "ti,reset-bits" should have been
> > > called "reset-bits" in the first place, and you shouldn't repeat adding
> > > the vendor prefix, as that is implied by the compatible. So this should
> > > probably be just "reset-bits".
> >
> > Hmm, not sure about that. I think Rob wants the reset data itself to be added in
> > the driver as is being done on some other SoCs (eg: like in reset-qcom-pdc.c).
> >
> > regards
> > Suman
> >
> Hi Rob,
>
> Can you help to comment about this point?
> Modify "ti,reset-bits" to "reset-bits" or add "mediatek,reset-bits" ?
>
> Many thanks~
> Crystal
>
> > >
> > > Otherwise this looks like it should work.
> > >
> > > regards
> > > Philipp
> > >
> >
Dears,
I have uploaded the changes at
https://patchwork.kernel.org/cover/11805937/
Please help me to review, many thanks~~
regards
Crystal
>