[PATCH 0/2] adjust the reset assert and deassert interface

[PATCH 0/2] adjust the reset assert and deassert interface

[Crystal Guo]

1. adjust the reset assert and deassert interface
to make it more compatible.
2. add infracfg_rst device node.

Crystal Guo (2):
  reset-controller: ti: adjust the reset assert and deassert interface
  arm64: dts: mt8192: add infracfg_rst node

 arch/arm64/boot/dts/mediatek/mt8192.dtsi | 10 +++++++++-
 drivers/reset/reset-ti-syscon.c          | 13 +++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

[PATCH 1/2] reset-controller: ti: adjust the reset assert and deassert interface

[Crystal Guo]

Add ti_syscon_reset() to integrate assert and deassert together,
and change return value of the reset assert and deassert interface
from regmap_update_bits to regmap_write_bits.

when clear bit is already 1, regmap_update_bits can not write 1 to it again.
Some IC has the feature that, when set bit is 1, the clear bit change
to 1 together. It will truly clear bit to 0 by write 1 to the clear bit

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 drivers/reset/reset-ti-syscon.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
index a2635c2..5a8ec8f 100644
--- a/drivers/reset/reset-ti-syscon.c
+++ b/drivers/reset/reset-ti-syscon.c
@@ -89,7 +89,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 +120,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,10 +158,19 @@ 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)
+{
+	ti_syscon_reset_assert(rcdev, id);
+
+	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,
 	.status		= ti_syscon_reset_status,
+	.reset		= ti_syscon_reset,
 };
 
 static int ti_syscon_reset_probe(struct platform_device *pdev)
-- 
1.8.1.1.dirty

[PATCH 2/2] 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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
index b16dbbd..adc6239 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
@@ -217,9 +217,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 = "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 {
-- 
1.8.1.1.dirty

Re: [PATCH 2/2] arm64: dts: mt8192: add infracfg_rst node

[Matthias Brugger]

On 29/07/2020 09:39, Crystal Guo wrote:
> 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 | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> index b16dbbd..adc6239 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> @@ -217,9 +217,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 = "ti,syscon-reset";
> +				#reset-cells = <1>;
> +				ti,reset-bits = <
> +					0x140 15 0x144 15 0 0 (ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 0: pcie */

You have Texas Instruments hardware inside infracfg? Are you sure?

> +				>;
> +			};
>   		};
>   
>   		pericfg: pericfg@10003000 {
> 

Re: [PATCH 1/2] reset-controller: ti: adjust the reset assert and deassert interface

[Matthias Brugger]

On 29/07/2020 09:39, Crystal Guo wrote:
> Add ti_syscon_reset() to integrate assert and deassert together,
> and change return value of the reset assert and deassert interface
> from regmap_update_bits to regmap_write_bits.
> 
> when clear bit is already 1, regmap_update_bits can not write 1 to it again.
> Some IC has the feature that, when set bit is 1, the clear bit change
> to 1 together. It will truly clear bit to 0 by write 1 to the clear bit
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>   drivers/reset/reset-ti-syscon.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index a2635c2..5a8ec8f 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -89,7 +89,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);

Nack, this will break the driver for the other devices.
The kernel has to work not just for your SoC but for all devices of all 
architectures. You can't just hack something up, that will work on your specific 
SoC.

Regards,
Matthias

>   }
>   
>   /**
> @@ -120,7 +120,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,10 +158,19 @@ 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)
> +{
> +	ti_syscon_reset_assert(rcdev, id);
> +
> +	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,
>   	.status		= ti_syscon_reset_status,
> +	.reset		= ti_syscon_reset,
>   };
>   
>   static int ti_syscon_reset_probe(struct platform_device *pdev)
> 

Re: [PATCH 1/2] reset-controller: ti: adjust the reset assert and deassert interface

[Philipp Zabel]

Hi Crystal, Matthias,

On Wed, 2020-07-29 at 09:48 +0200, Matthias Brugger wrote:
> 
> On 29/07/2020 09:39, Crystal Guo wrote:
> > Add ti_syscon_reset() to integrate assert and deassert together,
> > and change return value of the reset assert and deassert interface
> > from regmap_update_bits to regmap_write_bits.
> > 
> > when clear bit is already 1, regmap_update_bits can not write 1 to it again.
> > Some IC has the feature that, when set bit is 1, the clear bit change
> > to 1 together. It will truly clear bit to 0 by write 1 to the clear bit
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >   drivers/reset/reset-ti-syscon.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> > index a2635c2..5a8ec8f 100644
> > --- a/drivers/reset/reset-ti-syscon.c
> > +++ b/drivers/reset/reset-ti-syscon.c
> > @@ -89,7 +89,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);
> 
> Nack, this will break the driver for the other devices.

I don't think this will break the driver for existing hardware.
regmap_write_bits() is the same as regmap_update_bits(), it just forces
the write in case the read already happens to return the correct value.
Of course it would be good to check that this actually works.

> The kernel has to work not just for your SoC but for all devices of all 
> architectures. You can't just hack something up, that will work on your specific 
> SoC.
> 
> Regards,
> Matthias
> 
> >   }
> >   
> >   /**
> > @@ -120,7 +120,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,10 +158,19 @@ 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)
> > +{
> > +	ti_syscon_reset_assert(rcdev, id);
> > +
> > +	return ti_syscon_reset_deassert(rcdev, id);
> > +}
> > +

I'm unsure about this one, though. This is an incompatible change. At
the very least this would have to be optional depending on compatible.

regards
Philipp

Re: [PATCH 2/2] arm64: dts: mt8192: add infracfg_rst node

[Crystal Guo]

On Wed, 2020-07-29 at 15:45 +0800, Matthias Brugger wrote:
> 
> On 29/07/2020 09:39, Crystal Guo wrote:
> > 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 | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > index b16dbbd..adc6239 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > @@ -217,9 +217,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 = "ti,syscon-reset";
> > +				#reset-cells = <1>;
> > +				ti,reset-bits = <
> > +					0x140 15 0x144 15 0 0 (ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 0: pcie */
> 
> You have Texas Instruments hardware inside infracfg? Are you sure?
> 
TI reset-controller driver is a common driver, MTK SOC has the similar
control flow, thus can reuse it.

> > +				>;
> > +			};
> >   		};
> >   
> >   		pericfg: pericfg@10003000 {
> > 

Re: [PATCH 1/2] reset-controller: ti: adjust the reset assert and deassert interface

[Crystal Guo]

On Wed, 2020-07-29 at 16:02 +0800, Philipp Zabel wrote:
> Hi Crystal, Matthias,
> 
> On Wed, 2020-07-29 at 09:48 +0200, Matthias Brugger wrote:
> > 
> > On 29/07/2020 09:39, Crystal Guo wrote:
> > > Add ti_syscon_reset() to integrate assert and deassert together,
> > > and change return value of the reset assert and deassert interface
> > > from regmap_update_bits to regmap_write_bits.
> > > 
> > > when clear bit is already 1, regmap_update_bits can not write 1 to it again.
> > > Some IC has the feature that, when set bit is 1, the clear bit change
> > > to 1 together. It will truly clear bit to 0 by write 1 to the clear bit
> > > 
> > > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > > ---
> > >   drivers/reset/reset-ti-syscon.c | 13 +++++++++++--
> > >   1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> > > index a2635c2..5a8ec8f 100644
> > > --- a/drivers/reset/reset-ti-syscon.c
> > > +++ b/drivers/reset/reset-ti-syscon.c
> > > @@ -89,7 +89,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);
> > 
> > Nack, this will break the driver for the other devices.
> 
> I don't think this will break the driver for existing hardware.
> regmap_write_bits() is the same as regmap_update_bits(), it just forces
> the write in case the read already happens to return the correct value.
> Of course it would be good to check that this actually works.

Yes, regmap_write_bits() is the same as regmap_update_bits(), it would
not affect existed users. Or should I use a property to separate
regmap_write_bits() and regmap_update_bits() ?

> 
> > The kernel has to work not just for your SoC but for all devices of all 
> > architectures. You can't just hack something up, that will work on your specific 
> > SoC.
> > 
> > Regards,
> > Matthias 
> > 

This TI driver was intend to be a generic reset controller
(https://lore.kernel.org/patchwork/cover/683585/), so this patch may
not just work on a specific SoC.

Thanks,
Crystal

> > >   }
> > >   
> > >   /**
> > > @@ -120,7 +120,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,10 +158,19 @@ 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)
> > > +{
> > > +	ti_syscon_reset_assert(rcdev, id);
> > > +
> > > +	return ti_syscon_reset_deassert(rcdev, id);
> > > +}
> > > +
> 
> I'm unsure about this one, though. This is an incompatible change. At
> the very least this would have to be optional depending on compatible.
> 
> regards
> Philipp

I will add a property to make this change be optional, thanks for you
advice.

Thanks,
Crystal