On Wed, Oct 28, 2015 at 10:32:37PM -0600, Stephen Warren wrote:
> On 10/28/2015 02:40 PM, Alexander Aring wrote:
> > This patch adds support for RPi several Power Domains and enable support
> > to enable the USB Power Domain when it's not enabled before.
> >
> > This patch based on Eric Anholt's patch to support Power Domains. He had
> > an issue about -EPROBE_DEFER inside the power domain subsystem, this
> > issue was solved by commit <311fa6a> ("PM / Domains: Return -EPROBE_DEFER
> > if we fail to init or turn-on domain").
>
> > diff --git
> > a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.txt
> >
> > b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.txt
>
> > firmware {
> > compatible = "raspberrypi,bcm2835-firmware";
> > mboxes = <&mailbox>;
> > + #power-domain-cells = <1>;
> > +};
>
> I would have expected a separate DT node for the power domains driver
> that referenced the firmware node by phandle. I believe that's why the
> firmware node exports mailboxes to other drivers. If the firmware driver
> was going to implement all the features directly, it wouldn't need to
> act as a mailbox provider, since all the mailbox programming would be
> internal.
>
ok.
> > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
>
> > +#define RPI_POWER_DOMAIN(_domain, _name) \
> > + [_domain] = \
> > + { \
>
> I'd expect { wrapped onto the previous line.
>
ok.
> > +static int raspberrypi_firmware_set_power(struct rpi_firmware *fw,
> > + u32 domain, bool on)
>
> > + packet.on = on;
> > + ret = rpi_firmware_property(fw, RPI_FIRMWARE_SET_POWER_STATE, &packet,
> > + sizeof(packet));
> > + if (!ret && !packet.on)
> > + ret = -EINVAL;
>
> The error is only reported for power off requests?
>
grab it from old patch. I will fix that, I suppose it should something
like:
if (!ret && packet.on != on)
ret = -EINVAL;
to confirm if packet.on is set or unset afterwards. I don't know if this
is a valid error reporting mechanism from firmware.
I will drop such checking here.
> > +/* Asks the firmware to if power is on for a specific power domain. */
> > +static int raspberrypi_firmware_power_is_on(struct rpi_firmware *fw,
> > + u32 domain)
>
> > + packet.domain = domain;
> > + ret = rpi_firmware_property(fw, RPI_FIRMWARE_GET_POWER_STATE, &packet,
> > + sizeof(packet));
> > + if (ret < 0)
> > + return ret;
>
> Hmm. If rpi_firmware_property() returns <0 on error, I'm confused what
> the test I commented on above is intended to do.
>
yes, I will fix it.
> > +/*
> > + * IMPORTANT: be sure this array has no entries which are not specified
> > + * between others by RPI_POWER_DOMAIN, otherwise mapping between
> > + * generic_pm_domain array doesn't work anymore.
> > + */
>
> "has no entries which are not specified between others by
> RPI_POWER_DOMAIN" might be better phrased as "is contiguous" or
> "contains only contiguous entries".
>
Okay, Eric Anholt told me to support the power domains which he had in
his patch only. I would add add the power domains only which has an
use-case -> "USB".
> > @@ -208,15 +312,44 @@ static int rpi_firmware_probe(struct platform_device
> > *pdev)
>
> > + for (i = 0; i < num_domains; i++) {
> > + bool is_off;
> > +
> > + rpi_power_domains[i].fw = fw;
> > + power_domains[i] = &rpi_power_domains[i].base;
> > +
> > + /* get the initial state */
> > + ret = raspberrypi_firmware_power_is_on(fw, i);
> > + if (ret < 0)
> > + goto mbox;
>
> The label name "mbox" doesn't give a clue that it's an error handler.
> "free_mbox" might be better.
>
ok.
> > +mbox:
> > + mbox_free_channel(fw->chan);
> > + return ret;
> > }
>
> Does the pm_genpd_init() call for all the power domains need to be
> undone at all?
I would say yes. The function will call:
list_add(&genpd->gpd_list_node, &gpd_list);
And gpd_list is a _static_ list inside the generic power domain subsystem.
If probing fails we need to delete these entries from gpd_list again.
I searched the whole file about "gpd_list_node" and can't find a
list_del on it. :-( Seems such handling isn't supported, but I think
"normally" it should be cleanuped then.
- Alex
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
