path: root/wiki/Chat_log/20190307-io-chatlog

09:03 < wsa> welcome to today's IO meeting
09:03 < wsa> here are the status updates:
09:03 < wsa> A - what have I done since last time
09:03 < wsa> ------------------------------------
09:03 < wsa> Geert
09:03 < wsa> : corrected and tested SCIF5 DMA on R-Car E3, and upported MSIOF patches for
09:03 < wsa>   reset handling and bits-per-word restrictions
09:03 < wsa> Kaneko-san
09:03 < wsa> : posted patch to update thermal calculation for E3
09:03 < wsa> Marek
09:03 < wsa> : sent patches for AHCI and NVME PCI to respect the 32bit limitation of the
09:03 < wsa>   R-Car PCIe controller
09:03 < wsa> Shimoda-san
09:03 < wsa> : fixed a memory leak in the R-Car Gen2 PHY driver, discussed with BSP team
09:03 < wsa>   a HS400 problem and found a patch for a similar issue
09:03 < wsa> Ulrich
09:03 < wsa> :
09:03 < wsa> Wolfram
09:03 < wsa> : sent out another RFC implementing atomic I2C transfers, sent out a series
09:03 < wsa>   improving concurrency and one improving DMA robustness for i2c-rcar, fixed
09:03 < wsa>   SDHI issue regarding Block Count register, helped fixing an I2C core issue
09:03 < wsa>   about reusing irqs, reviewed I2C, thermal, MSIOF, PMIC related patches, did
09:03 < wsa>   minor periject work
09:03 < wsa> B - what I want to do until next time
09:03 < wsa> -------------------------------------
09:03 < wsa> Geert
09:03 < wsa> : wants to convert MSIOF to use GPIO descriptors for CS and to make use of
09:03 < wsa>   readl_poll_timeout()
09:03 < wsa> Kaneko-san
09:03 < wsa> : wants to post patch to update thermal calculation for H3, M3-W, M3-N
09:03 < wsa> Niklas
09:03 < wsa> : wants to investigate MMC PM imbalance issue on APE6EVM
09:03 < wsa> Shimoda-san
09:03 < wsa> : wants to wants to work on PWM suspend/resume once his atomic API patches
09:03 < wsa>   are merged, and to continue to improve phy-rcar-gen3-usb2 driver to enable
09:03 < wsa>   each source independency instead of all sources
09:03 < wsa> Simon
09:03 < wsa> : wants to asses RAVB patches in BSP v3.9.2, measure MMC performance across
09:03 < wsa>   various Gen3 boards
09:03 < wsa> Wolfram
09:03 < wsa> : wants to keep up with the atomic I2C transfer topic, handle 'arbitration lost'
09:03 < wsa>   and 'NAK' better with the IIC core, investigate SDR104 regression with SDIO
09:03 < wsa> C - problems I currently have
09:03 < wsa> -----------------------------
09:03 < wsa> Geert
09:03 < wsa> : detected a QSPI FLASH regression on Koelsch
09:03 < wsa> Wolfram
09:03 < wsa> : needs attention of someone with expertise to comment on the use of 'in_atomic()'
09:03 < wsa>   in his series about atomic I2C transfers
09:04 < wsa> uli__: so, is the D3 phy-mode patch one of those you tested?
09:05 < wsa> ah, now i see the mail
09:05 < wsa> so, it is, thanks
09:05 < uli__> yup
09:07 < wsa> horms: what about the RAVB upporting? do you have time for them or are you super busy and wouldn't mind someone else working on them?
09:08 < wsa> shimoda: a similar question to you. There is a PWM patch to be upported fixing an issue found by lockdep. Do you have time for that or would you actually be happy if this was delegated to someone interested?
09:08 < jmondi> 'morning, sorry, I'm a bit late
09:09 < horms> wsa: I am a bit busy. My main problem is that I don't know how to test them and they seem likely to have negative performance impatct.
09:09 < shimoda> wsa: please wait a little, I'd like to check the BSP patch
09:10 < wsa> shimoda: https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=70b4a22f220fef943c45a1122fa02d6faa42d2fe
09:11 < wsa> horms: one of them references an errata, wouldn't that do?
09:12 < wsa> and maybe we could request more information about the other one then?
09:13 < wsa> neg: did the cable arrive?
09:13 < neg> wsa: Not since last night when I wrote the status update ;-P
09:13 < wsa> ehrm, yes :)
09:14 < neg> wsa: If it's not here tomorrow I will buy one over the counter to get this thing rolling
09:14 < wsa> neg: yes, please
09:14 < shimoda> wsa: OK. I'll try to upport it. of course, reproduce this issue on mainline first :)
09:14 < horms> wsa: the errata patch has a fix which involves not supporting 1GB/s on E3.
09:14 < wsa> shimoda: thanks!
09:14 < horms> And yet that works fine in my environment.
09:15 < horms> But I do take your point.
09:15 < horms> Perhaps the way forwards is to a) test (probably they work) b) post the patches
09:15 < horms> I just feel that these patches apply a sledgehammer to crack an egg
09:15 < horms> But I don't have an alternate solution
09:17 < wsa> horms: right, the patch description say "It communication at 1Gbps may fail" ... "may"
09:17 < horms> Yes, this is my red flag
09:17 < horms> In my limited testing I have never seen it fail
09:17 < wsa> horms: probably this would need communication with the HW guys then?
09:17 < horms> Yes, maybe that is a good idea
09:17 < wsa> if they can think of a better solution?
09:18 < horms> The other one I should test
09:18 < horms> and see if there is a performance impact
09:18 < wsa> Marex: so, was there anything more needed for D3 CAN?
09:18 < horms> if not then I'm not so worried about it
09:18 < wsa> horms: please, do
09:18 < horms> but even in that case I feel that upstream will require some sort of test case
09:19 < horms> How about this
09:19 < horms> 1) I will test the patches
09:19 < horms> 2) I will send some feedback to the periperi ML
09:19 < horms> 3) Shimoda-san or Morimoto-san can pass some questions on to Renesas
09:20 < wsa> sounds good to me
09:20 < wsa> thanks!
09:21 < horms> Likewise, thanks!
09:21 < Marex> wsa: DT enablement ?
09:21 < wsa> Marex: driver or board?
09:21 < morimoto> horms: for 3) anytime is OK for me. But I'm not familiar with it, please add "background, question" etc ;)
09:21 < Marex> wsa: board
09:22 < horms> morimoto: thanks. I will include background information
09:22 < morimoto> horms: thanks
09:22 < Marex> wsa: Draak has both CANs on pins, it just needs to be enabled and tested
09:23 < wsa> I recall Uli did some CAN enablement patches, I was wondering...
09:24 < Marex> wsa: I didn't see any for Draak
09:24 < geertu> wsa: : They were just .dtsi additions based on the datasheet, AFAIK
09:24 < wsa> will check
09:24 < Marex> yep, only the DTSi and documentation
09:24 < Marex> s/documentation/dt bindings/
09:26 < wsa> right
09:26 < wsa> the board was missed :(
09:26 < wsa> OK, thanks for doing it, Marek
09:27 < Marex> wsa: I need to test it when I have access to Draak tho
09:28 < Marex> wsa: damm -san said he might install a cable between those two CANs on Draak at least, so I can play around with it remotely
09:28 < wsa> OK, nice
09:28 < wsa> any questions from your side?
09:29 < Marex> wsa: PCI 32bit limitation , will we have to patch each and every driver to not rewrite the DMA masks ?
09:29 < Marex> wsa: I think so
09:30 < wsa> I am not deep enough into that topic, it doesn't sound like it would scale well, though
09:31 < Marex> wsa: eventually the drivers shouldn't change the DMA masks at all
09:31 < Marex> unless there's a good reason, in which case they need to respect the bus DMA mask too
09:31 < wsa> that sounds way better to me
09:32 < Marex> wsa: it still means patching a whole lot of drivers
09:32 < geertu> Yah, DMA masks are not a property of the driver, but of the device and bus
09:32 < Marex> I sent out an RFC for AHCI and NVME drivers, which I tested on Gen3 and Gen2 with a PCIe card and those start working with the change
09:33 < Marex> so did xhci controller, but I didn't send a patch yet, I want some feedback on those two RFCs first
09:33 < Marex> on Gen2 , I still have trouble with NVMe, I am not sure why though ... and it seems unrelated to this problem
09:33 < geertu> Marex: So NVMe is working now?
09:33 < Marex> geertu: on Gen3
09:34 < Marex> geertu: on Gen2 I get timeouts , but see above
09:34 < Marex> geertu: I suspect this has to do with the NVMe being mostly tested on 64bit systems
09:35 < geertu> Marex: Probably. It's not a good performance improvement investion on systems limited to 32-bit DMA anyway ;-)
09:36 < wsa> so, shall we move to discussing Magnus' request?
09:36 < wsa> and my own for that matter? :)
09:36  * geertu assumes his NVMe does DMA to all 36-bit of RAM present ;-)
09:36  * morimoto focus to Renesas inside work from now
09:37 < Marex> geertu: probably all 64bit
09:38 < wsa> so, some comments have been made by mail
09:38 < wsa> About the original proposal from damm:
09:38 < geertu> damm: Alive?
09:39 < damm> yep
09:39 < wsa> I agree with pinchartl that review doesn't always result in a rev-by tag
09:39 < geertu> True
09:39 < wsa> if I have review comments not addressed yet, I don't give the tag
09:40 < geertu> I always assume a new version will appear, where I can provide mine, but probably I'm too optimistic...
09:40 < pinchartl> I sometimes end a review e-mail with "and if you address all these, Reviewed-by: ..." but that's only when the patch is mostly ready and I trust that the next round will be correct
09:40 < pinchartl> if deep rework is needed I don't do that
09:41 < wsa> pinchartl: ack
09:41 < pinchartl> sometimes the result of a review is also the realization that a patch isn't needed, in which case no new version appears
09:41 < pinchartl> there's still value there
09:42 < geertu> The list I provide to Magnus is auto-generated, so it only includes actual R-b's given.
09:42 < wsa> would it be an option to include 4) "reviewed patches still pending" or something alike?
09:42 < geertu> Sure!
09:42 < geertu> Reviewed-but-not-approved-by ;-)
09:43 < pinchartl> it's more work to split the two for me, so I'd like to know if that's actually needed
09:43 < pinchartl> same for the tested patches, if the test fails I don't give a Tested-by tag :-)
09:44 < geertu> Everything seems to be success-driven, like social media
09:44 < pinchartl> damm: was your request about splitting the list of patches in the submitted, reviewed and tested catagories, or do you need to account patches that carry a tag separately ?
09:44 < geertu> "bad" outcomes don't end up in the report
09:44 < neg> Test-failed-by: :-)
09:44 < pinchartl> in the latter case, can't we just harvest Linus' tree to extract tags automatically without needing us to account those patches manually ?
09:45 < pinchartl> it won't be linked to a particular reporting period though
09:45 < geertu> Yep, add a 2-3 month delay
09:45 < geertu> Harvesting from patchwork would have less latency
09:45 < pinchartl> but as I explained in the e-mails, a metric based on tags given out is quite useless as most of the value is attached to tags that are not given
09:45 < damm> pinchartl: splitting patches in categories would be good from my side
09:45 < jmondi> that might fail to capture reviewes that do not end up in a tag
09:46 < pinchartl> damm: then I'm fine with that, that's what I do already :-)
09:46 < damm> =)
09:48 < wsa> well, if it is just seperating reviews from tests done (without the need of a tag), I can easily do that
09:48 < wsa> and will happily do so
09:49 < wsa> consensus on that one? damm-san?
09:49  * geertu has already copied-and-modified his list-patches-reviewed-by-me script
09:50 < damm> sure
09:50 < damm> thanks guys
09:50 < wsa> cool
09:51 < wsa> so, about my request on adding desc to reviews / tests...
09:51 < wsa> I am pretty much with pinchartl here again when it comes to seperation between ack and rev. but this is not the main point for me.
09:52 < wsa> rev by only is fine enough for me, but i'd like to know what kind of review
09:52 < jmondi> wsa: I have missed if you're suggesting such a lenghty description in patch reviews or for the bi-weekly report
09:52 < wsa> nope
09:53 < wsa> just some text around the rev-by tag like "i verified against the errata" or "not familiar with PCI internals, but checked the API" or "did formal check"
09:54 < wsa> jmondi: do you get the picture?
09:54 < jmondi> I see, in the patch review email then, not just as a note to report during our meetings, right?
09:54 < pinchartl> wsa: how about not making that a strict requirement to start with, only adding such text when there's value to it
09:54 < pinchartl> (especially when the review is partial only)
09:54 < wsa> pinchartl: I'd like that
09:54 < pinchartl> and the other question was what you'll do with the information
09:55 < pinchartl> as there's no standard it then include it in the commit message
09:55 < pinchartl> s/it then/to then/
09:55 < wsa> What I'll do with it? As a maintainer, it gives me an idea what I still need to review
09:56 < wsa> Or if a patch has enough review / test that I can send it back to stable or give it another cycle in -next
09:56 < pinchartl> ok, it gives you more information to know how much you can trust the review
09:56 < pinchartl> that's useful
09:57 -!- Marex [~Marex@195.140.253.167] has quit Ping timeout: 259 seconds
09:57 < wsa> It doesn't need to be a whole lot of text; I think it can be covered in one line mostly
09:59 < marex-cloud> Hmmm, something went wrong with my primary irc system
10:00 < wsa> I think it will increase quality because it is clear what is reviewed. There are no assumptions about the review which could be wrong
10:00 < neg> marex-cloud *switchs to the backup troll generator console* ;-)
10:00 < wsa> This is an overall experience, also coming from I2C maintenance
10:01 < wsa> but it seems there are no further comments
10:01 < geertu> I agree it's good to have such information, and thus provide it, if it makes sense.
10:02 < wsa> I totally agree it can be skipped for trivial patches
10:02 -!- Marex [~Marex@195.140.253.167] has joined #periperi
10:02 < wsa> ok, so, let's please do that from now on
10:03 < wsa> and with that we can switch to the Core meeting?
10:03 < geertu> Sounds good to me!