| Message ID | 20250321151831.623575-4-elder@riscstar.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: spacemit: add K1 reset support | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | fail | Failed to apply series |
Hi Alex: On 10:18 Fri 21 Mar , Alex Elder wrote: > Define ccu_reset_data as a structure that contains the constant > register offset and bitmasks used to assert and deassert a reset > control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as > a structure that contains the address of an array of those structures > and a count of the number of elements in the array. > > Add a pointer to a ccu_reset_controller_data structure to the > k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; > the new pointer field will be null for CCUs without any resets. > > Finally, define a new ccu_reset_controller structure, which (for > a CCU with resets) contains a pointer to the constant reset data, > the regmap to be used for the controller, and an embedded a reset > controller structure. > > Each reset control is asserted or deasserted by updating bits in > a register. The bits used are defined by an assert mask and a > deassert mask. In some cases, one (non-zero) mask asserts reset > and a different (non-zero) mask deasserts it. Otherwise one mask > is nonzero, and the other is zero. Either way, the bits in > both masks are cleared, then either the assert mask or the deassert > mask is set in a register to affect the state of a reset control. > > Signed-off-by: Alex Elder <elder@riscstar.com> > --- > drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c > index f7367271396a0..6d879411c6c05 100644 > --- a/drivers/clk/spacemit/ccu-k1.c > +++ b/drivers/clk/spacemit/ccu-k1.c > @@ -10,6 +10,7 @@ > #include <linux/minmax.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/reset-controller.h> > > #include "ccu_common.h" > #include "ccu_pll.h" > @@ -134,8 +135,26 @@ struct spacemit_ccu_clk { > struct clk_hw *hw; > }; > > +struct ccu_reset_data { > + u32 offset; > + u32 assert_mask; > + u32 deassert_mask; > +}; > + > +struct ccu_reset_controller_data { > + u32 count; > + const struct ccu_reset_data *data; /* array */ > +}; > + > struct k1_ccu_data { > struct spacemit_ccu_clk *clk; /* array with sentinel */ > + const struct ccu_reset_controller_data *rst_data; > +}; > + > +struct ccu_reset_controller { > + struct regmap *regmap; > + const struct ccu_reset_controller_data *data; > + struct reset_controller_dev rcdev; > }; > > /* APBS clocks start */ > @@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = { > .clk = k1_ccu_apmu_clks, > }; > > +static struct ccu_reset_controller * > +rcdev_to_controller(struct reset_controller_dev *rcdev) I'd suggest to avoid the line break to make it slightly more readable, intuitive as the 80 column limit isn't hard rule there are maybe more place similar to this, I won't add more comments https://github.com/torvalds/linux/commit/bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > +{ > + return container_of(rcdev, struct ccu_reset_controller, rcdev); > +} since this function is only used once, open-code it? but I'd fine with either way if you prefer to keep it > + > +static int > +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert) s/k1_rst_update/k1_reset_update/g this is a taste change, but I found more people follow this when grep driver/reset > +{ > + struct ccu_reset_controller *controller = rcdev_to_controller(rcdev); > + struct regmap *regmap = controller->regmap; > + const struct ccu_reset_data *data; > + u32 val; > + int ret; > + > + data = &controller->data->data[id]; > + > + ret = regmap_read(regmap, data->offset, &val); > + if (ret) > + return ret; > + > + val &= ~(data->assert_mask | data->deassert_mask); > + val |= assert ? data->assert_mask : data->deassert_mask; > + > + return regmap_write(regmap, data->offset, val); > +} > + > +static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id) same reason, rst -> reset, more below > +{ > + return k1_rst_update(rcdev, id, true); > +} > + > +static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + return k1_rst_update(rcdev, id, false); > +} > + > +static const struct reset_control_ops k1_reset_control_ops = { > + .assert = k1_rst_assert, > + .deassert = k1_rst_deassert, > +}; > + > static int k1_ccu_register(struct device *dev, struct regmap *regmap, > struct regmap *lock_regmap, > struct spacemit_ccu_clk *clks) > @@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap, > return ret; > } > > +static int > +k1_reset_controller_register(struct device *dev, struct regmap *regmap, > + const struct ccu_reset_controller_data *data) > +{ > + struct ccu_reset_controller *controller; > + struct reset_controller_dev *rcdev; > + > + /* Resets are optional */ > + if (!data) > + return 0; > + > + controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); > + if (!controller) > + return -ENOMEM; > + > + controller->regmap = regmap; > + controller->data = data; > + > + rcdev = &controller->rcdev; .. > + rcdev->owner = THIS_MODULE; move to last? > + rcdev->nr_resets = data->count; > + rcdev->ops = &k1_reset_control_ops; > + rcdev->of_node = dev->of_node; > + > + return devm_reset_controller_register(dev, rcdev); > +} > + > static int k1_ccu_probe(struct platform_device *pdev) > { > struct regmap *base_regmap, *lock_regmap = NULL; > @@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev) > if (ret) > return dev_err_probe(dev, ret, "failed to register clocks\n"); > > + ret = k1_reset_controller_register(dev, base_regmap, data->rst_data); .. > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to register reset controller\n"); same 100 column reason > + > return 0; > } > > -- > 2.43.0 >
On 3/22/25 11:19 AM, Yixun Lan wrote: > Hi Alex: > > On 10:18 Fri 21 Mar , Alex Elder wrote: >> Define ccu_reset_data as a structure that contains the constant >> register offset and bitmasks used to assert and deassert a reset >> control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as >> a structure that contains the address of an array of those structures >> and a count of the number of elements in the array. >> >> Add a pointer to a ccu_reset_controller_data structure to the >> k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; >> the new pointer field will be null for CCUs without any resets. >> >> Finally, define a new ccu_reset_controller structure, which (for >> a CCU with resets) contains a pointer to the constant reset data, >> the regmap to be used for the controller, and an embedded a reset >> controller structure. >> >> Each reset control is asserted or deasserted by updating bits in >> a register. The bits used are defined by an assert mask and a >> deassert mask. In some cases, one (non-zero) mask asserts reset >> and a different (non-zero) mask deasserts it. Otherwise one mask >> is nonzero, and the other is zero. Either way, the bits in >> both masks are cleared, then either the assert mask or the deassert >> mask is set in a register to affect the state of a reset control. >> >> Signed-off-by: Alex Elder <elder@riscstar.com> >> --- >> drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 93 insertions(+) >> >> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c >> index f7367271396a0..6d879411c6c05 100644 >> --- a/drivers/clk/spacemit/ccu-k1.c >> +++ b/drivers/clk/spacemit/ccu-k1.c >> @@ -10,6 +10,7 @@ >> #include <linux/minmax.h> >> #include <linux/module.h> >> #include <linux/platform_device.h> >> +#include <linux/reset-controller.h> >> >> #include "ccu_common.h" >> #include "ccu_pll.h" >> @@ -134,8 +135,26 @@ struct spacemit_ccu_clk { >> struct clk_hw *hw; >> }; >> >> +struct ccu_reset_data { >> + u32 offset; >> + u32 assert_mask; >> + u32 deassert_mask; >> +}; >> + >> +struct ccu_reset_controller_data { >> + u32 count; >> + const struct ccu_reset_data *data; /* array */ >> +}; >> + >> struct k1_ccu_data { >> struct spacemit_ccu_clk *clk; /* array with sentinel */ >> + const struct ccu_reset_controller_data *rst_data; >> +}; >> + >> +struct ccu_reset_controller { >> + struct regmap *regmap; >> + const struct ccu_reset_controller_data *data; >> + struct reset_controller_dev rcdev; >> }; >> >> /* APBS clocks start */ >> @@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = { >> .clk = k1_ccu_apmu_clks, >> }; >> >> +static struct ccu_reset_controller * >> +rcdev_to_controller(struct reset_controller_dev *rcdev) > I'd suggest to avoid the line break to make it slightly more readable, intuitive > as the 80 column limit isn't hard rule > > there are maybe more place similar to this, I won't add more comments > https://github.com/torvalds/linux/commit/bdc48fa11e46f867ea4d75fa59ee87a7f48be144 I disagree with this suggestion. I personally find this more readable. As the first line of the patch you link to, "80 columns is still preferred". And regardless, it is my (strong) preference to work within 80 columns in almost all cases. >> +{ >> + return container_of(rcdev, struct ccu_reset_controller, rcdev); >> +} > since this function is only used once, open-code it? > but I'd fine with either way if you prefer to keep it The "to_<containing_type>()" function pattern is extremely common, but I like this suggestion, given it's used only once. I'll implement it in v2. > >> + >> +static int >> +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert) > s/k1_rst_update/k1_reset_update/g > this is a taste change, but I found more people follow this when grep driver/reset I actually had reset (not rst) before, throughout. But it made a few lines too long, leading to line wraps, so I did this. In addition, there was a sort of consistency with the use of "clk" instead of "clock", though I do recognize that abbreviation goes way back to when Mike implemented the common clock framework. I'll switch back to "reset" (and "RESET") in names, but be warned I'll add some line breaks to fit within 80 columns. >> +{ >> + struct ccu_reset_controller *controller = rcdev_to_controller(rcdev); >> + struct regmap *regmap = controller->regmap; >> + const struct ccu_reset_data *data; >> + u32 val; >> + int ret; >> + >> + data = &controller->data->data[id]; >> + >> + ret = regmap_read(regmap, data->offset, &val); >> + if (ret) >> + return ret; >> + >> + val &= ~(data->assert_mask | data->deassert_mask); >> + val |= assert ? data->assert_mask : data->deassert_mask; >> + >> + return regmap_write(regmap, data->offset, val); >> +} >> + >> +static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id) > same reason, rst -> reset, more below >> +{ >> + return k1_rst_update(rcdev, id, true); >> +} >> + >> +static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id) >> +{ >> + return k1_rst_update(rcdev, id, false); >> +} >> + >> +static const struct reset_control_ops k1_reset_control_ops = { >> + .assert = k1_rst_assert, >> + .deassert = k1_rst_deassert, >> +}; >> + >> static int k1_ccu_register(struct device *dev, struct regmap *regmap, >> struct regmap *lock_regmap, >> struct spacemit_ccu_clk *clks) >> @@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap, >> return ret; >> } >> >> +static int >> +k1_reset_controller_register(struct device *dev, struct regmap *regmap, >> + const struct ccu_reset_controller_data *data) >> +{ >> + struct ccu_reset_controller *controller; >> + struct reset_controller_dev *rcdev; >> + >> + /* Resets are optional */ >> + if (!data) >> + return 0; >> + >> + controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); >> + if (!controller) >> + return -ENOMEM; >> + >> + controller->regmap = regmap; >> + controller->data = data; >> + >> + rcdev = &controller->rcdev; > .. >> + rcdev->owner = THIS_MODULE; > move to last? You mean move nr_resets to last? I'll do that. I'll order the assignments in the order they're defined in "reset-controller.h". >> + rcdev->nr_resets = data->count; >> + rcdev->ops = &k1_reset_control_ops; >> + rcdev->of_node = dev->of_node; >> + >> + return devm_reset_controller_register(dev, rcdev); >> +} >> + >> static int k1_ccu_probe(struct platform_device *pdev) >> { >> struct regmap *base_regmap, *lock_regmap = NULL; >> @@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev) >> if (ret) >> return dev_err_probe(dev, ret, "failed to register clocks\n"); >> >> + ret = k1_reset_controller_register(dev, base_regmap, data->rst_data); > .. >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "failed to register reset controller\n"); > same 100 column reason This one I might go beyond columns, because it's only a few characters. -Alex >> + >> return 0; >> } >> >> -- >> 2.43.0 >> >
Hi Alex: On 08:23 Sun 23 Mar , Alex Elder wrote: > On 3/22/25 11:19 AM, Yixun Lan wrote: > > Hi Alex: > > > > On 10:18 Fri 21 Mar , Alex Elder wrote: > >> Define ccu_reset_data as a structure that contains the constant > >> register offset and bitmasks used to assert and deassert a reset > >> control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as > >> a structure that contains the address of an array of those structures > >> and a count of the number of elements in the array. > >> > >> Add a pointer to a ccu_reset_controller_data structure to the > >> k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; > >> the new pointer field will be null for CCUs without any resets. > >> > >> Finally, define a new ccu_reset_controller structure, which (for > >> a CCU with resets) contains a pointer to the constant reset data, > >> the regmap to be used for the controller, and an embedded a reset > >> controller structure. > >> > >> Each reset control is asserted or deasserted by updating bits in > >> a register. The bits used are defined by an assert mask and a > >> deassert mask. In some cases, one (non-zero) mask asserts reset > >> and a different (non-zero) mask deasserts it. Otherwise one mask > >> is nonzero, and the other is zero. Either way, the bits in > >> both masks are cleared, then either the assert mask or the deassert > >> mask is set in a register to affect the state of a reset control. > >> > >> Signed-off-by: Alex Elder <elder@riscstar.com> > >> --- > >> drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++ > >> 1 file changed, 93 insertions(+) > >> > >> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c > >> index f7367271396a0..6d879411c6c05 100644 > >> --- a/drivers/clk/spacemit/ccu-k1.c > >> +++ b/drivers/clk/spacemit/ccu-k1.c > >> @@ -10,6 +10,7 @@ > >> #include <linux/minmax.h> > >> #include <linux/module.h> > >> #include <linux/platform_device.h> > >> +#include <linux/reset-controller.h> > >> > >> #include "ccu_common.h" > >> #include "ccu_pll.h" > >> @@ -134,8 +135,26 @@ struct spacemit_ccu_clk { > >> struct clk_hw *hw; > >> }; > >> > >> +struct ccu_reset_data { > >> + u32 offset; > >> + u32 assert_mask; > >> + u32 deassert_mask; > >> +}; > >> + > >> +struct ccu_reset_controller_data { > >> + u32 count; > >> + const struct ccu_reset_data *data; /* array */ > >> +}; > >> + > >> struct k1_ccu_data { > >> struct spacemit_ccu_clk *clk; /* array with sentinel */ > >> + const struct ccu_reset_controller_data *rst_data; > >> +}; > >> + > >> +struct ccu_reset_controller { > >> + struct regmap *regmap; > >> + const struct ccu_reset_controller_data *data; > >> + struct reset_controller_dev rcdev; > >> }; > >> > >> /* APBS clocks start */ > >> @@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = { > >> .clk = k1_ccu_apmu_clks, > >> }; > >> > >> +static struct ccu_reset_controller * > >> +rcdev_to_controller(struct reset_controller_dev *rcdev) > > I'd suggest to avoid the line break to make it slightly more readable, intuitive > > as the 80 column limit isn't hard rule > > > > there are maybe more place similar to this, I won't add more comments > > https://github.com/torvalds/linux/commit/bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > > I disagree with this suggestion. I personally find this > more readable. As the first line of the patch you link to, > "80 columns is still preferred". And regardless, it is my > (strong) preference to work within 80 columns in almost all > cases. > I can understand this isn't *hard* rule, and even subsystem maintainer may has their own preference, but for contributing SpacemiT, I'd hope we could reach certain consensus, so will have sorts of consistent coding style, I've been requested several times to extend to 100 columns (see link below), and I do agree it will end at less lines which makes the code more readable.. https://lore.kernel.org/all/20250302-04-gpio-irq-threecell-v2-1-34f13ad37ea4@gentoo.org > >> +{ > >> + return container_of(rcdev, struct ccu_reset_controller, rcdev); > >> +} > > since this function is only used once, open-code it? > > but I'd fine with either way if you prefer to keep it > > The "to_<containing_type>()" function pattern is extremely > common, but I like this suggestion, given it's used only > once. I'll implement it in v2. > > > > >> + > >> +static int > >> +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert) > > s/k1_rst_update/k1_reset_update/g > > this is a taste change, but I found more people follow this when grep driver/reset > > I actually had reset (not rst) before, throughout. But it made > a few lines too long, leading to line wraps, so I did this. > > In addition, there was a sort of consistency with the use of > "clk" instead of "clock", though I do recognize that abbreviation > goes way back to when Mike implemented the common clock framework. > > I'll switch back to "reset" (and "RESET") in names, but be warned > I'll add some line breaks to fit within 80 columns. > > >> +{ > >> + struct ccu_reset_controller *controller = rcdev_to_controller(rcdev); > >> + struct regmap *regmap = controller->regmap; > >> + const struct ccu_reset_data *data; > >> + u32 val; > >> + int ret; > >> + > >> + data = &controller->data->data[id]; > >> + > >> + ret = regmap_read(regmap, data->offset, &val); > >> + if (ret) > >> + return ret; > >> + > >> + val &= ~(data->assert_mask | data->deassert_mask); > >> + val |= assert ? data->assert_mask : data->deassert_mask; > >> + > >> + return regmap_write(regmap, data->offset, val); > >> +} > >> + > >> +static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id) > > same reason, rst -> reset, more below > >> +{ > >> + return k1_rst_update(rcdev, id, true); > >> +} > >> + > >> +static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id) > >> +{ > >> + return k1_rst_update(rcdev, id, false); > >> +} > >> + > >> +static const struct reset_control_ops k1_reset_control_ops = { > >> + .assert = k1_rst_assert, > >> + .deassert = k1_rst_deassert, > >> +}; > >> + > >> static int k1_ccu_register(struct device *dev, struct regmap *regmap, > >> struct regmap *lock_regmap, > >> struct spacemit_ccu_clk *clks) > >> @@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap, > >> return ret; > >> } > >> > >> +static int > >> +k1_reset_controller_register(struct device *dev, struct regmap *regmap, > >> + const struct ccu_reset_controller_data *data) > >> +{ > >> + struct ccu_reset_controller *controller; > >> + struct reset_controller_dev *rcdev; > >> + > >> + /* Resets are optional */ > >> + if (!data) > >> + return 0; > >> + > >> + controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); > >> + if (!controller) > >> + return -ENOMEM; > >> + > >> + controller->regmap = regmap; > >> + controller->data = data; > >> + > >> + rcdev = &controller->rcdev; > > .. > >> + rcdev->owner = THIS_MODULE; > > move to last? > > You mean move nr_resets to last? I'll do that. I'll > order the assignments in the order they're defined in > "reset-controller.h". > I mean after rcdev->of_node, suggested based on alphabet letter, but I do see people group them based on functionality.. so whatever you decide, fine by me > >> + rcdev->nr_resets = data->count; > >> + rcdev->ops = &k1_reset_control_ops; > >> + rcdev->of_node = dev->of_node; > >> + > >> + return devm_reset_controller_register(dev, rcdev); > >> +} > >> + > >> static int k1_ccu_probe(struct platform_device *pdev) > >> { > >> struct regmap *base_regmap, *lock_regmap = NULL; > >> @@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev) > >> if (ret) > >> return dev_err_probe(dev, ret, "failed to register clocks\n"); > >> > >> + ret = k1_reset_controller_register(dev, base_regmap, data->rst_data); > > .. > >> + if (ret) > >> + return dev_err_probe(dev, ret, > >> + "failed to register reset controller\n"); > > same 100 column reason > > This one I might go beyond columns, because it's only a few characters. > yes, we have rules not to break strings, but sometimes, I found even better when not breaking the whole line (better to do grep) thanks for your work!
On Fri, Mar 21, 2025 at 10:18:26AM -0500, Alex Elder wrote: > Define ccu_reset_data as a structure that contains the constant > register offset and bitmasks used to assert and deassert a reset > control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as > a structure that contains the address of an array of those structures > and a count of the number of elements in the array. > > Add a pointer to a ccu_reset_controller_data structure to the > k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; > the new pointer field will be null for CCUs without any resets. > > Finally, define a new ccu_reset_controller structure, which (for > a CCU with resets) contains a pointer to the constant reset data, > the regmap to be used for the controller, and an embedded a reset > controller structure. > > Each reset control is asserted or deasserted by updating bits in > a register. The bits used are defined by an assert mask and a > deassert mask. In some cases, one (non-zero) mask asserts reset > and a different (non-zero) mask deasserts it. Otherwise one mask > is nonzero, and the other is zero. Either way, the bits in > both masks are cleared, then either the assert mask or the deassert > mask is set in a register to affect the state of a reset control. > > Signed-off-by: Alex Elder <elder@riscstar.com> > --- > drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c > index f7367271396a0..6d879411c6c05 100644 > --- a/drivers/clk/spacemit/ccu-k1.c > +++ b/drivers/clk/spacemit/ccu-k1.c ... > +static int > +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert) > +{ > + struct ccu_reset_controller *controller = rcdev_to_controller(rcdev); > + struct regmap *regmap = controller->regmap; > + const struct ccu_reset_data *data; > + u32 val; > + int ret; > + > + data = &controller->data->data[id]; > + > + ret = regmap_read(regmap, data->offset, &val); > + if (ret) > + return ret; > + > + val &= ~(data->assert_mask | data->deassert_mask); > + val |= assert ? data->assert_mask : data->deassert_mask; > + > + return regmap_write(regmap, data->offset, val); > +} I don't think it's safe to write the regmap based on a value read earlier without the regmap's inner lock held: it's totally fine for the clock part to issue an update of the register at the same time. Without knowledge on it, reset code may rollback the clock bits written by clock code earlier to the original value. That's why I keep using ccu_update() everywhere and dropped ccu_write(). Thanks, Haylen Chu
On 3/24/25 7:20 AM, Haylen Chu wrote: > On Fri, Mar 21, 2025 at 10:18:26AM -0500, Alex Elder wrote: >> Define ccu_reset_data as a structure that contains the constant >> register offset and bitmasks used to assert and deassert a reset >> control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as >> a structure that contains the address of an array of those structures >> and a count of the number of elements in the array. >> >> Add a pointer to a ccu_reset_controller_data structure to the >> k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; >> the new pointer field will be null for CCUs without any resets. >> >> Finally, define a new ccu_reset_controller structure, which (for >> a CCU with resets) contains a pointer to the constant reset data, >> the regmap to be used for the controller, and an embedded a reset >> controller structure. >> >> Each reset control is asserted or deasserted by updating bits in >> a register. The bits used are defined by an assert mask and a >> deassert mask. In some cases, one (non-zero) mask asserts reset >> and a different (non-zero) mask deasserts it. Otherwise one mask >> is nonzero, and the other is zero. Either way, the bits in >> both masks are cleared, then either the assert mask or the deassert >> mask is set in a register to affect the state of a reset control. >> >> Signed-off-by: Alex Elder <elder@riscstar.com> >> --- >> drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 93 insertions(+) >> >> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c >> index f7367271396a0..6d879411c6c05 100644 >> --- a/drivers/clk/spacemit/ccu-k1.c >> +++ b/drivers/clk/spacemit/ccu-k1.c > > ... > >> +static int >> +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert) >> +{ >> + struct ccu_reset_controller *controller = rcdev_to_controller(rcdev); >> + struct regmap *regmap = controller->regmap; >> + const struct ccu_reset_data *data; >> + u32 val; >> + int ret; >> + >> + data = &controller->data->data[id]; >> + >> + ret = regmap_read(regmap, data->offset, &val); >> + if (ret) >> + return ret; >> + >> + val &= ~(data->assert_mask | data->deassert_mask); >> + val |= assert ? data->assert_mask : data->deassert_mask; >> + >> + return regmap_write(regmap, data->offset, val); >> +} > > I don't think it's safe to write the regmap based on a value read > earlier without the regmap's inner lock held: it's totally fine for the > clock part to issue an update of the register at the same time. Without > knowledge on it, reset code may rollback the clock bits written by clock > code earlier to the original value. That's why I keep using ccu_update() > everywhere and dropped ccu_write(). That's a great point, thank you. I'll modify it to use regmap_update_bits(), which is better anyway. -Alex > > Thanks, > Haylen Chu
diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c index f7367271396a0..6d879411c6c05 100644 --- a/drivers/clk/spacemit/ccu-k1.c +++ b/drivers/clk/spacemit/ccu-k1.c @@ -10,6 +10,7 @@ #include <linux/minmax.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/reset-controller.h> #include "ccu_common.h" #include "ccu_pll.h" @@ -134,8 +135,26 @@ struct spacemit_ccu_clk { struct clk_hw *hw; }; +struct ccu_reset_data { + u32 offset; + u32 assert_mask; + u32 deassert_mask; +}; + +struct ccu_reset_controller_data { + u32 count; + const struct ccu_reset_data *data; /* array */ +}; + struct k1_ccu_data { struct spacemit_ccu_clk *clk; /* array with sentinel */ + const struct ccu_reset_controller_data *rst_data; +}; + +struct ccu_reset_controller { + struct regmap *regmap; + const struct ccu_reset_controller_data *data; + struct reset_controller_dev rcdev; }; /* APBS clocks start */ @@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = { .clk = k1_ccu_apmu_clks, }; +static struct ccu_reset_controller * +rcdev_to_controller(struct reset_controller_dev *rcdev) +{ + return container_of(rcdev, struct ccu_reset_controller, rcdev); +} + +static int +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert) +{ + struct ccu_reset_controller *controller = rcdev_to_controller(rcdev); + struct regmap *regmap = controller->regmap; + const struct ccu_reset_data *data; + u32 val; + int ret; + + data = &controller->data->data[id]; + + ret = regmap_read(regmap, data->offset, &val); + if (ret) + return ret; + + val &= ~(data->assert_mask | data->deassert_mask); + val |= assert ? data->assert_mask : data->deassert_mask; + + return regmap_write(regmap, data->offset, val); +} + +static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id) +{ + return k1_rst_update(rcdev, id, true); +} + +static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id) +{ + return k1_rst_update(rcdev, id, false); +} + +static const struct reset_control_ops k1_reset_control_ops = { + .assert = k1_rst_assert, + .deassert = k1_rst_deassert, +}; + static int k1_ccu_register(struct device *dev, struct regmap *regmap, struct regmap *lock_regmap, struct spacemit_ccu_clk *clks) @@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap, return ret; } +static int +k1_reset_controller_register(struct device *dev, struct regmap *regmap, + const struct ccu_reset_controller_data *data) +{ + struct ccu_reset_controller *controller; + struct reset_controller_dev *rcdev; + + /* Resets are optional */ + if (!data) + return 0; + + controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); + if (!controller) + return -ENOMEM; + + controller->regmap = regmap; + controller->data = data; + + rcdev = &controller->rcdev; + rcdev->owner = THIS_MODULE; + rcdev->nr_resets = data->count; + rcdev->ops = &k1_reset_control_ops; + rcdev->of_node = dev->of_node; + + return devm_reset_controller_register(dev, rcdev); +} + static int k1_ccu_probe(struct platform_device *pdev) { struct regmap *base_regmap, *lock_regmap = NULL; @@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev) if (ret) return dev_err_probe(dev, ret, "failed to register clocks\n"); + ret = k1_reset_controller_register(dev, base_regmap, data->rst_data); + if (ret) + return dev_err_probe(dev, ret, + "failed to register reset controller\n"); + return 0; }
Define ccu_reset_data as a structure that contains the constant register offset and bitmasks used to assert and deassert a reset control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as a structure that contains the address of an array of those structures and a count of the number of elements in the array. Add a pointer to a ccu_reset_controller_data structure to the k1_ccu_data structure. Reset support is optional for SpacemiT CCUs; the new pointer field will be null for CCUs without any resets. Finally, define a new ccu_reset_controller structure, which (for a CCU with resets) contains a pointer to the constant reset data, the regmap to be used for the controller, and an embedded a reset controller structure. Each reset control is asserted or deasserted by updating bits in a register. The bits used are defined by an assert mask and a deassert mask. In some cases, one (non-zero) mask asserts reset and a different (non-zero) mask deasserts it. Otherwise one mask is nonzero, and the other is zero. Either way, the bits in both masks are cleared, then either the assert mask or the deassert mask is set in a register to affect the state of a reset control. Signed-off-by: Alex Elder <elder@riscstar.com> --- drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)