Re: [PATCH] arm64: dts: marvell: armada-37xx: Set linux,pci-domain to zero
On Thu, 15 Apr 2021 10:36:40 +0200
Pali Rohár wrote:
> On Tuesday 13 April 2021 13:17:29 Rob Herring wrote:
> > On Mon, Apr 12, 2021 at 7:41 AM Pali Rohár wrote:
> > >
> > > Since commit 526a76991b7b ("PCI: aardvark: Implement driver 'remove'
> > > function and allow to build it as module") PCIe controller driver for
> > > Armada 37xx can be dynamically loaded and unloaded at runtime. Also driver
> > > allows dynamic binding and unbinding of PCIe controller device.
> > >
> > > Kernel PCI subsystem assigns by default dynamically allocated PCI domain
> > > number (starting from zero) for this PCIe controller every time when
> > > device
> > > is bound. So PCI domain changes after every unbind / bind operation.
> >
> > PCI host bridges as a module are relatively new, so seems likely a bug to
> > me.
>
> Why a bug? It is there since 5.10 and it is working.
>
> > > Alternative way for assigning PCI domain number is to use static allocated
> > > numbers defined in Device Tree. This option has requirement that every PCI
> > > controller in system must have defined PCI bus number in Device Tree.
> >
> > That seems entirely pointless from a DT point of view with a single PCI
> > bridge.
>
> If domain id is not specified in DT then kernel uses counter and assigns
> counter++. So it is not pointless if we want to have stable domain id.
What Rob is trying to say is that
- the bug is that kernel assigns counter++
- device-tree should not be used to fix problems with how kernel does
things
- if a device has only one PCIe controller, it is pointless to define
it's pci-domain. If there were multiple controllers, then it would
make sense, but there is only one
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 20:16:24 +0200 Tobias Waldekranz wrote: > You could imagine a different mode in which the DSA driver would receive > the bucket allocation from the bond/team driver (which in turn could > come all the way from userspace). Userspace could then implement > whatever strategy it wants to maximize utilization, though still bound > by the limitations of the hardware in terms of fields considered during > hashing of course. The problem is that even with the ability to change the bucket configuration however we want it still can happen with non-trivial probability that all (src,dst) pairs on the network will hash to one bucket. The probability of that happening is 1/(8^(n-1)) for n (src,dst) pairs. On Turris Omnia the most common configuration is that the switch ports are bridged. If the user plugs only two devices into the lan ports, one would expect that both devices could utilize 1 gbps each. In this case there is 1/8 probability that both devices would hash to the same bucket. It is quite bad if multi-CPU upload won't work for 12.5% of our customers that are using our device in this way. So if there is some reasonable solution how to implement multi-CPU via the port vlan mask, I will try to pursue this. Marek
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 16:46:32 +0200 Tobias Waldekranz wrote: > On Tue, Apr 13, 2021 at 02:27, Marek Behun wrote: > > On Tue, 13 Apr 2021 01:54:50 +0200 > > Marek Behun wrote: > > > >> I will look into this, maybe ask some follow-up questions. > > > > Tobias, > > > > it seems that currently the LAGs in mv88e6xxx driver do not use the > > HashTrunk feature (which can be enabled via bit 11 of the > > MV88E6XXX_G2_TRUNK_MAPPING register). > > This should be set at the bottom of mv88e6xxx_lag_sync_masks. > > > If we used this feature and if we knew what hash function it uses, we > > could write a userspace tool that could recompute new MAC > > addresses for the CPU ports in order to avoid the problem I explained > > previously... > > > > Or the tool can simply inject frames into the switch and try different > > MAC addresses for the CPU ports until desired load-balancing is reached. > > > > What do you think? > > As you concluded in your followup, not being able to have a fixed MAC > for the CPU seems weird. > > Maybe you could do the inverse? Allow userspace to set the masks for an > individual bond/team port in a hash-based LAG, then you can offload that > to DSA. What masks?
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 02:27:30 +0200 Marek Behun wrote: > On Tue, 13 Apr 2021 01:54:50 +0200 > Marek Behun wrote: > > > I will look into this, maybe ask some follow-up questions. > > Tobias, > > it seems that currently the LAGs in mv88e6xxx driver do not use the > HashTrunk feature (which can be enabled via bit 11 of the > MV88E6XXX_G2_TRUNK_MAPPING register). > > If we used this feature and if we knew what hash function it uses, we > could write a userspace tool that could recompute new MAC > addresses for the CPU ports in order to avoid the problem I explained > previously... > > Or the tool can simply inject frames into the switch and try different > MAC addresses for the CPU ports until desired load-balancing is reached. > > What do you think? > > Marek Although changing MAC addresses of the CPU ports each time some new device comes into the network doesn't seem like a good idea, now that I think about it. Hmm.
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 01:54:50 +0200 Marek Behun wrote: > I will look into this, maybe ask some follow-up questions. Tobias, it seems that currently the LAGs in mv88e6xxx driver do not use the HashTrunk feature (which can be enabled via bit 11 of the MV88E6XXX_G2_TRUNK_MAPPING register). If we used this feature and if we knew what hash function it uses, we could write a userspace tool that could recompute new MAC addresses for the CPU ports in order to avoid the problem I explained previously... Or the tool can simply inject frames into the switch and try different MAC addresses for the CPU ports until desired load-balancing is reached. What do you think? Marek
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 01:13:53 +0200 Tobias Waldekranz wrote: > > ...you could get the isolation in place. But you will still lookup the > > DA in the ATU, and there you will find a destination of either cpu0 or > > cpu1. So for one of the ports, the destination will be outside of its > > port based VLAN. Once the vectors are ANDed together, it is left with no > > valid port to egress through, and the packet is dropped. > > > >> Am I wrong? I confess that I did not understand this into the most fine > >> details, so it is entirely possible that I am missing something > >> important and am completely wrong. Maybe this cannot be done. > > > > I really doubt that it can be done. Not in any robust way at > > least. Happy to be proven wrong though! :) > > I think I figured out why it "works" for you. Since the CPU address is > never added to the ATU, traffic for it is treated as unknown. Thanks to > that, it flooded and the isolation brings it together. As soon as > mv88e6xxx starts making use of Vladimirs offloading of host addresses > though, I suspect this will fall apart. Hmm :( This is bad news. I would really like to make it balance via input ports. The LAG balancing for this usecase is simply unacceptable, since the switch puts so little information into the hash function. I will look into this, maybe ask some follow-up questions. Marek
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 01:48:05 +0300 Vladimir Oltean wrote: > On Tue, Apr 13, 2021 at 12:26:52AM +0200, Tobias Waldekranz wrote: > > On Tue, Apr 13, 2021 at 01:06, Vladimir Oltean wrote: > > > On Mon, Apr 12, 2021 at 11:49:22PM +0200, Tobias Waldekranz wrote: > > >> On Tue, Apr 13, 2021 at 00:34, Vladimir Oltean > > >> wrote: > > >> > On Mon, Apr 12, 2021 at 11:22:45PM +0200, Tobias Waldekranz wrote: > > >> >> On Mon, Apr 12, 2021 at 21:30, Marek Behun > > >> >> wrote: > > >> >> > On Mon, 12 Apr 2021 14:46:11 +0200 > > >> >> > Tobias Waldekranz wrote: > > >> >> > > > >> >> >> I agree. Unless you only have a few really wideband flows, a LAG > > >> >> >> will > > >> >> >> typically do a great job with balancing. This will happen without > > >> >> >> the > > >> >> >> user having to do any configuration at all. It would also perform > > >> >> >> well > > >> >> >> in "router-on-a-stick"-setups where the incoming and outgoing port > > >> >> >> is > > >> >> >> the same. > > >> >> > > > >> >> > TLDR: The problem with LAGs how they are currently implemented is > > >> >> > that > > >> >> > for Turris Omnia, basically in 1/16 of configurations the traffic > > >> >> > would > > >> >> > go via one CPU port anyway. > > >> >> > > > >> >> > > > >> >> > > > >> >> > One potencial problem that I see with using LAGs for aggregating CPU > > >> >> > ports on mv88e6xxx is how these switches determine the port for a > > >> >> > packet: only the src and dst MAC address is used for the hash that > > >> >> > chooses the port. > > >> >> > > > >> >> > The most common scenario for Turris Omnia, for example, where we > > >> >> > have 2 > > >> >> > CPU ports and 5 user ports, is that into these 5 user ports the user > > >> >> > plugs 5 simple devices (no switches, so only one peer MAC address > > >> >> > for > > >> >> > port). So we have only 5 pairs of src + dst MAC addresses. If we > > >> >> > simply > > >> >> > fill the LAG table as it is done now, then there is 2 * 0.5^5 = 1/16 > > >> >> > chance that all packets would go through one CPU port. > > >> >> > > > >> >> > In order to have real load balancing in this scenario, we would > > >> >> > either > > >> >> > have to recompute the LAG mask table depending on the MAC > > >> >> > addresses, or > > >> >> > rewrite the LAG mask table somewhat randomly periodically. (This > > >> >> > could > > >> >> > be in theory offloaded onto the Z80 internal CPU for some of the > > >> >> > switches of the mv88e6xxx family, but not for Omnia.) > > >> >> > > >> >> I thought that the option to associate each port netdev with a DSA > > >> >> master would only be used on transmit. Are you saying that there is a > > >> >> way to configure an mv88e6xxx chip to steer packets to different CPU > > >> >> ports depending on the incoming port? > > >> >> > > >> >> The reason that the traffic is directed towards the CPU is that some > > >> >> kind of entry in the ATU says so, and the destination of that entry > > >> >> will > > >> >> either be a port vector or a LAG. Of those two, only the LAG will > > >> >> offer > > >> >> any kind of balancing. What am I missing? > > >> >> > > >> >> Transmit is easy; you are already in the CPU, so you can use an > > >> >> arbitrarily fancy hashing algo/ebpf classifier/whatever to load > > >> >> balance > > >> >> in that case. > > >> > > > >> > Say a user port receives a broadcast frame. Based on your understanding > > >> > where user-to-CPU port assignments are used only for TX, which CPU port > > >> > should be selected by the sw
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 00:05:51 +0200 Tobias Waldekranz wrote: > On Mon, Apr 12, 2021 at 23:50, Marek Behun wrote: > > On Mon, 12 Apr 2021 23:22:45 +0200 > > Tobias Waldekranz wrote: > > > >> On Mon, Apr 12, 2021 at 21:30, Marek Behun wrote: > >> > On Mon, 12 Apr 2021 14:46:11 +0200 > >> > Tobias Waldekranz wrote: > >> > > >> >> I agree. Unless you only have a few really wideband flows, a LAG will > >> >> typically do a great job with balancing. This will happen without the > >> >> user having to do any configuration at all. It would also perform well > >> >> in "router-on-a-stick"-setups where the incoming and outgoing port is > >> >> the same. > >> > > >> > TLDR: The problem with LAGs how they are currently implemented is that > >> > for Turris Omnia, basically in 1/16 of configurations the traffic would > >> > go via one CPU port anyway. > >> > > >> > > >> > > >> > One potencial problem that I see with using LAGs for aggregating CPU > >> > ports on mv88e6xxx is how these switches determine the port for a > >> > packet: only the src and dst MAC address is used for the hash that > >> > chooses the port. > >> > > >> > The most common scenario for Turris Omnia, for example, where we have 2 > >> > CPU ports and 5 user ports, is that into these 5 user ports the user > >> > plugs 5 simple devices (no switches, so only one peer MAC address for > >> > port). So we have only 5 pairs of src + dst MAC addresses. If we simply > >> > fill the LAG table as it is done now, then there is 2 * 0.5^5 = 1/16 > >> > chance that all packets would go through one CPU port. > >> > > >> > In order to have real load balancing in this scenario, we would either > >> > have to recompute the LAG mask table depending on the MAC addresses, or > >> > rewrite the LAG mask table somewhat randomly periodically. (This could > >> > be in theory offloaded onto the Z80 internal CPU for some of the > >> > switches of the mv88e6xxx family, but not for Omnia.) > >> > >> I thought that the option to associate each port netdev with a DSA > >> master would only be used on transmit. Are you saying that there is a > >> way to configure an mv88e6xxx chip to steer packets to different CPU > >> ports depending on the incoming port? > >> > >> The reason that the traffic is directed towards the CPU is that some > >> kind of entry in the ATU says so, and the destination of that entry will > >> either be a port vector or a LAG. Of those two, only the LAG will offer > >> any kind of balancing. What am I missing? > > > > Via port vectors you can "load balance" by ports only, i.e. input port X > > -> trasmit via CPU port Y. > > How is this done? In a case where there is no bridging between the > ports, then I understand. Each port could have its own FID. But if you > have this setup... > >br0wan >/ \ > lan0 lan1 > > lan0 and lan1 would use the same FID. So how could you say that frames > from lan0 should go to cpu0 and frames from lan1 should go to cpu1 if > the DA is the same? What would be the content of the ATU in a setup > like that? > > > When using LAGs, you are load balancing via hash(src MAC | dst mac) > > only. This is better in some ways. But what I am saying is that if the > > LAG mask table is static, as it is now implemented in mv88e6xxx code, > > then for many scenarios there is a big probability of no load balancing > > at all. For Turris Omnia for example there is 6.25% probability that > > the switch chip will send all traffic to the CPU via one CPU port. > > This is because the switch chooses the LAG port only from the hash of > > dst+src MAC address. (By the 1/16 = 6.25% probability I mean that for > > cca 1 in 16 customers, the switch would only use one port when sending > > data to the CPU). > > > > The round robin solution here is therefore better in this case. > > I agree that it would be better in that case. I just do not get how you > get the switch to do it for you. I thought that this is configured in the mv88e6xxx_port_vlan() function. For each port, you specify via which ports data can egress. So for ports 0, 2, 4 you can enable CPU port 0, and for ports 1 and 3 CPU port 1. Am I wrong? I confess that I did not understand this into the most fine details, so it is entirely possible that I am missing something important and am completely wrong. Maybe this cannot be done. Marek
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Tue, 13 Apr 2021 01:17:21 +0300 Vladimir Oltean wrote: > On Tue, Apr 13, 2021 at 12:04:57AM +0200, Marek Behun wrote: > > On Mon, 12 Apr 2021 19:32:11 +0300 > > Vladimir Oltean wrote: > > > > > On Mon, Apr 12, 2021 at 11:00:45PM +0800, DENG Qingfang wrote: > > > > On Sun, Apr 11, 2021 at 09:50:17PM +0300, Vladimir Oltean wrote: > > > > > > > > > > So I'd be tempted to say 'tough luck' if all your ports are not up, > > > > > and > > > > > the ones that are are assigned statically to the same CPU port. It's a > > > > > compromise between flexibility and simplicity, and I would go for > > > > > simplicity here. That's the most you can achieve with static > > > > > assignment, > > > > > just put the CPU ports in a LAG if you want better dynamic load > > > > > balancing > > > > > (for details read on below). > > > > > > > > > > > > > Many switches such as mv88e6xxx only support MAC DA/SA load balancing, > > > > which make it not ideal in router application (Router WAN <--> ISP BRAS > > > > traffic will always have the same DA/SA and thus use only one port). > > > > > > Is this supposed to make a difference? Choose a better switch vendor! > > > > :-) Are you saying that we shall abandon trying to make the DSA > > subsystem work with better performace for our routers, in order to > > punish ourselves for our bad decision to use Marvell switches? > > No, not at all, I just don't understand what is the point you and > Qingfang are trying to make. I am not trying to make a point for this patch series. I did not touch it since the last time I sent it. Ansuel just took over this series and I am just contributing my thoughts to the RFC :) I agree with you that this patch series still needs a lot of work. > LAG is useful in general for load balancing. > With the particular case of point-to-point links with Marvell Linkstreet, > not so much. Okay. With a different workload, maybe it is useful with > Marvell Linkstreet too. Again okay. Same for static assignment, > sometimes it is what is needed and sometimes it just isn't. > It was proposed that you write up a user space program that picks the > CPU port assignment based on your favorite metric and just tells DSA to > reconfigure itself, either using a custom fancy static assignment based > on traffic rate (read MIB counters every minute) or simply based on LAG. > All the data laid out so far would indicate that this would give you the > flexibility you need, however you didn't leave any comment on that, > either acknowledging or explaining why it wouldn't be what you want. Yes, you are right. A custom userspace utility for assigning CPU ports would be better here than adding lots of complication into the kernel abstraction.
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Mon, 12 Apr 2021 19:32:11 +0300 Vladimir Oltean wrote: > On Mon, Apr 12, 2021 at 11:00:45PM +0800, DENG Qingfang wrote: > > On Sun, Apr 11, 2021 at 09:50:17PM +0300, Vladimir Oltean wrote: > > > > > > So I'd be tempted to say 'tough luck' if all your ports are not up, and > > > the ones that are are assigned statically to the same CPU port. It's a > > > compromise between flexibility and simplicity, and I would go for > > > simplicity here. That's the most you can achieve with static assignment, > > > just put the CPU ports in a LAG if you want better dynamic load balancing > > > (for details read on below). > > > > > > > Many switches such as mv88e6xxx only support MAC DA/SA load balancing, > > which make it not ideal in router application (Router WAN <--> ISP BRAS > > traffic will always have the same DA/SA and thus use only one port). > > Is this supposed to make a difference? Choose a better switch vendor! :-) Are you saying that we shall abandon trying to make the DSA subsystem work with better performace for our routers, in order to punish ourselves for our bad decision to use Marvell switches?
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Mon, 12 Apr 2021 23:49:22 +0200 Tobias Waldekranz wrote: > On Tue, Apr 13, 2021 at 00:34, Vladimir Oltean wrote: > > On Mon, Apr 12, 2021 at 11:22:45PM +0200, Tobias Waldekranz wrote: > >> On Mon, Apr 12, 2021 at 21:30, Marek Behun wrote: > >> > On Mon, 12 Apr 2021 14:46:11 +0200 > >> > Tobias Waldekranz wrote: > >> > > >> >> I agree. Unless you only have a few really wideband flows, a LAG will > >> >> typically do a great job with balancing. This will happen without the > >> >> user having to do any configuration at all. It would also perform well > >> >> in "router-on-a-stick"-setups where the incoming and outgoing port is > >> >> the same. > >> > > >> > TLDR: The problem with LAGs how they are currently implemented is that > >> > for Turris Omnia, basically in 1/16 of configurations the traffic would > >> > go via one CPU port anyway. > >> > > >> > > >> > > >> > One potencial problem that I see with using LAGs for aggregating CPU > >> > ports on mv88e6xxx is how these switches determine the port for a > >> > packet: only the src and dst MAC address is used for the hash that > >> > chooses the port. > >> > > >> > The most common scenario for Turris Omnia, for example, where we have 2 > >> > CPU ports and 5 user ports, is that into these 5 user ports the user > >> > plugs 5 simple devices (no switches, so only one peer MAC address for > >> > port). So we have only 5 pairs of src + dst MAC addresses. If we simply > >> > fill the LAG table as it is done now, then there is 2 * 0.5^5 = 1/16 > >> > chance that all packets would go through one CPU port. > >> > > >> > In order to have real load balancing in this scenario, we would either > >> > have to recompute the LAG mask table depending on the MAC addresses, or > >> > rewrite the LAG mask table somewhat randomly periodically. (This could > >> > be in theory offloaded onto the Z80 internal CPU for some of the > >> > switches of the mv88e6xxx family, but not for Omnia.) > >> > >> I thought that the option to associate each port netdev with a DSA > >> master would only be used on transmit. Are you saying that there is a > >> way to configure an mv88e6xxx chip to steer packets to different CPU > >> ports depending on the incoming port? > >> > >> The reason that the traffic is directed towards the CPU is that some > >> kind of entry in the ATU says so, and the destination of that entry will > >> either be a port vector or a LAG. Of those two, only the LAG will offer > >> any kind of balancing. What am I missing? > >> > >> Transmit is easy; you are already in the CPU, so you can use an > >> arbitrarily fancy hashing algo/ebpf classifier/whatever to load balance > >> in that case. > > > > Say a user port receives a broadcast frame. Based on your understanding > > where user-to-CPU port assignments are used only for TX, which CPU port > > should be selected by the switch for this broadcast packet, and by which > > mechanism? > > AFAIK, the only option available to you (again, if there is no LAG set > up) is to statically choose one CPU port per entry. But hopefully Marek > can teach me some new tricks! > > So for any known (since the broadcast address is loaded in the ATU it is > known) destination (b/m/u-cast), you can only "load balance" based on > the DA. You would also have to make sure that unknown unicast and > unknown multicast is only allowed to egress one of the CPU ports. > > If you have a LAG OTOH, you could include all CPU ports in the port > vectors of those same entries. The LAG mask would then do the final > filtering so that you only send a single copy to the CPU. The problem is that when the mv88e6xxx switch chooses the LAG entry, it takes into account only hash(src MAC | dst MAC). There is no other option, Marvell switches are unable to take more information into this decision (for example hash of the IP + TCP/UDP header). And in many usecases, there are only a couple of this (src,dst) MAC pairs. On Turris Omnia in most cases there are only 5 such pairs, because the user plugs into the router only 5 devices. So for each of these 5 (src,dst) MAC pairs, there is probability 1/2 that the packet will be sent via CPU port 0. So 1/32 probability that all packets will be sent via CPU port 0, and the same probability that all packets will be sent via CPU port 1. This means that in 1/16 of cases the LAG is useless in this scenario.
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Mon, 12 Apr 2021 23:22:45 +0200 Tobias Waldekranz wrote: > On Mon, Apr 12, 2021 at 21:30, Marek Behun wrote: > > On Mon, 12 Apr 2021 14:46:11 +0200 > > Tobias Waldekranz wrote: > > > >> I agree. Unless you only have a few really wideband flows, a LAG will > >> typically do a great job with balancing. This will happen without the > >> user having to do any configuration at all. It would also perform well > >> in "router-on-a-stick"-setups where the incoming and outgoing port is > >> the same. > > > > TLDR: The problem with LAGs how they are currently implemented is that > > for Turris Omnia, basically in 1/16 of configurations the traffic would > > go via one CPU port anyway. > > > > > > > > One potencial problem that I see with using LAGs for aggregating CPU > > ports on mv88e6xxx is how these switches determine the port for a > > packet: only the src and dst MAC address is used for the hash that > > chooses the port. > > > > The most common scenario for Turris Omnia, for example, where we have 2 > > CPU ports and 5 user ports, is that into these 5 user ports the user > > plugs 5 simple devices (no switches, so only one peer MAC address for > > port). So we have only 5 pairs of src + dst MAC addresses. If we simply > > fill the LAG table as it is done now, then there is 2 * 0.5^5 = 1/16 > > chance that all packets would go through one CPU port. > > > > In order to have real load balancing in this scenario, we would either > > have to recompute the LAG mask table depending on the MAC addresses, or > > rewrite the LAG mask table somewhat randomly periodically. (This could > > be in theory offloaded onto the Z80 internal CPU for some of the > > switches of the mv88e6xxx family, but not for Omnia.) > > I thought that the option to associate each port netdev with a DSA > master would only be used on transmit. Are you saying that there is a > way to configure an mv88e6xxx chip to steer packets to different CPU > ports depending on the incoming port? > > The reason that the traffic is directed towards the CPU is that some > kind of entry in the ATU says so, and the destination of that entry will > either be a port vector or a LAG. Of those two, only the LAG will offer > any kind of balancing. What am I missing? Via port vectors you can "load balance" by ports only, i.e. input port X -> trasmit via CPU port Y. When using LAGs, you are load balancing via hash(src MAC | dst mac) only. This is better in some ways. But what I am saying is that if the LAG mask table is static, as it is now implemented in mv88e6xxx code, then for many scenarios there is a big probability of no load balancing at all. For Turris Omnia for example there is 6.25% probability that the switch chip will send all traffic to the CPU via one CPU port. This is because the switch chooses the LAG port only from the hash of dst+src MAC address. (By the 1/16 = 6.25% probability I mean that for cca 1 in 16 customers, the switch would only use one port when sending data to the CPU). The round robin solution here is therefore better in this case.
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Mon, 12 Apr 2021 14:46:11 +0200 Tobias Waldekranz wrote: > I agree. Unless you only have a few really wideband flows, a LAG will > typically do a great job with balancing. This will happen without the > user having to do any configuration at all. It would also perform well > in "router-on-a-stick"-setups where the incoming and outgoing port is > the same. TLDR: The problem with LAGs how they are currently implemented is that for Turris Omnia, basically in 1/16 of configurations the traffic would go via one CPU port anyway. One potencial problem that I see with using LAGs for aggregating CPU ports on mv88e6xxx is how these switches determine the port for a packet: only the src and dst MAC address is used for the hash that chooses the port. The most common scenario for Turris Omnia, for example, where we have 2 CPU ports and 5 user ports, is that into these 5 user ports the user plugs 5 simple devices (no switches, so only one peer MAC address for port). So we have only 5 pairs of src + dst MAC addresses. If we simply fill the LAG table as it is done now, then there is 2 * 0.5^5 = 1/16 chance that all packets would go through one CPU port. In order to have real load balancing in this scenario, we would either have to recompute the LAG mask table depending on the MAC addresses, or rewrite the LAG mask table somewhat randomly periodically. (This could be in theory offloaded onto the Z80 internal CPU for some of the switches of the mv88e6xxx family, but not for Omnia.) Marek
Re: [PATCH RFC net-next 0/3] Multi-CPU DSA support
On Sat, 10 Apr 2021 15:34:46 +0200 Ansuel Smith wrote: > Hi, > this is a respin of the Marek series in hope that this time we can > finally make some progress with dsa supporting multi-cpu port. > > This implementation is similar to the Marek series but with some tweaks. > This adds support for multiple-cpu port but leave the driver the > decision of the type of logic to use about assigning a CPU port to the > various port. The driver can also provide no preference and the CPU port > is decided using a round-robin way. In the last couple of months I have been giving some thought to this problem, and came up with one important thing: if there are multiple upstream ports, it would make a lot of sense to dynamically reallocate them to each user port, based on which user port is actually used, and at what speed. For example on Turris Omnia we have 2 CPU ports and 5 user ports. All ports support at most 1 Gbps. Round-robin would assign: CPU port 0 - Port 0 CPU port 1 - Port 1 CPU port 0 - Port 2 CPU port 1 - Port 3 CPU port 0 - Port 4 Now suppose that the user plugs ethernet cables only into ports 0 and 2, with 1, 3 and 4 free: CPU port 0 - Port 0 (plugged) CPU port 1 - Port 1 (free) CPU port 0 - Port 2 (plugged) CPU port 1 - Port 3 (free) CPU port 0 - Port 4 (free) We end up in a situation where ports 0 and 2 share 1 Gbps bandwidth to CPU, and the second CPU port is not used at all. A mechanism for automatic reassignment of CPU ports would be ideal here. What do you guys think? Marek
Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver
On Thu, 25 Mar 2021 06:04:43 + Hermes Zhang wrote: > > LED_FULL / LED_OFF are deprecated, don't use them. > > Then could I use just 0 (instead LED_OFF) and led_cdev->max_brightness > > (instead of LED_FULL) here? The idea here is map the states defined in dts > > to the full brightness range. Yes, you can and should use 0 insted of LED_OFF. > > + priv->cdev.max_brightness = LED_FULL; > > max_brightness is not 255 (= LED_FULL). max_brightness must be > > derived from the led-states property. > > Yeah, I will fix this. the max-brightness should for the whole LED, > right? So > > it will at same level with led-states. max_brightness should be (the number of states - 1). I.e. if you have 4 gpios and the LED supports full 2^4 = 16 states, max brightness should be 15. Marek
Re: [PATCH 0/2] New multiple GPIOs LED driver
On Wed, 24 Mar 2021 15:56:29 +0800 Hermes Zhang wrote: > From: Hermes Zhang > > *** BLURB HERE *** "*** BLURB HERE ***" is meant to be substituted with your text :) All in all I think you are adding functionality which can be incorporated simply into the existing leds-gpio driver instead of new driver. Marek
Re: [PATCH 2/2] dt-binding: leds: Document leds-multi-gpio bindings
On Wed, 24 Mar 2021 15:56:31 +0800
Hermes Zhang wrote:
> From: Hermes Zhang
>
> Document the device tree bindings of the multiple GPIOs LED driver
> Documentation/devicetree/bindings/leds/leds-multi-gpio.yaml.
The dt-binding should come before the actual driver. Otherwise there
will be a commit with the driver existing but no documentation for its
bindings.
> +description:
> + This will support some LED made of multiple GPIOs and the brightness of the
> + LED could map to different states of the GPIOs.
Don't use future tense. Don't mention drivers in device tree
binding documentation, if it can be helped. The device tree binding
documentation should describe devices and their nodes... (and I see
that I did a similar mistake for the cznic,turris-omnia-leds.yaml
binding, I will have to fix that).
Instead the description should be something like this:
This binding represents LED devices which are controller with
multiple GPIO lines in order to achieve more than two brightness
states.
> +
> +properties:
> + compatible:
> +const: multi-gpio-led
> +
> + led-gpios:
> +description: Array of one or more GPIOs pins used to control the LED.
> +minItems: 1
> +maxItems: 8 # Should be enough
> +
> + led-states:
> +description: |
> + The array list the supported states here which will map to brightness
> + from 0 to maximum. Each item in the array will present all the GPIOs
> + value by bit.
Again future tense...
> +$ref: /schemas/types.yaml#/definitions/uint8-array
> +minItems: 1
> +maxItems: 16 # Should be enough
> +
> +required:
> + - compatible
> + - led-gpios
> + - led-states
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +gpios-led {
> + compatible = "multi-gpio-led";
> +
> + led-gpios = < 23 0x1>,
> + < 24 0x1>;
> + led-states = /bits/ 8 <0x00 0x01 0x02 0x03>;
> +};
> +...
Re: [PATCH 1/2] leds: leds-multi-gpio: Add multiple GPIOs LED driver
On Wed, 24 Mar 2021 15:56:30 +0800
Hermes Zhang wrote:
> From: Hermes Zhang
>
> Introduce a new multiple GPIOs LED driver. This LED will made of
> multiple GPIOs (up to 8) and will map different brightness to different
> GPIOs states which defined in dts file.
I wonder how many boards have such LEDs.
Also if it wouldn't be better to expand the original leds-gpio driver.
Probably depends on how much larger would such expansion make the
leds-gpio driver.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Why do you include slab.h?
> +
> +#define MAX_GPIO_NUM 8
> +
> +struct multi_gpio_led_priv {
> + struct led_classdev cdev;
> +
> + struct gpio_descs *gpios;
> +
> + u8 *states;
> + int nr_states;
> +};
Use flexible array members. Allocate with
devm_kzalloc(dev, struct_size(priv, states, priv->nr_states),
GFP_KERNEL)
> +
> +
> +static void multi_gpio_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct multi_gpio_led_priv *priv;
> + int idx;
> +
> + DECLARE_BITMAP(values, MAX_GPIO_NUM);
> +
> + priv = container_of(led_cdev, struct multi_gpio_led_priv, cdev);
> +
> + idx = (value - LED_OFF) * priv->nr_states / (LED_FULL + 1);
LED_FULL / LED_OFF are deprecated, don't use them.
> +
> + values[0] = priv->states[idx];
> +
> + gpiod_set_array_value(priv->gpios->ndescs, priv->gpios->desc,
> + priv->gpios->info, values);
> +}
> +
> +static int multi_gpio_led_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct device_node *node = dev->of_node;
> + struct multi_gpio_led_priv *priv = NULL;
> + int ret;
> + const char *state = NULL;
> + struct led_init_data init_data = {};
> +
> + priv = devm_kzalloc(dev, sizeof(struct multi_gpio_led_priv),
> GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->gpios = devm_gpiod_get_array(dev, "led", GPIOD_OUT_LOW);
> + if (IS_ERR(priv->gpios))
> + return PTR_ERR(priv->gpios);
> +
> + if (priv->gpios->ndescs >= MAX_GPIO_NUM) {
> + dev_err(dev, "Too many GPIOs\n");
> + return -EINVAL;
> + }
> +
> + ret = of_property_count_u8_elems(node, "led-states");
> + if (ret < 0)
> + return ret;
> +
> + priv->nr_states = ret;
> + priv->states = devm_kzalloc(dev, sizeof(*priv->states) *
> priv->nr_states, GFP_KERNEL);
> + if (!priv->states)
> + return -ENOMEM;
> +
> + ret = of_property_read_u8_array(node, "led-states", priv->states,
> priv->nr_states);
> + if (ret)
> + return ret;
> +
> + priv->cdev.max_brightness = LED_FULL;
max_brightness is not 255 (= LED_FULL). max_brightness must be
derived from the led-states property.
> + priv->cdev.default_trigger = of_get_property(node,
> "linux,default-trigger", NULL);
> + priv->cdev.brightness_set = multi_gpio_led_set;
> +
> + init_data.fwnode = of_fwnode_handle(node);
> +
> + ret = devm_led_classdev_register_ext(dev, >cdev, _data);
> + if (ret < 0)
> + return ret;
> +
> + of_property_read_string(node, "default-state", );
> + if (!strcmp(state, "on"))
> + multi_gpio_led_set(>cdev, LED_FULL);
> + else
> + multi_gpio_led_set(>cdev, LED_OFF);
Again LED_FULL and LED_OFF...
What about default-state = "keep" ?
Hermes, do you actually have a device that controls LEDs this way? How
many brightness options do they have?
Also I think this functionality could be easily incorporated into the
existing leds-gpio driver, instead of creating new driver.
Moreover your driver can control only one LED, so it needs to be
probed multiple times for multiple LEDs. Meanwhile the leds-gpio driver
can register multiple LEDs in one probe...
Marek
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
On Fri, 12 Mar 2021 08:48:55 + Hermes Zhang wrote: > Hi Alexander, > > > Am Donnerstag, 11. März 2021, 14:04:08 CET schrieb Hermes Zhang: > > > From: Hermes Zhang > > > > > > Introduce a new Dual GPIO LED driver. These two GPIOs LED will act as > > > one LED as normal GPIO LED but give the possibility to change the > > > intensity in four levels: OFF, LOW, MIDDLE and HIGH. > > > > Interesting use case. Is there any real world hardware wired like that you > > could point to? > > > > Yes, we have the HW, it's not a chip but just some circuit to made of. > > > > +config LEDS_DUAL_GPIO > > > + tristate "LED Support for Dual GPIO connected LEDs" > > > + depends on LEDS_CLASS > > > + depends on GPIOLIB || COMPILE_TEST > > > + help > > > + This option enables support for the two LEDs connected to GPIO > > > + outputs. These two GPIO LEDs act as one LED in the sysfs and > > > + perform different intensity by enable either one of them or both. > > > > Well, although I never had time to implement that, I suspect that could > > conflict if someone will eventually write a driver for two pin dual color > > LEDs > > connected to GPIO pins. We actually do that on our hardware and I know > > others do, too. > > > > I asked about that back in 2019, see this thread: > > > > https://www.spinics.net/lists/linux-leds/msg11665.html > > > > At the time the multicolor framework was not yet merged, so today I would > > probably make something which either uses the multicolor framework or at > > least has a similar interface to userspace. However, it probably won't > > surprise > > you all, this is not highest priority on my ToDo list. ;-) > > > > (What we actually do is pretend those are separate LEDs and ignore the > > conflicting case where both GPIOs are on and the LED is dark then.) > > > > Yes, that case seems conflict with mine, the pattern for me is like: > > P1 | P2 | LED > -- + -- + - > 0 | 0 | off > 0 | 1 | Any color > 1 | 0 | Any color > 1 | 1 | both on > > Now I'm investigate another way from Marek's suggestion by using > REGULATOR_GPIO, to see if could meet my requirement. If yes, then I do think > no new driver is needed. Maybe you could even implement multicolor-gpio, now that we have multicolor LED class :) Marek
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
On Fri, 12 Mar 2021 04:48:52 +
Hermes Zhang wrote:
> >
> > Sorry, leds-regulator has only a binary state LED.
> >
> > Maybe you could extend leds-regulator to be able to use all regulator
> > states?
> >
> > Or you can extend leds-gpio driver to support N states via log N gpios,
> > instead of adding new driver.
>
> It seems a good idea to extend leds-gpio, so in my case, I should have such
> dts:
>
> 63 leds {
> 64 compatible = "gpio-leds";
> 65
> 66 recording_front {
> 67 label = "recording_front:red";
> 68 gpios = < 130 GPIO_ACTIVE_HIGH>, < 129
> GPIO_ACTIVE_HIGH>;
> 69 default-state = "off";
> 70 };
> 71 };
>
> For my case, two leds is enough, but it sill easy to extend the support
> number bigger than two. And the length of gpios array is not fixed, so it
> could compatible with exist "gpio-leds" dts, right?
>
> If this idea work, should I create a new commit or still in this track (V2)?
However you want :)
Look at the states property of gpio regulator:
https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.yaml
It is possible to have a multi-GPIO LED which brightness is set via N
GPIOs and it has 2^N brightness states encoded by binary values of
those GPIOs, but it is entirely possible to have less than 2^N states,
or that the states are encoded in a different way.
In the first version though imlpemenent just the simplest case: N GPIOs
with 2^N states.
Marek
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
On Thu, 11 Mar 2021 16:38:14 +0100 Marek Behun wrote: > LED_FULL, LED_HALF, LED_OFF are deprecated. > > And this driver is redundant. This can be done with leds-regulator, > with a gpio-regulator. > > Marek Sorry, leds-regulator has only a binary state LED. Maybe you could extend leds-regulator to be able to use all regulator states? Or you can extend leds-gpio driver to support N states via log N gpios, instead of adding new driver. Marek
Re: [PATCH] leds: leds-dual-gpio: Add dual GPIO LEDs driver
LED_FULL, LED_HALF, LED_OFF are deprecated. And this driver is redundant. This can be done with leds-regulator, with a gpio-regulator. Marek
Re: [PATCH] Leds: made enum led_brightness into typedef
This patch touches only code in drivers/leds and include/linux/leds.h. Meanwhile enum led_brightness is used in many other parts of kernel as well, just try git grep "enum led_brightness" Also changing it probably to a simple int would be better. But if we wanted a typedef anyway, it should be called led_brightness_t. Marek
Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT
On Wed, 3 Feb 2021 17:35:55 +0100
Pavel Machek wrote:
> On Wed 2021-02-03 15:39:59, Sven Schuchmann wrote:
> > Hello Pavel,
> >
> > > > In order to use a multicolor-led together with a trigger
> > > > from DT the led needs to have an intensity set to see something.
> > > > The trigger changes the brightness of the led but if there
> > > > is no intensity we actually see nothing.
> > > >
> > > > This patch adds the ability to set the default intensity
> > > > of each led so that it is turned on from DT.
> > >
> > > Do we need this to be configurable from device tree? Can we just set
> > > it to max or something?
> > >
> > > Aha, this basically sets the initial color for LEDs the monochromatic
> > > triggers, right?
> >
> > Let me try to explain in other words: I have one RGB-LED
> > which consists of 3 Colors. Each of the three colors (Red, Green, Blue) you
> > have
> > to define in the DT. For example this is my setup for one RGB-LED which I
> > wanted
> > to show the heartbeat in Red (half intensity):
> >
> > multi-led@3 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <0x3>;
> > color = ;
> >
> > linux,default-trigger = "heartbeat";
> > function = LED_FUNCTION_HEARTBEAT;
> >
> > led-9 {
> > color = ;
> > default-intensity = <100>;
> > };
> >
> > led-10 {
> > color =
> > ;
> > };
> >
> > led-11 {
> > color = ;
> > };
> > };
> >
> > If I would not have the default-intensity I would actually see nothing,
> > since the intensity (which goes from 0-255) of each led is initialized with
> > 0.
> >
> > I hope I could clarify this a little more?
>
> Yes, sounds reasonable. Could we get default intensity of 100% on all
> channels if nothing else is specified?
>
> Or maybe simply "if intensity is not specified, start with 100%, and
> use explicit =0 if other color is expected".
>
> Best regards,
> Pavel
Is the property default-intensity documented in DT bindings?
Wouldn't it be better if the property was used in the multi-led node
instead of the channel node? I.e.
multi-led@3 {
color = ;
default-intensity = <100 0 0>;
};
Re: [PATCH] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720
Hi Pali and Miquel,
On Wed, 23 Dec 2020 17:24:03 +0100
Pali Rohár wrote:
> int xhci_mvebu_a3700_init_quirk(struct usb_hcd *hcd)
> {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + struct device *dev = hcd->self.controller;
> + struct phy *phy;
> + int ret;
>
> /* Without reset on resume, the HC won't work at all */
> xhci->quirks |= XHCI_RESET_ON_RESUME;
>
> + /* Old bindings miss the PHY handle */
> + phy = of_phy_get(dev->of_node, "usb3-phy");
> + if (IS_ERR(phy) && PTR_ERR(phy) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + else if (IS_ERR(phy))
> + goto phy_out;
> +
> + ret = phy_init(phy);
> + if (ret)
> + goto phy_put;
> +
> + ret = phy_set_mode(phy, PHY_MODE_USB_HOST_SS);
> + if (ret)
> + goto phy_exit;
> +
> + ret = phy_power_on(phy);
> + if (ret == -EOPNOTSUPP) {
> + /* Skip initializatin of XHCI PHY when it is unsupported by
> firmware */
> + dev_warn(dev, "PHY unsupported by firmware\n");
> + xhci->quirks |= XHCI_SKIP_PHY_INIT;
> + }
> + if (ret)
> + goto phy_exit;
I am not sure if this is the correct way to check whether PHY_INIT
should be skipped.
Moreover the subsequent phy_power_off:
> +
> + phy_power_off(phy);
won't power off the PHY, because the corresponding handler in ATF is
currently empty.
I guess the patch needs to be in kernel if users are unwilling to upgrade
ATF firmware.
The SMC calls for Marvell's comphy are designed to be generic for
several Marvell platforms (the constants are the same and so one), but
we still have different drivers for them anyway.
Maybe it would be better to just not use the ATF implementation at all,
and implement the comphy driver for A3720 entirely in kernel...
Miquel, what do you think?
Marek
Re: [PATCH -next] drivers: leds: simplify the return expression of register_nasgpio_led()
subject prefix should be leds: ss4200: simplify the return expression of register_nasgpio_led
Re: [PATCH] leds: led-core: Get rid of enum led_brightness
On Fri, 11 Dec 2020 14:08:43 + David Laight wrote: > > More than 8 bits would be good. > While not really relevant for actual 'brightness' it allows > for 'strange' things be encoded in the brightness field. > > For instance we have some hardware that has RGB leds on it. > They are a single device so it really needs a colour property. > But it is more complex than that, between the driver and LED > there is an FPGA - so it can modulate the LED output in many ways. > As well as using PWM to change the brightness and (eg) 1/2HZ flashing > it is possible to alternate between red and green to get a reasonable > orange (works better than driving both at the same time!). Please don't do that. Don't misuse brightness for other settings. Instead try to implement the settings in other sysfs files, maybe even make it generic. Document the new sysfs ABI. But to not encode non-brightness properties into brightness. For you multicolor example there is multicolor LED framework now in kernel. Marek
Re: [PATCH] leds: led-core: Get rid of enum led_brightness
On Fri, 11 Dec 2020 03:48:40 +0200 Abanoub Sameh wrote: > This gets rid of enum led_brightness in the main led files, > because it is deprecated, and an int can be used instead, > or maybe even a uint8_t since it only goes up to 255. > Next we can also patch the other files to get rid of it completely. 1. unsigned int should be used IMO - using int may force all implementers to check for negative value and return -EINVAL, which is stupid - some LED controllers may offer more than 8bit brightness value, so no uint8_t 2. I think we should remove all usages with one commit Marek
Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree
On Wed, 25 Nov 2020 11:42:42 +0100 Pavel Machek wrote: > Hi! > > > Allow setting netdev LED trigger as default when given LED DT node has > > the `trigger-sources` property pointing to a node corresponding to a > > network device. > > > > The specific netdev trigger mode is determined from the `function` LED > > property. > > Sounds reasonable. > > > + netdev = of_find_net_device_by_node(args.np); > > + if (!netdev) > > + return false; > > + > > + np = dev_of_node(led_cdev->dev); > > + if (!np) > > + return false; > > Missing of_node_put? I will look into this. > > > +++ b/include/dt-bindings/leds/common.h > > @@ -77,6 +77,7 @@ > > #define LED_FUNCTION_HEARTBEAT "heartbeat" > > #define LED_FUNCTION_INDICATOR "indicator" > > #define LED_FUNCTION_LAN "lan" > > +#define LED_FUNCTION_LINK "link" > > #define LED_FUNCTION_MAIL "mail" > > #define LED_FUNCTION_MTD "mtd" > > #define LED_FUNCTION_PANIC "panic" > > We have function "lan" already defined; "link" would do mostly same > thing. Should we use "lan"? Or should we delete "lan" and replace it > with "link"? Removal of "lan" should depend on whether it is used somewhere... But I think "link" is more sensible since: - it can distinguish better from "activity" if "activity" should mean blinking on RX/TX - the interface must not necessarily be a LAN (in the sense that on routers we have WAN and LAN and possibly other, user defined networks). We could use "lan" though to mean "link" and "activity" together (ie. LED on when interface linked, LED blinking on RX/TX). We have to decide whether specifying more LED functions to one LED in DT should look like: function = LED_FUNCTION_LINK, LED_FUNCTION_ACTIVITY; or rather like function = LED_FUNCTION_LAN; The problem with the second one is that we would need specific functions for different compound modes (if we wanted to do that), eg: function = LED_FUNCTION_LAN_RX; Marek
Re: [PATCH v10 4/4] net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell
Hi Andrew, On Fri, 20 Nov 2020 02:29:06 +0100 Andrew Lunn wrote: > > + if (speed >= 2500 && port > 0 && port < 9) > > + return -EOPNOTSUPP; > > Maybe i'm missing something, but it looks like at this point you can > call > > return mv88e6xxx_port_set_speed_duplex(chip, port, speed, true, true, > duplex); He can't. That function does not support speed 5000. You can't simply add it, because it clashes with register value for speed 2500 on previous switches (Peridot, Topaz). Amethyst reg valPeridot + Topaz reg val 2500SPD_1000 | ALT_BIT SPD_1 | ALT_BIT 5000SPD_1 | ALT_BIT not supported 1 SPD_UNFORCEDSPD_UNFORCED When I sent my proposal for Amethyst I somehow did it, and you commented [1]: > This is getting more and more complex. Maybe it is time to refactor it? And I agree :) Marek [1] https://www.spinics.net/lists/netdev/msg678090.html
Re: [PATCH] leds: various: add missing put_device() call in netxbig_leds_get_of_pdata()
On Thu, 29 Oct 2020 18:49:52 +0100 Pavel Machek wrote: > > Thanks, applied. > > But it seems to me similar handling is needed in "success" paths, no? > > Best regards, > Pavel Pavel, the subject of this commit is wrong. It should begin with leds: netxbig: instead of leds: various: since the patch does not touch various drivers, only one: netxbig. Marek
Re: [PATCH v6 0/4] Add support for mv88e6393x family of Marvell
On Thu, 29 Oct 2020 15:40:25 +1000 Pavana Sharma wrote: > Updated patchset. > > Split the patch to separate mv88e6393 changes from refactoring > serdes_get_lane. > Update Documentation before adding new mode. Pavana, the patch adding support for Amethyst has to be the last in the series. The patch refactoring the serdes_get_lane code must go before the patch adding the support for the new family, because Amethyst already needs this functionality.
Re: [PATCH v6 3/4] net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell
On Thu, 29 Oct 2020 15:42:29 +1000
Pavana Sharma wrote:
> The Marvell 88E6393X device is a single-chip integration of a 11-port
> Ethernet switch with eight integrated Gigabit Ethernet (GbE) transceivers
> and three 10-Gigabit interfaces.
>
> This patch adds functionalities specific to mv88e6393x family (88E6393X,
> 88E6193X and 88E6191X)
>
> Co-developed-by: Ashkan Boldaji
> Signed-off-by: Ashkan Boldaji
> Signed-off-by: Pavana Sharma
> ---
> Changes in v2:
> - Fix a warning (Reported-by: kernel test robot )
> Changes in v3:
> - Fix 'unused function' warning
> Changes in v4, v5, v6:
> - Incorporated feedback from maintainers.
> ---
> drivers/net/dsa/mv88e6xxx/chip.c| 91 +++
> drivers/net/dsa/mv88e6xxx/chip.h| 4 +
> drivers/net/dsa/mv88e6xxx/global1.h | 2 +
> drivers/net/dsa/mv88e6xxx/global2.h | 8 +
> drivers/net/dsa/mv88e6xxx/port.c| 234
> drivers/net/dsa/mv88e6xxx/port.h| 43 -
> drivers/net/dsa/mv88e6xxx/serdes.c | 225 ++
> drivers/net/dsa/mv88e6xxx/serdes.h | 39 +
> 8 files changed, 644 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> b/drivers/net/dsa/mv88e6xxx/chip.c
> index f0dbc05e30a4..de96fd08e77a 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -634,6 +634,24 @@ static void mv88e6390x_phylink_validate(struct
> mv88e6xxx_chip *chip, int port,
> mv88e6390_phylink_validate(chip, port, mask, state);
> }
>
> +static void mv88e6393x_phylink_validate(struct mv88e6xxx_chip *chip, int
> port,
> + unsigned long *mask,
> + struct phylink_link_state *state)
> +{
> + if (port == 0 || port == 9 || port == 10) {
> + phylink_set(mask, 1baseT_Full);
> + phylink_set(mask, 1baseKR_Full);
> + phylink_set(mask, 5000baseT_Full);
> + phylink_set(mask, 2500baseX_Full);
> + phylink_set(mask, 2500baseT_Full);
> + }
> +
> + phylink_set(mask, 1000baseT_Full);
> + phylink_set(mask, 1000baseX_Full);
> +
> + mv88e6065_phylink_validate(chip, port, mask, state);
> +}
> +
> static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
> unsigned long *supported,
> struct phylink_link_state *state)
> @@ -4141,6 +4159,56 @@ static const struct mv88e6xxx_ops mv88e6191_ops = {
> .phylink_validate = mv88e6390_phylink_validate,
> };
>
> +static const struct mv88e6xxx_ops mv88e6193x_ops = {
> + /* MV88E6XXX_FAMILY_6393 */
> + .setup_errata = mv88e6393x_setup_errata,
> + .irl_init_all = mv88e6390_g2_irl_init_all,
> + .get_eeprom = mv88e6xxx_g2_get_eeprom8,
> + .set_eeprom = mv88e6xxx_g2_set_eeprom8,
> + .set_switch_mac = mv88e6xxx_g2_set_switch_mac,
> + .phy_read = mv88e6xxx_g2_smi_phy_read,
> + .phy_write = mv88e6xxx_g2_smi_phy_write,
> + .port_set_link = mv88e6xxx_port_set_link,
> + .port_set_speed_duplex = mv88e6393x_port_set_speed_duplex,
> + .port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
> + .port_tag_remap = mv88e6390_port_tag_remap,
> + .port_set_frame_mode = mv88e6351_port_set_frame_mode,
> + .port_set_egress_floods = mv88e6352_port_set_egress_floods,
> + .port_set_ether_type = mv88e6393x_port_set_ether_type,
> + .port_set_jumbo_size = mv88e6165_port_set_jumbo_size,
> + .port_egress_rate_limiting = mv88e6097_port_egress_rate_limiting,
> + .port_pause_limit = mv88e6390_port_pause_limit,
> + .port_set_cmode = mv88e6393x_port_set_cmode,
> + .port_disable_learn_limit = mv88e6xxx_port_disable_learn_limit,
> + .port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
> + .port_get_cmode = mv88e6352_port_get_cmode,
> + .stats_snapshot = mv88e6390_g1_stats_snapshot,
> + .stats_set_histogram = mv88e6390_g1_stats_set_histogram,
> + .stats_get_sset_count = mv88e6320_stats_get_sset_count,
> + .stats_get_strings = mv88e6320_stats_get_strings,
> + .stats_get_stats = mv88e6390_stats_get_stats,
> + .set_cpu_port = mv88e6393x_port_set_cpu_dest,
> + .set_egress_port = mv88e6393x_set_egress_port,
> + .watchdog_ops = _watchdog_ops,
> + .mgmt_rsvd2cpu = mv88e6393x_port_mgmt_rsvd2cpu,
> + .pot_clear = mv88e6xxx_g2_pot_clear,
> + .reset = mv88e6352_g1_reset,
> + .rmu_disable = mv88e6390_g1_rmu_disable,
> + .vtu_getnext = mv88e6390_g1_vtu_getnext,
> + .vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
> + .serdes_power = mv88e6393x_serdes_power,
> + .serdes_get_lane = mv88e6393x_serdes_get_lane,
> + /* Check status register pause & lpa register */
Is this a TODO comment? If so, it should be marked as such.
> + .serdes_pcs_get_state = mv88e6390_serdes_pcs_get_state,
> + .serdes_irq_mapping = mv88e6390_serdes_irq_mapping,
> + .serdes_irq_enable
Re: [PATCH v6 2/4] net: phy: Add 5GBASER interface mode
On Thu, 29 Oct 2020 15:42:00 +1000 Pavana Sharma wrote: > Add new mode supported by MV88E6393 family. > This commit message isn't ideal. It infers that the Amethyst is first such device to implement this mode, which is not true. The 5gbase-r mode is supported by various other hardware, for example Marvell's 88X3310 PHY. Just say: Add 5gbase-r PHY interface mode. > PHY_INTERFACE_MODE_2500BASEX, > PHY_INTERFACE_MODE_RXAUI, > PHY_INTERFACE_MODE_XAUI, > + PHY_INTERFACE_MODE_5GBASER, > /* 10GBASE-R, XFI, SFI - single lane 10G Serdes */ > PHY_INTERFACE_MODE_10GBASER, > PHY_INTERFACE_MODE_USXGMII, The position is IMO out of order. RXAUI and XAUI are both 10G modes, so 5gbase-r should be between 2500base-x and rxaui. > @@ -187,6 +188,8 @@ static inline const char *phy_modes(phy_interface_t > interface) > return "rxaui"; > case PHY_INTERFACE_MODE_XAUI: > return "xaui"; > + case PHY_INTERFACE_MODE_5GBASER: > + return "5gbase-r"; > case PHY_INTERFACE_MODE_10GBASER: > return "10gbase-r"; > case PHY_INTERFACE_MODE_USXGMII: Here as well.
Re: [PATCH v5 3/3] net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell
Pavana, please add me to Cc for this. Does USXGMII mode work? There are some erratas for for 10gb serdes mode. Also you should split this patch. The code that refactores the serdes_get_lane methods should be in a separate patch. I have a device with this switch and also a SFP module which can operate with USXGMII. I would like to get USXGMII mode to work before this support is added, if possible. For some reason though I was unable to get it to work, I will have to look at those erratas. Marek
Re: [PATCH v3 1/3] net: dsa: mv88e6xxx: Don't force link when using in-band-status
On Tue, 20 Oct 2020 15:15:25 +0100 Russell King - ARM Linux admin wrote: > On Tue, Oct 20, 2020 at 04:05:35PM +0200, Andrew Lunn wrote: > > On Tue, Oct 20, 2020 at 03:49:40PM +0200, Marek Behun wrote: > > > On Tue, 20 Oct 2020 11:15:52 +0100 > > > Russell King - ARM Linux admin wrote: > > > > > > > On Tue, Oct 20, 2020 at 04:45:56PM +1300, Chris Packham wrote: > > > > > When a port is configured with 'managed = "in-band-status"' don't > > > > > force > > > > > the link up, the switch MAC will detect the link status correctly. > > > > > > > > > > Signed-off-by: Chris Packham > > > > > Reviewed-by: Andrew Lunn > > > > > > > > I thought we had issues with the 88E6390 where the PCS does not > > > > update the MAC with its results. Isn't this going to break the > > > > 6390? Andrew? > > > > > > > > > > Russell, I tested this patch on Turris MOX with 6390 on port 9 (cpu > > > port) which is configured in devicetree as 2500base-x, in-band-status, > > > and it works... > > > > > > Or will this break on user ports? > > > > User ports is what needs testing, ideally with an SFP. > > > > There used to be explicit code which when the SERDES reported link up, > > the MAC was configured in software with the correct speed etc. With > > the move to pcs APIs, it is less obvious how this works now, does it > > still software configure the MAC, or do we have the right magic so > > that the hardware updates itself. > > It's still there. The speed/duplex etc are read from the serdes PHY > via mv88e6390_serdes_pcs_get_state(). When the link comes up, we > pass the negotiated link parameters read from there to the link_up() > functions. For ports where mv88e6xxx_port_ppu_updates() returns false > (no external PHY) we update the port's speed and duplex setting and > (currently, before this patch) force the link up. > > That was the behaviour before I converted the code, the one that you > referred to. I had assumed the code was correct, and _none_ of the > speed, duplex, nor link state was propagated from the serdes PCS to > the port on the 88E6390 - hence why the code you refer to existed. > Russell, you are right. SFP on 88E6390 does not work with this patch applied. So this patch breaks 88E6390. Marek
Re: [PATCH v3 1/3] net: dsa: mv88e6xxx: Don't force link when using in-band-status
On Tue, 20 Oct 2020 11:15:52 +0100 Russell King - ARM Linux admin wrote: > On Tue, Oct 20, 2020 at 04:45:56PM +1300, Chris Packham wrote: > > When a port is configured with 'managed = "in-band-status"' don't force > > the link up, the switch MAC will detect the link status correctly. > > > > Signed-off-by: Chris Packham > > Reviewed-by: Andrew Lunn > > I thought we had issues with the 88E6390 where the PCS does not > update the MAC with its results. Isn't this going to break the > 6390? Andrew? > Russell, I tested this patch on Turris MOX with 6390 on port 9 (cpu port) which is configured in devicetree as 2500base-x, in-band-status, and it works... Or will this break on user ports?
Re: [PATCH v2] leds: lm3697: Rename struct into more appropriate name
On Sat, 10 Oct 2020 20:57:00 +0200 Pavel Machek wrote: > On Fri 2020-10-09 15:51:35, Gabriel David wrote: > > The mentioned struct, lm3697_led, was renamed to lm3697_bank since the > > structure is representing the control banks. This name, in my opinion, > > is more semantically correct. The pointers referring to it were also > > renamed. > > > Signed-off-by: Gabriel David > > --- > > Yes, this is the same Gabriel David from ultracool...@tutanota.org and > > ultracool...@disroot.org. If you want me to confirm it I'll gladly do > > it. > > No problem with that, and no need to resend. This can proably wait > for 5.11... > > I'd like some comment from Dan... and perhaps I'd want to understand > what the difference between LED and bank is. > > ...there can be more than one LED connected to the given bank, that's > what you are pointing out? > > ...but these LEDs will always work in unison, and they are handled as > single LED by Linux, right? > Pavel, the controller can connect 3 LED strips (to 3 different pins on the chip). There are 2 LED control banks (this is where you can set brightness). For each LED strip (each output pin) you can configure to which control bank it connects. So you have 3 LED strips and 2 control banks, that is 2^3 = 8 different configurations of connecting LED control bank to LED strip. From the perspective of Linux you see the two control banks as 2 LED class devices (because you are setting brightness for control banks, not for the LED strips). Marek
Re: [PATCH] lm3697: Rename struct into more appropiate name
On Mon, 5 Oct 2020 22:17:14 +0200 (CEST)
ultracool...@tutanota.com wrote:
> Subject says it all. This rename was briefly discussed in this other patch:
> https://www.spinics.net/lists/linux-leds/msg16865.html (I don't know another
> way to link to emails, so I'll just use this archive).
>
> Feel free to suggest another name for the commit; that was just the better
> name I could come up with :/ .
>
>
>
>
Gabriel,
the subject of the patch should be
leds: lm3697: Rename struct into more appropiate name
("leds: " is prefixed). Look at history
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/leds?h=v5.9-rc8
The commit message should mention why you are renaming the type
(something like "to be semantically more correct, since that structure
represents LED control bank as described by the datasheet").
Also it seems that you are using git format-patch for generating patch
files, but you are sending these patches as regular e-mail attachements.
You should instead use git send-email, as is normally required
for kernel patches (and they would also appear in patchwork
(https://patches.linaro.org/project/linux-leds/list/). Please look at
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
and https://git-send-email.io/.
Marek
Re: [PATCH] leds: lm3697: Fix out-of-bound access
On Tue, 6 Oct 2020 09:57:08 -0500 Dan Murphy wrote: > Unfortunately we cannot and should not change the ABI now. > > Using the led-sources as the bank indicator does not conform to the > definition of the description of the led-sources in the yaml. > > The preference was to use the led-sources to define the LED out to the bank. > > Here is the conversation on how the driver got to where it is. > > https://lore.kernel.org/patchwork/patch/972337/ > > Dan > Oh, if this was discussed already, then never mind. Sorry for taking your time. Marek
Re: [PATCH] leds: lm3697: Fix out-of-bound access
Adding Rob to Cc, Rob, could we have your opinion on this? Mine is below. On Tue, 6 Oct 2020 07:21:14 -0500 Dan Murphy wrote: > >> By the way I just realized that the DT binding in this driver seems > >> incorrect to me. > >> > >> The controller logically supports 3 LED strings, each having > >> configurable control bank. > > There are two control banks. You can connect the HVLED outputs to either > control bank A or B there is no individual control of the LED strings. > > > >> But the DT binding supports 2 DT nodes, one for each control bank > >> (identified by the `reg` property) and then `led-sources` says which > >> string should be controlled by given bank. > >> > >> But taking in mind that DT should describe how devices are connected to > >> each other, I think the child nodes in the binding should instead > >> describe the 3 supported LED strings... > > The outputs in this case are virtual outputs which are the banks (A and B). > > Since the device is bank controlled the actual current sinks are not > defined thus making the the banks the actual outputs. > > This is why the 'reg' property defines the control bank either A or B > and the led-sources indicates the strings associated with the control bank. > > Dan > Dan, I looked at the datasheet, I understand this. Nonetheless, device tree should describe how devices are connected to each other. The chip has 3 pins for 3 LED strings. If this controller should be able to support 3 LED strings via 3 outputs, the device tree binding nodes should, in my opinion, describe each pin connected string. The nodes should maybe even be called 'led-string@N' where N is from [0, 1, 2]. The fact that the device is bank controlled and there are only two banks (and it is configurable by which bank each LED string is controlled) is more relevant to the driver, not as much to device tree binding. But yes, I do realize that if we had 3 child nodes, and the driver created 3 LEDs, then changing brithrness on one of the 3 LEDs would change brightness on at least another one, which we do not want. Maybe this driver could parse these 3 `led-string` nodes, each having defined bank via `led-sources`, and then register LED classdevs for each bank that is mentioned. This way the device tree would be more correct, IMO, and the driver would not have the problem mentioned in the paragraph above. Marek
Re: [PATCH] leds: lm3697: Fix out-of-bound access
By the way I just realized that the DT binding in this driver seems incorrect to me. The controller logically supports 3 LED strings, each having configurable control bank. But the DT binding supports 2 DT nodes, one for each control bank (identified by the `reg` property) and then `led-sources` says which string should be controlled by given bank. But taking in mind that DT should describe how devices are connected to each other, I think the child nodes in the binding should instead describe the 3 supported LED strings... Marek
Re: [PATCH] leds: lm3697: Fix out-of-bound access
On Sat, 3 Oct 2020 15:02:51 +0200 (CEST)
ultracool...@tutanota.com wrote:
> From 0dfd5ab647ccbc585c543d702b44d20f0e3fe436 Mon Sep 17 00:00:00 2001
> From: Ultracoolguy
> Date: Fri, 2 Oct 2020 18:27:00 -0400
> Subject: [PATCH] leds:lm3697:Fix out-of-bound access
>
> If both led banks aren't used in device tree,
> an out-of-bounds condition in lm3697_init occurs
> because of the for loop assuming that all the banks are used.
> Fix it by adding a variable that contains the number of used banks.
>
> Signed-off-by: Ultracoolguy
> ---
> drivers/leds/leds-lm3697.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
> index 024983088d59..a4ec2b6077e6 100644
> --- a/drivers/leds/leds-lm3697.c
> +++ b/drivers/leds/leds-lm3697.c
> @@ -56,7 +56,7 @@ struct lm3697_led {
> struct ti_lmu_bank lmu_data;
> int control_bank;
> int enabled;
> - int num_leds;
> + int num_led_strings;
OK, I looked at the datasheet for this controlled. The controlled can
control 3 LED strings, each having several LEDs connected in series.
But only 2 different brightnesses can be set (control bank), so for each
string there is a register setting which control bank should control it.
The Control Bank is set via the `reg` DT property (reg=0 means
Control Bank A, reg=1 means Control Bank B). The `led-sources`
property defines which strings should be controlled by each bank.
So I think this variable name should stay num_leds (as in number of leds
in this control bank).
The structure though should be renamed:
struct lm3697_led -> struct lm3697_bank.
> };
>
> /**
> @@ -78,6 +78,7 @@ struct lm3697 {
> struct mutex lock;
>
> int bank_cfg;
> + int num_leds;
This should be named num_banks.
>
> struct lm3697_led leds[];
This variable should be named banks, i.e.:
struct lm3697_bank banks[];
> };
> @@ -180,7 +181,7 @@ static int lm3697_init(struct lm3697 *priv)
> if (ret)
> dev_err(>client->dev, "Cannot write OUTPUT config\n");
>
> - for (i = 0; i < LM3697_MAX_CONTROL_BANKS; i++) {
> + for (i = 0; i < priv->num_leds; i++) {
Ultracoolguy is correct that this for cycle should not iterate
LM3697_MAX_CONTROL_BANKS. Instead, the count check in lm3697_probe should be
changed from
if (!count)
to
if (!count || count > LM3697_MAX_CONTROL_BANKS)
(the error message should also be changed, or maybe dropped, and the
error code changed from -ENODEV to -EINVAL, if we use || operator).
> led = >leds[i];
> ret = ti_lmu_common_set_ramp(>lmu_data);
> if (ret)
> @@ -244,22 +245,22 @@ static int lm3697_probe_dt(struct lm3697 *priv)
> led->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB +
> led->control_bank * 2;
>
> - led->num_leds = fwnode_property_count_u32(child, "led-sources");
> - if (led->num_leds > LM3697_MAX_LED_STRINGS) {
> + led->num_led_strings = fwnode_property_count_u32(child,
> "led-sources");
> + if (led->num_led_strings > LM3697_MAX_LED_STRINGS) {
> dev_err(>client->dev, "Too many LED strings
> defined\n");
> continue;
> }
>
> ret = fwnode_property_read_u32_array(child, "led-sources",
> led->hvled_strings,
> - led->num_leds);
> + led->num_led_strings);
> if (ret) {
> dev_err(>client->dev, "led-sources property
> missing\n");
> fwnode_handle_put(child);
> goto child_out;
> }
>
> - for (j = 0; j < led->num_leds; j++)
> + for (j = 0; j < led->num_led_strings; j++)
> priv->bank_cfg |=
> (led->control_bank << led->hvled_strings[j]);
>
> @@ -317,6 +318,8 @@ static int lm3697_probe(struct i2c_client *client,
> if (!led)
> return -ENOMEM;
>
> + led->num_leds = count;
> +
> mutex_init(>lock);
> i2c_set_clientdata(client, led);
>
> --
> 2.28.0
>
Marek
Re: [PATCH v2 2/7] drivers: mfd: Add a driver for iEi WT61P803 PUZZLE MCU
On Sat, 26 Sep 2020 15:55:09 +0200
Luka Kovacic wrote:
> Add a driver for the iEi WT61P803 PUZZLE microcontroller, used in some
> iEi Puzzle series devices. The microcontroller controls system power,
> temperature sensors, fans and LEDs.
>
> This driver implements the core functionality for device communication
> over the system serial (serdev bus). It handles MCU messages and the
> internal MCU properties. Some properties can be managed over sysfs.
>
Hi Luka,
this patch produces some checkpatch warnings, some of them are
reasonable.
My thought below:
> Signed-off-by: Luka Kovacic
> Cc: Luka Perkov
> Cc: Robert Marko
> ---
> drivers/mfd/Kconfig |8 +
> drivers/mfd/Makefile|1 +
> drivers/mfd/iei-wt61p803-puzzle.c | 1069 +++
> include/linux/mfd/iei-wt61p803-puzzle.h | 69 ++
> 4 files changed, 1147 insertions(+)
> create mode 100644 drivers/mfd/iei-wt61p803-puzzle.c
> create mode 100644 include/linux/mfd/iei-wt61p803-puzzle.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df0837ab41..b1588845894e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2118,5 +2118,13 @@ config SGI_MFD_IOC3
> If you have an SGI Origin, Octane, or a PCI IOC3 card,
> then say Y. Otherwise say N.
>
> +config MFD_IEI_WT61P803_PUZZLE
> + tristate "iEi WT61P803 PUZZLE MCU driver"
> + depends on SERIAL_DEV_BUS
> + help
> + iEi WT61P803 PUZZLE is a system power management microcontroller
> + used for fan control, temperature sensor reading, LED control
> + and system identification.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f835283..33b88023a68d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -236,6 +236,7 @@ obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
> obj-$(CONFIG_MFD_DLN2) += dln2.o
> obj-$(CONFIG_MFD_RT5033) += rt5033.o
> obj-$(CONFIG_MFD_SKY81452) += sky81452.o
> +obj-$(CONFIG_MFD_IEI_WT61P803_PUZZLE)+= iei-wt61p803-puzzle.o
>
> intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o
> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> diff --git a/drivers/mfd/iei-wt61p803-puzzle.c
> b/drivers/mfd/iei-wt61p803-puzzle.c
> new file mode 100644
> index ..5cba010ac9b9
> --- /dev/null
> +++ b/drivers/mfd/iei-wt61p803-puzzle.c
> @@ -0,0 +1,1069 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* iEi WT61P803 PUZZLE MCU Driver
> + * System management microcontroller for fan control, temperature sensor
> reading,
> + * LED control and system identification on iEi Puzzle series ARM-based
> appliances.
> + *
> + * Copyright (C) 2020 Sartura Ltd.
> + * Author: Luka Kovacic
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IEI_WT61P803_PUZZLE_MAX_COMMAND_LENGTH (20 + 2)
> +#define IEI_WT61P803_PUZZLE_RESP_BUF_SIZE512
> +
> +/* Use HZ as a timeout value throughout the driver */
> +#define IEI_WT61P803_PUZZLE_GENERAL_TIMEOUT HZ
> +
> +/**
> + * struct iei_wt61p803_puzzle_mcu_status - MCU flags state
> + *
> + * @ac_recovery_status_flag: AC Recovery Status Flag
> + * @power_loss_recovery: System recovery after power loss
> + * @power_status:System Power-on Method
> + */
> +struct iei_wt61p803_puzzle_mcu_status {
> + u8 ac_recovery_status_flag;
> + u8 power_loss_recovery;
> + u8 power_status;
> +};
> +
> +/**
> + * enum iei_wt61p803_puzzle_reply_state - State of the reply
> + * @FRAME_OK:The frame was completely processed/received
> + * @FRAME_PROCESSING:First bytes were received, but the frame isn't
> complete
> + * @FRAME_STRUCT_EMPTY: The frame struct is empty, no data was received
> + * @FRAME_TIMEOUT: The frame processing timed out, communication failed
> + *
> + * Describes the general state of the frame that is currently being received.
> + */
> +enum iei_wt61p803_puzzle_reply_state {
> + FRAME_OK = 0x00,
> + FRAME_PROCESSING = 0x01,
> + FRAME_STRUCT_EMPTY = 0xFF,
> + FRAME_TIMEOUT = 0xFE
> +};
> +
> +/**
> + * struct iei_wt61p803_puzzle_reply - MCU reply
> + *
> + * @size:Size of the MCU reply
> + * @data:Full MCU reply buffer
> + * @state: Current state of the packet
> + * @received:Was the response fullfilled
> + */
> +struct iei_wt61p803_puzzle_reply {
> + size_t size;
> + unsigned char *data;
> + u8 state;
> + struct completion received;
> +};
> +
> +/**
> + * struct iei_wt61p803_puzzle_mcu_version - MCU version status
> + *
> + * @version: Primary firmware version
> + * @build_info: Build date and time
> + * @bootloader_mode: Status of the MCU operation
> + * @protocol_version:MCU
Re: [PATCH v2 5/7] Documentation/ABI: Add iei-wt61p803-puzzle driver sysfs interface documentation
On Sat, 26 Sep 2020 15:55:12 +0200 Luka Kovacic wrote: > Add the iei-wt61p803-puzzle driver sysfs interface documentation to allow > monitoring and control of the microcontroller from user space. > > Signed-off-by: Luka Kovacic > Cc: Luka Perkov > Cc: Robert Marko > --- > .../stable/sysfs-driver-iei-wt61p803-puzzle | 65 +++ > 1 file changed, 65 insertions(+) > create mode 100644 Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > > diff --git a/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > b/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle > new file mode 100644 > index ..36fca70d66ef > --- /dev/null > +++ b/Documentation/ABI/stable/sysfs-driver-iei-wt61p803-puzzle I think this should go to testing, not stable. It should go to stable only after it is stable for some time. > @@ -0,0 +1,65 @@ > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/mac_address_* > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the internal iEi WT61P803 PUZZLE MCU MAC address values. > + These are factory assigned and can be changed. > + > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/serial_number > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the internal iEi WT61P803 PUZZLE MCU serial number. > + This value is factory assigned and can be changed. > + Please use (RO) and (RW) prefixes before the Description, instead of writing "This value is read only", i.e.: Description: (RO) Internal ... serial number. JFI: Why can these values be changed? Shouldn't they be burned into OTP? Marek > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/version > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the internal iEi WT61P803 PUZZLE MCU version. > + This value is read only. > + > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/protocol_version > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the internal iEi WT61P803 PUZZLE MCU protocol version. > + This value is read only. > + > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_loss_recovery > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the iEi WT61P803 PUZZLE MCU power loss recovery value. > + This value is read write. > + Value mapping: 0 - Always-On, 1 - Always-Off, 2 - Always-AC, 3 > - Always-WA > + > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/bootloader_mode > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read whether the MCU is in bootloader mode. > + This value is read only. > + > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/power_status > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the iEi WT61P803 PUZZLE MCU power status. Power status > indicates > + the power on method. > + This value is read only. > + Value mapping (bitwise list): > + 0x80 - Null > + 0x40 - Firmware flag > + 0x20 - Power loss detection flag (powered off) > + 0x10 - Power loss detection flag (AC mode) > + 0x08 - Button power on > + 0x04 - WOL power on > + 0x02 - RTC alarm power on > + 0x01 - AC recover power on > + > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/build_info > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the iEi WT61P803 PUZZLE MCU firmware build date. > + This value is read only. > + Format: /mm/dd hh:mm > + > +What: > /sys/bus/serial/devices/.../iei_wt61p803_puzzle_core/ac_recovery_status > +Date:September 2020 > +Contact: Luka Kovacic > +Description: Read the iEi WT61P803 PUZZLE MCU AC recovery status. > + This value is read only.
Re: [PATCH v2 4/7] drivers: leds: Add the iEi WT61P803 PUZZLE LED driver
On Sat, 26 Sep 2020 15:55:11 +0200
Luka Kovacic wrote:
> Add support for the iEi WT61P803 PUZZLE LED driver.
> Currently only the front panel power LED is supported.
>
> This driver depends on the iEi WT61P803 PUZZLE MFD driver.
>
> Signed-off-by: Luka Kovacic
> Cc: Luka Perkov
> Cc: Robert Marko
> ---
> drivers/leds/Kconfig| 8 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-iei-wt61p803-puzzle.c | 174
> 3 files changed, 183 insertions(+)
> create mode 100644 drivers/leds/leds-iei-wt61p803-puzzle.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1c181df24eae..8a25fb753dec 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -332,6 +332,14 @@ config LEDS_IPAQ_MICRO
> Choose this option if you want to use the notification LED on
> Compaq/HP iPAQ h3100 and h3600.
>
> +config LEDS_IEI_WT61P803_PUZZLE
> + tristate "LED Support for the iEi WT61P803 PUZZLE MCU"
> + depends on LEDS_CLASS
> + depends on MFD_IEI_WT61P803_PUZZLE
> + help
> + This option enables support for LEDs controlled by the iEi WT61P803
> + M801 MCU.
> +
> config LEDS_HP6XX
> tristate "LED Support for the HP Jornada 6xx"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index c2c7d7ade0d0..cd362437fefd 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_HP6XX)+= leds-hp6xx.o
> obj-$(CONFIG_LEDS_INTEL_SS4200) += leds-ss4200.o
> obj-$(CONFIG_LEDS_IP30) += leds-ip30.o
> obj-$(CONFIG_LEDS_IPAQ_MICRO)+= leds-ipaq-micro.o
> +obj-$(CONFIG_LEDS_IEI_WT61P803_PUZZLE) += leds-iei-wt61p803-puzzle.o
> obj-$(CONFIG_LEDS_IS31FL319X)+= leds-is31fl319x.o
> obj-$(CONFIG_LEDS_IS31FL32XX)+= leds-is31fl32xx.o
> obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
> diff --git a/drivers/leds/leds-iei-wt61p803-puzzle.c
> b/drivers/leds/leds-iei-wt61p803-puzzle.c
> new file mode 100644
> index ..b9a977575a23
> --- /dev/null
> +++ b/drivers/leds/leds-iei-wt61p803-puzzle.c
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* iEi WT61P803 PUZZLE MCU LED Driver
> + *
> + * Copyright (C) 2020 Sartura Ltd.
> + * Author: Luka Kovacic
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +enum iei_wt61p803_puzzle_led_state {
> + IEI_LED_OFF = 0x30,
> + IEI_LED_ON = 0x31,
> + IEI_LED_BLINK_5HZ = 0x32,
> + IEI_LED_BLINK_1HZ = 0x33,
> +};
> +
> +/**
> + * struct iei_wt61p803_puzzle_led - MCU LED Driver
> + *
> + * @mcu: MCU struct pointer
> + * @response_buffer Global MCU response buffer allocation
> + * @lock:General mutex lock for LED operations
> + * @led_power_state: State of the front panel power LED
> + */
> +struct iei_wt61p803_puzzle_led {
> + struct iei_wt61p803_puzzle *mcu;
> + unsigned char *response_buffer;
> + struct mutex lock;
> + int led_power_state;
> +};
> +
> +static inline struct iei_wt61p803_puzzle_led *cdev_to_iei_wt61p803_puzzle_led
> + (struct led_classdev *led_cdev)
> +{
> + return dev_get_drvdata(led_cdev->dev->parent);
> +}
> +
> +static int iei_wt61p803_puzzle_led_brightness_set_blocking(struct
> led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct iei_wt61p803_puzzle_led *mcu_led =
> + cdev_to_iei_wt61p803_puzzle_led(cdev);
> + unsigned char led_power_cmd[5] = {
> + IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> + IEI_WT61P803_PUZZLE_CMD_LED,
> + IEI_WT61P803_PUZZLE_CMD_LED_POWER,
> + (char)IEI_LED_OFF
> + };
> + unsigned char *resp_buf = mcu_led->response_buffer;
> + size_t reply_size;
> +
> + mutex_lock(_led->lock);
> + if (brightness == LED_OFF) {
> + led_power_cmd[3] = (char)IEI_LED_OFF;
> + mcu_led->led_power_state = LED_OFF;
> + } else {
> + led_power_cmd[3] = (char)IEI_LED_ON;
> + mcu_led->led_power_state = LED_ON;
> + }
> + mutex_unlock(_led->lock);
> +
> + return iei_wt61p803_puzzle_write_command(mcu_led->mcu, led_power_cmd,
> + sizeof(led_power_cmd), resp_buf, _size);
> +}
> +
> +static enum led_brightness
> +iei_wt61p803_puzzle_led_brightness_get(struct led_classdev *cdev)
> +{
> + struct iei_wt61p803_puzzle_led *mcu_led =
> + cdev_to_iei_wt61p803_puzzle_led(cdev);
> + int led_state;
> +
> + mutex_lock(_led->lock);
> + led_state = mcu_led->led_power_state;
> + mutex_unlock(_led->lock);
> +
> + return led_state;
> +}
> +
> +static int iei_wt61p803_puzzle_led_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct iei_wt61p803_puzzle *mcu =
Re: [PATCH v2 1/7] dt-bindings: Add iEi vendor prefix and iEi WT61P803 PUZZLE driver bindings
On Sat, 26 Sep 2020 15:55:08 +0200 Luka Kovacic wrote: > diff --git > a/Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml > b/Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml > new file mode 100644 > index ..502d97630ecc > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/iei,wt61p803-puzzle-leds.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/iei,wt61p803-puzzle-leds.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: iEi WT61P803 PUZZLE MCU LED module from IEI Integration Corp. > + > +maintainers: > + - Luka Kovacic > + > +description: | > + This module is a part of the iEi WT61P803 PUZZLE MFD device. For more > details > + see Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml. > + > + The LED module is a sub-node of the MCU node in the Device Tree. > + > +properties: > + compatible: > +const: iei,wt61p803-puzzle-leds > + > + "#address-cells": > +const: 1 > + > + "#size-cells": > +const: 0 > + > +patternProperties: > + "^led@0$": > +type: object Here should be a ref to LED common.yaml: $ref: common.yaml# > +description: | > + Properties for a single LED. > + > +properties: > + reg: > +description: > + Index of the LED. Only one LED is supported at the moment. > +minimum: 0 > +maximum: 0 > + > + label: true > + > + linux,default-trigger: true > + label is obsolete, linux,default-trigger as well. > +required: > + - compatible > + - "#address-cells" > + - "#size-cells" > diff --git a/Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml > b/Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml > new file mode 100644 > index ..38846c758372 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/iei,wt61p803-puzzle.yaml > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/iei,wt61p803-puzzle.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: iEi WT61P803 PUZZLE MCU from IEI Integration Corp. > + > +maintainers: > + - Luka Kovacic > + > +description: | > + iEi WT61P803 PUZZLE MCU is embedded in some iEi Puzzle series boards. > + It's used for controlling system power states, fans, LEDs and temperature > + sensors. > + > + For Device Tree bindings of other sub-modules (HWMON, LEDs) refer to the > + binding documents under the respective subsystem directories. > + > +properties: > + compatible: > +const: iei,wt61p803-puzzle > + > + current-speed: > +description: > + Serial bus speed in bps > +maxItems: 1 What does this mean? Is this connected via uart? Why not name it `baud`? I realize that `current-speed` is used in other device trees (and `baud` as well), but what "current" is this? I don't suppose this means electric current :) Have you passed these via dt_binding_check? Marek
Re: [PATCH v2 7/7] arm64: dts: marvell: Add a device tree for the iEi Puzzle-M801 board
On Sat, 26 Sep 2020 15:55:14 +0200
Luka Kovacic wrote:
> + leds {
> + compatible = "gpio-leds";
> + status = "okay";
> + pinctrl-0 = <_sfpplus_led_pins _sfpplus_led_pins>;
> + pinctrl-names = "default";
> +
> + led0 {
> + function = LED_FUNCTION_STATUS;
> + label = "p2_act";
> + gpios = <_gpio1 6 GPIO_ACTIVE_LOW>;
> + };
There should be a dash in LED node name, please pass this dts via
dt_binding_check
led-0 {
...
};
Also why not add the `color` property to the LED? This is DTS for a
specific device, right?
`label` is obsolete. The LED subsystem creates a name in form
[device:]color:function
If this LED should blink for activity on port 2 (is this an ethernet
port?), the function should be LED_FUNCTION_LAN and function-enumerator
should be <2> (or function should be LED_FUNCTION_ACTIVITY, depending
on how the LED subsystem goes forward with this, but certainly not
LED_FUNCTION_STATUS), and trigger-sources should be set to point to the
ethernet port.
Luka, are you willing to change this once we solve this API properly
in LED subsystem?
> + led6 {
> + function = LED_FUNCTION_STATUS;
> + linux,default-trigger = "disk-activity";
> + label = "front-hdd-led";
> + gpios = <_gpio2 22 GPIO_ACTIVE_HIGH>;
> + };
led-6. LED_FUNCTION_DISK. `label` deprecated.
> + leds {
> + compatible = "iei,wt61p803-puzzle-leds";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
> + reg = <0>;
> + color = ;
> + label = "front-power-led";
> + };
Again, `label` is deprecated. Rather use function =
;
Marek
Re: [PATCH] leds: omnia: fix leak of device node iterator
Already fixed in Pavel's for-next https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next=62aa40d0e907849d740ceba2a7f6bcc88896699f
Re: [PATCH] leds: tlc591xx: fix leak of device node iterator
On Sat, 26 Sep 2020 01:10:11 +0200
Tobias Jordan wrote:
> In one of the error paths of the for_each_child_of_node loop in
> tlc591xx_probe, add missing call to of_node_put.
>
> Fixes: 1ab4531ad132 ("leds: tlc591xx: simplify driver by using the
> managed led API")
>
> Signed-off-by: Tobias Jordan
> ---
> drivers/leds/leds-tlc591xx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> index 0929f1275814..a8cc49752cd5 100644
> --- a/drivers/leds/leds-tlc591xx.c
> +++ b/drivers/leds/leds-tlc591xx.c
> @@ -214,6 +214,7 @@ tlc591xx_probe(struct i2c_client *client,
> err = devm_led_classdev_register_ext(dev, >ldev,
>_data);
> if (err < 0) {
> + of_node_put(child);
> if (err != -EPROBE_DEFER)
> dev_err(dev, "couldn't register LED %s\n",
> led->ldev.name);
This won't apply on pavel's for-next tree, there is no check for
EPROBE_DEFER, see
https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/tree/drivers/leds/leds-tlc591xx.c?h=for-next
Re: [PATCH] leds: lp55xx: fix device node iterator memory leaks
On Sat, 26 Sep 2020 00:59:05 +0200
Tobias Jordan wrote:
> Fix error paths in for_each_child_of_node loops that were missing an
> of_node_put call.
>
> Fixes: 92a81562e695 ("leds: lp55xx: Add multicolor framework support to
> lp55xx")
> Signed-off-by: Tobias Jordan
> ---
> drivers/leds/leds-lp55xx-common.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/leds-lp55xx-common.c
> b/drivers/leds/leds-lp55xx-common.c
> index 56210f4ad919..f8b55cfd21ef 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -614,8 +614,10 @@ static int lp55xx_parse_multi_led(struct device_node *np,
> for_each_child_of_node(np, child) {
> ret = lp55xx_parse_multi_led_child(child, cfg, child_number,
> num_colors);
> - if (ret)
> + if (ret) {
> + of_node_put(child);
> return ret;
> + }
> num_colors++;
> }
>
> @@ -681,8 +683,10 @@ struct lp55xx_platform_data
> *lp55xx_of_populate_pdata(struct device *dev,
>
> for_each_child_of_node(np, child) {
> ret = lp55xx_parse_logical_led(child, cfg, i);
> - if (ret)
> + if (ret) {
> + of_node_put(child);
> return ERR_PTR(-EINVAL);
> + }
> i++;
> }
>
This is already patched
Re: [PATCH 1/2] leds: lm3552: Fix warnings for undefined parameters
Wrong subject, it says lm3552 but driver is called lm3532 Besides this: Reviewed-by: Marek Behún
Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
On Mon, 21 Sep 2020 16:03:22 +0200
Simon Guinot wrote:
> On Mon, Sep 21, 2020 at 03:02:08PM +0200, Marek Behun wrote:
> > On Mon, 21 Sep 2020 14:53:43 +0200
> > Simon Guinot wrote:
> >
> > > On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> > > > On Fri, 18 Sep 2020 15:02:06 +0200
> > > > Simon Guinot wrote:
> > > >
> > > > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > > > >
> > > > > Hi Marek,
> > > > >
> > > > > > By using struct led_init_data when registering we do not need to
> > > > > > parse
> > > > > > `label` DT property nor `linux,default-trigger` property.
> > > > > >
> > > > > > Also, move forward from platform data to device tree only:
> > > > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > > > platform data structure is absorbed into the driver, because nothing
> > > > > > else in the source tree used it. Since nobody complained and all
> > > > > > usage
> > > > >
> > > > > Well, I probably should have...
> > > > >
> > > > > I am using this driver on the Seagate Superbee NAS devices. This
> > > > > devices
> > > > > are based on a x86 SoC. Since I have been unable to get from the ODM
> > > > > the
> > > > > LED information written in the ACPI tables, then platform data are
> > > > > used
> > > > > to pass the LED description to the driver.
> > > > >
> > > > > The support of this boards is not available mainline yet but it is
> > > > > still
> > > > > on my todo list. So that's why I am complaining right now :) If it is
> > > > > not too much trouble I'd like to keep platform data support in this
> > > > > driver.
> > > > >
> > > > > Thanks in advance.
> > > > >
> > > > > Simon
> > > > >
> > > >
> > > > Simon, what if we refactored the driver to use fwnode API instead of OF
> > > > API? Then if it is impossible for you to write DTS for that device,
> > > > instead of platform data you could implement your device via swnode
> > > > fwnodes. :)
> > >
> > > Yes. That would be perfect.
> > >
> > > Simon
> >
> > BTW if you have access to device schematics I could try to write DTS,
> > with schematics and the current board source file it should not be that
> > hard. But I can't test it, since I don't have the board.
>
> Don't worry, I'll do the writing and the testing of the fwnode in the
> x86 board files. This boards are not mainlined yet. So it is my problem.
>
> And actually if you don't have the time I can do the writing of the
> fwnode support in the driver as well. And you can just let the driver
> with the OF support. That's fine.
>
> But if you are willing to add fwnode support to the driver yourself,
> then you are more than welcome to do it. On my side, I can help with
> the testing. I can check that the ARM boards ant their DTB are still
> supported by the driver. And I can also check the support of the x86
> boards with the addition of the fwnode properties.
>
> Simon
Very well, I shall convert the driver to fwnode API and send the patch.
Marek
Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
On Mon, 21 Sep 2020 14:53:43 +0200
Simon Guinot wrote:
> On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> > On Fri, 18 Sep 2020 15:02:06 +0200
> > Simon Guinot wrote:
> >
> > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > >
> > > Hi Marek,
> > >
> > > > By using struct led_init_data when registering we do not need to parse
> > > > `label` DT property nor `linux,default-trigger` property.
> > > >
> > > > Also, move forward from platform data to device tree only:
> > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > platform data structure is absorbed into the driver, because nothing
> > > > else in the source tree used it. Since nobody complained and all usage
> > > >
> > >
> > > Well, I probably should have...
> > >
> > > I am using this driver on the Seagate Superbee NAS devices. This devices
> > > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > > LED information written in the ACPI tables, then platform data are used
> > > to pass the LED description to the driver.
> > >
> > > The support of this boards is not available mainline yet but it is still
> > > on my todo list. So that's why I am complaining right now :) If it is
> > > not too much trouble I'd like to keep platform data support in this
> > > driver.
> > >
> > > Thanks in advance.
> > >
> > > Simon
> > >
> >
> > Simon, what if we refactored the driver to use fwnode API instead of OF
> > API? Then if it is impossible for you to write DTS for that device,
> > instead of platform data you could implement your device via swnode
> > fwnodes. :)
>
> Yes. That would be perfect.
>
> Simon
BTW if you have access to device schematics I could try to write DTS,
with schematics and the current board source file it should not be that
hard. But I can't test it, since I don't have the board.
Marek
Re: [PATCH leds] leds: regulator: remove driver
On Sun, 20 Sep 2020 23:46:47 +0200 Pavel Machek wrote: > On Sun 2020-09-20 22:42:03, Marek Behún wrote: > > The leds-regulator driver only supports the old platform data binding > > and no in-tree code uses it. It also seems that no OpenWRT board uses > > it. > > > > Remove this driver. > > Lets keep this one. Very well. > Connecting LED directly to regulator simply makes sense. It does makes sence to me as well, but at least it needs to be rewritten to use OF instead of platdata. The way it is written now it is not used by anyone, apparently. Marek
Re: ledtrig-cpu: Limit to 4 CPUs
On Sun, 20 Sep 2020 18:55:28 +0200
Jacek Anaszewski wrote:
> On 9/20/20 5:39 PM, Marek Behun wrote:
> > On Sun, 20 Sep 2020 16:15:09 +0200
> > Jacek Anaszewski wrote:
> >
> >> Hi Pavel,
> >>
> >> On 9/19/20 11:38 AM, Pavel Machek wrote:
> >>> commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
> >>> Author: Pavel Machek
> >>> Date: Sat Sep 19 11:34:58 2020 +0200
> >>>
> >>> ledtrig-cpu: Limit to 4 CPUs
> >>>
> >>> Some machines have thousands of CPUs... and trigger mechanisms was
> >>> not
> >>> really meant for thousands of triggers. I doubt anyone uses this
> >>> trigger on many-CPU machine; but if they do, they'll need to do it
> >>> properly.
> >>>
> >>> Signed-off-by: Pavel Machek
> >>>
> >>> diff --git a/drivers/leds/trigger/ledtrig-cpu.c
> >>> b/drivers/leds/trigger/ledtrig-cpu.c
> >>> index 869976d1b734..b7e00b09b137 100644
> >>> --- a/drivers/leds/trigger/ledtrig-cpu.c
> >>> +++ b/drivers/leds/trigger/ledtrig-cpu.c
> >>> @@ -2,14 +2,18 @@
> >>>/*
> >>> * ledtrig-cpu.c - LED trigger based on CPU activity
> >>> *
> >>> - * This LED trigger will be registered for each possible CPU and named as
> >>> - * cpu0, cpu1, cpu2, cpu3, etc.
> >>> + * This LED trigger will be registered for first four CPUs and named
> >>> + * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
> >>> + * is on when any CPU is active.
> >>> + *
> >>> + * If you want support for arbitrary number of CPUs, make it one trigger,
> >>> + * with additional sysfs file selecting which CPU to watch.
> >>> *
> >>> * It can be bound to any LED just like other triggers using either a
> >>> * board file or via sysfs interface.
> >>> *
> >>> * An API named ledtrig_cpu is exported for any user, who want to add
> >>> CPU
> >>> - * activity indication in their code
> >>> + * activity indication in their code.
> >>> *
> >>> * Copyright 2011 Linus Walleij
> >>> * Copyright 2011 - 2012 Bryan Wu
> >>> @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
> >>> for_each_possible_cpu(cpu) {
> >>> struct led_trigger_cpu *trig = _cpu(cpu_trig, cpu);
> >>>
> >>> + if (cpu > 4)
> >>
> >> NACK. The workaround for this trigger was implemented for a reason -
> >> to make it working on platforms with arbitrary number of logical cpus.
> >> I've got 8, so I am discriminated now. Not saying, that it precludes
> >> trigger registration with no single line of warning.
> >> Regardless of that - you have no guarantee that you're not breaking
> >> anyone - "I doubt" is not a sufficient argument.
> >>
> >
> > If that is the case Jacek, I would try 16 and then see if people
> > complain. Do you really think that someone sets a specific LED to
> > trigger on activity on CPU id > 16?
>
> I have an access to the machine with 80 cpus, so I could once
> get surprised not being able to find cpuN triggers not being
> listed among available triggers.
>
> And say that I have a solution where I install 80 userspace LEDs
> (drivers/leds/uleds.c) and register them on each cpuN triggers to get
> notifications on how cpus work.
Hi Jacek,
I understand (and Pavel does for sure too) that many people
currently have that possibility, that they have access to machines with
many CPUs and many LEDs. We also understand that currently it is
possible for users to set 1847th LED to trigger on activity on CPU ID
1337. What we are suggesting is that practically no one uses this, and
for those 10 people who do, well it would be better for them to migrate
to new ABI than for kernel developers having forever maintain this
legacy ABI.
Legacy drivers get removed from kernel from time to time, if no one
uses them. So I think Pavel's proposal (although I may not agree with
the limit 4) has some merit. If we try this, and someone complains, we
can then discuss. If we don't try, we may never know.
Marek
> > If you do not agree, then I think we should implement a "cpu" trigger
> > where the cpu ID (or maybe mask of multiple CPUs) is configurable via
> > another sysfs file. And then declare current cpu trigger (with names
> > "cpu%d") as legacy.
>
> Yes, we can do that, and even mark the cpu trigger as legacy but we
> cannot prevent people from using it if that was present in kernel
> for many years.
>
Re: ledtrig-cpu: Limit to 4 CPUs
On Sun, 20 Sep 2020 16:15:09 +0200
Jacek Anaszewski wrote:
> Hi Pavel,
>
> On 9/19/20 11:38 AM, Pavel Machek wrote:
> > commit 318681d3e019e39354cc6c2155a7fd1bb8e8084d
> > Author: Pavel Machek
> > Date: Sat Sep 19 11:34:58 2020 +0200
> >
> > ledtrig-cpu: Limit to 4 CPUs
> >
> > Some machines have thousands of CPUs... and trigger mechanisms was not
> > really meant for thousands of triggers. I doubt anyone uses this
> > trigger on many-CPU machine; but if they do, they'll need to do it
> > properly.
> >
> > Signed-off-by: Pavel Machek
> >
> > diff --git a/drivers/leds/trigger/ledtrig-cpu.c
> > b/drivers/leds/trigger/ledtrig-cpu.c
> > index 869976d1b734..b7e00b09b137 100644
> > --- a/drivers/leds/trigger/ledtrig-cpu.c
> > +++ b/drivers/leds/trigger/ledtrig-cpu.c
> > @@ -2,14 +2,18 @@
> > /*
> >* ledtrig-cpu.c - LED trigger based on CPU activity
> >*
> > - * This LED trigger will be registered for each possible CPU and named as
> > - * cpu0, cpu1, cpu2, cpu3, etc.
> > + * This LED trigger will be registered for first four CPUs and named
> > + * as cpu0, cpu1, cpu2, cpu3. There's additional trigger called cpu that
> > + * is on when any CPU is active.
> > + *
> > + * If you want support for arbitrary number of CPUs, make it one trigger,
> > + * with additional sysfs file selecting which CPU to watch.
> >*
> >* It can be bound to any LED just like other triggers using either a
> >* board file or via sysfs interface.
> >*
> >* An API named ledtrig_cpu is exported for any user, who want to add CPU
> > - * activity indication in their code
> > + * activity indication in their code.
> >*
> >* Copyright 2011 Linus Walleij
> >* Copyright 2011 - 2012 Bryan Wu
> > @@ -145,6 +149,9 @@ static int __init ledtrig_cpu_init(void)
> > for_each_possible_cpu(cpu) {
> > struct led_trigger_cpu *trig = _cpu(cpu_trig, cpu);
> >
> > + if (cpu > 4)
>
> NACK. The workaround for this trigger was implemented for a reason -
> to make it working on platforms with arbitrary number of logical cpus.
> I've got 8, so I am discriminated now. Not saying, that it precludes
> trigger registration with no single line of warning.
> Regardless of that - you have no guarantee that you're not breaking
> anyone - "I doubt" is not a sufficient argument.
>
If that is the case Jacek, I would try 16 and then see if people
complain. Do you really think that someone sets a specific LED to
trigger on activity on CPU id > 16?
If you do not agree, then I think we should implement a "cpu" trigger
where the cpu ID (or maybe mask of multiple CPUs) is configurable via
another sysfs file. And then declare current cpu trigger (with names
"cpu%d") as legacy.
Marek
Re: [PATCH v5 1/3] leds: pwm: Remove platform_data support
Besides Pavel's note about the __attribute__((nonnull)) position Reviewed-by: Marek Behún
Re: [PATCH leds v2 03/50] leds: fsg: compile if COMPILE_TEST=y
On Sat, 19 Sep 2020 11:56:16 +0200 Pavel Machek wrote: > #include It can't include this header on other platforms...
Re: [PATCH leds v2 15/50] leds: lm3697: cosmetic change: use helper variable, reverse christmas tree
On Fri, 18 Sep 2020 06:47:20 -0500 Dan Murphy wrote: > > Reviewed-by: Dan Murphy > > Dan, could you also review patch 14/50? That one is also lm3697 and this one depends on it. Marek
Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
On Fri, 18 Sep 2020 15:02:06 +0200
Simon Guinot wrote:
> On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
>
> Hi Marek,
>
> > By using struct led_init_data when registering we do not need to parse
> > `label` DT property nor `linux,default-trigger` property.
> >
> > Also, move forward from platform data to device tree only:
> > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > platform data structure is absorbed into the driver, because nothing
> > else in the source tree used it. Since nobody complained and all usage
>
> Well, I probably should have...
>
> I am using this driver on the Seagate Superbee NAS devices. This devices
> are based on a x86 SoC. Since I have been unable to get from the ODM the
> LED information written in the ACPI tables, then platform data are used
> to pass the LED description to the driver.
>
> The support of this boards is not available mainline yet but it is still
> on my todo list. So that's why I am complaining right now :) If it is
> not too much trouble I'd like to keep platform data support in this
> driver.
>
> Thanks in advance.
>
> Simon
>
Simon, what if we refactored the driver to use fwnode API instead of OF
API? Then if it is impossible for you to write DTS for that device,
instead of platform data you could implement your device via swnode
fwnodes. :)
static const struct property_entry entries[] = {
PROPERTY_ENTRY_STRING("compatible", "lacie,ns2-leds"),
...
};
Look at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/chromeos_laptop.c?h=v5.9-rc5
search for PROPERTY_ENTRY.
I am willing to work on this with you, but I would really like to rid
the LED drivers of platform data.
Marek
Re: [PATCH leds v2 05/50] leds: various: guard of_match_table member value with of_match_ptr
On Fri, 18 Sep 2020 12:57:59 +0300 Sakari Ailus wrote: > On Fri, Sep 18, 2020 at 11:20:58AM +0200, Marek Behun wrote: > > On Fri, 18 Sep 2020 09:15:00 +0300 > > Sakari Ailus wrote: > > > > > Hi Marek, > > > > > > On Fri, Sep 18, 2020 at 12:32:53AM +0200, Marek Behún wrote: > > > > Change > > > > .of_match_table = xxx, > > > > to > > > > .of_match_table = of_match_ptr(xxx), > > > > in various drivers. > > > > > > > > This should be standard even for drivers that depend on OF. > > > > > > After this patch, none of these drivers will work on ACPI systems > > > anymore. > > ^ > > If CONFIG_OF is disabled, that is. > > > > > Hi Sakari, > > > > I don't understand. Why not? Does ACPI subsystem parse of_match_table > > as well? > > It does. The compatible string is used the same way as in DT for matching > devices with "PRP0001" _HID or _CID. > > Please read Documentation/firmware-guide/acpi/enumeration.rst . > > IOW, you can safely do the above only for drivers that depend on OF in > Kconfig. Otherwise you'll probably break something. > Sakari, thank you for the pointer to the docs. I thought that of_match_table is used only by OF (hence the name). Marek
Re: [PATCH leds v2 05/50] leds: various: guard of_match_table member value with of_match_ptr
On Fri, 18 Sep 2020 09:15:00 +0300 Sakari Ailus wrote: > Hi Marek, > > On Fri, Sep 18, 2020 at 12:32:53AM +0200, Marek Behún wrote: > > Change > > .of_match_table = xxx, > > to > > .of_match_table = of_match_ptr(xxx), > > in various drivers. > > > > This should be standard even for drivers that depend on OF. > > After this patch, none of these drivers will work on ACPI systems anymore. Hi Sakari, I don't understand. Why not? Does ACPI subsystem parse of_match_table as well?
Re: [PATCH leds v1 09/10] leds: lm36274: use struct led_init_data when registering
On Thu, 17 Sep 2020 10:28:12 -0500 Dan Murphy wrote: > Marek > > On 9/16/20 6:16 PM, Marek Behún wrote: > > By using struct led_init_data when registering we do not need to parse > > `label` DT property nor `linux,default-trigger` property. > > > > A small refactor was also done: > > - with using devm_led_classdev_register_ext the driver remove method is > >not needed > > - since only one child node is allowed for this driver, use > >device_get_next_child_node instead of device_for_each_child_node > > > > Previously if the `label` DT property was not present, the code composed > > name for the LED in the form > >"parent_name::" > > For backwards compatibility we therefore set > >init_data->default_label = ":"; > > so that the LED will not get a different name if `label` property is not > > present. > > You are going to re-factor this as well a lot of changes in a single > patch is hard to review > > Dan > I am trying to do this now.
Re: [PATCH leds v1 07/10] leds: is31fl32xx: use struct led_init_data when registering
This can be done without refactoring, please ignore this patch in this spin.
Re: [PATCH leds v1 06/10] leds: pm8058: use struct led_init_data when registering
On Wed, 16 Sep 2020 19:46:25 -0500 Bjorn Andersson wrote: > The pdev->dev -> dev and of_node changes are reasonable, but shouldn't > be part of this patch. It simply makes it hard to reason about he actual > change. > > Please respin this with only the introduction of led_init_data. > > Thanks, > Bjorn > Am working on this :)
Re: [PATCH leds v1 03/10] leds: lm3697: use struct led_init_data when registering
On Thu, 17 Sep 2020 06:39:56 -0500 Dan Murphy wrote: > Marek > > On 9/16/20 6:16 PM, Marek Behún wrote: > > By using struct led_init_data when registering we do not need to parse > > `label` DT property nor `linux,default-trigger` property. > > I would prefer 2 separate patches. One implementing the init_data and > the other removing the default trigger > > Dan > > Am working on it :)
Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree
On Tue, 15 Sep 2020 23:35:25 +0200
Jacek Anaszewski wrote:
> Hi Marek,
>
> On 9/15/20 5:26 PM, Marek Behún wrote:
> > Allow setting netdev LED trigger as default when given LED DT node has
> > the `trigger-sources` property pointing to a node corresponding to a
> > network device.
> >
> > The specific netdev trigger mode is determined from the `function` LED
> > property.
> >
> > Example:
> >eth0: ethernet@3 {
> > compatible = "xyz";
> > #trigger-source-cells = <0>;
> >};
> >
> >led {
> > color = ;
> > function = LED_FUNCTION_LINK;
> > trigger-sources = <>;
> >};
> >
> > Signed-off-by: Marek Behún
> > Cc: Rob Herring
> > Cc: devicet...@vger.kernel.org
> > ---
> > drivers/leds/trigger/ledtrig-netdev.c | 80 ++-
> > include/dt-bindings/leds/common.h | 1 +
> > 2 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> > b/drivers/leds/trigger/ledtrig-netdev.c
> > index d5e774d830215..99fc2f0c68e12 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -20,6 +20,7 @@
> [...]
>
> > static int netdev_trig_activate(struct led_classdev *led_cdev)
> > {
> > struct led_netdev_data *trigger_data;
> > @@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev
> > *led_cdev)
> > trigger_data->last_activity = 0;
> >
> > led_set_trigger_data(led_cdev, trigger_data);
> > + netdev_trig_of_parse(led_cdev, trigger_data);
>
> Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
> sense to use it here so as not to unnecessarily call
> netdev_trig_of_parse(), which makes sense only if trigger will be
> default, I presume.
>
> See timer_trig_activate() in drivers/leds/trigger/ledtrig-timer.c
> for reference.
>
Hmmm. Jacek, all the triggers that work with the macro
LED_INIT_DEFAULT_TRIGGER are oneshot, timer and pattern.
If this macro is set, they all call pattern_init function where they
read led-pattern from fwnode.
But there is no device tree in Linux sources using this property.
In fact the command
git grep led-pattern
yields only 2 files:
Documentation/devicetree/bindings/leds/common.yaml
drivers/leds/led-core.c
What is the purpose if no device tree uses this property? Is this used
from other fwnode sources, like acpi or efi?
The reason why I am asking this is that the `led-pattern` property in
device tree goes against the principle of device tree, that it
shouldn't set settings settable from userspace, only describe the
devices on the system and how they are connected to each other.
This is the same reason why multi-CPU DSA proposals which proposed to
set mappings between CPU ports and user ports were rejected...
Marek
Re: [PATCH leds + devicetree v2 1/2] leds: trigger: add trigger sources validating method and helper functions
On Tue, 15 Sep 2020 17:26:15 +0200 Marek Behún wrote: > + /* > + * Check whether LED has defined valid source for this trigger. > + * If yes, this trigger should be set as default trigger for LED. > + * This should use of_led_count_trigger_sources and > + * of_led_get_trigger_source functions. > + */ > + bool(*has_valid_source)(struct led_classdev *led_cdev); Hmm, the heartbeat and timer triggers do not need trigger sources. If we want to deprecate linux,default-trigger device tree property for these triggers also, this method should look to `function` DT property for heartbeat and timer triggers. In that case the name should be changed to something like of_is_valid_default_for, or something like that. Marek
Yet another ethernet PHY LED control proposal
Hello, I have been thinking about another way to implement ABI for HW control of ethernet PHY connected LEDs. This proposal is inspired by the fact that for some time there is a movement in the kernel to do transparent HW offloading of things (DSA is an example of that). So currently we have the `netdev` trigger. When this is enabled for a LED, new files will appear in that LED's sysfs directory: - `device_name` where user is supposed to write interface name - `link` if set to 1, the LED will be ON if the interface is linked - `rx` if set to 1, the LED will blink on receive event - `tx` if set to 1, the LED will blink on transmit event - `interval` specifies duration of the LED blink Now what is interesting is that almost all combinations of link/rx/tx settings are offloadable to a Marvell PHY! (Not to all LEDs, though...) So what if we abandoned the idea of a `hw` trigger, and instead just allowed a LED trigger to be offloadable, if that specific LED supports it? For the HW mode for different speed we can just expand the `link` sysfs file ABI, so that if user writes a specific speed to this file, instead of just "1", the LED will be on if the interface is linked on that specific speed. Or maybe another sysfs file could be used for "light on N mbps" setting... Afterwards we can figure out other possible modes. What do you think? Marek
Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs
On Thu, 10 Sep 2020 22:23:45 +0200 Pavel Machek wrote: > Hi! > > > Okay, so the netdev trigger offers modes `link`, `rx`, `tx`. > > You can enable/disable either of these (via separate sysfs files). `rx` > > and `tx` blink the LED, `link` turns the LED on if the interface is > > linked. > > I wonder if people really need separate rx and tx, but... this sounds > reasonable. > > > The phy_led_trigger subsystem works differently. Instead of registering > > one trigger (like netdev) it registers one trigger per PHY device and > > per speed. So for a PHY with name XYZ and supported speeds 1Gbps, > > 100Mbps, 10Mbps it registers 3 triggers: > > XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps > > That is not reasonable. > > > I propose that at least these HW modes should be available (and > > documented) for ethernet PHY controlled LEDs: > > Ok, and which of these will you actually use? > > > mode to determine link on: > > - `link` > > mode for activity (these should blink): > > - `activity` (both rx and tx), `rx`, `tx` > > mode for link (on) and activity (blink) > > - `link/activity`, maybe `link/rx` and `link/tx` > > mode for every supported speed: > > - `1Gbps`, `100Mbps`, `10Mbps`, ... > > mode for every supported cable type: > > - `copper`, `fiber`, ... (are there others?) > > That's ... way too many options. > > Can we do it like netdev trigger? link? yes/no. rx? yes/no. tx? yes/no. > > If displaying link only for certain speeds is useful, have link_min > and link_max, specifying values in Mbps? Default would be link_min == > 0, and link_max = 25000, so it would react on any link speed. > > Is mode for cable type really useful? Then we should have link_fiber? > yes/no. link_copper? yes/no. > I want to put the speed differentiating mode by default on MOX on one LED, and activity on other LED. I think there are devices which have written on the case next to the LED that this LED is on for this specific speed, that LED is on for other specific speed. So modes for speed are reasonable, I think. In my opinion the disjunctive modes the Marvell PHYs support are useless (like ON when 1000Mbps or 10Mbps). You can't have link_min and link_max setting. The hardware does not support it this way. You can tell the hardware to light the LED when linked on a specific speed, and this is actually used on some devices (as I have written above, some devices have this written on the case). In my opinion the set `link`, `link/activity`, `activity`, `speed`, and one mode for each supported speed on the PHY is reasonable. This could be also compatible with software triggering via the proposed phydev trigger. > > mode that allows the user to determine link speed > > - `speed` (or maybe `linkspeed` ?) > > - on some Marvell PHYs the speed can be determined by how fast > > the LED is blinking (ie. 1Gbps blinks with default blinking > > frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps > > of half blinking frequency of 100Mbps) > > - on other Marvell PHYs this is instead: > > 1Gpbs blinks 3 times, pause, 3 times, pause, ... > > 100Mpbs blinks 2 times, pause, 2 times, pause, ... > > 10Mpbs blinks 1 time, pause, 1 time, pause, ... > > - we don't need to differentiate these modes with different names, > > because the important thing is just that this mode allows the > > user to determine the speed from how the LED blinks > > I'd be very careful. Userspace should know what they are asking > for. I'd propose simply ignoring this feature. As I wrote above, I think this mode is rather useful when you have just two LEDs for a port. You can tell speed by looking on one LED and activity by looking at the other LED. And I want to set this as default on Turris MOX. > > mode to just force blinking - `blink` > > We already have different support for blinking in LED subsystem. Lets use > that. > > Best regards, > Pavel
Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs
On Thu, 10 Sep 2020 19:34:35 +0100 Russell King - ARM Linux admin wrote: > On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote: > > Generally the driver will default to the hardware reset blink > > pattern. There are a few PHY drivers which change this at probe, but > > not many. The silicon defaults are pretty good. > > The "right" blink pattern can be a matter of how the hardware is > wired. For example, if you have bi-colour LEDs and the PHY supports > special bi-colour mixing modes. > Have you seen such, Russell? This could be achieved via the multicolor LED framework, but I don't have a device which uses such LEDs, so I did not write support for this in the Marvell PHY driver. (I guess I could test it though, since on my device LED0 and LED1 are used, and this to can be put into bi-colour LED mode.)
Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware
On Wed, 9 Sep 2020 23:40:09 +0200 Pavel Machek wrote: > > > > > > 80 columns :-) (and please fix that globally, at least at places where > > > it is easy, like comments). > > > > > > > Linux is at 100 columns now since commit bdc48fa11e46, commited by > > Linus. See > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > > There was actually an article about this on Phoronix, I think. > > It is not. Checkpatch no longer warns about it, but 80 columns is > still preffered, see Documentation/process/coding-style.rst . Plus, > you want me to take the patch, not Linus. Very well, I shall rewrap it to 80 columns :) Marek
Re: [PATCH net-next + leds v2 0/7] PLEASE REVIEW: Add support for LEDs on Marvell PHYs
On Wed, 9 Sep 2020 23:42:59 +0200 Andrew Lunn wrote: > On Wed, Sep 09, 2020 at 06:25:45PM +0200, Marek Behún wrote: > > Hello Andrew and Pavel, > > > > please review these patches adding support for HW controlled LEDs. > > The main difference from previous version is that the API is now generalized > > and lives in drivers/leds, so that part needs to be reviewed (and maybe even > > applied) by Pavel. > > > > As discussed previously between you two, I made it so that the devicename > > part of the LED is now in the form `ethernet-phy%i` when the LED is probed > > for an ethernet PHY. Userspace utility wanting to control LEDs for a > > specific > > network interface can access them via /sys/class/net/eth0/phydev/leds: > > > > mox ~ # ls /sys/class/net/eth0/phydev/leds > > It is nice they are directly in /sys/class/net/eth0/phydev. Do they > also appear in /sys/class/led? They are in /sys/class/net/eth0/phydev/leds by default, because they are children of the PHY device and are of `leds` class, and the PHY subsystem creates a symlink `phydev` when PHY is attached to the interface. They are in /sys/class/leds/ as symlinks, because AFAIK everything in /sys/class// is a symlink... > Have you played with network namespaces? What happens with > > ip netns add ns1 > > ip link set eth0 netns ns1 > >Andrew If you move eth0 to other network namespace, naturally the /sys/class/net/eth0 symlink will disappear, as will the directory it pointed to. The symlink phydev does will disappear from /sys/class/net/eth0/ directory after eth0 is moved to ns1, and is lost. It does not return even if eth0 is moved back to root namespace. The LED will of course stay in ns1 and also in root namespace, as will the phydev the LED is a child to. But they are no longer accessible via /sys/class/net/eth0, instead you can access the LEDs either via /sys/class/leds or /sys/class/mdio_bus///leds, or without symlinks via /sys/devices/ tree. Marek
Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs
On Wed, 9 Sep 2020 15:15:52 -0600
Rob Herring wrote:
> On Wed, Sep 09, 2020 at 06:25:46PM +0200, Marek Behún wrote:
> > Document binding for LEDs connected to and controlled by various chips
> > (such as ethernet PHY chips).
>
> If they are h/w controlled, then why are they in DT?
The idea is that by default these LEDs are in some specific HW control
mode (the chip default), but the chip supports various HW control
modes, and also supports SW control.
For example on Marvell PHYs there is a 4-bit register for each LED, so
16 values, and some of them are:
: On - Receive
Off - No receive
0001: On - Link
Blink - Activity
Off - No Link
...
0101: On - 100Mbps or Fiber Link
Off - Else
...
1000: Force Off
1001: Force On
...
1011: Force Blink
So writing 0x8 disables the LED, 0x9 enabled it (SW control via
/sys/class/leds//brightness), other values change the HW control
mode (in this proposal /sys/class/leds//hw_mode when trigger is
set to dev-hw-mode).
> >
> > Signed-off-by: Marek Behún
> > Cc: Rob Herring
> > Cc: devicet...@vger.kernel.org
> > ---
> > .../leds/linux,hw-controlled-leds.yaml| 99 +++
> > 1 file changed, 99 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > new file mode 100644
> > index 0..eaf6e5d80c5f5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/linux,hw-controlled-leds.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LEDs that can be controlled by hardware (eg. by an ethernet PHY
> > chip)
> > +
> > +maintainers:
> > + - Marek Behún
> > +
> > +description:
> > + Many an ethernet PHY (and other chips) supports various HW control modes
> > + for LEDs connected directly to them. With this binding such LEDs can be
> > + described.
> > +
> > +properties:
> > + compatible:
> > +const: linux,hw-controlled-leds
>
> What makes this linux specific?
>
> Unless you're going to make this h/w specific, then it probably should
> just be dropped.
>
Will do, thanks.
> The phy schema will need:
>
> leds:
> type: object
> $ref: /schemas/leds/hw-controlled-leds.yaml#
>
> > +
> > + "#address-cells":
> > +const: 1
> > +
> > + "#size-cells":
> > +const: 0
> > +
> > +patternProperties:
> > + "^led@[0-9a-f]+$":
> > +type: object
> > +allOf:
> > + - $ref: common.yaml#
> > +description:
> > + This node represents a LED device connected to a chip that can
> > control
> > + the LED in various HW controlled modes.
> > +
> > +properties:
> > + reg:
> > +maxItems: 1
> > +description:
> > + This property identifies the LED to the chip the LED is
> > connected to
> > + (eg. an ethernet PHY chip can have multiple LEDs connected to
> > it).
> > +
> > + enable-active-high:
> > +description:
> > + Polarity of LED is active high. If missing, assumed default is
> > active
> > + low.
> > +type: boolean
> > +
> > + led-tristate:
> > +description:
> > + LED pin is tristate type. If missing, assumed false.
> > +type: boolean
> > +
> > + linux,default-hw-mode:
>
> How is this linux specific? It sounds device specific. Your choice is
> make this a device specific property with device specific values or come
> up with generic modes.
I was inspired by `linux,default-trigger` and `linux,keycode`
properties...
> Perhaps 'function' should be expanded.
The thing is that `function` is now used for creating LED device name.
I was not aware that `linux,default-trigger` is deprecated
Perhaps this should be discussed with Pavel as well.
But of course from the perspective that DT should be independent from
Linux, you are right.
I fear this will be quite a pain to resolve...
> > +description:
> > + This parameter, if present, specifies the default HW triggering
> > mode
> > + of the LED when LED trigger is set to `dev-hw-mode`.
> > + Available values are specific per device the LED is connected to
> > and
> > + per LED itself.
> > +$ref: /schemas/types.yaml#definitions/string
> > +
> > +required:
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +
> > +#include
> > +
> > +ethernet-phy@0 {
> > +compatible = "ethernet-phy-ieee802.3-c45";
> > +reg = <0>;
> > +
> > +leds {
> > +compatible = "linux,hw-controlled-leds";
> > +#address-cells = <1>;
>
Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs
On Wed, 9 Sep 2020 15:31:22 -0600 Rob Herring wrote: > On Wed, Sep 09, 2020 at 11:07:26PM +0200, Marek Behun wrote: > > On Wed, 9 Sep 2020 14:59:23 -0600 > > Rob Herring wrote: > > > > > > > > > > I don't know :) I copied this from other drivers, I once tried setting > > > > up environment for doing checking of device trees with YAML schemas, > > > > and it was a little painful :) > > > > > > pip3 install dtschema ? > > > > > > Can you elaborate on the issue. > > > > > > Rob > > > > > > > I am using Gentoo and didn't want to bloat system with non-portage > > packages, nor try to start a virtual environment. In the end I did it > > in a chroot Ubuntu :) > > A user install doesn't work? > > I don't really care for virtual env either. > > > The other thing is that the make dt_binding_check executed for > > quite a long time, and I didn't find a way to just do the binding check > > some of the schemas. > > It's a bit faster now with what's queued for 5.10. The schema > validation is under 10sec now on my laptop. For the examples, any > new schema could apply to any example, so we have to check them all. > It's faster too, but still minutes to run. > > > But I am not criticizing anything, I think that it is a good thing to > > have this system. > > Good to hear. Just want to improve any pain points if possible. > > Rob > OK I am running it now in my chroot environment :)
Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware
On Wed, 9 Sep 2020 22:48:15 +0200
Pavel Machek wrote:
> Hi!
>
> > Many an ethernet PHY (and other chips) supports various HW control modes
> > for LEDs connected directly to them.
>
> I guess this should be
>
> "Many ethernet PHYs (and other chips) support various HW control modes
> for LEDs connected directly to them."
>
I guess it is older English, used mainly in poetry, but I read it in
works of contemporary fiction as well. As far as I could find, it is still
actually gramatically correct.
https://idioms.thefreedictionary.com/many+an
https://en.wiktionary.org/wiki/many_a
But I will change it if you insist on it.
> > This API registers a new private LED trigger called dev-hw-mode. When
> > this trigger is enabled for a LED, the various HW control modes which
> > are supported by the device for given LED can be get/set via hw_mode
> > sysfs file.
> >
> > Signed-off-by: Marek Behún
> > ---
> > .../sysfs-class-led-trigger-dev-hw-mode | 8 +
> > drivers/leds/Kconfig | 10 +
>
> I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd
> call the trigger just "hw"...
>
Will move in next version. Lets see what others think about the trigger
name.
> > +Contact: Marek Behún
> > + linux-l...@vger.kernel.org
> > +Description: (W) Set the HW control mode of this LED. The various
> > available HW control modes
> > + are specific per device to which the LED is connected to
> > and per LED itself.
> > + (R) Show the available HW control modes and the currently
> > selected one.
>
> 80 columns :-) (and please fix that globally, at least at places where
> it is easy, like comments).
>
Linux is at 100 columns now since commit bdc48fa11e46, commited by
Linus. See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
There was actually an article about this on Phoronix, I think.
> > + return 0;
> > +err_free:
> > + devm_kfree(dev, led);
> > + return ret;
> > +}
>
> No need for explicit free with devm_ infrastructure?
No, but if the registration failed, I don't see any reason why the
memory should be freed only when the PHY device is destroyed, if the
memory is not used... On failures other code in Linux frees even devm_
allocated resources.
> > + cur_mode = led->ops->led_get_hw_mode(dev->parent, led);
> > +
> > + for (mode = led->ops->led_iter_hw_mode(dev->parent, led, );
> > +mode;
> > +mode = led->ops->led_iter_hw_mode(dev->parent, led, )) {
> > + bool sel;
> > +
> > + sel = cur_mode && !strcmp(mode, cur_mode);
> > +
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ?
> > "[" : "", mode,
> > +sel ? "]" : "");
> > + }
> > +
> > + if (buf[len - 1] == ' ')
> > + buf[len - 1] = '\n';
>
> Can this ever be false? Are you accessing buf[-1] in such case?
>
It can be false if whole PAGE_SIZE is written. The code above
using scnprintf only writes the first PAGE_SIZE bytes.
You are right about the buf[-1] case though. len has to be nonzero.
Thanks.
> > +int of_register_hw_controlled_leds(struct device *dev, const char
> > *devicename,
> > + const struct hw_controlled_led_ops *ops);
> > +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum
> > led_brightness brightness);
> > +
>
> Could we do something like hw_controlled_led -> hw_led to keep
> verbosity down and line lengths reasonable? Or hwc_led?
>
I am not opposed, lets see what Andrew / others think.
> > +extern struct led_hw_trigger_type hw_control_led_trig_type;
> > +extern struct led_trigger hw_control_led_trig;
> > +
> > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
>
> CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW?
The second option looks more reasonable to me, if we move to
drivers/leds/trigger.
Marek
Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs
On Wed, 9 Sep 2020 14:59:23 -0600 Rob Herring wrote: > > > > I don't know :) I copied this from other drivers, I once tried setting > > up environment for doing checking of device trees with YAML schemas, > > and it was a little painful :) > > pip3 install dtschema ? > > Can you elaborate on the issue. > > Rob > I am using Gentoo and didn't want to bloat system with non-portage packages, nor try to start a virtual environment. In the end I did it in a chroot Ubuntu :) The other thing is that the make dt_binding_check executed for quite a long time, and I didn't find a way to just do the binding check some of the schemas. But I am not criticizing anything, I think that it is a good thing to have this system. Marek
Re: [PATCH net-next v1 1/3] dt-bindings: net: ethernet-phy: add description for PHY LEDs
Please ignore this series, still refactoring...
Re: [PATCH net-next v1 1/3] dt-bindings: net: ethernet-phy: add description for PHY LEDs
Please ignore this patch, still refactoring...
Re: [PATCH RFC leds + net-next v4 0/2] Add support for LEDs on Marvell PHYs
On Tue, 25 Aug 2020 10:13:59 +0200 Matthias Schiffer wrote: > On Tue, 2020-07-28 at 17:05 +0200, Marek Behún wrote: > > Hi, > > > > this is v4 of my RFC adding support for LEDs connected to Marvell > > PHYs. > > > > Please note that if you want to test this, you still need to first > > apply > > the patch adding the LED private triggers support from Pavel's tree. > > > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next=93690cdf3060c61dfce813121d0bfc055e7fa30d > > > > What I still don't like about this is that the LEDs created by the > > code > > don't properly support device names. LEDs should have name in format > > "device:color:function", for example "eth0:green:activity". > > > > The code currently looks for attached netdev for a given PHY, but > > at the time this happens there is no netdev attached, so the LEDs > > gets > > names without the device part (ie ":green:activity"). > > > > This can be addressed in next version by renaming the LED when a > > netdev > > is attached to the PHY, but first a API for LED device renaming needs > > to > > be proposed. I am going to try to do that. This would also solve the > > same problem when userspace renames an interface. > > > > And no, I don't want phydev name there. > > > Hello Marek, > > thanks for your patches - Andrew suggested me to have a look at them as > I'm currently trying to add LED trigger support to the TI DP83867 PHY. > > Is there already a plan to add support for polarity and similiar > settings, at least to the generic part of your changes? > Hello Matthias, sorry for answering with delay, I somehow overlooked your email. Yes, I plan to add some basic platform data properties (like polarity) in the generic part. > In the TI DP83867, there are 2 separate settings for each LED: > > - Trigger event > - Polarity or override (active-high/active-low/force-high/force-low - > the latter two would be used for led_brightness_set) > - (There is also a 3rd register that defines the blink frequency, but > as it allows only a single setting for all LEDs, I would ignore it for > now) I think that blink frequency should be in the generic part as well. > > At least the per-LED polarity setting would be essential to have for > this feature to be useful for our TQ-Systems mainboards with TI PHYs. > I will try to send new version next week (starting 7th September). Marek > > Kind regards, > Matthias > > > > > > > Changes since v3: > > - addressed some of Andrew's suggestions > > - phy_hw_led_mode.c renamed to phy_led.c > > - the DT reading code is now also generic, moved to phy_led.c and > > called > > from phy_probe > > - the function registering the phydev-hw-mode trigger is now called > > from > > phy_device.c function phy_init before registering genphy drivers > > - PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS > > > > Changes since v2: > > - to share code with other drivers which may want to also offer PHY > > HW > > control of LEDs some of the code was refactored and now resides in > > phy_hw_led_mode.c. This code is compiled in when config option > > LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW > > control > > of LEDs should depend on this option. > > - the "hw-control" trigger is renamed to "phydev-hw-mode" and is > > registered by the code in phy_hw_led_mode.c > > - the "hw_control" sysfs file is renamed to "hw_mode" > > - struct phy_driver is extended by three methods to support PHY HW > > LED > > control > > - I renamed the various HW control modes offeret by Marvell PHYs to > > conform to other Linux mode names, for example the > > "1000/100/10/else" > > mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was > > renamed > > to "rx" (this is the name of the mode in netdev trigger). > > > > Marek > > > > > > Marek Behún (2): > > net: phy: add API for LEDs controlled by PHY HW > > net: phy: marvell: add support for PHY LEDs via LED class > > > > drivers/net/phy/Kconfig | 4 + > > drivers/net/phy/Makefile | 1 + > > drivers/net/phy/marvell.c| 287 > > +++ > > drivers/net/phy/phy_device.c | 25 ++- > > drivers/net/phy/phy_led.c| 176 + > > include/linux/phy.h | 51 +++ > > 6 files changed, 537 insertions(+), 7 deletions(-) > > create mode 100644 drivers/net/phy/phy_led.c > > >
Re: [PATCH RFC leds + net-next v4 1/2] net: phy: add API for LEDs controlled by PHY HW
On Tue, 28 Jul 2020 18:18:00 +0200
Andrew Lunn wrote:
> > +static int of_phy_register_led(struct phy_device *phydev, struct
> > device_node *np)
> > +{
> > + struct led_init_data init_data = {};
> > + struct phy_device_led *led;
> > + u32 reg;
> > + int ret;
> > +
> > + ret = of_property_read_u32(np, "reg", );
> > + if (ret < 0)
> > + return ret;
> > +
> > + led = devm_kzalloc(>mdio.dev, sizeof(struct phy_device_led),
> > GFP_KERNEL);
> > + if (!led)
> > + return -ENOMEM;
> > +
> > + led->cdev.brightness_set_blocking = phy_led_brightness_set;
> > + led->cdev.trigger_type = _hw_led_trig_type;
> > + led->addr = reg;
> > +
> > + of_property_read_string(np, "linux,default-trigger",
> > >cdev.default_trigger);
>
> Hi Marek
>
> I think we need one more optional property. If the trigger has been
> set to the PHY hardware trigger, we then should be able to set which
> of the different blink patterns we want the LED to use. I guess most
> users will never actually make use of the sys/class/led interface, if
> the default in device tree is sensible. But that requires DT can fully
> configure the LED.
>
>Andrew
Yes, I also thought about that. We have the linux,default-trigger
property, so maybe we could add linux,default-hw-control-mode property
as well.
Marek
Re: [PATCH RFC leds + net-next v4 1/2] net: phy: add API for LEDs controlled by PHY HW
On Tue, 28 Jul 2020 18:28:16 +0200
Andrew Lunn wrote:
> > > @@ -736,6 +777,16 @@ struct phy_driver {
> > > int (*set_loopback)(struct phy_device *dev, bool enable);
> > > int (*get_sqi)(struct phy_device *dev);
> > > int (*get_sqi_max)(struct phy_device *dev);
> > > +
> > > + /* PHY LED support */
> > > + int (*led_init)(struct phy_device *dev, struct
> > > phy_device_led *led);
> > > + int (*led_brightness_set)(struct phy_device *dev, struct
> > > phy_device_led *led,
> > > + enum led_brightness brightness);
> > > + const char *(*led_iter_hw_mode)(struct phy_device *dev,
> > > struct phy_device_led *led,
> > > + void ** iter);
> > > + int (*led_set_hw_mode)(struct phy_device *dev, struct
> > > phy_device_led *led,
> > > +const char *mode);
> > > + const char *(*led_get_hw_mode)(struct phy_device *dev,
> > > struct phy_device_led *led); };
> > > #define to_phy_driver(d)
> > > container_of(to_mdio_common_driver(d),\ struct
> > > phy_driver, mdiodrv)
> >
> > The problem here is that the same code will have to be added to DSA
> > switch ops structure, which is not OK.
>
> Not necessarily. DSA drivers do have access to the phydev structure.
>
> I think putting these members into a structure is a good idea. That
> structure can be part of phy_driver and initialised just like other
> members. But on probing the phy, it can be copied over to the
> phy_device structure. And we can provide an API which DSA drivers can
> use to register there own structure of ops to be placed into
> phy_device, which would call into the DSA driver.
>
> Andrew
On Marvell switches there are LEDs that do not necesarrily blink on
events on a specific port, but instead on the whole switch. Ie a LED
can be put into a mode "act on any port". Vendors may create devices
with this as intender mode for a LED, and such a LED may be on the
other side of the device from where the ports are, or something. Such a
LED should be described in the device tree not as a child of any PHY or
port, but instead as a child of the switch itself. And since all the
LEDs on Marvell switches are technically controlled by the switch, not
it's internal PHYs, I think all of them should be children of the
switch node (or a "leds" node which is a child of the switch node),
instead of being descended from the internal PHYs.
Marek
Re: [PATCH RFC leds + net-next v3 2/2] net: phy: marvell: add support for PHY LEDs via LED class
On Sat, 25 Jul 2020 20:48:46 +0200
Andrew Lunn wrote:
> > > > +#if 0
> > > > + /* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */
> > > > + /* TODO: Support DUAL MODE */
> > > > + if (color == LED_COLOR_ID_MULTI) {
> > > > + phydev_warn(phydev, "node %pOF: This driver does not
> > > > yet support multicolor LEDs\n",
> > > > + np);
> > > > + return -ENOTSUPP;
> > > > + }
> > > > +#endif
> > >
> > > Code getting committed should not be using #if 0. Is the needed code
> > > in the LED tree? Do we want to consider a stable branch of the LED
> > > tree which DaveM can pull into net-next? Or do you want to wait until
> > > the next merge cycle?
> >
> > That's why this is RFC. But yes, I would like to have this merged for
> > 5.9, so maybe we should ask Dave. Is this common? Do we also need to
> > tell Pavel or how does this work?
>
> The Pavel needs to create a stable branch. DaveM then merges that
> branch into net-next. Your patches can then be merged. When Linus
> pulls the two branches, led and net-next, git sees the exact same
> patches twice, and simply drops them from the second pull request.
>
> So you need to ask Pavel and DaveM if they are willing to do this.
>
> > > > + init_data.fwnode = >fwnode;
> > > > + init_data.devname_mandatory = true;
> > > > + init_data.devicename = phydev->attached_dev ?
> > > > netdev_name(phydev->attached_dev) : "";
> > >
> > > This we need to think about. Are you running this on a system with
> > > systemd? Does the interface have a name like enp2s0? Does the LED get
> > > registered before or after systemd renames it from eth0 to enp2s0?
> >
> > Yes, well, this should be discussed also with LED guys. I don't suppose
> > that renaming the sysfs symlink on interface rename event is
> > appropriate, but who knows?
> > The interfaces are platform specific, on mvebu. They aren't connected
> > via PCI, so their names remain eth0, eth1 ...
>
> But the Marvell driver is used with more than just mvebu. And we need
> this generic. There are USB Ethernet dongles which used phylib. They
> will get their interfaces renamed to include the MAC address, etc.
>
> It is possible to hook the notifier so we know when an interface is
> renamed. We can then either destroy and re-create the LED, or if the
> LED framework allows it, rename it.
>
> Or we avoid interface names all together and stick with the phy name,
> which is stable. To make it more user friendly, you could create
> additional symlinks. We already have /sys/class/net/ethX/phydev
> linking into sys/bus/mdio_bus/devices/.. . We could add
> /sys/class/net/ethX/ledY linking into /sys/class/led/...
>
> It would also be possible to teach ethtool about LEDs, so that it
> follows the symbolic links, and manipulates the LED class files.
I will propose rename of the LED name on interface rename event.
> > I also want this code to be generalized somehow so that it can be
> > reused. The problem is that I want to have support for DUAL mode, which
> > is Marvell specific, and a DUAL LED needs to be defined in device tree.
>
> It sounds like you first need to teach the LED core about dual LEDs
> and triggers which affect two LEDs..
This is already done. DUAL LEDs will be handled by the multicolor LED
framework which also is already in Pavel's tree. The problem is that if
we make PHY LEDs OF parsing code generic in phy-core, it will also need
to be robust enough to take care of DUAL LEDs OF parsing. That is
something which is Marvell specific. It is possible that some other
vendor also manufactures PHYs with something like that, but the LEDs
on other PHYs may have different maximum value of brightness, or
additional configurations which will need to be in device tree...
But I will try to make it generic and then we will see where it goes...
Marek
Re: [PATCH RFC leds + net-next v3 2/2] net: phy: marvell: add support for PHY LEDs via LED class
On Sat, 25 Jul 2020 19:23:18 +0200
Andrew Lunn wrote:
> On Fri, Jul 24, 2020 at 06:46:03PM +0200, Marek Behún wrote:
> > This patch adds support for controlling the LEDs connected to several
> > families of Marvell PHYs via the PHY HW LED trigger API. These families
> > are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> > be added.
> >
> > The code reads LEDs definitions from the device-tree node of the PHY.
> >
> > This patch does not yet add support for compound LED modes. This could
> > be achieved via the LED multicolor framework (which is not yet in
> > upstream).
> >
> > Settings such as HW blink rate or pulse stretch duration are not yet
> > supported, nor are LED polarity settings.
> >
> > Signed-off-by: Marek Behún
> > ---
> > drivers/net/phy/Kconfig | 1 +
> > drivers/net/phy/marvell.c | 364 ++
> > 2 files changed, 365 insertions(+)
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index ffea11f73acd..5428a8af26d2 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -462,6 +462,7 @@ config LXT_PHY
> >
> > config MARVELL_PHY
> > tristate "Marvell PHYs"
> > + depends on LED_TRIGGER_PHY_HW
>
> Does it really depend on it? I think the driver will work fine without
> it, just the LED control will be missing.
>
> It is really a policy question. Cable test is always available, there
> is no Kconfig'ury to stop it getting built. Is LED support really big
> so that somebody might want to disable it? I think not. So lets just
> enable it all the time.
OK
> > help
> > Currently has a driver for the 88E1011S
> >
>
> > +enum {
> > + L1V0_RECV = BIT(0),
> > + L1V0_COPPER = BIT(1),
> > + L1V5_100_FIBER = BIT(2),
> > + L1V5_100_10 = BIT(3),
> > + L2V2_INIT = BIT(4),
> > + L2V2_PTP= BIT(5),
> > + L2V2_DUPLEX = BIT(6),
> > + L3V0_FIBER = BIT(7),
> > + L3V0_LOS= BIT(8),
> > + L3V5_TRANS = BIT(9),
> > + L3V7_FIBER = BIT(10),
> > + L3V7_DUPLEX = BIT(11),
>
> Maybe also add COMMON?
These are meant to be interpreted as flag bits, one LED can have
multiple flags. Most time in kernel bit fields use 0, not a constant,
to express no flag.
> > +};
> > +
> > +struct marvell_led_mode_info {
> > + const char *name;
> > + s8 regval[MARVELL_PHY_MAX_LEDS];
> > + u32 flags;
>
> Maybe give the enum a name, and use it here? It can be quite hard
> tracking the meaning of flags in this code. Here, it is an indication
> of an individual feature.
led_flags?
>
> > +};
> > +
> > +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> > + { "link", { 0x0, -1, 0x0, -1, -1, -1, }, 0 },
> >
>
> Replace flags 0 with COMMON?
>
> > + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
> > + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 },
> > + { "act",{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
> > + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
> > + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 },
> > + { "tx", { -1, -1, -1, 0x5, -1, -1, },
> > L3V5_TRANS },
> > + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 },
> > + { "rx", { -1, 0x0, -1, -1, -1, -1, },
> > L1V0_RECV },
> > + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 },
> > + { "copper", { -1, 0x0, -1, -1, -1, -1, },
> > L1V0_COPPER },
> > + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 },
> > + { "link/rx",{ -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 },
> > + { "100Mbps-fiber", { -1, 0x5, -1, -1, -1, -1, },
> > L1V5_100_FIBER },
> > + { "100Mbps-10Mbps", { -1, 0x5, -1, -1, -1, -1, },
> > L1V5_100_10 },
> > + { "1Gbps-100Mbps", { -1, 0x6, -1, -1, -1, -1, }, 0 },
> > + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 },
> > + { "100Mbps",{ -1, 0x7, -1, -1, -1, -1, }, 0 },
> > + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 },
> > + { "fiber", { -1, -1, -1, 0x0, -1, -1, },
> > L3V0_FIBER },
> > + { "fiber", { -1, -1, -1, 0x7, -1, -1, },
> > L3V7_FIBER },
> > + { "FullDuplex", { -1, -1, -1, 0x7, -1, -1, },
> > L3V7_DUPLEX },
> > + { "FullDuplex", { -1, -1, -1, -1, 0x6, 0x6, }, 0 },
> > + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 },
> > + { "FullDuplex/collision", { -1, -1, 0x2, -1, -1, -1, },
> > L2V2_DUPLEX },
> > +
Re: [PATCH RFC leds + net-next v3 2/2] net: phy: marvell: add support for PHY LEDs via LED class
On Sat, 25 Jul 2020 17:03:42 +0200 Andrew Lunn wrote: > Does hi-z mean off? In the implementation i did, i did not list off > and on as triggers. I instead used them for untriggered > brightness. That allowed the software triggers to work, so i had the > PHY blinking the heartbeat etc. But i had to make it optional, since a > quick survey of datasheets suggested not all PHYs support simple > on/off control. I don't actually know what hi-z means, but enabling it disabled the LED. But there is another register value for OFF... > Something beyond the scope of this patchset is implementing etHool -p > >-p --identify > Initiates adapter-specific action intended to enable an > operator to > easily identify the adapter by sight. Typically this involves > blink‐ > ing one or more LEDs on the specific network port. > > If we have software controlled on/off, then a software trigger seems > like i good way to do this. I'll look into this. Marek
Re: [PATCH RFC leds + net-next v3 1/2] net: phy: add API for LEDs controlled by PHY HW
On Sat, 25 Jul 2020 11:21:24 +0200
Pavel Machek wrote:
> Hi!
>
> > Many PHYs support various HW control modes for LEDs connected directly
> > to them.
> >
> > This code adds a new private LED trigger called phydev-hw-mode. When
> > this trigger is enabled for a LED, the various HW control modes which
> > the PHY supports for given LED can be get/set via hw_mode sysfs file.
> >
> > A PHY driver wishing to utilize this API needs to register the LEDs on
> > its own and set the .trigger_type member of LED classdev to
> > _hw_led_trig_type. It also needs to implement the methods
> > .led_iter_hw_mode, .led_set_hw_mode and .led_get_hw_mode in struct
> > phydev.
> >
> > Signed-off-by: Marek Behún
>
> Nothing too wrong.
>
> New sysfs file will require documentation.
>
> Plus I wonder: should we have single hw_mode file? It seems many
> different "bits" fit inside. Would it be possible to split it further,
> and have bits saying:
>
> "I want the LED to be on if link is 10Mbps".
> "I want the LED to be on if link is 100Mbps".
> "I want the LED to be on if link is 1000Mbps".
> "I want the LED to blink on tx".
> "I want the LED to blink on rx".
>
> ?
I don't think this is possible. Only specific combinations are possible
on Marvell PHYs. There is no HW control mode which would do, for
example:
- ON when linked to 100Mbps
- BLINK when receive
PHYs from other vendors can different mode sets.
Marek
> + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1,
> + { "1Gbps", { 0x7, -1, -1, -1, -1,
> + { "100Mbps-fiber", { -1, 0x5, -1, -1, -1,
> + { "100Mbps-10Mbps", { -1, 0x5, -1, -1, -1,
> + { "1Gbps-100Mbps", { -1, 0x6, -1, -1, -1,
> + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1,
> + { "100Mbps",{ -1, 0x7, -1, -1, -1,
> + { "10Mbps", { -1, -1, 0x7, -1, -1,
>
> Best regards,
> Pavel
>
Re: [PATCH RFC leds + net-next v3 2/2] net: phy: marvell: add support for PHY LEDs via LED class
On Sat, 25 Jul 2020 11:23:39 +0200
Pavel Machek wrote:
> Hi!
>
> > +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> > + { "link", { 0x0, -1, 0x0, -1, -1, -1, }, 0 },
> > + { "link/act", { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
> > + { "1Gbps/100Mbps/10Mbps", { 0x2, -1, -1, -1, -1, -1, }, 0 },
> >
>
> is this "1Gbps-10Mbps"?
Most of these modes mean "ON on event", eg
"link" means ON when link up, else OFF. "
"tx" means ON on when transmitting, else OFF
"act" means ON when activity, else OFF
"copper" means ON when copper link up, else OFF
but some are blinking modes
"blink-act" means BLINK when activity, else OFF
Some modes can do ON and BLINK, these have one '/' in their name
"link/act" means ON when link up, BLINK on activity, else OFF
"link/rx" means ON when link up, BLINK on receive, else OFF
there is one mode, "1Gbps/100Mbps/10Mbps", which behaves differently:
blinks 3 times when linked on 1Gbps
blinks 2 times when linked on 100Mbps
blinks 1 time when linked on 10Mbps
(and this blinking is repeating, ie blinks 3 times, pause, blinks 3 times,
pause)
Some modes are disjunctive:
"100Mbps-fiber" means ON when linked on 100Mbps or via fiber, else OFF
>
> > + { "act",{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
> > + { "blink-act", { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
> > + { "tx", { 0x5, -1, 0x5, -1, 0x5, 0x5, }, 0 },
> > + { "tx", { -1, -1, -1, 0x5, -1, -1, },
> > L3V5_TRANS },
> > + { "rx", { -1, -1, -1, -1, 0x0, 0x0, }, 0 },
> > + { "rx", { -1, 0x0, -1, -1, -1, -1, },
> > L1V0_RECV },
> > + { "copper", { 0x6, -1, -1, -1, -1, -1, }, 0 },
> > + { "copper", { -1, 0x0, -1, -1, -1, -1, },
> > L1V0_COPPER },
> > + { "1Gbps", { 0x7, -1, -1, -1, -1, -1, }, 0 },
> > + { "link/rx",{ -1, 0x2, -1, 0x2, 0x2, 0x2, }, 0 },
> > + { "100Mbps-fiber", { -1, 0x5, -1, -1, -1, -1, },
> > L1V5_100_FIBER },
> > + { "100Mbps-10Mbps", { -1, 0x5, -1, -1, -1, -1, },
> > L1V5_100_10 },
> > + { "1Gbps-100Mbps", { -1, 0x6, -1, -1, -1, -1, }, 0 },
> > + { "1Gbps-10Mbps", { -1, -1, 0x6, 0x6, -1, -1, }, 0 },
> > + { "100Mbps",{ -1, 0x7, -1, -1, -1, -1, }, 0 },
> > + { "10Mbps", { -1, -1, 0x7, -1, -1, -1, }, 0 },
> > + { "fiber", { -1, -1, -1, 0x0, -1, -1, },
> > L3V0_FIBER },
> > + { "fiber", { -1, -1, -1, 0x7, -1, -1, },
> > L3V7_FIBER },
> > + { "FullDuplex", { -1, -1, -1, 0x7, -1, -1, },
> > L3V7_DUPLEX },
> > + { "FullDuplex", { -1, -1, -1, -1, 0x6, 0x6, }, 0 },
> > + { "FullDuplex/collision", { -1, -1, -1, -1, 0x7, 0x7, }, 0 },
> > + { "FullDuplex/collision", { -1, -1, 0x2, -1, -1, -1, },
> > L2V2_DUPLEX },
> > + { "ptp",{ -1, -1, 0x2, -1, -1, -1, },
> > L2V2_PTP },
> > + { "init", { -1, -1, 0x2, -1, -1, -1, },
> > L2V2_INIT },
> > + { "los",{ -1, -1, -1, 0x0, -1, -1, },
> > L3V0_LOS },
> > + { "hi-z", { 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
> > + { "blink", { 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
> > +};
>
> Certainly more documentation will be required here, what "ptp" setting
> does, for example, is not very obvious to me.
"ptp" means it will light up when the PTP functionality is enabled on
the PHY and a PTP packet is received.
> Best regards,
> Pavel
Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class
On Thu, 23 Jul 2020 23:35:31 +0200 Andrew Lunn wrote: > Hi Marek > > I expect some of this should be moved into the phylib core. We don't > want each PHY inventing its own way to do this. The core should > provide a framework and the PHY driver fills in the gaps. > > Take a look at for example mscc_main.c and its LED information. It has > pretty similar hardware to the Marvell. And microchip.c also has LED > handling, etc. OK, this makes sense. I will have to think about this a little. My main issue though is whether one "hw-control" trigger should be registered via LED API and the specific mode should be chosen via another sysfs file as in this RFC, or whether each HW control mode should have its own trigger. The second solution would either result in a lot of registered triggers or complicate LED API, though... Marek
Re: [PATCH RFC leds + net-next v2 1/1] net: phy: marvell: add support for PHY LEDs via LED class
On Thu, 23 Jul 2020 23:35:31 +0200 Andrew Lunn wrote: > I thought the brightness file disappeared when a trigger takes > over. So is this possible? > > Andrew It does not disappear nor should it. When you have a LED with 10 levels of brightness, you want to be able to configure with which brightness it blinks when controlled by a trigger. SW triggers use that brightness which was stored into the brightness file when blinking the LED. Marek
Re: [PATCH RFC] leds: Add support for per-LED device triggers
On Mon, 13 Jul 2020 01:15:44 +0200 Marek Behun wrote: > On Mon, 13 Jul 2020 00:38:21 +0200 > Ondřej Jirman wrote: > > > So after trying to use this, this seems to disallow the use of multiple HW > > triggers per LED. That's fine by me, because using one HW sysfs configured > > trigger per LED that use case is my proposal, but is it desireable in > > general? > > Why? If you register one LED and several triggers, all sharing the same > trigger_type pointer, I think it should work. > > Marek The problem arises when I have two LEDs and two HW triggers, and the hardware allows setting one HW trigger on both LEDs and other HW trigger only on one LED. But this could simply be ignored - the set_trigger function could simply return -ENOTSUPP or something. Marek
Re: [PATCH RFC] leds: Add support for per-LED device triggers
On Mon, 13 Jul 2020 00:38:21 +0200
Ondřej Jirman wrote:
> Hello,
>
> On Sun, Jul 12, 2020 at 09:11:11PM +0200, Pavel Machek wrote:
> > Hi!
> >
>
> []
>
> > }
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 2451962d1ec5..cba52714558f 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -57,6 +57,10 @@ struct led_init_data {
> > bool devname_mandatory;
> > };
> >
> > +struct led_hw_trigger_type {
> > + int dummy;
> > +}
> > +
> > struct led_classdev {
> > const char *name;
> > enum led_brightness brightness;
> > @@ -150,6 +154,8 @@ struct led_classdev {
> >
> > /* Ensures consistent access to the LED Flash Class device */
> > struct mutexled_access;
> > +
> > + struct led_hw_trigger_type *trigger_type;
> > };
> >
> > /**
> > @@ -345,6 +351,9 @@ struct led_trigger {
> > int (*activate)(struct led_classdev *led_cdev);
> > void(*deactivate)(struct led_classdev *led_cdev);
> >
> > + /* LED-private triggers have this set. */
> > + struct led_hw_trigger_type *trigger_type;
> > +
> > /* LEDs under control by this trigger (for simple triggers) */
> > rwlock_t leddev_list_lock;
> > struct list_head led_cdevs;
>
> So after trying to use this, this seems to disallow the use of multiple HW
> triggers per LED. That's fine by me, because using one HW sysfs configured
> trigger per LED that use case is my proposal, but is it desireable in general?
Why? If you register one LED and several triggers, all sharing the same
trigger_type pointer, I think it should work.
Marek
Re: [PATCH RFC] leds: Add support for per-LED device triggers
On Sun, 12 Jul 2020 21:11:11 +0200
Pavel Machek wrote:
> Hi!
>
> > > > > > Some LED controllers may come with an internal HW triggering
> > > > > > mechanism
> > > > > > for the LED and an ability to switch between user control of the
> > > > > > LED,
> > > > > > or the internal control. One such example is AXP20X PMIC, that
> > > > > > allows
> > > > > > wither for user control of the LED, or for internal control based on
> > > > > > the state of the battery charger.
> > > > > >
> > > > > > Add support for registering per-LED device trigger.
> > > > > >
> > > > > > Names of private triggers need to be globally unique, but may clash
> > > > > > with other private triggers. This is enforced during trigger
> > > > > > registration. Developers can register private triggers just like
> > > > > > the normal triggers, by setting private_led to a classdev
> > > > > > of the LED the trigger is associated with.
> > > > >
> > > > > What about this? Should address Marek's concerns about resource
> > > > > use...
> > > >
> > > > What concerns? Marek's concerns seem to be about case where we register
> > > > a trigger for (each led * self-working configuration) which I admit
> > > > can be quite a lot of triggers if there are many functions. But that's
> > > > not my proposal.
> > > >
> > > > My proposal is to only register on trigger per LED at most. So on my
> > > > system that's 1 extra trigger and on Marek's system that'd be 48 new
> > > > triggers. Neither seems like a meaningful problem from resource
> > > > use perspective.
> > >
> > > So.. 48 triggers on Marek's systems means I'll not apply your patch.
> > >
> > > Please take a look at my version, it is as simple and avoids that
> > > problem.
> >
> > I would, but I don't see your version linked or mentioned in this
> > thread.
>
> Ah! Sorry about that. Here it is. (I verified it compiles in the
> meantime).
>
> Best regards,
> Pavel
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..e8333675959c 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
>
> /* Used by LED Class */
>
> +static inline bool
> +trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
> +{
> + return !trig->trigger_type || trig->trigger_type ==
> led_cdev->trigger_type;
> +}
> +
> ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr, char *buf,
> loff_t pos, size_t count)
> @@ -50,7 +56,8 @@ ssize_t led_trigger_write(struct file *filp, struct kobject
> *kobj,
>
> down_read(_list_lock);
> list_for_each_entry(trig, _list, next_trig) {
> - if (sysfs_streq(buf, trig->name)) {
> + if (sysfs_streq(buf, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> down_write(_cdev->trigger_lock);
> led_trigger_set(led_cdev, trig);
> up_write(_cdev->trigger_lock);
> @@ -96,6 +103,9 @@ static int led_trigger_format(char *buf, size_t size,
> bool hit = led_cdev->trigger &&
> !strcmp(led_cdev->trigger->name, trig->name);
>
> + if (!trigger_relevant(led_cdev, trig))
> + continue;
> +
> len += led_trigger_snprintf(buf + len, size - len,
> " %s%s%s", hit ? "[" : "",
> trig->name, hit ? "]" : "");
> @@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev
> *led_cdev)
> down_read(_list_lock);
> down_write(_cdev->trigger_lock);
> list_for_each_entry(trig, _list, next_trig) {
> - if (!strcmp(led_cdev->default_trigger, trig->name)) {
> + if (!strcmp(led_cdev->default_trigger, trig->name) &&
> + trigger_relevant(led_cdev, trig)) {
> led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
> led_trigger_set(led_cdev, trig);
> break;
> @@ -280,7 +291,8 @@ int led_trigger_register(struct led_trigger *trig)
> down_write(_list_lock);
> /* Make sure the trigger's name isn't already in use */
> list_for_each_entry(_trig, _list, next_trig) {
> - if (!strcmp(_trig->name, trig->name)) {
> + if (!strcmp(_trig->name, trig->name) &&
> + (!_trig->private_led || _trig->private_led ==
> trig->private_led)) {
> up_write(_list_lock);
> return -EEXIST;
> }
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..cba52714558f 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -57,6 +57,10 @@ struct led_init_data {
> bool
Re: [PATCH v29 05/16] leds: lp50xx: Add the LP50XX family of the RGB LED driver
Hi Dan, one bug in this driver, see below. On Mon, 22 Jun 2020 13:59:08 -0500 Dan Murphy wrote: > Introduce the LP5036/30/24/18/12/9 RGB LED driver. > The difference in these parts are the number of > LED outputs where the: > > LP5036 can control 36 LEDs > LP5030 can control 30 LEDs > LP5024 can control 24 LEDs > LP5018 can control 18 LEDs > LP5012 can control 12 LEDs > LP5009 can control 9 LEDs > > The device has the ability to group LED output into control banks > so that multiple LED banks can be controlled with the same mixing and > brightness. Inversely the LEDs can also be controlled independently. > > Acked-by: Jacek Anaszewski > Signed-off-by: Dan Murphy > --- > drivers/leds/Kconfig | 11 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-lp50xx.c | 783 + > 3 files changed, 795 insertions(+) > create mode 100644 drivers/leds/leds-lp50xx.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 0d034453eeb9..68f63d1a7d48 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -384,6 +384,17 @@ config LEDS_LP3952 > To compile this driver as a module, choose M here: the > module will be called leds-lp3952. > > +config LEDS_LP50XX > + tristate "LED Support for TI LP5036/30/24/18/12/9 LED driver chip" > + depends on LEDS_CLASS && REGMAP_I2C > + depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR > + help > + If you say yes here you get support for the Texas Instruments > + LP5036, LP5030, LP5024, LP5018, LP5012 and LP5009 LED driver. > + > + To compile this driver as a module, choose M here: the > + module will be called leds-lp50xx. > + > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 53a752c32e67..68c05faec99e 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -49,6 +49,7 @@ obj-$(CONFIG_LEDS_LM3697) += leds-lm3697.o > obj-$(CONFIG_LEDS_LOCOMO)+= leds-locomo.o > obj-$(CONFIG_LEDS_LP3944)+= leds-lp3944.o > obj-$(CONFIG_LEDS_LP3952)+= leds-lp3952.o > +obj-$(CONFIG_LEDS_LP50XX)+= leds-lp50xx.o > obj-$(CONFIG_LEDS_LP5521)+= leds-lp5521.o > obj-$(CONFIG_LEDS_LP5523)+= leds-lp5523.o > obj-$(CONFIG_LEDS_LP5562)+= leds-lp5562.o > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c > new file mode 100644 > index ..407c2f42bac2 > --- /dev/null > +++ b/drivers/leds/leds-lp50xx.c > @@ -0,0 +1,783 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// TI LP50XX LED chip family driver > +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include "leds.h" > + > +#define LP50XX_DEV_CFG0 0x00 > +#define LP50XX_DEV_CFG1 0x01 > +#define LP50XX_LED_CFG0 0x02 > + > +/* LP5009 and LP5012 registers */ > +#define LP5012_BNK_BRT 0x03 > +#define LP5012_BNKA_CLR 0x04 > +#define LP5012_BNKB_CLR 0x05 > +#define LP5012_BNKC_CLR 0x06 > +#define LP5012_LED0_BRT 0x07 > +#define LP5012_LED1_BRT 0x08 > +#define LP5012_LED2_BRT 0x09 > +#define LP5012_LED3_BRT 0x0a > +#define LP5012_OUT0_CLR 0x0b > +#define LP5012_OUT1_CLR 0x0c > +#define LP5012_OUT2_CLR 0x0d > +#define LP5012_OUT3_CLR 0x0e > +#define LP5012_OUT4_CLR 0x0f > +#define LP5012_OUT5_CLR 0x10 > +#define LP5012_OUT6_CLR 0x11 > +#define LP5012_OUT7_CLR 0x12 > +#define LP5012_OUT8_CLR 0x13 > +#define LP5012_OUT9_CLR 0x14 > +#define LP5012_OUT10_CLR 0x15 > +#define LP5012_OUT11_CLR 0x16 > +#define LP5012_RESET 0x17 > + > +/* LP5018 and LP5024 registers */ > +#define LP5024_BNK_BRT 0x03 > +#define LP5024_BNKA_CLR 0x04 > +#define LP5024_BNKB_CLR 0x05 > +#define LP5024_BNKC_CLR 0x06 > +#define LP5024_LED0_BRT 0x07 > +#define LP5024_LED1_BRT 0x08 > +#define LP5024_LED2_BRT 0x09 > +#define LP5024_LED3_BRT 0x0a > +#define LP5024_LED4_BRT 0x0b > +#define LP5024_LED5_BRT 0x0c > +#define LP5024_LED6_BRT 0x0d > +#define LP5024_LED7_BRT 0x0e > + > +#define LP5024_OUT0_CLR 0x0f > +#define LP5024_OUT1_CLR 0x10 > +#define LP5024_OUT2_CLR 0x11 > +#define LP5024_OUT3_CLR 0x12 > +#define LP5024_OUT4_CLR
Re: [PATCH v29 00/16] Multicolor Framework v29
On Sat, 4 Jul 2020 14:47:29 +0200 Pavel Machek wrote: > Hi! > > > This is the multi color LED framework. This framework presents clustered > > colored LEDs into an array and allows the user space to adjust the > > brightness > > of the cluster using a single file write. The individual colored LEDs > > intensities are controlled via a single file that is an array of LEDs > > > > Change to the LEDs Kconfig to fix dependencies on the LP55XX_COMMON. > > Added update to the u8500_defconfig > > Marek, would you be willing to look over this series? Overall this series looks good to me. I wanted to apply version 29 of the patches, but I didn't receive all patches in v29 (some are missing), so I had to search for previous versions of selected patches. I have seen some typos in documentation, but that can be solved afterwards. One thing I don't like much is that in the sysfs multi_index and multi_intensity files there is a trailing space after the last color. This is not true for example for the trigger file. It is trivial to fix this, so again maybe a will send a follow-up patch after this series is accepted. Marek > Dan, can we please get it in the order > > 1) fixes first > > 2) changes needed for multicolor but not depending on dt acks > > 3) dt changes > > 4) rest? > > This is the order it should have been in the first place, and I'd like > to get fixes applied, and perhaps some of the preparation. > > Best regards, > Pavel >
Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options
On Fri, 5 Jun 2020 19:10:58 +0100
Jonathan McDowell wrote:
> The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
> mode depending on what it's connected to (e.g. CPU vs external PHY or
> SFP). At present the driver does no configuration of this port even if
> it is selected.
>
> Add support for making sure the SGMII is enabled if it's in use, and
> device tree support for configuring the connection details.
>
> Signed-off-by: Jonathan McDowell
> ---
> drivers/net/dsa/qca8k.c | 44 -
> drivers/net/dsa/qca8k.h | 12 +++
> 2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 9f4205b4439b..5b7979aca9b9 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -418,6 +418,40 @@ qca8k_mib_init(struct qca8k_priv *priv)
> mutex_unlock(>reg_mutex);
> }
>
> +static int
> +qca8k_setup_sgmii(struct qca8k_priv *priv)
> +{
> + const char *mode;
> + u32 val;
> +
> + val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> +
> + val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> + QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> +
> + if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))
> + val |= QCA8K_SGMII_CLK125M_DELAY;
> +
> + if (of_property_read_string(priv->dev->of_node, "sgmii-mode", )) {
> + val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> +
> + if (!strcasecmp(mode, "basex")) {
> + val |= QCA8K_SGMII_MODE_CTRL_BASEX;
> + } else if (!strcasecmp(mode, "mac")) {
> + val |= QCA8K_SGMII_MODE_CTRL_MAC;
> + } else if (!strcasecmp(mode, "phy")) {
> + val |= QCA8K_SGMII_MODE_CTRL_PHY;
> + } else {
> + pr_err("Unrecognised SGMII mode %s\n", mode);
> + return -EINVAL;
> + }
> + }
There is no sgmii-mode device tree property documented. You should
infere this settings from the existing device tree bindings, ie look at
phy-mode. You can use of_get_phy_mode function, or something from
of_mdio.c, or better yet change the api in this driver to use the new
phylink API.
Marek
> +
> + qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
> +
> + return 0;
> +}
> +
> static int
> qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
> {
> @@ -458,7 +492,8 @@ qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int
> mode)
> break;
> case PHY_INTERFACE_MODE_SGMII:
> qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> - break;
> +
> + return qca8k_setup_sgmii(priv);
> default:
> pr_err("xMII mode %d not supported\n", mode);
> return -EINVAL;
> @@ -661,6 +696,13 @@ qca8k_setup(struct dsa_switch *ds)
> if (ret)
> return ret;
>
> + if (of_property_read_bool(priv->dev->of_node,
> + "disable-serdes-autoneg")) {
> + mask = qca8k_read(priv, QCA8K_REG_PWS) |
> +QCA8K_PWS_SERDES_AEN_DIS;
> + qca8k_write(priv, QCA8K_REG_PWS, mask);
> + }
> +
> /* Initialize CPU port pad mode (xMII type, delays...) */
> ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, _mode);
> if (ret) {
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 42d6ea24eb14..cd97c212f3f8 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -36,6 +36,8 @@
> #define QCA8K_MAX_DELAY3
> #define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24)
> #define QCA8K_PORT_PAD_SGMII_ENBIT(7)
> +#define QCA8K_REG_PWS0x010
> +#define QCA8K_PWS_SERDES_AEN_DIS BIT(7)
> #define QCA8K_REG_MODULE_EN 0x030
> #define QCA8K_MODULE_EN_MIBBIT(0)
> #define QCA8K_REG_MIB0x034
> @@ -77,6 +79,16 @@
> #define QCA8K_PORT_HDR_CTRL_ALL2
> #define QCA8K_PORT_HDR_CTRL_MGMT 1
> #define QCA8K_PORT_HDR_CTRL_NONE 0
> +#define QCA8K_REG_SGMII_CTRL 0x0e0
> +#define QCA8K_SGMII_EN_PLL BIT(1)
> +#define QCA8K_SGMII_EN_RX BIT(2)
> +#define QCA8K_SGMII_EN_TX BIT(3)
> +#define QCA8K_SGMII_EN_SD BIT(4)
> +#define QCA8K_SGMII_CLK125M_DELAY BIT(7)
> +#define QCA8K_SGMII_MODE_CTRL_MASK (BIT(22) | BIT(23))
> +#define QCA8K_SGMII_MODE_CTRL_BASEX0
> +#define QCA8K_SGMII_MODE_CTRL_PHY BIT(22)
> +#define QCA8K_SGMII_MODE_CTRL_MAC BIT(23)
>
> /* EEE control registers */
>
Re: [PATCH v4 00/12] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards
On Mon, 18 May 2020 14:46:14 +0100 Lorenzo Pieralisi wrote: > On Mon, May 18, 2020 at 12:30:04PM +0200, Pali Rohár wrote: > > On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote: > > > Hello, > > > > > > > Hello, > > > > > > > > On Thu, 30 Apr 2020 10:06:13 +0200 > > > > Pali Rohár wrote: > > > > > > > >> Marek Behún (5): > > > >> PCI: aardvark: Improve link training > > > >> PCI: aardvark: Add PHY support > > > >> dt-bindings: PCI: aardvark: Describe new properties > > > >> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function > > > >> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property > > > >> > > > >> Pali Rohár (7): > > > >> PCI: aardvark: Train link immediately after enabling training > > > >> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to > > > >> read-only register > > > >> PCI: of: Zero max-link-speed value is invalid > > > >> PCI: aardvark: Issue PERST via GPIO > > > >> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access > > > >> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h > > > >> macros > > > >> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property > > > > > > > > Thanks a lot for this work. For a number of reasons, I'm less involved > > > > in Marvell platform support in Linux, but I reviewed your series and > > > > followed the discussions around it, and I'm happy to give my: > > > > > > > > Acked-by: Thomas Petazzoni > > > > > > With this acked-by for the series, the reviewed-by from Rob on the > > > binding and the tested-by, I am pretty confident so I applied the > > > patches 10, 11 and 12 on mvebu/dt64. > > > > > > Thanks, > > > > > > Gregory > > > > Thank you! > > > > Lorenzo, would you now take remaining patches? > > Yes - even though I have reservations about patch (5) and the > problem is related to a complete lack of programming model for > these host controllers and a clear separation between what's > done in the OS vs bootloader, PERST handling in this host > bridge is *really* a mess. > > I applied 1-9 to pci/aardvark. > > Lorenzo Hooray, thanks, Lorenzo (and everyone else).
Re: linux-next: Fixes tags need some work in the mvebu tree
On Wed, 9 Oct 2019 07:38:03 +1100
Stephen Rothwell wrote:
> Hi all,
>
> In commit
>
> 69eea31a26da ("arm64: dts: armada-3720-turris-mox: convert usb-phy to
> phy-supply")
>
> Fixes tag
>
> Fixes: eb6c2eb6c7fb ("usb: host: xhci-plat: Prevent an abnormally
>
This is weird, in the patch I sent the tag ends there with ...")
Marek
Re: [PATCH] bus: moxtet: Update proper type 'size_t' to 'ssize_t'
Hi Austin, this was already fixed and is staged for soc/for-next, see https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git/commit/?h=for-next=6811d26df50d96635dd339cf8cdf43a6abc0c4b6 Thanks, Marek On Wed, 11 Sep 2019 14:59:38 +0900 Austin Kim wrote: > The simple_write_to_buffer() returns ssize_t type value, > which is either positive or negative. > > However 'res' is declared as size_t(unsigned int) > which contains non-negative type. > > So 'res < 0' statement is always false, > this cannot execute execptional-case handling. > > To prevent this case, > update proper type 'size_t' to 'ssize_t' for execptional handling. > > Signed-off-by: Austin Kim > --- > drivers/bus/moxtet.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bus/moxtet.c b/drivers/bus/moxtet.c > index 1ee4570..288a9e4 100644 > --- a/drivers/bus/moxtet.c > +++ b/drivers/bus/moxtet.c > @@ -514,7 +514,7 @@ static ssize_t output_write(struct file *file, const char > __user *buf, > struct moxtet *moxtet = file->private_data; > u8 bin[TURRIS_MOX_MAX_MODULES]; > u8 hex[sizeof(bin) * 2 + 1]; > - size_t res; > + ssize_t res; > loff_t dummy = 0; > int err, i; >
Re: [PATCH][next] bus: moxtet: fix unsigned comparison to less than zero
On Fri, 16 Aug 2019 23:41:06 +0100
Colin King wrote:
> From: Colin Ian King
>
> Currently the size_t variable res is being checked for
> an error failure however the unsigned variable is never
> less than zero so this test is always false. Fix this by
> making variable res ssize_t
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 5bc7f990cd98 ("bus: Add support for Moxtet bus")
> Signed-off-by: Colin Ian King
> ---
> drivers/bus/moxtet.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bus/moxtet.c b/drivers/bus/moxtet.c
> index 1ee4570e7e17..288a9e4c6c7b 100644
> --- a/drivers/bus/moxtet.c
> +++ b/drivers/bus/moxtet.c
> @@ -514,7 +514,7 @@ static ssize_t output_write(struct file *file, const char
> __user *buf,
> struct moxtet *moxtet = file->private_data;
> u8 bin[TURRIS_MOX_MAX_MODULES];
> u8 hex[sizeof(bin) * 2 + 1];
> - size_t res;
> + ssize_t res;
> loff_t dummy = 0;
> int err, i;
>
Hi Colin,
thanks. Should I just Ack this, or do I need to send patch to the
developer who commited my patches?
Thanks.
Marek
