On Tuesday 23 February 2016 05:58:32 Anurag Kumar Vulisha wrote:
> >
> > I don't know what is appropriate because I have no idea what Rxwatermark is
> > good for. Can you try describing why we can't just set it to the correct
> > value
> > for everyone automatically?
> >
>
> This RX watermark level sets the minimum number of free locations within the
> RX FIFO .When the rx fifo level crosses the programmed watermark level ,sata
> controller will transmit HOLDS to the device asking it to wait. This happens
> when dma
> reads the rx fifo data slower than the device is sending the data. Note that
> it can take some time for the HOLDs to get to
> the other end and in the time there must be enough room in the FIFO to absorb
> all data that could arrive from the device.
> Currently we are using 0x40 for this value, which works fine with all
> hardware designs we are currently having. But hoping
> that this value may vary for future silicon versions, I wanted to make this
> as a configurable value. So for this reason I thought
> of moving it either to device-tree or making it as a module_param() property.
>
Ok, so if this depends on the silicon version, your initial approach
would be better than the module_param.
I would probably make this dependent on the compatible string instead,
and have a table in the device driver that uses a specific value
for each variant of the device, but either way should be fine.
Having a separate property is most appropriate if for each hardware
revision there is exactly one ideal value, while a table in the
driver makes more sense if this takes a bit of tuning and the driver
might choose to optimize it differently based on other constraints,
such as its own interrupt handler implementation.
Arnd
