| Message ID | 1403660878-56350-3-git-send-email-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi! > The non-DT support has to be maintained for now to not break > OMAP3 legacy boot, and the legacy-style code will be cleaned > up once OMAP3 is also converted to DT-boot only. > @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) > return 0; > } > > +static const struct omap_mbox_device_data omap2_data = { > + .num_users = 4, > + .num_fifos = 6, > + .intr_type = MBOX_INTR_CFG_TYPE1, > +}; > + > +static const struct omap_mbox_device_data omap3_data = { > + .num_users = 2, > + .num_fifos = 2, > + .intr_type = MBOX_INTR_CFG_TYPE1, > +}; > + > +static const struct omap_mbox_device_data am335x_data = { > + .num_users = 4, > + .num_fifos = 8, > + .intr_type = MBOX_INTR_CFG_TYPE2, > +}; So you use compatible strings to look up 3 integers. Would it be better to have num_users/num_fifos/intr_type directly in the device tree? That should be cleaner and more flexible... If you do that, would it be possible to have share compatible string? Pavel
Hi Pavel, On 06/28/2014 12:07 PM, Pavel Machek wrote: > Hi! > >> The non-DT support has to be maintained for now to not break >> OMAP3 legacy boot, and the legacy-style code will be cleaned >> up once OMAP3 is also converted to DT-boot only. > >> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) >> return 0; >> } >> >> +static const struct omap_mbox_device_data omap2_data = { >> + .num_users = 4, >> + .num_fifos = 6, >> + .intr_type = MBOX_INTR_CFG_TYPE1, >> +}; >> + >> +static const struct omap_mbox_device_data omap3_data = { >> + .num_users = 2, >> + .num_fifos = 2, >> + .intr_type = MBOX_INTR_CFG_TYPE1, >> +}; >> + >> +static const struct omap_mbox_device_data am335x_data = { >> + .num_users = 4, >> + .num_fifos = 8, >> + .intr_type = MBOX_INTR_CFG_TYPE2, >> +}; > > So you use compatible strings to look up 3 integers. Would it be better to have > num_users/num_fifos/intr_type directly in the device tree? That should be cleaner > and more flexible... > > If you do that, would it be possible to have share compatible string? Yeah, I have actually encoded the .num_users and .num_fifos in DT in the previous version [1] with shared compatible strings, but dropped those properties in favour of adding minimal custom properties to DT based on some offline IRC comments. I have no objections either way, but there is really nothing to be gained from minimizing compatible strings. Tony, Any comments on this? regards Suman [1] https://patchwork.kernel.org/patch/2839662/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! > >> The non-DT support has to be maintained for now to not break > >> OMAP3 legacy boot, and the legacy-style code will be cleaned > >> up once OMAP3 is also converted to DT-boot only. > > > >> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) > >> return 0; > >> } > >> > >> +static const struct omap_mbox_device_data omap2_data = { > >> + .num_users = 4, > >> + .num_fifos = 6, > >> + .intr_type = MBOX_INTR_CFG_TYPE1, > >> +}; > >> + > >> +static const struct omap_mbox_device_data omap3_data = { > >> + .num_users = 2, > >> + .num_fifos = 2, > >> + .intr_type = MBOX_INTR_CFG_TYPE1, > >> +}; > >> + > >> +static const struct omap_mbox_device_data am335x_data = { > >> + .num_users = 4, > >> + .num_fifos = 8, > >> + .intr_type = MBOX_INTR_CFG_TYPE2, > >> +}; > > > > So you use compatible strings to look up 3 integers. Would it be better to have > > num_users/num_fifos/intr_type directly in the device tree? That should be cleaner > > and more flexible... > > > > If you do that, would it be possible to have share compatible string? > > Yeah, I have actually encoded the .num_users and .num_fifos in DT in the > previous version [1] with shared compatible strings, but dropped those > properties in favour of adding minimal custom properties to DT based on > some offline IRC comments. I have no objections either way, but there is > really nothing to be gained from minimizing compatible strings. Actually, I'd guess best solution would be to do both: have it encoded in device tree _and_ have separate compatible string for each version (in case there are other differences). You'd still get rid of the table... Pavel
Hi Pavel, > >>>> The non-DT support has to be maintained for now to not break >>>> OMAP3 legacy boot, and the legacy-style code will be cleaned >>>> up once OMAP3 is also converted to DT-boot only. >>> >>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) >>>> return 0; >>>> } >>>> >>>> +static const struct omap_mbox_device_data omap2_data = { >>>> + .num_users = 4, >>>> + .num_fifos = 6, >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, >>>> +}; >>>> + >>>> +static const struct omap_mbox_device_data omap3_data = { >>>> + .num_users = 2, >>>> + .num_fifos = 2, >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, >>>> +}; >>>> + >>>> +static const struct omap_mbox_device_data am335x_data = { >>>> + .num_users = 4, >>>> + .num_fifos = 8, >>>> + .intr_type = MBOX_INTR_CFG_TYPE2, >>>> +}; >>> >>> So you use compatible strings to look up 3 integers. Would it be better to have >>> num_users/num_fifos/intr_type directly in the device tree? That should be cleaner >>> and more flexible... >>> >>> If you do that, would it be possible to have share compatible string? >> >> Yeah, I have actually encoded the .num_users and .num_fifos in DT in the >> previous version [1] with shared compatible strings, but dropped those >> properties in favour of adding minimal custom properties to DT based on >> some offline IRC comments. I have no objections either way, but there is >> really nothing to be gained from minimizing compatible strings. > > Actually, I'd guess best solution would be to do both: have it encoded > in device tree _and_ have separate compatible string for each version > (in case there are other differences). You'd still get rid of the > table... Do note that the .intr_type has to with the register layout rather than a physical property (mainly to distinguish the pre-OMAP4 IP register layout), so I am not convinced that belongs to DT. This is the reason why I didn't represent it in DT even in the previous version. The other two are HW IP design parameters, so in general putting them in DT isn't completely a bad idea, but I will wait to see if there are any further comments on this from Tony or DT maintainers before I make changes. regards Suman -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! > >>>> The non-DT support has to be maintained for now to not break > >>>> OMAP3 legacy boot, and the legacy-style code will be cleaned > >>>> up once OMAP3 is also converted to DT-boot only. > >>> > >>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) > >>>> return 0; > >>>> } > >>>> > >>>> +static const struct omap_mbox_device_data omap2_data = { > >>>> + .num_users = 4, > >>>> + .num_fifos = 6, > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > >>>> +}; > >>>> + > >>>> +static const struct omap_mbox_device_data omap3_data = { > >>>> + .num_users = 2, > >>>> + .num_fifos = 2, > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > >>>> +}; > >>>> + > >>>> +static const struct omap_mbox_device_data am335x_data = { > >>>> + .num_users = 4, > >>>> + .num_fifos = 8, > >>>> + .intr_type = MBOX_INTR_CFG_TYPE2, > >>>> +}; > >>> > >>> So you use compatible strings to look up 3 integers. Would it be better to have > >>> num_users/num_fifos/intr_type directly in the device tree? That should be cleaner > >>> and more flexible... > >>> > >>> If you do that, would it be possible to have share compatible string? > >> > >> Yeah, I have actually encoded the .num_users and .num_fifos in DT in the > >> previous version [1] with shared compatible strings, but dropped those > >> properties in favour of adding minimal custom properties to DT based on > >> some offline IRC comments. I have no objections either way, but there is > >> really nothing to be gained from minimizing compatible strings. > > > > Actually, I'd guess best solution would be to do both: have it encoded > > in device tree _and_ have separate compatible string for each version > > (in case there are other differences). You'd still get rid of the > > table... > > Do note that the .intr_type has to with the register layout rather than > a physical property (mainly to distinguish the pre-OMAP4 IP register > layout), so I am not convinced that belongs to DT. This is the reason > why I didn't represent it in DT even in the previous version. The > other Aha, ok, then the intr_type should be derived from compatible-string. Or rather... you should have three compatible-strings for the three possibilities? (And then subtype, currently unused, in case there are more hw differences). > two are HW IP design parameters, so in general putting them in DT isn't > completely a bad idea, but I will wait to see if there are any further > comments on this from Tony or DT maintainers before I make changes. Ok, right... I'd vote for putting them into DT. Pavel
* Pavel Machek <pavel@ucw.cz> [140630 13:34]: > Hi! > > > >>>> The non-DT support has to be maintained for now to not break > > >>>> OMAP3 legacy boot, and the legacy-style code will be cleaned > > >>>> up once OMAP3 is also converted to DT-boot only. > > >>> > > >>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) > > >>>> return 0; > > >>>> } > > >>>> > > >>>> +static const struct omap_mbox_device_data omap2_data = { > > >>>> + .num_users = 4, > > >>>> + .num_fifos = 6, > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > > >>>> +}; > > >>>> + > > >>>> +static const struct omap_mbox_device_data omap3_data = { > > >>>> + .num_users = 2, > > >>>> + .num_fifos = 2, > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > > >>>> +}; > > >>>> + > > >>>> +static const struct omap_mbox_device_data am335x_data = { > > >>>> + .num_users = 4, > > >>>> + .num_fifos = 8, > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE2, > > >>>> +}; > > >>> > > >>> So you use compatible strings to look up 3 integers. Would it be better to have > > >>> num_users/num_fifos/intr_type directly in the device tree? That should be cleaner > > >>> and more flexible... > > >>> > > >>> If you do that, would it be possible to have share compatible string? > > >> > > >> Yeah, I have actually encoded the .num_users and .num_fifos in DT in the > > >> previous version [1] with shared compatible strings, but dropped those > > >> properties in favour of adding minimal custom properties to DT based on > > >> some offline IRC comments. I have no objections either way, but there is > > >> really nothing to be gained from minimizing compatible strings. > > > > > > Actually, I'd guess best solution would be to do both: have it encoded > > > in device tree _and_ have separate compatible string for each version > > > (in case there are other differences). You'd still get rid of the > > > table... > > > > Do note that the .intr_type has to with the register layout rather than > > a physical property (mainly to distinguish the pre-OMAP4 IP register > > layout), so I am not convinced that belongs to DT. This is the reason > > why I didn't represent it in DT even in the previous version. The > > other > > Aha, ok, then the intr_type should be derived from > compatible-string. Or rather... you should have three > compatible-strings for the three possibilities? (And then subtype, > currently unused, in case there are more hw differences). The compatible string can and should be separate for each revision unless they are the same exacat hardware revision. > > two are HW IP design parameters, so in general putting them in DT isn't > > completely a bad idea, but I will wait to see if there are any further > > comments on this from Tony or DT maintainers before I make changes. > > Ok, right... I'd vote for putting them into DT. I would avoid adding custom DT properties where possible and let the driver just initialize the right data based on the compatible flag. That is as long as the amount of data built into the driver is minimal. If we wanted to have some fifo property in DT, it should be Linux generic property to avoid adding yet more custom driver specific properties. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! > > > >>>> The non-DT support has to be maintained for now to not break > > > >>>> OMAP3 legacy boot, and the legacy-style code will be cleaned > > > >>>> up once OMAP3 is also converted to DT-boot only. > > > >>> > > > >>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) > > > >>>> return 0; > > > >>>> } > > > >>>> > > > >>>> +static const struct omap_mbox_device_data omap2_data = { > > > >>>> + .num_users = 4, > > > >>>> + .num_fifos = 6, > > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > > > >>>> +}; > > > >>>> + > > > >>>> +static const struct omap_mbox_device_data omap3_data = { > > > >>>> + .num_users = 2, > > > >>>> + .num_fifos = 2, > > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > > > >>>> +}; > > > >>>> + > > > >>>> +static const struct omap_mbox_device_data am335x_data = { > > > >>>> + .num_users = 4, > > > >>>> + .num_fifos = 8, > > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE2, > > > >>>> +}; > > > >>> > > Aha, ok, then the intr_type should be derived from > > compatible-string. Or rather... you should have three > > compatible-strings for the three possibilities? (And then subtype, > > currently unused, in case there are more hw differences). > > The compatible string can and should be separate for each revision > unless they are the same exacat hardware revision. ACK. > > > two are HW IP design parameters, so in general putting them in DT isn't > > > completely a bad idea, but I will wait to see if there are any further > > > comments on this from Tony or DT maintainers before I make changes. > > > > Ok, right... I'd vote for putting them into DT. > > I would avoid adding custom DT properties where possible and let the > driver just initialize the right data based on the compatible flag. If these are HW IP design parameters, we can expect to see many different combinations. Yet we know ahead of time how to handle different parameters HW people select. Thus IMO we should do it in the device tree. Pavel
* Pavel Machek <pavel@ucw.cz> [140704 01:07]: > Hi! > > > > > >>>> The non-DT support has to be maintained for now to not break > > > > >>>> OMAP3 legacy boot, and the legacy-style code will be cleaned > > > > >>>> up once OMAP3 is also converted to DT-boot only. > > > > >>> > > > > >>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) > > > > >>>> return 0; > > > > >>>> } > > > > >>>> > > > > >>>> +static const struct omap_mbox_device_data omap2_data = { > > > > >>>> + .num_users = 4, > > > > >>>> + .num_fifos = 6, > > > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > > > > >>>> +}; > > > > >>>> + > > > > >>>> +static const struct omap_mbox_device_data omap3_data = { > > > > >>>> + .num_users = 2, > > > > >>>> + .num_fifos = 2, > > > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > > > > >>>> +}; > > > > >>>> + > > > > >>>> +static const struct omap_mbox_device_data am335x_data = { > > > > >>>> + .num_users = 4, > > > > >>>> + .num_fifos = 8, > > > > >>>> + .intr_type = MBOX_INTR_CFG_TYPE2, > > > > >>>> +}; > > > > >>> > > > > Aha, ok, then the intr_type should be derived from > > > compatible-string. Or rather... you should have three > > > compatible-strings for the three possibilities? (And then subtype, > > > currently unused, in case there are more hw differences). > > > > The compatible string can and should be separate for each revision > > unless they are the same exacat hardware revision. > > ACK. > > > > > two are HW IP design parameters, so in general putting them in DT isn't > > > > completely a bad idea, but I will wait to see if there are any further > > > > comments on this from Tony or DT maintainers before I make changes. > > > > > > Ok, right... I'd vote for putting them into DT. > > > > I would avoid adding custom DT properties where possible and let the > > driver just initialize the right data based on the compatible flag. > > If these are HW IP design parameters, we can expect to see many > different combinations. Yet we know ahead of time how to handle > different parameters HW people select. > > Thus IMO we should do it in the device tree. Oh you mean from supporting new hardware with just .dts changes? From that point of view it makes sense to have them as DT properties, so I'm fine with that. Let's just try to use something that's generic like fifosize. No idea how the property for num_fifos should be handled though as that implies some knowledge in the driver which num_users have fifos? So unless that can be described clearly in a DT property as well, the binding might be still unusable for new hardware :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tony, Pavel, On 07/04/2014 03:23 AM, Tony Lindgren wrote: > * Pavel Machek <pavel@ucw.cz> [140704 01:07]: >> Hi! >> >>>>>>>>> The non-DT support has to be maintained for now to not break >>>>>>>>> OMAP3 legacy boot, and the legacy-style code will be cleaned >>>>>>>>> up once OMAP3 is also converted to DT-boot only. >>>>>>>> >>>>>>>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) >>>>>>>>> return 0; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static const struct omap_mbox_device_data omap2_data = { >>>>>>>>> + .num_users = 4, >>>>>>>>> + .num_fifos = 6, >>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE1, >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> +static const struct omap_mbox_device_data omap3_data = { >>>>>>>>> + .num_users = 2, >>>>>>>>> + .num_fifos = 2, >>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE1, >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> +static const struct omap_mbox_device_data am335x_data = { >>>>>>>>> + .num_users = 4, >>>>>>>>> + .num_fifos = 8, >>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE2, >>>>>>>>> +}; >>>>>>>> >> >>>> Aha, ok, then the intr_type should be derived from >>>> compatible-string. Or rather... you should have three >>>> compatible-strings for the three possibilities? (And then subtype, >>>> currently unused, in case there are more hw differences). >>> >>> The compatible string can and should be separate for each revision >>> unless they are the same exacat hardware revision. >> >> ACK. I checked the revision register from all SoCs. OMAP2 and OMAP3 have different revisions compared to OMAP4+. All of OMAP4, OMAP5, DRA7, AM335x and AM437x have the same version, but with different num-fifos and num-users. So, I can switch back to using omap4-mailbox for all of these SoCs only if we encode the num-fifos and num-users in DT. >> >>>>> two are HW IP design parameters, so in general putting them in DT isn't >>>>> completely a bad idea, but I will wait to see if there are any further >>>>> comments on this from Tony or DT maintainers before I make changes. >>>> >>>> Ok, right... I'd vote for putting them into DT. >>> >>> I would avoid adding custom DT properties where possible and let the >>> driver just initialize the right data based on the compatible flag. >> >> If these are HW IP design parameters, we can expect to see many >> different combinations. Yet we know ahead of time how to handle >> different parameters HW people select. That's right, the above OMAP4+ SoCs already demonstrate this behavior. >> >> Thus IMO we should do it in the device tree. > > Oh you mean from supporting new hardware with just .dts changes? > From that point of view it makes sense to have them as DT properties, > so I'm fine with that. > > Let's just try to use something that's generic like fifosize. No idea > how the property for num_fifos should be handled though as that > implies some knowledge in the driver which num_users have fifos? The fifos are not per num_users, but rather the total number of fifos within the IP block. The num_users will be the number of targets the IP block can interrupt. I tried looking for generic properties, but there weren't any that seem to fit the description. If you want generic names, I can use num-fifos and num-users, otherwise will stick to the names defined in the previous series. > > So unless that can be described clearly in a DT property as well, > the binding might be still unusable for new hardware :) > I don't expect the OMAP mailbox IP to change much in the future. There is a FIFO depth parameter as well, but that's constant in all the current versions, and even if they change it, I can already use the generic property for that. Tony, Depending on the agreement here, I may have to respin the OMAP mailbox DT/hwmod cleanup series [1] regards Suman [1] http://marc.info/?l=linux-omap&m=140365833121612&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Suman Anna <s-anna@ti.com> [140708 10:57]: > Hi Tony, Pavel, > > On 07/04/2014 03:23 AM, Tony Lindgren wrote: > > * Pavel Machek <pavel@ucw.cz> [140704 01:07]: > >> Hi! > >> > >>>>>>>>> The non-DT support has to be maintained for now to not break > >>>>>>>>> OMAP3 legacy boot, and the legacy-style code will be cleaned > >>>>>>>>> up once OMAP3 is also converted to DT-boot only. > >>>>>>>> > >>>>>>>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) > >>>>>>>>> return 0; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> +static const struct omap_mbox_device_data omap2_data = { > >>>>>>>>> + .num_users = 4, > >>>>>>>>> + .num_fifos = 6, > >>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > >>>>>>>>> +}; > >>>>>>>>> + > >>>>>>>>> +static const struct omap_mbox_device_data omap3_data = { > >>>>>>>>> + .num_users = 2, > >>>>>>>>> + .num_fifos = 2, > >>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE1, > >>>>>>>>> +}; > >>>>>>>>> + > >>>>>>>>> +static const struct omap_mbox_device_data am335x_data = { > >>>>>>>>> + .num_users = 4, > >>>>>>>>> + .num_fifos = 8, > >>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE2, > >>>>>>>>> +}; > >>>>>>>> > >> > >>>> Aha, ok, then the intr_type should be derived from > >>>> compatible-string. Or rather... you should have three > >>>> compatible-strings for the three possibilities? (And then subtype, > >>>> currently unused, in case there are more hw differences). > >>> > >>> The compatible string can and should be separate for each revision > >>> unless they are the same exacat hardware revision. > >> > >> ACK. > > I checked the revision register from all SoCs. OMAP2 and OMAP3 have > different revisions compared to OMAP4+. All of OMAP4, OMAP5, DRA7, > AM335x and AM437x have the same version, but with different num-fifos > and num-users. So, I can switch back to using omap4-mailbox for all of > these SoCs only if we encode the num-fifos and num-users in DT. > > >> > >>>>> two are HW IP design parameters, so in general putting them in DT isn't > >>>>> completely a bad idea, but I will wait to see if there are any further > >>>>> comments on this from Tony or DT maintainers before I make changes. > >>>> > >>>> Ok, right... I'd vote for putting them into DT. > >>> > >>> I would avoid adding custom DT properties where possible and let the > >>> driver just initialize the right data based on the compatible flag. > >> > >> If these are HW IP design parameters, we can expect to see many > >> different combinations. Yet we know ahead of time how to handle > >> different parameters HW people select. > > That's right, the above OMAP4+ SoCs already demonstrate this behavior. > > >> > >> Thus IMO we should do it in the device tree. > > > > Oh you mean from supporting new hardware with just .dts changes? > > From that point of view it makes sense to have them as DT properties, > > so I'm fine with that. > > > > Let's just try to use something that's generic like fifosize. No idea > > how the property for num_fifos should be handled though as that > > implies some knowledge in the driver which num_users have fifos? > > The fifos are not per num_users, but rather the total number of fifos > within the IP block. The num_users will be the number of targets the IP > block can interrupt. I tried looking for generic properties, but there > weren't any that seem to fit the description. If you want generic names, > I can use num-fifos and num-users, otherwise will stick to the > names defined in the previous series. OK since we already have some .dts entries with ti,mbox-num-fifos and ti,mbox-num-users I'd use those for now. Adding parsing for a generic property can be done later on. > > So unless that can be described clearly in a DT property as well, > > the binding might be still unusable for new hardware :) > > > > I don't expect the OMAP mailbox IP to change much in the future. There > is a FIFO depth parameter as well, but that's constant in all the > current versions, and even if they change it, I can already use the > generic property for that. OK > Tony, > Depending on the agreement here, I may have to respin the OMAP > mailbox DT/hwmod cleanup series [1] OK let me know. Regards, Tony > [1] http://marc.info/?l=linux-omap&m=140365833121612&w=2 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2014 03:29 AM, Tony Lindgren wrote: > * Suman Anna <s-anna@ti.com> [140708 10:57]: >> Hi Tony, Pavel, >> >> On 07/04/2014 03:23 AM, Tony Lindgren wrote: >>> * Pavel Machek <pavel@ucw.cz> [140704 01:07]: >>>> Hi! >>>> >>>>>>>>>>> The non-DT support has to be maintained for now to not break >>>>>>>>>>> OMAP3 legacy boot, and the legacy-style code will be cleaned >>>>>>>>>>> up once OMAP3 is also converted to DT-boot only. >>>>>>>>>> >>>>>>>>>>> @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) >>>>>>>>>>> return 0; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> +static const struct omap_mbox_device_data omap2_data = { >>>>>>>>>>> + .num_users = 4, >>>>>>>>>>> + .num_fifos = 6, >>>>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE1, >>>>>>>>>>> +}; >>>>>>>>>>> + >>>>>>>>>>> +static const struct omap_mbox_device_data omap3_data = { >>>>>>>>>>> + .num_users = 2, >>>>>>>>>>> + .num_fifos = 2, >>>>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE1, >>>>>>>>>>> +}; >>>>>>>>>>> + >>>>>>>>>>> +static const struct omap_mbox_device_data am335x_data = { >>>>>>>>>>> + .num_users = 4, >>>>>>>>>>> + .num_fifos = 8, >>>>>>>>>>> + .intr_type = MBOX_INTR_CFG_TYPE2, >>>>>>>>>>> +}; >>>>>>>>>> >>>> >>>>>> Aha, ok, then the intr_type should be derived from >>>>>> compatible-string. Or rather... you should have three >>>>>> compatible-strings for the three possibilities? (And then subtype, >>>>>> currently unused, in case there are more hw differences). >>>>> >>>>> The compatible string can and should be separate for each revision >>>>> unless they are the same exacat hardware revision. >>>> >>>> ACK. >> >> I checked the revision register from all SoCs. OMAP2 and OMAP3 have >> different revisions compared to OMAP4+. All of OMAP4, OMAP5, DRA7, >> AM335x and AM437x have the same version, but with different num-fifos >> and num-users. So, I can switch back to using omap4-mailbox for all of >> these SoCs only if we encode the num-fifos and num-users in DT. >> >>>> >>>>>>> two are HW IP design parameters, so in general putting them in DT isn't >>>>>>> completely a bad idea, but I will wait to see if there are any further >>>>>>> comments on this from Tony or DT maintainers before I make changes. >>>>>> >>>>>> Ok, right... I'd vote for putting them into DT. >>>>> >>>>> I would avoid adding custom DT properties where possible and let the >>>>> driver just initialize the right data based on the compatible flag. >>>> >>>> If these are HW IP design parameters, we can expect to see many >>>> different combinations. Yet we know ahead of time how to handle >>>> different parameters HW people select. >> >> That's right, the above OMAP4+ SoCs already demonstrate this behavior. >> >>>> >>>> Thus IMO we should do it in the device tree. >>> >>> Oh you mean from supporting new hardware with just .dts changes? >>> From that point of view it makes sense to have them as DT properties, >>> so I'm fine with that. >>> >>> Let's just try to use something that's generic like fifosize. No idea >>> how the property for num_fifos should be handled though as that >>> implies some knowledge in the driver which num_users have fifos? >> >> The fifos are not per num_users, but rather the total number of fifos >> within the IP block. The num_users will be the number of targets the IP >> block can interrupt. I tried looking for generic properties, but there >> weren't any that seem to fit the description. If you want generic names, >> I can use num-fifos and num-users, otherwise will stick to the >> names defined in the previous series. > > OK since we already have some .dts entries with ti,mbox-num-fifos and > ti,mbox-num-users I'd use those for now. Adding parsing for a generic > property can be done later on. Alright, will stick to the existing properties. > >>> So unless that can be described clearly in a DT property as well, >>> the binding might be still unusable for new hardware :) >>> >> >> I don't expect the OMAP mailbox IP to change much in the future. There >> is a FIFO depth parameter as well, but that's constant in all the >> current versions, and even if they change it, I can already use the >> generic property for that. > > OK > >> Tony, >> Depending on the agreement here, I may have to respin the OMAP >> mailbox DT/hwmod cleanup series [1] > > OK let me know. I will refresh both this series and the mailbox DT/hwmod cleanup with these changes, and post the patches tomorrow. regards Suman -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c index a27e00e..c61435b 100644 --- a/drivers/mailbox/omap-mailbox.c +++ b/drivers/mailbox/omap-mailbox.c @@ -31,6 +31,7 @@ #include <linux/err.h> #include <linux/notifier.h> #include <linux/module.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/platform_data/mailbox-omap.h> @@ -94,6 +95,24 @@ struct omap_mbox_device { struct list_head elem; }; +struct omap_mbox_fifo_info { + int tx_id; + int tx_usr; + int tx_irq; + + int rx_id; + int rx_usr; + int rx_irq; + + const char *name; +}; + +struct omap_mbox_device_data { + u32 num_users; + u32 num_fifos; + u32 intr_type; +}; + struct omap_mbox { const char *name; int irq; @@ -587,24 +606,157 @@ static int omap_mbox_unregister(struct omap_mbox_device *mdev) return 0; } +static const struct omap_mbox_device_data omap2_data = { + .num_users = 4, + .num_fifos = 6, + .intr_type = MBOX_INTR_CFG_TYPE1, +}; + +static const struct omap_mbox_device_data omap3_data = { + .num_users = 2, + .num_fifos = 2, + .intr_type = MBOX_INTR_CFG_TYPE1, +}; + +static const struct omap_mbox_device_data am335x_data = { + .num_users = 4, + .num_fifos = 8, + .intr_type = MBOX_INTR_CFG_TYPE2, +}; + +static const struct omap_mbox_device_data omap4_data = { + .num_users = 3, + .num_fifos = 8, + .intr_type = MBOX_INTR_CFG_TYPE2, +}; + +static const struct omap_mbox_device_data dra7_data = { + .num_users = 4, + .num_fifos = 12, + .intr_type = MBOX_INTR_CFG_TYPE2, +}; + +static const struct of_device_id omap_mailbox_of_match[] = { + { + .compatible = "ti,omap2-mailbox", + .data = &omap2_data, + }, + { + .compatible = "ti,omap3-mailbox", + .data = &omap3_data, + }, + { + .compatible = "ti,am3352-mailbox", + .data = &am335x_data, + }, + { + .compatible = "ti,omap4-mailbox", + .data = &omap4_data, + }, + { + .compatible = "ti,am4372-mailbox", + .data = &am335x_data, + }, + { + .compatible = "ti,dra7-mailbox", + .data = &dra7_data, + }, + { + /* end */ + }, +}; +MODULE_DEVICE_TABLE(of, omap_mailbox_of_match); + static int omap_mbox_probe(struct platform_device *pdev) { struct resource *mem; int ret; struct omap_mbox **list, *mbox, *mboxblk; struct omap_mbox_pdata *pdata = pdev->dev.platform_data; - struct omap_mbox_dev_info *info; + struct omap_mbox_dev_info *info = NULL; + struct omap_mbox_device_data *data; + struct omap_mbox_fifo_info *finfo, *finfoblk; struct omap_mbox_device *mdev; struct omap_mbox_fifo *fifo; - u32 intr_type; + struct device_node *node = pdev->dev.of_node; + struct device_node *child; + const struct of_device_id *match; + u32 intr_type, info_count; + u32 num_users, num_fifos; + u32 tmp[3]; u32 l; int i; - if (!pdata || !pdata->info_cnt || !pdata->info) { + if (!node && (!pdata || !pdata->info_cnt || !pdata->info)) { pr_err("%s: platform not supported\n", __func__); return -ENODEV; } + if (node) { + match = of_match_device(omap_mailbox_of_match, &pdev->dev); + if (!match) + return -ENODEV; + data = (struct omap_mbox_device_data *)match->data; + + info_count = of_get_available_child_count(node); + if (!info_count) { + dev_err(&pdev->dev, "no available mbox devices found\n"); + return -ENODEV; + } + + intr_type = data->intr_type; + num_users = data->num_users; + num_fifos = data->num_fifos; + } else { /* non-DT device creation */ + info_count = pdata->info_cnt; + info = pdata->info; + intr_type = pdata->intr_type; + num_users = pdata->num_users; + num_fifos = pdata->num_fifos; + } + + finfoblk = devm_kzalloc(&pdev->dev, info_count * sizeof(*finfoblk), + GFP_KERNEL); + if (!finfoblk) + return -ENOMEM; + + finfo = finfoblk; + child = NULL; + for (i = 0; i < info_count; i++, finfo++) { + if (!node) { + finfo->tx_id = info->tx_id; + finfo->rx_id = info->rx_id; + finfo->tx_usr = info->usr_id; + finfo->tx_irq = info->irq_id; + finfo->rx_usr = info->usr_id; + finfo->rx_irq = info->irq_id; + finfo->name = info->name; + info++; + } else { + child = of_get_next_available_child(node, child); + ret = of_property_read_u32_array(child, "ti,mbox-tx", + tmp, ARRAY_SIZE(tmp)); + if (ret) + return ret; + finfo->tx_id = tmp[0]; + finfo->tx_irq = tmp[1]; + finfo->tx_usr = tmp[2]; + + ret = of_property_read_u32_array(child, "ti,mbox-rx", + tmp, ARRAY_SIZE(tmp)); + if (ret) + return ret; + finfo->rx_id = tmp[0]; + finfo->rx_irq = tmp[1]; + finfo->rx_usr = tmp[2]; + + finfo->name = child->name; + } + if (finfo->tx_id >= num_fifos || finfo->rx_id >= num_fifos || + finfo->tx_usr >= num_users || finfo->rx_usr >= num_users) + return -EINVAL; + } + mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL); if (!mdev) return -ENOMEM; @@ -615,41 +767,40 @@ static int omap_mbox_probe(struct platform_device *pdev) return PTR_ERR(mdev->mbox_base); /* allocate one extra for marking end of list */ - list = devm_kzalloc(&pdev->dev, (pdata->info_cnt + 1) * sizeof(*list), + list = devm_kzalloc(&pdev->dev, (info_count + 1) * sizeof(*list), GFP_KERNEL); if (!list) return -ENOMEM; - mboxblk = devm_kzalloc(&pdev->dev, pdata->info_cnt * sizeof(*mbox), + mboxblk = devm_kzalloc(&pdev->dev, info_count * sizeof(*mbox), GFP_KERNEL); if (!mboxblk) return -ENOMEM; - info = pdata->info; - intr_type = pdata->intr_type; mbox = mboxblk; - for (i = 0; i < pdata->info_cnt; i++, info++) { + finfo = finfoblk; + for (i = 0; i < info_count; i++, finfo++) { fifo = &mbox->tx_fifo; - fifo->msg = MAILBOX_MESSAGE(info->tx_id); - fifo->fifo_stat = MAILBOX_FIFOSTATUS(info->tx_id); - fifo->intr_bit = MAILBOX_IRQ_NOTFULL(info->tx_id); - fifo->irqenable = MAILBOX_IRQENABLE(intr_type, info->usr_id); - fifo->irqstatus = MAILBOX_IRQSTATUS(intr_type, info->usr_id); - fifo->irqdisable = MAILBOX_IRQDISABLE(intr_type, info->usr_id); + fifo->msg = MAILBOX_MESSAGE(finfo->tx_id); + fifo->fifo_stat = MAILBOX_FIFOSTATUS(finfo->tx_id); + fifo->intr_bit = MAILBOX_IRQ_NOTFULL(finfo->tx_id); + fifo->irqenable = MAILBOX_IRQENABLE(intr_type, finfo->tx_usr); + fifo->irqstatus = MAILBOX_IRQSTATUS(intr_type, finfo->tx_usr); + fifo->irqdisable = MAILBOX_IRQDISABLE(intr_type, finfo->tx_usr); fifo = &mbox->rx_fifo; - fifo->msg = MAILBOX_MESSAGE(info->rx_id); - fifo->msg_stat = MAILBOX_MSGSTATUS(info->rx_id); - fifo->intr_bit = MAILBOX_IRQ_NEWMSG(info->rx_id); - fifo->irqenable = MAILBOX_IRQENABLE(intr_type, info->usr_id); - fifo->irqstatus = MAILBOX_IRQSTATUS(intr_type, info->usr_id); - fifo->irqdisable = MAILBOX_IRQDISABLE(intr_type, info->usr_id); + fifo->msg = MAILBOX_MESSAGE(finfo->rx_id); + fifo->msg_stat = MAILBOX_MSGSTATUS(finfo->rx_id); + fifo->intr_bit = MAILBOX_IRQ_NEWMSG(finfo->rx_id); + fifo->irqenable = MAILBOX_IRQENABLE(intr_type, finfo->rx_usr); + fifo->irqstatus = MAILBOX_IRQSTATUS(intr_type, finfo->rx_usr); + fifo->irqdisable = MAILBOX_IRQDISABLE(intr_type, finfo->rx_usr); mbox->intr_type = intr_type; mbox->parent = mdev; - mbox->name = info->name; - mbox->irq = platform_get_irq(pdev, info->irq_id); + mbox->name = finfo->name; + mbox->irq = platform_get_irq(pdev, finfo->tx_irq); if (mbox->irq < 0) return mbox->irq; list[i] = mbox++; @@ -657,8 +808,8 @@ static int omap_mbox_probe(struct platform_device *pdev) mutex_init(&mdev->cfg_lock); mdev->dev = &pdev->dev; - mdev->num_users = pdata->num_users; - mdev->num_fifos = pdata->num_fifos; + mdev->num_users = num_users; + mdev->num_fifos = num_fifos; mdev->mboxes = list; ret = omap_mbox_register(mdev); if (ret) @@ -684,6 +835,7 @@ static int omap_mbox_probe(struct platform_device *pdev) if (ret < 0) goto unregister; + devm_kfree(&pdev->dev, finfoblk); return 0; unregister: @@ -708,6 +860,7 @@ static struct platform_driver omap_mbox_driver = { .driver = { .name = "omap-mailbox", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(omap_mailbox_of_match), }, };
Logic has been added to the OMAP2+ mailbox code to parse the mailbox dt nodes and construct the different sub-mailboxes associated with the instance. The DT representation of the sub-mailbox devices is different from legacy platform data representation to allow flexibility of interrupt configuration between Tx and Rx fifos (to also possibly allow simplex devices in the future). The DT representation gathers similar information that was being passed previously through the platform data, except for the number of fifos, interrupts and interrupt type information, which are gathered through driver compatible match data. The non-DT support has to be maintained for now to not break OMAP3 legacy boot, and the legacy-style code will be cleaned up once OMAP3 is also converted to DT-boot only. Cc: Jassi Brar <jassisinghbrar@gmail.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Suman Anna <s-anna@ti.com> --- drivers/mailbox/omap-mailbox.c | 201 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 177 insertions(+), 24 deletions(-)