* [PATCH 0/2] Handle I2C and SPI reset on HI6220 SoC @ 2019-03-09 1:56 Manivannan Sadhasivam 2019-03-09 1:56 ` [PATCH 1/2] amba: Take device out of reset before reading pid and cid values Manivannan Sadhasivam 2019-03-09 1:56 ` [PATCH 2/2] arm64: dts: hisilicon: Add reset properties for HI6220 I2C and SPI Manivannan Sadhasivam 0 siblings, 2 replies; 10+ messages in thread From: Manivannan Sadhasivam @ 2019-03-09 1:56 UTC (permalink / raw) To: linux, xuwei5 Cc: linux-arm-kernel, linux-kernel, linus.walleij, daniel.thompson, peter.griffin, guodong.xu, haojian.zhuang, Manivannan Sadhasivam Hello, This small patchset adds the reset functionality to the I2C and SPI peripherals on HI6220 SoC from HiSilicon. Those peripherals needs to be taken out of reset before being used. But earlier we were depending on the bootloader to do the job but as suggested by Daniel Thompson, a more cleaner approach would be to handle the reset in corresponding drivers. Hence, one of the patch adds reset properties to the I2C and SPI nodes and the other one adds missing reset functionality to the AMBA Primecell bus driver. Because the AMBA devices are being accessed before the driver probe (reading pid and cid values), we need to deassert the reset line in the bus driver itself just before the read. I'd expect the patch 1 to go via arm tree and patch 2 via hisi tree. Thanks, Mani Manivannan Sadhasivam (2): amba: Take device out of reset before reading pid and cid values arm64: dts: hisilicon: Add reset properties for HI6220 I2C and SPI arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 8 ++++++++ drivers/amba/bus.c | 9 +++++++++ 2 files changed, 17 insertions(+) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-09 1:56 [PATCH 0/2] Handle I2C and SPI reset on HI6220 SoC Manivannan Sadhasivam @ 2019-03-09 1:56 ` Manivannan Sadhasivam 2019-03-09 10:50 ` Linus Walleij 2019-03-12 12:27 ` Daniel Thompson 2019-03-09 1:56 ` [PATCH 2/2] arm64: dts: hisilicon: Add reset properties for HI6220 I2C and SPI Manivannan Sadhasivam 1 sibling, 2 replies; 10+ messages in thread From: Manivannan Sadhasivam @ 2019-03-09 1:56 UTC (permalink / raw) To: linux, xuwei5 Cc: linux-arm-kernel, linux-kernel, linus.walleij, daniel.thompson, peter.griffin, guodong.xu, haojian.zhuang, Manivannan Sadhasivam For the AMBA Primecell devices having the reset lines wired, it is necessary to take them out of reset before reading the pid and cid values. Earlier we were dependent on the bootloader to do this but a more cleaner approach would be to do it in the kernel itself. Hence, this commit deasserts the reset line just before reading the pid and cid values. Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/amba/bus.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index 41b706403ef7..da8f1aac5315 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -21,6 +21,7 @@ #include <linux/limits.h> #include <linux/clk/clk-conf.h> #include <linux/platform_device.h> +#include <linux/reset.h> #include <asm/irq.h> @@ -352,6 +353,7 @@ static void amba_device_release(struct device *dev) static int amba_device_try_add(struct amba_device *dev, struct resource *parent) { + struct reset_control *rst; u32 size; void __iomem *tmp; int i, ret; @@ -388,6 +390,13 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent) if (ret == 0) { u32 pid, cid; + /* De-assert the reset line to take the device out of reset */ + rst = reset_control_get_optional_exclusive(&dev->dev, NULL); + if (IS_ERR(rst)) + return PTR_ERR(rst); + + reset_control_deassert(rst); + /* * Read pid and cid based on size of resource * they are located at end of region -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-09 1:56 ` [PATCH 1/2] amba: Take device out of reset before reading pid and cid values Manivannan Sadhasivam @ 2019-03-09 10:50 ` Linus Walleij 2019-03-12 12:27 ` Daniel Thompson 1 sibling, 0 replies; 10+ messages in thread From: Linus Walleij @ 2019-03-09 10:50 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Russell King, Xu Wei, Linux ARM, linux-kernel, Daniel Thompson, Peter Griffin, Guodong Xu, Haojian Zhuang On Sat, Mar 9, 2019 at 2:59 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > For the AMBA Primecell devices having the reset lines wired, it is > necessary to take them out of reset before reading the pid and cid values. > Earlier we were dependent on the bootloader to do this but a more cleaner > approach would be to do it in the kernel itself. Hence, this commit > deasserts the reset line just before reading the pid and cid values. > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> This looks correct to me. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I think you can put it in Russell's patch tracker in some week. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-09 1:56 ` [PATCH 1/2] amba: Take device out of reset before reading pid and cid values Manivannan Sadhasivam 2019-03-09 10:50 ` Linus Walleij @ 2019-03-12 12:27 ` Daniel Thompson 2019-03-20 6:56 ` Manivannan Sadhasivam 1 sibling, 1 reply; 10+ messages in thread From: Daniel Thompson @ 2019-03-12 12:27 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: linux, xuwei5, linux-arm-kernel, linux-kernel, linus.walleij, peter.griffin, guodong.xu, haojian.zhuang On Sat, Mar 09, 2019 at 07:26:34AM +0530, Manivannan Sadhasivam wrote: > For the AMBA Primecell devices having the reset lines wired, it is > necessary to take them out of reset before reading the pid and cid values. > Earlier we were dependent on the bootloader to do this but a more cleaner > approach would be to do it in the kernel itself. Hence, this commit > deasserts the reset line just before reading the pid and cid values. > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/amba/bus.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > index 41b706403ef7..da8f1aac5315 100644 > --- a/drivers/amba/bus.c > +++ b/drivers/amba/bus.c > @@ -21,6 +21,7 @@ > #include <linux/limits.h> > #include <linux/clk/clk-conf.h> > #include <linux/platform_device.h> > +#include <linux/reset.h> > > #include <asm/irq.h> > > @@ -352,6 +353,7 @@ static void amba_device_release(struct device *dev) > > static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > { > + struct reset_control *rst; > u32 size; > void __iomem *tmp; > int i, ret; > @@ -388,6 +390,13 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > if (ret == 0) { > u32 pid, cid; > > + /* De-assert the reset line to take the device out of reset */ > + rst = reset_control_get_optional_exclusive(&dev->dev, NULL); > + if (IS_ERR(rst)) > + return PTR_ERR(rst); It is really correct to propagate an error if we cannot get exclusive ownership of the reset line. With drivers for vendor specific cells it is ok to "just know" that the reset line is never shared but we cannot know this for generic cells and we certainly can't know this for the bus. I think it *might* be OK to propagate an error if you used reset_control_get_optional_shared() instead because if that reports an error than arguably we have either a mistake in the DT or a bug in the driver we are sharing a reset with. > + > + reset_control_deassert(rst); Perhaps we might also need to explain why we can ignore -ENOTSUPP here. Perhaps something like the following based on the comment found in in reset_control_deassert(): /* * -ENOTSUPP means occurs when the reset controller * does not implement .deassert(), in which case the * the reset lines should be self-deasserting (and * deasserted by default). */ WARN_ON(deassert did not return 0 or -ENOTSUPP); > + > /* > * Read pid and cid based on size of resource > * they are located at end of region This looks like it will leak the control reference... shouldn't there be a put after you have read the pid/cid? Daniel. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-12 12:27 ` Daniel Thompson @ 2019-03-20 6:56 ` Manivannan Sadhasivam 2019-03-20 14:29 ` Daniel Thompson 2019-03-20 17:29 ` Russell King - ARM Linux admin 0 siblings, 2 replies; 10+ messages in thread From: Manivannan Sadhasivam @ 2019-03-20 6:56 UTC (permalink / raw) To: Daniel Thompson Cc: linux, xuwei5, linux-arm-kernel, linux-kernel, linus.walleij, peter.griffin, guodong.xu, haojian.zhuang, rmk+kernel Hi Daniel, On Tue, Mar 12, 2019 at 12:27:11PM +0000, Daniel Thompson wrote: > On Sat, Mar 09, 2019 at 07:26:34AM +0530, Manivannan Sadhasivam wrote: > > For the AMBA Primecell devices having the reset lines wired, it is > > necessary to take them out of reset before reading the pid and cid values. > > Earlier we were dependent on the bootloader to do this but a more cleaner > > approach would be to do it in the kernel itself. Hence, this commit > > deasserts the reset line just before reading the pid and cid values. > > > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/amba/bus.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > > index 41b706403ef7..da8f1aac5315 100644 > > --- a/drivers/amba/bus.c > > +++ b/drivers/amba/bus.c > > @@ -21,6 +21,7 @@ > > #include <linux/limits.h> > > #include <linux/clk/clk-conf.h> > > #include <linux/platform_device.h> > > +#include <linux/reset.h> > > > > #include <asm/irq.h> > > > > @@ -352,6 +353,7 @@ static void amba_device_release(struct device *dev) > > > > static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > { > > + struct reset_control *rst; > > u32 size; > > void __iomem *tmp; > > int i, ret; > > @@ -388,6 +390,13 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > if (ret == 0) { > > u32 pid, cid; > > > > + /* De-assert the reset line to take the device out of reset */ > > + rst = reset_control_get_optional_exclusive(&dev->dev, NULL); > > + if (IS_ERR(rst)) > > + return PTR_ERR(rst); > > It is really correct to propagate an error if we cannot get exclusive > ownership of the reset line. > > With drivers for vendor specific cells it is ok to "just know" that the > reset line is never shared but we cannot know this for generic cells and > we certainly can't know this for the bus. > > I think it *might* be OK to propagate an error if you used > reset_control_get_optional_shared() instead because if that reports an > error than arguably we have either a mistake in the DT or a bug in the > driver we are sharing a reset with. > Hmm. I'm not sure whether we can assume shared reset lines here or not! Maybe Russell can share his opinion here. > > > + > > + reset_control_deassert(rst); > > Perhaps we might also need to explain why we can ignore -ENOTSUPP > here. Perhaps something like the following based on the comment > found in in reset_control_deassert(): > > /* > * -ENOTSUPP means occurs when the reset controller > * does not implement .deassert(), in which case the > * the reset lines should be self-deasserting (and > * deasserted by default). > */ > WARN_ON(deassert did not return 0 or -ENOTSUPP); Ack. > > + > > /* > > * Read pid and cid based on size of resource > > * they are located at end of region > > This looks like it will leak the control reference... shouldn't there > be a put after you have read the pid/cid? > Yes, but it can come before this as well. Right after deassert. Thanks, Mani > > Daniel. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-20 6:56 ` Manivannan Sadhasivam @ 2019-03-20 14:29 ` Daniel Thompson 2019-03-20 17:29 ` Russell King - ARM Linux admin 1 sibling, 0 replies; 10+ messages in thread From: Daniel Thompson @ 2019-03-20 14:29 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: linux, xuwei5, linux-arm-kernel, linux-kernel, linus.walleij, peter.griffin, guodong.xu, haojian.zhuang, rmk+kernel On Wed, Mar 20, 2019 at 12:26:58PM +0530, Manivannan Sadhasivam wrote: > Hi Daniel, > > On Tue, Mar 12, 2019 at 12:27:11PM +0000, Daniel Thompson wrote: > > On Sat, Mar 09, 2019 at 07:26:34AM +0530, Manivannan Sadhasivam wrote: > > > For the AMBA Primecell devices having the reset lines wired, it is > > > necessary to take them out of reset before reading the pid and cid values. > > > Earlier we were dependent on the bootloader to do this but a more cleaner > > > approach would be to do it in the kernel itself. Hence, this commit > > > deasserts the reset line just before reading the pid and cid values. > > > > > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/amba/bus.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > > > index 41b706403ef7..da8f1aac5315 100644 > > > --- a/drivers/amba/bus.c > > > +++ b/drivers/amba/bus.c > > > @@ -21,6 +21,7 @@ > > > #include <linux/limits.h> > > > #include <linux/clk/clk-conf.h> > > > #include <linux/platform_device.h> > > > +#include <linux/reset.h> > > > > > > #include <asm/irq.h> > > > > > > @@ -352,6 +353,7 @@ static void amba_device_release(struct device *dev) > > > > > > static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > > { > > > + struct reset_control *rst; > > > u32 size; > > > void __iomem *tmp; > > > int i, ret; > > > @@ -388,6 +390,13 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > > if (ret == 0) { > > > u32 pid, cid; > > > > > > + /* De-assert the reset line to take the device out of reset */ > > > + rst = reset_control_get_optional_exclusive(&dev->dev, NULL); > > > + if (IS_ERR(rst)) > > > + return PTR_ERR(rst); > > > > It is really correct to propagate an error if we cannot get exclusive > > ownership of the reset line. > > > > With drivers for vendor specific cells it is ok to "just know" that the > > reset line is never shared but we cannot know this for generic cells and > > we certainly can't know this for the bus. > > > > I think it *might* be OK to propagate an error if you used > > reset_control_get_optional_shared() instead because if that reports an > > error than arguably we have either a mistake in the DT or a bug in the > > driver we are sharing a reset with. > > > > Hmm. I'm not sure whether we can assume shared reset lines here or not! Maybe > Russell can share his opinion here. I didn't *assume* anything. I read about the difference between what a shared reset line does and an exclusive reset line. Providing the get/put is properly balanced it is both safe and correct to request shared access to the reset line. The question is whether it is OK to propagate an error code if another device has already requested exclusive access to the reset line (which would cause our request for shared access to fail) since this causes us to give up on adding the device. Here I concluded that it OK because a failure does indicates a bug in DT or other driver but added the "I think" because just-WARN_ON()-and-carry-on" is also a defensible recovery approach (and *much* less likely to provoke boot sequence regressions for existing systems). > > > + > > > + reset_control_deassert(rst); > > > > Perhaps we might also need to explain why we can ignore -ENOTSUPP > > here. Perhaps something like the following based on the comment > > found in in reset_control_deassert(): > > > > /* > > * -ENOTSUPP means occurs when the reset controller > > * does not implement .deassert(), in which case the > > * the reset lines should be self-deasserting (and > > * deasserted by default). > > */ > > WARN_ON(deassert did not return 0 or -ENOTSUPP); > > Ack. > > > > + > > > /* > > > * Read pid and cid based on size of resource > > > * they are located at end of region > > > > This looks like it will leak the control reference... shouldn't there > > be a put after you have read the pid/cid? > > > > Yes, but it can come before this as well. Right after deassert. No it can't. If the reset line really really is shared then another driver could but the cell back into the reset state before you have read the pid/cid. Daniel. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-20 6:56 ` Manivannan Sadhasivam 2019-03-20 14:29 ` Daniel Thompson @ 2019-03-20 17:29 ` Russell King - ARM Linux admin 2019-03-21 11:17 ` Daniel Thompson 1 sibling, 1 reply; 10+ messages in thread From: Russell King - ARM Linux admin @ 2019-03-20 17:29 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Daniel Thompson, xuwei5, linux-arm-kernel, linux-kernel, linus.walleij, peter.griffin, guodong.xu, haojian.zhuang On Wed, Mar 20, 2019 at 12:26:58PM +0530, Manivannan Sadhasivam wrote: > Hi Daniel, > > On Tue, Mar 12, 2019 at 12:27:11PM +0000, Daniel Thompson wrote: > > On Sat, Mar 09, 2019 at 07:26:34AM +0530, Manivannan Sadhasivam wrote: > > > For the AMBA Primecell devices having the reset lines wired, it is > > > necessary to take them out of reset before reading the pid and cid values. > > > Earlier we were dependent on the bootloader to do this but a more cleaner > > > approach would be to do it in the kernel itself. Hence, this commit > > > deasserts the reset line just before reading the pid and cid values. > > > > > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/amba/bus.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > > > index 41b706403ef7..da8f1aac5315 100644 > > > --- a/drivers/amba/bus.c > > > +++ b/drivers/amba/bus.c > > > @@ -21,6 +21,7 @@ > > > #include <linux/limits.h> > > > #include <linux/clk/clk-conf.h> > > > #include <linux/platform_device.h> > > > +#include <linux/reset.h> > > > > > > #include <asm/irq.h> > > > > > > @@ -352,6 +353,7 @@ static void amba_device_release(struct device *dev) > > > > > > static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > > { > > > + struct reset_control *rst; > > > u32 size; > > > void __iomem *tmp; > > > int i, ret; > > > @@ -388,6 +390,13 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > > if (ret == 0) { > > > u32 pid, cid; > > > > > > + /* De-assert the reset line to take the device out of reset */ > > > + rst = reset_control_get_optional_exclusive(&dev->dev, NULL); > > > + if (IS_ERR(rst)) > > > + return PTR_ERR(rst); > > > > It is really correct to propagate an error if we cannot get exclusive > > ownership of the reset line. > > > > With drivers for vendor specific cells it is ok to "just know" that the > > reset line is never shared but we cannot know this for generic cells and > > we certainly can't know this for the bus. > > > > I think it *might* be OK to propagate an error if you used > > reset_control_get_optional_shared() instead because if that reports an > > error than arguably we have either a mistake in the DT or a bug in the > > driver we are sharing a reset with. > > > > Hmm. I'm not sure whether we can assume shared reset lines here or not! Maybe > Russell can share his opinion here. I've no reference to base an opinion on as I have no hardware that would use this facility. However, it seems obvious to me that if a reset line is shared between several devices, and when the reset line is asserted, it blocks reading the ID, then we do need a way for the AMBA primecell code to be able to lift the reset to read the device ID, and we need to do it in a way that is capable of being done even when another device/driver has already bound and taken control of the reset line. That said, if a reset line is shared between multiple devices, and a driver wants to assert the reset line, it would disrupt the operation of all those devices, so there would need to be some kind of synchronisation between the drivers. A different way to look at it though is that such a reset line is not a property of the individual devices, but of the bus - in which case it should be specified at bus level and controlled at bus level, not at individual device level. What is appropriate is entirely dependent on the hardware setup, and as I said above, with no view of the hardware I am unable to give a meaningful opinion. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-20 17:29 ` Russell King - ARM Linux admin @ 2019-03-21 11:17 ` Daniel Thompson 2019-03-28 16:43 ` Linus Walleij 0 siblings, 1 reply; 10+ messages in thread From: Daniel Thompson @ 2019-03-21 11:17 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Manivannan Sadhasivam, xuwei5, linux-arm-kernel, linux-kernel, linus.walleij, peter.griffin, guodong.xu, haojian.zhuang On Wed, Mar 20, 2019 at 05:29:56PM +0000, Russell King - ARM Linux admin wrote: > On Wed, Mar 20, 2019 at 12:26:58PM +0530, Manivannan Sadhasivam wrote: > > Hi Daniel, > > > > On Tue, Mar 12, 2019 at 12:27:11PM +0000, Daniel Thompson wrote: > > > On Sat, Mar 09, 2019 at 07:26:34AM +0530, Manivannan Sadhasivam wrote: > > > > For the AMBA Primecell devices having the reset lines wired, it is > > > > necessary to take them out of reset before reading the pid and cid values. > > > > Earlier we were dependent on the bootloader to do this but a more cleaner > > > > approach would be to do it in the kernel itself. Hence, this commit > > > > deasserts the reset line just before reading the pid and cid values. > > > > > > > > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > drivers/amba/bus.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > > > > index 41b706403ef7..da8f1aac5315 100644 > > > > --- a/drivers/amba/bus.c > > > > +++ b/drivers/amba/bus.c > > > > @@ -21,6 +21,7 @@ > > > > #include <linux/limits.h> > > > > #include <linux/clk/clk-conf.h> > > > > #include <linux/platform_device.h> > > > > +#include <linux/reset.h> > > > > > > > > #include <asm/irq.h> > > > > > > > > @@ -352,6 +353,7 @@ static void amba_device_release(struct device *dev) > > > > > > > > static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > > > { > > > > + struct reset_control *rst; > > > > u32 size; > > > > void __iomem *tmp; > > > > int i, ret; > > > > @@ -388,6 +390,13 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent) > > > > if (ret == 0) { > > > > u32 pid, cid; > > > > > > > > + /* De-assert the reset line to take the device out of reset */ > > > > + rst = reset_control_get_optional_exclusive(&dev->dev, NULL); > > > > + if (IS_ERR(rst)) > > > > + return PTR_ERR(rst); > > > > > > It is really correct to propagate an error if we cannot get exclusive > > > ownership of the reset line. > > > > > > With drivers for vendor specific cells it is ok to "just know" that the > > > reset line is never shared but we cannot know this for generic cells and > > > we certainly can't know this for the bus. > > > > > > I think it *might* be OK to propagate an error if you used > > > reset_control_get_optional_shared() instead because if that reports an > > > error than arguably we have either a mistake in the DT or a bug in the > > > driver we are sharing a reset with. > > > > > > > Hmm. I'm not sure whether we can assume shared reset lines here or not! Maybe > > Russell can share his opinion here. > > I've no reference to base an opinion on as I have no hardware that > would use this facility. > > However, it seems obvious to me that if a reset line is shared between > several devices, and when the reset line is asserted, it blocks reading > the ID, then we do need a way for the AMBA primecell code to be able > to lift the reset to read the device ID, and we need to do it in a way > that is capable of being done even when another device/driver has > already bound and taken control of the reset line. > > That said, if a reset line is shared between multiple devices, and a > driver wants to assert the reset line, it would disrupt the operation > of all those devices, so there would need to be some kind of > synchronisation between the drivers. That is what shared ownership of the reset line provides. When a line is shared a single driver does not have the authority to unilaterally assert reset because deasserts and asserts are counted and the line only goes high again when they balance. > A different way to look at it though is that such a reset line is not > a property of the individual devices, but of the bus - in which case > it should be specified at bus level and controlled at bus level, not > at individual device level. > > What is appropriate is entirely dependent on the hardware setup, and > as I said above, with no view of the hardware I am unable to give a > meaningful opinion. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] amba: Take device out of reset before reading pid and cid values 2019-03-21 11:17 ` Daniel Thompson @ 2019-03-28 16:43 ` Linus Walleij 0 siblings, 0 replies; 10+ messages in thread From: Linus Walleij @ 2019-03-28 16:43 UTC (permalink / raw) To: Daniel Thompson Cc: Russell King - ARM Linux admin, Manivannan Sadhasivam, Xu Wei, Linux ARM, linux-kernel, Peter Griffin, Guodong Xu, Haojian Zhuang On Thu, Mar 21, 2019 at 12:17 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: > On Wed, Mar 20, 2019 at 05:29:56PM +0000, Russell King - ARM Linux admin wrote: > > On Wed, Mar 20, 2019 at 12:26:58PM +0530, Manivannan Sadhasivam wrote: > > That said, if a reset line is shared between multiple devices, and a > > driver wants to assert the reset line, it would disrupt the operation > > of all those devices, so there would need to be some kind of > > synchronisation between the drivers. > > That is what shared ownership of the reset line provides. When a line is > shared a single driver does not have the authority to unilaterally > assert reset because deasserts and asserts are counted and the line only > goes high again when they balance. This is what we want for this I'm pretty sure. Regulators have the same thing with internal reference counting so that if you turn a shared regulator on from several clients it just increases the refcount and it will not really be turned off until the last consumer is disabling it. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] arm64: dts: hisilicon: Add reset properties for HI6220 I2C and SPI 2019-03-09 1:56 [PATCH 0/2] Handle I2C and SPI reset on HI6220 SoC Manivannan Sadhasivam 2019-03-09 1:56 ` [PATCH 1/2] amba: Take device out of reset before reading pid and cid values Manivannan Sadhasivam @ 2019-03-09 1:56 ` Manivannan Sadhasivam 1 sibling, 0 replies; 10+ messages in thread From: Manivannan Sadhasivam @ 2019-03-09 1:56 UTC (permalink / raw) To: linux, xuwei5 Cc: linux-arm-kernel, linux-kernel, linus.walleij, daniel.thompson, peter.griffin, guodong.xu, haojian.zhuang, Manivannan Sadhasivam Both I2C and SPI needs to be taken out of reset before being used. Earlier, we relied on the bootloader to do the job but a more cleaner approach would be to handle the reset in kernel. Hence, add the reset properties to the nodes to let the corresponding drivers take the peripherals out of reset. Suggested-by: Daniel Thompson <daniel.thompson@linaro.org> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi index 108e2a4227f6..b071dc466374 100644 --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi @@ -730,6 +730,8 @@ pinctrl-0 = <&spi0_pmx_func &spi0_cfg_func>; num-cs = <1>; cs-gpios = <&gpio6 2 0>; + resets = <&sys_ctrl PERIPH_RSTEN3_SSP>; + reset-names = "reset"; status = "disabled"; }; @@ -741,6 +743,8 @@ i2c-sda-hold-time-ns = <300>; pinctrl-names = "default"; pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>; + resets = <&sys_ctrl PERIPH_RSTEN3_I2C0>; + reset-names = "reset"; status = "disabled"; }; @@ -752,6 +756,8 @@ i2c-sda-hold-time-ns = <300>; pinctrl-names = "default"; pinctrl-0 = <&i2c1_pmx_func &i2c1_cfg_func>; + resets = <&sys_ctrl PERIPH_RSTEN3_I2C1>; + reset-names = "reset"; status = "disabled"; }; @@ -763,6 +769,8 @@ i2c-sda-hold-time-ns = <300>; pinctrl-names = "default"; pinctrl-0 = <&i2c2_pmx_func &i2c2_cfg_func>; + resets = <&sys_ctrl PERIPH_RSTEN3_I2C2>; + reset-names = "reset"; status = "disabled"; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-03-28 16:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-09 1:56 [PATCH 0/2] Handle I2C and SPI reset on HI6220 SoC Manivannan Sadhasivam 2019-03-09 1:56 ` [PATCH 1/2] amba: Take device out of reset before reading pid and cid values Manivannan Sadhasivam 2019-03-09 10:50 ` Linus Walleij 2019-03-12 12:27 ` Daniel Thompson 2019-03-20 6:56 ` Manivannan Sadhasivam 2019-03-20 14:29 ` Daniel Thompson 2019-03-20 17:29 ` Russell King - ARM Linux admin 2019-03-21 11:17 ` Daniel Thompson 2019-03-28 16:43 ` Linus Walleij 2019-03-09 1:56 ` [PATCH 2/2] arm64: dts: hisilicon: Add reset properties for HI6220 I2C and SPI Manivannan Sadhasivam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).