From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932693AbcG1Ckq (ORCPT ); Wed, 27 Jul 2016 22:40:46 -0400 Received: from conssluserg-06.nifty.com ([210.131.2.91]:53215 "EHLO conssluserg-06.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932653AbcG1Ckd (ORCPT ); Wed, 27 Jul 2016 22:40:33 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-06.nifty.com u6S2eHk6019769 X-Nifty-SrcIP: [209.85.161.171] MIME-Version: 1.0 In-Reply-To: <1469611051.2470.15.camel@pengutronix.de> References: <1469557209-13089-1-git-send-email-yamada.masahiro@socionext.com> <1469611051.2470.15.camel@pengutronix.de> From: Masahiro Yamada Date: Thu, 28 Jul 2016 11:40:16 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] reset: uniphier: add reset controller drivers for UniPhier SoCs To: Philipp Zabel Cc: Mark Rutland , devicetree@vger.kernel.org, Rob Herring , Linux Kernel Mailing List , linux-arm-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Philipp, 2016-07-27 18:17 GMT+09:00 Philipp Zabel : >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "reset-uniphier.h" >> + >> +struct uniphier_reset_priv { >> + struct reset_controller_dev rcdev; >> + struct device *dev; >> + struct regmap *regmap; >> + const struct uniphier_reset_data *data; >> +}; >> + >> +#define to_uniphier_reset_priv(_rcdev) \ >> + container_of(_rcdev, struct uniphier_reset_priv, rcdev) >> + >> +static int uniphier_reset_update(struct reset_controller_dev *rcdev, >> + unsigned long id, bool assert) >> +{ >> + struct uniphier_reset_priv *priv = to_uniphier_reset_priv(rcdev); >> + const struct uniphier_reset_data *p; >> + bool handled = false; >> + >> + for (p = priv->data; p->id != UNIPHIER_RESET_ID_END; p++) { >> + unsigned int val; >> + int ret; >> + >> + if (p->id != id) >> + continue; >> + >> + val = p->assert_val; >> + if (!assert) >> + val = ~val; >> + >> + ret = regmap_write_bits(priv->regmap, p->reg, p->mask, val); >> + if (ret) >> + return ret; >> + >> + handled = true; > > Why does this continue to walk through the list after the correct id was > found? Looks like you already found the answer for this. >> +#define UNIPHIER_MIO_RESET_USB2(index, ch) \ >> + UNIPHIER_RESETX_SIMPLE((index), 0x110 + 0x200 * (ch), BIT(24)), \ >> + UNIPHIER_RESETX_SIMPLE((index), 0x114 + 0x200 * (ch), BIT(0)) > > Ah, so for USB2 reset you have two reset bits in separate registers. Are > you sure these are controlling the same reset line? I am not a hardware guy, so I am not sure about the hardware design. >>From my best guess, I think each bit controls a different block. But both of them must be de-asserted before starting up USB. There is no use-case where they are asserted/de-asserted independently. So, I thought it made sense to couple them into a single ID. > If the USB core does in fact have two separate reset inputs that just > happen to need asserting at the same time, this should still get two > separate ids. Same issue for the SD reset above, if the reset lines are > physically separate, please don't combine them in the driver. Right. >>From the view of point of Device Tree interface, it should reflect the hardware design. I believe they are separate reset signals, so should be given with separate IDs. But, as a software engineer, it is sometimes difficult to fully understand the hardware structure. The hardware document often just says "how to use USB", but "how clock/reset signals are connected in each block" is not mentioned, or at least very unclear. Probably, I will come back with real per-reset-line ID, but I need some time to take a look. -- Best Regards Masahiro Yamada From mboxrd@z Thu Jan 1 00:00:00 1970 From: Masahiro Yamada Subject: Re: [PATCH v2] reset: uniphier: add reset controller drivers for UniPhier SoCs Date: Thu, 28 Jul 2016 11:40:16 +0900 Message-ID: References: <1469557209-13089-1-git-send-email-yamada.masahiro@socionext.com> <1469611051.2470.15.camel@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <1469611051.2470.15.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Philipp Zabel Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring , Linux Kernel Mailing List , linux-arm-kernel List-Id: devicetree@vger.kernel.org Hi Philipp, 2016-07-27 18:17 GMT+09:00 Philipp Zabel : >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "reset-uniphier.h" >> + >> +struct uniphier_reset_priv { >> + struct reset_controller_dev rcdev; >> + struct device *dev; >> + struct regmap *regmap; >> + const struct uniphier_reset_data *data; >> +}; >> + >> +#define to_uniphier_reset_priv(_rcdev) \ >> + container_of(_rcdev, struct uniphier_reset_priv, rcdev) >> + >> +static int uniphier_reset_update(struct reset_controller_dev *rcdev, >> + unsigned long id, bool assert) >> +{ >> + struct uniphier_reset_priv *priv = to_uniphier_reset_priv(rcdev); >> + const struct uniphier_reset_data *p; >> + bool handled = false; >> + >> + for (p = priv->data; p->id != UNIPHIER_RESET_ID_END; p++) { >> + unsigned int val; >> + int ret; >> + >> + if (p->id != id) >> + continue; >> + >> + val = p->assert_val; >> + if (!assert) >> + val = ~val; >> + >> + ret = regmap_write_bits(priv->regmap, p->reg, p->mask, val); >> + if (ret) >> + return ret; >> + >> + handled = true; > > Why does this continue to walk through the list after the correct id was > found? Looks like you already found the answer for this. >> +#define UNIPHIER_MIO_RESET_USB2(index, ch) \ >> + UNIPHIER_RESETX_SIMPLE((index), 0x110 + 0x200 * (ch), BIT(24)), \ >> + UNIPHIER_RESETX_SIMPLE((index), 0x114 + 0x200 * (ch), BIT(0)) > > Ah, so for USB2 reset you have two reset bits in separate registers. Are > you sure these are controlling the same reset line? I am not a hardware guy, so I am not sure about the hardware design. >>From my best guess, I think each bit controls a different block. But both of them must be de-asserted before starting up USB. There is no use-case where they are asserted/de-asserted independently. So, I thought it made sense to couple them into a single ID. > If the USB core does in fact have two separate reset inputs that just > happen to need asserting at the same time, this should still get two > separate ids. Same issue for the SD reset above, if the reset lines are > physically separate, please don't combine them in the driver. Right. >>From the view of point of Device Tree interface, it should reflect the hardware design. I believe they are separate reset signals, so should be given with separate IDs. But, as a software engineer, it is sometimes difficult to fully understand the hardware structure. The hardware document often just says "how to use USB", but "how clock/reset signals are connected in each block" is not mentioned, or at least very unclear. Probably, I will come back with real per-reset-line ID, but I need some time to take a look. -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: yamada.masahiro@socionext.com (Masahiro Yamada) Date: Thu, 28 Jul 2016 11:40:16 +0900 Subject: [PATCH v2] reset: uniphier: add reset controller drivers for UniPhier SoCs In-Reply-To: <1469611051.2470.15.camel@pengutronix.de> References: <1469557209-13089-1-git-send-email-yamada.masahiro@socionext.com> <1469611051.2470.15.camel@pengutronix.de> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Philipp, 2016-07-27 18:17 GMT+09:00 Philipp Zabel : >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "reset-uniphier.h" >> + >> +struct uniphier_reset_priv { >> + struct reset_controller_dev rcdev; >> + struct device *dev; >> + struct regmap *regmap; >> + const struct uniphier_reset_data *data; >> +}; >> + >> +#define to_uniphier_reset_priv(_rcdev) \ >> + container_of(_rcdev, struct uniphier_reset_priv, rcdev) >> + >> +static int uniphier_reset_update(struct reset_controller_dev *rcdev, >> + unsigned long id, bool assert) >> +{ >> + struct uniphier_reset_priv *priv = to_uniphier_reset_priv(rcdev); >> + const struct uniphier_reset_data *p; >> + bool handled = false; >> + >> + for (p = priv->data; p->id != UNIPHIER_RESET_ID_END; p++) { >> + unsigned int val; >> + int ret; >> + >> + if (p->id != id) >> + continue; >> + >> + val = p->assert_val; >> + if (!assert) >> + val = ~val; >> + >> + ret = regmap_write_bits(priv->regmap, p->reg, p->mask, val); >> + if (ret) >> + return ret; >> + >> + handled = true; > > Why does this continue to walk through the list after the correct id was > found? Looks like you already found the answer for this. >> +#define UNIPHIER_MIO_RESET_USB2(index, ch) \ >> + UNIPHIER_RESETX_SIMPLE((index), 0x110 + 0x200 * (ch), BIT(24)), \ >> + UNIPHIER_RESETX_SIMPLE((index), 0x114 + 0x200 * (ch), BIT(0)) > > Ah, so for USB2 reset you have two reset bits in separate registers. Are > you sure these are controlling the same reset line? I am not a hardware guy, so I am not sure about the hardware design. >>From my best guess, I think each bit controls a different block. But both of them must be de-asserted before starting up USB. There is no use-case where they are asserted/de-asserted independently. So, I thought it made sense to couple them into a single ID. > If the USB core does in fact have two separate reset inputs that just > happen to need asserting at the same time, this should still get two > separate ids. Same issue for the SD reset above, if the reset lines are > physically separate, please don't combine them in the driver. Right. >>From the view of point of Device Tree interface, it should reflect the hardware design. I believe they are separate reset signals, so should be given with separate IDs. But, as a software engineer, it is sometimes difficult to fully understand the hardware structure. The hardware document often just says "how to use USB", but "how clock/reset signals are connected in each block" is not mentioned, or at least very unclear. Probably, I will come back with real per-reset-line ID, but I need some time to take a look. -- Best Regards Masahiro Yamada