[v4,0/4] introduce TI reset controller for MT8192 SoC

[v4,0/4] introduce TI reset controller for MT8192 SoC

[Crystal Guo]

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(-)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[v4, 1/4] dt-binding: reset-controller: ti: add reset-duration-us property

[Crystal Guo]

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 <crystal.guo@mediatek.com>
---
 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[v4, 2/4] dt-binding: reset-controller: ti: add 'mediatek, infra-reset' to compatible

[Crystal Guo]

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 <crystal.guo@mediatek.com>
---
 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[v4,3/4] reset-controller: ti: introduce a new reset handler

[Crystal Guo]

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 <crystal.guo@mediatek.com>
---
 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[v4,4/4] arm64: dts: mt8192: add infracfg_rst node

[Crystal Guo]

add infracfg_rst node which is for MT8192 platform

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4, 1/4] dt-binding: reset-controller: ti: add reset-duration-us property

[Rob Herring]

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 <crystal.guo@mediatek.com>
> ---
>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,2/4] dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to compatible

[Rob Herring]

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 <crystal.guo@mediatek.com>
> ---
>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,1/4] dt-binding: reset-controller: ti: add reset-duration-us property

[Crystal Guo]

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 <crystal.guo@mediatek.com>
> > ---
> >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,2/4] dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to compatible

[Crystal Guo]

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 <crystal.guo@mediatek.com>
> > ---
> >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,0/4] introduce TI reset controller for MT8192 SoC

[Crystal Guo]

Hi Rob, Philipp, Matthias and all

Gentle ping for this patch set.

Thanks
Crystal

> 
> -----Original Message-----
> From: Crystal Guo [mailto:crystal.guo@mediatek.com]
> Sent: Monday, August 17, 2020 11:03 AM
> To: p.zabel@pengutronix.de; robh+dt@kernel.org; matthias.bgg@gmail.com
> Cc: srv_heupstream; linux-mediatek@lists.infradead.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; s-anna@ti.com; afd@ti.com; 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*********************

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,2/4] dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to compatible

[Suman Anna]

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 <crystal.guo@mediatek.com>
>>> ---
>>>  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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,4/4] arm64: dts: mt8192: add infracfg_rst node

[Suman Anna]

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 <crystal.guo@mediatek.com>

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 {
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Suman Anna]

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 <crystal.guo@mediatek.com>
> ---
>  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;
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,4/4] arm64: dts: mt8192: add infracfg_rst node

[Crystal Guo]

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 <crystal.guo@mediatek.com>
> 
> 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 {
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,4/4] arm64: dts: mt8192: add infracfg_rst node

[Suman Anna]

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 <crystal.guo@mediatek.com>
>>
>> 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 {
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,2/4] dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to compatible

[Rob Herring]

On Wed, Sep 2, 2020 at 5:26 PM Suman Anna <s-anna@ti.com> 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 <crystal.guo@mediatek.com>
> >>> ---
> >>>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Crystal Guo]

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 <crystal.guo@mediatek.com>
> > ---
> >  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;
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,2/4] dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to compatible

[Suman Anna]

On 9/8/20 1:49 PM, Rob Herring wrote:
> On Wed, Sep 2, 2020 at 5:26 PM Suman Anna <s-anna@ti.com> 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 <crystal.guo@mediatek.com>
>>>>> ---
>>>>>  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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Suman Anna]

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 <crystal.guo@mediatek.com>
>>> ---
>>>  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;
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,2/4] dt-binding: reset-controller: ti: add 'mediatek,infra-reset' to compatible

[Rob Herring]

On Wed, Sep 9, 2020 at 9:10 AM Suman Anna <s-anna@ti.com> wrote:
>
> On 9/8/20 1:49 PM, Rob Herring wrote:
> > On Wed, Sep 2, 2020 at 5:26 PM Suman Anna <s-anna@ti.com> 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 <crystal.guo@mediatek.com>
> >>>>> ---
> >>>>>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Crystal Guo]

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 <crystal.guo@mediatek.com>
> >>> ---
> >>>  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;
> >>>
> >>
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Suman Anna]

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 <crystal.guo@mediatek.com>
>>>>> ---
>>>>>  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;
>>>>>
>>>>
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Crystal Guo]

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 <crystal.guo@mediatek.com>
> >>>>> ---
> >>>>>  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;
> >>>>>
> >>>>
> >>>
> >>
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Philipp Zabel]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Suman Anna]

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Crystal Guo]

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
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v4,3/4] reset-controller: ti: introduce a new reset handler

[Crystal Guo]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel