| Message ID | 20211006061204.2854-4-sergio.paracuellos@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: ralink: make system controller a reset provider | expand |
On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote: > @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node) > } > CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init); > > +struct mt7621_rst { > + struct reset_controller_dev rcdev; > + struct regmap *sysc; > +}; > + > +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev) No need to mark this as inline. The compiler should do it automatically or it will ignore the inline. > +{ > + return container_of(dev, struct mt7621_rst, rcdev); > +} > + > +static int mt7621_assert_device(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct mt7621_rst *data = to_mt7621_rst(rcdev); > + struct regmap *sysc = data->sysc; > + > + if (id == MT7621_RST_SYS) > + return -1; Please, return proper error codes. > + > + return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), BIT(id)); > +} > + > +static int mt7621_deassert_device(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct mt7621_rst *data = to_mt7621_rst(rcdev); > + struct regmap *sysc = data->sysc; > + > + if (id == MT7621_RST_SYS) > + return -1; Here too. > + > + return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), 0); > +} > + > +static int mt7621_reset_device(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + int ret; > + > + ret = mt7621_assert_device(rcdev, id); > + if (ret < 0) > + return ret; > + > + return mt7621_deassert_device(rcdev, id); > +} > + > +static const struct reset_control_ops reset_ops = { > + .reset = mt7621_reset_device, > + .assert = mt7621_assert_device, > + .deassert = mt7621_deassert_device > +}; > + > +static int mt7621_reset_init(struct device *dev, struct regmap *sysc) > +{ > + struct mt7621_rst *rst_data; > + > + rst_data = kzalloc(sizeof(*rst_data), GFP_KERNEL); Can we use devm_ to allocate this or do we need to clean up if devm_reset_controller_register() fails? Also a free in the release function I suppose. (Please, use devm_). > + if (!rst_data) > + return -ENOMEM; > + > + rst_data->sysc = sysc; > + rst_data->rcdev.ops = &reset_ops; > + rst_data->rcdev.owner = THIS_MODULE; > + rst_data->rcdev.nr_resets = 32; > + rst_data->rcdev.of_reset_n_cells = 1; > + rst_data->rcdev.of_node = dev_of_node(dev); > + > + return devm_reset_controller_register(dev, &rst_data->rcdev); > +} regards, dan carpenter
Hi Dan, Thanks for the review. Comments below. On Wed, Oct 6, 2021 at 10:29 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote: > > @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node) > > } > > CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init); > > > > +struct mt7621_rst { > > + struct reset_controller_dev rcdev; > > + struct regmap *sysc; > > +}; > > + > > +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev) > > No need to mark this as inline. The compiler should do it automatically > or it will ignore the inline. Ok, I have other functions to_* with the same inline syntax, that's why I have added also here. I think I will maintain it to be coherent and can be removed afterwards with another patch not belonging to this series. > > > +{ > > + return container_of(dev, struct mt7621_rst, rcdev); > > +} > > + > > +static int mt7621_assert_device(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + struct mt7621_rst *data = to_mt7621_rst(rcdev); > > + struct regmap *sysc = data->sysc; > > + > > + if (id == MT7621_RST_SYS) > > + return -1; > > Please, return proper error codes. Current code at 'reset.c' of the arch was returning -1 in this case. I guess it is better to change it to -EINVAL. > > > + > > + return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), BIT(id)); > > +} > > + > > +static int mt7621_deassert_device(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + struct mt7621_rst *data = to_mt7621_rst(rcdev); > > + struct regmap *sysc = data->sysc; > > + > > + if (id == MT7621_RST_SYS) > > + return -1; > > Here too. Will change to -EINVAL. > > > + > > + return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), 0); > > +} > > + > > +static int mt7621_reset_device(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + int ret; > > + > > + ret = mt7621_assert_device(rcdev, id); > > + if (ret < 0) > > + return ret; > > + > > + return mt7621_deassert_device(rcdev, id); > > +} > > + > > +static const struct reset_control_ops reset_ops = { > > + .reset = mt7621_reset_device, > > + .assert = mt7621_assert_device, > > + .deassert = mt7621_deassert_device > > +}; > > + > > +static int mt7621_reset_init(struct device *dev, struct regmap *sysc) > > +{ > > + struct mt7621_rst *rst_data; > > + > > + rst_data = kzalloc(sizeof(*rst_data), GFP_KERNEL); > > > Can we use devm_ to allocate this or do we need to clean up if > devm_reset_controller_register() fails? Also a free in the release > function I suppose. (Please, use devm_). True, yes we can use devm_ for this. Will change it, thanks. > > > > + if (!rst_data) > > + return -ENOMEM; > > + > > + rst_data->sysc = sysc; > > + rst_data->rcdev.ops = &reset_ops; > > + rst_data->rcdev.owner = THIS_MODULE; > > + rst_data->rcdev.nr_resets = 32; > > + rst_data->rcdev.of_reset_n_cells = 1; > > + rst_data->rcdev.of_node = dev_of_node(dev); > > + > > + return devm_reset_controller_register(dev, &rst_data->rcdev); > > +} > > > regards, > dan carpenter > I will properly take into account your comments and send v2. Thanks, Sergio Paracuellos
On Wed, Oct 06, 2021 at 12:02:12PM +0200, Sergio Paracuellos wrote: > Hi Dan, > > Thanks for the review. Comments below. > > On Wed, Oct 6, 2021 at 10:29 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote: > > > @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node) > > > } > > > CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init); > > > > > > +struct mt7621_rst { > > > + struct reset_controller_dev rcdev; > > > + struct regmap *sysc; > > > +}; > > > + > > > +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev) > > > > No need to mark this as inline. The compiler should do it automatically > > or it will ignore the inline. > > Ok, I have other functions to_* with the same inline syntax, that's > why I have added also here. I think I will maintain it to be coherent > and can be removed afterwards with another patch not belonging to this > series. Consistency is never an important goal. It's better to be different than to be wrong. regards, dan carpenter
On Wed, Oct 6, 2021 at 12:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Oct 06, 2021 at 12:02:12PM +0200, Sergio Paracuellos wrote: > > Hi Dan, > > > > Thanks for the review. Comments below. > > > > On Wed, Oct 6, 2021 at 10:29 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > > > On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote: > > > > @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node) > > > > } > > > > CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init); > > > > > > > > +struct mt7621_rst { > > > > + struct reset_controller_dev rcdev; > > > > + struct regmap *sysc; > > > > +}; > > > > + > > > > +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev) > > > > > > No need to mark this as inline. The compiler should do it automatically > > > or it will ignore the inline. > > > > Ok, I have other functions to_* with the same inline syntax, that's > > why I have added also here. I think I will maintain it to be coherent > > and can be removed afterwards with another patch not belonging to this > > series. > > Consistency is never an important goal. It's better to be different > than to be wrong. Pretty clear, thanks. Will change this also, then. > > regards, > dan carpenter > Best regards, Sergio Paracuellos
diff --git a/drivers/clk/ralink/clk-mt7621.c b/drivers/clk/ralink/clk-mt7621.c index a2c045390f00..67ccc9582c46 100644 --- a/drivers/clk/ralink/clk-mt7621.c +++ b/drivers/clk/ralink/clk-mt7621.c @@ -11,14 +11,17 @@ #include <linux/mfd/syscon.h> #include <linux/platform_device.h> #include <linux/regmap.h> +#include <linux/reset-controller.h> #include <linux/slab.h> #include <dt-bindings/clock/mt7621-clk.h> +#include <dt-bindings/reset/mt7621-reset.h> /* Configuration registers */ #define SYSC_REG_SYSTEM_CONFIG0 0x10 #define SYSC_REG_SYSTEM_CONFIG1 0x14 #define SYSC_REG_CLKCFG0 0x2c #define SYSC_REG_CLKCFG1 0x30 +#define SYSC_REG_RESET_CTRL 0x34 #define SYSC_REG_CUR_CLK_STS 0x44 #define MEMC_REG_CPU_PLL 0x648 @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node) } CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init); +struct mt7621_rst { + struct reset_controller_dev rcdev; + struct regmap *sysc; +}; + +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev) +{ + return container_of(dev, struct mt7621_rst, rcdev); +} + +static int mt7621_assert_device(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct mt7621_rst *data = to_mt7621_rst(rcdev); + struct regmap *sysc = data->sysc; + + if (id == MT7621_RST_SYS) + return -1; + + return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), BIT(id)); +} + +static int mt7621_deassert_device(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct mt7621_rst *data = to_mt7621_rst(rcdev); + struct regmap *sysc = data->sysc; + + if (id == MT7621_RST_SYS) + return -1; + + return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), 0); +} + +static int mt7621_reset_device(struct reset_controller_dev *rcdev, + unsigned long id) +{ + int ret; + + ret = mt7621_assert_device(rcdev, id); + if (ret < 0) + return ret; + + return mt7621_deassert_device(rcdev, id); +} + +static const struct reset_control_ops reset_ops = { + .reset = mt7621_reset_device, + .assert = mt7621_assert_device, + .deassert = mt7621_deassert_device +}; + +static int mt7621_reset_init(struct device *dev, struct regmap *sysc) +{ + struct mt7621_rst *rst_data; + + rst_data = kzalloc(sizeof(*rst_data), GFP_KERNEL); + if (!rst_data) + return -ENOMEM; + + rst_data->sysc = sysc; + rst_data->rcdev.ops = &reset_ops; + rst_data->rcdev.owner = THIS_MODULE; + rst_data->rcdev.nr_resets = 32; + rst_data->rcdev.of_reset_n_cells = 1; + rst_data->rcdev.of_node = dev_of_node(dev); + + return devm_reset_controller_register(dev, &rst_data->rcdev); +} + static int mt7621_clk_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -424,6 +497,12 @@ static int mt7621_clk_probe(struct platform_device *pdev) return ret; } + ret = mt7621_reset_init(dev, priv->sysc); + if (ret) { + pr_err("Could not init reset controller\n"); + return ret; + } + count = ARRAY_SIZE(mt7621_clks_base) + ARRAY_SIZE(mt7621_fixed_clks) + ARRAY_SIZE(mt7621_gates); clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, count),
MT7621 system controller node is already providing the clocks for the whole system but must also serve as a reset provider. Hence, add reset controller related code to the clock driver itself. Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> --- drivers/clk/ralink/clk-mt7621.c | 79 +++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)