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(-)
add infracfg_rst node which is for MT8192 platform
Signed-off-by: Crystal Guo <[email protected]>
---
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
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 <[email protected]>
---
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
On 29/07/2020 09:39, Crystal Guo wrote:
> add infracfg_rst node which is for MT8192 platform
>
> Signed-off-by: Crystal Guo <[email protected]>
> ---
> 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 {
>
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 <[email protected]>
> ---
> 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)
>
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > ---
> > 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 {
> >
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 <[email protected]>
> > > ---
> > > 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