| Message ID | 20230404060011.108561-1-jaewon02.kim@samsung.com |
|---|---|
| Headers | show |
| Series | spi: s3c64xx: improve SPI polling mode | expand |
On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: > This patch adds new 'samsung,spi-polling' property to support polling mode. > In some environments, polling mode is required even if DMA is supported. > Changed it to support not only with quick but also optinally with > devicetree. Why would this be required if we can use DMA? If this is a performance optimisation for small messages the driver should just work out when to choose PIO over DMA like other drivers do. It is hard to see this as a hardware property which should be configured via DT.
Hello Mark, On 23. 4. 4. 19:54, Mark Brown wrote: > On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: >> This patch adds new 'samsung,spi-polling' property to support polling mode. >> In some environments, polling mode is required even if DMA is supported. >> Changed it to support not only with quick but also optinally with >> devicetree. > Why would this be required if we can use DMA? If this is a performance > optimisation for small messages the driver should just work out when to > choose PIO over DMA like other drivers do. It is hard to see this as a > hardware property which should be configured via DT. We are providing a VM environment in which several Guest OSs are running. If Host OS has DMA, GuestOS should use SPI as polling mode. In order to support s3c64xx in a DMA-less environment, it must be separated with a quirk. However, there is DMA in the Host OS and no DMA in the Guest OS, it is not correct to separate them with quirk. I'm considering supporting this systems with DeviceTree rather than qurik. If 'samsung,spi-polling' looks to be a SW configuration, how about 'samsung,no-dma'. This is not to simply change the mode using DeviceTree, but to support an environment without DMA. Thanks Jaewon Kim
On Tue, Apr 04, 2023 at 08:17:13PM +0900, Jaewon Kim wrote: > On 23. 4. 4. 19:54, Mark Brown wrote: > > On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: > >> This patch adds new 'samsung,spi-polling' property to support polling mode. > >> In some environments, polling mode is required even if DMA is supported. > >> Changed it to support not only with quick but also optinally with > >> devicetree. > > Why would this be required if we can use DMA? If this is a performance > > optimisation for small messages the driver should just work out when to > > choose PIO over DMA like other drivers do. It is hard to see this as a > > hardware property which should be configured via DT. > We are providing a VM environment in which several Guest OSs are running. > If Host OS has DMA, GuestOS should use SPI as polling mode. This sounds like some sort of virtualised environment with passthrough? If that's the case then the host OS will be in control of the device tree provided to the guest so it simply shouldn't be describing the DMA configuration if it doesn't want the guest to use DMA for some reason. There's no value in describing the DMA the guest shouldn't use then providing an additional property telling the guest not to pay attention to the DMA when we could simply not do the first step.
Hello Mark, On 23. 4. 4. 20:41, Mark Brown wrote: > On Tue, Apr 04, 2023 at 08:17:13PM +0900, Jaewon Kim wrote: >> On 23. 4. 4. 19:54, Mark Brown wrote: >>> On Tue, Apr 04, 2023 at 03:00:09PM +0900, Jaewon Kim wrote: >>>> This patch adds new 'samsung,spi-polling' property to support polling mode. >>>> In some environments, polling mode is required even if DMA is supported. >>>> Changed it to support not only with quick but also optinally with >>>> devicetree. >>> Why would this be required if we can use DMA? If this is a performance >>> optimisation for small messages the driver should just work out when to >>> choose PIO over DMA like other drivers do. It is hard to see this as a >>> hardware property which should be configured via DT. >> We are providing a VM environment in which several Guest OSs are running. >> If Host OS has DMA, GuestOS should use SPI as polling mode. > This sounds like some sort of virtualised environment with passthrough? > If that's the case then the host OS will be in control of the device > tree provided to the guest so it simply shouldn't be describing the DMA > configuration if it doesn't want the guest to use DMA for some reason. > There's no value in describing the DMA the guest shouldn't use then > providing an additional property telling the guest not to pay attention > to the DMA when we could simply not do the first step. Is it correct in your opinion to change to polling mode if there is no DMA describing in DeviceTree? Currently, if there is no DMA, the probe failed in s3c64xx driver. So I added the "samsung,spi-polling" property not to check DMA. If your opinion is to switch to Polling mode if there is no DMA, I will fix it in the next version. Thanks Jaewon Kim
On Tue, Apr 04, 2023 at 09:22:25PM +0900, Jaewon Kim wrote: > On 23. 4. 4. 20:41, Mark Brown wrote: > > There's no value in describing the DMA the guest shouldn't use then > > providing an additional property telling the guest not to pay attention > > to the DMA when we could simply not do the first step. > Is it correct in your opinion to change to polling mode if there is no > DMA describing in DeviceTree? Yes, exactly. > Currently, if there is no DMA, the probe failed in s3c64xx driver. > So I added the "samsung,spi-polling" property not to check DMA. > If your opinion is to switch to Polling mode if there is no DMA, I will > fix it in the next version. Great, that sounds like a better solution. If there is a description of DMA but it can't be fetched then an error should be right, but if there's just no DMA described then switching to polling mode seems better.
On Tue, Apr 04, 2023 at 03:00:11PM +0900, Jaewon Kim wrote: > This patch adds IRQ based PIO mode instead of cpu polling. > By using the FIFO trigger level, interrupts are received. > CPU consumption is reduced in PIO mode because registers are not > constantly checked. Is there some lower limit where it's still worth using polling, for example for just one or two bytes like a register address? Taking an interrupt isn't free...
Hello Mark, On 23. 4. 4. 21:58, Mark Brown wrote: > On Tue, Apr 04, 2023 at 03:00:11PM +0900, Jaewon Kim wrote: > >> This patch adds IRQ based PIO mode instead of cpu polling. >> By using the FIFO trigger level, interrupts are received. >> CPU consumption is reduced in PIO mode because registers are not >> constantly checked. > Is there some lower limit where it's still worth using polling, for > example for just one or two bytes like a register address? Taking an > interrupt isn't free... I did not considers lower limit. According to your review, interrupt seems to be called too often. However, It can't prevent the CPU utilization going to 100% during spi transmission. We will give more consideration and deliver a better solution to the next patch version. Thanks Jaewon Kim
On Tue, Apr 04, 2023 at 10:15:05PM +0900, Jaewon Kim wrote: > On 23. 4. 4. 21:58, Mark Brown wrote: > > Is there some lower limit where it's still worth using polling, for > > example for just one or two bytes like a register address? Taking an > > interrupt isn't free... > I did not considers lower limit. > According to your review, interrupt seems to be called too often. > However, It can't prevent the CPU utilization going to 100% during spi > transmission. It's not so much that the interrupt could be called too often as that the time taken to take the interrupt (including all the overhead the CPU has) might be large compared to what busy waiting would take if the transfer is very small. If the FIFO is deep enough and the transfer is long enough to use that then you start to see a win from interrupts. > We will give more consideration and deliver a better solution to the > next patch version. Great.
On 04/04/2023 08:00, Jaewon Kim wrote: > This patch adds new 'samsung,spi-polling' property to support polling mode. Do not use "This commit/patch", but imperative mood. See: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 Also, binding should be before its usage. > In some environments, polling mode is required even if DMA is supported. Why? What are these environments? You need to explain all this in commit msg. > Changed it to support not only with quick but also optinally with typo: optionally > devicetree. > Best regards, Krzysztof
On 23. 4. 5. 14:42, Krzysztof Kozlowski wrote: > On 04/04/2023 08:00, Jaewon Kim wrote: >> This patch adds new 'samsung,spi-polling' property to support polling mode. > Do not use "This commit/patch", but imperative mood. See: > https://protect2.fireeye.com/v1/url?k=3cb451b1-5d3f4488-3cb5dafe-000babffae10-4326e9d41dfad262&q=1&e=1125b69d-6d9e-4c91-a8fd-3470cd2278e4&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.17.1%2Fsource%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst%23L95 > > Also, binding should be before its usage. thanks. I will refer to it in next version. >> In some environments, polling mode is required even if DMA is supported. > Why? What are these environments? You need to explain all this in commit > msg. > We are providing a VM environment in which several Guest OSs are running. There are cases where DMA exist only in HostOS and not exist in GuestOS. In this case, SPI in GuestOS runs with polling mode. I thought it was correct that the polling mode was supported optional, not quirk. I have plan to change the polling mode if there is no 'dmas' property. How about your opinion? >> Changed it to support not only with quick but also optinally with > typo: optionally > >> devicetree. >> > Best regards, > Krzysztof > > Thanks Jaewon Kim