| Message ID | 1442316018.15519.4.camel@mm-sol.com |
|---|---|
| State | New |
| Headers | show |
On 09/15, Ivan T. Ivanov wrote: > > On Mon, 2015-09-14 at 18:28 -0700, Stephen Boyd wrote: > > On 09/14/2015 02:54 PM, Stephen Boyd wrote: > > > The current driver implementation supports only 128 peripherals. > > > Add support for more than 128 peripherals by taking a lazy > > > caching approach to the mapping tables. Instead of reading the > > > tables at boot given some fixed size, read them on an as needed > > > basis and cache the results. We still assume a max number of 512 > > > peripherals, trading off some space for simplicity. > > > > > > Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and > > > Sagar Dharia <sdharia@codeaurora.org>. > > > > > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > > > --- > > > > Hi Ivan, > > > > This patch causes 8916 to crash, because there isn't a mapping for ppid > > 257 in the ppid to channel table. It seems that we're reading the revid > > from the slave id 1 pmic by going through channel 0, which seems to be > > setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can > > we stop reading the revid registers from non-zero slave id pmic devices? > > That would be one solution to fix this problem. Or maybe we need to > > special case this in the pmic arbiter code to fold ppid 0xN01 (slave id > > N and address 0x100) onto channel 0 all the time? > > > > Yes, we can. We are not using this information at the moment. > Right now, revision read is more or less for debug purposes. > > Would following patch work for you? Of course it will be difficult Yes the patch works fine. Feel free to add a Tested-by: Stephen Boyd <sboyd@codeaurora.org> > to guaranties that some other driver misbehave and touch non-existing > register, right? Yeah some driver could misbehave and try to read/write something that doesn't exist. As in this case, the device will reset. Given that all this stuff is in the kernel though, it seems fine to blow up instead of add a bunch of checks that aren't ever going to matter in practice.
On 09/15/2015 11:27 AM, Stephen Boyd wrote: > On 09/15, Ivan T. Ivanov wrote: >> On Mon, 2015-09-14 at 18:28 -0700, Stephen Boyd wrote: >>> On 09/14/2015 02:54 PM, Stephen Boyd wrote: >>>> The current driver implementation supports only 128 peripherals. >>>> Add support for more than 128 peripherals by taking a lazy >>>> caching approach to the mapping tables. Instead of reading the >>>> tables at boot given some fixed size, read them on an as needed >>>> basis and cache the results. We still assume a max number of 512 >>>> peripherals, trading off some space for simplicity. >>>> >>>> Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and >>>> Sagar Dharia <sdharia@codeaurora.org>. >>>> >>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> >>>> --- >>> Hi Ivan, >>> >>> This patch causes 8916 to crash, because there isn't a mapping for ppid >>> 257 in the ppid to channel table. It seems that we're reading the revid >>> from the slave id 1 pmic by going through channel 0, which seems to be >>> setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can >>> we stop reading the revid registers from non-zero slave id pmic devices? >>> That would be one solution to fix this problem. Or maybe we need to >>> special case this in the pmic arbiter code to fold ppid 0xN01 (slave id >>> N and address 0x100) onto channel 0 all the time? >>> >> Yes, we can. We are not using this information at the moment. >> Right now, revision read is more or less for debug purposes. >> >> Would following patch work for you? Of course it will be difficult > Yes the patch works fine. Feel free to add a > > Tested-by: Stephen Boyd <sboyd@codeaurora.org> > > I have to take this back. I missed the part where some pmics are on slave id 2 or slave id 4, so this check isn't going to work. I've adjusted it to use sid % 2 instead and I'll resend these two patches, but I imagine to be more robust we're going to need to add a revid node to the DT under the SID that actually has it. Then we can search the child nodes for a revid compatible node and do the rev probing stuff.
> On Oct 15, 2015, at 1:43 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 09/15/2015 11:27 AM, Stephen Boyd wrote: >> On 09/15, Ivan T. Ivanov wrote: >>> On Mon, 2015-09-14 at 18:28 -0700, Stephen Boyd wrote: >>>> On 09/14/2015 02:54 PM, Stephen Boyd wrote: >>>>> The current driver implementation supports only 128 peripherals. >>>>> Add support for more than 128 peripherals by taking a lazy >>>>> caching approach to the mapping tables. Instead of reading the >>>>> tables at boot given some fixed size, read them on an as needed >>>>> basis and cache the results. We still assume a max number of 512 >>>>> peripherals, trading off some space for simplicity. >>>>> >>>>> Based on a patch by Gilad Avidov <gavidov@codeaurora.org> and >>>>> Sagar Dharia <sdharia@codeaurora.org>. >>>>> >>>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> >>>>> --- >>>> Hi Ivan, >>>> >>>> This patch causes 8916 to crash, because there isn't a mapping for ppid >>>> 257 in the ppid to channel table. It seems that we're reading the revid >>>> from the slave id 1 pmic by going through channel 0, which seems to be >>>> setup for ppid 9 (slave id 0 and the peripheral starting at 0x900). Can >>>> we stop reading the revid registers from non-zero slave id pmic devices? >>>> That would be one solution to fix this problem. Or maybe we need to >>>> special case this in the pmic arbiter code to fold ppid 0xN01 (slave id >>>> N and address 0x100) onto channel 0 all the time? >>>> >>> Yes, we can. We are not using this information at the moment. >>> Right now, revision read is more or less for debug purposes. >>> >>> Would following patch work for you? Of course it will be difficult >> Yes the patch works fine. Feel free to add a >> >> Tested-by: Stephen Boyd <sboyd@codeaurora.org> >> >> > > I have to take this back. I missed the part where some pmics are on > slave id 2 or slave id 4, so this check isn't going to work. I've > adjusted it to use sid % 2 instead and I'll resend these two patches, > but I imagine to be more robust we're going to need to add a revid node > to the DT under the SID that actually has it. Then we can search the > child nodes for a revid compatible node and do the rev probing stuff. Ah, yes. We don’t use revision information for now. I suppose we can just remove these reads until we need this information? Regards, Ivan
On 10/15, Ivan T. Ivanov wrote: > > > On Oct 15, 2015, at 1:43 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > > I have to take this back. I missed the part where some pmics are on > > slave id 2 or slave id 4, so this check isn't going to work. I've > > adjusted it to use sid % 2 instead and I'll resend these two patches, > > but I imagine to be more robust we're going to need to add a revid node > > to the DT under the SID that actually has it. Then we can search the > > child nodes for a revid compatible node and do the rev probing stuff. > > Ah, yes. We don’t use revision information for now. > I suppose we can just remove these reads until we need > this information? > True, we could just remove all the code and make it look for a revid node at some later time. But later would be soon because I'm working on patches to add the read/write/volatile regmap tables to this driver. I guess I'll just go all the way and do the revid node part.
From d7c9c59b7134f093cf3f829832f4f7771a65664e Mon Sep 17 00:00:00 2001 From: "Ivan T. Ivanov" <ivan.ivanov@linaro.org> Date: Tue, 15 Sep 2015 09:43:10 +0300 Subject: [PATCH] mfd: qcom-spmi-pmic: Do not access non existing registers Cc: linux-kernel@vger.kernel.org Revision ID registers are available only on devices with Slave ID Zero, so don't make access to unavailable registers. Signed-off-by: Ivan T. Ivanov <ivan.ivanov@linaro.org> --- drivers/mfd/qcom-spmi-pmic.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c index af6ac1c4b45c..d7ad72af5682 100644 --- a/drivers/mfd/qcom-spmi-pmic.c +++ b/drivers/mfd/qcom-spmi-pmic.c @@ -122,12 +122,22 @@ static int pmic_spmi_probe(struct spmi_device *sdev) { struct device_node *root = sdev->dev.of_node; struct regmap *regmap; + u32 sid; + int ret; + + ret = of_property_read_u32(root, "reg", &sid); + if (ret < 0) { + dev_err(&sdev->dev, "Missing SID\n"); + return ret; + } regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config); if (IS_ERR(regmap)) return PTR_ERR(regmap); - pmic_spmi_show_revid(regmap, &sdev->dev); + /* Only devices with Slave ID Zero contain this information */ + if (sid == 0) + pmic_spmi_show_revid(regmap, &sdev->dev); return of_platform_populate(root, NULL, NULL, &sdev->dev); } -- 1.9.1