Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 于2019年12月3日周二 下午10:15写道： > > Hi Jun, > > On Tue, 2019-12-03 at 11:53 +0800, Jun Nie wrote: > [...] > > @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev, > > > > u32 offset, reg; > > > > u8 bit; > > > > > > > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT; > > > > + if (flags & HISI_ASSERT_NONE) > > > > + return -ENOTSUPP; /* assert not supported for this reset */ > > > > > > As long as .assert and .deassert are the only implemented operations for > > > this reset controller, this does not make any sense to me. Are there > > > resets that can only be deasserted? > > > > Some reset is asserted on power on automatically. So only .deassert is needed. > > But if the bit was set/cleared again after being deasserted, would it > assert the reset line? Basically, I wonder if those bits are write-once > or not. I did not see vendor's code to clear it, seems the bit is only polled to be set on every device enablement. > > [...] > > > > + if (flags & HISI_ASSERT_SET) > > > > + return readl_poll_timeout(rstc->membase + offset, > > > > + reg, reg & BIT(bit), 0, 5000); > > > > + else > > > > + return readl_poll_timeout(rstc->membase + offset, > > > > + reg, !(reg & BIT(bit)), > > > > + 0, 5000); > > > > > > And this is effectively dead code. Do you really have hardware that > > > asserts or asserts a reset line in reaction to a read access? > > > > > > Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive? > > This is missing an explanation. They are not mutually exclusive in logic. But I did not see the need to poll for a assert yet. > > [...] > > > > + writel(reg, rstc->membase + offset); > > > > > > > > spin_unlock_irqrestore(&rstc->lock, flags); > > > > > > > > @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev, > > > > u32 offset, reg; > > > > u8 bit; > > > > > > > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT; > > > > + if (flags & HISI_DEASSERT_NONE) > > > > + return -ENOTSUPP; /* deassert not supported for this reset */ > > > > + > > > > > > Are there resets that can only ever be asserted? > > > > I do not know yet. Only extend this driver with all combination in logic. > > If this is not used, let's leave it out. > > [...] > > > > @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev) > > > > rstc->rcdev.owner = THIS_MODULE; > > > > rstc->rcdev.ops = &hisi_reset_ops; > > > > rstc->rcdev.of_node = pdev->dev.of_node; > > > > - rstc->rcdev.of_reset_n_cells = 2; > > > > + rstc->rcdev.of_reset_n_cells = 3; > > > > > > This breaks current device trees (before patch 3). You can make sure > > > device trees with #reset-cells = <2> keep working by parsing the #reset- > > > cells and setting this value to 2 for old DTs. > > > > All current dts file are modified accordingly. But existing dtb binary support > > is an issue. Do you have any suggestion? > > Since this is for a new SoC, you should keep using of_reset_n_cells = 2 > for the current SoCs and only set it to 3 for a new compatible, for > example using of_device_get_match_data(). Yeah, this should be the solution. I will check the detail, thanks! Regards, Jun
