| Message ID | 20200817030324.5690-4-crystal.guo@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce TI reset controller for MT8192 SoC | expand |
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; >
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; > > >
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; >>> >> >
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; > >>> > >> > > >
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; >>>>> >>>> >>> >> >
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; > >>>>> > >>>> > >>> > >> > > >
Hi Crystal, On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote: [...] > Should I add the SoC-specific data as follows? > This may also modify the ti original code, is it OK? > > + data->reset_data = of_device_get_match_data(&pdev->dev); > + > + list = of_get_property(np, data->reset_data->reset_bits, &size); > > +static const struct common_reset_data ti_reset_data = { > + .reset_op_available = false, > + .reset_bits = "ti, reset-bits", ^ That space doesn't belong there. > +}; > + > +static const struct common_reset_data mediatek_reset_data = { > + .reset_op_available = true, > + .reset_bits = "mediatek, reset-bits", > +}; I understand Robs comments as meaning "ti,reset-bits" should have been called "reset-bits" in the first place, and you shouldn't repeat adding the vendor prefix, as that is implied by the compatible. So this should probably be just "reset-bits". Otherwise this looks like it should work. regards Philipp
On 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 >
On Fri, 2020-09-11 at 22:44 +0800, Suman Anna wrote: > On 9/11/20 9:26 AM, Philipp Zabel wrote: > > Hi Crystal, > > > > On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote: > > [...] > >> Should I add the SoC-specific data as follows? > >> This may also modify the ti original code, is it OK? > >> > >> + data->reset_data = of_device_get_match_data(&pdev->dev); > >> + > >> + list = of_get_property(np, data->reset_data->reset_bits, &size); > >> > >> +static const struct common_reset_data ti_reset_data = { > >> + .reset_op_available = false, > >> + .reset_bits = "ti, reset-bits", > > ^ > > That space doesn't belong there. > > > >> +}; > >> + > >> +static const struct common_reset_data mediatek_reset_data = { > >> + .reset_op_available = true, > >> + .reset_bits = "mediatek, reset-bits", > >> +}; > > > > I understand Robs comments as meaning "ti,reset-bits" should have been > > called "reset-bits" in the first place, and you shouldn't repeat adding > > the vendor prefix, as that is implied by the compatible. So this should > > probably be just "reset-bits". > > Hmm, not sure about that. I think Rob wants the reset data itself to be added in > the driver as is being done on some other SoCs (eg: like in reset-qcom-pdc.c). > > regards > Suman > Hi Rob, Can you help to comment about this point? Modify "ti,reset-bits" to "reset-bits" or add "mediatek,reset-bits" ? Many thanks~ Crystal > > > > Otherwise this looks like it should work. > > > > regards > > Philipp > > >
On Mon, 2020-09-14 at 22:00 +0800, Crystal Guo wrote: > On Fri, 2020-09-11 at 22:44 +0800, Suman Anna wrote: > > On 9/11/20 9:26 AM, Philipp Zabel wrote: > > > Hi Crystal, > > > > > > On Fri, 2020-09-11 at 14:07 +0800, Crystal Guo wrote: > > > [...] > > >> Should I add the SoC-specific data as follows? > > >> This may also modify the ti original code, is it OK? > > >> > > >> + data->reset_data = of_device_get_match_data(&pdev->dev); > > >> + > > >> + list = of_get_property(np, data->reset_data->reset_bits, &size); > > >> > > >> +static const struct common_reset_data ti_reset_data = { > > >> + .reset_op_available = false, > > >> + .reset_bits = "ti, reset-bits", > > > ^ > > > That space doesn't belong there. > > > > > >> +}; > > >> + > > >> +static const struct common_reset_data mediatek_reset_data = { > > >> + .reset_op_available = true, > > >> + .reset_bits = "mediatek, reset-bits", > > >> +}; > > > > > > I understand Robs comments as meaning "ti,reset-bits" should have been > > > called "reset-bits" in the first place, and you shouldn't repeat adding > > > the vendor prefix, as that is implied by the compatible. So this should > > > probably be just "reset-bits". > > > > Hmm, not sure about that. I think Rob wants the reset data itself to be added in > > the driver as is being done on some other SoCs (eg: like in reset-qcom-pdc.c). > > > > regards > > Suman > > > Hi Rob, > > Can you help to comment about this point? > Modify "ti,reset-bits" to "reset-bits" or add "mediatek,reset-bits" ? > > Many thanks~ > Crystal > > > > > > > Otherwise this looks like it should work. > > > > > > regards > > > Philipp > > > > > Dears, I have uploaded the changes at https://patchwork.kernel.org/cover/11805937/ Please help me to review, many thanks~~ regards Crystal >
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;
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(-)