| Message ID | 20240626052238.1577580-2-aniketmaurya@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Select IBI ops for base platform | expand |
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
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
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
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
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
> > 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.
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
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.
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
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(+)