Re: [PATCH v2 00/16] intel-lpss: support non-ACPI platforms

[Mika Westerberg]

On Wed, Jan 06, 2016 at 08:19:49AM -0800, Laura Abbott wrote:
> I picked up all the patches from the device-properties merge but the
> problem still shows up. Are there others I should pick up? Hardware
> details about the touchpad are at 
> https://bugzilla.redhat.com/show_bug.cgi?id=1275718#c34

Can you get full dmesg of the failure and acpidump as well? I did not
find those from the bug report.

Also dmesg of non-failure case would be nice.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 8/8] i2c-mux: relax locking of the top i2c adapter during i2c controlled muxing

[Peter Rosin]

Hi Rob,

On 2016-01-06 15:49, Rob Herring wrote:
> On Tue, Jan 05, 2016 at 04:57:18PM +0100, Peter Rosin wrote:
>> From: Peter Rosin 
>>
>> With a i2c topology like the following
>>
>>GPIO ---|  -- BAT1
>> |  v /
>>I2C  -+--+ MUX
>>  |   \
>>EEPROM -- BAT2
> 
> Yuck. One would think you would just use an I2C controlled mux in this 
> case...
>  
>> there is a locking problem with the GPIO controller since it is a client
>> on the same i2c bus that it muxes. Transfers to the mux clients (e.g. BAT1)
>> will lock the whole i2c bus prior to attempting to switch the mux to the
>> correct i2c segment. In the above case, the GPIO device is an I/O expander
>> with an i2c interface, and since the GPIO subsystem knows nothing (and
>> rightfully so) about the lockless needs of the i2c mux code, this results
>> in a deadlock when the GPIO driver issues i2c transfers to modify the
>> mux.
>>
>> So, observing that while it is needed to have the i2c bus locked during the
>> actual MUX update in order to avoid random garbage on the slave side, it
>> is not strictly a must to have it locked over the whole sequence of a full
>> select-transfer-deselect mux client operation. The mux itself needs to be
>> locked, so transfers to clients behind the mux are serialized, and the mux
>> needs to be stable during all i2c traffic (otherwise individual mux slave
>> segments might see garbage, or worse).
>>
>> Add devive tree properties (bool named i2c-controlled) to i2c-mux-gpio and
>> i2c-mux-pinctrl that asserts that the the gpio/pinctrl is controlled via
>> the same i2c bus that it muxes.
> 
> Can't you determine this condition by checking the mux parent and gpio 
> parent are the same i2c controller?

Good suggestion, I wrote code that implements this, and will include it in
v3. Do not expect v3 to hit the dt crowd though, since no dt changes will
be needed then, but I'm sure that is not a problem...

> Alternatively, can't you just always do the locking like i2c-controlled 
> is set when a mux is involved? What is the harm in doing that if the 
> GPIO is controlled somewhere else?

No, that is not possible. If you change a non-i2c-controlled gpio in the
middle of some i2c-access, the slave side of the mux might see partial
i2c transfers, and that is a recipe for disaster.

> I would prefer to see a solution not requiring DT updates to fix and 
> this change seems like it is working around kernel issues.

Right, I'll make it so.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/8] i2c mux cleanup and locking update

[Peter Rosin]

Hi Antti,

On 2016-01-06 18:17, Antti Palosaari wrote:
> On 01/05/2016 05:57 PM, Peter Rosin wrote:
>> From: Peter Rosin 
>>
>> Hi!
>>
>> I have a pair of boards with this i2c topology:
>>
>> GPIO ---|  -- BAT1
>>  |  v /
>> I2C  -+--B---+ MUX
>>   |   \
>> EEPROM -- BAT2
>>
>> (B denotes the boundary between the boards)
> 
> Handling of I2C muxes that close channel automatically, after the first I2C 
> stop (P) is seen?
> 
> For example channel is selected to BAT1 => there is EEPROM write => mux 
> closes channel BAT1 => access to BAT1 will fail.

The proposed locking changes affect gpio- and pinctrl-controlled muxes
only, and I can't see one of those actually know anything about the
i2c-signals that they mux. Such muxes certainly has to obey the pins
that control them, no?

That fact that other muxes might piggy-back and also use the new
i2c-controlled flag is a different question, and it may indeed not be
safe to declare a mux "i2c-controlled" just to avoid the locking, if
the locking is in fact required. Maybe the name "i2c-controlled" is
poor?

> Is it possible to lock whole adapter, but allow only traffic to i2c mux 
> client?

This is basically what is there today, and which does not work for the
above i2c topology. So maybe you need to expand on what you meant?



There is no neat way for the i2c mux code to dip its fingers into all
relevant i2c-accesses to categorize them as "mux updates" or "mux slave
side accesses", when they come from generic subsystems such as gpio
och pinctrl.

The only thing I have been able to think of in that area is to add i2c
devices that are used to change the mux to a virtual slave adapter of
the mux, so that the mux can identify those accesses and bypass the
locking. But that isn't generic enough to cover the case where one
device is used to control more than one mux (since it needs to sit on
more than one adapter in that case), so I scrapped that idea pretty
early. But I also find that I have to scrap it again and again, since
everything else I cook up usually ends up being some variant of the
virtual slave adapter when I think some more about it. Maybe it *is*
possible to have the same device in different places in the adapter
tree (if all those places have a common ancestor)? But I don't think
so...

That does not address your concerns about extra accesses creeping
in between the mux select and the mux slave side access for auto-
closing muxes (or arbitrators), but I never pretended that there would
be protection from that. If there are such requirements you basically
have to lock the root adapter to prevent i2c "noise".

I can't see recursive locks helping either, because that would need
lock ownership to be propagated into gpio/pinctrl, and that is again
too ugly.

Cheers,
Peter

> regards
> Antti
> 
>>
>> The problem with this is that the GPIO controller sits on the same i2c bus
>> that it MUXes. For pca954x devices this is worked around by using unlocked
>> transfers when updating the MUX. I have no such luck as the GPIO is a general
>> purpose IO expander and the MUX is just a random bidirectional MUX, unaware
>> of the fact that it is muxing an i2c bus, and extending unlocked transfers
>> into the GPIO subsystem is too ugly to even think about. But the general hw
>> approach is sane in my opinion, with the number of connections between the
>> two boards minimized. To put is plainly, I need support for it.
>>
>> So, I observe that while it is needed to have the i2c bus locked during the
>> actual MUX update in order to avoid random garbage on the slave side, it
>> is not strictly a must to have it locked over the whole sequence of a full
>> select-transfer-deselect operation. The MUX itself needs to be locked, so
>> transfers to clients behind the mux are serialized, and the MUX needs to be
>> stable during all i2c traffic (otherwise individual mux slave segments
>> might see garbage).
>>
>> This series accomplishes this by adding a dt property to i2c-mux-gpio and
>> i2c-mux-pinctrl that can be used to state that the mux is updated by means
>> of the muxed master bus, and that the select-transfer-deselect operations
>> should be locked individually. When this holds, the i2c bus *is* locked
>> during muxing, since the muxing happens as part of i2c transfers. This
>> is true even if the MUX is updated with several transfers to the GPIO (at
>> least as long as *all* MUX changes are using the i2s master bus). A lock
>> is added to the mux so that transfers through the mux are serialized.
>>
>> Concerns:
>> - The locking is perhaps too complex?
>> - I worry about the priority inheritance aspect of the adapter lock. When
>>the transfers behind the mux are divided into select-transfer-deselect all
>>locked individually, low priority transfers get more chances to interfere
>>with high priority transfers.

Re: [RESEND PATCH v2 0/9] eeprom: at24: at24cs series serial number read

[Bartosz Golaszewski]

2016-01-05 19:58 GMT+01:00 Wolfram Sang :
> On Mon, Jan 04, 2016 at 03:01:54PM +0100, Bartosz Golaszewski wrote:
>> 2016-01-02 21:50 GMT+01:00 Wolfram Sang :
>> > On Fri, Dec 11, 2015 at 02:55:10PM +0100, Bartosz Golaszewski wrote:
>> >> 2015-12-11 13:08 GMT+01:00 Wolfram Sang :
>> >> > On Wed, Dec 02, 2015 at 11:25:17AM +0100, Bartosz Golaszewski wrote:
>> >> >> Chips from the at24cs EEPROM series have an additional read-only 
>> >> >> memory area
>> >> >> containing a factory pre-programmed serial number. In order to access 
>> >> >> it, a
>> >> >> dummy write must be executed before reading the serial number bytes.
>> >> >
>> >> > Can't you instantiate a read-only EEPROM on this second address? Or a
>> >> > seperate driver attaching to this address? What is the advantage of
>> >> > having this in at24?
>> >> >
>> >>
>> >> The regular memory area and serial number read-only block share the
>> >> internal address pointer. We must ensure that there's no race
>> >> conditions between normal EEPROM reads/writes and serial number reads.
>> >
>> > I don't get it. Both, regular at24 reads and the serial read, setup the
>> > pointer every time by using two messages, first write to set the
>> > pointer, then read. The per-adapter lock makes sure those two messages
>> > will not get interrupted.
>>
>> If that's correct, then is there any need to have an additional mutex
>> for at24_data?
>
> I can't see a need, yes.

Then I'll see if it can be safely removed in the next iteration.

>> In that case would the preferred method be to access the regular
>> memory area like before - by allocating, for example, a 24c02 device -
>> while allocating a second device - in that case 24cs02 - on the
>> corresponding serial number address would give the user access to the
>> serial number via the eeprom sysfs attribute (which for the latter
>> would be read-only and 16 bytes in size)?
>
> Yes, a seperate driver for the second address is what I meant to suggest
> in the above paragraph. Only that the data should probably be exported
> via the NVMEM framework, not directly via sysfs. We have patches pending
> doing that for at24.

Right, but then these patches keep the driver backwards compatible in
that they keep the 'eeprom' sysfs attribute, so it's still a viable
option.

> What happens if you assign another at24 instance (read-only) to the
> second address? I mean, there is not only the serial number, but also a
> MAC address IIRC.

Nothing - it can't be read with the regular driver. Its protocol
requires certain bits set just like in the function from patch 4/9 in
this series.

As for the MAC address - I can't find anything in the datasheet, and
haven't heard about it.

> Regards,
>
>Wolfram
>

Best regards,
Bartosz Golaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 2/4] dt-bindings: i2c: mux: demux-pinctrl: add bindings

[Jan Lübbe]

Hi Wolfram,

On Mi, 2016-01-06 at 14:51 +0100, Wolfram Sang wrote:
[...]
> +Required properties:
> +- compatible: "i2c-demux-pinctrl"
> +- i2c-parent: List of phandles of I2C masters available for selection. The 
> first
> +   one will be used as default.
> +- i2c-bus-name: The name of this bus. Also needed as pinctrl-name for the I2C
> + parents.
[...]
> + i2chdmi: i2c@8 {
> + compatible = "i2c-demux-pinctrl";
> + i2c-parent = <>, <>, <>;
> + i2c-bus-name = "i2c-hdmi";
> + #address-cells = <1>;
> + #size-cells = <0>;
[...]
> + gpioi2c: i2c@9 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "i2c-gpio";
[...]
> +{
> + pinctrl-0 = <_pins>;
> + pinctrl-names = "i2c-hdmi";
> +
> + clock-frequency = <10>;
> +};
[...]
> +{
> + pinctrl-0 = <_pins>;
> + pinctrl-names = "i2c-hdmi";
> +
> + clock-frequency = <10>;
> +};
[...]

It seems that the demux-pinctrl driver reconfigures the pinctrl settings
for the parent devices. I would have expected to have alternative
pinctrl state on the demux node support switching the same external pins
between the different controllers. Wouldn't it be possible to have
pinctrl conflicts between _pins and _pins otherwise?

Regards,
Jan
-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html