diff mbox series

[v2,04/12] PCI: brcmstb: Use swinit reset if available

Message ID 20240703180300.42959-5-james.quinlan@broadcom.com (mailing list archive)
State New, archived
Headers show
Series PCI: brcnstb: Enable STB 7712 SOC | expand

Commit Message

Jim Quinlan July 3, 2024, 6:02 p.m. UTC
The 7712 SOC adds a software init reset device for the PCIe HW.
If found in the DT node, use it.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Stanimir Varbanov July 4, 2024, 12:56 p.m. UTC | #1
Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> The 7712 SOC adds a software init reset device for the PCIe HW.
> If found in the DT node, use it.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4104c3668fdb..69926ee5c961 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,6 +266,7 @@ struct brcm_pcie {
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge;
> +	struct reset_control	*swinit;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "could not enable clock\n");
>  		return ret;
>  	}
> +
> +	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit)) {
> +		ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> +				    "failed to get 'swinit' reset\n");
> +		goto clk_out;
> +	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>  	if (IS_ERR(pcie->rescal)) {
>  		ret = PTR_ERR(pcie->rescal);
> @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		goto clk_out;
>  	}
>  
> +	ret = reset_control_assert(pcie->swinit);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> +		goto clk_out;
> +	}
> +	ret = reset_control_deassert(pcie->swinit);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> +		goto clk_out;
> +	}

why not call reset_control_reset(pcie->swinit) directly?

~Stan
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
Stanimir Varbanov July 5, 2024, 8:23 a.m. UTC | #2
Hi,

On 7/3/24 21:02, Jim Quinlan wrote:
> The 7712 SOC adds a software init reset device for the PCIe HW.
> If found in the DT node, use it.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4104c3668fdb..69926ee5c961 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,6 +266,7 @@ struct brcm_pcie {
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge;
> +	struct reset_control	*swinit;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "could not enable clock\n");
>  		return ret;
>  	}
> +
> +	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit)) {
> +		ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> +				    "failed to get 'swinit' reset\n");

Why "swinit" reset is special in this series? For example previous 3/12
patch which add "bridge" reset does not use dev_err_probe.

IMO you should use dev_err() here and below, and make a new follow-up
patch which change dev_err -> dev_err_probe on all places where getting
resources like resets and clocks.

> +		goto clk_out;
> +	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>  	if (IS_ERR(pcie->rescal)) {
>  		ret = PTR_ERR(pcie->rescal);
> @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		goto clk_out;
>  	}
>  
> +	ret = reset_control_assert(pcie->swinit);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> +		goto clk_out;

ditto

> +	}
> +	ret = reset_control_deassert(pcie->swinit);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> +		goto clk_out;

this is fine

> +	}
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");

~Stan
Jim Quinlan July 5, 2024, 5:46 p.m. UTC | #3
On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 7/3/24 21:02, Jim Quinlan wrote:
> > The 7712 SOC adds a software init reset device for the PCIe HW.
> > If found in the DT node, use it.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 4104c3668fdb..69926ee5c961 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -266,6 +266,7 @@ struct brcm_pcie {
> >       struct reset_control    *rescal;
> >       struct reset_control    *perst_reset;
> >       struct reset_control    *bridge;
> > +     struct reset_control    *swinit;
> >       int                     num_memc;
> >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> >       u32                     hw_rev;
> > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >               dev_err(&pdev->dev, "could not enable clock\n");
> >               return ret;
> >       }
> > +
> > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > +     if (IS_ERR(pcie->swinit)) {
> > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > +                                 "failed to get 'swinit' reset\n");
> > +             goto clk_out;
> > +     }
> >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> >       if (IS_ERR(pcie->rescal)) {
> >               ret = PTR_ERR(pcie->rescal);
> > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >               goto clk_out;
> >       }
> >
> > +     ret = reset_control_assert(pcie->swinit);
> > +     if (ret) {
> > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > +             goto clk_out;
> > +     }
> > +     ret = reset_control_deassert(pcie->swinit);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > +             goto clk_out;
> > +     }
>
> why not call reset_control_reset(pcie->swinit) directly?
Hi Stan,

There is no reset_control_reset() method defined for reset-brcmstb.c.
The only reason I can
think of for this is that it allows the callers of assert/deassert to
insert a delay if desired.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> ~Stan
> > +
> >       ret = reset_control_reset(pcie->rescal);
> >       if (ret) {
> >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
Philipp Zabel July 8, 2024, 9:37 a.m. UTC | #4
On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
> On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> > 
> > Hi Jim,
> > 
> > On 7/3/24 21:02, Jim Quinlan wrote:
> > > The 7712 SOC adds a software init reset device for the PCIe HW.
> > > If found in the DT node, use it.
> > > 
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index 4104c3668fdb..69926ee5c961 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -266,6 +266,7 @@ struct brcm_pcie {
> > >       struct reset_control    *rescal;
> > >       struct reset_control    *perst_reset;
> > >       struct reset_control    *bridge;
> > > +     struct reset_control    *swinit;
> > >       int                     num_memc;
> > >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> > >       u32                     hw_rev;
> > > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >               dev_err(&pdev->dev, "could not enable clock\n");
> > >               return ret;
> > >       }
> > > +
> > > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > > +     if (IS_ERR(pcie->swinit)) {
> > > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > > +                                 "failed to get 'swinit' reset\n");
> > > +             goto clk_out;
> > > +     }
> > >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > >       if (IS_ERR(pcie->rescal)) {
> > >               ret = PTR_ERR(pcie->rescal);
> > > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >               goto clk_out;
> > >       }
> > > 
> > > +     ret = reset_control_assert(pcie->swinit);
> > > +     if (ret) {
> > > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > > +             goto clk_out;
> > > +     }
> > > +     ret = reset_control_deassert(pcie->swinit);
> > > +     if (ret) {
> > > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > > +             goto clk_out;
> > > +     }
> > 
> > why not call reset_control_reset(pcie->swinit) directly?
> Hi Stan,
> 
> There is no reset_control_reset() method defined for reset-brcmstb.c.
> The only reason I can
> think of for this is that it allows the callers of assert/deassert to
> insert a delay if desired.

The main reason for the existence of reset_control_reset() is that
there are reset controllers that can only be triggered (e.g. by writing
a bit to a self-clearing register) to produce a complete reset pulse,
with assertion, delay, and deassertion all handled by the reset
controller.

regards
Philipp
Stanimir Varbanov July 8, 2024, 11:14 a.m. UTC | #5
Hi Philipp,

On 7/8/24 12:37, Philipp Zabel wrote:
> On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
>> On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>>
>>> Hi Jim,
>>>
>>> On 7/3/24 21:02, Jim Quinlan wrote:
>>>> The 7712 SOC adds a software init reset device for the PCIe HW.
>>>> If found in the DT node, use it.
>>>>
>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>> ---
>>>>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>>>>  1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>>> index 4104c3668fdb..69926ee5c961 100644
>>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>>> @@ -266,6 +266,7 @@ struct brcm_pcie {
>>>>       struct reset_control    *rescal;
>>>>       struct reset_control    *perst_reset;
>>>>       struct reset_control    *bridge;
>>>> +     struct reset_control    *swinit;
>>>>       int                     num_memc;
>>>>       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
>>>>       u32                     hw_rev;
>>>> @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>>>               dev_err(&pdev->dev, "could not enable clock\n");
>>>>               return ret;
>>>>       }
>>>> +
>>>> +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
>>>> +     if (IS_ERR(pcie->swinit)) {
>>>> +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
>>>> +                                 "failed to get 'swinit' reset\n");
>>>> +             goto clk_out;
>>>> +     }
>>>>       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>>>>       if (IS_ERR(pcie->rescal)) {
>>>>               ret = PTR_ERR(pcie->rescal);
>>>> @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>>>               goto clk_out;
>>>>       }
>>>>
>>>> +     ret = reset_control_assert(pcie->swinit);
>>>> +     if (ret) {
>>>> +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
>>>> +             goto clk_out;
>>>> +     }
>>>> +     ret = reset_control_deassert(pcie->swinit);
>>>> +     if (ret) {
>>>> +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
>>>> +             goto clk_out;
>>>> +     }
>>>
>>> why not call reset_control_reset(pcie->swinit) directly?
>> Hi Stan,
>>
>> There is no reset_control_reset() method defined for reset-brcmstb.c.
>> The only reason I can
>> think of for this is that it allows the callers of assert/deassert to
>> insert a delay if desired.
> 
> The main reason for the existence of reset_control_reset() is that
> there are reset controllers that can only be triggered (e.g. by writing
> a bit to a self-clearing register) to produce a complete reset pulse,
> with assertion, delay, and deassertion all handled by the reset
> controller.


Got it. Thank you for explanation.

But, IMO that means that the consumer driver should have knowledge of
low-level reset implementation, which is not generic API?

Otherwise, I don't see a problem to implement asset/deassert sequence in
.reset op in this particular reset-brcmstb.c low-level driver.


~Stan
Philipp Zabel July 8, 2024, 1:26 p.m. UTC | #6
Hi Stanimir,

On Mo, 2024-07-08 at 14:14 +0300, Stanimir Varbanov wrote:
> Hi Philipp,
> 
> On 7/8/24 12:37, Philipp Zabel wrote:
> > On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
> > > On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> > > > 
> > > > Hi Jim,
> > > > 
> > > > On 7/3/24 21:02, Jim Quinlan wrote:
> > > > > The 7712 SOC adds a software init reset device for the PCIe HW.
> > > > > If found in the DT node, use it.
> > > > > 
> > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > > ---
> > > > >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > > index 4104c3668fdb..69926ee5c961 100644
> > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > @@ -266,6 +266,7 @@ struct brcm_pcie {
> > > > >       struct reset_control    *rescal;
> > > > >       struct reset_control    *perst_reset;
> > > > >       struct reset_control    *bridge;
> > > > > +     struct reset_control    *swinit;
> > > > >       int                     num_memc;
> > > > >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> > > > >       u32                     hw_rev;
> > > > > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > >               dev_err(&pdev->dev, "could not enable clock\n");
> > > > >               return ret;
> > > > >       }
> > > > > +
> > > > > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > > > > +     if (IS_ERR(pcie->swinit)) {
> > > > > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > > > > +                                 "failed to get 'swinit' reset\n");
> > > > > +             goto clk_out;
> > > > > +     }
> > > > >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > > > >       if (IS_ERR(pcie->rescal)) {
> > > > >               ret = PTR_ERR(pcie->rescal);
> > > > > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > >               goto clk_out;
> > > > >       }
> > > > > 
> > > > > +     ret = reset_control_assert(pcie->swinit);
> > > > > +     if (ret) {
> > > > > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > > > > +             goto clk_out;
> > > > > +     }
> > > > > +     ret = reset_control_deassert(pcie->swinit);
> > > > > +     if (ret) {
> > > > > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > > > > +             goto clk_out;
> > > > > +     }
> > > > 
> > > > why not call reset_control_reset(pcie->swinit) directly?
> > > Hi Stan,
> > > 
> > > There is no reset_control_reset() method defined for reset-brcmstb.c.
> > > The only reason I can
> > > think of for this is that it allows the callers of assert/deassert to
> > > insert a delay if desired.
> > 
> > The main reason for the existence of reset_control_reset() is that
> > there are reset controllers that can only be triggered (e.g. by writing
> > a bit to a self-clearing register) to produce a complete reset pulse,
> > with assertion, delay, and deassertion all handled by the reset
> > controller.
> 
> Got it. Thank you for explanation.
> 
> But, IMO that means that the consumer driver should have knowledge of
> low-level reset implementation, which is not generic API?

Kind of. If the reset controller hardware has self-clearing resets, it
is impossible to implement manual reset_control_assert/deassert() on
top. So if a reset consumer requires that level of control, it just
can't work with a self-deasserting controller. The other way around, a
reset controller driver can emulate self-deasserting resets, iff it
knows the timing requirements of all consumers.

If the reset consumer only needs to see a pulse on the reset line, and
there are no ordering requirements with other resets or clocks, and the
device either doesn't care about timing or the reset controller knows
how to produce the required delay, then using reset_control_reset()
would be semantically correct.

> Otherwise, I don't see a problem to implement asset/deassert sequence in
> .reset op in this particular reset-brcmstb.c low-level driver.

When reset_control_reset() is used, every reset controller that can be
paired with this consumer needs to implement the .reset method,
requiring to know the delay requirements for all of their consumers.
The reset-simple driver implements this with a configurable worst-case
delay, for example. As far as I can see, that has never been used.

So yes, in this particular case, pcie-brcmstb only ever being paired
with reset-brcmstb, it might be no problem to implement .reset in
reset-brcmstb correctly, if all its consumers and their required delays
are known.

regards
Philipp
Jim Quinlan July 8, 2024, 2:38 p.m. UTC | #7
On Mon, Jul 8, 2024 at 9:26 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Stanimir,
>
> On Mo, 2024-07-08 at 14:14 +0300, Stanimir Varbanov wrote:
> > Hi Philipp,
> >
> > On 7/8/24 12:37, Philipp Zabel wrote:
> > > On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
> > > > On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> > > > >
> > > > > Hi Jim,
> > > > >
> > > > > On 7/3/24 21:02, Jim Quinlan wrote:
> > > > > > The 7712 SOC adds a software init reset device for the PCIe HW.
> > > > > > If found in the DT node, use it.
> > > > > >
> > > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > > > ---
> > > > > >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > > > index 4104c3668fdb..69926ee5c961 100644
> > > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > > @@ -266,6 +266,7 @@ struct brcm_pcie {
> > > > > >       struct reset_control    *rescal;
> > > > > >       struct reset_control    *perst_reset;
> > > > > >       struct reset_control    *bridge;
> > > > > > +     struct reset_control    *swinit;
> > > > > >       int                     num_memc;
> > > > > >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> > > > > >       u32                     hw_rev;
> > > > > > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > > >               dev_err(&pdev->dev, "could not enable clock\n");
> > > > > >               return ret;
> > > > > >       }
> > > > > > +
> > > > > > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > > > > > +     if (IS_ERR(pcie->swinit)) {
> > > > > > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > > > > > +                                 "failed to get 'swinit' reset\n");
> > > > > > +             goto clk_out;
> > > > > > +     }
> > > > > >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > > > > >       if (IS_ERR(pcie->rescal)) {
> > > > > >               ret = PTR_ERR(pcie->rescal);
> > > > > > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > > >               goto clk_out;
> > > > > >       }
> > > > > >
> > > > > > +     ret = reset_control_assert(pcie->swinit);
> > > > > > +     if (ret) {
> > > > > > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > > > > > +             goto clk_out;
> > > > > > +     }
> > > > > > +     ret = reset_control_deassert(pcie->swinit);
> > > > > > +     if (ret) {
> > > > > > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > > > > > +             goto clk_out;
> > > > > > +     }
> > > > >
> > > > > why not call reset_control_reset(pcie->swinit) directly?
> > > > Hi Stan,
> > > >
> > > > There is no reset_control_reset() method defined for reset-brcmstb.c.
> > > > The only reason I can
> > > > think of for this is that it allows the callers of assert/deassert to
> > > > insert a delay if desired.
> > >
> > > The main reason for the existence of reset_control_reset() is that
> > > there are reset controllers that can only be triggered (e.g. by writing
> > > a bit to a self-clearing register) to produce a complete reset pulse,
> > > with assertion, delay, and deassertion all handled by the reset
> > > controller.
> >
> > Got it. Thank you for explanation.
> >
> > But, IMO that means that the consumer driver should have knowledge of
> > low-level reset implementation, which is not generic API?
>
> Kind of. If the reset controller hardware has self-clearing resets, it
> is impossible to implement manual reset_control_assert/deassert() on
> top. So if a reset consumer requires that level of control, it just
> can't work with a self-deasserting controller. The other way around, a
> reset controller driver can emulate self-deasserting resets, iff it
> knows the timing requirements of all consumers.
>
> If the reset consumer only needs to see a pulse on the reset line, and
> there are no ordering requirements with other resets or clocks, and the
> device either doesn't care about timing or the reset controller knows
> how to produce the required delay, then using reset_control_reset()
> would be semantically correct.
>
> > Otherwise, I don't see a problem to implement asset/deassert sequence in
> > .reset op in this particular reset-brcmstb.c low-level driver.
>
> When reset_control_reset() is used, every reset controller that can be
> paired with this consumer needs to implement the .reset method,
> requiring to know the delay requirements for all of their consumers.
> The reset-simple driver implements this with a configurable worst-case
> delay, for example. As far as I can see, that has never been used.
>
> So yes, in this particular case, pcie-brcmstb only ever being paired
> with reset-brcmstb, it might be no problem to implement .reset in
> reset-brcmstb correctly, if all its consumers and their required delays
> are known.

This will not work.  reset-brcmstb.c is a reset provider; it provides
resets for dozens of different HW blocks.  Forcing  dozens of the
resets  to operate at the worst-case delay of the slowest one of them
is unacceptable, especially if a particular HW block uses its reset
often  and/or requires timely execution.

Regards,
Jim Quinlan
Broadcom STB/CM


>
> regards
> Philipp
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4104c3668fdb..69926ee5c961 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -266,6 +266,7 @@  struct brcm_pcie {
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
 	struct reset_control	*bridge;
+	struct reset_control	*swinit;
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
@@ -1626,6 +1627,13 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "could not enable clock\n");
 		return ret;
 	}
+
+	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
+	if (IS_ERR(pcie->swinit)) {
+		ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
+				    "failed to get 'swinit' reset\n");
+		goto clk_out;
+	}
 	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
 	if (IS_ERR(pcie->rescal)) {
 		ret = PTR_ERR(pcie->rescal);
@@ -1637,6 +1645,17 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 		goto clk_out;
 	}
 
+	ret = reset_control_assert(pcie->swinit);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
+		goto clk_out;
+	}
+	ret = reset_control_deassert(pcie->swinit);
+	if (ret) {
+		dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
+		goto clk_out;
+	}
+
 	ret = reset_control_reset(pcie->rescal);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");