| Message ID | 20240829-wip-bl-ad3552r-axi-v0-v1-4-b6da6015327a@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: dac: introducing ad3552r-axi | expand |
On Thu, 29 Aug 2024 14:32:02 +0200, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Add bus property. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml: properties:bus-type: 'enum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']} hint: Scalar and array keywords cannot be mixed from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240829-wip-bl-ad3552r-axi-v0-v1-4-b6da6015327a@baylibre.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Add bus property. RFC it may be, but you do need to explain what this bus-type actually describes for commenting on the suitability of the method to be meaningful. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > index a55e9bfc66d7..a7ce72e1cd81 100644 > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > @@ -38,6 +38,15 @@ properties: > clocks: > maxItems: 1 You mentioned about new compatible strings, does the one currently listed in this binding support both bus types? Making the bus type decision based on compatible only really makes sense if they're different versions of the IP, but not if they're different configuration options for a given version. > + bus-type: If, as you mentioned, there are multiple bus types, a non-flag property does make sense. However, I am really not keen on these "forced" numerical properties at all, I'd much rather see strings used here. Thanks, Conor. > + maxItems: 1 > + description: | > + Configure bus type: > + - 0: none > + - 1: qspi > + enum: [0, 1] > + default: 0 > + > '#io-backend-cells': > const: 0 > > > -- > 2.45.0.rc1 >
On Thu, Aug 29, 2024 at 04:46:59PM +0100, Conor Dooley wrote: > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Add bus property. > > RFC it may be, but you do need to explain what this bus-type actually > describes for commenting on the suitability of the method to be > meaningful. > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > --- > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > @@ -38,6 +38,15 @@ properties: > > clocks: > > maxItems: 1 > > You mentioned about new compatible strings, does the one currently > listed in this binding support both bus types? > > Making the bus type decision based on compatible only really makes sense > if they're different versions of the IP, but not if they're different > configuration options for a given version. > Yeah, in general the parent defines the bus type. Best regards, Krzysztof
Hi Conor, On 29/08/24 5:46 PM, Conor Dooley wrote: > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: >> From: Angelo Dureghello <adureghello@baylibre.com> >> >> Add bus property. > RFC it may be, but you do need to explain what this bus-type actually > describes for commenting on the suitability of the method to be > meaningful. thanks for the feedbacks, a "bus" is intended as a generic interface connected to the target, may be used from a custom IP (fpga) to communicate with the target device (by read/write(reg and value)) using a special custom interface. The bus could also be physically the same of some well-known existing interfaces (as parallel, lvds or other uncommon interfaces), but using an uncommon/custom protocol over it. In concrete, actually bus-type is added to the backend since the ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR parallel bus (interface that i named QSPI, but it's not exactly a QSPI as a protocol), so it's a device-specific interface. With additions in this patchset, other frontends, of course not only DACs, will be able to add specific busses and read/wrtie to the bus as needed. >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >> --- >> Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >> index a55e9bfc66d7..a7ce72e1cd81 100644 >> --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >> +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >> @@ -38,6 +38,15 @@ properties: >> clocks: >> maxItems: 1 > You mentioned about new compatible strings, does the one currently > listed in this binding support both bus types? > > Making the bus type decision based on compatible only really makes sense > if they're different versions of the IP, but not if they're different > configuration options for a given version. > >> + bus-type: DAC IP on fpga actually respects same structure and register set, except for a named "custom" register that may use specific bitfields depending on the application of the IP. > If, as you mentioned, there are multiple bus types, a non-flag property > does make sense. However, I am really not keen on these "forced" numerical > properties at all, I'd much rather see strings used here. ack, thanks. > Thanks, > Conor. thanks a lot, regards, Angelo >> + maxItems: 1 >> + description: | >> + Configure bus type: >> + - 0: none >> + - 1: qspi >> + enum: [0, 1] >> + default: 0 >> + >> '#io-backend-cells': >> const: 0 >> >> >> -- >> 2.45.0.rc1 >>
On Fri, Aug 30, 2024 at 10:16:36AM +0200, Krzysztof Kozlowski wrote: > On Thu, Aug 29, 2024 at 04:46:59PM +0100, Conor Dooley wrote: > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > Add bus property. > > > > RFC it may be, but you do need to explain what this bus-type actually > > describes for commenting on the suitability of the method to be > > meaningful. > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > --- > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > @@ -38,6 +38,15 @@ properties: > > > clocks: > > > maxItems: 1 > > > > You mentioned about new compatible strings, does the one currently > > listed in this binding support both bus types? > > > > Making the bus type decision based on compatible only really makes sense > > if they're different versions of the IP, but not if they're different > > configuration options for a given version. > > > > Yeah, in general the parent defines the bus type. Right, if the bus that's being used isn't spi anymore, you should be able to detect that without a property. However, the device that "left" the spi bus is not this "adi,axi-dac" it is the adi,ad3552r. I think this property is actually representing the bus that this adi,axi-dac is /providing/, rather than the bus it is "consuming".
On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > Hi Conor, > > On 29/08/24 5:46 PM, Conor Dooley wrote: > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > Add bus property. > > RFC it may be, but you do need to explain what this bus-type actually > > describes for commenting on the suitability of the method to be > > meaningful. > > thanks for the feedbacks, > > a "bus" is intended as a generic interface connected to the target, > may be used from a custom IP (fpga) to communicate with the target > device (by read/write(reg and value)) using a special custom interface. > > The bus could also be physically the same of some well-known existing > interfaces (as parallel, lvds or other uncommon interfaces), but using > an uncommon/custom protocol over it. > > In concrete, actually bus-type is added to the backend since the > ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > parallel bus (interface that i named QSPI, but it's not exactly a QSPI > as a protocol), so it's a device-specific interface. > > With additions in this patchset, other frontends, of course not only > DACs, will be able to add specific busses and read/wrtie to the bus > as needed. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > --- > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > @@ -38,6 +38,15 @@ properties: > > > clocks: > > > maxItems: 1 > > You mentioned about new compatible strings, does the one currently > > listed in this binding support both bus types? You didn't answer this, and there's insufficient explanation of the "hardware" in this RFC, but I found this which is supposedly the backend: https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r adi,axi-dac.yaml has a single compatible, and that compatible has nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would expect either justification for reuse of the compatible, or a brand new compatible for this backend, even if the driver can mostly be reused. Could you please link to whatever ADI wiki has detailed information on how this stuff works so that I can look at it to better understand the axes of configuration here? > > > > Making the bus type decision based on compatible only really makes sense > > if they're different versions of the IP, but not if they're different > > configuration options for a given version. > > > > > + bus-type: > > DAC IP on fpga actually respects same structure and register set, except > for a named "custom" register that may use specific bitfields depending > on the application of the IP. To paraphrase: "The register map is the same, except for the bit that is different". If ADI is shipping several different configurations of this IP for different DACs, I'd be expecting different compatibles for each backend to be honest. If each DAC specific backend was to have a unique compatible, would the type of bus used be determinable from it? Doesn't have to work for all devices from now until the heath death of the universe, but at least for the devices that you're currently aware of? > > If, as you mentioned, there are multiple bus types, a non-flag property > > does make sense. However, I am really not keen on these "forced" numerical > > properties at all, I'd much rather see strings used here. > > > + maxItems: 1 > > > + description: | > > > + Configure bus type: > > > + - 0: none > > > + - 1: qspi Also, re-reading the cover letter, it says "this platform driver uses a 4 lanes parallel bus, plus a clock line, similar to a qspi." I don't think we should call this "qspi" if it is not actually qspi, that's just confusing. Cheers, Conor. > > > + enum: [0, 1] > > > + default: 0 > > > + > > > '#io-backend-cells': > > > const: 0 > > > > > > -- > > > 2.45.0.rc1 > > > > -- > ,,, Angelo Dureghello > :: :. BayLibre -runtime team- Developer > :`___: > `____: >
Hi Conor, On 30/08/24 5:33 PM, Conor Dooley wrote: > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: >> Hi Conor, >> >> On 29/08/24 5:46 PM, Conor Dooley wrote: >>> On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: >>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>> >>>> Add bus property. >>> RFC it may be, but you do need to explain what this bus-type actually >>> describes for commenting on the suitability of the method to be >>> meaningful. >> thanks for the feedbacks, >> >> a "bus" is intended as a generic interface connected to the target, >> may be used from a custom IP (fpga) to communicate with the target >> device (by read/write(reg and value)) using a special custom interface. >> >> The bus could also be physically the same of some well-known existing >> interfaces (as parallel, lvds or other uncommon interfaces), but using >> an uncommon/custom protocol over it. >> >> In concrete, actually bus-type is added to the backend since the >> ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR >> parallel bus (interface that i named QSPI, but it's not exactly a QSPI >> as a protocol), so it's a device-specific interface. >> >> With additions in this patchset, other frontends, of course not only >> DACs, will be able to add specific busses and read/wrtie to the bus >> as needed. >> >>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >>>> --- >>>> Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >>>> index a55e9bfc66d7..a7ce72e1cd81 100644 >>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >>>> @@ -38,6 +38,15 @@ properties: >>>> clocks: >>>> maxItems: 1 >>> You mentioned about new compatible strings, does the one currently >>> listed in this binding support both bus types? > You didn't answer this, and there's insufficient explanation of the > "hardware" in this RFC, but I found this which is supposedly the > backend: > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > adi,axi-dac.yaml has a single compatible, and that compatible has > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > expect either justification for reuse of the compatible, or a brand new > compatible for this backend, even if the driver can mostly be reused. > > Could you please link to whatever ADI wiki has detailed information on > how this stuff works so that I can look at it to better understand the > axes of configuration here? https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html that has same structure and register set of the generic ADI AXI-DAC IP: https://wiki.analog.com/resources/fpga/docs/axi_dac_ip >>> Making the bus type decision based on compatible only really makes sense >>> if they're different versions of the IP, but not if they're different >>> configuration options for a given version. >>> >>>> + bus-type: >> DAC IP on fpga actually respects same structure and register set, except >> for a named "custom" register that may use specific bitfields depending >> on the application of the IP. > To paraphrase: > "The register map is the same, except for the bit that is different". > If ADI is shipping several different configurations of this IP for > different DACs, I'd be expecting different compatibles for each backend > to be honest i am still quite new to this fpga-based implementations, at least for how such IPs are actually interfacing to the linux subsystem, so i may miss some point. About the "adi,axi-dac-9.1.b" compatible, the generic DAC IP register set is mostly the same structure of this ad3552r IP (links above), except for bitfields in the DAC_CUSTOM_CTRL register. My choice for now was to add a bus-type property. Not an HDL expert, but i think a different bus means, from an hardware point of view, a different IP in terms of internal fpga circuitry, even if not as a register-set. > . > If each DAC specific backend was to have a unique compatible, would the > type of bus used be determinable from it? Doesn't have to work for all > devices from now until the heath death of the universe, but at least for > the devices that you're currently aware of? > >>> If, as you mentioned, there are multiple bus types, a non-flag property >>> does make sense. However, I am really not keen on these "forced" numerical >>> properties at all, I'd much rather see strings used here. >>>> + maxItems: 1 >>>> + description: | >>>> + Configure bus type: >>>> + - 0: none >>>> + - 1: qspi > Also, re-reading the cover letter, it says "this platform driver uses a 4 > lanes parallel bus, plus a clock line, similar to a qspi." > I don't think we should call this "qspi" if it is not actually qspi, > that's just confusing. Agree, name should be something different. > Cheers, > Conor. Thanks, regards, Angelo >>>> + enum: [0, 1] >>>> + default: 0 >>>> + >>>> '#io-backend-cells': >>>> const: 0 >>>> >>>> -- >>>> 2.45.0.rc1 >>>> >> -- >> ,,, Angelo Dureghello >> :: :. BayLibre -runtime team- Developer >> :`___: >> `____: >>
On Mon, 2 Sep 2024 11:32:37 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > Hi Conor, > > > On 30/08/24 5:33 PM, Conor Dooley wrote: > > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > >> Hi Conor, > >> > >> On 29/08/24 5:46 PM, Conor Dooley wrote: > >>> On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > >>>> From: Angelo Dureghello <adureghello@baylibre.com> > >>>> > >>>> Add bus property. > >>> RFC it may be, but you do need to explain what this bus-type actually > >>> describes for commenting on the suitability of the method to be > >>> meaningful. > >> thanks for the feedbacks, > >> > >> a "bus" is intended as a generic interface connected to the target, > >> may be used from a custom IP (fpga) to communicate with the target > >> device (by read/write(reg and value)) using a special custom interface. > >> > >> The bus could also be physically the same of some well-known existing > >> interfaces (as parallel, lvds or other uncommon interfaces), but using > >> an uncommon/custom protocol over it. > >> > >> In concrete, actually bus-type is added to the backend since the > >> ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > >> parallel bus (interface that i named QSPI, but it's not exactly a QSPI > >> as a protocol), so it's a device-specific interface. > >> > >> With additions in this patchset, other frontends, of course not only > >> DACs, will be able to add specific busses and read/wrtie to the bus > >> as needed. > >> > >>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > >>>> --- > >>>> Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ > >>>> 1 file changed, 9 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > >>>> index a55e9bfc66d7..a7ce72e1cd81 100644 > >>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > >>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > >>>> @@ -38,6 +38,15 @@ properties: > >>>> clocks: > >>>> maxItems: 1 > >>> You mentioned about new compatible strings, does the one currently > >>> listed in this binding support both bus types? > > You didn't answer this, and there's insufficient explanation of the > > "hardware" in this RFC, but I found this which is supposedly the > > backend: > > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > > adi,axi-dac.yaml has a single compatible, and that compatible has > > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > > expect either justification for reuse of the compatible, or a brand new > > compatible for this backend, even if the driver can mostly be reused. > > > > Could you please link to whatever ADI wiki has detailed information on > > how this stuff works so that I can look at it to better understand the > > axes of configuration here? > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html > > that has same structure and register set of the generic ADI AXI-DAC IP: > https://wiki.analog.com/resources/fpga/docs/axi_dac_ip > > > >>> Making the bus type decision based on compatible only really makes sense > >>> if they're different versions of the IP, but not if they're different > >>> configuration options for a given version. > >>> > >>>> + bus-type: > >> DAC IP on fpga actually respects same structure and register set, except > >> for a named "custom" register that may use specific bitfields depending > >> on the application of the IP. > > To paraphrase: > > "The register map is the same, except for the bit that is different". > > If ADI is shipping several different configurations of this IP for > > different DACs, I'd be expecting different compatibles for each backend > > to be honest > > i am still quite new to this fpga-based implementations, at least for how > such IPs are actually interfacing to the linux subsystem, so i may miss > some point. > > About the "adi,axi-dac-9.1.b" compatible, the generic DAC IP register set > is mostly the same structure of this ad3552r IP (links above), except for > bitfields in the DAC_CUSTOM_CTRL register. > > My choice for now was to add a bus-type property. > > Not an HDL expert, but i think a different bus means, from an hardware > point of > view, a different IP in terms of internal fpga circuitry, even if not as a > register-set. Whilst I'm not sure we should be hiding the bus element away (rather than doing something closer to an spi bus + offloads) if we do end up like this maybe encode the bus type in the compatible. It's definitely the adi,axi-dac-qspi* specific variant if there isn't an explicit register to program to tell it not use a parallel bus or similar. Jonathan > > > > . > > If each DAC specific backend was to have a unique compatible, would the > > type of bus used be determinable from it? Doesn't have to work for all > > devices from now until the heath death of the universe, but at least for > > the devices that you're currently aware of? > > > >>> If, as you mentioned, there are multiple bus types, a non-flag property > >>> does make sense. However, I am really not keen on these "forced" numerical > >>> properties at all, I'd much rather see strings used here. > >>>> + maxItems: 1 > >>>> + description: | > >>>> + Configure bus type: > >>>> + - 0: none > >>>> + - 1: qspi > > Also, re-reading the cover letter, it says "this platform driver uses a 4 > > lanes parallel bus, plus a clock line, similar to a qspi." > > I don't think we should call this "qspi" if it is not actually qspi, > > that's just confusing. > > Agree, name should be something different. > > > > Cheers, > > Conor. > > Thanks, > regards, > > Angelo > > > >>>> + enum: [0, 1] > >>>> + default: 0 > >>>> + > >>>> '#io-backend-cells': > >>>> const: 0 > >>>> > >>>> -- > >>>> 2.45.0.rc1 > >>>> > >> -- > >> ,,, Angelo Dureghello > >> :: :. BayLibre -runtime team- Developer > >> :`___: > >> `____: > >>
On Fri, 2024-08-30 at 16:33 +0100, Conor Dooley wrote: > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > > Hi Conor, > > > > On 29/08/24 5:46 PM, Conor Dooley wrote: > > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > Add bus property. > > > RFC it may be, but you do need to explain what this bus-type actually > > > describes for commenting on the suitability of the method to be > > > meaningful. > > > > thanks for the feedbacks, > > > > a "bus" is intended as a generic interface connected to the target, > > may be used from a custom IP (fpga) to communicate with the target > > device (by read/write(reg and value)) using a special custom interface. > > > > The bus could also be physically the same of some well-known existing > > interfaces (as parallel, lvds or other uncommon interfaces), but using > > an uncommon/custom protocol over it. > > > > In concrete, actually bus-type is added to the backend since the > > ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > > parallel bus (interface that i named QSPI, but it's not exactly a QSPI > > as a protocol), so it's a device-specific interface. > > > > With additions in this patchset, other frontends, of course not only > > DACs, will be able to add specific busses and read/wrtie to the bus > > as needed. > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > --- > > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 > > > > +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > @@ -38,6 +38,15 @@ properties: > > > > clocks: > > > > maxItems: 1 > > > You mentioned about new compatible strings, does the one currently > > > listed in this binding support both bus types? > > You didn't answer this, and there's insufficient explanation of the > "hardware" in this RFC, but I found this which is supposedly the > backend: > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > adi,axi-dac.yaml has a single compatible, and that compatible has > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > expect either justification for reuse of the compatible, or a brand new > compatible for this backend, even if the driver can mostly be reused. > Hi Conor, So most of these designs have some changes (even if minimal) in the register map and the idea (mine actually) with this backend stuff was to keep the backend driver (axi-dac/adc) with the generic compatible since all the (different) functionality is basically defined by the frontend they connect too and that functionality is modeled by IIO backend ops. For some more significant/fundamental differences in the IP like this bus controller kind of thing, we would add have proper FW properties. The main idea was kind of using the frontend + generic backend combo so no need for new compatibles for every new design. It's still early days (at least upstream) for these IP cores and the backend code so if you say that we should have new compatibles for every new design that has some differences in the register map (even if minimal), I'm of course fine with it. I've done it like this because I was (am) kind of afraid for things to get complicated fairly quickly both in the bindings and driver (well maybe not in the driver). OTOH, it can simplify things a lot as it's way easier to identify different implementations of the IP directly in the driver so we have way more flexibility. > Could you please link to whatever ADI wiki has detailed information on > how this stuff works so that I can look at it to better understand the > axes of configuration here? > > > > > > > Making the bus type decision based on compatible only really makes sense > > > if they're different versions of the IP, but not if they're different > > > configuration options for a given version. > > > > > > > + bus-type: > > > > DAC IP on fpga actually respects same structure and register set, except > > for a named "custom" register that may use specific bitfields depending > > on the application of the IP. > > To paraphrase: > "The register map is the same, except for the bit that is different". > If ADI is shipping several different configurations of this IP for > different DACs, I'd be expecting different compatibles for each backend > to be honest. Yes, pretty much we have a generic core with most of the designs being based on it but with some slight differences. At least for the new ones, almost all of them have slight deviations from the generic/base core. > If each DAC specific backend was to have a unique compatible, would the > type of bus used be determinable from it? Doesn't have to work for all > devices from now until the heath death of the universe, but at least for > the devices that you're currently aware of? > My original idea was to have a bus controller boolean for this core at least for now that we only have one bus type (so we could assume qspi in the driver). If the time comes we need to add support for something else, then we would need another property to identify the type. > > > If, as you mentioned, there are multiple bus types, a non-flag property > > > does make sense. However, I am really not keen on these "forced" numerical > > > properties at all, I'd much rather see strings used here. > > > > > + maxItems: 1 > > > > + description: | > > > > + Configure bus type: > > > > + - 0: none > > > > + - 1: qspi > > Also, re-reading the cover letter, it says "this platform driver uses a 4 > lanes parallel bus, plus a clock line, similar to a qspi." > I don't think we should call this "qspi" if it is not actually qspi, > that's just confusing. > Just by looking at the datasheet it feels like typical qspi to be honest. And, fwiw, even if not really qspi, this is how the datasheet names the interface. - Nuno Sá > >
On Thu, Sep 05, 2024 at 11:50:45AM +0200, Nuno Sá wrote: > On Fri, 2024-08-30 at 16:33 +0100, Conor Dooley wrote: > > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > > > Hi Conor, > > > > > > On 29/08/24 5:46 PM, Conor Dooley wrote: > > > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > Add bus property. > > > > RFC it may be, but you do need to explain what this bus-type actually > > > > describes for commenting on the suitability of the method to be > > > > meaningful. > > > > > > thanks for the feedbacks, > > > > > > a "bus" is intended as a generic interface connected to the target, > > > may be used from a custom IP (fpga) to communicate with the target > > > device (by read/write(reg and value)) using a special custom interface. > > > > > > The bus could also be physically the same of some well-known existing > > > interfaces (as parallel, lvds or other uncommon interfaces), but using > > > an uncommon/custom protocol over it. > > > > > > In concrete, actually bus-type is added to the backend since the > > > ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > > > parallel bus (interface that i named QSPI, but it's not exactly a QSPI > > > as a protocol), so it's a device-specific interface. > > > > > > With additions in this patchset, other frontends, of course not only > > > DACs, will be able to add specific busses and read/wrtie to the bus > > > as needed. > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > --- > > > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 > > > > > +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > @@ -38,6 +38,15 @@ properties: > > > > > clocks: > > > > > maxItems: 1 > > > > You mentioned about new compatible strings, does the one currently > > > > listed in this binding support both bus types? > > > > You didn't answer this, and there's insufficient explanation of the > > "hardware" in this RFC, but I found this which is supposedly the > > backend: > > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > > adi,axi-dac.yaml has a single compatible, and that compatible has > > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > > expect either justification for reuse of the compatible, or a brand new > > compatible for this backend, even if the driver can mostly be reused. > > > > Hi Conor, > > So most of these designs have some changes (even if minimal) in the register map > and the idea (mine actually) with this backend stuff was to keep the backend > driver (axi-dac/adc) with the generic compatible since all the (different) > functionality is basically defined by the frontend they connect too and that > functionality is modeled by IIO backend ops. For some more > significant/fundamental differences in the IP like this bus controller kind of > thing, we would add have proper FW properties. The main idea was kind of using > the frontend + generic backend combo so no need for new compatibles for every > new design. > > It's still early days (at least upstream) for these IP cores and the backend > code so if you say that we should have new compatibles for every new design that > has some differences in the register map (even if minimal), I'm of course fine > with it. I've done it like this because I was (am) kind of afraid for things to > get complicated fairly quickly both in the bindings and driver (well maybe not > in the driver). OTOH, it can simplify things a lot as it's way easier to > identify different implementations of the IP directly in the driver so we have > way more flexibility. Most of my opinion on this from a usability perspective for your customers, rather than how the kernel is going to handle it. If a user is inserting a preconfigured instance of the IP, for a specific ADC or DAC, into their design I think it makes more sense to have a compatible, rather than expect the user to reverse engineer how the IP has been configured and which properties they should select. My own policy for Microchip's stuff is that if something has a name or entry in the IP catalogue then it should have a dedicated compatible, even if it is just a preconfigured version of some other IP block and I guess what I am saying here is an extension of that. I suspect that in many cases the specific compatible won't be required, and a fallback to the generic one will suffice for the driver, and it would only be for cases like this, that have "significant/fundamental differences" that the driver would need the specific one. > > > Could you please link to whatever ADI wiki has detailed information on > > how this stuff works so that I can look at it to better understand the > > axes of configuration here? > > > > > > > > > > Making the bus type decision based on compatible only really makes sense > > > > if they're different versions of the IP, but not if they're different > > > > configuration options for a given version. > > > > > > > > > + bus-type: > > > > > > DAC IP on fpga actually respects same structure and register set, except > > > for a named "custom" register that may use specific bitfields depending > > > on the application of the IP. > > > > To paraphrase: > > "The register map is the same, except for the bit that is different". > > If ADI is shipping several different configurations of this IP for > > different DACs, I'd be expecting different compatibles for each backend > > to be honest. > > Yes, pretty much we have a generic core with most of the designs being based on > it but with some slight differences. At least for the new ones, almost all of > them have slight deviations from the generic/base core. > > > If each DAC specific backend was to have a unique compatible, would the > > type of bus used be determinable from it? Doesn't have to work for all > > devices from now until the heath death of the universe, but at least for > > the devices that you're currently aware of? > > > > My original idea was to have a bus controller boolean for this core at least for > now that we only have one bus type (so we could assume qspi in the driver). If > the time comes we need to add support for something else, then we would need > another property to identify the type. With a specific compatible, you can "easily" add different defaults. So the other devices could default to no bus when a bus related property is required and this one could default to qspi. But unless there are ADCs/DACs that have a backend that can be configured with different types of bus, a property for this wouldn't be needed - the compatible and match data would suffice. > > > > > If, as you mentioned, there are multiple bus types, a non-flag property > > > > does make sense. However, I am really not keen on these "forced" numerical > > > > properties at all, I'd much rather see strings used here. > > > > > > > + maxItems: 1 > > > > > + description: | > > > > > + Configure bus type: > > > > > + - 0: none > > > > > + - 1: qspi > > > > Also, re-reading the cover letter, it says "this platform driver uses a 4 > > lanes parallel bus, plus a clock line, similar to a qspi." > > I don't think we should call this "qspi" if it is not actually qspi, > > that's just confusing. > > > > Just by looking at the datasheet it feels like typical qspi to be honest. And, > fwiw, even if not really qspi, this is how the datasheet names the interface. Right, just a phrasing issue in the cover letter I guess :)
On Fri, Sep 06, 2024 at 09:50:30AM +0100, Conor Dooley wrote: > On Thu, Sep 05, 2024 at 11:50:45AM +0200, Nuno Sá wrote: > > On Fri, 2024-08-30 at 16:33 +0100, Conor Dooley wrote: > > > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > > > > > > + maxItems: 1 > > > > > > + description: | > > > > > > + Configure bus type: > > > > > > + - 0: none > > > > > > + - 1: qspi > > > > > > Also, re-reading the cover letter, it says "this platform driver uses a 4 > > > lanes parallel bus, plus a clock line, similar to a qspi." > > > I don't think we should call this "qspi" if it is not actually qspi, > > > that's just confusing. > > > > > > > Just by looking at the datasheet it feels like typical qspi to be honest. And, > > fwiw, even if not really qspi, this is how the datasheet names the interface. > > Right, just a phrasing issue in the cover letter I guess :) The other thing that this brings into question, and I forget if I said it before (perhaps to David on IRC) was whether or not the ADC/DAC needs to be a child of the backend, if the backend is providing the SPI bus that the device is attached to. Why would it not be the case, if as you say, it appears to be a real qspi controller?
On Mon, Sep 02, 2024 at 11:32:37AM +0200, Angelo Dureghello wrote: > Hi Conor, > > > On 30/08/24 5:33 PM, Conor Dooley wrote: > > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > > > Hi Conor, > > > > > > On 29/08/24 5:46 PM, Conor Dooley wrote: > > > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > Add bus property. > > > > RFC it may be, but you do need to explain what this bus-type actually > > > > describes for commenting on the suitability of the method to be > > > > meaningful. > > > thanks for the feedbacks, > > > > > > a "bus" is intended as a generic interface connected to the target, > > > may be used from a custom IP (fpga) to communicate with the target > > > device (by read/write(reg and value)) using a special custom interface. > > > > > > The bus could also be physically the same of some well-known existing > > > interfaces (as parallel, lvds or other uncommon interfaces), but using > > > an uncommon/custom protocol over it. > > > > > > In concrete, actually bus-type is added to the backend since the > > > ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > > > parallel bus (interface that i named QSPI, but it's not exactly a QSPI > > > as a protocol), so it's a device-specific interface. > > > > > > With additions in this patchset, other frontends, of course not only > > > DACs, will be able to add specific busses and read/wrtie to the bus > > > as needed. > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > --- > > > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > @@ -38,6 +38,15 @@ properties: > > > > > clocks: > > > > > maxItems: 1 > > > > You mentioned about new compatible strings, does the one currently > > > > listed in this binding support both bus types? > > You didn't answer this, and there's insufficient explanation of the > > "hardware" in this RFC, but I found this which is supposedly the > > backend: > > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > > adi,axi-dac.yaml has a single compatible, and that compatible has > > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > > expect either justification for reuse of the compatible, or a brand new > > compatible for this backend, even if the driver can mostly be reused. > > > > Could you please link to whatever ADI wiki has detailed information on > > how this stuff works so that I can look at it to better understand the > > axes of configuration here? > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html > > that has same structure and register set of the generic ADI AXI-DAC IP: > https://wiki.analog.com/resources/fpga/docs/axi_dac_ip > > > > > > Making the bus type decision based on compatible only really makes sense > > > > if they're different versions of the IP, but not if they're different > > > > configuration options for a given version. > > > > > > > > > + bus-type: > > > DAC IP on fpga actually respects same structure and register set, except > > > for a named "custom" register that may use specific bitfields depending > > > on the application of the IP. > > To paraphrase: > > "The register map is the same, except for the bit that is different". > > If ADI is shipping several different configurations of this IP for > > different DACs, I'd be expecting different compatibles for each backend > > to be honest > > i am still quite new to this fpga-based implementations, at least for how > such IPs are actually interfacing to the linux subsystem, so i may miss > some point. > > About the "adi,axi-dac-9.1.b" compatible, the generic DAC IP register set > is mostly the same structure of this ad3552r IP (links above), except for > bitfields in the DAC_CUSTOM_CTRL register. > > My choice for now was to add a bus-type property. > > Not an HDL expert, but i think a different bus means, from an hardware point > of > view, a different IP in terms of internal fpga circuitry, even if not as a > register-set. Depending on whether or not the unmodified driver can be used with this IP (so the QSPI bus stuff would need to be optional) then a fallback should be used given the degree of similarity. It, however, seems likely that is not the case, and without the QSPI bus there'd be no way to communicate with the device. Is there any reason to use this IP as a backend, without connecting the QSPI bus at all, leaving the ADC/DAC on a regular SPI bus? > > > > . > > If each DAC specific backend was to have a unique compatible, would the > > type of bus used be determinable from it? Doesn't have to work for all > > devices from now until the heath death of the universe, but at least for > > the devices that you're currently aware of? > > > > > > If, as you mentioned, there are multiple bus types, a non-flag property > > > > does make sense. However, I am really not keen on these "forced" numerical > > > > properties at all, I'd much rather see strings used here. > > > > > + maxItems: 1 > > > > > + description: | > > > > > + Configure bus type: > > > > > + - 0: none > > > > > + - 1: qspi > > Also, re-reading the cover letter, it says "this platform driver uses a 4 > > lanes parallel bus, plus a clock line, similar to a qspi." > > I don't think we should call this "qspi" if it is not actually qspi, > > that's just confusing. > > Agree, name should be something different. Nuno's comment appears to disagree, and that is /is/ actually a qspi controller. Please see my comments to him about parentage.
On Fri, 2024-09-06 at 09:50 +0100, Conor Dooley wrote: > On Thu, Sep 05, 2024 at 11:50:45AM +0200, Nuno Sá wrote: > > On Fri, 2024-08-30 at 16:33 +0100, Conor Dooley wrote: > > > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > > > > Hi Conor, > > > > > > > > On 29/08/24 5:46 PM, Conor Dooley wrote: > > > > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > > > Add bus property. > > > > > RFC it may be, but you do need to explain what this bus-type actually > > > > > describes for commenting on the suitability of the method to be > > > > > meaningful. > > > > > > > > thanks for the feedbacks, > > > > > > > > a "bus" is intended as a generic interface connected to the target, > > > > may be used from a custom IP (fpga) to communicate with the target > > > > device (by read/write(reg and value)) using a special custom interface. > > > > > > > > The bus could also be physically the same of some well-known existing > > > > interfaces (as parallel, lvds or other uncommon interfaces), but using > > > > an uncommon/custom protocol over it. > > > > > > > > In concrete, actually bus-type is added to the backend since the > > > > ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > > > > parallel bus (interface that i named QSPI, but it's not exactly a QSPI > > > > as a protocol), so it's a device-specific interface. > > > > > > > > With additions in this patchset, other frontends, of course not only > > > > DACs, will be able to add specific busses and read/wrtie to the bus > > > > as needed. > > > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > > --- > > > > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 > > > > > > +++++++++ > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > @@ -38,6 +38,15 @@ properties: > > > > > > clocks: > > > > > > maxItems: 1 > > > > > You mentioned about new compatible strings, does the one currently > > > > > listed in this binding support both bus types? > > > > > > You didn't answer this, and there's insufficient explanation of the > > > "hardware" in this RFC, but I found this which is supposedly the > > > backend: > > > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > > > adi,axi-dac.yaml has a single compatible, and that compatible has > > > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > > > expect either justification for reuse of the compatible, or a brand new > > > compatible for this backend, even if the driver can mostly be reused. > > > > > > > Hi Conor, > > > > So most of these designs have some changes (even if minimal) in the register map > > and the idea (mine actually) with this backend stuff was to keep the backend > > driver (axi-dac/adc) with the generic compatible since all the (different) > > functionality is basically defined by the frontend they connect too and that > > functionality is modeled by IIO backend ops. For some more > > significant/fundamental differences in the IP like this bus controller kind of > > thing, we would add have proper FW properties. The main idea was kind of using > > the frontend + generic backend combo so no need for new compatibles for every > > new design. > > > > It's still early days (at least upstream) for these IP cores and the backend > > code so if you say that we should have new compatibles for every new design that > > has some differences in the register map (even if minimal), I'm of course fine > > with it. I've done it like this because I was (am) kind of afraid for things to > > get complicated fairly quickly both in the bindings and driver (well maybe not > > in the driver). OTOH, it can simplify things a lot as it's way easier to > > identify different implementations of the IP directly in the driver so we have > > way more flexibility. > > Most of my opinion on this from a usability perspective for your > customers, rather than how the kernel is going to handle it. If a user > is inserting a preconfigured instance of the IP, for a specific ADC or > DAC, into their design I think it makes more sense to have a compatible, > rather than expect the user to reverse engineer how the IP has been > configured and which properties they should select. My own policy for > Microchip's stuff is that if something has a name or entry in the IP > catalogue then it should have a dedicated compatible, even if it is just a > preconfigured version of some other IP block and I guess what I am > saying here is an extension of that. > Hmm, indeed the above makes sense... > I suspect that in many cases the specific compatible won't be required, > and a fallback to the generic one will suffice for the driver, and it > would only be for cases like this, that have "significant/fundamental > differences" that the driver would need the specific one. > Hopefully yes :) > > > > > Could you please link to whatever ADI wiki has detailed information on > > > how this stuff works so that I can look at it to better understand the > > > axes of configuration here? > > > > > > > > > > > > > Making the bus type decision based on compatible only really makes sense > > > > > if they're different versions of the IP, but not if they're different > > > > > configuration options for a given version. > > > > > > > > > > > + bus-type: > > > > > > > > DAC IP on fpga actually respects same structure and register set, except > > > > for a named "custom" register that may use specific bitfields depending > > > > on the application of the IP. > > > > > > To paraphrase: > > > "The register map is the same, except for the bit that is different". > > > If ADI is shipping several different configurations of this IP for > > > different DACs, I'd be expecting different compatibles for each backend > > > to be honest. > > > > Yes, pretty much we have a generic core with most of the designs being based on > > it but with some slight differences. At least for the new ones, almost all of > > them have slight deviations from the generic/base core. > > > > > If each DAC specific backend was to have a unique compatible, would the > > > type of bus used be determinable from it? Doesn't have to work for all > > > devices from now until the heath death of the universe, but at least for > > > the devices that you're currently aware of? > > > > > > > My original idea was to have a bus controller boolean for this core at least for > > now that we only have one bus type (so we could assume qspi in the driver). If > > the time comes we need to add support for something else, then we would need > > another property to identify the type. > > With a specific compatible, you can "easily" add different defaults. So > the other devices could default to no bus when a bus related property is > required and this one could default to qspi. But unless there are > ADCs/DACs that have a backend that can be configured with different > types of bus, a property for this wouldn't be needed - the compatible > and match data would suffice. > Agreed... - Nuno Sá > > > > >
On Fri, 2024-09-06 at 10:04 +0100, Conor Dooley wrote: > On Mon, Sep 02, 2024 at 11:32:37AM +0200, Angelo Dureghello wrote: > > Hi Conor, > > > > > > On 30/08/24 5:33 PM, Conor Dooley wrote: > > > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > > > > Hi Conor, > > > > > > > > On 29/08/24 5:46 PM, Conor Dooley wrote: > > > > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > > > Add bus property. > > > > > RFC it may be, but you do need to explain what this bus-type actually > > > > > describes for commenting on the suitability of the method to be > > > > > meaningful. > > > > thanks for the feedbacks, > > > > > > > > a "bus" is intended as a generic interface connected to the target, > > > > may be used from a custom IP (fpga) to communicate with the target > > > > device (by read/write(reg and value)) using a special custom interface. > > > > > > > > The bus could also be physically the same of some well-known existing > > > > interfaces (as parallel, lvds or other uncommon interfaces), but using > > > > an uncommon/custom protocol over it. > > > > > > > > In concrete, actually bus-type is added to the backend since the > > > > ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > > > > parallel bus (interface that i named QSPI, but it's not exactly a QSPI > > > > as a protocol), so it's a device-specific interface. > > > > > > > > With additions in this patchset, other frontends, of course not only > > > > DACs, will be able to add specific busses and read/wrtie to the bus > > > > as needed. > > > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > > --- > > > > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 > > > > > > +++++++++ > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > @@ -38,6 +38,15 @@ properties: > > > > > > clocks: > > > > > > maxItems: 1 > > > > > You mentioned about new compatible strings, does the one currently > > > > > listed in this binding support both bus types? > > > You didn't answer this, and there's insufficient explanation of the > > > "hardware" in this RFC, but I found this which is supposedly the > > > backend: > > > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > > > adi,axi-dac.yaml has a single compatible, and that compatible has > > > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > > > expect either justification for reuse of the compatible, or a brand new > > > compatible for this backend, even if the driver can mostly be reused. > > > > > > Could you please link to whatever ADI wiki has detailed information on > > > how this stuff works so that I can look at it to better understand the > > > axes of configuration here? > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html > > > > that has same structure and register set of the generic ADI AXI-DAC IP: > > https://wiki.analog.com/resources/fpga/docs/axi_dac_ip > > > > > > > > > Making the bus type decision based on compatible only really makes sense > > > > > if they're different versions of the IP, but not if they're different > > > > > configuration options for a given version. > > > > > > > > > > > + bus-type: > > > > DAC IP on fpga actually respects same structure and register set, except > > > > for a named "custom" register that may use specific bitfields depending > > > > on the application of the IP. > > > To paraphrase: > > > "The register map is the same, except for the bit that is different". > > > If ADI is shipping several different configurations of this IP for > > > different DACs, I'd be expecting different compatibles for each backend > > > to be honest > > > > i am still quite new to this fpga-based implementations, at least for how > > such IPs are actually interfacing to the linux subsystem, so i may miss > > some point. > > > > About the "adi,axi-dac-9.1.b" compatible, the generic DAC IP register set > > is mostly the same structure of this ad3552r IP (links above), except for > > bitfields in the DAC_CUSTOM_CTRL register. > > > > My choice for now was to add a bus-type property. > > > > Not an HDL expert, but i think a different bus means, from an hardware point > > of > > view, a different IP in terms of internal fpga circuitry, even if not as a > > register-set. > > Depending on whether or not the unmodified driver can be used with this > IP (so the QSPI bus stuff would need to be optional) then a fallback > should be used given the degree of similarity. It, however, seems likely > that is not the case, and without the QSPI bus there'd be no way to > communicate with the device. Is there any reason to use this IP as a > backend, without connecting the QSPI bus at all, leaving the ADC/DAC on > a regular SPI bus? > Somewhere in my replies, I'm doing the exact same question to myself. We probably need to speak with the FPGA folks but I guess (hope) they had a good reason for this. - Nuno Sá
On 06/09/24 1:32 PM, Nuno Sá wrote: > On Fri, 2024-09-06 at 10:04 +0100, Conor Dooley wrote: >> On Mon, Sep 02, 2024 at 11:32:37AM +0200, Angelo Dureghello wrote: >>> Hi Conor, >>> >>> >>> On 30/08/24 5:33 PM, Conor Dooley wrote: >>>> On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: >>>>> Hi Conor, >>>>> >>>>> On 29/08/24 5:46 PM, Conor Dooley wrote: >>>>>> On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: >>>>>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>>>>> >>>>>>> Add bus property. >>>>>> RFC it may be, but you do need to explain what this bus-type actually >>>>>> describes for commenting on the suitability of the method to be >>>>>> meaningful. >>>>> thanks for the feedbacks, >>>>> >>>>> a "bus" is intended as a generic interface connected to the target, >>>>> may be used from a custom IP (fpga) to communicate with the target >>>>> device (by read/write(reg and value)) using a special custom interface. >>>>> >>>>> The bus could also be physically the same of some well-known existing >>>>> interfaces (as parallel, lvds or other uncommon interfaces), but using >>>>> an uncommon/custom protocol over it. >>>>> >>>>> In concrete, actually bus-type is added to the backend since the >>>>> ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR >>>>> parallel bus (interface that i named QSPI, but it's not exactly a QSPI >>>>> as a protocol), so it's a device-specific interface. >>>>> >>>>> With additions in this patchset, other frontends, of course not only >>>>> DACs, will be able to add specific busses and read/wrtie to the bus >>>>> as needed. >>>>> >>>>>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >>>>>>> --- >>>>>>> Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 >>>>>>> +++++++++ >>>>>>> 1 file changed, 9 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >>>>>>> b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >>>>>>> index a55e9bfc66d7..a7ce72e1cd81 100644 >>>>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml >>>>>>> @@ -38,6 +38,15 @@ properties: >>>>>>> clocks: >>>>>>> maxItems: 1 >>>>>> You mentioned about new compatible strings, does the one currently >>>>>> listed in this binding support both bus types? >>>> You didn't answer this, and there's insufficient explanation of the >>>> "hardware" in this RFC, but I found this which is supposedly the >>>> backend: >>>> https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r >>>> adi,axi-dac.yaml has a single compatible, and that compatible has >>>> nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would >>>> expect either justification for reuse of the compatible, or a brand new >>>> compatible for this backend, even if the driver can mostly be reused. >>>> >>>> Could you please link to whatever ADI wiki has detailed information on >>>> how this stuff works so that I can look at it to better understand the >>>> axes of configuration here? >>> https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html >>> >>> that has same structure and register set of the generic ADI AXI-DAC IP: >>> https://wiki.analog.com/resources/fpga/docs/axi_dac_ip >>> >>> >>>>>> Making the bus type decision based on compatible only really makes sense >>>>>> if they're different versions of the IP, but not if they're different >>>>>> configuration options for a given version. >>>>>> >>>>>>> + bus-type: >>>>> DAC IP on fpga actually respects same structure and register set, except >>>>> for a named "custom" register that may use specific bitfields depending >>>>> on the application of the IP. >>>> To paraphrase: >>>> "The register map is the same, except for the bit that is different". >>>> If ADI is shipping several different configurations of this IP for >>>> different DACs, I'd be expecting different compatibles for each backend >>>> to be honest >>> i am still quite new to this fpga-based implementations, at least for how >>> such IPs are actually interfacing to the linux subsystem, so i may miss >>> some point. >>> >>> About the "adi,axi-dac-9.1.b" compatible, the generic DAC IP register set >>> is mostly the same structure of this ad3552r IP (links above), except for >>> bitfields in the DAC_CUSTOM_CTRL register. >>> >>> My choice for now was to add a bus-type property. >>> >>> Not an HDL expert, but i think a different bus means, from an hardware point >>> of >>> view, a different IP in terms of internal fpga circuitry, even if not as a >>> register-set. >> Depending on whether or not the unmodified driver can be used with this >> IP (so the QSPI bus stuff would need to be optional) then a fallback >> should be used given the degree of similarity. It, however, seems likely >> that is not the case, and without the QSPI bus there'd be no way to >> communicate with the device. Is there any reason to use this IP as a >> backend, without connecting the QSPI bus at all, leaving the ADC/DAC on >> a regular SPI bus? >> > Somewhere in my replies, I'm doing the exact same question to myself. We probably > need to speak with the FPGA folks but I guess (hope) they had a good reason for this. > > - Nuno Sá to clarify a bit the custom (fpga-based) QSPI need, i did some checks in the datasheets: 1. ADI is actually supporting ad3552r by eval-ad3552r-fmcx eval boards, with specific fmc connector for the ZedBoard (zynq7000). This is the current focused hardware for this job. 2. Zynq7000 std non-fpga controller is designed to control flash memories, but can operate in "raw I/O" mode, so it may work with ad3552r, but is not supporting DDR, even if it may reach 100Mhz clock. 3. ad3552r accepts a maximum clock of 66Mhz. So for ZedBoard maximum speed of 33MUPS cannot be reached without DDR. 4. ad3552r requires DDR only in the data part, and in DDR mode we may also send some "non-loop" reg read/write, so requiring also the address to be sent in SDR. Not sure how many QSPI controllers in the market are working this way, even if it seems quite standard, looks like not many are actually supporting DDR. There may be, but not actually in the priority of my customer right now. And in that case, we could extend the generic spi ad3552r.c. Regards,
On Sat, Sep 07, 2024 at 10:53:07AM +0200, Angelo Dureghello wrote: > > On 06/09/24 1:32 PM, Nuno Sá wrote: > > On Fri, 2024-09-06 at 10:04 +0100, Conor Dooley wrote: > > > On Mon, Sep 02, 2024 at 11:32:37AM +0200, Angelo Dureghello wrote: > > > > Hi Conor, > > > > > > > > > > > > On 30/08/24 5:33 PM, Conor Dooley wrote: > > > > > On Fri, Aug 30, 2024 at 10:19:49AM +0200, Angelo Dureghello wrote: > > > > > > Hi Conor, > > > > > > > > > > > > On 29/08/24 5:46 PM, Conor Dooley wrote: > > > > > > > On Thu, Aug 29, 2024 at 02:32:02PM +0200, Angelo Dureghello wrote: > > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > > > > > > > Add bus property. > > > > > > > RFC it may be, but you do need to explain what this bus-type actually > > > > > > > describes for commenting on the suitability of the method to be > > > > > > > meaningful. > > > > > > thanks for the feedbacks, > > > > > > > > > > > > a "bus" is intended as a generic interface connected to the target, > > > > > > may be used from a custom IP (fpga) to communicate with the target > > > > > > device (by read/write(reg and value)) using a special custom interface. > > > > > > > > > > > > The bus could also be physically the same of some well-known existing > > > > > > interfaces (as parallel, lvds or other uncommon interfaces), but using > > > > > > an uncommon/custom protocol over it. > > > > > > > > > > > > In concrete, actually bus-type is added to the backend since the > > > > > > ad3552r DAC chip can be connected (for maximum speed) by a 5 lanes DDR > > > > > > parallel bus (interface that i named QSPI, but it's not exactly a QSPI > > > > > > as a protocol), so it's a device-specific interface. > > > > > > > > > > > > With additions in this patchset, other frontends, of course not only > > > > > > DACs, will be able to add specific busses and read/wrtie to the bus > > > > > > as needed. > > > > > > > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > --- > > > > > > > > Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml | 9 > > > > > > > > +++++++++ > > > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > > > index a55e9bfc66d7..a7ce72e1cd81 100644 > > > > > > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml > > > > > > > > @@ -38,6 +38,15 @@ properties: > > > > > > > > clocks: > > > > > > > > maxItems: 1 > > > > > > > You mentioned about new compatible strings, does the one currently > > > > > > > listed in this binding support both bus types? > > > > > You didn't answer this, and there's insufficient explanation of the > > > > > "hardware" in this RFC, but I found this which is supposedly the > > > > > backend: > > > > > https://github.com/analogdevicesinc/hdl/tree/main/library/axi_ad3552r > > > > > adi,axi-dac.yaml has a single compatible, and that compatible has > > > > > nothing to do with "axi_ad3552r" as it is "adi,axi-dac-9.1.b". I would > > > > > expect either justification for reuse of the compatible, or a brand new > > > > > compatible for this backend, even if the driver can mostly be reused. > > > > > > > > > > Could you please link to whatever ADI wiki has detailed information on > > > > > how this stuff works so that I can look at it to better understand the > > > > > axes of configuration here? > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html > > > > > > > > that has same structure and register set of the generic ADI AXI-DAC IP: > > > > https://wiki.analog.com/resources/fpga/docs/axi_dac_ip > > > > > > > > > > > > > > > Making the bus type decision based on compatible only really makes sense > > > > > > > if they're different versions of the IP, but not if they're different > > > > > > > configuration options for a given version. > > > > > > > > > > > > > > > + bus-type: > > > > > > DAC IP on fpga actually respects same structure and register set, except > > > > > > for a named "custom" register that may use specific bitfields depending > > > > > > on the application of the IP. > > > > > To paraphrase: > > > > > "The register map is the same, except for the bit that is different". > > > > > If ADI is shipping several different configurations of this IP for > > > > > different DACs, I'd be expecting different compatibles for each backend > > > > > to be honest > > > > i am still quite new to this fpga-based implementations, at least for how > > > > such IPs are actually interfacing to the linux subsystem, so i may miss > > > > some point. > > > > > > > > About the "adi,axi-dac-9.1.b" compatible, the generic DAC IP register set > > > > is mostly the same structure of this ad3552r IP (links above), except for > > > > bitfields in the DAC_CUSTOM_CTRL register. > > > > > > > > My choice for now was to add a bus-type property. > > > > > > > > Not an HDL expert, but i think a different bus means, from an hardware point > > > > of > > > > view, a different IP in terms of internal fpga circuitry, even if not as a > > > > register-set. > > > > > > Depending on whether or not the unmodified driver can be used with this > > > IP (so the QSPI bus stuff would need to be optional) then a fallback > > > should be used given the degree of similarity. It, however, seems likely > > > that is not the case, and without the QSPI bus there'd be no way to > > > communicate with the device. Is there any reason to use this IP as a > > > backend, without connecting the QSPI bus at all, leaving the ADC/DAC on > > > a regular SPI bus? > > > > > Somewhere in my replies, I'm doing the exact same question to myself. We probably > > need to speak with the FPGA folks but I guess (hope) they had a good reason for this. > > to clarify a bit the custom (fpga-based) QSPI need, i did some checks in the > datasheets: > > 1. ADI is actually supporting ad3552r by eval-ad3552r-fmcx eval boards, > with specific fmc connector for the ZedBoard (zynq7000). This is the > current focused hardware for this job. "currently focused" being the key words! Since it is FPGA IP, you've got no control over where it is being used, so the particular use case you're developing for is not really that important. > 2. Zynq7000 std non-fpga controller is designed to control flash memories, > but can operate in "raw I/O" mode, so it may work with ad3552r, but is not > supporting DDR, even if it may reach 100Mhz clock. > > 3. ad3552r accepts a maximum clock of 66Mhz. So for ZedBoard maximum speed > of 33MUPS cannot be reached without DDR. > > 4. ad3552r requires DDR only in the data part, and in DDR mode we > may also send some "non-loop" reg read/write, so requiring also the > address to be sent in SDR. Not sure how many QSPI controllers in the market > are > working this way, even if it seems quite standard, looks like not many are > actually > supporting DDR. There may be, but not actually in the priority of my > customer > right now. And in that case, we could extend the generic spi ad3552r.c. I think you need to ignore your use case here, and just consider whether or not this IP can be used as a backend without the QSPI feature. That's probably an easier thing to determine than whether or not there's another controller out there that can satisfy the constraints. The docs say it "interfaces", but that's such a generic word that it ultimately means close to nothing.. There's the sync ability, but from my reading of the github.io page, it doesn't do anything when the axi-dac is not actually in the data path. Cheers, Conor.
diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml index a55e9bfc66d7..a7ce72e1cd81 100644 --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml @@ -38,6 +38,15 @@ properties: clocks: maxItems: 1 + bus-type: + maxItems: 1 + description: | + Configure bus type: + - 0: none + - 1: qspi + enum: [0, 1] + default: 0 + '#io-backend-cells': const: 0