| Message ID | 20220110083910.1351020-4-sergio.paracuellos@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: ralink: make system controller a reset provider | expand |
Hi Sergio, On Mon, 2022-01-10 at 09:39 +0100, Sergio Paracuellos wrote: > +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 -EINVAL; Better implement the .of_xlate callback and check there instead. That way it would fail on reset_control_get() rather than handing out a valid reset controller that just doesn't work. > + > + 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 -EINVAL; Same as above. > + 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); > +} Is this known to work for all possible users, without delay between assert and deassert? Are there any users of the reset_control_reset() API at all? This API was added for self-clearing reset bits, so if there are no users that need to pretend this is a reset pulse at the hardware level (there may be), I'd prefer if this was just left out. Apart from this, this looks good to me. regards Philipp
Hi Philipp, Thanks for the review. On Mon, Jan 10, 2022 at 10:13 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Sergio, > > On Mon, 2022-01-10 at 09:39 +0100, Sergio Paracuellos wrote: > > +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 -EINVAL; > > Better implement the .of_xlate callback and check there instead. > > That way it would fail on reset_control_get() rather than handing out a > valid reset controller that just doesn't work. Pretty clear, thanks. Will change this check into '.of_xlate' callback and remove it from here. > > > + > > + 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 -EINVAL; > > Same as above. Ditto. > > > + 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); > > +} > > Is this known to work for all possible users, without delay between > assert and deassert? It seems it is. This is based on the original reset implementation for ralink. See: https://elixir.bootlin.com/linux/v5.16/source/arch/mips/ralink/reset.c#L55 > > Are there any users of the reset_control_reset() API at all? This API > was added for self-clearing reset bits, so if there are no users that > need to pretend this is a reset pulse at the hardware level (there may > be), I'd prefer if this was just left out. I am not following you here. What do you mean? > > Apart from this, this looks good to me. Thanks, I will include of_xlate callback changes and send v7. > > regards > Philipp Best regards, Sergio Paracuellos
On Mon, 2022-01-10 at 10:39 +0100, Sergio Paracuellos wrote: [...] > > Is this known to work for all possible users, without delay between > > assert and deassert? > > It seems it is. This is based on the original reset implementation for > ralink. See: > > https://elixir.bootlin.com/linux/v5.16/source/arch/mips/ralink/reset.c#L55 > > > Are there any users of the reset_control_reset() API at all? This API > > was added for self-clearing reset bits, so if there are no users that > > need to pretend this is a reset pulse at the hardware level (there may > > be), I'd prefer if this was just left out. > > I am not following you here. What do you mean? Looking at drivers/staging/mt7621-dts/mt7621.dtsi, it appears the current reset users are: mediatek,mt7621-i2c ralink,mt7621-spi ralink,rt3883-gdma mediatek,mt7621-hsdma mediatek,mt7621-eth mediatek,mt7621 mediatek,mt7621-pci Many of the corresponding drivers use device_reset(), which is a shorthand for requesting a reset control and calling reset_control_reset() on it. That will call mt7621_reset_device, so it is indeed used. regards Philipp
On Mon, Jan 10, 2022 at 11:23 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Mon, 2022-01-10 at 10:39 +0100, Sergio Paracuellos wrote: > [...] > > > Is this known to work for all possible users, without delay between > > > assert and deassert? > > > > It seems it is. This is based on the original reset implementation for > > ralink. See: > > > > https://elixir.bootlin.com/linux/v5.16/source/arch/mips/ralink/reset.c#L55 > > > > > Are there any users of the reset_control_reset() API at all? This API > > > was added for self-clearing reset bits, so if there are no users that > > > need to pretend this is a reset pulse at the hardware level (there may > > > be), I'd prefer if this was just left out. > > > > I am not following you here. What do you mean? > > Looking at drivers/staging/mt7621-dts/mt7621.dtsi, it appears the > current reset users are: > mediatek,mt7621-i2c > ralink,mt7621-spi > ralink,rt3883-gdma > mediatek,mt7621-hsdma > mediatek,mt7621-eth > mediatek,mt7621 > mediatek,mt7621-pci > > Many of the corresponding drivers use device_reset(), which is a > shorthand for requesting a reset control and calling > reset_control_reset() on it. That will call mt7621_reset_device, so it > is indeed used. > Ok, pretty clear now, thanks for clarification :). > regards > Philipp Best regards, Sergio Paracuellos
diff --git a/drivers/clk/ralink/clk-mt7621.c b/drivers/clk/ralink/clk-mt7621.c index a2c045390f00..c725bf6e6e07 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 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 -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 -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 = devm_kzalloc(dev, 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) { + dev_err(dev, "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), @@ -485,4 +564,9 @@ static struct platform_driver mt7621_clk_driver = { .of_match_table = mt7621_clk_of_match, }, }; -builtin_platform_driver(mt7621_clk_driver); + +static int __init mt7621_clk_reset_init(void) +{ + return platform_driver_register(&mt7621_clk_driver); +} +arch_initcall(mt7621_clk_reset_init);
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. To get resets properly ready for the rest of the world we need to move platform driver initialization process to 'arch_initcall'. CC: Philipp Zabel <p.zabel@pengutronix.de> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> --- drivers/clk/ralink/clk-mt7621.c | 86 ++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-)