On Mon, Jun 02, 2025 at 12:23:40PM -0500, David Lechner wrote:
> On 6/2/25 11:32 AM, Jorge Marques wrote:
> > Hi David,
> >
> > On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote:
> >> On 6/2/25 4:17 AM, Jorge Marques wrote:
> >>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote:
> >>>> On 4/29/25 8:48 AM, Jorge Marques wrote:
> >>>>> Hi David,
> >>>>>
> >>>>> I didn't went through your's and Jonathan's ad4052.c review yet,
> >>>>> but for the trigger-source-cells I need to dig deeper and make
> >>>>> considerable changes to the driver, as well as hardware tests.
> >>>>> My idea was to have a less customizable driver, but I get that it is
> >>>>> more interesting to make it user-definable.
> >>>>
> >>>> We don't need to make the driver support all possibilities, but the devicetree
> >>>> needs to be as complete as possible since it can't be as easily changed in the
> >>>> future.
> >>>>
> >>>
> >>> Ack.
> >>>
> >>> I see that the node goes in the spi controller (the parent). To use the
> >>> same information in the driver I need to look-up the parent node, then
> >>> the node. I don't plan to do that in the version of the driver, just an
> >>> observation.
> >>>
> >>> There is something else I want to discuss on the dt-bindings actually.
> >>> According to the schema, the spi-max-frequency is:
> >>>
> >>> > Maximum SPI clocking speed of the device in Hz.
> >>>
> >>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode
> >>> (higher, depends on VIO). The solution I came up, to not require a
> >>> custom regmap spi bus, is to have spi-max-frequency bound the
> >>> Configuration mode speed,
> >>
> >> The purpose of spi-max-frequency in the devicetree is that sometimes
> >> the wiring of a complete system makes the effective max frequency
> >> lower than what is allowed by the datasheet. So this really needs
> >> to be the absolute highest frequency allowed.
> >>
> >>> and have ADC Mode set by VIO regulator
> >>> voltage, through spi_transfer.speed_hz. At the end of the day, both are
> >>> bounded by the spi controller maximum speed.
> >>
> >> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code
> >> uses spi-max-frequency. So I don't think this would actually work.
> >>
> > Ok, so that's something that may be worth some attention.
> >
> > At spi/spi.c#2472
> > if (!of_property_read_u32(nc, "spi-max-frequency", &value))
> > spi->max_speed_hz = value;
> >
> > At spi/spi.c#4090
> > if (!xfer->speed_hz)
> > xfer->speed_hz = spi->max_speed_hz;
> >
> > So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and
> > not bounded by it.
>
> Ah, OK, my memory was wrong. It is only bound by the controller max
> speed, not the device max speed.
>
> if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz)
> xfer->speed_hz = ctlr->max_speed_hz;
>
> It does seem odd that it would allow setting an individual xfer
> speed higher than than the given device max speed. I suppose we
> could submit a patch adding that check to the SPI core code and
> see what Mark has to say.
>
Agreed, the patch itself would be simple:
if (!xfer->speed_hz || xfer->speed_hz > spi->max_speed_hz)
xfer->speed_hz = spi->max_speed_hz;
But I wonder how many drivers rely on this behaviour
> >
> > Then at spi-axi-spi-engine.c:
> >
> > static int spi_engine_precompile_message(struct spi_message *msg)
> > {
> > clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz);
> > xfer->effective_speed_hz = max_hz / min(clk_div, 256U);
> > }
> >
> > Where max_hz is set only by the IP spi_clk. If at the driver I set
> > xfer.speed_hz, it won't be bounded by max-spi-frequency.
> >
> > The only that seems to bound as described is the layer for flash memory
> > at spi-mem.c@spi_mem_adjust_op_freq.
> >
> > For the adc driver, I will then consider your behavioral description and
> > create a custom regmap bus to limit set the reg access speed (fixed),
> > and keep adc mode speed set by VIO. And consider spi-max-frequency can
> > further reduce both speeds.
> > (or should instead be handled at the driver like spi-mem.c ?)
>
> It would be more work, but if it is common enough, we could generalize this
> in the core code. For example add a spi-register-max-frequency binding (or
> even a more general spi-max-freqency-map to map operations to max frequencies).
> Then we could bake it into the regmap_spi code to handle this property
> and not have to make a separate bus.
>
> FWIW, there are also some SPI TFT displays that use a different frequency
> for register access compared to framebuffer data that could potentially
> use this too. Right now, these just have a hard-coded register access
> frequency of e.g. 10 MHz.
>
I implemented the custom regmap bus for this series.
With a `spi-max-frequency-map`, the regmap bus can be removed.
I don't want to include this regmap spi patch to this series.
As I see it, struct regmap_but first need to be extended to add
a max_speed, e.g.
@max_speed: Max transfer speed that can be used on the bus.
regmap_spi.c would then look for the devicetree node to fill the value
and on regmap_write/read fill speed_hz.
In this case, it could be called "register-frequency" or
"regmap-frequency"
If instead it is up to spi.c to read the devicetree node, then a way to
differentiate "regular" transfers from "regmap" transfers would be
necessary.
About submitting v3, should I submit only up-to the base driver, or can
I submit also the add offload support and add event support commits?
Regards,
Jorge
