Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.
On Fri, 8 Mar 2019 at 16:51, Eric Anholt wrote:
>
> Dave Emett writes:
>
> > Sorry, a few things I thought of after sending the Reviewed-by email...
> >
> >> + v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
> >> + if (IS_ERR(v3d->reset)) {
> >> + ret = PTR_ERR(v3d->reset);
> >> +
> >> + if (ret == -EPROBE_DEFER)
> >> + goto dev_free;
> > Might be preferable to make this explicitly check against the
> > not-found error code (whatever that is)? As in if (not found)
> > else . Similarly...
>
> You won't have both a bridge and an external reset controller in the DT,
> so I'm not clear on what functional change you're looking for. You're
> just concerned about what the return code from this function is?
> -EPROBE_DEFER is the only one that matters from a probe, really.
I don't think it matters here. I just figured it should probably be
done the same way as the IRQ.
> >> + if (platform_get_irq(v3d->pdev, 1) < 0) {
> > This should probably explicitly check for not-found rather than any
> > error. As-is, we might silently go down the single-interrupt-line path
> > on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
> > hits some other error.
>
> If I do the -EPROBE_DEFER check here, will that be good enough for you?
If that's the only other error we can get then sure. It wasn't clear
to me what errors might be returned from platform_get_irq.
Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.
Eric Anholt writes:
> [ Unknown signature status ]
> Dave Emett writes:
>
>> Sorry, a few things I thought of after sending the Reviewed-by email...
>>
>>> + v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
>>> + if (IS_ERR(v3d->reset)) {
>>> + ret = PTR_ERR(v3d->reset);
>>> +
>>> + if (ret == -EPROBE_DEFER)
>>> + goto dev_free;
>> Might be preferable to make this explicitly check against the
>> not-found error code (whatever that is)? As in if (not found)
>> else . Similarly...
>
> You won't have both a bridge and an external reset controller in the DT,
> so I'm not clear on what functional change you're looking for. You're
> just concerned about what the return code from this function is?
> -EPROBE_DEFER is the only one that matters from a probe, really.
>
>>> + if (platform_get_irq(v3d->pdev, 1) < 0) {
>> This should probably explicitly check for not-found rather than any
>> error. As-is, we might silently go down the single-interrupt-line path
>> on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
>> hits some other error.
>
> If I do the -EPROBE_DEFER check here, will that be good enough for you?
>
>>> + ret = devm_request_irq(v3d->dev,
>>> platform_get_irq(v3d->pdev, 0),
>>> + v3d_hub_irq, IRQF_SHARED,
>>> + "v3d_hub", v3d);
>>> + ret = devm_request_irq(v3d->dev,
>>> platform_get_irq(v3d->pdev, 1),
>>> + v3d_irq, IRQF_SHARED,
>>> + "v3d_core0", v3d);
>> Not introduced by this change, but return value from first
>> devm_request_irq ignored here?
>
> True, but let's handle separate bugs separately.
Resubmitted with the fix anyway.
signature.asc
Description: PGP signature
Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.
Dave Emett writes:
> Sorry, a few things I thought of after sending the Reviewed-by email...
>
>> + v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
>> + if (IS_ERR(v3d->reset)) {
>> + ret = PTR_ERR(v3d->reset);
>> +
>> + if (ret == -EPROBE_DEFER)
>> + goto dev_free;
> Might be preferable to make this explicitly check against the
> not-found error code (whatever that is)? As in if (not found)
> else . Similarly...
You won't have both a bridge and an external reset controller in the DT,
so I'm not clear on what functional change you're looking for. You're
just concerned about what the return code from this function is?
-EPROBE_DEFER is the only one that matters from a probe, really.
>> + if (platform_get_irq(v3d->pdev, 1) < 0) {
> This should probably explicitly check for not-found rather than any
> error. As-is, we might silently go down the single-interrupt-line path
> on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
> hits some other error.
If I do the -EPROBE_DEFER check here, will that be good enough for you?
>> + ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev,
>> 0),
>> + v3d_hub_irq, IRQF_SHARED,
>> + "v3d_hub", v3d);
>> + ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev,
>> 1),
>> + v3d_irq, IRQF_SHARED,
>> + "v3d_core0", v3d);
> Not introduced by this change, but return value from first
> devm_request_irq ignored here?
True, but let's handle separate bugs separately.
signature.asc
Description: PGP signature
Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.
Sorry, a few things I thought of after sending the Reviewed-by email...
> + v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
> + if (IS_ERR(v3d->reset)) {
> + ret = PTR_ERR(v3d->reset);
> +
> + if (ret == -EPROBE_DEFER)
> + goto dev_free;
Might be preferable to make this explicitly check against the
not-found error code (whatever that is)? As in if (not found)
else . Similarly...
> + if (platform_get_irq(v3d->pdev, 1) < 0) {
This should probably explicitly check for not-found rather than any
error. As-is, we might silently go down the single-interrupt-line path
on a platform with 2 interrupt lines if platform_get_irq(v3d->pdev, 1)
hits some other error.
> + ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev,
> 0),
> + v3d_hub_irq, IRQF_SHARED,
> + "v3d_hub", v3d);
> + ret = devm_request_irq(v3d->dev, platform_get_irq(v3d->pdev,
> 1),
> + v3d_irq, IRQF_SHARED,
> + "v3d_core0", v3d);
Not introduced by this change, but return value from first
devm_request_irq ignored here?
Re: [PATCH v3 1/3] drm/v3d: Add support for V3D v4.2.
On Wed, 20 Feb 2019 at 23:37, Eric Anholt wrote:
>
> No compatible string for it yet, just the version-dependent changes.
> They've now tied the hub and the core interrupt lines into a single
> interrupt line coming out of the block. It also turns out I made a
> mistake in modeling the V3D v3.3 and v4.1 bridge as a part of V3D
> itself -- the bridge is going away in favor of an external reset
> controller in a larger HW module.
>
> v2: Use consistent checks for whether we're on 4.2, and fix a leak in
> an error path.
> v3: Use more general means of determining if the current 4.2 changes
> are in place, as apparently other platforms may switch back (noted
> by Dave). Update the binding doc.
>
> Signed-off-by: Eric Anholt
Reviewed-by: Dave Emett
> ---
> .../devicetree/bindings/gpu/brcm,bcm-v3d.txt | 11 ++--
> drivers/gpu/drm/v3d/v3d_drv.c | 21 +++---
> drivers/gpu/drm/v3d/v3d_drv.h | 2 ++
> drivers/gpu/drm/v3d/v3d_gem.c | 12 +++-
> drivers/gpu/drm/v3d/v3d_irq.c | 28 +++
> 5 files changed, 60 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
> b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
> index c907aa8dd755..b2df82b44625 100644
> --- a/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
> +++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-v3d.txt
> @@ -6,15 +6,20 @@ For V3D 2.x, see brcm,bcm-vc4.txt.
> Required properties:
> - compatible: Should be "brcm,7268-v3d" or "brcm,7278-v3d"
> - reg: Physical base addresses and lengths of the register areas
> -- reg-names: Names for the register areas. The "hub", "bridge", and
> "core0"
> +- reg-names: Names for the register areas. The "hub" and "core0"
> register areas are always required. The "gca" register area
> - is required if the GCA cache controller is present.
> + is required if the GCA cache controller is present. The
> + "bridge" register area is required if an external reset
> + controller is not present.
> - interrupts: The interrupt numbers. The first interrupt is for the hub,
> - while the following interrupts are for the cores.
> + while the following interrupts are separate interrupt lines
> + for the cores (if they don't share the hub's interrupt).
> See bindings/interrupt-controller/interrupts.txt
>
> Optional properties:
> - clocks: The core clock the unit runs on
> +- resets: The reset line for v3d, if not using a mapping of the bridge
> + See bindings/reset/reset.txt
>
> v3d {
> compatible = "brcm,7268-v3d";
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> index b189b1a0ae7e..392e458b55bd 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -496,10 +497,6 @@ static int v3d_platform_drm_probe(struct platform_device
> *pdev)
> v3d->pdev = pdev;
> drm = >drm;
>
> - ret = map_regs(v3d, >bridge_regs, "bridge");
> - if (ret)
> - goto dev_free;
> -
> ret = map_regs(v3d, >hub_regs, "hub");
> if (ret)
> goto dev_free;
> @@ -514,6 +511,22 @@ static int v3d_platform_drm_probe(struct platform_device
> *pdev)
> v3d->cores = V3D_GET_FIELD(ident1, V3D_HUB_IDENT1_NCORES);
> WARN_ON(v3d->cores > 1); /* multicore not yet implemented */
>
> + v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
> + if (IS_ERR(v3d->reset)) {
> + ret = PTR_ERR(v3d->reset);
> +
> + if (ret == -EPROBE_DEFER)
> + goto dev_free;
> +
> + v3d->reset = NULL;
> + ret = map_regs(v3d, >bridge_regs, "bridge");
> + if (ret) {
> + dev_err(dev,
> + "Failed to get reset control or bridge
> regs\n");
> + goto dev_free;
> + }
> + }
> +
> if (v3d->ver < 41) {
> ret = map_regs(v3d, >gca_regs, "gca");
> if (ret)
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index 9e0e11f57307..fab9979b7e1f 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -39,6 +39,7 @@ struct v3d_dev {
> * and revision.
> */
> int ver;
> + bool single_irq_line;
>
> struct device *dev;
> struct platform_device *pdev;
> @@ -47,6 +48,7 @@ struct v3d_dev {
> void __iomem *bridge_regs;
> void __iomem *gca_regs;
> struct clk *clk;
> + struct reset_control *reset;
>
> /* Virtual and DMA
