[PATCH 0/2] Select IBI ops for base platform

[PATCH 0/2] Select IBI ops for base platform

[Aniket]

This adds the option to select ibi ops for base platform driver

Aniket (2):
  dt-bindings: i3c: dw: Add property to select IBI ops
  i3c: dw: Select ibi ops for base platform driver

 Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 4 ++++
 drivers/i3c/master/dw-i3c-master.c                            | 3 +++
 2 files changed, 7 insertions(+)

-- 
2.45.2.741.gdbec12cfda-goog

[PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Aniket]

Use this property to select IBI related ops in the base platform driver.
Otherwise the driver defaults to return EINVAL for any IBI requests.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
index c0e805e531be..08c8ccdef691 100644
--- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
@@ -25,6 +25,10 @@ properties:
   interrupts:
     maxItems: 1
 
+  ibi-capable:
+    description: Set to select IBI ops.
+    type: boolean
+
 required:
   - compatible
   - reg
-- 
2.45.2.741.gdbec12cfda-goog

[PATCH 2/2] i3c: dw: Select ibi ops for base platform driver

[Aniket]

The AST2600 platform driver can always select the IBI ops.
We also need a way for the base platform driver to select
the ibi ops. Hence introduce this DT property which can be
used to register ibi ops from the base platform driver.

Signed-off-by: Aniket <aniketmaurya@google.com>
---
 drivers/i3c/master/dw-i3c-master.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 77a2a1c3fd1d..dff4f8e4e44e 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -1547,6 +1547,9 @@ static int dw_i3c_probe(struct platform_device *pdev)
 	if (!master)
 		return -ENOMEM;
 
+	if (of_property_read_bool(pdev->dev.of_node, "ibi-capable"))
+		master->ibi_capable = true;
+
 	return dw_i3c_common_probe(master, pdev);
 }
 
-- 
2.45.2.741.gdbec12cfda-goog

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Jeremy Kerr]

Hi Aniket,

> Use this property to select IBI related ops in the base platform
> driver. Otherwise the driver defaults to return EINVAL for any IBI
> requests.

[...]

> --- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> @@ -25,6 +25,10 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  ibi-capable:
> +    description: Set to select IBI ops.
> +    type: boolean
> +

Wouldn't the compatible string select whether the hardware instance
supports IBI or not?

I'd imagine that each specific synthesis of the DW IP would imply
corresponding hardware settings, and so would warrant its own compatible
value.

Maybe one for the DT folks: would this work better as individual
properties? Is there a policy here?

Cheers,


Jeremy

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Krzysztof Kozlowski]

On 26/06/2024 07:31, Jeremy Kerr wrote:
> Hi Aniket,
> 
>> Use this property to select IBI related ops in the base platform
>> driver. Otherwise the driver defaults to return EINVAL for any IBI
>> requests.
> 
> [...]
> 
>> --- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
>> +++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
>> @@ -25,6 +25,10 @@ properties:
>>    interrupts:
>>      maxItems: 1
>>  
>> +  ibi-capable:
>> +    description: Set to select IBI ops.

What are IBI ops? Standard form letter:

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

>> +    type: boolean
>> +
> 
> Wouldn't the compatible string select whether the hardware instance
> supports IBI or not?
> 
> I'd imagine that each specific synthesis of the DW IP would imply
> corresponding hardware settings, and so would warrant its own compatible
> value.
> 
> Maybe one for the DT folks: would this work better as individual
> properties? Is there a policy here?

Usually if feature is specific to given hardware, e.g. always capable of
foobar, then it can be deduced from compatible, so no need for new property.

Best regards,
Krzysztof

Re: [PATCH 2/2] i3c: dw: Select ibi ops for base platform driver

[Krzysztof Kozlowski]

On 26/06/2024 07:22, Aniket wrote:
> The AST2600 platform driver can always select the IBI ops.

So it is deducible from compatible.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Jeremy Kerr]

Hi Krysztof,

> > > +  ibi-capable:
> > > +    description: Set to select IBI ops.
> 
> What are IBI ops? Standard form letter:
> 
> You described the desired Linux feature or behavior, not the actual
> hardware.

In this case it is the actual hardware; my understanding is that the
gateware IP can be configured to support in-band-interrupts or not,
before being baked-in to hardware.

> > Wouldn't the compatible string select whether the hardware instance
> > supports IBI or not?
> > 
> > I'd imagine that each specific synthesis of the DW IP would imply
> > corresponding hardware settings, and so would warrant its own
> > compatible
> > value.
> > 
> > Maybe one for the DT folks: would this work better as individual
> > properties? Is there a policy here?
> 
> Usually if feature is specific to given hardware, e.g. always capable
> of foobar, then it can be deduced from compatible, so no need for new
> property.

Sounds good.

Aniket: the hardware you're dealing with there may need a new, specific
compatible property, which will dictate whether we enable IBIs in the
driver.

For cases where no other special behaviour is required, we can
represent this just as an entry in the OF match table.

Cheers,


Jeremy

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Aniket .]

Hi Jeremy,

> Aniket: the hardware you're dealing with there may need a new, specific
> compatible property, which will dictate whether we enable IBIs in the
> driver.
>
> For cases where no other special behaviour is required, we can
> represent this just as an entry in the OF match table.

Actually I see that IBI support is always present in the HW(DW I3C
IP). It's just that we have
an option in SW to decide whether to populate function pointers for IBI or not.
So can we remove this selection of ops and always go with ibi ops?

Thanks,
Aniket.

On Wed, Jun 26, 2024 at 1:48 PM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Hi Krysztof,
>
> > > > +  ibi-capable:
> > > > +    description: Set to select IBI ops.
> >
> > What are IBI ops? Standard form letter:
> >
> > You described the desired Linux feature or behavior, not the actual
> > hardware.
>
> In this case it is the actual hardware; my understanding is that the
> gateware IP can be configured to support in-band-interrupts or not,
> before being baked-in to hardware.
>
> > > Wouldn't the compatible string select whether the hardware instance
> > > supports IBI or not?
> > >
> > > I'd imagine that each specific synthesis of the DW IP would imply
> > > corresponding hardware settings, and so would warrant its own
> > > compatible
> > > value.
> > >
> > > Maybe one for the DT folks: would this work better as individual
> > > properties? Is there a policy here?
> >
> > Usually if feature is specific to given hardware, e.g. always capable
> > of foobar, then it can be deduced from compatible, so no need for new
> > property.
>
> Sounds good.
>
> Aniket: the hardware you're dealing with there may need a new, specific
> compatible property, which will dictate whether we enable IBIs in the
> driver.
>
> For cases where no other special behaviour is required, we can
> represent this just as an entry in the OF match table.
>
> Cheers,
>
>
> Jeremy

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Jeremy Kerr]

Hi Aniket,

> > For cases where no other special behaviour is required, we can
> > represent this just as an entry in the OF match table.
> 
> Actually I see that IBI support is always present in the HW(DW I3C
> IP). It's just that we have an option in SW to decide whether to
> populate function pointers for IBI or not.

OK, in that case this /definitely/ doesn't belong in the DT then, as
it's purely software configuration.

> So can we remove this selection of ops and always go with ibi ops?

Sounds fine to me, but I don't have direct experience with the non-
ast2600 uses of the dw core.

Cheers,


Jeremy

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Aniket .]

> > So can we remove this selection of ops and always go with ibi ops?
>
> Sounds fine to me, but I don't have direct experience with the non-
> ast2600 uses of the dw core.

I am using dw core directly through Synopsys virtualizer development
kit(VDK) setup.
I think it should be fine to always have the all ops supported in the
SW as they are in HW.
Shall I remove the ibi_capable property from the dw_i3c_master struct?

Thanks,
Aniket.

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Jeremy Kerr]

Hi Aniket,

> I am using dw core directly through Synopsys virtualizer development
> kit(VDK) setup.

Does that mean that *all* existing implementations of this design will
have IBI support? Changing this in the pre-existing driver will be
asserting that.

> Shall I remove the ibi_capable property from the dw_i3c_master
> struct?

Only if you can ensure it's not going to break the driver for existing
hardware deployments.

Cheers,


Jeremy

Re: [PATCH 1/2] dt-bindings: i3c: dw: Add property to select IBI ops

[Aniket .]

Hi Jeremy,

> Does that mean that *all* existing implementations of this design will
> have IBI support? Changing this in the pre-existing driver will be
> asserting that.

Yeah, I checked with Synopsys. All the implementations of dw core have
basic(without payload) IBI support in master mode.

> > Shall I remove the ibi_capable property from the dw_i3c_master
> > struct?
>
> Only if you can ensure it's not going to break the driver for existing
> hardware deployments.

Since all the implementations have basic support, I believe it should be fine.
Sending a patch to fix this.

Thanks,
Aniket.

Re: [PATCH 2/2] i3c: dw: Select ibi ops for base platform driver

[Aniket .]

Hi Krzysztof.

> > The AST2600 platform driver can always select the IBI ops.
>
> So it is deducible from compatible.

Yes, you are right. It shouldn't be a DT property.
Abandoning this patch. I'll send a different patch to always have IBI ops.

Thanks,
Aniket.