| Message ID | 20190524002847.30961-1-dinguyen@kernel.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2] dt-bindings: pl330: document the optional resets property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On 23-05-19, 19:28, Dinh Nguyen wrote: > The DMA controller on some SoCs can be held in reset, and thus requires > the reset signal(s) to deasserted. Most SoCs will have just one reset > signal, but there are others, i.e. Arria10/Stratix10 will have an > additional reset signal, referred to as the OCP. > > Add code to get the reset property from the device tree for deassert and > assert. > > Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> > --- > drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 6e6837214210..6018c43e785d 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -29,6 +29,7 @@ > #include <linux/err.h> > #include <linux/pm_runtime.h> > #include <linux/bug.h> > +#include <linux/reset.h> > > #include "dmaengine.h" > #define PL330_MAX_CHAN 8 > @@ -500,6 +501,9 @@ struct pl330_dmac { > unsigned int num_peripherals; > struct dma_pl330_chan *peripherals; /* keep at end */ > int quirks; > + > + struct reset_control *rstc; > + struct reset_control *rstc_ocp; > }; > > static struct pl330_of_quirks { > @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) > > amba_set_drvdata(adev, pl330); > > + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma"); > + if (IS_ERR(pl330->rstc)) { > + dev_err(&adev->dev, "No reset controller specified.\n"); Wasnt this optional?? > + return PTR_ERR(pl330->rstc); > + } else { > + ret = reset_control_deassert(pl330->rstc); > + if (ret) { > + dev_err(&adev->dev, "Couldn't deassert the device from reset!\n"); > + return ret; > + } > + } > + > + pl330->rstc_ocp = devm_reset_control_get_optional(&adev->dev, "dma-ocp"); > + if (IS_ERR(pl330->rstc_ocp)) { > + dev_err(&adev->dev, "No reset controller specified.\n"); > + return PTR_ERR(pl330->rstc_ocp); > + } else { > + ret = reset_control_deassert(pl330->rstc_ocp); > + if (ret) { > + dev_err(&adev->dev, "Couldn't deassert the device from OCP reset!\n"); > + return ret; > + } > + } > + > for (i = 0; i < AMBA_NR_IRQS; i++) { > irq = adev->irq[i]; > if (irq) { > @@ -3168,6 +3196,11 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) > probe_err2: > pl330_del(pl330); > > + if (pl330->rstc_ocp) > + reset_control_assert(pl330->rstc_ocp); > + > + if (pl330->rstc) > + reset_control_assert(pl330->rstc); > return ret; > } > > @@ -3206,6 +3239,11 @@ static int pl330_remove(struct amba_device *adev) > > pl330_del(pl330); > > + if (pl330->rstc_ocp) > + reset_control_assert(pl330->rstc_ocp); > + > + if (pl330->rstc) > + reset_control_assert(pl330->rstc); > return 0; > } > > -- > 2.20.0
Hi Vinod, On 6/4/19 7:14 AM, Vinod Koul wrote: > On 23-05-19, 19:28, Dinh Nguyen wrote: >> The DMA controller on some SoCs can be held in reset, and thus requires >> the reset signal(s) to deasserted. Most SoCs will have just one reset >> signal, but there are others, i.e. Arria10/Stratix10 will have an >> additional reset signal, referred to as the OCP. >> >> Add code to get the reset property from the device tree for deassert and >> assert. >> >> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> >> --- >> drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >> index 6e6837214210..6018c43e785d 100644 >> --- a/drivers/dma/pl330.c >> +++ b/drivers/dma/pl330.c >> @@ -29,6 +29,7 @@ >> #include <linux/err.h> >> #include <linux/pm_runtime.h> >> #include <linux/bug.h> >> +#include <linux/reset.h> >> >> #include "dmaengine.h" >> #define PL330_MAX_CHAN 8 >> @@ -500,6 +501,9 @@ struct pl330_dmac { >> unsigned int num_peripherals; >> struct dma_pl330_chan *peripherals; /* keep at end */ >> int quirks; >> + >> + struct reset_control *rstc; >> + struct reset_control *rstc_ocp; >> }; >> >> static struct pl330_of_quirks { >> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) >> >> amba_set_drvdata(adev, pl330); >> >> + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma"); >> + if (IS_ERR(pl330->rstc)) { >> + dev_err(&adev->dev, "No reset controller specified.\n"); > > Wasnt this optional?? Yes, this is optional. The call devm_reset_control_get_optional() will just return NULL if the reset property is not there, but an error pointer if something really went wrong. Thus, I'm using IS_ERR() for the error checking. Dinh
Hi Dinh, On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote: > On 6/4/19 7:14 AM, Vinod Koul wrote: > > On 23-05-19, 19:28, Dinh Nguyen wrote: > >> The DMA controller on some SoCs can be held in reset, and thus requires > >> the reset signal(s) to deasserted. Most SoCs will have just one reset > >> signal, but there are others, i.e. Arria10/Stratix10 will have an > >> additional reset signal, referred to as the OCP. > >> > >> Add code to get the reset property from the device tree for deassert and > >> assert. > >> > >> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> > >> --- > >> drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 38 insertions(+) > >> > >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > >> index 6e6837214210..6018c43e785d 100644 > >> --- a/drivers/dma/pl330.c > >> +++ b/drivers/dma/pl330.c > >> @@ -29,6 +29,7 @@ > >> #include <linux/err.h> > >> #include <linux/pm_runtime.h> > >> #include <linux/bug.h> > >> +#include <linux/reset.h> > >> > >> #include "dmaengine.h" > >> #define PL330_MAX_CHAN 8 > >> @@ -500,6 +501,9 @@ struct pl330_dmac { > >> unsigned int num_peripherals; > >> struct dma_pl330_chan *peripherals; /* keep at end */ > >> int quirks; > >> + > >> + struct reset_control *rstc; > >> + struct reset_control *rstc_ocp; > >> }; > >> > >> static struct pl330_of_quirks { > >> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) > >> > >> amba_set_drvdata(adev, pl330); > >> > >> + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma"); > >> + if (IS_ERR(pl330->rstc)) { > >> + dev_err(&adev->dev, "No reset controller specified.\n"); > > > > Wasnt this optional?? > > Yes, this is optional. The call devm_reset_control_get_optional() will > just return NULL if the reset property is not there, but an error > pointer if something really went wrong. Thus, I'm using IS_ERR() for the > error checking. So the error message is incorrect, as this is a real error condition? Gr{oetje,eeting}s, Geert
Hi Geert, On 6/4/19 11:31 AM, Geert Uytterhoeven wrote: > Hi Dinh, > > On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote: >> On 6/4/19 7:14 AM, Vinod Koul wrote: >>> On 23-05-19, 19:28, Dinh Nguyen wrote: >>>> The DMA controller on some SoCs can be held in reset, and thus requires >>>> the reset signal(s) to deasserted. Most SoCs will have just one reset >>>> signal, but there are others, i.e. Arria10/Stratix10 will have an >>>> additional reset signal, referred to as the OCP. >>>> >>>> Add code to get the reset property from the device tree for deassert and >>>> assert. >>>> >>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> >>>> --- >>>> drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 38 insertions(+) >>>> >>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>> index 6e6837214210..6018c43e785d 100644 >>>> --- a/drivers/dma/pl330.c >>>> +++ b/drivers/dma/pl330.c >>>> @@ -29,6 +29,7 @@ >>>> #include <linux/err.h> >>>> #include <linux/pm_runtime.h> >>>> #include <linux/bug.h> >>>> +#include <linux/reset.h> >>>> >>>> #include "dmaengine.h" >>>> #define PL330_MAX_CHAN 8 >>>> @@ -500,6 +501,9 @@ struct pl330_dmac { >>>> unsigned int num_peripherals; >>>> struct dma_pl330_chan *peripherals; /* keep at end */ >>>> int quirks; >>>> + >>>> + struct reset_control *rstc; >>>> + struct reset_control *rstc_ocp; >>>> }; >>>> >>>> static struct pl330_of_quirks { >>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) >>>> >>>> amba_set_drvdata(adev, pl330); >>>> >>>> + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma"); >>>> + if (IS_ERR(pl330->rstc)) { >>>> + dev_err(&adev->dev, "No reset controller specified.\n"); >>> >>> Wasnt this optional?? >> >> Yes, this is optional. The call devm_reset_control_get_optional() will >> just return NULL if the reset property is not there, but an error >> pointer if something really went wrong. Thus, I'm using IS_ERR() for the >> error checking. > > So the error message is incorrect, as this is a real error condition? > Yes, you're right! Will correct in V2. Dinh
Hi Geert, On 6/5/19 9:41 AM, Dinh Nguyen wrote: > Hi Geert, > > On 6/4/19 11:31 AM, Geert Uytterhoeven wrote: >> Hi Dinh, >> >> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote: >>> On 6/4/19 7:14 AM, Vinod Koul wrote: >>>> On 23-05-19, 19:28, Dinh Nguyen wrote: >>>>> The DMA controller on some SoCs can be held in reset, and thus requires >>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset >>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an >>>>> additional reset signal, referred to as the OCP. >>>>> >>>>> Add code to get the reset property from the device tree for deassert and >>>>> assert. >>>>> >>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> >>>>> --- >>>>> drivers/dma/pl330.c | 38 ++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 38 insertions(+) >>>>> >>>>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c >>>>> index 6e6837214210..6018c43e785d 100644 >>>>> --- a/drivers/dma/pl330.c >>>>> +++ b/drivers/dma/pl330.c >>>>> @@ -29,6 +29,7 @@ >>>>> #include <linux/err.h> >>>>> #include <linux/pm_runtime.h> >>>>> #include <linux/bug.h> >>>>> +#include <linux/reset.h> >>>>> >>>>> #include "dmaengine.h" >>>>> #define PL330_MAX_CHAN 8 >>>>> @@ -500,6 +501,9 @@ struct pl330_dmac { >>>>> unsigned int num_peripherals; >>>>> struct dma_pl330_chan *peripherals; /* keep at end */ >>>>> int quirks; >>>>> + >>>>> + struct reset_control *rstc; >>>>> + struct reset_control *rstc_ocp; >>>>> }; >>>>> >>>>> static struct pl330_of_quirks { >>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) >>>>> >>>>> amba_set_drvdata(adev, pl330); >>>>> >>>>> + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma"); >>>>> + if (IS_ERR(pl330->rstc)) { >>>>> + dev_err(&adev->dev, "No reset controller specified.\n"); >>>> >>>> Wasnt this optional?? >>> >>> Yes, this is optional. The call devm_reset_control_get_optional() will >>> just return NULL if the reset property is not there, but an error >>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the >>> error checking. >> >> So the error message is incorrect, as this is a real error condition? >> > > Yes, you're right! Will correct in V2. Looking at this again, I think the error message is correct. The optional call will return NULL if the resets property is not specified, and will return an error pointer if the reset propert is specified, but the pointer to the reset controller is not found. So I think the error message is correct. Dinh
Hi Dinh, On Wed, Jun 5, 2019 at 5:31 PM Dinh Nguyen <dinguyen@kernel.org> wrote: > On 6/5/19 9:41 AM, Dinh Nguyen wrote: > > On 6/4/19 11:31 AM, Geert Uytterhoeven wrote: > >> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote: > >>> On 6/4/19 7:14 AM, Vinod Koul wrote: > >>>> On 23-05-19, 19:28, Dinh Nguyen wrote: > >>>>> The DMA controller on some SoCs can be held in reset, and thus requires > >>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset > >>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an > >>>>> additional reset signal, referred to as the OCP. > >>>>> > >>>>> Add code to get the reset property from the device tree for deassert and > >>>>> assert. > >>>>> > >>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> > >>>>> --- a/drivers/dma/pl330.c > >>>>> +++ b/drivers/dma/pl330.c > >>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) > >>>>> > >>>>> amba_set_drvdata(adev, pl330); > >>>>> > >>>>> + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma"); > >>>>> + if (IS_ERR(pl330->rstc)) { > >>>>> + dev_err(&adev->dev, "No reset controller specified.\n"); "No reset controller specified.\n" > >>>> > >>>> Wasnt this optional?? > >>> > >>> Yes, this is optional. The call devm_reset_control_get_optional() will > >>> just return NULL if the reset property is not there, but an error > >>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the > >>> error checking. > >> > >> So the error message is incorrect, as this is a real error condition? > > > > Yes, you're right! Will correct in V2. > > Looking at this again, I think the error message is correct. The > optional call will return NULL if the resets property is not specified, > and will return an error pointer if the reset propert is specified, but > the pointer to the reset controller is not found. > > So I think the error message is correct. Please reread the error message, and what you wrote above. Error message: "No reset controller specified". Rationale: NULL (i.e. no error) if "the resets property is not specified". If an error pointer is returned, this may be due to probe deferral (to be propagated, but further ignored), or due to a real failure. So IMHO the code should read: if (IS_ERR(pl330->rstc)) { if (PTR_ERR(pl330->rstc) != -EPROBE_DEFER) dev_err(&adev->dev, "Failed to get reset.\n"); return PTR_ERR(pl330->rstc); } else { ... } Gr{oetje,eeting}s, Geert
Hi Geert, On 6/7/19 2:37 AM, Geert Uytterhoeven wrote: > Hi Dinh, > > On Wed, Jun 5, 2019 at 5:31 PM Dinh Nguyen <dinguyen@kernel.org> wrote: >> On 6/5/19 9:41 AM, Dinh Nguyen wrote: >>> On 6/4/19 11:31 AM, Geert Uytterhoeven wrote: >>>> On Tue, Jun 4, 2019 at 4:21 PM Dinh Nguyen <dinguyen@kernel.org> wrote: >>>>> On 6/4/19 7:14 AM, Vinod Koul wrote: >>>>>> On 23-05-19, 19:28, Dinh Nguyen wrote: >>>>>>> The DMA controller on some SoCs can be held in reset, and thus requires >>>>>>> the reset signal(s) to deasserted. Most SoCs will have just one reset >>>>>>> signal, but there are others, i.e. Arria10/Stratix10 will have an >>>>>>> additional reset signal, referred to as the OCP. >>>>>>> >>>>>>> Add code to get the reset property from the device tree for deassert and >>>>>>> assert. >>>>>>> >>>>>>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> > >>>>>>> --- a/drivers/dma/pl330.c >>>>>>> +++ b/drivers/dma/pl330.c > >>>>>>> @@ -3028,6 +3032,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) >>>>>>> >>>>>>> amba_set_drvdata(adev, pl330); >>>>>>> >>>>>>> + pl330->rstc = devm_reset_control_get_optional(&adev->dev, "dma"); >>>>>>> + if (IS_ERR(pl330->rstc)) { >>>>>>> + dev_err(&adev->dev, "No reset controller specified.\n"); > > "No reset controller specified.\n" > >>>>>> >>>>>> Wasnt this optional?? >>>>> >>>>> Yes, this is optional. The call devm_reset_control_get_optional() will >>>>> just return NULL if the reset property is not there, but an error >>>>> pointer if something really went wrong. Thus, I'm using IS_ERR() for the >>>>> error checking. >>>> >>>> So the error message is incorrect, as this is a real error condition? >>> >>> Yes, you're right! Will correct in V2. >> >> Looking at this again, I think the error message is correct. The >> optional call will return NULL if the resets property is not specified, >> and will return an error pointer if the reset propert is specified, but >> the pointer to the reset controller is not found. >> >> So I think the error message is correct. > > Please reread the error message, and what you wrote above. > > Error message: "No reset controller specified". > Rationale: NULL (i.e. no error) if "the resets property is not specified". > > If an error pointer is returned, this may be due to probe deferral (to be > propagated, but further ignored), or due to a real failure. > > So IMHO the code should read: > > if (IS_ERR(pl330->rstc)) { > if (PTR_ERR(pl330->rstc) != -EPROBE_DEFER) > dev_err(&adev->dev, "Failed to get reset.\n"); > return PTR_ERR(pl330->rstc); > } else { ... } > You're right! Will update in v2. Thanks! Dinh
diff --git a/Documentation/devicetree/bindings/dma/arm-pl330.txt b/Documentation/devicetree/bindings/dma/arm-pl330.txt index db7e2260f9c5..2c7fd1941abb 100644 --- a/Documentation/devicetree/bindings/dma/arm-pl330.txt +++ b/Documentation/devicetree/bindings/dma/arm-pl330.txt @@ -16,6 +16,9 @@ Optional properties: - dma-channels: contains the total number of DMA channels supported by the DMAC - dma-requests: contains the total number of DMA requests supported by the DMAC - arm,pl330-broken-no-flushp: quirk for avoiding to execute DMAFLUSHP + - resets: contains an entry for each entry in reset-names. + See ../reset/reset.txt for details. + - reset-names: must contain at least "dma", and optional is "dma-ocp". Example:
Add the optional resets property the pl330 dma node. Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> --- Documentation/devicetree/bindings/dma/arm-pl330.txt | 3 +++ 1 file changed, 3 insertions(+)