| Message ID | 20221128020613.14821-8-xiangsheng.hou@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] spi: mtk-snfi: add snfi support for mt7986 IC | expand |
Hi Krzysztof, On Mon, 2022-11-28 at 10:04 +0100, Krzysztof Kozlowski wrote: > On 28/11/2022 03:06, Xiangsheng Hou wrote: > > add rx-sample-delay and rx-latch-latency property. > > Start sentences with capital letter. > > Here and in commit subject: property->properties Will be fixed in next series. > > > > --- a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- > > snfi.yaml > > +++ b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- > > snfi.yaml > > @@ -55,6 +55,22 @@ properties: > > description: device-tree node of the accompanying ECC engine. > > $ref: /schemas/types.yaml#/definitions/phandle > > > > + rx-sample-delay: > > No, use existing property, don't invent your own stuff - missing unit > suffix. See spi-peripheral-props.yaml. Will change to other private property. The read sample delay with MediaTek SPI NAND controller can be set with values from 0 to 47. However, it`s difficult to say the unit of each vaule, because the unit value will be difference with different chip process or different corner IC. > > + description: Rx delay to sample data with this value, the > > valid > > + values are from 0 to 47. The delay is smaller > > than > > + the rx-latch-latency. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > Drop $ref. Will do. > > > + minItems: 0 > > + maxItems: 47 > > + default: 0 > > + > > + rx-latch-latency: > > Same problems. Did you check spi-peripheral-props.yaml or other SPI > controller schemas for such property? > > > + description: Rx delay to sample data with this value, the > > value > > + unit is clock cycle. > > I think the unit should be rather time (e.g. us). > Yes, I checked the spi-peripheral-props.yaml and the delay values are described exactly unit with ns or us. However the unit of MediaTek read latch latency is clock cycle and it`s difference with different clock frequency. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1, 2, 3] > > + default: 0 > > + > > required: > > - compatible > > - reg > Best regards, Xiangsheng Hou
On 29/11/2022 03:50, Xiangsheng Hou (侯祥胜) wrote: >>> --- a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- >>> snfi.yaml >>> +++ b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- >>> snfi.yaml >>> @@ -55,6 +55,22 @@ properties: >>> description: device-tree node of the accompanying ECC engine. >>> $ref: /schemas/types.yaml#/definitions/phandle >>> >>> + rx-sample-delay: >> >> No, use existing property, don't invent your own stuff - missing unit >> suffix. See spi-peripheral-props.yaml. > Will change to other private property. The read sample delay with > MediaTek SPI NAND controller can be set with values from 0 to 47. > However, it`s difficult to say the unit of each vaule, because the unit > value will be difference with different chip process or different > corner IC. Why you cannot use same formula as other SPI drivers for sample-delay? And divide/multiple by some factor specific to SoC, which is taken from driver_data? > >>> + description: Rx delay to sample data with this value, the >>> valid >>> + values are from 0 to 47. The delay is smaller >>> than >>> + the rx-latch-latency. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >> >> Drop $ref. > Will do. > >> >>> + minItems: 0 >>> + maxItems: 47 >>> + default: 0 >>> + >>> + rx-latch-latency: >> >> Same problems. Did you check spi-peripheral-props.yaml or other SPI >> controller schemas for such property? >> >>> + description: Rx delay to sample data with this value, the >>> value >>> + unit is clock cycle. >> >> I think the unit should be rather time (e.g. us). >> > Yes, I checked the spi-peripheral-props.yaml and the delay values are > described exactly unit with ns or us. However the unit of MediaTek read > latch latency is clock cycle and it`s difference with different clock > frequency. This is fine in such case. > >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1, 2, 3] >>> + default: 0 >>> + Best regards, Krzysztof
Hi Krzysztof, On Tue, 2022-11-29 at 08:47 +0100, Krzysztof Kozlowski wrote: > On 29/11/2022 03:50, Xiangsheng Hou (侯祥胜) wrote: > > > > > --- a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- > > > > snfi.yaml > > > > +++ b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- > > > > snfi.yaml > > > > @@ -55,6 +55,22 @@ properties: > > > > description: device-tree node of the accompanying ECC > > > > engine. > > > > $ref: /schemas/types.yaml#/definitions/phandle > > > > > > > > + rx-sample-delay: > > > > > > No, use existing property, don't invent your own stuff - missing > > > unit > > > suffix. See spi-peripheral-props.yaml. > > > > Will change to other private property. The read sample delay with > > MediaTek SPI NAND controller can be set with values from 0 to 47. > > However, it`s difficult to say the unit of each vaule, because the > > unit > > value will be difference with different chip process or different > > corner IC. > > Why you cannot use same formula as other SPI drivers for sample- > delay? > And divide/multiple by some factor specific to SoC, which is taken > from > driver_data? Even for specific SoC, the unit of sample delay may be various with different corner IC. Besides, whether it`s acceptable by change the property rx-sample-delay and rx-latch-latency to mediatek,rx-sample-delay and mediatek,rx-latch- latency? Thanks Xiangsheng Hou
On 30/11/2022 09:18, Xiangsheng Hou (侯祥胜) wrote: > Hi Krzysztof, > > On Tue, 2022-11-29 at 08:47 +0100, Krzysztof Kozlowski wrote: >> On 29/11/2022 03:50, Xiangsheng Hou (侯祥胜) wrote: >> >>>>> --- a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- >>>>> snfi.yaml >>>>> +++ b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk- >>>>> snfi.yaml >>>>> @@ -55,6 +55,22 @@ properties: >>>>> description: device-tree node of the accompanying ECC >>>>> engine. >>>>> $ref: /schemas/types.yaml#/definitions/phandle >>>>> >>>>> + rx-sample-delay: >>>> >>>> No, use existing property, don't invent your own stuff - missing >>>> unit >>>> suffix. See spi-peripheral-props.yaml. >>> >>> Will change to other private property. The read sample delay with >>> MediaTek SPI NAND controller can be set with values from 0 to 47. >>> However, it`s difficult to say the unit of each vaule, because the >>> unit >>> value will be difference with different chip process or different >>> corner IC. >> >> Why you cannot use same formula as other SPI drivers for sample- >> delay? >> And divide/multiple by some factor specific to SoC, which is taken >> from >> driver_data? > > Even for specific SoC, the unit of sample delay may be various with > different corner IC. Which is easy to achieve with driver_data as I said. > Besides, whether it`s acceptable by change the property rx-sample-delay > and rx-latch-latency to mediatek,rx-sample-delay and mediatek,rx-latch- > latency? Not for sample delay, because you should use existing properties. Your driver implementation is not usually argument to duplicate properties in the bindings. Best regards, Krzysztof
Hi! On Wed, Nov 30, 2022 at 4:35 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>> Will change to other private property. The read sample delay with > >>> MediaTek SPI NAND controller can be set with values from 0 to 47. > >>> However, it`s difficult to say the unit of each vaule, because the > >>> unit > >>> value will be difference with different chip process or different > >>> corner IC. > >> > >> Why you cannot use same formula as other SPI drivers for sample- > >> delay? > >> And divide/multiple by some factor specific to SoC, which is taken > >> from > >> driver_data? > > > > Even for specific SoC, the unit of sample delay may be various with > > different corner IC. > > Which is easy to achieve with driver_data as I said. I think Xiangsheng means this: This sample delay isn't achieved using a fixed clock signal. It's probably some kind of delay circuit whose delay value varies due to its manufacturing process. Every single chip made got different delay units, so it's impossible to specify a single unit for one chip model. If that's true, shouldn't this be a value calibrated on-the-fly on probe instead? A single device-tree is supposed to be applied to all devices of the same model, so a value that varies on a device-by-device basis probably shouldn't be a device-tree property.
diff --git a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-snfi.yaml b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-snfi.yaml index ee20075cd0e7..367862688e92 100644 --- a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-snfi.yaml +++ b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-snfi.yaml @@ -55,6 +55,22 @@ properties: description: device-tree node of the accompanying ECC engine. $ref: /schemas/types.yaml#/definitions/phandle + rx-sample-delay: + description: Rx delay to sample data with this value, the valid + values are from 0 to 47. The delay is smaller than + the rx-latch-latency. + $ref: /schemas/types.yaml#/definitions/uint32 + minItems: 0 + maxItems: 47 + default: 0 + + rx-latch-latency: + description: Rx delay to sample data with this value, the value + unit is clock cycle. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3] + default: 0 + required: - compatible - reg
add rx-sample-delay and rx-latch-latency property. Signed-off-by: Xiangsheng Hou <xiangsheng.hou@mediatek.com> --- .../bindings/spi/mediatek,spi-mtk-snfi.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)