| Message ID | 1596008357-11213-2-git-send-email-crystal.guo@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | adjust the reset assert and deassert interface | expand |
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) >
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
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
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)
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(-)